<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:<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>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><br></div><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">Thus, similar to netdev, I use a ref here, so the memory is not freed<br>

until both the cache and main thread are done with the group. Again, like<br>netdev_mutex, the groups&#39; rwlock is to serialize access to the group and<br>bucket stats. Plus the rwlock won&#39;t work once the group is freed.<br>

<br>Let me know if you have a cleaner suggestion to deal with this issue; I<br>agree, I would prefer to use just a lock and / or a ref count.<br></blockquote><div><br></div><div>This sounds consistent with the behaviour for other xlate_cache items.</div>

<div><br></div><div>Could you place a description like this in the commit message?</div><div>Perhaps also, rather than listing the users above the refcount field in &#39;struct ofgroup&#39;, describe why the refcount exists (ie, to ensure that the structure exists when we try to attribute stats to it).</div>

</div>