[ovs-dev] [PATCHv2 3/4] upcall: Track ukey states.

[Joe Stringer]

On 31 August 2016 at 13:17, Jarno Rajahalme <jarno at ovn.org> wrote:
> With small nits below,
>
> Acked-by: Jarno Rajahalme <jarno at ovn.org>

Thanks, I also noticed a couple of VLOGs missing their ratelimiters.

>> @@ -334,9 +345,9 @@ static int ukey_create_from_dpif_flow(const struct udpif *,
>>                                       struct udpif_key **);
>> static void ukey_get_actions(struct udpif_key *, const struct nlattr **actions,
>>                              size_t *size);
>> -static bool ukey_install_start(struct udpif *, struct udpif_key *ukey);
>> -static bool ukey_install_finish(struct udpif_key *ukey, int error);
>> +static bool ukey_install__(struct udpif *, struct udpif_key *ukey);
>
> You need OVS_TRY_LOCK(true, ukey->mutex) here as, and in general all declarations should have the same thread safety annotations as the definitions themselves.

OK. I wasn't sure whether this was necessary.

>> static bool ukey_install(struct udpif *udpif, struct udpif_key *ukey);
>> +static void transition_ukey(struct udpif_key *, enum ukey_state dat);
>
> You need OVS_REQUIRES(ukey->mutex) here for thread safety analysis to be done.

Ack, will update.

>> @@ -1359,20 +1372,23 @@ handle_upcalls(struct udpif *udpif, struct upcall *upcalls,
>>         }
>>     }
>>
>> -    /* Execute batch.
>> -     *
>> -     * We install ukeys before installing the flows, locking them for exclusive
>> -     * access by this thread for the period of installation. This ensures that
>> -     * other threads won't attempt to delete the flows as we are creating them.
>> -     */
>> +    /* Execute batch. */
>>     n_opsp = 0;
>>     for (i = 0; i < n_ops; i++) {
>>         opsp[n_opsp++] = &ops[i].dop;
>>     }
>>     dpif_operate(udpif->dpif, opsp, n_opsp);
>>     for (i = 0; i < n_ops; i++) {
>> -        if (ops[i].ukey) {
>> -            ukey_install_finish(ops[i].ukey, ops[i].dop.error);
>> +        struct udpif_key *ukey = ops[i].ukey;
>> +
>> +        if (ukey) {
>> +            if (ops[i].dop.error) {
>> +                transition_ukey(ukey, UKEY_EVICTED);
>> +            } else {
>> +                ovs_mutex_lock(&ukey->mutex);
>> +                transition_ukey(ukey, UKEY_OPERATIONAL);
>> +                ovs_mutex_unlock(&ukey->mutex);
>> +            }
>
> Upper transition_ukey() requires the mutex as well.

True. I was thinking that the upper was still in UKEY_CREATED state,
but it is actually UKEY_VISIBLE which means someone else could look at
it. I'll update this.