Hi Thomas,
On 8/18/19 5:21 PM, Thomas Gleixner wrote:
[..]
> I'm happy to review well written stuff which makes progress and takes
> review comments into account or the submitter discusses them for
> resolution.
Thanks again for both your and Andy time!
[..]
> Coming back to Andy's idea. Create your time namespace page as an exact
> copy of the vdso data page. When the page is created do:
>
> memset(p->vdso_data, 0, sizeof(p->vdso_data));
> p->vdso_data[0].clock_mode = CLOCK_TIMENS;
> p->vdso_data[0].seq = 1;
>
> /* Store the namespace offsets in basetime */
> p->vdso_data[0].basetime[CLOCK_MONOTONIC].sec = myns->mono_sec;
> p->vdso_data[0].basetime[CLOCK_MONOTONIC].nsec = myns->mono_nsec;
> p->vdso_data[0].basetime[CLOCK_BOOTTIME].sec = myns->boot_sec;
> p->vdso_data[0].basetime[CLOCK_BOOTTIME].nsec = myns->boot_nsec;
>
> p->vdso_data[1].clock_mode = CLOCK_TIMENS;
> p->vdso_data[1].seq = 1;
>
> For a normal task the VVAR pages are installed in the normal ordering:
>
> VVAR
> PVCLOCK
> HVCLOCK
> TIMENS <- Not really required
>
> Now for a timens task you install the pages in the following order
>
> TIMENS
> PVCLOCK
> HVCLOCK
> VVAR
>
> The check for vdso_data->clock_mode is in the unlikely path of the now open
> coded seq begin magic. So for the non-timens case most of the time 'seq' is
> even, so the branch is not taken.
>
> If 'seq' is odd, i.e. a concurrent update is in progress, the extra check
> for vdso_data->clock_mode is a non-issue. The task is spin waiting for the
> update to finish and for 'seq' to become even anyway.
>
> Patch below. I tested this with the normal order and by installing a
> 'timens' page unconditionally for all processes. I'll reply with the timens
> testing hacks so you can see what I did.
>
> The test results are pretty good.
>
> Base (upstream) + VDSO patch + timens page
>
> MONO 30ns 30ns 32ns
> REAL 30ns 30ns 32ns
> BOOT 30ns 30ns 32ns
> MONOCOARSE 7ns 8ns 10ns
> REALCOARSE 7ns 8ns 10ns
> TAI 30ns 30ns 32ns
> MONORAW 30ns 30ns 32ns
>
> So except for the coarse clocks there is no change when the timens page is
> not used, i.e. the regular VVAR page is at the proper place. But that's on
> one machine, a different one showed an effect in the noise range. I'm not
> worried about that as the VDSO behaviour varies depending on micro
> architecture anyway.
>
> With timens enabled the performance hit (cache hot microbenchmark) is
> somewhere in the range of 5-7% when looking at the perf counters
> numbers. The hit for the coarse accessors is larger obviously because the
> overhead is constant time.
>
> I did a quick comparison of the array vs. switch case (what you used for
> your clk_to_ns() helper). The switch case is slower.
>
> So I rather go for the array based approach. It's simpler code and the
> I-cache footprint is smaller and no conditional branches involved.
>
> That means your timens_to_host() and host_to_timens() conversion functions
> should just use that special VDSO page and do the same array based
> unconditional add/sub of the clock specific offset.
I was a bit scarred that clock_mode change would result in some complex
logic, but your patch showed me that it's definitely not so black as I
was painting it.
Will rework the patches set with Andrei based on your and Andy's
suggestions and patches.
Thanks,
Dmitry
