| Message ID | 20221209154843.4162814-1-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/5] linux/minmax.h: add non-atomic version of xchg | expand |
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
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?
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?
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)
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
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
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
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
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
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
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)
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
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().
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)
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
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.
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 */
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(+)