Re: [PATCH v2 3/4] x86, mwaitt: introduce mwaix delay with a configurable timer

[Peter Zijlstra]

On Tue, 2015-06-09 at 09:46 -0700, Andy Lutomirski wrote:
> On Jun 9, 2015 2:30 AM, "Peter Zijlstra" <peterz@xxxxxxxxxxxxx> wrote:

> > How about you think instead and do something like:
> >
> >         rdtsc(start);
> >         rdtsc_barrier();
> 
> Other way around.  We really need a function static inline u64
> rdtsc_with_barrier().

So admittedly I have not actually looked at how the tsc barrier stuff
works, but what? We don't care if the rdtsc goes up, we just want to
make sure its done before going further.

> 
> >
> >         for (;;) {
> >                 delay = min(MWAIT_MAX_LOOPS, loops);
> >
> >                 __monitorx(&addr, 0, 0);
> >                 mwaitx(delay, true);
> 
> I don't like this hack.  The compiler is entirely within is rights to
> poke addr's cacheline (i.e. the stack) between the two instructions.
> I'd suggest either making the thing a full cacheline long or using a
> single asm statement.
> 
> Also, "addr" is a bad name for a dummy variable that isn't an address
> at all.  How about "dummy"?

Sure, and I like your question on why monitorx exists at all. But none
of that was the point here, the main point being that if loops was too
big, we should do multiple mwaitx invocations, not punt and busy loop.

> >                 rdtsc_barrier();
> >                 rdtsc(end);
> >                 rdtsc_barrier();
> 
> The second barrier is unnecessary.

By virtue of the address dependency?

> >
> >                 loops -= end - start;
> >                 if (loops <= 0)
> >                         break;
> >
> >                 start = end;
> >         }

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/