Re: [PATCH v3] Avoid memory barrier in read_seqcount() through load acquire

Re: [PATCH v3] Avoid memory barrier in read_seqcount() through load acquire

[Thomas Gleixner]

Cc+ i915 people

On Tue, Sep 17 2024 at 08:37, Will Deacon wrote:
> On Mon, Sep 16, 2024 at 10:52:18AM -0700, Christoph Lameter (Ampere) wrote:
>> On Fri, 13 Sep 2024, kernel test robot wrote:
>> 
>> > >> drivers/gpu/drm/i915/gt/intel_tlb.h:21:47: error: macro "seqprop_sequence" requires 2 arguments, but only 1 given
>> 
>> From 15d86bc9589f16947c5fb0f34d2947eacd48f853 Mon Sep 17 00:00:00 2001
>> From: Christoph Lameter <cl@gentwo.org>
>> Date: Mon, 16 Sep 2024 10:44:16 -0700
>> Subject: [PATCH] Update Intel DRM use of seqprop_sequence
>> 
>> One of Intels drivers uses seqprop_sequence() for its tlb sequencing.
>> We added a parameter so that we can use acquire. Its pretty safe to
>> assume that this will work without acquire.
>>
>> Signed-off-by: Christoph Lameter <cl@linux.com>
>> ---
>>  drivers/gpu/drm/i915/gt/intel_tlb.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.h b/drivers/gpu/drm/i915/gt/intel_tlb.h
>> index 337327af92ac..81998c4cd4fb 100644
>> --- a/drivers/gpu/drm/i915/gt/intel_tlb.h
>> +++ b/drivers/gpu/drm/i915/gt/intel_tlb.h
>> @@ -18,7 +18,7 @@ void intel_gt_fini_tlb(struct intel_gt *gt);
>> 
>>  static inline u32 intel_gt_tlb_seqno(const struct intel_gt *gt)
>>  {
>> -	return seqprop_sequence(&gt->tlb.seqno);
>> +	return seqprop_sequence(&gt->tlb.seqno, false);
>>  }
>
> Yikes, why is the driver using the seqlock internals here? It's a bit of
> a pity, as a quick grep suggest that this is the _only_ user of
> 'seqcount_mutex_t', yet it's still having to work around the API.

Why the hell can't i915 use the proper interfaces and has to bypass the
core code? Just because C allows that does not make it correct.

Can the i915 people please remove this blatant violation of layering?

Thanks,

        tglx

Re: [PATCH v3] Avoid memory barrier in read_seqcount() through load acquire

[Vivi, Rodrigo]

On Tue, 2024-09-17 at 13:50 +0200, Thomas Gleixner wrote:
> Cc+ i915 people
> 
> On Tue, Sep 17 2024 at 08:37, Will Deacon wrote:
> > On Mon, Sep 16, 2024 at 10:52:18AM -0700, Christoph Lameter
> > (Ampere) wrote:
> > > On Fri, 13 Sep 2024, kernel test robot wrote:
> > > 
> > > > > > drivers/gpu/drm/i915/gt/intel_tlb.h:21:47: error: macro
> > > > > > "seqprop_sequence" requires 2 arguments, but only 1 given
> > > 
> > > From 15d86bc9589f16947c5fb0f34d2947eacd48f853 Mon Sep 17 00:00:00
> > > 2001
> > > From: Christoph Lameter <cl@gentwo.org>
> > > Date: Mon, 16 Sep 2024 10:44:16 -0700
> > > Subject: [PATCH] Update Intel DRM use of seqprop_sequence
> > > 
> > > One of Intels drivers uses seqprop_sequence() for its tlb
> > > sequencing.
> > > We added a parameter so that we can use acquire. Its pretty safe
> > > to
> > > assume that this will work without acquire.
> > > 
> > > Signed-off-by: Christoph Lameter <cl@linux.com>
> > > ---
> > >  drivers/gpu/drm/i915/gt/intel_tlb.h | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/gpu/drm/i915/gt/intel_tlb.h
> > > b/drivers/gpu/drm/i915/gt/intel_tlb.h
> > > index 337327af92ac..81998c4cd4fb 100644
> > > --- a/drivers/gpu/drm/i915/gt/intel_tlb.h
> > > +++ b/drivers/gpu/drm/i915/gt/intel_tlb.h
> > > @@ -18,7 +18,7 @@ void intel_gt_fini_tlb(struct intel_gt *gt);
> > > 
> > >  static inline u32 intel_gt_tlb_seqno(const struct intel_gt *gt)
> > >  {
> > > -	return seqprop_sequence(&gt->tlb.seqno);
> > > +	return seqprop_sequence(&gt->tlb.seqno, false);
> > >  }
> > 
> > Yikes, why is the driver using the seqlock internals here? It's a
> > bit of
> > a pity, as a quick grep suggest that this is the _only_ user of
> > 'seqcount_mutex_t', yet it's still having to work around the API.
> 
> Why the hell can't i915 use the proper interfaces and has to bypass
> the
> core code? Just because C allows that does not make it correct.
> 
> Can the i915 people please remove this blatant violation of layering?

Yeap, we gotta remove this. Just need to be careful on this TLB
invalidation code without causing some funny deadlocks...

> 
> Thanks,
> 
>         tglx
>