[ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

Joe Stringer joestringer at nicira.com
Wed May 21 00:27:41 UTC 2014


On 21 May 2014 10:33, Joe Stringer <joestringer at nicira.com> wrote:

> 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
>> >solving
>> >the same problem as the ref count proposed in this patch. It would be
>> >better
>> >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.
>

Andy, I looked again at struct ofgroup and I see the comment now :-)

I'll describe my understanding of the possible problem:
* ofproto lock ensures that the adding/deletion/lookup of ofgroups is
consistent across multiple threads.
* ofgroup->rwlock can ensure that reading/writing of ofgroup contents is
consistent across multiple threads.
* We can currently guarantee existence of ofgroup because we always lookup
(grabbing both locks), read/write, release.

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't freed.
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.
The refcount proposed in this patch allows the pointer to be held
longer-term and guaranteed to still be available.

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?
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openvswitch.org/pipermail/ovs-dev/attachments/20140521/77ad5ea5/attachment-0005.html>


More information about the dev mailing list