[ovs-dev] [PATCH v2 2/2] ofproto: Allow in-place modifications of datapath flows.

[Joe Stringer]

On 11 August 2015 at 11:13, Jarno Rajahalme <jrajahalme at nicira.com> wrote:
>> -            if (!keep) {
>> +            if (result == UKEY_DELETE) {
>>                 delete_op_init(udpif, &ops[n_ops++], ukey);
>> +            } else if (result == UKEY_MODIFY) {
>> +                ofpbuf_delete(ukey->actions);
>> +                ukey->actions = ofpbuf_clone(&odp_actions);
>
> ukey’s actions is documented to be read only, so you should check if it is safe to change it. Also, ukey is said to be RCU protected, so maybe the ukey->actions pointer should be changed to an RCU pointer and the old key postpone deleted after the new pointer is set? Or, have you checked that holding the ukey mutex while changing the actions is enough? In that case you should update the struct ukey definition accordingly.

I think that SFLOW_UPCALL handling may dislike this; Generally I think
that the assumption is that you will lock the ukey while using its
contents. Either way, that code will need to be updated and the
definition documentation should be changed accordingly.

>> +                continue;
>> +            }
>> +
>> +            if (purge) {
>> +                result = UKEY_DELETE;
>> +            } else if (ukey->dump_seq == dump_seq
>> +                     || ukey->reval_seq == reval_seq) {
>> +                result = UKEY_KEEP;
>> +            } else {
>> +                struct dpif_flow_stats stats;
>> +                COVERAGE_INC(revalidate_missed_dp_flow);
>> +                memset(&stats, 0, sizeof stats);
>> +                result = revalidate_ukey(udpif, ukey, &stats, &odp_actions,
>> +                                         reval_seq);
>> +            }
>> +
>> +            if (result == UKEY_DELETE) {
>>                 struct ukey_op *op = &ops[n_ops++];
>>
>>                 delete_op_init(udpif, op, ukey);
>> @@ -2089,17 +2137,20 @@ revalidator_sweep__(struct revalidator *revalidator, bool purge)
>>                     push_ukey_ops(udpif, umap, ops, n_ops);
>
> push_key_ops() will try to take the locks of the ukeys, so it would double lock the key we are currently processing.

push_ukey_ops__() takes the ukey mutex, yes. This is avoided today by
unlocking earlier in the function.

IMO the most likely race condition trigger here is when a user
executes ovs-appctl revalidator/purge, because the main thread has a
small chance to iterate across a umap in synchronization (but slightly
out of time) with a corresponding revalidator thread. Jarno suggested
offline adding some new variable to ukey which indicates that the ukey
has been swept this round.