On Wed, Mar 16, 2016 at 11:49:32PM -0700, Mike Larkin wrote:
> On Thu, Mar 17, 2016 at 03:38:20PM +0900, Masao Uebayashi wrote:
> > On Wed, Mar 16, 2016 at 11:25:11PM -0700, Mike Larkin wrote:
> > > On Thu, Mar 17, 2016 at 09:44:22AM +0900, Masao Uebayashi wrote:
> > > > Implement delay() using TSC
> > > > 
> > > > - Calculate delay using 64-bit RDTSC instruction
> > > > - Enable tsc_delay() as delay(9) backend
> > > > - Use tsc_delay() only when TSC is invariant
> > > > - Configure tsc_delay() after primary CPU is identified
> > > > - Assume all CPUs are identical
> > > 
> > > Why is this needed, or wanted? There is no explanation given above
> > > aside from "let's make things different". Are you claiming that
> > > delay(9) is somehow inaccurate or incorrect without this?
> > 
> > I think LAPIC delay() is just fine.  Accuracy is not a problem for me.
> > 
> > In the long run, I want to use LAPIC for per-CPU one-shot timer.  This
> > will come later ... but it'd be nice for me to know if using TSC for
> > delay() is not a problem.
> > 
> 
> So are you asking people to just test this to see if there are side
> effects (in order to switch to this "in the long run"), or are you
> asking for oks?
> 
> The way the diff is written, it looks like if it were committed, it
> would force every machine to use TSC. Is that what you want?

I don't need this to be committed & enabled for everyone right now, but
tests and feedbacks are really appreciated.

(Unfortunately using TSC for delay() itself has nothing visible or
beneficial for users.)

> -ml
> 
> > > -ml
> > > 
> > > > 
> > > > diff --git a/sys/arch/amd64/amd64/cpu.c b/sys/arch/amd64/amd64/cpu.c
> > > > index df6c623..0863306 100644
> > > > --- a/sys/arch/amd64/amd64/cpu.c
> > > > +++ b/sys/arch/amd64/amd64/cpu.c
> > > > @@ -330,6 +330,7 @@ cpu_attach(struct device *parent, struct device 
> > > > *self, void *aux)
> > > >         vaddr_t kstack;
> > > >         struct pcb *pcb;
> > > >  #endif
> > > > +       void tsc_delay_init(struct cpu_info *);
> > > >  
> > > >         /*
> > > >          * If we're an Application Processor, allocate a cpu_info
> > > > @@ -409,6 +410,7 @@ cpu_attach(struct device *parent, struct device 
> > > > *self, void *aux)
> > > >  #endif /* MTRR */
> > > >                 cpu_init(ci);
> > > >                 cpu_init_mwait(sc);
> > > > +               tsc_delay_init(ci);
> > > >                 break;
> > > >  
> > > >         case CPU_ROLE_BP:
> > > > @@ -432,6 +434,7 @@ cpu_attach(struct device *parent, struct device 
> > > > *self, void *aux)
> > > >                 ioapic_bsp_id = caa->cpu_number;
> > > >  #endif
> > > >                 cpu_init_mwait(sc);
> > > > +               tsc_delay_init(ci);
> > > >                 break;
> > > >  
> > > >         case CPU_ROLE_AP:
> > > > diff --git a/sys/arch/amd64/amd64/tsc.c b/sys/arch/amd64/amd64/tsc.c
> > > > new file mode 100644
> > > > index 0000000..15242ca
> > > > --- /dev/null
> > > > +++ b/sys/arch/amd64/amd64/tsc.c
> > > > @@ -0,0 +1,68 @@
> > > > +/*
> > > > + * Copyright (c) 2016 Masao Uebayashi <[email protected]>
> > > > + *
> > > > + * Permission to use, copy, modify, and distribute this software for 
> > > > any
> > > > + * purpose with or without fee is hereby granted, provided that the 
> > > > above
> > > > + * copyright notice and this permission notice appear in all copies.
> > > > + *
> > > > + * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL 
> > > > WARRANTIES
> > > > + * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
> > > > + * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE 
> > > > FOR
> > > > + * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY 
> > > > DAMAGES
> > > > + * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN 
> > > > AN
> > > > + * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING 
> > > > OUT OF
> > > > + * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
> > > > + */
> > > > +
> > > > +#include <sys/param.h>
> > > > +#include <sys/systm.h>
> > > > +#include <sys/stdint.h>
> > > > +
> > > > +#include <machine/cpu.h>
> > > > +#include <machine/cpufunc.h>
> > > > +
> > > > +void tsc_delay_init(struct cpu_info *);
> > > > +void tsc_delay(int);
> > > > +uint64_t tsc2usec(uint64_t n);
> > > > +
> > > > +int64_t tsc_delay_mult;
> > > > +
> > > > +void
> > > > +tsc_delay_init(struct cpu_info *ci)
> > > > +{
> > > > +#ifdef DIAGNOSTIC
> > > > +       KASSERT(ci->ci_tsc_freq != 0);
> > > > +#endif
> > > > +       if ((ci->ci_flags & CPUF_CONST_TSC) == 0)
> > > > +               return;
> > > > +
> > > > +       tsc_delay_mult = ci->ci_tsc_freq / 1000000;
> > > > +       delay_func = tsc_delay;
> > > > +}
> > > > +
> > > > +void
> > > > +tsc_delay(int usec)
> > > > +{
> > > > +       int64_t n;
> > > > +       uint64_t now, prev;
> > > > +
> > > > +       n = tsc_delay_mult * usec;
> > > > +       prev = rdtsc();
> > > > +       while (n > 0) {
> > > > +               CPU_BUSY_CYCLE();
> > > > +               now = rdtsc();
> > > > +               if (now < prev)
> > > > +                       n -= UINT64_MAX - (prev - now);
> > > > +               else
> > > > +                       n -= now - prev;
> > > > +               prev = now;
> > > > +       }
> > > > +}
> > > > +
> > > > +uint64_t
> > > > +tsc2usec(uint64_t n)
> > > > +{
> > > > +       struct cpu_info *ci = curcpu();
> > > > +
> > > > +       return n / ci->ci_tsc_freq * 1000000;
> > > > +}
> > > > diff --git a/sys/arch/amd64/conf/files.amd64 
> > > > b/sys/arch/amd64/conf/files.amd64
> > > > index 5d58b34..d101cf2 100644
> > > > --- a/sys/arch/amd64/conf/files.amd64
> > > > +++ b/sys/arch/amd64/conf/files.amd64
> > > > @@ -97,6 +97,7 @@ define        cpu {[apid = -1]}
> > > >  device cpu
> > > >  attach cpu at mainbus
> > > >  file   arch/amd64/amd64/cpu.c  cpu
> > > > +file   arch/amd64/amd64/tsc.c  cpu
> > > >  
> > > >  
> > > >  define lapic
> > > > 
> > 
> 

Reply via email to