| Message ID | 20221006184228.3281392-1-edliaw@google.com |
|---|---|
| Headers | show |
| Series | mmapstress01: refactor to ltp framework | expand |
Hi! Looks mostly done now, but there are still a few things to fix, some of them purely cosmetic: * The finished global has to be volatile otherwise it may get miscompiled and it has to be reset at the start for the run() otherwise the -i option is broken * The sighandler setup can be moved to the test setup() * Now we do not use the pidarray to kill the child processes so it became mostly useless, I would consider getting rid of that too * I'm failing to see why we need to block SIGALRM just before alarm(0) as far as I can tell it's completely useless. Also blocking signals while we refork looks questionable. * I would consider getting rid of obvious comments, quite a few of them are commenting the obvious The ifdefs in setup() does not look nice, maybe we should do something as: #if _FILE_OFFSET_BITS == 64 # define FSIZE_MIN LONG_MIN # define FSIZE_MAX LONG_MAX #else # define FSIZE_MIN INT_MIN # define FSIZE_MAX INT_MAX #endif And use the FSIZE_MIN and FSIZE_MAX to avoid ifdefs in the test setup() * The top level comment should be converted to docparse comment so that it's picked up by the documentation parser, that means that the comment should start with: /*\ * [Description] * And should be formatted in asciidoc
Hi Cyril, I sent a v5 with changes based on your comments. > * I'm failing to see why we need to block SIGALRM just before alarm(0) > as far as I can tell it's completely useless. Also blocking signals > while we refork looks questionable. > Is the reason it's blocking signals while reforking is due to the second choice in https://man7.org/linux/man-pages/man7/signal-safety.7.html? I'm not familiar with signal handling. ``` Block signal delivery in the main program when calling functions that are unsafe or operating on global data that is also accessed by the signal handler. ``` Thanks, Edward
Hi! > > * I'm failing to see why we need to block SIGALRM just before alarm(0) > > as far as I can tell it's completely useless. Also blocking signals > > while we refork looks questionable. > > > > Is the reason it's blocking signals while reforking is due to the second > choice in https://man7.org/linux/man-pages/man7/signal-safety.7.html? I'm > not familiar with signal handling. > ``` > Block signal delivery in the main program when calling > functions that are unsafe or operating on global data that is > also accessed by the signal handler. > ``` I do not think that this is a valid reason. What that paragraph is trying to tell is that you either have to avoid calling anything that modifies global state and may cause trouble or block signals in sections that do the same but in the normal program context. Either way that effectively makes sure that such code is never run concurently. For example calling malloc()/free() modifies locks and data structures in libc, it's not safe to be called from signal handler and can easily cause deadlocks and corruptions. Technically you can avoid that either by not calling malloc()/free() from a signal handler, or by disabling signal handlers before you call malloc()/free() or anything that may possibly call malloc()/free() in the rest of the program. In this case the signal handler is as simple as it gets, it only sets a global flag that is checked in the while () loop that reforks. And the only important thing we do there is the fork(). However fork() cannot be interrupted by a signal and return EINTR so it cannot even fail. It would probably made some sense if we wanted to setup different signal handlers in the child and avoid getting signals before we do so, but that's not the case here either. So either this is a leftover code that was there to protect something that has been removed long in the past, or copy&pasted code from a different test that acutally made use of that. At least I do not see any other reason to have it there.
> > I do not think that this is a valid reason. What that paragraph is > trying to tell is that you either have to avoid calling anything that > modifies global state and may cause trouble or block signals in sections > that do the same but in the normal program context. Either way that > effectively makes sure that such code is never run concurently. > > For example calling malloc()/free() modifies locks and data structures > in libc, it's not safe to be called from signal handler and can easily > cause deadlocks and corruptions. Technically you can avoid that either > by not calling malloc()/free() from a signal handler, or by disabling > signal handlers before you call malloc()/free() or anything that may > possibly call malloc()/free() in the rest of the program. > > In this case the signal handler is as simple as it gets, it only sets a > global flag that is checked in the while () loop that reforks. And the > only important thing we do there is the fork(). However fork() cannot be > interrupted by a signal and return EINTR so it cannot even fail. It > would probably made some sense if we wanted to setup different signal > handlers in the child and avoid getting signals before we do so, but > that's not the case here either. > > So either this is a leftover code that was there to protect something > that has been removed long in the past, or copy&pasted code from a > different test that acutally made use of that. At least I do not see any > other reason to have it there. > > -- > Cyril Hrubis > chrubis@suse.cz Thanks for the explanation! That makes sense. Thanks, Edward
v3->v4: * squash previous changes * set defaults of nprocs=20 and max_runtime=12 * clean up comments * return TBROK instead of TFAIL for unexpected failures v2->v3: * apply make check lint suggestions * refactor cleanup * use SAFE_FORK * update license v1->v2: * clean up formatting * remove accidental change to header comment * use SAFE_MMAP v0->v1: * use tst_test framework * use FILE_OFFSET_BITS=64 instead of LARGE_FILE * use safe macros Edward Liaw (4): mmapstress01: refactor to tst_test framework mmapstress01: default nprocs 20 and cleanup mmapstress01: return TBROK for unexpected failures and TERRNO if syscall fails mmapstress01: use max_runtime testcases/kernel/mem/mmapstress/Makefile | 2 + .../kernel/mem/mmapstress/mmapstress01.c | 864 ++++++------------ 2 files changed, 276 insertions(+), 590 deletions(-)