lkml.org 
[lkml]   [2022]   [Jun]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: Commit 282d8998e997 (srcu: Prevent expedited GPs and blocking readers from consuming CPU) cause qemu boot slow
From
Date
Hi, Paul

On 2022/6/13 上午2:49, Paul E. McKenney wrote:
> On Sun, Jun 12, 2022 at 07:29:30PM +0200, Paolo Bonzini wrote:
>> On 6/12/22 18:40, Paul E. McKenney wrote:
>>>> Do these reserved memory regions really need to be allocated separately?
>>>> (For example, are they really all non-contiguous? If not, that is, if
>>>> there are a lot of contiguous memory regions, could you sort the IORT
>>>> by address and do one ioctl() for each set of contiguous memory regions?)
>>>>
>>>> Are all of these reserved memory regions set up before init is spawned?
>>>>
>>>> Are all of these reserved memory regions set up while there is only a
>>>> single vCPU up and running?
>>>>
>>>> Is the SRCU grace period really needed in this case? (I freely confess
>>>> to not being all that familiar with KVM.)
>>> Oh, and there was a similar many-requests problem with networking many
>>> years ago. This was solved by adding a new syscall/ioctl()/whatever
>>> that permitted many requests to be presented to the kernel with a single
>>> system call.
>>>
>>> Could a new ioctl() be introduced that requested a large number
>>> of these memory regions in one go so as to make each call to
>>> synchronize_rcu_expedited() cover a useful fraction of your 9000+
>>> requests? Adding a few of the KVM guys on CC for their thoughts.
>> Unfortunately not. Apart from this specific case, in general the calls to
>> KVM_SET_USER_MEMORY_REGION are triggered by writes to I/O registers in the
>> guest, and those writes then map to a ioctl. Typically the guest sets up a
>> device at a time, and each setup step causes a synchronize_srcu()---and
>> expedited at that.
> I was afraid of something like that...
>
>> KVM has two SRCUs:
>>
>> 1) kvm->irq_srcu is hardly relying on the "sleepable" part; it has readers
>> that are very very small, but it needs extremely fast detection of grace
>> periods; see commit 719d93cd5f5c ("kvm/irqchip: Speed up
>> KVM_SET_GSI_ROUTING", 2014-05-05) which split it off kvm->srcu. Readers are
>> not so frequent.
>>
>> 2) kvm->srcu is nastier because there are readers all the time. The
>> read-side critical section are still short-ish, but they need the sleepable
>> part because they access user memory.
> Which one of these two is in play in this case?
>
>> Writers are not frequent per se; the problem is they come in very large
>> bursts when a guest boots. And while the whole boot path overall can be
>> quadratic, O(n) expensive calls to synchronize_srcu() can have a larger
>> impact on runtime than the O(n^2) parts, as demonstrated here.
>>
>> Therefore, we operated on the assumption that the callers of
>> synchronized_srcu_expedited were _anyway_ busy running CPU-bound guest code
>> and the desire was to get past the booting phase as fast as possible. If
>> the guest wants to eat host CPU it can "for(;;)" as much as it wants;
>> therefore, as long as expedited GPs didn't eat CPU *throughout the whole
>> system*, a preemptable busy wait in synchronize_srcu_expedited() were not
>> problematic.
>>
>> This assumptions did match the SRCU code when kvm->srcu and kvm->irq_srcu
>> were was introduced (respectively in 2009 and 2014). But perhaps they do
>> not hold anymore now that each SRCU is not as independent as it used to be
>> in those years, and instead they use workqueues instead?
> The problem was not internal to SRCU, but rather due to the fact
> that kernel live patching (KLP) had problems with the CPU-bound tasks
> resulting from repeated synchronize_rcu_expedited() invocations. So I
> added heuristics to get the occasional sleep in there for KLP's benefit.
> Perhaps these heuristics need to be less aggressive about adding sleep.
>
> These heuristics have these aspects:
>
> 1. The longer readers persist in an expedited SRCU grace period,
> the longer the wait between successive checks of the reader
> state. Roughly speaking, we wait as long as the grace period
> has currently been in effect, capped at ten jiffies.
>
> 2. SRCU grace periods have several phases. We reset so that each
> phase starts by not waiting (new phase, new set of readers,
> so don't penalize this set for the sins of the previous set).
> But once we get to the point of adding delay, we add the
> delay based on the beginning of the full grace period.
>
> Right now, the checking for grace-period length does not allow for the
> possibility that a grace period might start just before the jiffies
> counter gets incremented (because I didn't realize that anyone cared),
> so that is one possible thing to change. I can also allow more no-delay
> checks per SRCU grace-period phase.
>
> Zhangfei, does something like the patch shown below help?
>
> Additional adjustments are likely needed to avoid re-breaking KLP,
> but we have to start somewhere...
>
> Thanx, Paul
>
> ------------------------------------------------------------------------
>
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 50ba70f019dea..6a354368ac1d1 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -513,7 +513,7 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
>
> #define SRCU_INTERVAL 1 // Base delay if no expedited GPs pending.
> #define SRCU_MAX_INTERVAL 10 // Maximum incremental delay from slow readers.
> -#define SRCU_MAX_NODELAY_PHASE 1 // Maximum per-GP-phase consecutive no-delay instances.
> +#define SRCU_MAX_NODELAY_PHASE 3 // Maximum per-GP-phase consecutive no-delay instances.
> #define SRCU_MAX_NODELAY 100 // Maximum consecutive no-delay instances.
>
> /*
> @@ -522,12 +522,18 @@ static bool srcu_readers_active(struct srcu_struct *ssp)
> */
> static unsigned long srcu_get_delay(struct srcu_struct *ssp)
> {
> + unsigned long gpstart;
> + unsigned long j;
> unsigned long jbase = SRCU_INTERVAL;
>
> if (ULONG_CMP_LT(READ_ONCE(ssp->srcu_gp_seq), READ_ONCE(ssp->srcu_gp_seq_needed_exp)))
> jbase = 0;
> - if (rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq)))
> - jbase += jiffies - READ_ONCE(ssp->srcu_gp_start);
> + if (rcu_seq_state(READ_ONCE(ssp->srcu_gp_seq))) {
> + j = jiffies - 1;
> + gpstart = READ_ONCE(ssp->srcu_gp_start);
> + if (time_after(j, gpstart))
> + jbase += j - gpstart;
> + }
> if (!jbase) {
> WRITE_ONCE(ssp->srcu_n_exp_nodelay, READ_ONCE(ssp->srcu_n_exp_nodelay) + 1);
> if (READ_ONCE(ssp->srcu_n_exp_nodelay) > SRCU_MAX_NODELAY_PHASE)
Unfortunately, this patch does not helpful.

Then re-add the debug info.

During the qemu boot
[  232.997667]  __synchronize_srcu loop=1000

[  361.094493]  __synchronize_srcu loop=9000
[  361.094501] Call trace:
[  361.094502]  dump_backtrace+0xe4/0xf0
[  361.094505]  show_stack+0x20/0x70
[  361.094507]  dump_stack_lvl+0x8c/0xb8
[  361.094509]  dump_stack+0x18/0x34
[  361.094511]  __synchronize_srcu+0x120/0x128
[  361.094514]  synchronize_srcu_expedited+0x2c/0x40
[  361.094515]  kvm_swap_active_memslots+0x130/0x198
[  361.094519]  kvm_activate_memslot+0x40/0x68
[  361.094520]  kvm_set_memslot+0x2f8/0x3b0
[  361.094523]  __kvm_set_memory_region+0x2e4/0x438
[  361.094524]  kvm_set_memory_region+0x78/0xb8
[  361.094526]  kvm_vm_ioctl+0x5a0/0x13e0
[  361.094528]  __arm64_sys_ioctl+0xb0/0xf8
[  361.094530]  invoke_syscall+0x4c/0x110
[  361.094533]  el0_svc_common.constprop.0+0x68/0x128
[  361.094536]  do_el0_svc+0x34/0xc0
[  361.094538]  el0_svc+0x30/0x98
[  361.094541]  el0t_64_sync_handler+0xb8/0xc0
[  361.094544]  el0t_64_sync+0x18c/0x190
[  363.942817]  kvm_set_memory_region loop=6000

\
 
 \ /
  Last update: 2022-06-13 05:05    [W:0.330 / U:0.096 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site