Re: [RFC PATCH] x86/p2m-pt: do type recalculations with p2m read lock

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Apr 03, 2023 at 05:32:39PM +0200, Jan Beulich wrote:
> On 03.04.2023 12:14, Roger Pau Monne wrote:
> > Global p2m type recalculations (as triggered by logdirty) can create
> > so much contention on the p2m lock that simple guest operations like
> > VCPUOP_set_singleshot_timer on guests with a high amount of vCPUs (32)
> > will cease to work in a timely manner, up to the point that Linux
> > kernel versions that sill use the VCPU_SSHOTTMR_future flag with the
> > singleshot timer will cease to work:
> > 
> > [   82.779470] CE: xen increased min_delta_ns to 1000000 nsec
> > [   82.793075] CE: Reprogramming failure. Giving up
> > [   82.779470] CE: Reprogramming failure. Giving up
> > [   82.821864] CE: xen increased min_delta_ns to 506250 nsec
> > [   82.821864] CE: xen increased min_delta_ns to 759375 nsec
> > [   82.821864] CE: xen increased min_delta_ns to 1000000 nsec
> > [   82.821864] CE: Reprogramming failure. Giving up
> > [   82.856256] CE: Reprogramming failure. Giving up
> > [   84.566279] CE: Reprogramming failure. Giving up
> > [   84.649493] Freezing user space processes ...
> > [  130.604032] INFO: rcu_sched detected stalls on CPUs/tasks: { 14} 
> > (detected by 10, t=60002 jiffies, g=4006, c=4005, q=14130)
> > [  130.604032] Task dump for CPU 14:
> > [  130.604032] swapper/14      R  running task        0     0      1 
> > 0x00000000
> > [  130.604032] Call Trace:
> > [  130.604032]  [<ffffffff90160f5d>] ? 
> > rcu_eqs_enter_common.isra.30+0x3d/0xf0
> > [  130.604032]  [<ffffffff907b9bde>] ? default_idle+0x1e/0xd0
> > [  130.604032]  [<ffffffff90039570>] ? arch_cpu_idle+0x20/0xc0
> > [  130.604032]  [<ffffffff9010820a>] ? cpu_startup_entry+0x14a/0x1e0
> > [  130.604032]  [<ffffffff9005d3a7>] ? start_secondary+0x1f7/0x270
> > [  130.604032]  [<ffffffff900000d5>] ? start_cpu+0x5/0x14
> > [  549.654536] INFO: rcu_sched detected stalls on CPUs/tasks: { 26} 
> > (detected by 24, t=60002 jiffies, g=6922, c=6921, q=7013)
> > [  549.655463] Task dump for CPU 26:
> > [  549.655463] swapper/26      R  running task        0     0      1 
> > 0x00000000
> > [  549.655463] Call Trace:
> > [  549.655463]  [<ffffffff90160f5d>] ? 
> > rcu_eqs_enter_common.isra.30+0x3d/0xf0
> > [  549.655463]  [<ffffffff907b9bde>] ? default_idle+0x1e/0xd0
> > [  549.655463]  [<ffffffff90039570>] ? arch_cpu_idle+0x20/0xc0
> > [  549.655463]  [<ffffffff9010820a>] ? cpu_startup_entry+0x14a/0x1e0
> > [  549.655463]  [<ffffffff9005d3a7>] ? start_secondary+0x1f7/0x270
> > [  549.655463]  [<ffffffff900000d5>] ? start_cpu+0x5/0x14
> > [  821.888478] INFO: rcu_sched detected stalls on CPUs/tasks: { 26} 
> > (detected by 24, t=60002 jiffies, g=8499, c=8498, q=7664)
> > [  821.888596] Task dump for CPU 26:
> > [  821.888622] swapper/26      R  running task        0     0      1 
> > 0x00000000
> > [  821.888677] Call Trace:
> > [  821.888712]  [<ffffffff90160f5d>] ? 
> > rcu_eqs_enter_common.isra.30+0x3d/0xf0
> > [  821.888771]  [<ffffffff907b9bde>] ? default_idle+0x1e/0xd0
> > [  821.888818]  [<ffffffff90039570>] ? arch_cpu_idle+0x20/0xc0
> > [  821.888865]  [<ffffffff9010820a>] ? cpu_startup_entry+0x14a/0x1e0
> > [  821.888917]  [<ffffffff9005d3a7>] ? start_secondary+0x1f7/0x270
> > [  821.888966]  [<ffffffff900000d5>] ? start_cpu+0x5/0x14
> > 
> > This is obviously undesirable.  One way to bodge the issue would be to
> > ignore VCPU_SSHOTTMR_future, but that's a deliberate breakage of the
> > hypercall ABI.
> > 
> > Instead lower the contention in the lock by doing the recalculation
> > with the lock in read mode.  This is safe because only the flags/type
> > are changed, there's no PTE mfn change in the AMD recalculation logic.
> > The Intel (EPT) case is likely more complicated, as superpage
> > splitting for diverging EMT values must be done with the p2m lock in
> > taken in write mode.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> > ---
> > I'm unsure whether such modification is fully safe:  I think changing
> > the flags/type should be fine: the PTE write is performed using
> > safwrite_p2m_entry() which must be atomic (as the guest is still
> > running and accessing the page tables).  I'm slightly worried about
> > all PTE readers not using atomic accesses to do so (ie: pointer
> > returned by p2m_find_entry() should be read atomicallly), and code
> > assuming that a gfn type cannot change while holding the p2m lock in
> > read mode.
> 
> Coming back to this: Yes, I think reads (at least the ones in do_recalc()
> which can now be done in parallel) will need to be tightened if this is a
> road we want to follow.

There are likely a lot of reads under the p2m read lock outside of
do_recalc() that will ideally need to be switched to use atomic
accesses also?

I'm open to suggestions to other ways to get this sorted.  And that's
a guest with 'just' 32 vCPUs, as we go up the contention on the p2m
lock during recalcs/misconfigs is going to increase massively.

Thanks, Roger.