[ovs-dev] [PATCH 1/6] ofproto: Hold ofproto_mutex when enabling or disabling eviction.

[Jarno Rajahalme]

Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>

I would have expected clang to flag different annotations as a warning… Did you look if we have similar situations with other functions? Would it be possible to automate such a check somehow?

I’ll review the rest of the series as well,

  Jarno

> On Jul 1, 2015, at 2:06 PM, Ben Pfaff <blp at nicira.com> wrote:
> 
> Jarno, this patch is probably a good one for you to look at.  It's a
> possible important bug fix and I know that you're knowledgeable about
> the mutex in question.
> 
> (If you wanted to look at the rest of the series that would be nice too
> but this patch in particular may be important.)
> 
> On Wed, Jun 24, 2015 at 10:57:01AM -0700, Ben Pfaff wrote:
>> ofproto_enable_eviction() and ofproto_disable_eviction() require
>> ofproto_mutex (and they were even annotated that way, though not on their
>> prototypes but only at definition), but it wasn't being held.  This fixes
>> the problem.
>> 
>> Found by inspection.
>> 
>> Signed-off-by: Ben Pfaff <blp at nicira.com>
>> ---
>> ofproto/ofproto.c | 21 ++++++++++++---------
>> 1 file changed, 12 insertions(+), 9 deletions(-)
>> 
>> diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
>> index 08ba043..278c97f 100644
>> --- a/ofproto/ofproto.c
>> +++ b/ofproto/ofproto.c
>> @@ -83,10 +83,12 @@ static void oftable_set_name(struct oftable *, const char *name);
>> 
>> static enum ofperr evict_rules_from_table(struct oftable *)
>>     OVS_REQUIRES(ofproto_mutex);
>> -static void oftable_disable_eviction(struct oftable *);
>> +static void oftable_disable_eviction(struct oftable *)
>> +    OVS_REQUIRES(ofproto_mutex);
>> static void oftable_enable_eviction(struct oftable *,
>>                                     const struct mf_subfield *fields,
>> -                                    size_t n_fields);
>> +                                    size_t n_fields)
>> +    OVS_REQUIRES(ofproto_mutex);
>> 
>> /* A set of rules within a single OpenFlow table (oftable) that have the same
>>  * values for the oftable's eviction_fields.  A rule to be evicted, when one is
>> @@ -1419,13 +1421,6 @@ ofproto_configure_table(struct ofproto *ofproto, int table_id,
>>         return;
>>     }
>> 
>> -    if (s->groups) {
>> -        oftable_enable_eviction(table, s->groups, s->n_groups);
>> -    } else {
>> -        oftable_disable_eviction(table);
>> -    }
>> -
>> -    table->max_flows = s->max_flows;
>> 
>>     if (classifier_set_prefix_fields(&table->cls,
>>                                      s->prefix_fields, s->n_prefix_fields)) {
>> @@ -1433,6 +1428,12 @@ ofproto_configure_table(struct ofproto *ofproto, int table_id,
>>     }
>> 
>>     ovs_mutex_lock(&ofproto_mutex);
>> +    if (s->groups) {
>> +        oftable_enable_eviction(table, s->groups, s->n_groups);
>> +    } else {
>> +        oftable_disable_eviction(table);
>> +    }
>> +    table->max_flows = s->max_flows;
>>     evict_rules_from_table(table);
>>     ovs_mutex_unlock(&ofproto_mutex);
>> }
>> @@ -7431,7 +7432,9 @@ static void
>> oftable_destroy(struct oftable *table)
>> {
>>     ovs_assert(classifier_is_empty(&table->cls));
>> +    ovs_mutex_lock(&ofproto_mutex);
>>     oftable_disable_eviction(table);
>> +    ovs_mutex_unlock(&ofproto_mutex);
>>     classifier_destroy(&table->cls);
>>     free(table->name);
>> }
>> -- 
>> 2.1.3
>>