Re: [PATCH v2 03/14] x86/pv: use populate_perdomain_mapping() to map the Xen GDT

[George Dunlap <dunlapg@xxxxxxxxx>]

On Fri, Sep 4, 2026 at 7:54 AM Jürgen Groß <jgross@xxxxxxxx> wrote:
>
> On 04.09.26 08:00, Jan Beulich wrote:
> > On 04.09.2026 00:35, George Dunlap wrote:
> >> On Thu, Sep 3, 2026 at 5:11 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
> >>>
> >>> On 02.09.2026 11:43, George Dunlap wrote:
> >>>> From: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>>>
> >>>> Currently, update_xen_slot_in_full_gdt() uses the stashed direct-map
> >>>> pointer in d->arch.pv.gdt_ldt_l1tab to update the incoming vcpu's
> >>>> page tables with Xen's GDT, by writing a stashed per-cpu copy of a
> >>>> pre-baked L1 entry (either 64-bit or compat version).
> >>>>
> >>>> Switch this to using populate_perdomain_mapping(), which doesn't rely
> >>>> on the stashed address of the l1 page in the direct map.  Rather than
> >>>> also stashing a pre-baked value for the payload, compute the mfn from
> >>>> the per-cpu GDT pointer at use: the conversion is a handful of cycles
> >>>> on a path costing thousands, and computing at use removes the
> >>>> parallel {,compat_}gdt_l1e bookkeeping along with its boot-ordering
> >>>> constraint (the cached value could only be generated after Xen's
> >>>> physical relocation, and had to be in place before the first context
> >>>> switch; a use-time lookup is correct by construction).  The flags on
> >>>> the final mapping are identical.
> >>>>
> >>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >>>> Assisted-by: Claude Code:claude-fable-5, Claude Code:claude-opus-4-8
> >>>> Signed-off-by: George Dunlap <gwd@xxxxxxxxxxxxxx>
> >>>> ---
> >>>> Changes in v2:
> >>>> - Drop the {,compat_}gdt_mfn caching entirely (suggested by Andrew
> >>>>     Cooper): compute virt_to_mfn() from the per-cpu GDT pointer at use.
> >>>>     The PDX lookup behind it measures ~5-10 cycles warm against a
> >>>>     ~1,500-cycle context switch, and this removes the double
> >>>>     bookkeeping and the after-relocation caching constraint.  The
> >>>>     cached-MFN assertion goes with the cache: a use-time computation
> >>>>     from a live pointer needs no staleness check.
> >>>
> >>> This looks to contradict what 564d261687c0 ("x86/ctxt-switch: Document
> >>> and improve GDT handling") used as justification to put in place the
> >>> caching. Also Cc-ing Jürgen, who also was involved there, for possible
> >>> further insight.
> >>>
> >>> Functionally the change looks okay to me, but the above will need
> >>> sorting, at the very least by specifically discussing why effectively
> >>> undoing that earlier change is okay.
> >>
> >> So looking back at the thread, Jürgen measured a 14% improvement for
> >> something that might be described as a microbenchmark before and after
> >> the patch (a benchmark purposely trying to set up an unusual scenario
> >> to maximize the effect of context switch overhead, not one to
> >> represent a typical workflow).  But are the numbers really plausible?
> >> Even at an implausible 100k switches/s across the box, saving 100
> >> cycles per switch is about 0.04% of eight 3 GHz cores.
> >>
> >> At any rate, we're already adding several map/unmap operations, and
> >> about to add several more.  Keeping the PTE caching would require
> >> adding a separate path that can write just PTEs, which then will
> >> potentially further complication future paths where we need to make
> >> sure we handle both domain-wide perdomain areas and per-vcpu areas.
> >> If it were easy I would already have been keeping it.
> >>
> >> I'd be inclined to say: Since we're going to be adding more
> >> populate_perdomain_mapping() calls anyway, let's do it the simple
> >> correct way first; and then explore the idea of stashing mfns of
> >> frequently-mapped L1s (rather than having to walk L3 -> L2 -> L1); and
> >> at that time look into stashing baked l1es to avoid conversions.
> >
> > Perhaps; I'd like to have Jürgen's and/or Andrew's input here, though.
>
> At that time I implemented core scheduling in Xen. I noticed that very
> subtle changes in the context switch path could result in unexpected large
> performance differences. As I had the performance test for my purpose
> already set up, I used it for Andrew's patch (which was a result of my
> context switch path performance findings) and really did measure the
> impressive effect of it.
>
> Note that you can't only count instructions, often cache effects and
> branch predictions are dominating the performance.

Right, but:

1. That's going to be very much hardware- and workload- dependent.
Even on the same hardware, if you'd made a slight change in the
workload, you might have seen a very different result; and on
different hardware you're going to see something different again

2. As I said, we're now adding two extra map / unmaps, which is going
to perturb everything again.

If anything, your argument says we should wait until we've stopped
modifying the context switch path (which won't happen until patch 49
at least, guessing from the patch titles), and then measure things
again to see what's actually slow.

I'm sorry Jan, your position here is really inconsistent:  You wave
away a partial pagetable walk with three map/unmap operations on the
context switch path as something we'll have to do in the interim, and
can optimize later, but are now threatening to make me add in
special-case codepaths and run tests to save a few memory reads and
shifts.

I could put back the mfn caching that was present in v1 of the series
(which Andy said was probably not sufficient, on balance, to make the
duplication involved worth it).  Even that I think isn't really
sensible, but it's not too difficult to do.  To isolate the PTE
caching effect I'd have to write an entire duplicate codepath anyway,
and then try to duplicate Jürgen's test.  I don't think that's really
a reasonable ask at this point in the series.

 -George