Re: [PATCH 1/2] drm/i915/display: use fetch_and_zero if applicable

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

 




On 08/12/2022 15:02, Jani Nikula wrote:
On Thu, 08 Dec 2022, "Vivi, Rodrigo" <rodrigo.vivi@xxxxxxxxx> wrote:
On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote:
On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@xxxxxxxxx> wrote:
Simplify the code.

Personally, I absolutely hate fetch_and_zero().

I understand the point, but there are two main traps:

First, the name implies atomicity, which there is none at all.

Second, the name implies it's part of a kernel core header, which it
isn't, and this just amplifies the first point.

It's surprising and misleading, and those are not things I like about
interfaces in the kernel.

I would not like to see this proliferate. If fetch_and_zero() was
atomic
*and* part of a core kernel header, it would be a different matter.
But
I don't think that's going to happen, exactly because it won't be
atomic
and the name implies it is.

+1 here.

Please let's go the other way around and try to kill macros like this.

we either kill or we ensure this gets accepted in the core kernel
libraries.

Agreed. I'd be fine with either:

1) Get something like this accepted in core kernel headers:

#define fetch_and_zero(ptr) xchg(ptr, 0)

2) Do this in i915:

@@
expression E;
@@

- fetch_and_zero(E)
+ xchg(E, 0)

We don't need atomic so both solution would IMO be bad.

We could propose __fetch_and_zero and fetch_and_zero, to mimic __set_bit/set_bit&co for some consistency in terms of atomic vs non-atomic API flavour?

Assuming of course people will think that the long-ish name of the utility macro brings an overall positive cost benefit.

Worth a try I guess.

First step I think we need a cocci script for finding the open coded "fetch and zero" pattern. Not my forte but I can try if no one else has an immediate solution or desire to drive the attempt.

Regards,

Tvrtko



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

  Powered by Linux