On Fri, Mar 14, 2014 at 10:44 AM, Ben Pfaff <b...@nicira.com> wrote:
> On Fri, Mar 14, 2014 at 01:36:15AM -0700, Andy Zhou wrote:
> > Acked-by: Andy Zhou <az...@nicira.com>
>
> Thanks.
>
> > On Tue, Mar 11, 2014 at 1:56 PM, Ben Pfaff <b...@nicira.com> wrote:
> >
> > > RCU allows multiple threads to read objects in parallel without any
> > > performance penalty. The following commit will introduce the first
> use.
> > >
> > > Signed-off-by: Ben Pfaff <b...@nicira.com>
> > > + * Use ovsrcu_set() to write an RCU-protected pointer and
> > > ovsrcu_postpone() to
> > > + * free the previous data. If more than one thread can write the
> > > pointer, then
> > > + * some form of external synchronization, e.g. a mutex, is needed to
> > > prevent
> > > + * writers from interfering with one another. For example, to write
> the
> > > + * pointer variable declared above while safely freeing the old value:
> > > + *
> > > + * static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
> > > + *
> > > + * static void
> > > + * free_flow(struct flow *flow)
> > > + * {
> > > + * free(flow);
> > > + * }
> > > + *
> > > + * void
> > > + * change_flow(struct flow *new_flow)
> > > + * {
> > > + * ovs_mutex_lock(&mutex);
> > > + * ovsrcu_postpone(free_flow, ovsrcu_get(struct flow *,
> &flowp));
> > > + * ovsrcu_set(&flowp, new_flow);
> > > + * ovs_mutex_unlock(&mutex);
> > >
> > I assume the mutex lock and unlock should have embeded memory barrier
> > operations to make sure all previous writes are visble. If true, then
> > ovsrcu_get() and ovsrcu_set() does not seem necessary in this example.
> They
> > are correct though.
>
> They are correct, but not really necessary, as you say. How about the
> following incremental:
>
> It is better. Thanks.
> diff --git a/lib/ovs-rcu.h b/lib/ovs-rcu.h
> index 6721273..710870a 100644
> --- a/lib/ovs-rcu.h
> +++ b/lib/ovs-rcu.h
> @@ -98,7 +98,8 @@
> * change_flow(struct flow *new_flow)
> * {
> * ovs_mutex_lock(&mutex);
> - * ovsrcu_postpone(free_flow, ovsrcu_get(struct flow *, &flowp));
> + * ovsrcu_postpone(free_flow,
> + * ovsrcu_get_protected(struct flow *, &flowp));
> * ovsrcu_set(&flowp, new_flow);
> * ovs_mutex_unlock(&mutex);
> * }
> @@ -118,33 +119,44 @@
> *
> * struct flow *flow = ovsrcu_get(struct flow *, flowp);
> *
> + * If the pointer variable is currently protected against change (because
> + * the current thread holds a mutex that protects it),
> ovsrcu_get_protected()
> + * may be used instead. Only on the Alpha architecture is this likely to
> + * generate different code, but it may be useful documentation.
> + *
> * (With GNU C or Clang, you get a compiler error if TYPE is wrong; other
> * compilers will merrily carry along accepting the wrong type.)
> */
> #if __GNUC__
> #define OVSRCU_TYPE(TYPE) struct { ATOMIC(TYPE) p; }
> -#define ovsrcu_get__(TYPE, VAR) \
> +#define ovsrcu_get__(TYPE, VAR, ORDER) \
> ({ \
> TYPE value__; \
> \
> atomic_read_explicit(CONST_CAST(ATOMIC(TYPE) *, &(VAR)->p), \
> - &value__, memory_order_consume); \
> + &value__, ORDER); \
> \
> value__; \
> })
> -#define ovsrcu_get(TYPE, VAR) CONST_CAST(TYPE, ovsrcu_get__(TYPE, VAR))
> +#define ovsrcu_get(TYPE, VAR) \
> + CONST_CAST(TYPE, ovsrcu_get__(TYPE, VAR, memory_order_consume))
> +#define ovsrcu_get_protected(TYPE, VAR) \
> + CONST_CAST(TYPE, ovsrcu_get__(TYPE, VAR, memory_order_relaxed))
> #else /* not GNU C */
> typedef struct ovsrcu_pointer { ATOMIC(void *) p; };
> #define OVSRCU_TYPE(TYPE) struct ovsrcu_pointer
> static inline void *
> -ovsrcu_get__(const struct ovsrcu_pointer *pointer)
> +ovsrcu_get__(const struct ovsrcu_pointer *pointer, memory_order order)
> {
> void *value;
> atomic_read_explicit(&CONST_CAST(struct ovsrcu_pointer *, pointer)->p,
> - &value, memory_order_consume);
> + &value, order);
> return value;
> }
> -#define ovsrcu_get(TYPE, VAR) CONST_CAST(TYPE, ovsrcu_get__(VAR))
> +#define ovsrcu_get(TYPE, VAR) \
> + CONST_CAST(TYPE, ovsrcu_get__(VAR, memory_order_consume))
> +#define ovsrcu_get_protected(TYPE, VAR) \
> + CONST_CAST(TYPE, ovsrcu_get__(VAR, memory_order_relaxed))
> #endif
>
> /* Writes VALUE to the RCU-protected pointer whose address is VAR.
>
>
> > > +/* Calls FUNCTION passing ARG as its pointer-type argument following
> the
> > > next
> > > + * grace period. See "Usage" above for example. */
> > > +void ovsrcu_postpone__(void (*function)(void *aux), void *aux);
> > > +#define ovsrcu_postpone(FUNCTION, ARG) \
> > > + ((void) sizeof((FUNCTION)(ARG), 1),
> >
> > I suppose the idea of using sizeof is for type checking. It is not
> obvious
> > to me why "1" required.
>
> It's because 'function' has to have a void return type, so
> sizeof(function()) is sizeof(void), which yields a warning.
>
Make sense. Thanks for explaining.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
