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 <i.maxim...@ovn.org>
>> ---
>
> 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.
>
> Cheers,
>
> Eelco
>
> Acked-by: Eelco Chaudron <echau...@redhat.com>
>
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
