On 5/12/26 9:51 AM, Eelco Chaudron wrote:
>
>
> On 11 May 2026, at 17:54, Ilya Maximets wrote:
>
>> A simplified ofproto destruction flow looks like this:
>>
>> ofproto_destroy()
>>
>> ofproto_flush__()
>> ofproto_class->flush(ofproto);
>> udpif_flush()
>> udpif_stop_threads(udpif, true)
>> revalidator_purge()
>> ...
>> ovsrcu_postpone(ukey_delete__, ukey)
>> udpif_start_threads()
>>
>> ofproto_class->destruct(p, del)
>> xlate_txn_start();
>> xlate_remove_ofproto(ofproto);
>> xlate_txn_commit();
>> ...
>> recirc_free_ofproto()
>> ...
>> ovsrcu_barrier()
>> close_dpif_backer(ofproto->backer, del);
>>
>> The recirc_free_ofproto() function is used to check if we leaked any
>> recirculation IDs, but it doesn't really wait for everything to be
>> completed and reports false positives. There are two races here in
>> particular:
>>
>> 1. Flush frees recirculation IDs in an RCU-postponed ukey_delete__().
>> There is one ovsrcu_synchronize() call between udpif_flush() and the
>> recirc_free_ofproto() inside xlate_txn_commit(), but synchronization
>> only waits for all threads to enter quiescent state, it doesn't wait
>> for the postponed callbacks to be completed. So, when the
>> recirc_free_ofproto() is called, some IDs can still be not freed.
>>
>> 2. udpif_flush() starts the threads back at the end, so they can still
>> add new ukeys in the short window between the flush and the
>> destruction. In this case some new IDs can be allocated after the
>> flush and the leak check will find them.
>>
>> We can solve both races by moving the leak check down to the end of
>> the ofproto_class->destruct(). We already have the ovsrcu_barrier()
>> there that actually waits for all the RCU-postponed cleanups. And the
>> close_dpif_backer() will take care of all the ukeys created after the
>> flush in a non-postponed fashion.
>>
>> It's hard to pinpoint the exact commit to blame, but citing one that
>> definitely played a role here.
>>
>> Fixes: e672ff9b4d22 ("ofproto-dpif: Restore metadata and registers on
>> recirculation.")
>> Signed-off-by: Ilya Maximets <[email protected]>
>
> Thanks, Ilya, for the patch and the detailed explanation. The
> change looks good to me; however, I do not have a replicator to
> verify it, so it would be good if Dumitru has one and could give it
> a try.
FWIW, I just found why we don't see these issues in our CI - we check logs
before stopping OVS:
diff --git a/tests/ofproto-macros.at b/tests/ofproto-macros.at
index 7f6ab8904..d924ffdad 100644
--- a/tests/ofproto-macros.at
+++ b/tests/ofproto-macros.at
@@ -381,9 +381,9 @@ m4_divert_pop([PREPARE_TESTS])
#
# OVS_VSWITCHD_STOP(["/expected error/d"])
m4_define([OVS_VSWITCHD_STOP],
- [AT_CHECK([check_logs $1])
- OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
- OVS_APP_EXIT_AND_WAIT([ovsdb-server])])
+ [OVS_APP_EXIT_AND_WAIT([ovs-vswitchd])
+ OVS_APP_EXIT_AND_WAIT([ovsdb-server])
+ AT_CHECK([check_logs $1])])
m4_define([OVS_VSWITCHD_DISABLE_TUNNEL_PUSH_POP],
[AT_CHECK([ovs-appctl dpif/set-dp-features br0 tnl_push_pop false], [0])
---
With the change above I see a bunch of test failures due to the leak check.
They go away with the set applied. OVN tests have a bit different order of
the cleanup sequence so the logs are checked after the sandbox is stopped.
Though I'm still a little confused why OVN tests are not failing much more
often.
There are some other failures in OVS testsuite if we move the check after
the exit. Those need to be addressed before we can change the macro, so
that should be a separate change.
>
> Cheers,
>
> Eelco
>
> Acked-by: Eelco Chaudron <[email protected]>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
