On 4/5/24 15:46, Eelco Chaudron wrote:
>
>
> On 5 Apr 2024, at 15:04, Ilya Maximets wrote:
>
>> On 4/5/24 14:33, Eelco Chaudron wrote:
>>>
>>>
>>> On 4 Apr 2024, at 14:09, Ilya Maximets wrote:
>>>
>>>> ukey_install() returns boolean signaling if the ukey was installed
>>>> or not. Installation may fail for a few reasons:
>>>>
>>>> 1. Conflicting ukey.
>>>> 2. Mutex contention while trying to replace existing ukey.
>>>> 3. The same ukey already exists and active.
>>>>
>>>> Only the first case here signals an actual problem. Third one is
>>>> a little odd for userspace datapath, but harmless. Second is the
>>>> most common one that can easily happen during normal operation
>>>> since other threads like revalidators may be currently working on
>>>> this ukey preventing an immediate access.
>>>>
>>>> Since only the first case is actually worth logging and it already
>>>> has its own log message, removing the 'upcall installation fails'
>>>> warning from the upcall_cb(). This should fix most of the random
>>>> failures of userspace system tests in CI.
>>>>
>>>> While at it, also fixing coverage counters. Mutex contention was
>>>> mistakenly counted as a duplicate upcall. ukey contention for
>>>> revalidators was counted only in one of two places.
>>>>
>>>> New counter added for the ukey contention on replace. We should
>>>> not re-use existing upcall_ukey_contention counter for this, since
>>>> it may lead to double counting.
>>>>
>>>> Fixes: 67f08985d769 ("upcall: Replace ukeys for deleted flows.")
>>>> Fixes: 9cec8274ed9a ("ofproto-dpif-upcall: Add VLOG_WARN_RL logs for
>>>> upcall_cb() error.")
>>>> Signed-off-by: Ilya Maximets <[email protected]>
>>>> ---
>>>
>>> Thanks for looking into this, and the patch looks good to me.
>>>
>>> Maybe we should have another patch fixing some of the namings?
>>>
>>> upcall_ukey_replace -> ukey_replace
>>> handler_duplicate_upcall -> duplicate_upcall
>>> upcall_ukey_contention -> revalidate_ukey_contention -> ukey_contention
>>
>> I had something similar in mind, but I didn't make any radical
>> renaming since I plan to backport this change.
>>
>> One more thing is that we would likely want to distinguish
>> contention in handlers/pmds from contention in revalidators,
>> so these will need separate counters, I think.
>
> Ack but all can call the update functions ;) We can follow this up after the
> backport…
Sure.
>
>>>
>>> Cheers,
>>>
>>> Eelco
>>>
>>> Acked-by: Eelco Chaudron <[email protected]>
>>>
>
Thanks! Applied and backported down to 2.17.
Best regards, Ilya Maximets.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
