On Tue, May 20, 2014 at 5:27 PM, Joe Stringer <joestrin...@nicira.com> wrote:
> On 21 May 2014 10:33, Joe Stringer <joestrin...@nicira.com> wrote:
>>
>> On 21 May 2014 10:05, Ryan Wilson 76511 <wr...@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?
Would this lead to writer to be locked out for a long time?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
