On Thu, Sep 09, 2021 at 12:10:49PM +0200, nsaenzju@xxxxxxxxxx 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@xxxxxxxxxx>
> > > ---
> > > 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
