| Message ID | 20221208111217.3734461-1-andrzej.hajda@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/2] drm/i915/display: use fetch_and_zero if applicable | expand |
On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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. BR, Jani. > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > --- > drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++++-------- > 1 file changed, 4 insertions(+), 8 deletions(-) > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c > index 907ab7526cb478..2972d7533da44e 100644 > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > @@ -304,10 +304,8 @@ static void i915_digport_work_func(struct work_struct *work) > u32 old_bits = 0; > > spin_lock_irq(&dev_priv->irq_lock); > - long_port_mask = dev_priv->display.hotplug.long_port_mask; > - dev_priv->display.hotplug.long_port_mask = 0; > - short_port_mask = dev_priv->display.hotplug.short_port_mask; > - dev_priv->display.hotplug.short_port_mask = 0; > + long_port_mask = fetch_and_zero(&dev_priv->display.hotplug.long_port_mask); > + short_port_mask = fetch_and_zero(&dev_priv->display.hotplug.short_port_mask); > spin_unlock_irq(&dev_priv->irq_lock); > > for_each_intel_encoder(&dev_priv->drm, encoder) { > @@ -379,10 +377,8 @@ static void i915_hotplug_work_func(struct work_struct *work) > > spin_lock_irq(&dev_priv->irq_lock); > > - hpd_event_bits = dev_priv->display.hotplug.event_bits; > - dev_priv->display.hotplug.event_bits = 0; > - hpd_retry_bits = dev_priv->display.hotplug.retry_bits; > - dev_priv->display.hotplug.retry_bits = 0; > + hpd_event_bits = fetch_and_zero(&dev_priv->display.hotplug.event_bits); > + hpd_retry_bits = fetch_and_zero(&dev_priv->display.hotplug.retry_bits); > > /* Enable polling for connectors which had HPD IRQ storms */ > intel_hpd_irq_storm_switch_to_polling(dev_priv);
On Thu, 08 Dec 2022, Jani Nikula <jani.nikula@linux.intel.com> wrote: > On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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. PS. The origin is commit 78ef2d9abad6 ("drm/i915: Add fetch_and_zero() macro") which presents the idea of making it a pattern that can be extended for atomic use, but six years and counting, that never happened. > > > BR, > Jani. > > >> >> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> >> --- >> drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++++-------- >> 1 file changed, 4 insertions(+), 8 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c >> index 907ab7526cb478..2972d7533da44e 100644 >> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c >> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c >> @@ -304,10 +304,8 @@ static void i915_digport_work_func(struct work_struct *work) >> u32 old_bits = 0; >> >> spin_lock_irq(&dev_priv->irq_lock); >> - long_port_mask = dev_priv->display.hotplug.long_port_mask; >> - dev_priv->display.hotplug.long_port_mask = 0; >> - short_port_mask = dev_priv->display.hotplug.short_port_mask; >> - dev_priv->display.hotplug.short_port_mask = 0; >> + long_port_mask = fetch_and_zero(&dev_priv->display.hotplug.long_port_mask); >> + short_port_mask = fetch_and_zero(&dev_priv->display.hotplug.short_port_mask); >> spin_unlock_irq(&dev_priv->irq_lock); >> >> for_each_intel_encoder(&dev_priv->drm, encoder) { >> @@ -379,10 +377,8 @@ static void i915_hotplug_work_func(struct work_struct *work) >> >> spin_lock_irq(&dev_priv->irq_lock); >> >> - hpd_event_bits = dev_priv->display.hotplug.event_bits; >> - dev_priv->display.hotplug.event_bits = 0; >> - hpd_retry_bits = dev_priv->display.hotplug.retry_bits; >> - dev_priv->display.hotplug.retry_bits = 0; >> + hpd_event_bits = fetch_and_zero(&dev_priv->display.hotplug.event_bits); >> + hpd_retry_bits = fetch_and_zero(&dev_priv->display.hotplug.retry_bits); >> >> /* Enable polling for connectors which had HPD IRQ storms */ >> intel_hpd_irq_storm_switch_to_polling(dev_priv);
On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote: > On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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. > > > BR, > Jani. > > > > > > Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> > > --- > > drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++++-------- > > 1 file changed, 4 insertions(+), 8 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c > > b/drivers/gpu/drm/i915/display/intel_hotplug.c > > index 907ab7526cb478..2972d7533da44e 100644 > > --- a/drivers/gpu/drm/i915/display/intel_hotplug.c > > +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c > > @@ -304,10 +304,8 @@ static void i915_digport_work_func(struct > > work_struct *work) > > u32 old_bits = 0; > > > > spin_lock_irq(&dev_priv->irq_lock); > > - long_port_mask = dev_priv->display.hotplug.long_port_mask; > > - dev_priv->display.hotplug.long_port_mask = 0; > > - short_port_mask = dev_priv- > > >display.hotplug.short_port_mask; > > - dev_priv->display.hotplug.short_port_mask = 0; > > + long_port_mask = fetch_and_zero(&dev_priv- > > >display.hotplug.long_port_mask); > > + short_port_mask = fetch_and_zero(&dev_priv- > > >display.hotplug.short_port_mask); > > spin_unlock_irq(&dev_priv->irq_lock); > > > > for_each_intel_encoder(&dev_priv->drm, encoder) { > > @@ -379,10 +377,8 @@ static void i915_hotplug_work_func(struct > > work_struct *work) > > > > spin_lock_irq(&dev_priv->irq_lock); > > > > - hpd_event_bits = dev_priv->display.hotplug.event_bits; > > - dev_priv->display.hotplug.event_bits = 0; > > - hpd_retry_bits = dev_priv->display.hotplug.retry_bits; > > - dev_priv->display.hotplug.retry_bits = 0; > > + hpd_event_bits = fetch_and_zero(&dev_priv- > > >display.hotplug.event_bits); > > + hpd_retry_bits = fetch_and_zero(&dev_priv- > > >display.hotplug.retry_bits); > > > > /* Enable polling for connectors which had HPD IRQ storms > > */ > > intel_hpd_irq_storm_switch_to_polling(dev_priv); >
On Thu, 08 Dec 2022, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: > On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote: >> On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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) BR, Jani.
On 08.12.2022 14:36, Vivi, Rodrigo wrote: > On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote: >> On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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. > There is about 80 uses of the macro in i915. So I guessed this is accepted solution in i915 :) Moreover it looked to me as a nice shortcut. If not, I can replace it with xchg(ptr, 0), besides tiny overkill, assuming atomicity is not required here, it should work. I can also expand it :) - quite big patch, but cocci should do the work. Anyway I think it would be good to take some decision here, to avoid further confusions. Regards Andrzej >> >> >> BR, >> Jani. >> >> >>> >>> Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> >>> --- >>> drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++++-------- >>> 1 file changed, 4 insertions(+), 8 deletions(-) >>> >>> diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c >>> b/drivers/gpu/drm/i915/display/intel_hotplug.c >>> index 907ab7526cb478..2972d7533da44e 100644 >>> --- a/drivers/gpu/drm/i915/display/intel_hotplug.c >>> +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c >>> @@ -304,10 +304,8 @@ static void i915_digport_work_func(struct >>> work_struct *work) >>> u32 old_bits = 0; >>> >>> spin_lock_irq(&dev_priv->irq_lock); >>> - long_port_mask = dev_priv->display.hotplug.long_port_mask; >>> - dev_priv->display.hotplug.long_port_mask = 0; >>> - short_port_mask = dev_priv- >>>> display.hotplug.short_port_mask; >>> - dev_priv->display.hotplug.short_port_mask = 0; >>> + long_port_mask = fetch_and_zero(&dev_priv- >>>> display.hotplug.long_port_mask); >>> + short_port_mask = fetch_and_zero(&dev_priv- >>>> display.hotplug.short_port_mask); >>> spin_unlock_irq(&dev_priv->irq_lock); >>> >>> for_each_intel_encoder(&dev_priv->drm, encoder) { >>> @@ -379,10 +377,8 @@ static void i915_hotplug_work_func(struct >>> work_struct *work) >>> >>> spin_lock_irq(&dev_priv->irq_lock); >>> >>> - hpd_event_bits = dev_priv->display.hotplug.event_bits; >>> - dev_priv->display.hotplug.event_bits = 0; >>> - hpd_retry_bits = dev_priv->display.hotplug.retry_bits; >>> - dev_priv->display.hotplug.retry_bits = 0; >>> + hpd_event_bits = fetch_and_zero(&dev_priv- >>>> display.hotplug.event_bits); >>> + hpd_retry_bits = fetch_and_zero(&dev_priv- >>>> display.hotplug.retry_bits); >>> >>> /* Enable polling for connectors which had HPD IRQ storms >>> */ >>> intel_hpd_irq_storm_switch_to_polling(dev_priv); >> >
On 08/12/2022 15:02, Jani Nikula wrote: > On Thu, 08 Dec 2022, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: >> On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote: >>> On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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
On 08.12.2022 16:44, Tvrtko Ursulin wrote: > > On 08/12/2022 15:02, Jani Nikula wrote: >> On Thu, 08 Dec 2022, "Vivi, Rodrigo" <rodrigo.vivi@intel.com> wrote: >>> On Thu, 2022-12-08 at 14:32 +0200, Jani Nikula wrote: >>>> On Thu, 08 Dec 2022, Andrzej Hajda <andrzej.hajda@intel.com> 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. Heh, too late, already sent :) > > 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? > Or non-atomic xchg > 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. About 1600 patterns: x = y; y = 0; but I guess there could be more: x = xchg(&y, 0); x = y; ... y = 0; custom macros Regards Andrzej > > Regards, > > Tvrtko
diff --git a/drivers/gpu/drm/i915/display/intel_hotplug.c b/drivers/gpu/drm/i915/display/intel_hotplug.c index 907ab7526cb478..2972d7533da44e 100644 --- a/drivers/gpu/drm/i915/display/intel_hotplug.c +++ b/drivers/gpu/drm/i915/display/intel_hotplug.c @@ -304,10 +304,8 @@ static void i915_digport_work_func(struct work_struct *work) u32 old_bits = 0; spin_lock_irq(&dev_priv->irq_lock); - long_port_mask = dev_priv->display.hotplug.long_port_mask; - dev_priv->display.hotplug.long_port_mask = 0; - short_port_mask = dev_priv->display.hotplug.short_port_mask; - dev_priv->display.hotplug.short_port_mask = 0; + long_port_mask = fetch_and_zero(&dev_priv->display.hotplug.long_port_mask); + short_port_mask = fetch_and_zero(&dev_priv->display.hotplug.short_port_mask); spin_unlock_irq(&dev_priv->irq_lock); for_each_intel_encoder(&dev_priv->drm, encoder) { @@ -379,10 +377,8 @@ static void i915_hotplug_work_func(struct work_struct *work) spin_lock_irq(&dev_priv->irq_lock); - hpd_event_bits = dev_priv->display.hotplug.event_bits; - dev_priv->display.hotplug.event_bits = 0; - hpd_retry_bits = dev_priv->display.hotplug.retry_bits; - dev_priv->display.hotplug.retry_bits = 0; + hpd_event_bits = fetch_and_zero(&dev_priv->display.hotplug.event_bits); + hpd_retry_bits = fetch_and_zero(&dev_priv->display.hotplug.retry_bits); /* Enable polling for connectors which had HPD IRQ storms */ intel_hpd_irq_storm_switch_to_polling(dev_priv);
Simplify the code. Signed-off-by: Andrzej Hajda <andrzej.hajda@intel.com> --- drivers/gpu/drm/i915/display/intel_hotplug.c | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-)