[ovs-dev] [PATCHv4 1/2] dpif: Support fetching flow mask via dpif_flow_get().

Ben Pfaff blp at nicira.com
Tue Jul 15 03:26:40 UTC 2014


On Tue, Jul 15, 2014 at 03:23:08PM +1200, Joe Stringer wrote:
> On 15 July 2014 12:11, Ben Pfaff <blp at nicira.com> wrote:
> 
> > dpif_netdev_flow_get() calls dp_netdev_flow_get_actions() twice.  I
> > did not check whether the actions could change in between.  Please
> > change the code to just retrieve the actions once.
> >
> > Acked-by: Ben Pfaff <blp at nicira.com>
> >
> > As an additional optimization I think it would be reasonable for
> > dpif_netdev_flow_get() to return the actions from the
> > dp_netdev_actions directly, without copying them, since they are
> > immutable and RCU-protected from destruction, something like this:
> >
> >             if (actionsp) {
> >                 struct dp_netdev_actions *actions;
> >
> >                 actions = dp_netdev_flow_get_actions(netdev_flow);
> >                 *actionsp = actions->actions;
> >                 *actions_len = actions->size;
> >             }
> >
> 
> Thanks, this cleans the code up quite a bit. I plan to push this soon.

Great.

> I notice that dpif_netdev_flow_dump_next() also does this optimization for
> actions, but there's no mention of RCU in the dpif_flow_dump_next() API. Do
> you think we should add a comment like this to dpif_flow_get() and
> dpif_flow_dump_next()?
> 
> "Implementations may opt to point flow->mask and/or flow->actions at
> RCU-protected data rather than making a copy of them. Therefore, callers
> that wish to hold these over quiesce periods must make a copy of these
> fields before quiescing."

I agree.  I've meant to do this for a while, it's only laziness
preventing me.

If you add this comment then you can remove the comment
    /* XXX the caller must use 'actions' without quiescing */
on dpif_netdev_flow_dump_next().



More information about the dev mailing list