Even with an atomic, one still needs the lock here to protect other data, and so the atomicity is redundant, and has some cost. I guess that you know that, and think that the cost is a good tradeoff for clarity, but I am not sure. Anyway, I am willing to talk about it, but for now I left it as-is.
I applied this to master. I forgot to add the actual documentation mentioned in the commit message. Here is what I folded in: diff --git a/lib/dpif.h b/lib/dpif.h index 7f1b6c9..d028085 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -317,6 +317,24 @@ * location. * * - Adding and removing ports to achieve a new configuration. + * + * + * Thread-safety + * ============= + * + * Most of the dpif functions are fully thread-safe: they may be called from + * any number of threads on the same or different dpif objects. The exceptions + * are: + * + * - dpif_port_poll() and dpif_port_poll_wait() are conditionally + * thread-safe: they may be called from different threads only on + * different dpif objects. + * + * - Functions that operate on struct dpif_port_dump or struct + * dpif_flow_dump are conditionally thread-safe with respect to those + * objects. That is, one may dump ports or flows from any number of + * threads at once, but each thread must use its own struct dpif_port_dump + * or dpif_flow_dump. */ #ifndef DPIF_H #define DPIF_H 1 On Wed, Jul 24, 2013 at 06:08:01PM -0700, Ethan Jackson wrote: > I'd be inclined to make the reference count an atomic. It'll be > consistent with a bunch of other code I've written, and it's harder to > forget to lock. > > Acked-by: Ethan Jackson <[email protected]> > > > On Tue, Jul 23, 2013 at 5:07 PM, Ben Pfaff <[email protected]> wrote: > > Signed-off-by: Ben Pfaff <[email protected]> > > --- > > lib/dpif.c | 108 > > +++++++++++++++++++++++++++++++++++++++++++++-------------- > > 1 files changed, 82 insertions(+), 26 deletions(-) > > > > diff --git a/lib/dpif.c b/lib/dpif.c > > index 4878aac..0d8dd9d 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -70,6 +70,9 @@ struct registered_dpif_class { > > static struct shash dpif_classes = SHASH_INITIALIZER(&dpif_classes); > > static struct sset dpif_blacklist = SSET_INITIALIZER(&dpif_blacklist); > > > > +/* Protects 'dpif_classes', including the refcount, and 'dpif_blacklist'. > > */ > > +static pthread_mutex_t dpif_mutex = PTHREAD_MUTEX_INITIALIZER; > > + > > /* Rate limit for individual messages going to or from the datapath, > > output at > > * DBG level. This is very high because, if these are enabled, it is > > because > > * we really need to see them. */ > > @@ -109,10 +112,8 @@ dp_initialize(void) > > } > > } > > > > -/* Registers a new datapath provider. After successful registration, new > > - * datapaths of that type can be opened using dpif_open(). */ > > -int > > -dp_register_provider(const struct dpif_class *new_class) > > +static int > > +dp_register_provider__(const struct dpif_class *new_class) > > { > > struct registered_dpif_class *registered_class; > > > > @@ -137,11 +138,25 @@ dp_register_provider(const struct dpif_class > > *new_class) > > return 0; > > } > > > > +/* Registers a new datapath provider. After successful registration, new > > + * datapaths of that type can be opened using dpif_open(). */ > > +int > > +dp_register_provider(const struct dpif_class *new_class) > > +{ > > + int error; > > + > > + xpthread_mutex_lock(&dpif_mutex); > > + error = dp_register_provider__(new_class); > > + xpthread_mutex_unlock(&dpif_mutex); > > + > > + return error; > > +} > > + > > /* Unregisters a datapath provider. 'type' must have been previously > > * registered and not currently be in use by any dpifs. After > > unregistration > > * new datapaths of that type cannot be opened using dpif_open(). */ > > -int > > -dp_unregister_provider(const char *type) > > +static int > > +dp_unregister_provider__(const char *type) > > { > > struct shash_node *node; > > struct registered_dpif_class *registered_class; > > @@ -165,12 +180,31 @@ dp_unregister_provider(const char *type) > > return 0; > > } > > > > +/* Unregisters a datapath provider. 'type' must have been previously > > + * registered and not currently be in use by any dpifs. After > > unregistration > > + * new datapaths of that type cannot be opened using dpif_open(). */ > > +int > > +dp_unregister_provider(const char *type) > > +{ > > + int error; > > + > > + dp_initialize(); > > + > > + xpthread_mutex_lock(&dpif_mutex); > > + error = dp_unregister_provider__(type); > > + xpthread_mutex_unlock(&dpif_mutex); > > + > > + return error; > > +} > > + > > /* Blacklists a provider. Causes future calls of dp_register_provider() > > with > > * a dpif_class which implements 'type' to fail. */ > > void > > dp_blacklist_provider(const char *type) > > { > > + xpthread_mutex_lock(&dpif_mutex); > > sset_add(&dpif_blacklist, type); > > + xpthread_mutex_unlock(&dpif_mutex); > > } > > > > /* Clears 'types' and enumerates the types of all currently registered > > datapath > > @@ -183,10 +217,36 @@ dp_enumerate_types(struct sset *types) > > dp_initialize(); > > sset_clear(types); > > > > + xpthread_mutex_lock(&dpif_mutex); > > SHASH_FOR_EACH(node, &dpif_classes) { > > const struct registered_dpif_class *registered_class = node->data; > > sset_add(types, registered_class->dpif_class->type); > > } > > + xpthread_mutex_unlock(&dpif_mutex); > > +} > > + > > +static void > > +dp_class_unref(struct registered_dpif_class *rc) > > +{ > > + xpthread_mutex_lock(&dpif_mutex); > > + ovs_assert(rc->refcount); > > + rc->refcount--; > > + xpthread_mutex_unlock(&dpif_mutex); > > +} > > + > > +static struct registered_dpif_class * > > +dp_class_lookup(const char *type) > > +{ > > + struct registered_dpif_class *rc; > > + > > + xpthread_mutex_lock(&dpif_mutex); > > + rc = shash_find_data(&dpif_classes, type); > > + if (rc) { > > + rc->refcount++; > > + } > > + xpthread_mutex_unlock(&dpif_mutex); > > + > > + return rc; > > } > > > > /* Clears 'names' and enumerates the names of all known created datapaths > > with > > @@ -198,14 +258,14 @@ dp_enumerate_types(struct sset *types) > > int > > dp_enumerate_names(const char *type, struct sset *names) > > { > > - const struct registered_dpif_class *registered_class; > > + struct registered_dpif_class *registered_class; > > const struct dpif_class *dpif_class; > > int error; > > > > dp_initialize(); > > sset_clear(names); > > > > - registered_class = shash_find_data(&dpif_classes, type); > > + registered_class = dp_class_lookup(type); > > if (!registered_class) { > > VLOG_WARN("could not enumerate unknown type: %s", type); > > return EAFNOSUPPORT; > > @@ -213,11 +273,11 @@ dp_enumerate_names(const char *type, struct sset > > *names) > > > > dpif_class = registered_class->dpif_class; > > error = dpif_class->enumerate ? dpif_class->enumerate(names) : 0; > > - > > if (error) { > > VLOG_WARN("failed to enumerate %s datapaths: %s", dpif_class->type, > > ovs_strerror(error)); > > } > > + dp_class_unref(registered_class); > > > > return error; > > } > > @@ -253,8 +313,7 @@ do_open(const char *name, const char *type, bool > > create, struct dpif **dpifp) > > dp_initialize(); > > > > type = dpif_normalize_type(type); > > - > > - registered_class = shash_find_data(&dpif_classes, type); > > + registered_class = dp_class_lookup(type); > > if (!registered_class) { > > VLOG_WARN("could not create datapath %s of unknown type %s", name, > > type); > > @@ -266,7 +325,8 @@ do_open(const char *name, const char *type, bool > > create, struct dpif **dpifp) > > name, create, &dpif); > > if (!error) { > > ovs_assert(dpif->dpif_class == registered_class->dpif_class); > > - registered_class->refcount++; > > + } else { > > + dp_class_unref(registered_class); > > } > > > > exit: > > @@ -326,15 +386,11 @@ void > > dpif_close(struct dpif *dpif) > > { > > if (dpif) { > > - struct registered_dpif_class *registered_class; > > - > > - registered_class = shash_find_data(&dpif_classes, > > - dpif->dpif_class->type); > > - ovs_assert(registered_class); > > - ovs_assert(registered_class->refcount); > > + struct registered_dpif_class *rc; > > > > - registered_class->refcount--; > > + rc = shash_find_data(&dpif_classes, dpif->dpif_class->type); > > dpif_uninit(dpif, true); > > + dp_class_unref(rc); > > } > > } > > > > @@ -421,18 +477,18 @@ dpif_get_dp_stats(const struct dpif *dpif, struct > > dpif_dp_stats *stats) > > const char * > > dpif_port_open_type(const char *datapath_type, const char *port_type) > > { > > - struct registered_dpif_class *registered_class; > > + struct registered_dpif_class *rc; > > > > datapath_type = dpif_normalize_type(datapath_type); > > > > - registered_class = shash_find_data(&dpif_classes, datapath_type); > > - if (!registered_class > > - || !registered_class->dpif_class->port_open_type) { > > - return port_type; > > + xpthread_mutex_lock(&dpif_mutex); > > + rc = shash_find_data(&dpif_classes, datapath_type); > > + if (rc && rc->dpif_class->port_open_type) { > > + port_type = rc->dpif_class->port_open_type(rc->dpif_class, > > port_type); > > } > > + xpthread_mutex_unlock(&dpif_mutex); > > > > - return registered_class->dpif_class->port_open_type( > > - registered_class->dpif_class, port_type); > > + return port_type; > > } > > > > /* Attempts to add 'netdev' as a port on 'dpif'. If 'port_nop' is > > -- > > 1.7.2.5 > > > > _______________________________________________ > > dev mailing list > > [email protected] > > http://openvswitch.org/mailman/listinfo/dev _______________________________________________ dev mailing list [email protected] http://openvswitch.org/mailman/listinfo/dev
