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 <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. Ack but all can call the update functions ;) We can follow this up after the backport… >> >> Cheers, >> >> Eelco >> >> Acked-by: Eelco Chaudron <echau...@redhat.com> >> _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev