From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-12.3 required=3.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 22982C4361A for ; Fri, 4 Dec 2020 11:29:16 +0000 (UTC) Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id A2D2E229F0 for ; Fri, 4 Dec 2020 11:29:15 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org A2D2E229F0 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=xen.org Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=xen-devel-bounces@lists.xenproject.org Received: from list by lists.xenproject.org with outflank-mailman.44516.79761 (Exim 4.92) (envelope-from ) id 1kl9Gs-00023w-JG; Fri, 04 Dec 2020 11:29:06 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 44516.79761; Fri, 04 Dec 2020 11:29:06 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kl9Gs-00023p-GB; Fri, 04 Dec 2020 11:29:06 +0000 Received: by outflank-mailman (input) for mailman id 44516; Fri, 04 Dec 2020 11:29:05 +0000 Received: from mail.xenproject.org ([104.130.215.37]) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kl9Gq-00023f-Km for xen-devel@lists.xenproject.org; Fri, 04 Dec 2020 11:29:05 +0000 Received: from xenbits.xenproject.org ([104.239.192.120]) by mail.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1kl9Go-0008D7-Us; Fri, 04 Dec 2020 11:29:02 +0000 Received: from [54.239.6.188] (helo=a483e7b01a66.ant.amazon.com) by xenbits.xenproject.org with esmtpsa (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1kl9Go-0002ny-Nh; Fri, 04 Dec 2020 11:29:02 +0000 X-BeenThere: xen-devel@lists.xenproject.org List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xenproject.org Precedence: list Sender: "Xen-devel" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=xen.org; s=20200302mail; h=Content-Transfer-Encoding:Content-Type:In-Reply-To: MIME-Version:Date:Message-ID:From:References:Cc:To:Subject; bh=7uul7Eq0jzdriWKnD6cl7d748nd4WelZ/jS33nSJLPc=; b=j80BGqbxTtx9AoQ5RdwYzj2VuM S6nAr4un1XOA9XeustC7t8bagLAfag9HbAvvTs2Mhc3sAXM+KKhjj2gksSFkaRCbKPBnI/JHCa52h ohCC5bxj1aoZHbMHQuIflZYp6nXMH4QwatdnPi4f5MGBriuqYCcsXH3P6nt/gV34Slv8=; Subject: Re: [PATCH v3 5/5] evtchn: don't call Xen consumer callback with per-channel lock held To: Jan Beulich Cc: Andrew Cooper , George Dunlap , Ian Jackson , Wei Liu , Stefano Stabellini , Tamas K Lengyel , Petre Ovidiu PIRCALABU , Alexandru Isaila , "xen-devel@lists.xenproject.org" References: <9d7a052a-6222-80ff-cbf1-612d4ca50c2a@suse.com> <17c90493-b438-fbc1-ca10-3bc4d89c4e5e@xen.org> <7a768bcd-80c1-d193-8796-7fb6720fa22a@suse.com> From: Julien Grall Message-ID: <1a8250f5-ea49-ac3a-e992-be7ec40deba9@xen.org> Date: Fri, 4 Dec 2020 11:28:59 +0000 User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: <7a768bcd-80c1-d193-8796-7fb6720fa22a@suse.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-GB Content-Transfer-Encoding: 7bit Hi Jan, On 03/12/2020 10:09, Jan Beulich wrote: > On 02.12.2020 22:10, Julien Grall wrote: >> On 23/11/2020 13:30, Jan Beulich wrote: >>> While there don't look to be any problems with this right now, the lock >>> order implications from holding the lock can be very difficult to follow >>> (and may be easy to violate unknowingly). The present callbacks don't >>> (and no such callback should) have any need for the lock to be held. >>> >>> However, vm_event_disable() frees the structures used by respective >>> callbacks and isn't otherwise synchronized with invocations of these >>> callbacks, so maintain a count of in-progress calls, for evtchn_close() >>> to wait to drop to zero before freeing the port (and dropping the lock). >> >> AFAICT, this callback is not the only place where the synchronization is >> missing in the VM event code. >> >> For instance, vm_event_put_request() can also race against >> vm_event_disable(). >> >> So shouldn't we handle this issue properly in VM event? > > I suppose that's a question to the VM event folks rather than me? Yes. From my understanding of Tamas's e-mail, they are relying on the monitoring software to do the right thing. I will refrain to comment on this approach. However, given the race is much wider than the event channel, I would recommend to not add more code in the event channel to deal with such problem. Instead, this should be fixed in the VM event code when someone has time to harden the subsystem. > >>> --- >>> Should we make this accounting optional, to be requested through a new >>> parameter to alloc_unbound_xen_event_channel(), or derived from other >>> than the default callback being requested? >> >> Aside the VM event, do you see any value for the other caller? > > No (albeit I'm not entirely certain about vpl011_notification()'s > needs), hence the consideration. It's unnecessary overhead in > those cases. I had another look and I think there is a small race in VPL011. It should be easy to fix (I will try to have a look later today). > >>> @@ -781,9 +786,15 @@ int evtchn_send(struct domain *ld, unsig >>> rport = lchn->u.interdomain.remote_port; >>> rchn = evtchn_from_port(rd, rport); >>> if ( consumer_is_xen(rchn) ) >>> + { >>> + /* Don't keep holding the lock for the call below. */ >>> + atomic_inc(&rchn->u.interdomain.active_calls); >>> + evtchn_read_unlock(lchn); >>> xen_notification_fn(rchn)(rd->vcpu[rchn->notify_vcpu_id], rport); >>> - else >>> - evtchn_port_set_pending(rd, rchn->notify_vcpu_id, rchn); >> >> atomic_dec() doesn't contain any memory barrier, so we will want one >> between xen_notification_fn() and atomic_dec() to avoid re-ordering. > > Oh, indeed. But smp_mb() is too heavy handed here - x86 doesn't > really need any barrier, yet would gain a full MFENCE that way. > Actually - looks like I forgot we gained smp_mb__before_atomic() > a little over half a year ago. Ah yes, I forgot that atomics instruction are ordered on x86. Cheers, -- Julien Grall