>On Tue, May 20, 2014 at 3:54 PM, Ryan Wilson 76511 <wr...@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?
You're right, in modify_group(), the ofgroup bucket's list is being changed here. So there is a possibility the bucket pointer in the cache will be invalid here. A few ideas here: 1) Use refs for buckets (this seems unnecessarily heavyweight) 2) Always try bucket_lookup when adding stats to a bucket via the cache. If the bucket is not there in the list, we know its been removed via modify_group() and we simply don't update stats. 3) Use ofgroup->rw_lock to protect writing to stats via the xlate cache (this seems to break modularity in the code but it would be the simplest to do). 4) Similar to netdev, remove the group then re-add the group to ofproto instead of modifying the group in place. Thus, the bucket will only be invalid when its group parent is also invalid. Let me know if you have any other ideas. I don't particularly like any of these, but these are what I could come up with on the top of my head. Ryan >> >> 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 <joestrin...@nicira.com> >> Date: Tuesday, May 20, 2014 3:33 PM >> To: Ryan Wilson <wr...@vmware.com> >> Cc: Andy Zhou <az...@nicira.com>, Ryan Wilson <wr...@nicira.com>, >> "dev@openvswitch.org" <dev@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 <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. >> >>> 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). _______________________________________________ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
