From: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
Date: Thu, 22 Dec 2022 09:21:47 -0800
On Thu, 22 Dec 2022 12:46:16 +0100 Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
Hi all,
I hope there will be place for such tiny helper in kernel.
Quick cocci analyze shows there is probably few thousands places
where it could be useful.
So to clarify, the intent here is a simple readability cleanup for
existing open-coded exchange operations. The intent is *not* to
identify existing xchg() sites which are unnecessarily atomic and to
optimize them by using the non-atomic version.
Have you considered the latter?
I am not sure who is good person to review/ack such patches,
I can take 'em.
so I've used my intuition to construct to/cc lists, sorry for mistakes.
This is the 2nd approach of the same idea, with comments addressed[0].
The helper is tiny and there are advices we can leave without it, so
I want to present few arguments why it would be good to have it:
1. Code readability/simplification/number of lines:
Real example from drivers/net/ethernet/mellanox/mlx5/core/esw/qos.c:
- previous_min_rate = evport->qos.min_rate;
- evport->qos.min_rate = min_rate;
+ previous_min_rate = __xchg(evport->qos.min_rate, min_rate);
For sure the code is more compact, and IMHO more readable.
2. Presence of similar helpers in other somehow related languages/libs:
a) Rust[1]: 'replace' from std::mem module, there is also 'take'
helper (__xchg(&x, 0)), which is the same as private helper in
i915 - fetch_and_zero, see latest patch.
b) C++ [2]: 'exchange' from utility header.
If the idea is OK there are still 2 qestions to answer:
1. Name of the helper, __xchg follows kernel conventions,
but for me Rust names are also OK.
I like replace(), or, shockingly, exchange().
But... Can we simply make swap() return the previous value?
previous_min_rate = swap(&evport->qos.min_rate, min_rate);
Unforunately, swap()'s arguments get passed by names, not as
pointers, so you can't do
swap(some_ptr, NULL);
-- pretty common pattern for xchg.
Thanks,
Olek
