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 Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 00B08C54E60 for ; Tue, 19 Mar 2024 06:54:43 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:Content-Type: Content-Transfer-Encoding:List-Subscribe:List-Help:List-Post:List-Archive: List-Unsubscribe:List-Id:In-Reply-To:From:References:Cc:To:Subject: MIME-Version:Date:Message-ID:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=LWxwLXpxuTzO/eTqz1UN/k1fX1HO0286SNRAfm8eom8=; b=KcPtIFK/Qv7ZXr 9diZey/M+/xP0b197rAWdyWAceYvjjIeyIILRh83OpBLR2QTVLQ3BA6PvUXh6CwNs90mlPeWPWqDF p7cuNV6y/wlua9qr1UcuIl66eoQMNQOxqmIVN1AUK7vCzLllv6j831sIG5pnnsvoassrfbAmYN/zO ZEIRZ4T+FLVvjhctAQRYMQoCbXit+5uJ7/tf+oteL82CjE912dCMlBZ35QaZYf+Z3jM5F3JQRkoAS 2JXyF70IV+HbDSLvztxDUKWG0/vSM4VoE0QDfjFlQuFkr+vIISNgvAZgRDHLeRIWF4ufVTE0Xl0UN uH+E4WVL6K9XMnUIcgoQ==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1rmTMk-0000000BaYf-3cOa; Tue, 19 Mar 2024 06:54:30 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.133.124]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1rmTMg-0000000BaY8-2od7 for linux-arm-kernel@lists.infradead.org; Tue, 19 Mar 2024 06:54:28 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1710831265; h=from:from:reply-to:subject:subject: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=icGFjCa1xrYUbecPG8RFGWPxGwZQyKoWyFdK0JxSbj4=; b=XkqGeczuFyoeRgLNjZL8QhwtUGKYlmhpd4sMJPCf93WSGkdC0W6WaeY9HxwUD27BWRWiNd g33deR7tpd6qW285Ct713DdTj2U899a8gpFVyYJD4mOxJePFC/73KWrLZO/k53179tPxTZ 1RwSh+2Ficoz29Kq7NcTWHRybi7fk60= Received: from mail-pl1-f199.google.com (mail-pl1-f199.google.com [209.85.214.199]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-426-IKT7EkUtOEyS6cQf1VxRrw-1; Tue, 19 Mar 2024 02:54:23 -0400 X-MC-Unique: IKT7EkUtOEyS6cQf1VxRrw-1 Received: by mail-pl1-f199.google.com with SMTP id d9443c01a7336-1e01f53a293so18745155ad.0 for ; Mon, 18 Mar 2024 23:54:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710831262; x=1711436062; h=content-transfer-encoding:in-reply-to:from:references:cc:to :content-language:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=icGFjCa1xrYUbecPG8RFGWPxGwZQyKoWyFdK0JxSbj4=; b=jgIc2jziiHs/GclxYiFKwG0V39zfANxaR/PoljC8OzBBxnqd+fYFp0ih6LXjboigh7 f592rYsHkeXgQJOiE9n0R/DnCX+o/1P+vOZeJggNdpdIyFpp58aMXBl3lLNul/AkTTrj +UoxGnE5kSESFF9J1BIDO3IouZIwxG62n36xR93QHoaqHv4ocAMe4xeI8qUgT1nh+R0+ Y5nYlV1TPp5S2ylB9febec3vqPWaz2F1RQ1vEZTRsDahNerutAQd2M8zQCXd73i3CO6C RMLwEKYAsLFBsSP0vMSjrZ6zvc0HGXE3qyF8u+DWXZRWfMkNSNrprQX2W8JHI853OfY7 gVFA== X-Forwarded-Encrypted: i=1; AJvYcCUZvHFvthQbygf71pP/AA2b0VxCVl+rGjxcx6gwuWD6kLr/T6HgFQaEs4ru66zRfQJWLK3E8Nlt0yMuCckAIWifA3FK2k/oimnO80e29z6jmUHCYf0= X-Gm-Message-State: AOJu0YzQ/pwIqEkVIWfIxbz5QbW3uGm6Mu5xmuKnHBgYE1mYXoEy/YEt +Xm1SLnf0Ag1sOEtPQMvpaG+oNNgXB/EeKd0fZ7HY5epwjY56W8pRPpGl1IleI48FbLy4un48f2 uPG8LdiY0ExFqcrGkZqUoyVxh4A11RbIjIgS9ibj7NQfijxljJJY0xZJdct66vefSk7/jcRLN X-Received: by 2002:a17:902:e882:b0:1e0:1bfd:c1cd with SMTP id w2-20020a170902e88200b001e01bfdc1cdmr5859885plg.54.1710831262335; Mon, 18 Mar 2024 23:54:22 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGS5nCGnHthe4SH4yG07pw+TKNbzItAryCrM74VL3kORDsmNhqLMKxBM1cXZ8h6C+/DTf6k8g== X-Received: by 2002:a17:902:e882:b0:1e0:1bfd:c1cd with SMTP id w2-20020a170902e88200b001e01bfdc1cdmr5859880plg.54.1710831262001; Mon, 18 Mar 2024 23:54:22 -0700 (PDT) Received: from [192.168.68.51] ([43.252.115.31]) by smtp.gmail.com with ESMTPSA id x7-20020a170902a38700b001dee4bd73e0sm9335536pla.59.2024.03.18.23.54.17 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 18 Mar 2024 23:54:21 -0700 (PDT) Message-ID: <6b829cfc-9cbe-42eb-9935-62d2cf5fbcc4@redhat.com> Date: Tue, 19 Mar 2024 16:54:15 +1000 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] virtio_ring: Fix the stale index in available ring To: "Michael S. Tsirkin" Cc: Will Deacon , virtualization@lists.linux.dev, linux-kernel@vger.kernel.org, jasowang@redhat.com, xuanzhuo@linux.alibaba.com, yihyu@redhat.com, shan.gavin@gmail.com, linux-arm-kernel@lists.infradead.org, Catalin Marinas , mochs@nvidia.com References: <20240314074923.426688-1-gshan@redhat.com> <20240318165924.GA1824@willie-the-truck> <35a6bcef-27cf-4626-a41d-9ec0a338fe28@redhat.com> <20240319020905-mutt-send-email-mst@kernel.org> <20240319020949-mutt-send-email-mst@kernel.org> From: Gavin Shan In-Reply-To: <20240319020949-mutt-send-email-mst@kernel.org> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240318_235427_172270_A07F7EDA X-CRM114-Status: GOOD ( 19.13 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On 3/19/24 16:10, Michael S. Tsirkin wrote: > On Tue, Mar 19, 2024 at 02:09:34AM -0400, Michael S. Tsirkin wrote: >> On Tue, Mar 19, 2024 at 02:59:23PM +1000, Gavin Shan wrote: >>> On 3/19/24 02:59, Will Deacon wrote: [...] >>>>> diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c >>>>> index 49299b1f9ec7..7d852811c912 100644 >>>>> --- a/drivers/virtio/virtio_ring.c >>>>> +++ b/drivers/virtio/virtio_ring.c >>>>> @@ -687,9 +687,15 @@ static inline int virtqueue_add_split(struct virtqueue *_vq, >>>>> avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1); >>>>> vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head); >>>>> - /* Descriptors and available array need to be set before we expose the >>>>> - * new available array entries. */ >>>>> - virtio_wmb(vq->weak_barriers); >>>>> + /* >>>>> + * Descriptors and available array need to be set before we expose >>>>> + * the new available array entries. virtio_wmb() should be enough >>>>> + * to ensuere the order theoretically. However, a stronger barrier >>>>> + * is needed by ARM64. Otherwise, the stale data can be observed >>>>> + * by the host (vhost). A stronger barrier should work for other >>>>> + * architectures, but performance loss is expected. >>>>> + */ >>>>> + virtio_mb(false); >>>>> vq->split.avail_idx_shadow++; >>>>> vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev, >>>>> vq->split.avail_idx_shadow); >>>> >>>> Replacing a DMB with a DSB is _very_ unlikely to be the correct solution >>>> here, especially when ordering accesses to coherent memory. >>>> >>>> In practice, either the larger timing different from the DSB or the fact >>>> that you're going from a Store->Store barrier to a full barrier is what >>>> makes things "work" for you. Have you tried, for example, a DMB SY >>>> (e.g. via __smb_mb()). >>>> >>>> We definitely shouldn't take changes like this without a proper >>>> explanation of what is going on. >>>> >>> >>> Thanks for your comments, Will. >>> >>> Yes, DMB should work for us. However, it seems this instruction has issues on >>> NVidia's grace-hopper. It's hard for me to understand how DMB and DSB works >>> from hardware level. I agree it's not the solution to replace DMB with DSB >>> before we fully understand the root cause. >>> >>> I tried the possible replacement like below. __smp_mb() can avoid the issue like >>> __mb() does. __ndelay(10) can avoid the issue, but __ndelay(9) doesn't. >>> >>> static inline int virtqueue_add_split(struct virtqueue *_vq, ...) >>> { >>> : >>> /* Put entry in available array (but don't update avail->idx until they >>> * do sync). */ >>> avail = vq->split.avail_idx_shadow & (vq->split.vring.num - 1); >>> vq->split.vring.avail->ring[avail] = cpu_to_virtio16(_vq->vdev, head); >>> >>> /* Descriptors and available array need to be set before we expose the >>> * new available array entries. */ >>> // Broken: virtio_wmb(vq->weak_barriers); >>> // Broken: __dma_mb(); >>> // Work: __mb(); >>> // Work: __smp_mb(); > > Did you try __smp_wmb ? And wmb? > virtio_wmb(false) is equivalent to __smb_wmb(), which is broken. __wmb() works either. No issue found with it. >>> // Work: __ndelay(100); >>> // Work: __ndelay(10); >>> // Broken: __ndelay(9); >>> >>> vq->split.avail_idx_shadow++; >>> vq->split.vring.avail->idx = cpu_to_virtio16(_vq->vdev, >>> vq->split.avail_idx_shadow); >> >> What if you stick __ndelay here? > > And keep virtio_wmb above? > The result has been shared through a separate reply. >> >>> vq->num_added++; >>> >>> pr_debug("Added buffer head %i to %p\n", head, vq); >>> END_USE(vq); >>> : >>> } >>> >>> I also tried to measure the consumed time for various barrier-relative instructions using >>> ktime_get_ns() which should have consumed most of the time. __smb_mb() is slower than >>> __smp_wmb() but faster than __mb() >>> >>> Instruction Range of used time in ns >>> ---------------------------------------------- >>> __smp_wmb() [32 1128032] >>> __smp_mb() [32 1160096] >>> __mb() [32 1162496] >>> Thanks, Gavin _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel