From mboxrd@z Thu Jan 1 00:00:00 1970 From: Dragan Stancevic Subject: Re: [PATCH 6/6] multipathd: Remove a busy-waiting loop Date: Wed, 17 Aug 2016 14:36:09 -0500 Message-ID: References: <31d43b64-36c8-1a24-a849-230b5cf6323c@sandisk.com> <14804b8b-51a7-e860-91d7-9b594aeed63c@sandisk.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============8021650317399159655==" Return-path: In-Reply-To: <14804b8b-51a7-e860-91d7-9b594aeed63c@sandisk.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com To: Bart Van Assche Cc: device-mapper development List-Id: dm-devel.ids --===============8021650317399159655== Content-Type: multipart/alternative; boundary=001a113f37ccc8de5a053a4993b7 --001a113f37ccc8de5a053a4993b7 Content-Type: text/plain; charset=UTF-8 Hi Bart- Acked-by: dragan.stancevic@canonical.com I agree with your patch, I have been tracking an issue where multipathd dumps core on the exit path just past the treads being canceled. Your patch is very similar to mine (minus nuking the depth) that I was going to send out to a user to test with. The checker thread accesses a valid pointer with garbage values.... On Mon, Aug 15, 2016 at 10:28 AM, Bart Van Assche < bart.vanassche@sandisk.com> wrote: > Use pthread_join() to wait until worker threads have finished > instead of using a counter to keep track of how many threads are > trying to grab a mutex. Remove mutex_lock.depth since this member > variable is no longer needed. > > This patch fixes two race conditions: > * Incrementing "depth" in lock() without holding a mutex. > * Destroying a mutex from the main thread without waiting for the > worker threads to finish using that mutex. > > Signed-off-by: Bart Van Assche > --- > libmultipath/lock.h | 15 +-------------- > multipathd/main.c | 13 ++++++------- > 2 files changed, 7 insertions(+), 21 deletions(-) > > diff --git a/libmultipath/lock.h b/libmultipath/lock.h > index 9808480..a170efe 100644 > --- a/libmultipath/lock.h > +++ b/libmultipath/lock.h > @@ -3,35 +3,22 @@ > > #include > > -/* > - * Wrapper for the mutex. Includes a ref-count to keep > - * track of how many there are out-standing threads blocking > - * on a mutex. */ > struct mutex_lock { > pthread_mutex_t mutex; > - int depth; > }; > > static inline void lock(struct mutex_lock *a) > { > - a->depth++; > pthread_mutex_lock(&a->mutex); > } > > static inline int timedlock(struct mutex_lock *a, struct timespec *tmo) > { > - int r; > - > - a->depth++; > - r = pthread_mutex_timedlock(&a->mutex, tmo); > - if (r) > - a->depth--; > - return r; > + return pthread_mutex_timedlock(&a->mutex, tmo); > } > > static inline void unlock(struct mutex_lock *a) > { > - a->depth--; > pthread_mutex_unlock(&a->mutex); > } > > diff --git a/multipathd/main.c b/multipathd/main.c > index fff482c..c288195 100644 > --- a/multipathd/main.c > +++ b/multipathd/main.c > @@ -2046,7 +2046,6 @@ init_vecs (void) > return NULL; > > pthread_mutex_init(&vecs->lock.mutex, NULL); > - vecs->lock.depth = 0; > > return vecs; > } > @@ -2394,16 +2393,16 @@ child (void * param) > pthread_cancel(uxlsnr_thr); > pthread_cancel(uevq_thr); > > + pthread_join(check_thr, NULL); > + pthread_join(uevent_thr, NULL); > + pthread_join(uxlsnr_thr, NULL); > + pthread_join(uevq_thr, NULL); > + > lock(&vecs->lock); > free_pathvec(vecs->pathvec, FREE_PATHS); > vecs->pathvec = NULL; > unlock(&vecs->lock); > - /* Now all the waitevent threads will start rushing in. */ > - while (vecs->lock.depth > 0) { > - sleep (1); /* This is weak. */ > - condlog(3, "Have %d wait event checkers threads to > de-alloc," > - " waiting...", vecs->lock.depth); > - } > + > pthread_mutex_destroy(&vecs->lock.mutex); > FREE(vecs); > vecs = NULL; > -- > 2.9.2 > > -- > dm-devel mailing list > dm-devel@redhat.com > https://www.redhat.com/mailman/listinfo/dm-devel > --001a113f37ccc8de5a053a4993b7 Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: quoted-printable
Hi Bart-

<= div>
I agree with your patch, I have been tracking an issue w= here multipathd dumps core on the exit path just past the treads being canc= eled. Your patch is very similar to mine (minus nuking the depth) that I wa= s going to send out to a user to test with. The checker thread accesses a v= alid pointer with garbage values....
=
On Mon, Aug 15, 2016 at 10:28 AM, Bart Van A= ssche <bart.vanassche@sandisk.com> wrote:
Use pthread_join() to wait until worker threads ha= ve finished
instead of using a counter to keep track of how many threads are
trying to grab a mutex. Remove mutex_lock.depth since this member
variable is no longer needed.

This patch fixes two race conditions:
* Incrementing "depth" in lock() without holding a mutex.
* Destroying a mutex from the main thread without waiting for the
=C2=A0 worker threads to finish using that mutex.

Signed-off-by: Bart Van Assche <bart.vanassche@sandisk.com>
---
=C2=A0libmultipath/lock.h | 15 +--------------
=C2=A0multipathd/main.c=C2=A0 =C2=A0| 13 ++++++-------
=C2=A02 files changed, 7 insertions(+), 21 deletions(-)

diff --git a/libmultipath/lock.h b/libmultipath/lock.h
index 9808480..a170efe 100644
--- a/libmultipath/lock.h
+++ b/libmultipath/lock.h
@@ -3,35 +3,22 @@

=C2=A0#include <pthread.h>

-/*
- * Wrapper for the mutex. Includes a ref-count to keep
- * track of how many there are out-standing threads blocking
- * on a mutex. */
=C2=A0struct mutex_lock {
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_mutex_t mutex;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0int depth;
=C2=A0};

=C2=A0static inline void lock(struct mutex_lock *a)
=C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0a->depth++;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_mutex_lock(&a->mutex);
=C2=A0}

=C2=A0static inline int timedlock(struct mutex_lock *a, struct timespec *tm= o)
=C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0int r;
-
-=C2=A0 =C2=A0 =C2=A0 =C2=A0a->depth++;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0r =3D pthread_mutex_timedlock(&a->m= utex, tmo);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (r)
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0a->depth--;
-=C2=A0 =C2=A0 =C2=A0 =C2=A0return r;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0return pthread_mutex_timedlock(&a->= mutex, tmo);
=C2=A0}

=C2=A0static inline void unlock(struct mutex_lock *a)
=C2=A0{
-=C2=A0 =C2=A0 =C2=A0 =C2=A0a->depth--;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_mutex_unlock(&a->mutex); =C2=A0}

diff --git a/multipathd/main.c b/multipathd/main.c
index fff482c..c288195 100644
--- a/multipathd/main.c
+++ b/multipathd/main.c
@@ -2046,7 +2046,6 @@ init_vecs (void)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 return NULL;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_mutex_init(&vecs->lock.mute= x, NULL);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0vecs->lock.depth =3D 0;

=C2=A0 =C2=A0 =C2=A0 =C2=A0 return vecs;
=C2=A0}
@@ -2394,16 +2393,16 @@ child (void * param)
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_cancel(uxlsnr_thr);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_cancel(uevq_thr);

+=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_join(check_thr, NULL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_join(uevent_thr, NULL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_join(uxlsnr_thr, NULL);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0pthread_join(uevq_thr, NULL);
+
=C2=A0 =C2=A0 =C2=A0 =C2=A0 lock(&vecs->lock);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 free_pathvec(vecs->pathvec, FREE_PATHS);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 vecs->pathvec =3D NULL;
=C2=A0 =C2=A0 =C2=A0 =C2=A0 unlock(&vecs->lock);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0/* Now all the waitevent threads will start rus= hing in. */
-=C2=A0 =C2=A0 =C2=A0 =C2=A0while (vecs->lock.depth > 0) {
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0sleep (1); /* This = is weak. */
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0condlog(3, "Ha= ve %d wait event checkers threads to de-alloc,"
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0" waiting...", vecs->lock.depth);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0}
+
=C2=A0 =C2=A0 =C2=A0 =C2=A0 pthread_mutex_destroy(&vecs->lock.m= utex);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 FREE(vecs);
=C2=A0 =C2=A0 =C2=A0 =C2=A0 vecs =3D NULL;
--
2.9.2

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-deve= l

--001a113f37ccc8de5a053a4993b7-- --===============8021650317399159655== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline --===============8021650317399159655==--