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. > 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. _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
