On 05/01/2023 13:34, David Laight wrote:
From: Jani Nikula
Sent: 05 January 2023 13:28
On Thu, 05 Jan 2023, Daniel Vetter <daniel@xxxxxxxx> wrote:
On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
From: Andrzej Hajda <andrzej.hajda@xxxxxxxxx>
Sent: 09 December 2022 15:49
The pattern of setting variable with new value and returning old
one is very common in kernel. Usually atomicity of the operation
is not required, so xchg seems to be suboptimal and confusing in
such cases. Since name xchg is already in use and __xchg is used
in architecture code, proposition is to name the macro exchange.
Dunno, if it is non-atomic then two separate assignment statements
is decidedly more obvious and needs less brain cells to process.
Otherwise someone will assume 'something clever' is going on
and the operation is atomic.
Yes, this also my take. The i915 code that uses this to excess is decidely
unreadable imo, and the macro should simply be replaced by open-coded
versions.
Not moved into shared headers where even more people can play funny games
with it.
My stand in i915 has been that the local fetch_and_zero() needs to
go. Either replaced by a common helper in core kernel headers, or open
coded, I personally don't care, but the local version can't stay.
My rationale has been that fetch_and_zero() looks atomic and looks like
it comes from shared headers, but it's neither. It's deceptive. It
started small and harmless, but things like this just proliferate and
get copy-pasted all over the place.
So here we are, with Andrzej looking to add the common helper. And the
same concerns crop up. What should it be called to make it clear that
it's not atomic? Is that possible?
old_value = read_write(variable, new_value);
But two statements are much clearer.
In a later thread there was more discussion on this and some new
suggestions - exchange(), replace() or even take() sound fine to me.
Last one is perhaps most specialized if it implies zeroing, which I at
least assume it does.
All three are distant enough from atomic connotations of xchg. If that
was a concern with __xchg, which I not sure it should be since there is
"prior art" in the kernel for atomic vs non-atomic like set_bit and
__set_bit.
My 2c, regardless of what name, that it is not something which is
strictly needed, but a convenient syntactic sugar. (Exploded line counts
with sometimes single use local variables are a bit meh.) And I am not
really sure that open coding is more readable once the new pattern would
be established. In short, if there can be swap there can be $insert_name
too I guess.
Bonus points if needlessly atomic sites can be converted but identifying
them is probably an exercise for a later phase.
Regards,
Tvrtko
P.S. FWIW my preference are either replace() or __xchg().
