| Message ID | 1665525183-27377-1-git-send-email-bmarzins@redhat.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | remove dangerous cleanup __attribute__ uses | expand |
On Tue, 2022-10-11 at 16:52 -0500, Benjamin Marzinski wrote: > the cleanup __attribute__ is only run when a variable goes out of > scope > normally. It is not run on pthread cancellation. This means that > multipathd could leak whatever resources were supposed to be cleaned > up > if the thread was cancelled in a function using variables with the > cleanup __attribute__. This patchset removes all these uses in cases > where the code is run by multipathd and includes a cancellation point > in the variables scope (usually condlog(), which calls fprintf(), a > cancellation point, the way multipathd is usually run). I have to say I don't like this. Cleaning up resources is certainly very important, but in most of cases it's only about freeing memory on exit: memory which is going to be freed by the system anyway when multipathd terminates. Resource cleanup matters much more during runtime than on exit. The only threads that are cancelled during multipathd runtime are the TUR threads. It's nice to have valgrind show zero leaks when we kill multipathd out if the sudden. But we should weigh this against the side effects it has, which is ugly, hard-to-maintain code. pthread_cleanup_push()/pop() calls contribute a lot to the bad readability and maintainability of much of the multipath code base, not to mention the weird errors some gcc versions throw for pthread_cleanup() constructs. I'd rather try to get rid of as much of them as we can. I know it won't be possible everywhere, because some resources (like file descriptors) really need to be cleaned up, but I am really unsure whether we need pthread cleanup for every free() operation. __attribute__((cleanup())), on the contrary, goes a long way to make code more readable and easier to review. It actually helps a lot to ensure resources are properly cleaned up, considering how easily a "goto cleanup;" statement may be forgotten. Replacing it by pthread_cleanup() and goto statements is undesirable, IMO. If your concern is really condlog(), let's discuss if we can find a way to convert this such that it is no cancellation point any more. I can imagine converting the locking in log_safe() from a mutex into some lockless scheme, using atomic variables, and using the log thread not only for syslog, but also for the fprintf() case. Regards Martin > > Benjamin Marzinski (4): > libmultipath: don't print garbage keywords > libmultipath: avoid STRBUF_ON_STACK with cancellation points > libmultipath: use regular array for field widths > libmultipath: avoid cleanup __attribute__ with cancellation points > > libmpathutil/parser.c | 13 ++-- > libmpathutil/strbuf.h | 4 +- > libmultipath/alias.c | 59 ++++++++++------- > libmultipath/blacklist.c | 4 +- > libmultipath/configure.c | 6 +- > libmultipath/discovery.c | 34 ++++++---- > libmultipath/dmparser.c | 23 +++---- > libmultipath/foreign.c | 9 +-- > libmultipath/generic.c | 14 ++-- > libmultipath/libmultipath.version | 4 +- > libmultipath/print.c | 82 ++++++++++++++-------- > -- > libmultipath/print.h | 4 +- > libmultipath/prioritizers/weightedpath.c | 22 ++++--- > libmultipath/propsel.c | 76 ++++++++++++++++------ > libmultipath/sysfs.h | 11 +--- > libmultipath/uevent.c | 6 +- > multipath/main.c | 5 +- > multipathd/cli_handlers.c | 52 +++++++-------- > multipathd/main.c | 49 ++++++++------ > 19 files changed, 286 insertions(+), 191 deletions(-) > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel
On Thu, Oct 20, 2022 at 04:03:46PM +0000, Martin Wilck wrote: > On Tue, 2022-10-11 at 16:52 -0500, Benjamin Marzinski wrote: > > the cleanup __attribute__ is only run when a variable goes out of > > scope > > normally. It is not run on pthread cancellation. This means that > > multipathd could leak whatever resources were supposed to be cleaned > > up > > if the thread was cancelled in a function using variables with the > > cleanup __attribute__. This patchset removes all these uses in cases > > where the code is run by multipathd and includes a cancellation point > > in the variables scope (usually condlog(), which calls fprintf(), a > > cancellation point, the way multipathd is usually run). > > I have to say I don't like this. > > Cleaning up resources is certainly very important, but in most of > cases it's only about freeing memory on exit: memory which is going to > be freed by the system anyway when multipathd terminates. Resource > cleanup matters much more during runtime than on exit. The only threads > that are cancelled during multipathd runtime are the TUR threads. > It's nice to have valgrind show zero leaks when we kill multipathd out > if the sudden. But we should weigh this against the side effects it > has, which is ugly, hard-to-maintain code. > > pthread_cleanup_push()/pop() calls contribute a lot to the bad > readability and maintainability of much of the multipath code base, not > to mention the weird errors some gcc versions throw for > pthread_cleanup() constructs. I'd rather try to get rid of as much of > them as we can. I know it won't be possible everywhere, because some > resources (like file descriptors) really need to be cleaned up, but I > am really unsure whether we need pthread cleanup for every free() > operation. > > __attribute__((cleanup())), on the contrary, goes a long way to make > code more readable and easier to review. It actually helps a lot to > ensure resources are properly cleaned up, considering how easily a > "goto cleanup;" statement may be forgotten. Replacing it by > pthread_cleanup() and goto statements is undesirable, IMO. > > If your concern is really condlog(), let's discuss if we can find a way > to convert this such that it is no cancellation point any more. I can > imagine converting the locking in log_safe() from a mutex into some > lockless scheme, using atomic variables, and using the log thread not > only for syslog, but also for the fprintf() case. Actually, I've never been a huge fan of all the work that we do to keep multipathd from leaking memory when it's killed, but I though that was the goal. If you don't care about these leaks, then I'm fine with ignoring them. How about just taking the first and third patches? libmultipath: don't print garbage keywords libmultipath: use regular array for field widths -Ben > Regards > Martin > > > > > > Benjamin Marzinski (4): > > libmultipath: don't print garbage keywords > > libmultipath: avoid STRBUF_ON_STACK with cancellation points > > libmultipath: use regular array for field widths > > libmultipath: avoid cleanup __attribute__ with cancellation points > > > > libmpathutil/parser.c | 13 ++-- > > libmpathutil/strbuf.h | 4 +- > > libmultipath/alias.c | 59 ++++++++++------- > > libmultipath/blacklist.c | 4 +- > > libmultipath/configure.c | 6 +- > > libmultipath/discovery.c | 34 ++++++---- > > libmultipath/dmparser.c | 23 +++---- > > libmultipath/foreign.c | 9 +-- > > libmultipath/generic.c | 14 ++-- > > libmultipath/libmultipath.version | 4 +- > > libmultipath/print.c | 82 ++++++++++++++-------- > > -- > > libmultipath/print.h | 4 +- > > libmultipath/prioritizers/weightedpath.c | 22 ++++--- > > libmultipath/propsel.c | 76 ++++++++++++++++------ > > libmultipath/sysfs.h | 11 +--- > > libmultipath/uevent.c | 6 +- > > multipath/main.c | 5 +- > > multipathd/cli_handlers.c | 52 +++++++-------- > > multipathd/main.c | 49 ++++++++------ > > 19 files changed, 286 insertions(+), 191 deletions(-) > > -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel