[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] xen/vcpu: remove vcpu_set_singleshot_timer flags field
On Wed, Apr 19, 2023 at 11:18:38AM +0200, Jan Beulich wrote: > On 19.04.2023 11:02, Roger Pau Monné wrote: > > On Wed, Apr 19, 2023 at 09:07:41AM +0200, Jan Beulich wrote: > >> On 18.04.2023 17:54, Andrew Cooper wrote: > >>> On 18/04/2023 4:42 pm, Roger Pau Monne wrote: > >>>> The addition of the flags field in the vcpu_set_singleshot_timer in > >>>> 505ef3ea8687 is an ABI breakage, as the size of the structure is > >>>> increased. > >>>> > >>>> Remove such field addition and drop the implementation of the > >>>> VCPU_SSHOTTMR_future flag. If a timer provides an expired timeout > >>>> value just inject the timer interrupt. > >>>> > >>>> Bump the Xen interface version, and keep the flags field and > >>>> VCPU_SSHOTTMR_future available for guests using the old interface. > >>>> > >>>> Note the removal of the field from the vcpu_set_singleshot_timer > >>>> struct allows removing the compat translation of the struct. > >>>> > >>>> Fixes: 505ef3ea8687 ('Add flags field to VCPUOP_set_singlsehot_timer.') > >>>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx> > >>>> Signed-off-by: Roger Pau Monné <roger.pau@xxxxxxxxxx> > >>> > >>> While everything said is true, this isn't the reason to to get rid of > >>> VCPU_SSHOTTMR_future > >>> > >>> It 505ef3ea8687 does appear to have been an ABI break, that's > >>> incidental. It dates from 2007 so whatever we have now is the de-facto > >>> ABI, whether we like it or not. > >>> > >>> The reason to delete this is because it is a monumentality and entirely > >>> stupid idea which should have been rejected outright at the time, and > >>> the only guest we can find which uses it also BUG_ON()'s in response to > >>> -ETIME. > >> > >> The instance in Linux (up to 4.6) that I could find was > >> > >> BUG_ON(ret != 0 && ret != -ETIME); > >> > >> i.e. not really dying when getting back -ETIME. (And if there really was > >> a BUG_ON(ret) somewhere despite setting the flag, it would be a bug there, > >> not something to "fix" in Xen.) I'm afraid I also disagree on "stupid > >> idea" as well as ... > > > > The logic in old Linux is indeed 'fine' in the sense that it doesn't > > hit a BUG_ON. > > > > The problem we are seeing is that when logdirty is enabled on a guest > > with 32vCPUs (and without any kind of logdirty hardware assistance) > > the contention on the p2m lock is so high that by the time > > VCPUOP_set_singleshot_timer has copied the hypercall data from HVM > > context the provided timeout has already expired, leading to: > > > > [ 65.543736] hrtimer: interrupt took 10817714 ns > > [ 65.514171] CE: xen increased min_delta_ns to 150000 nsec > > [ 65.514171] CE: xen increased min_delta_ns to 225000 nsec > > [ 65.514171] CE: xen increased min_delta_ns to 337500 nsec > > [ 65.566495] CE: xen increased min_delta_ns to 150000 nsec > > [ 65.514171] CE: xen increased min_delta_ns to 506250 nsec > > [ 65.573088] CE: xen increased min_delta_ns to 150000 nsec > > [ 65.572884] CE: xen increased min_delta_ns to 150000 nsec > > [ 65.514171] CE: xen increased min_delta_ns to 759375 nsec > > [ 65.638644] CE: xen increased min_delta_ns to 150000 nsec > > [ 65.566495] CE: xen increased min_delta_ns to 225000 nsec > > [ 65.514171] CE: xen increased min_delta_ns to 1000000 nsec > > [ 65.572884] CE: xen increased min_delta_ns to 225000 nsec > > [ 65.573088] CE: xen increased min_delta_ns to 225000 nsec > > [ 65.630224] CE: xen increased min_delta_ns to 150000 nsec > > ... > > > > xenrt1062821 login: [ 82.752788] CE: Reprogramming failure. Giving up > > [ 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 > > > > At some point Linux simply gives up trying to reprogram the timer, and > > that obviously lead to CPU stalls. > > And that's all with old enough Linux then, I suppose? That's Linux 3.10. > > Ignoring the VCPU_SSHOTTMR_future flag allows the guest to survive, by > > not returning ETIME and just injecting the expired interrupt. > > > > Overall I'm not sure how useful VCPU_SSHOTTMR_future is at least when > > implemented as done currently in Linux. > > > > Instead of trying to reprogram the timer Linux should do the > > equivalent of self-inject a timer interrupt in order to cope with the > > fact that the selected timeout has already expired. > > Indeed - that's what I was expecting would be happening. But I didn't > go check their code ... Yet them getting it wrong still isn't a reason > to ignore the request, at least not unconditionally. OSes could be > getting it right, and they could then benefit from the avoided event. Well, the reason to ignore would be because the introduction of the flags field and the VCPU_SSHOTTMR_future option did break the ABI. If we care about that behavior we should introduce a new hypercall, either that behaves in such way, or that has a flags field in order to implement it. > As to "unconditionally": Introducing a per-guest control is likely too > much overhead for something that, aiui, isn't commonly used (anymore). No, I don't think any guest I've looked at (Linux, NetBSD, FreeBSD) use the VCPU_SSHOTTMR_future flag. > But tying this to a command line option might make sense - engaging it > shouldn't (hopefully) lead to misbehavior in guests properly using the > flag, so ought to be okay to enable in a system-wide manner. I personally don't think we should go to those lengths in order to keep this behavior, because ignoring VCPU_SSHOTTMR_future (and thus never returning -ENOTIME) is compatible with the current implementation. The Linux implementation shows that even it's current/past user didn't know how the flag should be used anyway. As said above, if there's a willingness to have this behavior (which based on the current public implementations it seems it's not) it can always be implemented as a separate hypercall. Thanks, Roger.
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |