[ovs-dev] [PATCH 3/3] ofproto: Rename struct rule's evict lock and use it more widely.
Ethan Jackson
ethan at nicira.com
Wed Sep 4 22:58:53 UTC 2013
Thanks for the reviews, I've merged and backported this.
Ethan
On Wed, Sep 4, 2013 at 3:54 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Wed, Sep 04, 2013 at 03:47:26PM -0700, Ethan Jackson wrote:
>> > I wonder whether it makes sense to speak of 'cr' as being "guarded" by
>> > anything. Most of the data that its users really look at, notably
>> > 'priority' and 'match', is immutable. It is inserted into a
>> > classifier at construction time, and removed at destruction time, and
>> > other changes to the classifier can change 'hmap_node' and 'list', but
>> > few of the users really care:
>>
>> Good point I changed it.
>>
>> > Marking 'flow_cookie' as OVS_GUARDED, as I imagine it should be, makes
>> > tons of new Clang warnings pop up:
>>
>> As discussed off-list, you're right that ideally it should be, but
>> it's going to be a bit of a pain to get the thread safety to play
>> nicely with us here. In my judgement it was cleaner just to skip it.
>> Since it's pretty much impossible for a thread to get access to a rule
>> without taking it's rdlock, I think we're good.
>>
>> I fixed up that comment so it reads as below:
>>
>> /* The rwlock is used to protect those elements in struct rule which are
>> * accessed by multiple threads. While maintaining a pointer to struct
>> * rule, threads are required to hold a readlock. The main ofproto code is
>> * guaranteed not to evict the rule, or change any of the elements "Guarded
>> * by rwlock" without holding the writelock.
>> *
>> * A rule will not be evicted unless both its own and its classifier's
>> * write locks are held. Therefore, while holding a classifier readlock,
>> * one can be assured that write locked rules are safe to reference. */
>>
>> I can resend the patch if you'd like.
>
> No need, I wanted to make sure that you had considered these issues
> but you reassured me.
>
> Acked-by: Ben Pfaff <blp at nicira.com>
More information about the dev
mailing list