Re: [PATCH 1/5] linux/minmax.h: add non-atomic version of xchg

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




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().



[Index of Archives]     [AMD Graphics]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux