| Message ID | 20180711202752.GC3593@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, 2018-07-11 at 13:27 -0700, Paul E. McKenney wrote: > > Something like this, on top of the earlier patch? I am not at all > confident of this patch because there might be other entry/exit > paths I am missing. Plus there might be RCU uses on the arch- > specific patch to and from the guest OS. That was going to be my starting point, yes. With some additional manual review and testing to cover the "not at all confident" part. :) Thanks.
On Wed, Jul 11, 2018 at 09:54:52PM +0100, David Woodhouse wrote: > On Wed, 2018-07-11 at 13:27 -0700, Paul E. McKenney wrote: > > > > Something like this, on top of the earlier patch? I am not at all > > confident of this patch because there might be other entry/exit > > paths I am missing. Plus there might be RCU uses on the arch- > > specific patch to and from the guest OS. > > That was going to be my starting point, yes. With some additional > manual review and testing to cover the "not at all confident" part. :) Please feel free to change as needed, up to and including throwing my patch away and starting over. ;-) Thanx, Paul
On 07/11/2018 10:27 PM, Paul E. McKenney wrote: > On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: >> >> >> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: >>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: >>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: >>>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>>> >>>>> RCU can spend long periods of time waiting for a CPU which is actually in >>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace >>>>> modes, and don't wait for it. >>>>> >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>> >>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle >>>> code. I will reply with a fixed patch. >>>> >>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, >>>> so should be OK for testing, just not for mainline. >>> >>> And here is the updated code that allegedly avoids splatting when run with >>> CONFIG_RCU_EQS_DEBUG. >>> >>> Thoughts? >>> >>> Thanx, Paul >>> >>> ------------------------------------------------------------------------ >>> >>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 >>> Author: David Woodhouse <dwmw@amazon.co.uk> >>> Date: Wed Jul 11 19:01:01 2018 +0100 >>> >>> kvm/x86: Inform RCU of quiescent state when entering guest mode >>> >>> RCU can spend long periods of time waiting for a CPU which is actually in >>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace >>> modes, and don't wait for it. >>> >>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> >>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] >>> >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>> index 0046aa70205a..b0c82f70afa7 100644 >>> --- a/arch/x86/kvm/x86.c >>> +++ b/arch/x86/kvm/x86.c >>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; >>> } >>> >>> + rcu_kvm_enter(); >>> kvm_x86_ops->run(vcpu); >>> + rcu_kvm_exit(); >> >> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around >> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is >> not good enough, we should rather fix that instead of adding another rcu hint. > > Something like this, on top of the earlier patch? I am not at all > confident of this patch because there might be other entry/exit > paths I am missing. Plus there might be RCU uses on the arch-specific > patch to and from the guest OS. > > Thoughts? > If you instrment guest_enter/exit, you should cover all cases and all architectures as far as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular case of long running guests blocking rcu for many seconds. And I am pretty sure that this did help back then.
On Wed, Jul 11, 2018 at 11:11:19PM +0200, Christian Borntraeger wrote: > > > On 07/11/2018 10:27 PM, Paul E. McKenney wrote: > > On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: > >> > >> > >> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: > >>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: > >>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: > >>>>> From: David Woodhouse <dwmw@amazon.co.uk> > >>>>> > >>>>> RCU can spend long periods of time waiting for a CPU which is actually in > >>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > >>>>> modes, and don't wait for it. > >>>>> > >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > >>>> > >>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle > >>>> code. I will reply with a fixed patch. > >>>> > >>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, > >>>> so should be OK for testing, just not for mainline. > >>> > >>> And here is the updated code that allegedly avoids splatting when run with > >>> CONFIG_RCU_EQS_DEBUG. > >>> > >>> Thoughts? > >>> > >>> Thanx, Paul > >>> > >>> ------------------------------------------------------------------------ > >>> > >>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 > >>> Author: David Woodhouse <dwmw@amazon.co.uk> > >>> Date: Wed Jul 11 19:01:01 2018 +0100 > >>> > >>> kvm/x86: Inform RCU of quiescent state when entering guest mode > >>> > >>> RCU can spend long periods of time waiting for a CPU which is actually in > >>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > >>> modes, and don't wait for it. > >>> > >>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > >>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > >>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] > >>> > >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >>> index 0046aa70205a..b0c82f70afa7 100644 > >>> --- a/arch/x86/kvm/x86.c > >>> +++ b/arch/x86/kvm/x86.c > >>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > >>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > >>> } > >>> > >>> + rcu_kvm_enter(); > >>> kvm_x86_ops->run(vcpu); > >>> + rcu_kvm_exit(); > >> > >> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around > >> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is > >> not good enough, we should rather fix that instead of adding another rcu hint. > > > > Something like this, on top of the earlier patch? I am not at all > > confident of this patch because there might be other entry/exit > > paths I am missing. Plus there might be RCU uses on the arch-specific > > patch to and from the guest OS. > > > > Thoughts? > > > > If you instrment guest_enter/exit, you should cover all cases and all architectures as far > as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular > case of long running guests blocking rcu for many seconds. And I am pretty sure that > this did help back then. And my second patch on the email you replied to replaced the only call to rcu_virt_note_context_switch(). So maybe it covers what it needs to, but yes, there might well be things I missed. Let's see what David comes up with. What changed was RCU's reactions to longish grace periods. It used to be very aggressive about forcing the scheduler to do otherwise-unneeded context switches, which became a problem somewhere between v4.9 and v4.15. I therefore reduced the number of such context switches, which in turn caused KVM to tell RCU about quiescent states way too infrequently. The advantage of the rcu_kvm_enter()/rcu_kvm_exit() approach is that it tells RCU of an extended duration in the guest, which means that RCU can ignore the corresponding CPU, which in turn allows the guest to proceed without any RCU-induced interruptions. Does that make sense, or am I missing something? I freely admit to much ignorance of both kvm and s390! ;-) Thanx, Paul
On 07/11/2018 11:32 PM, Paul E. McKenney wrote: > On Wed, Jul 11, 2018 at 11:11:19PM +0200, Christian Borntraeger wrote: >> >> >> On 07/11/2018 10:27 PM, Paul E. McKenney wrote: >>> On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: >>>> >>>> >>>> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: >>>>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: >>>>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: >>>>>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>>>>> >>>>>>> RCU can spend long periods of time waiting for a CPU which is actually in >>>>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace >>>>>>> modes, and don't wait for it. >>>>>>> >>>>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>>>> >>>>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle >>>>>> code. I will reply with a fixed patch. >>>>>> >>>>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, >>>>>> so should be OK for testing, just not for mainline. >>>>> >>>>> And here is the updated code that allegedly avoids splatting when run with >>>>> CONFIG_RCU_EQS_DEBUG. >>>>> >>>>> Thoughts? >>>>> >>>>> Thanx, Paul >>>>> >>>>> ------------------------------------------------------------------------ >>>>> >>>>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 >>>>> Author: David Woodhouse <dwmw@amazon.co.uk> >>>>> Date: Wed Jul 11 19:01:01 2018 +0100 >>>>> >>>>> kvm/x86: Inform RCU of quiescent state when entering guest mode >>>>> >>>>> RCU can spend long periods of time waiting for a CPU which is actually in >>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace >>>>> modes, and don't wait for it. >>>>> >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> >>>>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] >>>>> >>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>>> index 0046aa70205a..b0c82f70afa7 100644 >>>>> --- a/arch/x86/kvm/x86.c >>>>> +++ b/arch/x86/kvm/x86.c >>>>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >>>>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; >>>>> } >>>>> >>>>> + rcu_kvm_enter(); >>>>> kvm_x86_ops->run(vcpu); >>>>> + rcu_kvm_exit(); >>>> >>>> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around >>>> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is >>>> not good enough, we should rather fix that instead of adding another rcu hint. >>> >>> Something like this, on top of the earlier patch? I am not at all >>> confident of this patch because there might be other entry/exit >>> paths I am missing. Plus there might be RCU uses on the arch-specific >>> patch to and from the guest OS. >>> >>> Thoughts? >>> >> >> If you instrment guest_enter/exit, you should cover all cases and all architectures as far >> as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular >> case of long running guests blocking rcu for many seconds. And I am pretty sure that >> this did help back then. > > And my second patch on the email you replied to replaced the only call > to rcu_virt_note_context_switch(). So maybe it covers what it needs to, > but yes, there might well be things I missed. Let's see what David > comes up with. > > What changed was RCU's reactions to longish grace periods. It used to > be very aggressive about forcing the scheduler to do otherwise-unneeded > context switches, which became a problem somewhere between v4.9 and v4.15. > I therefore reduced the number of such context switches, which in turn > caused KVM to tell RCU about quiescent states way too infrequently. You talk about commit bcbfdd01dce5556a952fae84ef16fd0f12525e7b rcu: Make non-preemptive schedule be Tasks RCU quiescent state correct? In fact, then whatever (properly sent) patch comes up should contain a fixes tag. > > The advantage of the rcu_kvm_enter()/rcu_kvm_exit() approach is that > it tells RCU of an extended duration in the guest, which means that > RCU can ignore the corresponding CPU, which in turn allows the guest > to proceed without any RCU-induced interruptions. > > Does that make sense, or am I missing something? I freely admit to > much ignorance of both kvm and s390! ;-) WIth that explanation it makes perfect sense to replace rcu_virt_note_context_switch with rcu_kvm_enter/exit from an rcu performance perspective. I assume that rcu_kvm_enter is not much slower than rcu_virt_note_context_switch? Because we do call it on every guest entry/exit which we might have plenty for ping pong I/O workload.
On Wed, Jul 11, 2018 at 02:32:59PM -0700, Paul E. McKenney wrote: > On Wed, Jul 11, 2018 at 11:11:19PM +0200, Christian Borntraeger wrote: > > > > > > On 07/11/2018 10:27 PM, Paul E. McKenney wrote: > > > On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: > > >> > > >> > > >> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: > > >>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: > > >>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: > > >>>>> From: David Woodhouse <dwmw@amazon.co.uk> > > >>>>> > > >>>>> RCU can spend long periods of time waiting for a CPU which is actually in > > >>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > > >>>>> modes, and don't wait for it. > > >>>>> > > >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > >>>> > > >>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle > > >>>> code. I will reply with a fixed patch. > > >>>> > > >>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, > > >>>> so should be OK for testing, just not for mainline. > > >>> > > >>> And here is the updated code that allegedly avoids splatting when run with > > >>> CONFIG_RCU_EQS_DEBUG. > > >>> > > >>> Thoughts? > > >>> > > >>> Thanx, Paul > > >>> > > >>> ------------------------------------------------------------------------ > > >>> > > >>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 > > >>> Author: David Woodhouse <dwmw@amazon.co.uk> > > >>> Date: Wed Jul 11 19:01:01 2018 +0100 > > >>> > > >>> kvm/x86: Inform RCU of quiescent state when entering guest mode > > >>> > > >>> RCU can spend long periods of time waiting for a CPU which is actually in > > >>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > > >>> modes, and don't wait for it. > > >>> > > >>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > >>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > >>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] > > >>> > > >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > >>> index 0046aa70205a..b0c82f70afa7 100644 > > >>> --- a/arch/x86/kvm/x86.c > > >>> +++ b/arch/x86/kvm/x86.c > > >>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > >>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > > >>> } > > >>> > > >>> + rcu_kvm_enter(); > > >>> kvm_x86_ops->run(vcpu); > > >>> + rcu_kvm_exit(); > > >> > > >> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around > > >> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is > > >> not good enough, we should rather fix that instead of adding another rcu hint. > > > > > > Something like this, on top of the earlier patch? I am not at all > > > confident of this patch because there might be other entry/exit > > > paths I am missing. Plus there might be RCU uses on the arch-specific > > > patch to and from the guest OS. > > > > > > Thoughts? > > > > > > > If you instrment guest_enter/exit, you should cover all cases and all architectures as far > > as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular > > case of long running guests blocking rcu for many seconds. And I am pretty sure that > > this did help back then. > > And my second patch on the email you replied to replaced the only call > to rcu_virt_note_context_switch(). So maybe it covers what it needs to, > but yes, there might well be things I missed. Let's see what David > comes up with. > > What changed was RCU's reactions to longish grace periods. It used to > be very aggressive about forcing the scheduler to do otherwise-unneeded > context switches, which became a problem somewhere between v4.9 and v4.15. > I therefore reduced the number of such context switches, which in turn > caused KVM to tell RCU about quiescent states way too infrequently. > > The advantage of the rcu_kvm_enter()/rcu_kvm_exit() approach is that > it tells RCU of an extended duration in the guest, which means that > RCU can ignore the corresponding CPU, which in turn allows the guest > to proceed without any RCU-induced interruptions. > > Does that make sense, or am I missing something? I freely admit to > much ignorance of both kvm and s390! ;-) But I am getting some rcutorture near misses on the commit that introduces rcu_kvm_enter() and rcu_kvm_exit() to the x86 arch-specific vcpu_enter_guest() function. These near misses occur when running rcutorture scenarios TREE01 and TREE03, and in my -rcu tree rather than the v4.15 version of this patch. Given that I am making pervasive changes to the way that RCU works, it might well be that this commit is an innocent bystander. I will run tests overnight and let you know what comes up. Thanx, Paul
On Wed, Jul 11, 2018 at 11:39:10PM +0200, Christian Borntraeger wrote: > > > On 07/11/2018 11:32 PM, Paul E. McKenney wrote: > > On Wed, Jul 11, 2018 at 11:11:19PM +0200, Christian Borntraeger wrote: > >> > >> > >> On 07/11/2018 10:27 PM, Paul E. McKenney wrote: > >>> On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: > >>>> > >>>> > >>>> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: > >>>>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: > >>>>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: > >>>>>>> From: David Woodhouse <dwmw@amazon.co.uk> > >>>>>>> > >>>>>>> RCU can spend long periods of time waiting for a CPU which is actually in > >>>>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > >>>>>>> modes, and don't wait for it. > >>>>>>> > >>>>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > >>>>>> > >>>>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle > >>>>>> code. I will reply with a fixed patch. > >>>>>> > >>>>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, > >>>>>> so should be OK for testing, just not for mainline. > >>>>> > >>>>> And here is the updated code that allegedly avoids splatting when run with > >>>>> CONFIG_RCU_EQS_DEBUG. > >>>>> > >>>>> Thoughts? > >>>>> > >>>>> Thanx, Paul > >>>>> > >>>>> ------------------------------------------------------------------------ > >>>>> > >>>>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 > >>>>> Author: David Woodhouse <dwmw@amazon.co.uk> > >>>>> Date: Wed Jul 11 19:01:01 2018 +0100 > >>>>> > >>>>> kvm/x86: Inform RCU of quiescent state when entering guest mode > >>>>> > >>>>> RCU can spend long periods of time waiting for a CPU which is actually in > >>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > >>>>> modes, and don't wait for it. > >>>>> > >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > >>>>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > >>>>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] > >>>>> > >>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > >>>>> index 0046aa70205a..b0c82f70afa7 100644 > >>>>> --- a/arch/x86/kvm/x86.c > >>>>> +++ b/arch/x86/kvm/x86.c > >>>>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > >>>>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > >>>>> } > >>>>> > >>>>> + rcu_kvm_enter(); > >>>>> kvm_x86_ops->run(vcpu); > >>>>> + rcu_kvm_exit(); > >>>> > >>>> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around > >>>> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is > >>>> not good enough, we should rather fix that instead of adding another rcu hint. > >>> > >>> Something like this, on top of the earlier patch? I am not at all > >>> confident of this patch because there might be other entry/exit > >>> paths I am missing. Plus there might be RCU uses on the arch-specific > >>> patch to and from the guest OS. > >>> > >>> Thoughts? > >>> > >> > >> If you instrment guest_enter/exit, you should cover all cases and all architectures as far > >> as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular > >> case of long running guests blocking rcu for many seconds. And I am pretty sure that > >> this did help back then. > > > > And my second patch on the email you replied to replaced the only call > > to rcu_virt_note_context_switch(). So maybe it covers what it needs to, > > but yes, there might well be things I missed. Let's see what David > > comes up with. > > > > What changed was RCU's reactions to longish grace periods. It used to > > be very aggressive about forcing the scheduler to do otherwise-unneeded > > context switches, which became a problem somewhere between v4.9 and v4.15. > > I therefore reduced the number of such context switches, which in turn > > caused KVM to tell RCU about quiescent states way too infrequently. > > You talk about > commit bcbfdd01dce5556a952fae84ef16fd0f12525e7b > rcu: Make non-preemptive schedule be Tasks RCU quiescent state > > correct? In fact, then whatever (properly sent) patch comes up should contain > a fixes tag. Not that one, but this one is at least part of the "team": 28053bc72c0e5 ("rcu: Add long-term CPU kicking"). I might need to use "git bisect" to find the most relevant commit... :-/ > > The advantage of the rcu_kvm_enter()/rcu_kvm_exit() approach is that > > it tells RCU of an extended duration in the guest, which means that > > RCU can ignore the corresponding CPU, which in turn allows the guest > > to proceed without any RCU-induced interruptions. > > > > Does that make sense, or am I missing something? I freely admit to > > much ignorance of both kvm and s390! ;-) > > WIth that explanation it makes perfect sense to replace > rcu_virt_note_context_switch with rcu_kvm_enter/exit from an rcu performance > perspective. I assume that rcu_kvm_enter is not much slower than > rcu_virt_note_context_switch? Because we do call it on every guest entry/exit > which we might have plenty for ping pong I/O workload. But is there any way for a guest OS to sneak back out to the hypervisor without executing one of the rcu_kvm_exit() calls? If there is, RCU is broken. Thanx, Paul
On Wed, Jul 11, 2018 at 04:37:27PM -0700, Paul E. McKenney wrote: > On Wed, Jul 11, 2018 at 02:32:59PM -0700, Paul E. McKenney wrote: > > On Wed, Jul 11, 2018 at 11:11:19PM +0200, Christian Borntraeger wrote: > > > > > > > > > On 07/11/2018 10:27 PM, Paul E. McKenney wrote: > > > > On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: > > > >> > > > >> > > > >> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: > > > >>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: > > > >>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: > > > >>>>> From: David Woodhouse <dwmw@amazon.co.uk> > > > >>>>> > > > >>>>> RCU can spend long periods of time waiting for a CPU which is actually in > > > >>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > > > >>>>> modes, and don't wait for it. > > > >>>>> > > > >>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > > >>>> > > > >>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle > > > >>>> code. I will reply with a fixed patch. > > > >>>> > > > >>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, > > > >>>> so should be OK for testing, just not for mainline. > > > >>> > > > >>> And here is the updated code that allegedly avoids splatting when run with > > > >>> CONFIG_RCU_EQS_DEBUG. > > > >>> > > > >>> Thoughts? > > > >>> > > > >>> Thanx, Paul > > > >>> > > > >>> ------------------------------------------------------------------------ > > > >>> > > > >>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 > > > >>> Author: David Woodhouse <dwmw@amazon.co.uk> > > > >>> Date: Wed Jul 11 19:01:01 2018 +0100 > > > >>> > > > >>> kvm/x86: Inform RCU of quiescent state when entering guest mode > > > >>> > > > >>> RCU can spend long periods of time waiting for a CPU which is actually in > > > >>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace > > > >>> modes, and don't wait for it. > > > >>> > > > >>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> > > > >>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> > > > >>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] > > > >>> > > > >>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c > > > >>> index 0046aa70205a..b0c82f70afa7 100644 > > > >>> --- a/arch/x86/kvm/x86.c > > > >>> +++ b/arch/x86/kvm/x86.c > > > >>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) > > > >>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; > > > >>> } > > > >>> > > > >>> + rcu_kvm_enter(); > > > >>> kvm_x86_ops->run(vcpu); > > > >>> + rcu_kvm_exit(); > > > >> > > > >> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around > > > >> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is > > > >> not good enough, we should rather fix that instead of adding another rcu hint. > > > > > > > > Something like this, on top of the earlier patch? I am not at all > > > > confident of this patch because there might be other entry/exit > > > > paths I am missing. Plus there might be RCU uses on the arch-specific > > > > patch to and from the guest OS. > > > > > > > > Thoughts? > > > > > > > > > > If you instrment guest_enter/exit, you should cover all cases and all architectures as far > > > as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular > > > case of long running guests blocking rcu for many seconds. And I am pretty sure that > > > this did help back then. > > > > And my second patch on the email you replied to replaced the only call > > to rcu_virt_note_context_switch(). So maybe it covers what it needs to, > > but yes, there might well be things I missed. Let's see what David > > comes up with. > > > > What changed was RCU's reactions to longish grace periods. It used to > > be very aggressive about forcing the scheduler to do otherwise-unneeded > > context switches, which became a problem somewhere between v4.9 and v4.15. > > I therefore reduced the number of such context switches, which in turn > > caused KVM to tell RCU about quiescent states way too infrequently. > > > > The advantage of the rcu_kvm_enter()/rcu_kvm_exit() approach is that > > it tells RCU of an extended duration in the guest, which means that > > RCU can ignore the corresponding CPU, which in turn allows the guest > > to proceed without any RCU-induced interruptions. > > > > Does that make sense, or am I missing something? I freely admit to > > much ignorance of both kvm and s390! ;-) > > But I am getting some rcutorture near misses on the commit that > introduces rcu_kvm_enter() and rcu_kvm_exit() to the x86 arch-specific > vcpu_enter_guest() function. These near misses occur when running > rcutorture scenarios TREE01 and TREE03, and in my -rcu tree rather > than the v4.15 version of this patch. > > Given that I am making pervasive changes to the way that RCU works, > it might well be that this commit is an innocent bystander. I will > run tests overnight and let you know what comes up. And thus far, the verdict is "intermittent". I am starting a new series with CONFIG_RCU_EQS_DEBUG=y, which should detect any mismatched rcu_kvm_enter()/rcu_kvm_exit() pairs. Thanx, Paul
On 07/12/2018 01:37 AM, Paul E. McKenney wrote: > On Wed, Jul 11, 2018 at 02:32:59PM -0700, Paul E. McKenney wrote: >> On Wed, Jul 11, 2018 at 11:11:19PM +0200, Christian Borntraeger wrote: >>> >>> >>> On 07/11/2018 10:27 PM, Paul E. McKenney wrote: >>>> On Wed, Jul 11, 2018 at 08:39:36PM +0200, Christian Borntraeger wrote: >>>>> >>>>> >>>>> On 07/11/2018 08:36 PM, Paul E. McKenney wrote: >>>>>> On Wed, Jul 11, 2018 at 11:20:53AM -0700, Paul E. McKenney wrote: >>>>>>> On Wed, Jul 11, 2018 at 07:01:01PM +0100, David Woodhouse wrote: >>>>>>>> From: David Woodhouse <dwmw@amazon.co.uk> >>>>>>>> >>>>>>>> RCU can spend long periods of time waiting for a CPU which is actually in >>>>>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace >>>>>>>> modes, and don't wait for it. >>>>>>>> >>>>>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>>>>> >>>>>>> And idiot here forgot about some of the debugging code in RCU's dyntick-idle >>>>>>> code. I will reply with a fixed patch. >>>>>>> >>>>>>> The code below works just fine as long as you don't enable CONFIG_RCU_EQS_DEBUG, >>>>>>> so should be OK for testing, just not for mainline. >>>>>> >>>>>> And here is the updated code that allegedly avoids splatting when run with >>>>>> CONFIG_RCU_EQS_DEBUG. >>>>>> >>>>>> Thoughts? >>>>>> >>>>>> Thanx, Paul >>>>>> >>>>>> ------------------------------------------------------------------------ >>>>>> >>>>>> commit 12cd59e49cf734f907f44b696e2c6e4b46a291c3 >>>>>> Author: David Woodhouse <dwmw@amazon.co.uk> >>>>>> Date: Wed Jul 11 19:01:01 2018 +0100 >>>>>> >>>>>> kvm/x86: Inform RCU of quiescent state when entering guest mode >>>>>> >>>>>> RCU can spend long periods of time waiting for a CPU which is actually in >>>>>> KVM guest mode, entirely pointlessly. Treat it like the idle and userspace >>>>>> modes, and don't wait for it. >>>>>> >>>>>> Signed-off-by: David Woodhouse <dwmw@amazon.co.uk> >>>>>> Signed-off-by: Paul E. McKenney <paulmck@linux.vnet.ibm.com> >>>>>> [ paulmck: Adjust to avoid bad advice I gave to dwmw, avoid WARN_ON()s. ] >>>>>> >>>>>> diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c >>>>>> index 0046aa70205a..b0c82f70afa7 100644 >>>>>> --- a/arch/x86/kvm/x86.c >>>>>> +++ b/arch/x86/kvm/x86.c >>>>>> @@ -7458,7 +7458,9 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) >>>>>> vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; >>>>>> } >>>>>> >>>>>> + rcu_kvm_enter(); >>>>>> kvm_x86_ops->run(vcpu); >>>>>> + rcu_kvm_exit(); >>>>> >>>>> As indicated in my other mail. This is supposed to be handled in the guest_enter|exit_ calls around >>>>> the run function. This would also handle other architectures. So if the guest_enter_irqoff code is >>>>> not good enough, we should rather fix that instead of adding another rcu hint. >>>> >>>> Something like this, on top of the earlier patch? I am not at all >>>> confident of this patch because there might be other entry/exit >>>> paths I am missing. Plus there might be RCU uses on the arch-specific >>>> patch to and from the guest OS. >>>> >>>> Thoughts? >>>> >>> >>> If you instrment guest_enter/exit, you should cover all cases and all architectures as far >>> as I can tell. FWIW, we did this rcu_note thing back then actually handling this particular >>> case of long running guests blocking rcu for many seconds. And I am pretty sure that >>> this did help back then. >> >> And my second patch on the email you replied to replaced the only call >> to rcu_virt_note_context_switch(). So maybe it covers what it needs to, >> but yes, there might well be things I missed. Let's see what David >> comes up with. >> >> What changed was RCU's reactions to longish grace periods. It used to >> be very aggressive about forcing the scheduler to do otherwise-unneeded >> context switches, which became a problem somewhere between v4.9 and v4.15. >> I therefore reduced the number of such context switches, which in turn >> caused KVM to tell RCU about quiescent states way too infrequently. >> >> The advantage of the rcu_kvm_enter()/rcu_kvm_exit() approach is that >> it tells RCU of an extended duration in the guest, which means that >> RCU can ignore the corresponding CPU, which in turn allows the guest >> to proceed without any RCU-induced interruptions. >> >> Does that make sense, or am I missing something? I freely admit to >> much ignorance of both kvm and s390! ;-) > > But I am getting some rcutorture near misses on the commit that > introduces rcu_kvm_enter() and rcu_kvm_exit() to the x86 arch-specific > vcpu_enter_guest() function. These near misses occur when running > rcutorture scenarios TREE01 and TREE03, and in my -rcu tree rather > than the v4.15 version of this patch. > > Given that I am making pervasive changes to the way that RCU works, > it might well be that this commit is an innocent bystander. I will > run tests overnight and let you know what comes up. Is there a single patch that that I can test or do I have to combine all the pieces that are sprinkled in this mail thread?
On Wed, 2018-07-11 at 16:47 -0700, Paul E. McKenney wrote: > > > > What changed was RCU's reactions to longish grace periods. It used to > > > be very aggressive about forcing the scheduler to do otherwise-unneeded > > > context switches, which became a problem somewhere between v4.9 and v4.15. > > > I therefore reduced the number of such context switches, which in turn > > > caused KVM to tell RCU about quiescent states way too infrequently. > > > > You talk about > > commit bcbfdd01dce5556a952fae84ef16fd0f12525e7b > > rcu: Make non-preemptive schedule be Tasks RCU quiescent state > > > > correct? In fact, then whatever (properly sent) patch comes up should contain > > a fixes tag. > > Not that one, but this one is at least part of the "team": > > 28053bc72c0e5 ("rcu: Add long-term CPU kicking"). I might need to use > "git bisect" to find the most relevant commit... :-/ Whichever commit we blame, I suspect the Fixes: tag wants to go on Paul's earlier 'Make need_resched() respond to urgent RCU-QS needs' patch. The rcu_kvm_enter() thing we're talking about now is actually addressing a problem which has existed for much longer — that with NO_HZ_FULL, a CPU might end up in guest mode for an indefinite period of time, with RCU waiting for it to return.
On Thu, 2018-07-12 at 08:21 +0200, Christian Borntraeger wrote: > > Is there a single patch that that I can test or do I have to combine > all the pieces that are sprinkled in this mail thread? I believe it should be just these two commits, and the first is probably optional as the second makes it mostly redundant for the KVM case. Fengguang's 0day bot is watching this tree, so I expect to get success/failure results fairly shortly for various build tests at least. http://git.infradead.org/users/dwmw2/random-2.6.git/shortlog/refs/heads/kvm-rcu As noted, the first (need_resched) patch took our observed RCU latencies down from 5-10 seconds to 160ms, while the second one that we're working on now took it down to 40ms.
On 07/12/2018 10:31 AM, David Woodhouse wrote: > > > On Wed, 2018-07-11 at 16:47 -0700, Paul E. McKenney wrote: >> >>>> What changed was RCU's reactions to longish grace periods. It used to >>>> be very aggressive about forcing the scheduler to do otherwise-unneeded >>>> context switches, which became a problem somewhere between v4.9 and v4.15. >>>> I therefore reduced the number of such context switches, which in turn >>>> caused KVM to tell RCU about quiescent states way too infrequently. >>> >>> You talk about >>> commit bcbfdd01dce5556a952fae84ef16fd0f12525e7b >>> rcu: Make non-preemptive schedule be Tasks RCU quiescent state >>> >>> correct? In fact, then whatever (properly sent) patch comes up should contain >>> a fixes tag. >> >> Not that one, but this one is at least part of the "team": >> >> 28053bc72c0e5 ("rcu: Add long-term CPU kicking"). I might need to use >> "git bisect" to find the most relevant commit... :-/ > > Whichever commit we blame, I suspect the Fixes: tag wants to go on > Paul's earlier 'Make need_resched() respond to urgent RCU-QS needs' > patch. > > The rcu_kvm_enter() thing we're talking about now is actually > addressing a problem which has existed for much longer — that with > NO_HZ_FULL, a CPU might end up in guest mode for an indefinite period > of time, with RCU waiting for it to return. On s390 this seems to add about 10ns (~3%) for a guest exit/rentry microbenchmark mostly due to rcu_eqs_enter and rcu_eqs_exit now being visible in perf samples. The older interface was cheaper.
On Thu, 2018-07-12 at 13:00 +0200, Christian Borntraeger wrote: > > On s390 this seems to add about 10ns (~3%) for a guest exit/rentry > microbenchmark mostly due to rcu_eqs_enter and rcu_eqs_exit now being > visible in perf samples. The older interface was cheaper. Well, the older interface wasn't actually working, which made it moderately suboptimal :) But that is fixed by the first patch of the two, so perhaps the second ought to be conditional on CONFIG_NO_HZ_FULL since it's only really fixing a real bug there? Unless we can contrive some way to do the rcu_eqs_enter/exit only on the paths to/from userspace, and *not* when we loop in the kernel to handle interrupts (etc.) and immediately re-enter the guest without returning? That's somewhat non-trivial though...
diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index b0c82f70afa7..0046aa70205a 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -7458,9 +7458,7 @@ static int vcpu_enter_guest(struct kvm_vcpu *vcpu) vcpu->arch.switch_db_regs &= ~KVM_DEBUGREG_RELOAD; } - rcu_kvm_enter(); kvm_x86_ops->run(vcpu); - rcu_kvm_exit(); /* * Do this here before restoring debug registers on the host. And diff --git a/include/linux/context_tracking.h b/include/linux/context_tracking.h index d05609ad329d..8d2a9d3073ad 100644 --- a/include/linux/context_tracking.h +++ b/include/linux/context_tracking.h @@ -118,12 +118,12 @@ static inline void guest_enter_irqoff(void) * one time slice). Lets treat guest mode as quiescent state, just like * we do with user-mode execution. */ - if (!context_tracking_cpu_is_enabled()) - rcu_virt_note_context_switch(smp_processor_id()); + rcu_kvm_enter(); } static inline void guest_exit_irqoff(void) { + rcu_kvm_exit(); if (context_tracking_is_enabled()) __context_tracking_exit(CONTEXT_GUEST); @@ -143,12 +143,13 @@ static inline void guest_enter_irqoff(void) */ vtime_account_system(current); current->flags |= PF_VCPU; - rcu_virt_note_context_switch(smp_processor_id()); + rcu_kvm_enter(); } static inline void guest_exit_irqoff(void) { /* Flush the guest cputime we spent on the guest */ + rcu_kvm_exit(); vtime_account_system(current); current->flags &= ~PF_VCPU; } diff --git a/include/linux/rcutiny.h b/include/linux/rcutiny.h index 4b2d691e453f..a7aa5b3cfb81 100644 --- a/include/linux/rcutiny.h +++ b/include/linux/rcutiny.h @@ -81,7 +81,6 @@ static inline int rcu_needs_cpu(u64 basemono, u64 *nextevt) * Take advantage of the fact that there is only one CPU, which * allows us to ignore virtualization-based context switches. */ -static inline void rcu_virt_note_context_switch(int cpu) { } static inline void rcu_cpu_stall_reset(void) { } static inline void rcu_idle_enter(void) { } static inline void rcu_idle_exit(void) { } diff --git a/include/linux/rcutree.h b/include/linux/rcutree.h index 48ce58b53ece..62b61e579bb4 100644 --- a/include/linux/rcutree.h +++ b/include/linux/rcutree.h @@ -34,17 +34,6 @@ void rcu_softirq_qs(void); void rcu_note_context_switch(bool preempt); int rcu_needs_cpu(u64 basem, u64 *nextevt); void rcu_cpu_stall_reset(void); - -/* - * Note a virtualization-based context switch. This is simply a - * wrapper around rcu_note_context_switch(), which allows TINY_RCU - * to save a few bytes. The caller must have disabled interrupts. - */ -static inline void rcu_virt_note_context_switch(int cpu) -{ - rcu_note_context_switch(false); -} - void synchronize_rcu_expedited(void); void kfree_call_rcu(struct rcu_head *head, rcu_callback_t func);