Re: [PATCH 2/3] oslat: Add aarch64 support

[Peter Xu <peterx@redhat.com>]

On Thu, Sep 09, 2021 at 12:10:49PM +0200, nsaenzju@redhat.com wrote:
> On Wed, 2021-09-08 at 14:09 -0400, Peter Xu wrote:
> > On Wed, Sep 08, 2021 at 12:02:08PM +0200, Nicolas Saenz Julienne wrote:
> > > The callbacks are based on Linux's implementation:
> > >  - CNTVCT_EL0 provides direct access to the system virtual timer[1].
> > >  - 'yield' serves as a CPU hint with similar semantics as x86's
> > >    'pause'[2].
> > > 
> > > [1] See Linux's '__arch_get_hw_counter()' in arch/arm64/include/asm/vdso/gettimeofday.h
> > > [2] See Linux's 1baa82f4803 ("arm64: Implement cpu_relax as yield").
> > > Signed-off-by: Nicolas Saenz Julienne <nsaenzju@redhat.com>
> > > ---
> > >  src/oslat/oslat.c | 13 +++++++++++++
> > >  1 file changed, 13 insertions(+)
> > > 
> > > diff --git a/src/oslat/oslat.c b/src/oslat/oslat.c
> > > index a4aa5f1..bd155a6 100644
> > > --- a/src/oslat/oslat.c
> > > +++ b/src/oslat/oslat.c
> > > @@ -71,6 +71,19 @@ static inline void frc(uint64_t *pval)
> > >  {
> > >  	__asm__ __volatile__("mfspr %0, 268\n" : "=r" (*pval));
> > >  }
> > > +# elif defined(__aarch64__)
> > > +#  define relax()          __asm__ __volatile("yield" : : : "memory")
> > > +
> > > +static inline void frc(uint64_t *pval)
> > > +{
> > > +
> > 
> > newline to drop?
> 
> Noted.
> 
> > > +	/*
> > > +	 * This isb() is required to prevent that the counter value
> > > +	 * is speculated.
> > > +	 */
> > > +	__asm__ __volatile__("isb; mrs %0, cntvct_el0" : "=r" (*pval));
> > 
> > I saw that commit 27e11a9fe2e2e added two isbs, one before, one after.  Then
> > commit 77ec462536a1 replaced the 2nd isb into another magic.  This function
> > dropped the 2nd barrier.  Also, the same to compiler barrier "memory" that's
> > gone too.
> > 
> > Is it on purpose to drop them?
> 
> Yes, I removed it on purpose. VDSO's gettimeofday implementation uses a seqlock
> to protect against changes to the counter's properties/state: you want to make
> sure access to the counter register is ordered WRT access to the seqlock
> protecting it. We don't really care for all this, as we trust our counters to
> be stable.

OK, since you've referenced the code, would you mind add these into the commit
message too?

I also don't understand why you explicitly removed the compiler barrier.  IIUC
when without it the compiler could move these instructions to be before/after
other instructions generated in the c code.  That may not really happen in
practise, but just curious why the explicit removal.

-- 
Peter Xu