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

[nsaenzju@redhat.com]

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.

-- 
Nicolás Sáenz