Re: [PATCH 8/8] locking/atomics: Use read-write instrumentation for atomic RMWs

[Mark Rutland]

Hi,

Sorry to come to this rather late -- this comment equally applies to v2
so I'm replying here to have context.

On Wed, Jul 22, 2020 at 12:11:18PM +0200, Marco Elver wrote:
> On Tue, 21 Jul 2020 at 16:19, Peter Zijlstra <peterz@xxxxxxxxxxxxx> wrote:
> >
> > On Tue, Jul 21, 2020 at 12:30:16PM +0200, Marco Elver wrote:
> >
> > > diff --git a/scripts/atomic/gen-atomic-instrumented.sh b/scripts/atomic/gen-atomic-instrumented.sh
> > > index 6afadf73da17..5cdcce703660 100755
> > > --- a/scripts/atomic/gen-atomic-instrumented.sh
> > > +++ b/scripts/atomic/gen-atomic-instrumented.sh
> > > @@ -5,9 +5,10 @@ ATOMICDIR=$(dirname $0)
> > >
> > >  . ${ATOMICDIR}/atomic-tbl.sh
> > >
> > > -#gen_param_check(arg)
> > > +#gen_param_check(meta, arg)
> > >  gen_param_check()
> > >  {
> > > +     local meta="$1"; shift
> > >       local arg="$1"; shift
> > >       local type="${arg%%:*}"
> > >       local name="$(gen_param_name "${arg}")"
> > > @@ -17,17 +18,24 @@ gen_param_check()
> > >       i) return;;
> > >       esac
> > >
> > > -     # We don't write to constant parameters
> > > -     [ ${type#c} != ${type} ] && rw="read"
> > > +     if [ ${type#c} != ${type} ]; then
> > > +             # We don't write to constant parameters
> > > +             rw="read"
> > > +     elif [ "${meta}" != "s" ]; then
> > > +             # Atomic RMW
> > > +             rw="read_write"
> > > +     fi
> >
> > If we have meta, should we then not be consistent and use it for read
> > too? Mark?
> 
> gen_param_check seems to want to generate an 'instrument_' check per
> pointer argument. So if we have 1 argument that is a constant pointer,
> and one that isn't, it should generate different instrumentation for
> each. By checking the argument type, we get that behaviour. Although
> we are making the assumption that if meta indicates it's not a 's'tore
> (with void return), it's always a read-write access on all non-const
> pointers.
> 
> Switching over to checking only meta would always generate the same
> 'instrument_' call for each argument. Although right now that would
> seem to work because we don't yet have an atomic that accepts a
> constant pointer and a non-const one.
> 
> Preferences?

Given the only non-rmw cases use the 'l' and 's' meta values, and those
only have a single argument, I reckon it's preferable to special-case
those specifically, e.g.

	case "{meta}" in
	l) rw="read";;	
	s) rw="write";;
	*) rw="read_write";;
	esac

... then we can rework that in future if we ever need to handle multiple
atomic variables that have distinct r/w/rw access types.

Thanks,
Mark.