| Message ID | 20171214154518.GX910@cbox (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 12/14/2017 11:45 PM, Christoffer Dall Wrote: > On Thu, Dec 14, 2017 at 11:28:04PM +0800, Jia He wrote: >> On 12/14/2017 9:09 PM, Christoffer Dall Wrote: >>> On Thu, Dec 14, 2017 at 12:57:54PM +0800, Jia He wrote: >>> Hi Jia, >>> >>>> I have tried your newer level-mapped-v7 branch, but bug is still there. >>>> >>>> There is no special load in both host and guest. The guest (kernel >>>> 4.14) is often hanging when booting >>>> >>>> the guest kernel log >>>> >>>> [ OK ] Reached target Remote File Systems. >>>> Starting File System Check on /dev/mapper/fedora-root... >>>> [ OK ] Started File System Check on /dev/mapper/fedora-root. >>>> Mounting /sysroot... >>>> [ 2.670764] SGI XFS with ACLs, security attributes, no debug enabled >>>> [ 2.678180] XFS (dm-0): Mounting V5 Filesystem >>>> [ 2.740364] XFS (dm-0): Ending clean mount >>>> [ OK ] Mounted /sysroot. >>>> [ OK ] Reached target Initrd Root File System. >>>> Starting Reload Configuration from the Real Root... >>>> [ 61.288215] INFO: rcu_sched detected stalls on CPUs/tasks: >>>> [ 61.290791] 1-...!: (0 ticks this GP) idle=574/0/0 softirq=5/5 fqs=1 >>>> [ 61.293664] (detected by 0, t=6002 jiffies, g=-263, c=-264, q=39760) >>>> [ 61.296480] Task dump for CPU 1: >>>> [ 61.297938] swapper/1 R running task 0 0 1 0x00000020 >>>> [ 61.300643] Call trace: >>>> [ 61.301260] __switch_to+0x6c/0x78 >>>> [ 61.302095] cpu_number+0x0/0x8 >>>> [ 61.302867] rcu_sched kthread starved for 6000 jiffies! >>>> g18446744073709551353 c18446744073709551352 f0x0 RCU_GP_WAIT_FQS(3) >>>> ->state=0x402 ->cpu=1 >>>> [ 61.305941] rcu_sched I 0 8 2 0x00000020 >>>> [ 61.307250] Call trace: >>>> [ 61.307854] __switch_to+0x6c/0x78 >>>> [ 61.308693] __schedule+0x268/0x8f0 >>>> [ 61.309545] schedule+0x2c/0x88 >>>> [ 61.310325] schedule_timeout+0x84/0x3b8 >>>> [ 61.311278] rcu_gp_kthread+0x4d4/0x7d8 >>>> [ 61.312213] kthread+0x134/0x138 >>>> [ 61.313001] ret_from_fork+0x10/0x1c >>>> >>>> Maybe my previous patch is not perfect enough, thanks for your comments. >>>> >>>> I digged it futher more, do you think below code logic is possibly >>>> problematic? >>>> >>>> >>>> vtimer_save_state (vtimer->loaded = false, cntv_ctl is 0) >>>> >>>> kvm_arch_timer_handler (read cntv_ctl and set vtimer->cnt_ctl = 0) >>>> >>>> vtimer_restore_state (write vtimer->cnt_ctl to cntv_ctl, >>>> then cntv_ctl will >>>> >>>> be 0 forever) >>>> >>>> >>>> If above analysis is reasonable >>> Yes, I think there's something there if the hardware doesn't retire the >>> signal fast enough... >>> >>>> how about below patch? already >>>> tested in my arm64 server. >>>> >>>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c >>>> index f9555b1..ee6dd3f 100644 >>>> --- a/virt/kvm/arm/arch_timer.c >>>> +++ b/virt/kvm/arm/arch_timer.c >>>> @@ -99,7 +99,7 @@ static irqreturn_t kvm_arch_timer_handler(int irq, >>>> void *dev_id) >>>> } >>>> vtimer = vcpu_vtimer(vcpu); >>>> >>>> - if (!vtimer->irq.level) { >>>> + if (vtimer->loaded && !vtimer->irq.level) { >>>> vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); >>>> if (kvm_timer_irq_can_fire(vtimer)) >>>> kvm_timer_update_irq(vcpu, true, vtimer); >>>> >>> There's nothing really wrong with that patch, I just didn't think it >>> would be necessary, as we really shouldn't see interrupts if the timer >>> is not loaded. Can you confirm that a WARN_ON(!vtimer->loaded) in >>> kvm_arch_timer_handler() gives you a splat? >> Please see the WARN_ON result (without my patch) >> [ 72.171706] WARNING: CPU: 24 PID: 1768 at >> arch/arm64/kvm/../../../virt/kvm/arm/arch_timer.c:101 >> kvm_arch_timer_handler+0xc0/0xc8 >> >>> Also, could you give the following a try (without your patch): >>> >>> diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c >>> index 73d262c4712b..4751255345d1 100644 >>> --- a/virt/kvm/arm/arch_timer.c >>> +++ b/virt/kvm/arm/arch_timer.c >>> @@ -367,6 +367,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) >>> /* Disable the virtual timer */ >>> write_sysreg_el0(0, cntv_ctl); >>> + isb(); >> No luck， the bug is still there >> > ok, so this is a slightly different approach to what you were trying to > do. Can you please give this a try and let me know how it goes? > This patch fixes the bug in our platform. > diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c > index 73d262c4712b..544ed15fbbb3 100644 > --- a/virt/kvm/arm/arch_timer.c > +++ b/virt/kvm/arm/arch_timer.c > @@ -46,7 +46,7 @@ static const struct kvm_irq_level default_vtimer_irq = { > .level = 1, > }; > > -static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx); > +static bool kvm_timer_irq_can_fire(u32 cnt_ctl); > static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level, > struct arch_timer_context *timer_ctx); > static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx); > @@ -94,6 +94,7 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) > { > struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; > struct arch_timer_context *vtimer; > + u32 cnt_ctl; > > if (!vcpu) { > pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n"); > @@ -101,8 +102,8 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) > } > vtimer = vcpu_vtimer(vcpu); > > - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); > - if (kvm_timer_irq_can_fire(vtimer)) > + cnt_ctl = read_sysreg_el0(cntv_ctl); > + if (kvm_timer_irq_can_fire(cnt_ctl)) > kvm_timer_update_irq(vcpu, true, vtimer); IIUC, your patch makes kvm_arch_timer_handler never changesvtimer->cnt_ctl > > if (unlikely(!irqchip_in_kernel(vcpu->kvm))) > @@ -148,10 +149,10 @@ static u64 kvm_timer_compute_delta(struct arch_timer_context *timer_ctx) > return 0; > } > > -static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx) > +static bool kvm_timer_irq_can_fire(u32 cnt_ctl) > { > - return !(timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) && > - (timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_ENABLE); > + return !(cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) && > + (cnt_ctl & ARCH_TIMER_CTRL_ENABLE); > } > > /* > @@ -164,10 +165,10 @@ static u64 kvm_timer_earliest_exp(struct kvm_vcpu *vcpu) > struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); > struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); > > - if (kvm_timer_irq_can_fire(vtimer)) > + if (kvm_timer_irq_can_fire(vtimer->cnt_ctl)) > min_virt = kvm_timer_compute_delta(vtimer); > > - if (kvm_timer_irq_can_fire(ptimer)) > + if (kvm_timer_irq_can_fire(ptimer->cnt_ctl)) > min_phys = kvm_timer_compute_delta(ptimer); > > /* If none of timers can fire, then return 0 */ > @@ -231,7 +232,7 @@ static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx) > { > u64 cval, now; > > - if (!kvm_timer_irq_can_fire(timer_ctx)) > + if (!kvm_timer_irq_can_fire(timer_ctx->cnt_ctl)) > return false; > > cval = timer_ctx->cnt_cval; > @@ -306,7 +307,7 @@ static void phys_timer_emulate(struct kvm_vcpu *vcpu) > * don't need to have a soft timer scheduled for the future. If the > * timer cannot fire at all, then we also don't need a soft timer. > */ > - if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer)) { > + if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer->cnt_ctl)) { > soft_timer_cancel(&timer->phys_timer, NULL); > return; > } > @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) > > /* Disable the virtual timer */ > write_sysreg_el0(0, cntv_ctl); > + isb(); My only concern is whether this isb() is required here? Sorryif this is a stupid question.I understand little about arm arch memory barrier. But seems isb will flush all the instruction prefetch.Do you think if an timer interrupt irq arrives, arm will use the previous instruction prefetch? Cheers, Jia
On 15/12/17 02:27, Jia He wrote: > > [...] >> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) >> >> /* Disable the virtual timer */ >> write_sysreg_el0(0, cntv_ctl); >> + isb(); > My only concern is whether this isb() is required here? > Sorryif this is a stupid question.I understand little about arm arch > memory barrier. But seems isb will flush all the instruction prefetch.Do > you think if an timer interrupt irq arrives, arm will use the previous > instruction prefetch? This barrier has little to do with prefetch. It just guarantees that the code after the isb() is now running with a disabled virtual timer. Otherwise, a CPU can freely reorder the write_sysreg() until the next context synchronization event. An interrupt coming between the write and the barrier will also act as a context synchronization event. For more details, see the ARMv8 ARM (the glossary has a section on the concept). Thanks, M.
On Fri, Dec 15, 2017 at 09:09:05AM +0000, Marc Zyngier wrote: > On 15/12/17 02:27, Jia He wrote: > > > > > > [...] > > >> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) > >> > >> /* Disable the virtual timer */ > >> write_sysreg_el0(0, cntv_ctl); > >> + isb(); > > My only concern is whether this isb() is required here? > > Sorryif this is a stupid question.I understand little about arm arch > > memory barrier. But seems isb will flush all the instruction prefetch.Do > > you think if an timer interrupt irq arrives, arm will use the previous > > instruction prefetch? > > > This barrier has little to do with prefetch. It just guarantees that the > code after the isb() is now running with a disabled virtual timer. > Otherwise, a CPU can freely reorder the write_sysreg() until the next > context synchronization event. > > An interrupt coming between the write and the barrier will also act as a > context synchronization event. For more details, see the ARMv8 ARM (the > glossary has a section on the concept). > So since an ISB doesn't guarantee that the timer will actually be disabled, is it a waste of energy to have it, or should we keep it as a best effort kind of thing? Thanks, -Christoffer
On 15/12/17 10:10, Christoffer Dall wrote: > On Fri, Dec 15, 2017 at 09:09:05AM +0000, Marc Zyngier wrote: >> On 15/12/17 02:27, Jia He wrote: >>> >>> >> >> [...] >> >>>> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) >>>> >>>> /* Disable the virtual timer */ >>>> write_sysreg_el0(0, cntv_ctl); >>>> + isb(); >>> My only concern is whether this isb() is required here? >>> Sorryif this is a stupid question.I understand little about arm arch >>> memory barrier. But seems isb will flush all the instruction prefetch.Do >>> you think if an timer interrupt irq arrives, arm will use the previous >>> instruction prefetch? >> >> >> This barrier has little to do with prefetch. It just guarantees that the >> code after the isb() is now running with a disabled virtual timer. >> Otherwise, a CPU can freely reorder the write_sysreg() until the next >> context synchronization event. >> >> An interrupt coming between the write and the barrier will also act as a >> context synchronization event. For more details, see the ARMv8 ARM (the >> glossary has a section on the concept). >> > > So since an ISB doesn't guarantee that the timer will actually be > disabled, is it a waste of energy to have it, or should we keep it as a > best effort kind of thing? nit: the ISB does offer that guarantee. It is just that the guarantee doesn't extend to an interrupt that has already been signalled. The main issue I have with not having an ISB is that it makes it harder to think of when the disabling actually happens. The disabling could be delayed for a very long time, and would make things harder to debug if they were going wrong. Thanks, M.
On Fri, Dec 15, 2017 at 10:33:48AM +0000, Marc Zyngier wrote: > On 15/12/17 10:10, Christoffer Dall wrote: > > On Fri, Dec 15, 2017 at 09:09:05AM +0000, Marc Zyngier wrote: > >> On 15/12/17 02:27, Jia He wrote: > >>> > >>> > >> > >> [...] > >> > >>>> @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) > >>>> > >>>> /* Disable the virtual timer */ > >>>> write_sysreg_el0(0, cntv_ctl); > >>>> + isb(); > >>> My only concern is whether this isb() is required here? > >>> Sorryif this is a stupid question.I understand little about arm arch > >>> memory barrier. But seems isb will flush all the instruction prefetch.Do > >>> you think if an timer interrupt irq arrives, arm will use the previous > >>> instruction prefetch? > >> > >> > >> This barrier has little to do with prefetch. It just guarantees that the > >> code after the isb() is now running with a disabled virtual timer. > >> Otherwise, a CPU can freely reorder the write_sysreg() until the next > >> context synchronization event. > >> > >> An interrupt coming between the write and the barrier will also act as a > >> context synchronization event. For more details, see the ARMv8 ARM (the > >> glossary has a section on the concept). > >> > > > > So since an ISB doesn't guarantee that the timer will actually be > > disabled, is it a waste of energy to have it, or should we keep it as a > > best effort kind of thing? > > nit: the ISB does offer that guarantee. It is just that the guarantee > doesn't extend to an interrupt that has already been signalled. right, I should have said that it doesn't guarantee that an already signalled interrupt will have been retired prior to enabling interrupts on the CPU again. > > The main issue I have with not having an ISB is that it makes it harder > to think of when the disabling actually happens. The disabling could be > delayed for a very long time, and would make things harder to debug if > they were going wrong. > Yes, it definitely indicates the intention of the code and the flow, and the fact that we have to work around delayed signals in the ISR is then something we can describe with a comment there. I'll add an ISB in the other version of the patch I sent before. Thanks, -Christoffer
diff --git a/virt/kvm/arm/arch_timer.c b/virt/kvm/arm/arch_timer.c index 73d262c4712b..544ed15fbbb3 100644 --- a/virt/kvm/arm/arch_timer.c +++ b/virt/kvm/arm/arch_timer.c @@ -46,7 +46,7 @@ static const struct kvm_irq_level default_vtimer_irq = { .level = 1, }; -static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx); +static bool kvm_timer_irq_can_fire(u32 cnt_ctl); static void kvm_timer_update_irq(struct kvm_vcpu *vcpu, bool new_level, struct arch_timer_context *timer_ctx); static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx); @@ -94,6 +94,7 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) { struct kvm_vcpu *vcpu = *(struct kvm_vcpu **)dev_id; struct arch_timer_context *vtimer; + u32 cnt_ctl; if (!vcpu) { pr_warn_once("Spurious arch timer IRQ on non-VCPU thread\n"); @@ -101,8 +102,8 @@ static irqreturn_t kvm_arch_timer_handler(int irq, void *dev_id) } vtimer = vcpu_vtimer(vcpu); - vtimer->cnt_ctl = read_sysreg_el0(cntv_ctl); - if (kvm_timer_irq_can_fire(vtimer)) + cnt_ctl = read_sysreg_el0(cntv_ctl); + if (kvm_timer_irq_can_fire(cnt_ctl)) kvm_timer_update_irq(vcpu, true, vtimer); if (unlikely(!irqchip_in_kernel(vcpu->kvm))) @@ -148,10 +149,10 @@ static u64 kvm_timer_compute_delta(struct arch_timer_context *timer_ctx) return 0; } -static bool kvm_timer_irq_can_fire(struct arch_timer_context *timer_ctx) +static bool kvm_timer_irq_can_fire(u32 cnt_ctl) { - return !(timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) && - (timer_ctx->cnt_ctl & ARCH_TIMER_CTRL_ENABLE); + return !(cnt_ctl & ARCH_TIMER_CTRL_IT_MASK) && + (cnt_ctl & ARCH_TIMER_CTRL_ENABLE); } /* @@ -164,10 +165,10 @@ static u64 kvm_timer_earliest_exp(struct kvm_vcpu *vcpu) struct arch_timer_context *vtimer = vcpu_vtimer(vcpu); struct arch_timer_context *ptimer = vcpu_ptimer(vcpu); - if (kvm_timer_irq_can_fire(vtimer)) + if (kvm_timer_irq_can_fire(vtimer->cnt_ctl)) min_virt = kvm_timer_compute_delta(vtimer); - if (kvm_timer_irq_can_fire(ptimer)) + if (kvm_timer_irq_can_fire(ptimer->cnt_ctl)) min_phys = kvm_timer_compute_delta(ptimer); /* If none of timers can fire, then return 0 */ @@ -231,7 +232,7 @@ static bool kvm_timer_should_fire(struct arch_timer_context *timer_ctx) { u64 cval, now; - if (!kvm_timer_irq_can_fire(timer_ctx)) + if (!kvm_timer_irq_can_fire(timer_ctx->cnt_ctl)) return false; cval = timer_ctx->cnt_cval; @@ -306,7 +307,7 @@ static void phys_timer_emulate(struct kvm_vcpu *vcpu) * don't need to have a soft timer scheduled for the future. If the * timer cannot fire at all, then we also don't need a soft timer. */ - if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer)) { + if (kvm_timer_should_fire(ptimer) || !kvm_timer_irq_can_fire(ptimer->cnt_ctl)) { soft_timer_cancel(&timer->phys_timer, NULL); return; } @@ -367,6 +368,7 @@ static void vtimer_save_state(struct kvm_vcpu *vcpu) /* Disable the virtual timer */ write_sysreg_el0(0, cntv_ctl); + isb(); vtimer->loaded = false; out: @@ -398,7 +400,8 @@ void kvm_timer_schedule(struct kvm_vcpu *vcpu) * If both timers are not capable of raising interrupts (disabled or * masked), then there's no more work for us to do. */ - if (!kvm_timer_irq_can_fire(vtimer) && !kvm_timer_irq_can_fire(ptimer)) + if (!kvm_timer_irq_can_fire(vtimer->cnt_ctl) && + !kvm_timer_irq_can_fire(ptimer->cnt_ctl)) return; /*