Re: [PATCH] seqlock: Use WRITE_ONCE() when updating sequence

On Thu, Dec 19, 2024 at 05:58:03PM +0000, Will Deacon wrote:
> On Thu, Dec 19, 2024 at 09:53:48AM -0800, Paul E. McKenney wrote:
> > On Thu, Dec 19, 2024 at 04:34:55PM +0000, Will Deacon wrote:
> > > On Wed, Dec 18, 2024 at 06:12:41PM +0100, Peter Zijlstra wrote:
> > > > diff --git a/arch/s390/kernel/idle.c b/arch/s390/kernel/idle.c
> > > > index 39cb8d0ae348..8fb7cd75fe62 100644
> > > > --- a/arch/s390/kernel/idle.c
> > > > +++ b/arch/s390/kernel/idle.c
> > > > @@ -45,7 +45,7 @@ void account_idle_time_irq(void)
> > > >  
> > > >  	/* Account time spent with enabled wait psw loaded as idle time. */
> > > >  	WRITE_ONCE(idle->idle_time, READ_ONCE(idle->idle_time) + idle_time);
> > > > -	WRITE_ONCE(idle->idle_count, READ_ONCE(idle->idle_count) + 1);
> > > > +	INC_ONC(idle->idle_count);
> > > 
> > > (nit: drive-by typo)
> > 
> > Heh!  I was clearly building with GCC.  ;-)
> 
> And not for S390 :p

True enough!  ;-)

> > > > diff --git a/include/linux/compiler-gcc.h b/include/linux/compiler-gcc.h
> > > > index c9b58188ec61..77c253e29758 100644
> > > > --- a/include/linux/compiler-gcc.h
> > > > +++ b/include/linux/compiler-gcc.h
> > > > @@ -137,3 +137,8 @@
> > > >  #if GCC_VERSION < 90100
> > > >  #undef __alloc_size__
> > > >  #endif
> > > > +
> > > > +/*
> > > > + * GCC can't properly optimize the real one with volatile on.
> > > > + */
> > > > +#define INC_ONCE(v) (v)++
> > > 
> > > I think I'm missing what we're trying to do here, but how is this a
> > > safe fallback? I'd have thought we'd need the whole READ_ONCE() +
> > > WRITE_ONCE() sequence if the compiler doesn't give us what we need...
> > > 
> > > > diff --git a/include/linux/compiler.h b/include/linux/compiler.h
> > > > index efd43df3a99a..b1b13dac1b9e 100644
> > > > --- a/include/linux/compiler.h
> > > > +++ b/include/linux/compiler.h
> > > > @@ -8,6 +8,10 @@
> > > >  
> > > >  #ifdef __KERNEL__
> > > >  
> > > > +#ifndef INC_ONCE
> > > > +#define INC_ONCE(v)	(*(volatile typeof(v) *)&(v))++
> > > > +#endif
> > > 
> > > ... but I'm honestly not sure what this is aiming to elide. Is this about
> > > having the compiler emit an increment instruction with memory operands
> > > on architectures that support it? If so, what about architectures that
> > > _don't_ support that? We still need to guarantee that the load/store
> > > instructions are single-copy atomic for the type in that case.
> > 
> > Architectures whose increment instructions lack memory operands can still
> > load, increment, then store.  So yes, if two of these run concurrently,
> > you can lose counts when incrementing normal memory.  (Of course, with
> > device memory, you get what you get.)
> 
> ... but can we guarantee that those load and store instructions won't
> now be torn?

The volatile type qualifier must take care of that, or device drivers
no longer work.

Or are you worried about the C-language ++ fallback?  In that case,
as I understand it, this fallback is only there until GCC learns to
generate a increment-to-memory instruction for x86.

> > > Given how hard it can be to get the compiler to break atomicity for
> > > plain accesses, I'm pretty worried about the robustness of this.
> > 
> > Your point being that the current plain C-language postfix ++ is unlikely
> > to be adversely optimized?  If so, although I agree here in 2024,
> > the years pass quickly and increasingly clever compiler optimizations
> > accumulate.
> 
> Right, I think we're in agreement. I'm saying that just because this
> proposed INC_ONCE() macro appears to generate the assembly code we want,
> I don't have even an instinctive feeling that it's deliberate or
> something that we should/can rely upon.

My perhaps naive hope is that GCC updates permit the plain C-language
postfix ++ fallback goes away sooner rather than later.

Or am I still missing your point?

							Thanx, Paul