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 01621C4361B for ; Fri, 11 Dec 2020 10:33:08 +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 787A923ED0 for ; Fri, 11 Dec 2020 10:33:08 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 787A923ED0 Authentication-Results: mail.kernel.org; dmarc=fail (p=quarantine dis=none) header.from=suse.com 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.50343.88956 (Exim 4.92) (envelope-from ) id 1knfjI-0007vT-Il; Fri, 11 Dec 2020 10:32:52 +0000 X-Outflank-Mailman: Message body and most headers restored to incoming version Received: by outflank-mailman (output) from mailman id 50343.88956; Fri, 11 Dec 2020 10:32:52 +0000 Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1knfjI-0007vM-FO; Fri, 11 Dec 2020 10:32:52 +0000 Received: by outflank-mailman (input) for mailman id 50343; Fri, 11 Dec 2020 10:32:51 +0000 Received: from all-amaz-eas1.inumbo.com ([34.197.232.57] helo=us1-amaz-eas2.inumbo.com) by lists.xenproject.org with esmtp (Exim 4.92) (envelope-from ) id 1knfjH-0007vG-8I for xen-devel@lists.xenproject.org; Fri, 11 Dec 2020 10:32:51 +0000 Received: from mx2.suse.de (unknown [195.135.220.15]) by us1-amaz-eas2.inumbo.com (Halon) with ESMTPS id 00046348-9ca6-4994-bafd-9171c6227e53; Fri, 11 Dec 2020 10:32:48 +0000 (UTC) Received: from relay2.suse.de (unknown [195.135.221.27]) by mx2.suse.de (Postfix) with ESMTP id C3397AE30; Fri, 11 Dec 2020 10:32:47 +0000 (UTC) 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" X-Inumbo-ID: 00046348-9ca6-4994-bafd-9171c6227e53 X-Virus-Scanned: by amavisd-new at test-mx.suse.de DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=suse.com; s=susede1; t=1607682767; h=from:from:reply-to:date:date:message-id:message-id:to:to:cc:cc: mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=0V2lVIbqbq9QYvDmJ9XN2D3RTwxAY+d7SpZ+n/u/Xkw=; b=kZfVF57fwQJPdXwoJ/AqRG9lDWpctHgu2OfC5PXnNFefgjwqlFd3Nc7jZwsCLVONf391DW pTDjNRDp6UmJs4oAti7D2YbQV723YOaHevNO5fG0Go9b/JwHDy5+OHzjrf535QK1NxGeUz gRPa7TW+UDVZ5HSb0E73gQQ/FD5/Jxg= Subject: Re: [PATCH v3 4/5] evtchn: convert domain event lock to an r/w one To: Julien Grall Cc: Andrew Cooper , George Dunlap , Ian Jackson , Wei Liu , Stefano Stabellini , "xen-devel@lists.xenproject.org" References: <9d7a052a-6222-80ff-cbf1-612d4ca50c2a@suse.com> <074be931-54b0-1b0f-72d8-5bd577884814@xen.org> From: Jan Beulich Message-ID: <6e34fd25-14a2-f655-b019-aca94ce086c8@suse.com> Date: Fri, 11 Dec 2020 11:32:47 +0100 User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101 Thunderbird/78.5.1 MIME-Version: 1.0 In-Reply-To: <074be931-54b0-1b0f-72d8-5bd577884814@xen.org> Content-Type: text/plain; charset=utf-8 Content-Language: en-US Content-Transfer-Encoding: 7bit On 09.12.2020 12:54, Julien Grall wrote: > On 23/11/2020 13:29, Jan Beulich wrote: >> @@ -620,7 +620,7 @@ int evtchn_close(struct domain *d1, int >> long rc = 0; >> >> again: >> - spin_lock(&d1->event_lock); >> + write_lock(&d1->event_lock); >> >> if ( !port_is_valid(d1, port1) ) >> { >> @@ -690,13 +690,11 @@ int evtchn_close(struct domain *d1, int >> BUG(); >> >> if ( d1 < d2 ) >> - { >> - spin_lock(&d2->event_lock); >> - } >> + read_lock(&d2->event_lock); > > This change made me realized that I don't quite understand how the > rwlock is meant to work for event_lock. I was actually expecting this to > be a write_lock() given there are state changed in the d2 events. Well, the protection needs to be against racing changes, i.e. parallel invocations of this same function, or evtchn_close(). It is debatable whether evtchn_status() and domain_dump_evtchn_info() would better also be locked out (other read_lock() uses aren't applicable to interdomain channels). > Could you outline how a developper can find out whether he/she should > use read_lock or write_lock? I could try to, but it would again be a port type dependent model, just like for the per-channel locks. So I'd like it to be clarified first whether you aren't instead indirectly asking for these to become write_lock(). >> --- a/xen/common/rwlock.c >> +++ b/xen/common/rwlock.c >> @@ -102,6 +102,14 @@ void queue_write_lock_slowpath(rwlock_t >> spin_unlock(&lock->lock); >> } >> >> +void _rw_barrier(rwlock_t *lock) >> +{ >> + check_barrier(&lock->lock.debug); >> + smp_mb(); >> + while ( _rw_is_locked(lock) ) >> + arch_lock_relax(); >> + smp_mb(); >> +} > > As I pointed out when this implementation was first proposed (see [1]), > there is a risk that the loop will never exit. The [1] reference was missing, but I recall you saying so. > I think the following implementation would be better (although it is ugly): > > write_lock(); > /* do nothing */ > write_unlock(); > > This will act as a barrier between lock held before and after the call. Right, and back then I indicated agreement. When getting to actually carry out the change, I realized though that then the less restrictive check_barrier() can't be used anymore (or to be precise, it could be used, but the stronger check_lock() would subsequently still come into play). This isn't a problem here, but would be for any IRQ-safe r/w lock that the barrier may want to be used on down the road. Thinking about it, a read_lock() / read_unlock() pair would suffice though. But this would then still have check_lock() involved. Given all of this, maybe it's better not to introduce the function at all and instead open-code the read_lock() / read_unlock() pair at the use site. > As an aside, I think the introduction of rw_barrier() deserve to be a in > separate patch to help the review. I'm aware there are differing views on this - to me, putting this in a separate patch would be introduction of dead code. But as per above maybe the function now won't get introduced anymore anyway. Jan