[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/xen: remove unneeded preempt_disable() from xen_irq_enable()
On Tue, Sep 21, 2021 at 09:02:26AM +0200, Juergen Gross wrote: > Disabling preemption in xen_irq_enable() is not needed. There is no > risk of missing events due to preemption, as preemption can happen > only in case an event is being received, which is just the opposite > of missing an event. > > Signed-off-by: Juergen Gross <jgross@xxxxxxxx> > --- > arch/x86/xen/irq.c | 18 +++++++----------- > 1 file changed, 7 insertions(+), 11 deletions(-) > > diff --git a/arch/x86/xen/irq.c b/arch/x86/xen/irq.c > index dfa091d79c2e..ba9b14a97109 100644 > --- a/arch/x86/xen/irq.c > +++ b/arch/x86/xen/irq.c > @@ -57,24 +57,20 @@ asmlinkage __visible void xen_irq_enable(void) > { > struct vcpu_info *vcpu; > > - /* > - * We may be preempted as soon as vcpu->evtchn_upcall_mask is > - * cleared, so disable preemption to ensure we check for > - * events on the VCPU we are still running on. > - */ > - preempt_disable(); > - > vcpu = this_cpu_read(xen_vcpu); > vcpu->evtchn_upcall_mask = 0; > > - /* Doesn't matter if we get preempted here, because any > - pending event will get dealt with anyway. */ > + /* > + * Now preemption could happen, but this is only possible if an event > + * was handled, so missing an event due to preemption is not > + * possible at all. > + * The worst possible case is to be preempted and then check events > + * pending on the old vcpu, but this is not problematic. > + */ > > barrier(); /* unmask then check (avoid races) */ > if (unlikely(vcpu->evtchn_upcall_pending)) > xen_force_evtchn_callback(); > - > - preempt_enable(); > } > PV_CALLEE_SAVE_REGS_THUNK(xen_irq_enable); > > -- > 2.26.2 > So the reason I asked about this is: vmlinux.o: warning: objtool: xen_irq_disable()+0xa: call to preempt_count_add() leaves .noinstr.text section vmlinux.o: warning: objtool: xen_irq_enable()+0xb: call to preempt_count_add() leaves .noinstr.text section as reported by sfr here: https://lkml.kernel.org/r/20210920113809.18b9b70c@xxxxxxxxxxxxxxxx (I'm still not entirely sure why I didn't see them in my build, or why 0day didn't either) Anyway, I can 'fix' xen_irq_disable(), see below, but I'm worried about that still having a hole vs the preempt model. Consider: xen_irq_disable() preempt_disable(); <IRQ> set_tif_need_resched() </IRQ no preemption because preempt_count!=0> this_cpu_read(xen_vcpu)->evtchn_upcall_mask = 1; // IRQs are actually disabled preempt_enable_no_resched(); // can't resched because IRQs are disabled ... xen_irq_enable() preempt_disable(); vcpu->evtch_upcall_mask = 0; // IRQs are on preempt_enable() // catches the resched from above Now your patch removes that preempt_enable() and we'll have a missing preemption. Trouble is, because this is noinstr, we can't do schedule().. catch-22 --- Subject: x86/xen: Fixup noinstr in xen_irq_{en,dis}able() From: Peter Zijlstra <peterz@xxxxxxxxxxxxx> Date: Mon Sep 20 13:46:19 CEST 2021 vmlinux.o: warning: objtool: xen_irq_disable()+0xa: call to preempt_count_add() leaves .noinstr.text section vmlinux.o: warning: objtool: xen_irq_enable()+0xb: call to preempt_count_add() leaves .noinstr.text section XXX, trades it for: vmlinux.o: warning: objtool: xen_irq_enable()+0x5c: call to __SCT__preempt_schedule_notrace() leaves .noinstr.text section Reported-by: Stephen Rothwell <sfr@xxxxxxxxxxxxxxxx> Signed-off-by: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx> --- arch/x86/xen/irq.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) --- a/arch/x86/xen/irq.c +++ b/arch/x86/xen/irq.c @@ -44,12 +44,18 @@ __PV_CALLEE_SAVE_REGS_THUNK(xen_save_fl, asmlinkage __visible noinstr void xen_irq_disable(void) { - /* There's a one instruction preempt window here. We need to - make sure we're don't switch CPUs between getting the vcpu - pointer and updating the mask. */ - preempt_disable(); + /* + * There's a one instruction preempt window here. We need to + * make sure we're don't switch CPUs between getting the vcpu + * pointer and updating the mask. + */ + preempt_disable_notrace(); this_cpu_read(xen_vcpu)->evtchn_upcall_mask = 1; - preempt_enable_no_resched(); + /* + * We have IRQs disabled at this point, rescheduling isn't going to + * happen, so no point calling into the scheduler for it. + */ + preempt_enable_no_resched_notrace(); } __PV_CALLEE_SAVE_REGS_THUNK(xen_irq_disable, ".noinstr.text"); @@ -62,7 +68,7 @@ asmlinkage __visible noinstr void xen_ir * cleared, so disable preemption to ensure we check for * events on the VCPU we are still running on. */ - preempt_disable(); + preempt_disable_notrace(); vcpu = this_cpu_read(xen_vcpu); vcpu->evtchn_upcall_mask = 0; @@ -74,7 +80,11 @@ asmlinkage __visible noinstr void xen_ir if (unlikely(vcpu->evtchn_upcall_pending)) xen_force_evtchn_callback(); - preempt_enable(); + /* + * XXX if we noinstr we shouldn't be calling schedule(), OTOH we also + * cannot not schedule() as that would violate PREEMPT. + */ + preempt_enable_notrace(); } __PV_CALLEE_SAVE_REGS_THUNK(xen_irq_enable, ".noinstr.text");
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |