[ovs-dev] [PATCH 1/5] ovs-thread: Add a comment

[Ben Pfaff]

On Wed, Feb 05, 2014 at 11:47:17AM +0900, YAMAMOTO Takashi wrote:
> > On Wed, Jan 15, 2014 at 12:41:20PM +0900, YAMAMOTO Takashi wrote:
> >> Add a comment about implicit synchronization which
> >> fat-rwlock seems to rely on.
> >> 
> >> Signed-off-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
> >> ---
> >>  lib/ovs-thread.h | 8 ++++++++
> >>  1 file changed, 8 insertions(+)
> >> 
> >> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> >> index 8cf2ecc..dab7510 100644
> >> --- a/lib/ovs-thread.h
> >> +++ b/lib/ovs-thread.h
> >> @@ -447,6 +447,14 @@ void xpthread_join(pthread_t, void **);
> >>   * Compared to pthread_key_t, ovsthread_key_t has the follow limitations:
> >>   *
> >>   *    - Destructors must not access thread-specific data (via ovsthread_key).
> >> + *
> >> + * Unlike pthread_key_t, this API implicitly serializes key removal
> >> + * (ovsthread_key_delete) and thread exit. (destructor)
> >> + * I.e.
> >> + *    - ovsthread_key_delete ensures the completion of destructors for
> >> + *      the key.
> >> + *    - Once ovsthread_key_delete completed, no destructors for the key
> >> + *      will be executed on thread exit.
> >>   */
> >>  typedef struct ovsthread_key *ovsthread_key_t;
> > 
> > I think I understand what you are saying, but I think that I would
> > phrase it differently.  Does the following express what you mean?
> > 
> > diff --git a/lib/ovs-thread.c b/lib/ovs-thread.c
> > index a20b2fd..18e524e 100644
> > --- a/lib/ovs-thread.c
> > +++ b/lib/ovs-thread.c
> > @@ -592,7 +592,8 @@ ovsthread_key_create(ovsthread_key_t *keyp, void (*destructor)(void *))
> >  }
> >  
> >  /* Frees 'key'.  The destructor supplied to ovsthread_key_create(), if any, is
> > - * not called.
> > + * not called.  This function returns only after any running destructors for
> > + * 'key' complete execution.
> 
> 'running destructors' is not exact.
> i believe pthread api allows for concurrent exiting threads to
> start executing the destructor after pthread_key_delete completed.
> but this ovsthread one doesn't.  and fat-rwlock relies on it.
> 
> i think it's better to say the difference explicitly as
> it guarantees more than pthread while using similar-looking names.
> i was confused when reading fat-rwlock.

Do you like the following version?

--8<--------------------------cut here-------------------------->8--

From: Ben Pfaff <blp at nicira.com>
Date: Thu, 6 Feb 2014 17:06:00 -0800
Subject: [PATCH] ovs-thread: Add a comment.

Reported-by: YAMAMOTO Takashi <yamamoto at valinux.co.jp>
Signed-off-by: Ben Pfaff <blp at nicira.com>
---
 lib/ovs-thread.h |    8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 8cf2ecc..f031894 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -444,9 +444,15 @@ void xpthread_join(pthread_t, void **);
  * (by recompiling).  Thus, one may more freely use this form of
  * thread-specific data.
  *
- * Compared to pthread_key_t, ovsthread_key_t has the follow limitations:
+ * ovsthread_key_t also differs from pthread_key_t in the following ways:
  *
  *    - Destructors must not access thread-specific data (via ovsthread_key).
+ *
+ *    - The pthread_key_t API allows concurrently exiting threads to start
+ *      executing the destructor after pthread_key_delete() returns.  The
+ *      ovsthread_key_t API guarantees that, when ovsthread_key_delete()
+ *      returns, all destructors have returned and no new ones will start
+ *      execution.
  */
 typedef struct ovsthread_key *ovsthread_key_t;
 
-- 
1.7.10.4