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

[Andy Zhou]

On Tue, May 20, 2014 at 3:54 PM, Ryan Wilson 76511 <wryan at vmware.com> wrote:
>
> Thanks for the review, Joe! I added a more clear description in 'struct
> ofgroup' and the commit message to explain why the refcount is needed.
That's good. Thanks for the improvement. I understand now the ref
count is for the fast
path.

I still have one question about the use of ref count.  Suppose one
thread is using the
cache to push stats while another thread is handling group
modification message from
the controller, calling modify_group() in ofproto/ofproto.c Without
holding the read lock,
how would the first thread know the bucket pointer in the cache is still valid?

>
> And Andy, none of my functions in the group patch need group->rw_lock since
> they simply ref / unref group. There are functions in ofproto.c that seem to
> be missing some annotations, but those seem unrelated to my patch. So it
> might be better to do them in a separate patch. Let me know what you think.

That's fine.

>
> Ryan
>
> From: Joe Stringer <joestringer at nicira.com>
> Date: Tuesday, May 20, 2014 3:33 PM
> To: Ryan Wilson <wryan at vmware.com>
> Cc: Andy Zhou <azhou at nicira.com>, Ryan Wilson <wryan at nicira.com>,
> "dev at openvswitch.org" <dev at openvswitch.org>
> Subject: Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for
> Openflow groups.
>
> 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.
>
>> 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).