Messages in this thread | | | From | Alexander Lobakin <> | Subject | Re: [PATCH 00/19] Introduce __xchg, non-atomic xchg | Date | Fri, 23 Dec 2022 15:23:00 +0100 |
| |
From: Andrew Morton <akpm@linux-foundation.org> Date: Thu, 22 Dec 2022 09:21:47 -0800
> On Thu, 22 Dec 2022 12:46:16 +0100 Andrzej Hajda <andrzej.hajda@intel.com> 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
| |