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…
>>
>> Cheers,
>>
>> Eelco
>>
>> Acked-by: Eelco Chaudron <[email protected]>
>>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
