On Tue, Jul 15, 2014 at 03:23:08PM +1200, Joe Stringer wrote:
> On 15 July 2014 12:11, Ben Pfaff <[email protected]> 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 <[email protected]>
> >
> > 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().
_______________________________________________
dev mailing list
[email protected]
http://openvswitch.org/mailman/listinfo/dev
