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

Message ID 20221209154843.4162814-1-andrzej.hajda@intel.com
State New
Headers
Series [1/5] linux/minmax.h: add non-atomic version of xchg |

Commit Message

Andrzej Hajda Dec. 9, 2022, 3:48 p.m. UTC
  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.

Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
---
Hi,

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 used, of course I do not intend to do it :).

I was not sure where to put this macro, I hope near swap definition
is the most suitable place.

Moreover sorry if to/cc is not correct - get_maintainers.pl was
more confused than me, to who address this patch.

Regards
Andrzej
---
 include/linux/minmax.h | 14 ++++++++++++++
 1 file changed, 14 insertions(+)
  

Comments

Arnd Bergmann Dec. 9, 2022, 5:16 p.m. UTC | #1
On Fri, Dec 9, 2022, at 16:48, Andrzej Hajda wrote:
> 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.
>
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>

While I generally don't like type invariant calling conventions
of xchg() and cmpxchg(), having a new function that has a similar
name without being able to tell which one is which from the
name seems more confusing.

Since __xchg() is only used on 11 architectures as an internal
name for the backing of arch_xchg() or arch_xchg_relaxed(),
maybe we can instead rename those to __arch_xchg() and use the
__xchg() name for the new non-atomic version?

> +/**
> + * exchange - set variable pointed by @ptr to @val, return old value
> + * @ptr: pointer to affected variable
> + * @val: value to be written
> + *
> + * This is non-atomic variant of xchg.
> + */
> +#define exchange(ptr, val) ({		\
> +	typeof(ptr) __ptr = ptr;	\
> +	typeof(*__ptr) __t = *__ptr;	\

I think you can better express this using __auto_type than typeof(),
it is now provided by all supported compilers now.

     Arnd
  
Andy Shevchenko Dec. 9, 2022, 6:56 p.m. UTC | #2
On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:
> 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.
> 
> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> ---
> Hi,
> 
> 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 used, of course I do not intend to do it :).
> 
> I was not sure where to put this macro, I hope near swap definition
> is the most suitable place.

Ah, swap() in this context is not the same. minmax.h hosts it because
it's often related to the swap function in the sort-type algorithms.

> Moreover sorry if to/cc is not correct - get_maintainers.pl was
> more confused than me, to who address this patch.

...

>  include/linux/minmax.h | 14 ++++++++++++++

Does it really suit this header? I would expect something else.
Maybe include/linux/non-atomic/xchg.h, dunno.

Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
and related headers? Maybe some ideas can be taken from there?
  
Andy Shevchenko Dec. 9, 2022, 6:58 p.m. UTC | #3
On Fri, Dec 09, 2022 at 08:56:28PM +0200, Andy Shevchenko wrote:
> On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:

...

> > 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 used, of course I do not intend to do it :).
> > 
> > I was not sure where to put this macro, I hope near swap definition
> > is the most suitable place.
> 
> Ah, swap() in this context is not the same. minmax.h hosts it because
> it's often related to the swap function in the sort-type algorithms.
> 
> > Moreover sorry if to/cc is not correct - get_maintainers.pl was
> > more confused than me, to who address this patch.
> 
> ...
> 
> >  include/linux/minmax.h | 14 ++++++++++++++
> 
> Does it really suit this header? I would expect something else.

> Maybe include/linux/non-atomic/xchg.h, dunno.

It may become a candidate to host io-64 non-atomic versions and other
non-atomic generic headers...

> Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> and related headers? Maybe some ideas can be taken from there?
  
David Laight Dec. 12, 2022, 9:38 a.m. UTC | #4
From: Andrzej Hajda <andrzej.hajda@intel.com>
> 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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  
Andrzej Hajda Dec. 13, 2022, 9:28 a.m. UTC | #5
On 09.12.2022 18:16, Arnd Bergmann wrote:
> On Fri, Dec 9, 2022, at 16:48, Andrzej Hajda wrote:
>> 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.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
> 
> While I generally don't like type invariant calling conventions
> of xchg() and cmpxchg(), having a new function that has a similar
> name without being able to tell which one is which from the
> name seems more confusing.
> 
> Since __xchg() is only used on 11 architectures as an internal

Quite big number for 'only' :)

> name for the backing of arch_xchg() or arch_xchg_relaxed(),
> maybe we can instead rename those to __arch_xchg() and use the
> __xchg() name for the new non-atomic version?

I will try, but even compile test will be some challenge, need to find 
cross-compilers for these archs.

Btw exchange is not totally new name, for example C++ uses it [1].

[1]: https://en.cppreference.com/w/cpp/utility/exchange

Regards
Andrzej

> 
>> +/**
>> + * exchange - set variable pointed by @ptr to @val, return old value
>> + * @ptr: pointer to affected variable
>> + * @val: value to be written
>> + *
>> + * This is non-atomic variant of xchg.
>> + */
>> +#define exchange(ptr, val) ({		\
>> +	typeof(ptr) __ptr = ptr;	\
>> +	typeof(*__ptr) __t = *__ptr;	\
> 
> I think you can better express this using __auto_type than typeof(),
> it is now provided by all supported compilers now.
> 
>       Arnd
  
Arnd Bergmann Dec. 13, 2022, 9:35 a.m. UTC | #6
On Tue, Dec 13, 2022, at 10:28, Andrzej Hajda wrote:
> On 09.12.2022 18:16, Arnd Bergmann wrote:
>> name for the backing of arch_xchg() or arch_xchg_relaxed(),
>> maybe we can instead rename those to __arch_xchg() and use the
>> __xchg() name for the new non-atomic version?
>
> I will try, but even compile test will be some challenge, need to find 
> cross-compilers for these archs.

I maintain this set of cross compilers, let me know if you
have problems running them:

https://mirrors.edge.kernel.org/pub/tools/crosstool/

    Arnd
  
Andrzej Hajda Dec. 13, 2022, 10:09 a.m. UTC | #7
On 09.12.2022 19:56, Andy Shevchenko wrote:
> On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:
>> 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.
>>
>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com>
>> ---
>> Hi,
>>
>> 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 used, of course I do not intend to do it :).
>>
>> I was not sure where to put this macro, I hope near swap definition
>> is the most suitable place.
> 
> Ah, swap() in this context is not the same. minmax.h hosts it because
> it's often related to the swap function in the sort-type algorithms. > >> Moreover sorry if to/cc is not correct - get_maintainers.pl was
>> more confused than me, to who address this patch.
> 
> ...
> 
>>   include/linux/minmax.h | 14 ++++++++++++++
> 
> Does it really suit this header? I would expect something else.
> Maybe include/linux/non-atomic/xchg.h, dunno.

non-atomic seems quite strange for me, I would assume everything not in 
atomic is non-atomic, unless explicitly specified.

> 
> Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> and related headers? Maybe some ideas can be taken from there?
> 

Grepping it didn't give any clue.

Looking at 'near' languages just to get an idea (they name the function 
differently):

C++ [1]: exchange and swap are in utility header
Rust[2]: replace and swap are in std::mem module

This is some argument to put them together.

[1]: https://en.cppreference.com/w/cpp/header/utility
[2]: https://doc.rust-lang.org/std/mem/index.html

Regards
Andrzej
  
Andy Shevchenko Dec. 13, 2022, 10:27 a.m. UTC | #8
On Tue, Dec 13, 2022 at 11:09:12AM +0100, Andrzej Hajda wrote:
> On 09.12.2022 19:56, Andy Shevchenko wrote:
> > On Fri, Dec 09, 2022 at 04:48:39PM +0100, Andrzej Hajda wrote:

...

> > > 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 used, of course I do not intend to do it :).
> > > 
> > > I was not sure where to put this macro, I hope near swap definition
> > > is the most suitable place.
> > 
> > Ah, swap() in this context is not the same. minmax.h hosts it because
> > it's often related to the swap function in the sort-type algorithms.

> >> Moreover sorry if to/cc is not correct - get_maintainers.pl was
> > > more confused than me, to who address this patch.

...

> > >   include/linux/minmax.h | 14 ++++++++++++++
> > 
> > Does it really suit this header? I would expect something else.
> > Maybe include/linux/non-atomic/xchg.h, dunno.
> 
> non-atomic seems quite strange for me, I would assume everything not in
> atomic is non-atomic, unless explicitly specified.
> 
> > 
> > Btw, have you looked if Ingo's gigantic series have done anything to cmpxchg.h
> > and related headers? Maybe some ideas can be taken from there?
> 
> Grepping it didn't give any clue.
> 
> Looking at 'near' languages just to get an idea (they name the function
> differently):
> 
> C++ [1]: exchange and swap are in utility header
> Rust[2]: replace and swap are in std::mem module
> 
> This is some argument to put them together.

Again, I left the above part on top of this message, the swap() in Linux kernel
is not related to __xchg() or similar. That said, minmax.h is not a good place
for the latter.

> [1]: https://en.cppreference.com/w/cpp/header/utility
> [2]: https://doc.rust-lang.org/std/mem/index.html
  
Daniel Vetter Jan. 5, 2023, 1:04 p.m. UTC | #9
On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
> From: Andrzej Hajda <andrzej.hajda@intel.com>
> > 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.

I think swap() is a standard idiom in C, this one here just isn't.
-Daniel
  
Jani Nikula Jan. 5, 2023, 1:28 p.m. UTC | #10
On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
>> From: Andrzej Hajda <andrzej.hajda@intel.com>
>> > 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?


BR,
Jani.



>
> I think swap() is a standard idiom in C, this one here just isn't.
> -Daniel
  
David Laight Jan. 5, 2023, 1:34 p.m. UTC | #11
From: Jani Nikula
> Sent: 05 January 2023 13:28
> 
> On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
> >> From: Andrzej Hajda <andrzej.hajda@intel.com>
> >> > 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.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  
Daniel Vetter Jan. 5, 2023, 2:13 p.m. UTC | #12
On Thu, Jan 05, 2023 at 01:34:33PM +0000, David Laight wrote:
> From: Jani Nikula
> > Sent: 05 January 2023 13:28
> > 
> > On Thu, 05 Jan 2023, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
> > >> From: Andrzej Hajda <andrzej.hajda@intel.com>
> > >> > 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.

Yeah the entire "is it atomic or not" is the issue on top here.

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

Yeah this is my point for fetch_and_zero or any of the other proposals.
We're essentially replacing these two lines:

	var = some->pointer->chase;
	some->pointer->chase = NULL;

with a macro. C is verbose, and sometimes painfully so, if the pointer
chase is really to onerous then I think that should be refactored with a
meaningfully locally name variable, not fancy macros wrapped around to
golf a few characters away.

But what about swap() you ask? That one needs a temp variable, and it does
make sense to hide that in a ({}) block in a macro. But for the above two
lines I really don't see a point outside of obfuscated C contexts.
-Daniel
  
Tvrtko Ursulin Jan. 5, 2023, 2:13 p.m. UTC | #13
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@ffwll.ch> wrote:
>>> On Mon, Dec 12, 2022 at 09:38:12AM +0000, David Laight wrote:
>>>> From: Andrzej Hajda <andrzej.hajda@intel.com>
>>>>> 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().
  
David Laight Jan. 5, 2023, 2:41 p.m. UTC | #14
From: Daniel Vetter
> Sent: 05 January 2023 14:13
...
> > > 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.
> 
> Yeah this is my point for fetch_and_zero or any of the other proposals.
> We're essentially replacing these two lines:
> 
> 	var = some->pointer->chase;
> 	some->pointer->chase = NULL;
> 
> with a macro. C is verbose, and sometimes painfully so,

Try ADA or VHDL :-)

> if the pointer
> chase is really to onerous then I think that should be refactored with a
> meaningfully locally name variable, not fancy macros wrapped around to
> golf a few characters away.

Provided 'var' is a local the compiler is pretty likely to only do the
'pointer chase' once.
You can also do:
	var = NULL;
	swap(some->pointer->chase, var);
and get pretty much the same object code.

> But what about swap() you ask? That one needs a temp variable, and it does
> make sense to hide that in a ({}) block in a macro.

Sometimes, but not enough for the 'missed opportunity for swap()'
message. 

> But for the above two
> lines I really don't see a point outside of obfuscated C contexts.

Indeed.

Isn't the suggested __xchg() in one of the 'reserved for implementation'
namespaces - so shouldn't be a function that might be expected to be
actually used.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
  
Daniel Vetter Jan. 5, 2023, 2:57 p.m. UTC | #15
On Thu, Jan 05, 2023 at 02:41:43PM +0000, David Laight wrote:
> From: Daniel Vetter
> > Sent: 05 January 2023 14:13
> ...
> > > > 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.
> > 
> > Yeah this is my point for fetch_and_zero or any of the other proposals.
> > We're essentially replacing these two lines:
> > 
> > 	var = some->pointer->chase;
> > 	some->pointer->chase = NULL;
> > 
> > with a macro. C is verbose, and sometimes painfully so,
> 
> Try ADA or VHDL :-)
> 
> > if the pointer
> > chase is really to onerous then I think that should be refactored with a
> > meaningfully locally name variable, not fancy macros wrapped around to
> > golf a few characters away.
> 
> Provided 'var' is a local the compiler is pretty likely to only do the
> 'pointer chase' once.
> You can also do:
> 	var = NULL;
> 	swap(some->pointer->chase, var);
> and get pretty much the same object code.
> 
> > But what about swap() you ask? That one needs a temp variable, and it does
> > make sense to hide that in a ({}) block in a macro.
> 
> Sometimes, but not enough for the 'missed opportunity for swap()'
> message. 
> 
> > But for the above two
> > lines I really don't see a point outside of obfuscated C contexts.
> 
> Indeed.
> 
> Isn't the suggested __xchg() in one of the 'reserved for implementation'
> namespaces - so shouldn't be a function that might be expected to be
> actually used.

It's more fun, for the atomic functions which don't have the atomic_
prefix in their names, the __ prefixed versions provide the non-atomic
implementation.  This pattern was started with the long * bitops stuff for
managing really big bitmasks.

And I really don't think it's a great function name scheme that we should
proliferate.

The "reserved for implementation" only applies to the standard C library
in userspace, which the kernel doesn't use, so can fairly freely use that
namespace.
-Daniel
  
Mark Rutland Jan. 5, 2023, 4:16 p.m. UTC | #16
On Thu, Jan 05, 2023 at 03:57:25PM +0100, Daniel Vetter wrote:
> It's more fun, for the atomic functions which don't have the atomic_
> prefix in their names, the __ prefixed versions provide the non-atomic
> implementation.  This pattern was started with the long * bitops stuff for
> managing really big bitmasks.
> 
> And I really don't think it's a great function name scheme that we should
> proliferate.

FWIW I agree it's not great, but we're stuck between a rock and a bikeshed
w.r.t. better naming -- it's quite hard to clean that up becuase the atomic_*()
namespace is reserved for atomic_t (and mirrors atomic64_*() and
atomic_long_*()).

We could consider renaming atomic_t to atomic32_t and atomic_*() to
atomic32_*(), which'd free up the atomic_*() namespace for more genral usage
(e.g. allowing us to have atomic_xchg() and xhcg(), with the latter not being
atomic).

Thanks,
Mark.
  

Patch

diff --git a/include/linux/minmax.h b/include/linux/minmax.h
index 5433c08fcc6858..17d48769203bd5 100644
--- a/include/linux/minmax.h
+++ b/include/linux/minmax.h
@@ -144,4 +144,18 @@ 
 #define swap(a, b) \
 	do { typeof(a) __tmp = (a); (a) = (b); (b) = __tmp; } while (0)
 
+/**
+ * exchange - set variable pointed by @ptr to @val, return old value
+ * @ptr: pointer to affected variable
+ * @val: value to be written
+ *
+ * This is non-atomic variant of xchg.
+ */
+#define exchange(ptr, val) ({		\
+	typeof(ptr) __ptr = ptr;	\
+	typeof(*__ptr) __t = *__ptr;	\
+	*(__ptr) = (val);		\
+	__t;				\
+})
+
 #endif	/* _LINUX_MINMAX_H */