Re: [PATCH v1] event/dlb2: race condition during dynamic linking

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 21 Oct 2026 13:20:41 -0400
Pravin Pathak <pravinpathakdev@gmail.com> wrote:

> dlb2 handles dynamic linking and linking asynchronously if queues
> being linked/unlinked have traffic. It starts a thread if not already
> started to handle the process. This change avoids the race condition
> in thread creation where sometimes the thread starts immediately,
> descheduling the calling thread and finishes the job and exits, and then
> caller sets worker launched true when the thread is not running.
> 
> Signed-off-by: Pravin Pathak <pravin.pathak.dev@gmail.com>
> ---

Ran a more detailed AI review on this.

Subject: Re: [PATCH] event/dlb2: fix race condition during dynamic linking

The fix is correct and worth having in stable. Some notes on what it
does and does not cover, since two related problems remain in this
code.

What the patch fixes
--------------------

os_schedule_work() set worker_launched = true after
rte_thread_create_internal_control() returned, but the worker thread
(dlb2_complete_queue_map_unmap()) clears that flag as its last act
before exiting. If the new thread ran to completion before the creator
was scheduled again, the creator stamped true over the worker's false.

The flag is then stuck true for the life of the device:

  - os_worker_active() always reports an active worker
  - all three call sites in dlb2_resource.c (2336, 2370, 6037) are
    guarded by if (!os_worker_active(hw)), so no worker is ever
    launched again
  - deferred map/unmap work is stranded, leaving ports in
    DLB2_QUEUE_MAP_IN_PROG / UNMAP_IN_PROG indefinitely

Setting the flag before thread creation closes that window: creation is
a synchronization point, so the worker is guaranteed to observe true and
its own clear is the last write. Resetting to false on the create-failure
path is right.

Remaining issues
----------------

Both of these predate the patch, so they are follow-up material rather
than objections, but they are in the same few lines and should probably
be cleaned up while the context is fresh.

1. Missed wakeup. resource_mutex is only ever taken inside the worker
loop:

  drivers/event/dlb2/pf/dlb2_main.h:46   rte_spinlock_t resource_mutex
  drivers/event/dlb2/pf/base/dlb2_osdep.h:172,178,190  worker only
  drivers/event/dlb2/pf/dlb2_main.c:34   rte_spinlock_init()

No caller holds it across the check-and-schedule pair, so for one
control thread against the worker:

  - worker's final iteration scans, finds nothing pending
  - control thread marks a new unmap in progress
  - control thread reads worker_launched as true, skips
    os_schedule_work()
  - worker sets worker_launched = false and exits

The work is stranded until the next link/unlink happens to launch a new
worker. This needs no MT-unsafe API use to hit: eventdev link/unlink is
not MT-safe by contract, and a single conforming control thread racing
the worker is enough.

The kernel driver this base code was ported from holds resource_mutex
around the check and the schedule; the DPDK port dropped that. Before
this patch the resulting failure was permanent, now it is transient.

2. worker_launched is a plain bool with mixed access. The worker writes
it under the spinlock, callers read and write it with no
synchronization, which is a C11 data race. The practical impact is
small since the worst case is a duplicate worker and two workers
looping under the same mutex are safe, but it should be
RTE_ATOMIC(bool) with release/acquire.