On Mon, Mar 19, 2018 at 01:59:24PM +0100, Martin Wilck wrote:
> On Fri, 2018-03-16 at 16:31 -0500, Benjamin Marzinski wrote:
> > diff --git a/multipathd/waiter.c b/multipathd/waiter.c
> > index cb9708b..e894294 100644
> > --- a/multipathd/waiter.c
> > +++ b/multipathd/waiter.c
> > @@ -23,6 +23,7 @@
> > #include "waiter.h"
> >
> > pthread_attr_t waiter_attr;
> > +struct mutex_lock waiter_lock = { .mutex = PTHREAD_MUTEX_INITIALIZER
> > };
> >
> > static struct event_thread *alloc_waiter (void)
> > {
> > @@ -59,8 +60,12 @@ void stop_waiter_thread (struct multipath *mpp,
> > struct vectors *vecs)
> > mpp->waiter);
> > thread = mpp->waiter;
> > mpp->waiter = (pthread_t)0;
> > - pthread_cancel(thread);
> > + pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > + lock(&waiter_lock);
> > + pthread_testcancel();
>
> What's the purpose of this pthread_testcancel() call?
>
It's not necessary. I feel like in general, after you get done waiting
on a lock, it's good form to see if you've been cancelled while you were
waiting. However, it's really unlikely that a thread will be waiting
here long, and this pthread_cancel isn't protecting it form accessing
any freed data. If you think it's likely to confuse people, I have no
problem with removing it.
> > pthread_kill(thread, SIGUSR2);
> > + pthread_cancel(thread);
> > + lock_cleanup_pop(&waiter_lock);
> > }
> >
> > /*
> > @@ -114,8 +119,13 @@ static int waiteventloop (struct event_thread
> > *waiter)
> > dm_task_destroy(waiter->dmt);
> > waiter->dmt = NULL;
> >
> > - if (!r) /* wait interrupted by signal */
> > - return -1;
> > + if (!r) { /* wait interrupted by signal. check for
> > cancellation */
> > + pthread_cleanup_push(cleanup_lock, &waiter_lock);
> > + lock(&waiter_lock);
> > + pthread_testcancel();
> > + lock_cleanup_pop(&waiter_lock);
>
> Nitpick: I'd prefer pthread_cleanup_pop(1) here.
I have no problem with changing that.
-Ben
> Regards,
> Martin
>
>
> > + return 1; /* If we weren't cancelled, just
> > reschedule */
> > + }
> >
> > waiter->event_nr++;
> >
>
> --
> 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)
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/dm-devel
