[ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.
joestringer at nicira.com
Tue May 20 22:33:13 UTC 2014
On 21 May 2014 10:05, Ryan Wilson 76511 <wryan at vmware.com> wrote:
> Hey Andy,
> >At a high level, ofgroup struct current has rwlock that essentially
> >the same problem as the ref count proposed in this patch. It would be
> >it seems confusing if we use both method together.
> >Looking at the code, I'd think extending rwlock to cover xlate
> >fastpath is the most
> >straight forward approach.
> The reason I use the reference count is because of the following situation:
> - The main thread and xlate cache both have pointers to the group.
> - The main thread deletes the group and frees memory.
> - However, the xlate cache still has a pointer to the group and if a
> handler is run prior to the revalidator clearing the xlate cache, a
> handler could write to freed memory.
Strictly speaking, the xlate_cache is only used inside revalidators at the
moment. However, this doesn't contradict your point. We need to guarantee
that the xlate_cache won'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.
Thus, similar to netdev, I use a ref here, so the memory is not freed
> until both the cache and main thread are done with the group. Again, like
> netdev_mutex, the groups' rwlock is to serialize access to the group and
> bucket stats. Plus the rwlock won't work once the group is freed.
> Let me know if you have a cleaner suggestion to deal with this issue; I
> agree, I would prefer to use just a lock and / or a ref count.
This sounds consistent with the behaviour for other xlate_cache items.
Could you place a description like this in the commit message?
Perhaps also, rather than listing the users above the refcount field in
'struct ofgroup', describe why the refcount exists (ie, to ensure that the
structure exists when we try to attribute stats to it).
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the dev