<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 21 May 2014 10:33, Joe Stringer <span dir="ltr">&lt;<a href="mailto:joestringer@nicira.com" target="_blank">joestringer@nicira.com</a>&gt;</span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr">On 21 May 2014 10:05, Ryan Wilson 76511 <span dir="ltr">&lt;<a href="mailto:wryan@vmware.com" target="_blank">wryan@vmware.com</a>&gt;</span> wrote:<div class="">

<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
Hey Andy,<br><div><br>&gt;At a high level,  ofgroup struct current has rwlock that essentially<br>&gt;solving<br>&gt;the same problem as the ref count proposed in this patch. It would be<br>&gt;better<br>&gt;it seems confusing if we use both method together.<br>


&gt;<br>&gt;Looking at the code, I&#39;d think extending rwlock to cover xlate<br>&gt;fastpath is the most<br>&gt;straight forward approach.<br><br></div>The reason I use the reference count is because of the following situation:<br>


- The main thread and xlate cache both have pointers to the group.<br>- The main thread deletes the group and frees memory.<br>- However, the xlate cache still has a pointer to the group and if a<br>handler is run prior to the revalidator clearing the xlate cache, a<br>


handler could write to freed memory.<br></blockquote><div><br></div></div><div>Strictly speaking, the xlate_cache is only used inside revalidators at the moment. However, this doesn&#39;t contradict your point. We need to guarantee that the xlate_cache won&#39;t try to write to the group stats after the main thread frees it. And this is not guaranteed by the group rwlock. As you say, for other types of stats, this is guaranteed with refcounting.</div>

</div></blockquote><div><br></div><div>Andy, I looked again at struct ofgroup and I see the comment now :-)</div><div><br></div><div>I&#39;ll describe my understanding of the possible problem:</div><div>* ofproto lock ensures that the adding/deletion/lookup of ofgroups is consistent across multiple threads.</div>

<div>* ofgroup-&gt;rwlock can ensure that reading/writing of ofgroup contents is consistent across multiple threads.</div><div>* We can currently guarantee existence of ofgroup because we always lookup (grabbing both locks), read/write, release.</div>

<div><br></div><div>As with current group access, when building up the xlate_cache, we grab the ofproto lock to lookup the ofgroup. We grab the ofgroup lock to ensure it isn&#39;t freed.</div><div>However, for xlate_cache we want to store a pointer to the ofgroup for a longer period. As soon as we release the lock, there is no guarantee that the ofgroup still exists.</div>

<div>The refcount proposed in this patch allows the pointer to be held longer-term and guaranteed to still be available.</div><div><br></div><div>The alternative would be to hold a readlock on the rwlock for the lifetime of the xlate_cache, which would prevent group deletion. Does that make sense?</div>

</div></div></div>