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

Jesse Gross jesse at nicira.com
Thu Jun 20 17:07:19 UTC 2013


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.

>  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?

> 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.



More information about the dev mailing list