<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Thu, Jun 20, 2013 at 10:07 AM, Jesse Gross <span dir="ltr"><<a href="mailto:jesse@nicira.com" target="_blank">jesse@nicira.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Wed, Jun 19, 2013 at 11:14 PM, Andy Zhou <<a href="mailto:azhou@nicira.com">azhou@nicira.com</a>> wrote:<br>
> This patch seems to work fine. Please feel free to push.<br>
<br>
</div>Thanks, I applied the combined patch.<br>
<div class="im"><br></div></blockquote><div>Sounds good. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">
> I have two concerns mainly on the style side:<br>
><br>
> 1. In the same file flow.c, we are mixing the _deferred_ functions with the<br>
> style of passing 'deferred' as a parameter in the same file. The<br>
> correctness of caller in setting 'deferred' value does not look obviously<br>
> correct.<br>
<br>
</div>I agree that this is not great from a consistency perspective. I<br>
changed the table destroy function to use the same style but left the<br>
actions alone since we only have a deferred destroy function. Other<br>
than conventions though, which part do you think is hard to<br>
understand?<br></blockquote><div>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. <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div class="im"><br>
> 2. In function ovw_flow_free()<br>
> ...<br>
> ovs_sw_flow_mask_del_ref((struct sw_flow_mask __force *)flow->mask,<br>
> deferred);<br>
> ...<br>
><br>
> The type cast to (flow->mask) does not look good to me. It also defeats some<br>
> useful lockdep checks.<br>
<br>
</div>In this case, the semantics are basically that the caller needs to own<br>
the table, which may occur either by holding OVS lock or through the<br>
expiration of an RCU grace period. The latter is hard for this<br>
function to determine, so using the usual ovsl_dereference would<br>
provoke spurious warnings. We have something similar for actions in<br>
__flow_free(), which has the same type of issues.<br></blockquote><div>Yes, I agree it is technically necessary for the cast, just that we will be losing the some benefits of lockdep checks. <br></div></div><br></div></div>