On 30 Sep 2022, at 16:46, Peng He wrote:
> Eelco Chaudron <echau...@redhat.com> 于2022年9月30日周五 22:37写道：
>
>>
>>
>> On 23 Sep 2022, at 18:29, Peng He wrote:
>>
>>> There is a race condition between the revalidator threads and
>>> the handler/pmd threads.
>>>
>>> revalidator PMD threads
>>> push_dp_ops deletes a key and tries
>>> to del the dp magaflow.
>>> does the upcall, generates a new
>> ukey,
>>> and replaces the old ukey, now the
>> old
>>> ukey state is UKEY_DELETED
>>>
>>> dp_ops succeeds, tries to change
>>> the old ukey's state into
>>> UKEY_EVICTED, however, the old
>>> ukey's state is already UKEY_DELETED,
>>> so OVS aborts.
>>
>> Can you give a call trace example, as try_ukey_replace() should handle
>> this?
>>
>> If it is a valid state change, it should be handled transition_ukey() not
>> by bypassing the call to it.
>>
>
> It means if the ukey->state >= UKEY_EVICTED, it's not needed to do the
> state transition.
> It's not a state change. since we have actually two threads moving forward
> the state machine.
Well, I think it should be handled through the state machine, as we still take
additional actions as if we would have entered this state.
Can you explain what code path is causing this, as try_ukey_replace() will fail
if it’s not in UKEY_EVICTED state?
>>
>>> I did not observe this in the real environment, as it takes time for
>>> PMDs to finish the upcall and replace the old ukeys. Normally, the
>>> revalidator will change ukey state into UKEY_EVICTED first.
>>> But it's better to cover this case.
>>>
>>> Signed-off-by: Peng He <hepeng.0...@bytedance.com>
>>> ---
>>> ofproto/ofproto-dpif-upcall.c | 8 ++++++--
>>> 1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/ofproto/ofproto-dpif-upcall.c
>> b/ofproto/ofproto-dpif-upcall.c
>>> index 7ea2a30f5..e8bbcfeaf 100644
>>> --- a/ofproto/ofproto-dpif-upcall.c
>>> +++ b/ofproto/ofproto-dpif-upcall.c
>>> @@ -2420,7 +2420,9 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
>> *ops, size_t n_ops)
>>> if (op->dop.error) {
>>> if (op->ukey) {
>>> ovs_mutex_lock(&op->ukey->mutex);
>>> - transition_ukey(op->ukey, UKEY_EVICTED);
>>> + if (op->ukey->state < UKEY_EVICTED) {
>>> + transition_ukey(op->ukey, UKEY_EVICTED);
>>> + }
>>> ovs_mutex_unlock(&op->ukey->mutex);
>>> }
>>> /* if it's a flow_del error, 'stats' is unusable, it's ok
>>> @@ -2441,7 +2443,9 @@ push_dp_ops(struct udpif *udpif, struct ukey_op
>> *ops, size_t n_ops)
>>>
>>> if (op->ukey) {
>>> ovs_mutex_lock(&op->ukey->mutex);
>>> - transition_ukey(op->ukey, UKEY_EVICTED);
>>> + if (op->ukey->state < UKEY_EVICTED) {
>>> + transition_ukey(op->ukey, UKEY_EVICTED);
>>> + }
>>> push->used = MAX(stats->used, op->ukey->stats.used);
>>> push->tcp_flags = stats->tcp_flags |
>> op->ukey->stats.tcp_flags;
>>> push->n_packets = stats->n_packets -
>> op->ukey->stats.n_packets;
>>> --
>>> 2.25.1
>>>
>>> _______________________________________________
>>> dev mailing list
>>> d...@openvswitch.org
>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
>
> --
> hepeng
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
