On Fri, 2018-02-09 at 17:04 -0600, Benjamin Marzinski wrote: > On Fri, Feb 09, 2018 at 09:30:56PM +0100, Martin Wilck wrote: > > On Thu, 2018-02-08 at 17:56 -0600, Benjamin Marzinski wrote: > > > ct->running is now an atomic variable. When the thread is > > > started > > > it is set to 1. When the checker wants to kill a thread, it > > > atomically > > > sets the value to 0 and reads the previous value. If it was 1, > > > the checker cancels the thread. If it was 0, the nothing needs to > > > be > > > done. After the checker has dealt with the thread, it sets ct- > > > > thread > > > > > > to NULL. > > > > > > When the thread is done, it atomicalllys sets the value of ct- > > > > running > > > > > > to 0 and reads the previous value. If it was 1, the thread just > > > exits. > > > If it was 0, then the checker is trying to cancel the thread, and > > > so > > > the thread calls pause(), which is a cancellation point. > > > > > > > I'm missing one thing here. My poor brain is aching. > > > > cleanup_func() can be entered in two ways: a) if the thread has > > been > > cancelled and passed a cancellation point already, or b) if it > > exits > > normally and calls pthread_cleanup_pop(). > > In case b), waiting for the cancellation request by calling pause() > > makes sense to me. But in case a), the thread has already seen the > > cancellation request - wouldn't calling pause() cause it to sleep > > forever? > > Urgh. You're right. If it is in the cleanup helper because it already > has been cancelled, then the pause isn't going get cancelled. So much > for my quick rewrite. Maybe it's easier than we thought. Attached is a patch on top of yours that I think might work, please have a look. It's quite late here, so I'll need to ponder your alternatives below the other day. Cheers Martin > > That leaves three options. > > 1. have either the thread or the checker detach the thread (depending > on which one exits first) > 2. make the checker always cancel and detach the thread. This > simplifies > the code, but there will zombie threads hanging around between > calls > to the checker. > 3. just move the condlog > > I really don't care which one we pick anymore. > > -Ben > > > > > Martin > > > > -- > > Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107 > > SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham > > Norton > > HRB 21284 (AG Nürnberg) > > -- Dr. Martin Wilck <mwilck@xxxxxxxx>, Tel. +49 (0)911 74053 2107 SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton HRB 21284 (AG Nürnberg)
From 831ef27b41858fa248201b74f2dd8ea5b7c4aece Mon Sep 17 00:00:00 2001 From: Martin Wilck <mwilck@xxxxxxxx> Date: Sat, 10 Feb 2018 00:22:17 +0100 Subject: [PATCH] tur checker: make sure pthread_cancel isn't called for exited thread If we enter the cleanup function as the result of a pthread_cancel by another thread, we don't need to wait for a cancellation any more. If we exit regularly, just tell the other thread not to try to cancel us. --- libmultipath/checkers/tur.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/libmultipath/checkers/tur.c b/libmultipath/checkers/tur.c index 894ad41c89c3..31a87d2b5cf2 100644 --- a/libmultipath/checkers/tur.c +++ b/libmultipath/checkers/tur.c @@ -221,8 +221,6 @@ static void cleanup_func(void *data) holders = uatomic_sub_return(&ct->holders, 1); if (!holders) cleanup_context(ct); - if (!running) - pause(); } static int tur_running(struct tur_checker_context *ct) @@ -266,6 +264,9 @@ static void *tur_thread(void *ctx) pthread_cond_signal(&ct->active); pthread_mutex_unlock(&ct->lock); + /* Tell main checker thread not to cancel us, as we exit anyway */ + running = uatomic_xchg(&ct->running, 0); + condlog(3, "%s: tur checker finished, state %s", tur_devt(devt, sizeof(devt), ct), checker_state_name(state)); tur_thread_cleanup_pop(ct); -- 2.16.1
-- dm-devel mailing list dm-devel@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/dm-devel
