From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Jan Beulich" Subject: Re: [PATCHv2 3/5] evtchn: use a per-event channel lock for sending events Date: Wed, 17 Jun 2015 08:05:54 +0100 Message-ID: <558138720200007800085D74@mail.emea.novell.com> References: <1434383299-21833-1-git-send-email-david.vrabel@citrix.com> <1434383299-21833-4-git-send-email-david.vrabel@citrix.com> <5580060502000078000855A9@mail.emea.novell.com> <55803E73.2010603@citrix.com> <558047AA.3040600@citrix.com> <558068BE0200007800085B14@mail.emea.novell.com> <5580514A.5000702@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1Z57Q5-0007VL-Qw for xen-devel@lists.xenproject.org; Wed, 17 Jun 2015 07:05:57 +0000 In-Reply-To: <5580514A.5000702@citrix.com> Content-Disposition: inline List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: David Vrabel Cc: xen-devel@lists.xenproject.org, Keir Fraser , Ian Campbell , Tim Deegan List-Id: xen-devel@lists.xenproject.org >>> On 16.06.15 at 18:39, wrote: > On 16/06/15 17:19, Jan Beulich wrote: >>>>> On 16.06.15 at 17:58, wrote: >>> On 16/06/15 16:19, David Vrabel wrote: >>>>>> @@ -1221,6 +1277,8 @@ void notify_via_xen_event_channel(struct domain *ld, >>> int lport) >>>>>> evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn); >>>>>> } >>>>>> >>>>>> + spin_unlock(&lchn->lock); >>>>>> + >>>>>> spin_unlock(&ld->event_lock); >>>>>> } >>>>> >>>>> Again I think the event lock can be dropped earlier now. >>>> >>>> Ditto. >>> >>> Uh, no. This is notify. I've kept the locking like this because of the >>> ld->is_dying check. I think we need the ld->event_lock in case >>> d->is_dying is set and evtchn_destroy(ld) is called. >> >> Right, but if evtchn_destroy() was a concern, then this wouldn't >> apply just here, but also in the sending path you are relaxing. >> Afaict due to the channel lock being taken in __evtchn_close() >> you can drop the even lock here as the latest after you acquired >> the channel one (I haven't been able to convince myself yet that >> dropping it even before that would be okay). > > But in the evtchn_send() case, we're in a hypercall so we know > ld->is_dying is false and thus cannot be racing with evtchn_destroy(ld). That's only the common code path; there's an evtchn_send() call from __domain_finalise_shutdown() which afaict doesn't make such guarantees (in particular there clearly are d != current->domain cases here). And then - how is being in a hypercall a guard against is_dying not getting set? > It would be good to remove event_lock from notify_xen_event_channel() as > well since this is heavily used for ioreqs and vm events. Let me have a > more careful look. Indeed, thanks. Jan