Messages in this thread | | | Subject | Re: [PATCH] x86/xen: remove unneeded preempt_disable() from xen_irq_enable() | From | Juergen Gross <> | Date | Tue, 21 Sep 2021 10:24:52 +0200 |
| |
On 21.09.21 10:11, Jan Beulich wrote: > On 21.09.2021 09:58, Juergen Gross wrote: >> On 21.09.21 09:53, Jan Beulich wrote: >>> On 21.09.2021 09:02, Juergen Gross wrote: >>>> --- 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. >>>> + */ >>> >>> I agree this isn't problematic from a functional perspective, but ... >>> >>>> barrier(); /* unmask then check (avoid races) */ >>>> if (unlikely(vcpu->evtchn_upcall_pending)) >>>> xen_force_evtchn_callback(); >>> >>> ... is a stray call here cheaper than ... >>> >>>> - >>>> - preempt_enable(); >>> >>> ... the preempt_{dis,en}able() pair? >> >> The question is if a stray call in case of preemption (very unlikely) >> is cheaper than the preempt_{dis|en}able() pair on each IRQ enabling. >> >> I'm quite sure removing the preempt_*() calls will be a net benefit. > > Well, yes, I agree. It would have been nice if the description pointed > out the fact that preemption kicking in precisely here is very unlikely. > But perhaps that's considered rather obvious ... The issue I'm having > is with the prior comments: They indicated that preemption happening > before the "pending" check would be okay, _despite_ the > preempt_{dis,en}able() pair. One could view this as an indication that > this pair was put there for another reason (e.g. to avoid the stray > calls). But it may of course also be that the comment simply was stale.
The comment is older than the preempt_*() calls.
Those were added 8 years ago claiming they'd prevent lost events, but at the same time at lease one other patch was added which really prevented lost events, so adding the preempt_*() calls might just have been a guess at that time.
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
Thanks,
Juergen
[unhandled content-type:application/pgp-keys][unhandled content-type:application/pgp-signature] | |