On Thu, Jun 20, 2013 at 10:07 AM, Jesse Gross <je...@nicira.com> wrote:
> On Wed, Jun 19, 2013 at 11:14 PM, Andy Zhou <az...@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.
_______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
