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.
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. Ryan From: Joe Stringer <joestrin...@nicira.com<mailto:joestrin...@nicira.com>> Date: Tuesday, May 20, 2014 3:33 PM To: Ryan Wilson <wr...@vmware.com<mailto:wr...@vmware.com>> Cc: Andy Zhou <az...@nicira.com<mailto:az...@nicira.com>>, Ryan Wilson <wr...@nicira.com<mailto:wr...@nicira.com>>, "dev@openvswitch.org<mailto:dev@openvswitch.org>" <dev@openvswitch.org<mailto: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<mailto: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
