[ovs-dev] [PATCH 1/2] datapath: Mega flow implementation

Andy Zhou azhou at nicira.com
Thu Jun 20 17:39:23 UTC 2013


On Thu, Jun 20, 2013 at 10:07 AM, Jesse Gross <jesse at nicira.com> wrote:

> On Wed, Jun 19, 2013 at 11:14 PM, Andy Zhou <azhou at nicira.com> wrote:
> > This patch seems to work fine.  Please feel free to push.
>
> Thanks, I applied the combined patch.
>
> Sounds good.

> >  I have two concerns mainly on the style side:
> >
> > 1. In the same file flow.c, we are mixing the _deferred_ functions with
> the
> > style of passing 'deferred' as a parameter in the same file.  The
> > correctness of caller in setting 'deferred' value does not look obviously
> > correct.
>
> I agree that this is not great from a consistency perspective. I
> changed the table destroy function to use the same style but left the
> actions alone since we only have a deferred destroy function. Other
> than conventions though, which part do you think is hard to
> understand?
>
Just how I would guess for some one new to look at this. It is not hard for
me now that I have looked at this so much in the last few days.

>
> > 2. In function ovw_flow_free()
> > ...
> > ovs_sw_flow_mask_del_ref((struct sw_flow_mask __force *)flow->mask,
> >                  deferred);
> > ...
> >
> > The type cast to (flow->mask) does not look good to me. It also defeats
> some
> > useful lockdep checks.
>
> In this case, the semantics are basically that the caller needs to own
> the table, which may occur either by holding OVS lock or through the
> expiration of an RCU grace period. The latter is hard for this
> function to determine, so using the usual ovsl_dereference would
> provoke spurious warnings. We have something similar for actions in
> __flow_free(), which has the same type of issues.
>
Yes, I agree it is technically necessary for the cast, just that we will be
losing the some benefits of lockdep checks.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20130620/60a7a27d/attachment-0003.html>


More information about the dev mailing list