Re: [PATCH 0/2] Pflock downgrade & stress tests for pflock/rwlock libraries

[Stephen Hemminger <stephen@networkplumber.org>]

On Wed, 10 Jun 2026 10:11:45 +0100
Eimear Morrissey <eimear.morrissey@huawei.com> wrote:

> Add new downgrade option for pflock. Add stress tests for this &
> by extension the rest of the pflock/rwlock libraries.
> 
> Eimear Morrissey (1):
>   app/test: add stress tests for rwlock and pflock
> 
> Konstantin Ananyev (1):
>   eal/pflock: add API to downgrade from wr to rd lock
> 
>  app/test/meson.build               |   2 +
>  app/test/test_pflock_stress.c      |  76 ++++++
>  app/test/test_rwlock_stress.c      |  59 +++++
>  app/test/test_rwlock_stress_impl.h | 393 +++++++++++++++++++++++++++++
>  lib/eal/include/rte_pflock.h       |  21 ++
>  5 files changed, 551 insertions(+)
>  create mode 100644 app/test/test_pflock_stress.c
>  create mode 100644 app/test/test_rwlock_stress.c
>  create mode 100644 app/test/test_rwlock_stress_impl.h
> 

Interesting idea, lots of feedback from AI. Mostly about the test.

Patch 1/2 (eal/pflock: add API to downgrade from wr to rd lock)

Warning: new public API is not marked __rte_experimental.
New APIs (including static inline) should carry the experimental tag
for at least one release per the ABI policy:

	__rte_experimental
	static inline void
	rte_pflock_write_downgrade(rte_pflock_t *pf)

Warning: new EAL API added without a release notes entry.
Please add a note to doc/guides/rel_notes/release_26_07.rst.

Info: the hunk adds a double blank line before the #ifdef __cplusplus,
and the two atomic calls use different continuation indentation
(one tab vs two). checkpatch will complain about the blank lines.

Patch 2/2 (app/test: add stress tests for rwlock and pflock)

Error: read lock is leaked on reader error paths, hanging the test.
handle_error() with write_lock=false does not unlock, and its comment
claims the lock is "already unlocked by the calling function" -- but
handle_reader_work() calls it while still holding the read lock (both
the array-mismatch path and the counter-changed path), and the
DOWNGRADE_TEST failure path in handle_writer_work() likewise calls it
while holding the downgraded read lock. The leaked reader keeps rd.out
from ever matching, so any writer blocks forever in write_lock() and
rte_eal_wait_lcore() never returns: a detected failure becomes a hang
instead of a test failure. Simplest fix is to have callers unlock
before calling handle_error() and drop the unlock from it entirely;
that also fixes the downgrade path incrementing reader_errors for a
writer thread.

Error: stop flag uses volatile instead of atomics.
"volatile bool stop" is written by workers (handle_error) and the main
lcore and polled by all workers. volatile provides no ordering or
atomicity guarantee; use RTE_ATOMIC(bool) with
rte_atomic_load_explicit/rte_atomic_store_explicit as
test_ring_stress_impl.h does for wrk_cmd.
The volatile on counter and counter_array is unnecessary -- they are
only accessed under the lock, which already provides ordering -- and
it defeats compiler optimization of the 1024-element verify loops.

Warning: DYNAMIC_ROLES does not switch roles.
should_be_writer() is called once in lcore_function() before the loop,
so each thread's role is fixed for the whole run; the flag's stated
purpose ("Threads can switch between reader/writer roles") never
happens. Move the role decision inside the while loop when
DYNAMIC_ROLES is set.

Warning: should_be_writer() assumes the main lcore has index 0.
	unsigned int idx = rte_lcore_index(rte_lcore_id()) - 1;
With --main-lcore set to a non-lowest core, a worker can have index 0,
so idx underflows to UINT_MAX and the reader/writer split no longer
matches num_readers/num_writers. Compute the worker's position by
iterating RTE_LCORE_FOREACH_WORKER or skip the main lcore's index
explicitly.

Warning: trailing alignment attribute placement.
	} __rte_cache_aligned;
on struct lcore_stats and struct rwlock_test_shared must be written as
	struct __rte_cache_aligned lcore_stats {
checkpatch enforces this (required for MSVC).

Info: "max wait" statistic does not measure lock wait time.
acquire_time spans the entire iteration including the verify loops and
the configured reader/writer delays, so for long_hold it reports the
delay, not contention. Either time only the lock call or rename it.

Info: missing space in the summary printf string concatenation:
	"%"PRIu64" writer ops," "total time: ..."
prints "writer ops,total time". Also the first element of
pflock_specific_tests is mis-indented (opening brace at column 0).