Message ID | 20230321085953.24949-1-huangjie.albert@bytedance.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a5d:604a:0:0:0:0:0 with SMTP id j10csp1679536wrt; Tue, 21 Mar 2023 02:41:51 -0700 (PDT) X-Google-Smtp-Source: AK7set9H6DU7ikL7+Oqj3b56cRsDb4avl/2UzVwQdv10/CJX/cPOstLihQ4RE4vbzS4UKsCii0xu X-Received: by 2002:a17:902:d4c3:b0:1a1:c6a7:44f5 with SMTP id o3-20020a170902d4c300b001a1c6a744f5mr1963708plg.52.1679391710315; Tue, 21 Mar 2023 02:41:50 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1679391710; cv=none; d=google.com; s=arc-20160816; b=GLlSzpQxJ66YKD8fFUUznt1BqaJgMPdDSETAThyv/vgPrK+vIA44ji5tPIFwkPbaSd 4FIZsx3Yww3NMy7VP73dKJ+al+IU/rGMbiiyisd16hrgl4GVu5Dp6tx7bSlbX+t3b6dE bSCwm/NkNtDjwifG4TueCW7swQCKZ6gTtVnOuUiCIITLmvHjbp4KYuxQjRBJPLoI+ylO HOXx0lFG+L7+dI7k/eLCFftWv8DY7Sr6X6x7Es9BHXjVoxFnAKzriyZ7p4+Sr9uf0SU3 8qPkAF2Cg0t8aeesOJiCdLXeW7CHlXfjjb+Z4MXFsF6gJMHdnJPVSALD33ulYrWilR+h GPbQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:mime-version :message-id:date:subject:cc:to:from:dkim-signature; bh=b7WQ13znr3CN4XKk2kspE4hbceE3ioQVtB8Zu2USju8=; b=c/vOcQU/7zX3gT9WEUHyNgVhwX4q6uN3z55UkMWY02W3mRhcrh23IiRiTVgXbpPdCK ci+5Shl+vrIPFTEBtAKqLsYlB2C0ShwJL7zNG7Oi1QiSj2Zs+l2cReiSOgtjertUWAfK T2IzEftuVzSiklrMjMKcqdSAn2hRe5lUJ7muZQy68IIwlgvBN8QAtDhkdIf2jmmw9sgC nvddMcqvrid1xaKkFQo2VDO2nEff907sQ2bSLt8389crMEf2ZTOqKoNNnADLF22FGjM+ lnOMin7BCwAJFOfvLo4tfXQ4UPX/qXKW40FwRkZwJEiTJNmB6OYG+d6xTuftuz9MiVrN hfQw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@bytedance.com header.s=google header.b=KmxEM5dn; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=bytedance.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id u11-20020a6540cb000000b0050fa3302c6asi2095911pgp.584.2023.03.21.02.41.05; Tue, 21 Mar 2023 02:41:50 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20; Authentication-Results: mx.google.com; dkim=pass header.i=@bytedance.com header.s=google header.b=KmxEM5dn; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=QUARANTINE sp=QUARANTINE dis=NONE) header.from=bytedance.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S230253AbjCUJC5 (ORCPT <rfc822;pusanteemu@gmail.com> + 99 others); Tue, 21 Mar 2023 05:02:57 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51770 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230294AbjCUJCi (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Tue, 21 Mar 2023 05:02:38 -0400 Received: from mail-pj1-x102c.google.com (mail-pj1-x102c.google.com [IPv6:2607:f8b0:4864:20::102c]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 1F9A935ECF for <linux-kernel@vger.kernel.org>; Tue, 21 Mar 2023 02:00:54 -0700 (PDT) Received: by mail-pj1-x102c.google.com with SMTP id mp3-20020a17090b190300b0023fcc8ce113so4504930pjb.4 for <linux-kernel@vger.kernel.org>; Tue, 21 Mar 2023 02:00:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bytedance.com; s=google; t=1679389209; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:from:to:cc:subject:date:message-id:reply-to; bh=b7WQ13znr3CN4XKk2kspE4hbceE3ioQVtB8Zu2USju8=; b=KmxEM5dn2jCC/nTRsrAfzPq82YzxBSOhtYw2Nl30BeA4kCmQ/z3QVVVjAN9LgjsqNy 71HnsWGOIgYv+Y4wMLKI3VJvw/yydPfyYzEFkikTdy/Y7kVPkYMgZiVJhc1T+LsSGzli 2LXawa0Y+MnBmMmtKLyO+62eYYktHMeBLcTW+Ka1hw5joUlHaW5psnZgywRtbS4Qs4DK f/xwH+H7SOPW8WIw7yLTPaFH8g7SNVxJ5RSq4QjNRKF/4Odrk4WYdMEGTKAtw4JlVrUZ vsS3uBcP13Ux3nK0DhJSLUXefAYpxJiKwpFPWyivHN6cJ11LZAuNq5MVPt5AbV73mFJC 85rA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1679389209; h=content-transfer-encoding:mime-version:message-id:date:subject:cc :to:from:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=b7WQ13znr3CN4XKk2kspE4hbceE3ioQVtB8Zu2USju8=; b=FVxt7FYFTB/Br1g4IyHSvvN3GPrBP5iEp3SqMjUGyxX2k1djuPyNsKgrntOOHP23b8 GK8600XXigiExU8C8Wi8c1A4jSpE060wxioSexUOq1d41pDVC0v0+qMIHLqzmZwvOe4t DxfrBB+zXKswOOPv3xBOPupiW7dis5plqzaC9KmasRNlTyRm3gxj46cDYVDjb0wkJdn3 sO4q68UOkSxMdCsqYm3W7o/6Kt5jEiVhu0mcqbFChM2xhWf+bOJGeOu3OmIajiswbu6T AcqVlkShd4A5d5Q2AzIBN/kH1/XpX2RkL10Nz2tZaLZIErzCKovUjWSD6Q9nJuUebE0b VuRA== X-Gm-Message-State: AO0yUKXf26VXowXeg0ZQ9jVFtV+iuSEYSRflaWlXjKMrXefUHitM0hJC DyHc5PgiKFlRd310zTIQa1Pre0G+hYPTzpUbzz4= X-Received: by 2002:a05:6a20:1a24:b0:da:b92c:a949 with SMTP id cj36-20020a056a201a2400b000dab92ca949mr1060077pzb.36.1679389208860; Tue, 21 Mar 2023 02:00:08 -0700 (PDT) Received: from C02FG34NMD6R.bytedance.net ([139.177.225.238]) by smtp.gmail.com with ESMTPSA id e15-20020a62ee0f000000b0058bacd6c4e8sm7586443pfi.207.2023.03.21.02.00.06 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 21 Mar 2023 02:00:08 -0700 (PDT) From: Albert Huang <huangjie.albert@bytedance.com> To: "Michael S . Tsirkin " <mst@redhat.com>, Jason Wang <jasowang@redhat.com> Cc: "huangjie.albert" <huangjie.albert@bytedance.com>, virtualization@lists.linux-foundation.org (open list:VIRTIO CORE AND NET DRIVERS), linux-kernel@vger.kernel.org (open list) Subject: [PATCH] virtio_ring: Suppress tx interrupt when napi_tx disable Date: Tue, 21 Mar 2023 16:59:53 +0800 Message-Id: <20230321085953.24949-1-huangjie.albert@bytedance.com> X-Mailer: git-send-email 2.37.0 (Apple Git-136) MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-getmail-retrieved-from-mailbox: =?utf-8?q?INBOX?= X-GMAIL-THRID: =?utf-8?q?1760969842062319270?= X-GMAIL-MSGID: =?utf-8?q?1760969842062319270?= |
Series |
virtio_ring: Suppress tx interrupt when napi_tx disable
|
|
Commit Message
黄杰
March 21, 2023, 8:59 a.m. UTC
From: "huangjie.albert" <huangjie.albert@bytedance.com> fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") if we disable the napi_tx. when we triger a tx interrupt, the vq->event_triggered will be set to true. It will no longer be set to false. Unless we explicitly call virtqueue_enable_cb_delayed or virtqueue_enable_cb_prepare if we disable the napi_tx, It will only be called when the tx ring buffer is relatively small: virtio_net->start_xmit: if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { netif_stop_subqueue(dev, qnum); if (!use_napi && unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { /* More just got used, free them then recheck. */ free_old_xmit_skbs(sq, false); if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { netif_start_subqueue(dev, qnum); virtqueue_disable_cb(sq->vq); } } } Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap every time we call virtqueue_get_buf_ctx.This will bring more interruptions. if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> --- drivers/virtio/virtio_ring.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
Comments
On Tue, Mar 21, 2023 at 5:00 PM Albert Huang <huangjie.albert@bytedance.com> wrote: > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > if we disable the napi_tx. when we triger a tx interrupt, the typo should be "trigger" > vq->event_triggered will be set to true. It will no longer be > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > or virtqueue_enable_cb_prepare > > if we disable the napi_tx, It will only be called when the tx ring > buffer is relatively small: > virtio_net->start_xmit: > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > netif_stop_subqueue(dev, qnum); > if (!use_napi && > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > /* More just got used, free them then recheck. */ > free_old_xmit_skbs(sq, false); > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > netif_start_subqueue(dev, qnum); > virtqueue_disable_cb(sq->vq); > } The code example here is out of date, make sure your tree has this: commit d71ebe8114b4bf622804b810f5e274069060a174 Author: Jason Wang <jasowang@redhat.com> Date: Tue Jan 17 11:47:07 2023 +0800 virtio-net: correctly enable callback during start_xmit > } > } > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. Can you please post how to test with the performance numbers? > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > or vq->packed.vring.driver->off_wrap > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > --- > drivers/virtio/virtio_ring.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 307e139cb11d..f486cccadbeb 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > /* If we expect an interrupt for the next entry, tell host > * by writing event index and flush out the write before > * the read in the next get_buf call. */ > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > + && (vq->event_triggered == false)) I'm not sure this can work, when event_triggered is true it means we've got an interrupt, in this case if we want another interrupt for the next entry, we should update used_event otherwise we will lose that interrupt? Thanks > virtio_store_mb(vq->weak_barriers, > &vring_used_event(&vq->split.vring), > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > * by writing event index and flush out the write before > * the read in the next get_buf call. > */ > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > + && (vq->event_triggered == false)) > virtio_store_mb(vq->weak_barriers, > &vq->packed.vring.driver->off_wrap, > cpu_to_le16(vq->last_used_idx)); > -- > 2.31.1 >
Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > <huangjie.albert@bytedance.com> wrote: > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > typo should be "trigger" > OK, thanks for this. I will correct it in the next version > > vq->event_triggered will be set to true. It will no longer be > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > or virtqueue_enable_cb_prepare > > > > if we disable the napi_tx, It will only be called when the tx ring > > buffer is relatively small: > > virtio_net->start_xmit: > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > netif_stop_subqueue(dev, qnum); > > if (!use_napi && > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > /* More just got used, free them then recheck. */ > > free_old_xmit_skbs(sq, false); > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > netif_start_subqueue(dev, qnum); > > virtqueue_disable_cb(sq->vq); > > } > > The code example here is out of date, make sure your tree has this: also, I will correct it in the next version,this is from kernel 5.15. > > commit d71ebe8114b4bf622804b810f5e274069060a174 > Author: Jason Wang <jasowang@redhat.com> > Date: Tue Jan 17 11:47:07 2023 +0800 > > virtio-net: correctly enable callback during start_xmit > > > } > > } > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > Can you please post how to test with the performance numbers? > iperf3 tcp stream: vm1 -----------------> vm2 vm2 just receive tcp data stream from vm1, and send the ack to vm1, there are so many tx interruptions in vm2. but without event_triggered there are just a few tx interruptions. > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > or vq->packed.vring.driver->off_wrap > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > --- > > drivers/virtio/virtio_ring.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 307e139cb11d..f486cccadbeb 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > /* If we expect an interrupt for the next entry, tell host > > * by writing event index and flush out the write before > > * the read in the next get_buf call. */ > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > + && (vq->event_triggered == false)) > > I'm not sure this can work, when event_triggered is true it means > we've got an interrupt, in this case if we want another interrupt for > the next entry, we should update used_event otherwise we will lose > that interrupt? > > Thanks Normally, if we receive an interrupt, we should disable the interrupt in the interrupt callback handler. But because of the introduction of event_triggered, here, virtqueue_get_buf_ctx_split cannot be recognized that the interrupt has been turned off. if we want another interrupt for the next entry, We should probably call virtqueue_enable_cb? Thanks > > > virtio_store_mb(vq->weak_barriers, > > &vring_used_event(&vq->split.vring), > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > * by writing event index and flush out the write before > > * the read in the next get_buf call. > > */ > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > + && (vq->event_triggered == false)) > > virtio_store_mb(vq->weak_barriers, > > &vq->packed.vring.driver->off_wrap, > > cpu_to_le16(vq->last_used_idx)); > > -- > > 2.31.1 > > >
On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > <huangjie.albert@bytedance.com> wrote: > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > typo should be "trigger" > > > > OK, thanks for this. I will correct it in the next version > > > > vq->event_triggered will be set to true. It will no longer be > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > or virtqueue_enable_cb_prepare > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > buffer is relatively small: > > > virtio_net->start_xmit: > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > netif_stop_subqueue(dev, qnum); > > > if (!use_napi && > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > /* More just got used, free them then recheck. */ > > > free_old_xmit_skbs(sq, false); > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > netif_start_subqueue(dev, qnum); > > > virtqueue_disable_cb(sq->vq); > > > } > > > > The code example here is out of date, make sure your tree has this: > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > Author: Jason Wang <jasowang@redhat.com> > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > virtio-net: correctly enable callback during start_xmit > > > > > } > > > } > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > Can you please post how to test with the performance numbers? > > > > iperf3 tcp stream: > vm1 -----------------> vm2 > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > there are so > many tx interruptions in vm2. > > but without event_triggered there are just a few tx interruptions. > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > or vq->packed.vring.driver->off_wrap > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > --- > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > index 307e139cb11d..f486cccadbeb 100644 > > > --- a/drivers/virtio/virtio_ring.c > > > +++ b/drivers/virtio/virtio_ring.c > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > /* If we expect an interrupt for the next entry, tell host > > > * by writing event index and flush out the write before > > > * the read in the next get_buf call. */ > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > + && (vq->event_triggered == false)) > > > > I'm not sure this can work, when event_triggered is true it means > > we've got an interrupt, in this case if we want another interrupt for > > the next entry, we should update used_event otherwise we will lose > > that interrupt? > > > > Thanks > > Normally, if we receive an interrupt, we should disable the interrupt > in the interrupt callback handler. So the problem is: 1) event_triggered was set to true in vring_interrupt() 2) after this nothing will happen for virtqueue_disable_cb() so VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled then it tries to publish new event This makes me think about whether or not we really need event_triggered. The assumption in the virtqueue_disable_cb() seems wrong: /* If device triggered an event already it won't trigger one again: * no need to disable. */ if (vq->event_triggered) return; This is wrong if there's no event index support. And the event_triggered is somehow duplicated with the VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix might be: 1) remove event_triggered 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in vring_interrrupt if event index is supported ? Thanks > But because of the introduction of event_triggered, here, > virtqueue_get_buf_ctx_split cannot be recognized > that the interrupt has been turned off. > > if we want another interrupt for the next entry, We should probably > call virtqueue_enable_cb? > > Thanks > > > > > > virtio_store_mb(vq->weak_barriers, > > > &vring_used_event(&vq->split.vring), > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > * by writing event index and flush out the write before > > > * the read in the next get_buf call. > > > */ > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > + && (vq->event_triggered == false)) > > > virtio_store_mb(vq->weak_barriers, > > > &vq->packed.vring.driver->off_wrap, > > > cpu_to_le16(vq->last_used_idx)); > > > -- > > > 2.31.1 > > > > > >
On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > typo should be "trigger" > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > or virtqueue_enable_cb_prepare > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > buffer is relatively small: > > > > virtio_net->start_xmit: > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > netif_stop_subqueue(dev, qnum); > > > > if (!use_napi && > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > /* More just got used, free them then recheck. */ > > > > free_old_xmit_skbs(sq, false); > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > netif_start_subqueue(dev, qnum); > > > > virtqueue_disable_cb(sq->vq); > > > > } > > > > > > The code example here is out of date, make sure your tree has this: > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > Author: Jason Wang <jasowang@redhat.com> > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > } > > > > } > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > Can you please post how to test with the performance numbers? > > > > > > > iperf3 tcp stream: > > vm1 -----------------> vm2 > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > there are so > > many tx interruptions in vm2. > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > --- > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > --- a/drivers/virtio/virtio_ring.c > > > > +++ b/drivers/virtio/virtio_ring.c > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > /* If we expect an interrupt for the next entry, tell host > > > > * by writing event index and flush out the write before > > > > * the read in the next get_buf call. */ > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > + && (vq->event_triggered == false)) > > > > > > I'm not sure this can work, when event_triggered is true it means > > > we've got an interrupt, in this case if we want another interrupt for > > > the next entry, we should update used_event otherwise we will lose > > > that interrupt? > > > > > > Thanks > > > > Normally, if we receive an interrupt, we should disable the interrupt > > in the interrupt callback handler. > > So the problem is: > > 1) event_triggered was set to true in vring_interrupt() > > 2) after this nothing will happen for virtqueue_disable_cb() so > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > then it tries to publish new event Oh. Good point! I think when I wrote up 8d622d21d248 ("virtio: fix up virtio_disable_cb") I missed this corner case. > This makes me think about whether or not we really need > event_triggered. The assumption in the virtqueue_disable_cb() seems > wrong: > > /* If device triggered an event already it won't trigger one again: > * no need to disable. > */ > if (vq->event_triggered) > return; > > This is wrong if there's no event index support. I don't get it. how does this get triggered? You are talking about device without event index? Here's code from vring_interrupt(): /* Just a hint for performance: so it's ok that this can be racy! */ if (vq->event) vq->event_triggered = true; > And the > event_triggered is somehow duplicated with the > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > might be: > > 1) remove event_triggered > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > vring_interrrupt if event index is supported > > ? > > Thanks I am not sure all this is right and I'd rather we focused performance/correctness and cleanups separately. > > > But because of the introduction of event_triggered, here, > > virtqueue_get_buf_ctx_split cannot be recognized > > that the interrupt has been turned off. > > > > if we want another interrupt for the next entry, We should probably > > call virtqueue_enable_cb? > > > > Thanks > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > &vring_used_event(&vq->split.vring), > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > * by writing event index and flush out the write before > > > > * the read in the next get_buf call. > > > > */ > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > + && (vq->event_triggered == false)) > > > > virtio_store_mb(vq->weak_barriers, > > > > &vq->packed.vring.driver->off_wrap, > > > > cpu_to_le16(vq->last_used_idx)); > > > > -- > > > > 2.31.1 > > > > > > > > >
Thanks for the patch! I picked it up. I made small changes, please look at it in my branch, both to see what I changed for your next submission, and to test that it still addresses the problem for you. Waiting for your confirmation to send upstream. Thanks! On Tue, Mar 21, 2023 at 04:59:53PM +0800, Albert Huang wrote: > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > if we disable the napi_tx. when we triger a tx interrupt, the > vq->event_triggered will be set to true. It will no longer be > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > or virtqueue_enable_cb_prepare > > if we disable the napi_tx, It will only be called when the tx ring > buffer is relatively small: > virtio_net->start_xmit: > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > netif_stop_subqueue(dev, qnum); > if (!use_napi && > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > /* More just got used, free them then recheck. */ > free_old_xmit_skbs(sq, false); > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > netif_start_subqueue(dev, qnum); > virtqueue_disable_cb(sq->vq); > } > } > } > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > or vq->packed.vring.driver->off_wrap > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > --- > drivers/virtio/virtio_ring.c | 6 ++++-- > 1 file changed, 4 insertions(+), 2 deletions(-) > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > index 307e139cb11d..f486cccadbeb 100644 > --- a/drivers/virtio/virtio_ring.c > +++ b/drivers/virtio/virtio_ring.c > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > /* If we expect an interrupt for the next entry, tell host > * by writing event index and flush out the write before > * the read in the next get_buf call. */ > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > + && (vq->event_triggered == false)) > virtio_store_mb(vq->weak_barriers, > &vring_used_event(&vq->split.vring), > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > * by writing event index and flush out the write before > * the read in the next get_buf call. > */ > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > + && (vq->event_triggered == false)) > virtio_store_mb(vq->weak_barriers, > &vq->packed.vring.driver->off_wrap, > cpu_to_le16(vq->last_used_idx)); > -- > 2.31.1
On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > > > typo should be "trigger" > > > > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > > or virtqueue_enable_cb_prepare > > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > > buffer is relatively small: > > > > > virtio_net->start_xmit: > > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > > netif_stop_subqueue(dev, qnum); > > > > > if (!use_napi && > > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > > /* More just got used, free them then recheck. */ > > > > > free_old_xmit_skbs(sq, false); > > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > > netif_start_subqueue(dev, qnum); > > > > > virtqueue_disable_cb(sq->vq); > > > > > } > > > > > > > > The code example here is out of date, make sure your tree has this: > > > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > > Author: Jason Wang <jasowang@redhat.com> > > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > > > } > > > > > } > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > > > Can you please post how to test with the performance numbers? > > > > > > > > > > iperf3 tcp stream: > > > vm1 -----------------> vm2 > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > > there are so > > > many tx interruptions in vm2. > > > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > > --- > > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > > /* If we expect an interrupt for the next entry, tell host > > > > > * by writing event index and flush out the write before > > > > > * the read in the next get_buf call. */ > > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > > + && (vq->event_triggered == false)) > > > > > > > > I'm not sure this can work, when event_triggered is true it means > > > > we've got an interrupt, in this case if we want another interrupt for > > > > the next entry, we should update used_event otherwise we will lose > > > > that interrupt? > > > > > > > > Thanks > > > > > > Normally, if we receive an interrupt, we should disable the interrupt > > > in the interrupt callback handler. > > > > So the problem is: > > > > 1) event_triggered was set to true in vring_interrupt() > > > > 2) after this nothing will happen for virtqueue_disable_cb() so > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > > then it tries to publish new event > > Oh. Good point! I think when I wrote up > 8d622d21d248 ("virtio: fix up virtio_disable_cb") > I missed this corner case. > > > > > This makes me think about whether or not we really need > > event_triggered. The assumption in the virtqueue_disable_cb() seems > > wrong: > > > > /* If device triggered an event already it won't trigger one again: > > * no need to disable. > > */ > > if (vq->event_triggered) > > return; > > > > This is wrong if there's no event index support. > > > I don't get it. how does this get triggered? > > You are talking about device without event index? > Here's code from vring_interrupt(): > > /* Just a hint for performance: so it's ok that this can be racy! */ > if (vq->event) > vq->event_triggered = true; But we have the following in virtqueue_disable_cb(): /* If device triggered an event already it won't trigger one again: * no need to disable. */ if (vq->event_triggered) return; if (vq->packed_ring) virtqueue_disable_cb_packed(_vq); else virtqueue_disable_cb_split(_vq); This means, without an event index, we don't set avail flags. So the interrupt is not disabled actually in this case. Thanks > > > > > > And the > > event_triggered is somehow duplicated with the > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > > might be: > > > > 1) remove event_triggered > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > > vring_interrrupt if event index is supported > > > > ? > > > > Thanks > > I am not sure all this is right and I'd rather we focused > performance/correctness and cleanups separately. > > > > > > > > > But because of the introduction of event_triggered, here, > > > virtqueue_get_buf_ctx_split cannot be recognized > > > that the interrupt has been turned off. > > > > > > if we want another interrupt for the next entry, We should probably > > > call virtqueue_enable_cb? > > > > > > Thanks > > > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > &vring_used_event(&vq->split.vring), > > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > * by writing event index and flush out the write before > > > > > * the read in the next get_buf call. > > > > > */ > > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > > + && (vq->event_triggered == false)) > > > > > virtio_store_mb(vq->weak_barriers, > > > > > &vq->packed.vring.driver->off_wrap, > > > > > cpu_to_le16(vq->last_used_idx)); > > > > > -- > > > > > 2.31.1 > > > > > > > > > > > > >
Hmm I sent a bit too fast, and my testing rig is down now. So please do send a new version, I sent comments on what to fix in this one. On Fri, Mar 24, 2023 at 02:08:55AM -0400, Michael S. Tsirkin wrote: > Thanks for the patch! > I picked it up. > I made small changes, please look at it in my branch, > both to see what I changed for your next submission, > and to test that it still addresses the problem for you. > Waiting for your confirmation to send upstream. > Thanks! > > > On Tue, Mar 21, 2023 at 04:59:53PM +0800, Albert Huang wrote: > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > vq->event_triggered will be set to true. It will no longer be > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > or virtqueue_enable_cb_prepare > > > > if we disable the napi_tx, It will only be called when the tx ring > > buffer is relatively small: > > virtio_net->start_xmit: > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > netif_stop_subqueue(dev, qnum); > > if (!use_napi && > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > /* More just got used, free them then recheck. */ > > free_old_xmit_skbs(sq, false); > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > netif_start_subqueue(dev, qnum); > > virtqueue_disable_cb(sq->vq); > > } > > } > > } > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > or vq->packed.vring.driver->off_wrap > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > --- > > drivers/virtio/virtio_ring.c | 6 ++++-- > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > index 307e139cb11d..f486cccadbeb 100644 > > --- a/drivers/virtio/virtio_ring.c > > +++ b/drivers/virtio/virtio_ring.c > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > /* If we expect an interrupt for the next entry, tell host > > * by writing event index and flush out the write before > > * the read in the next get_buf call. */ > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > + && (vq->event_triggered == false)) > > virtio_store_mb(vq->weak_barriers, > > &vring_used_event(&vq->split.vring), > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > * by writing event index and flush out the write before > > * the read in the next get_buf call. > > */ > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > + && (vq->event_triggered == false)) > > virtio_store_mb(vq->weak_barriers, > > &vq->packed.vring.driver->off_wrap, > > cpu_to_le16(vq->last_used_idx)); > > -- > > 2.31.1
On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote: > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > > > > > typo should be "trigger" > > > > > > > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > > > or virtqueue_enable_cb_prepare > > > > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > > > buffer is relatively small: > > > > > > virtio_net->start_xmit: > > > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > > > netif_stop_subqueue(dev, qnum); > > > > > > if (!use_napi && > > > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > > > /* More just got used, free them then recheck. */ > > > > > > free_old_xmit_skbs(sq, false); > > > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > > > netif_start_subqueue(dev, qnum); > > > > > > virtqueue_disable_cb(sq->vq); > > > > > > } > > > > > > > > > > The code example here is out of date, make sure your tree has this: > > > > > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > > > Author: Jason Wang <jasowang@redhat.com> > > > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > > > > > } > > > > > > } > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > > > > > Can you please post how to test with the performance numbers? > > > > > > > > > > > > > iperf3 tcp stream: > > > > vm1 -----------------> vm2 > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > > > there are so > > > > many tx interruptions in vm2. > > > > > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > > > --- > > > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > > > /* If we expect an interrupt for the next entry, tell host > > > > > > * by writing event index and flush out the write before > > > > > > * the read in the next get_buf call. */ > > > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > > I'm not sure this can work, when event_triggered is true it means > > > > > we've got an interrupt, in this case if we want another interrupt for > > > > > the next entry, we should update used_event otherwise we will lose > > > > > that interrupt? > > > > > > > > > > Thanks > > > > > > > > Normally, if we receive an interrupt, we should disable the interrupt > > > > in the interrupt callback handler. > > > > > > So the problem is: > > > > > > 1) event_triggered was set to true in vring_interrupt() > > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > > > then it tries to publish new event > > > > Oh. Good point! I think when I wrote up > > 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > I missed this corner case. > > > > > > > > > This makes me think about whether or not we really need > > > event_triggered. The assumption in the virtqueue_disable_cb() seems > > > wrong: > > > > > > /* If device triggered an event already it won't trigger one again: > > > * no need to disable. > > > */ > > > if (vq->event_triggered) > > > return; > > > > > > This is wrong if there's no event index support. > > > > > > I don't get it. how does this get triggered? > > > > You are talking about device without event index? > > Here's code from vring_interrupt(): > > > > /* Just a hint for performance: so it's ok that this can be racy! */ > > if (vq->event) > > vq->event_triggered = true; > > But we have the following in virtqueue_disable_cb(): > > /* If device triggered an event already it won't trigger one again: > * no need to disable. > */ > if (vq->event_triggered) > return; > > if (vq->packed_ring) > virtqueue_disable_cb_packed(_vq); > else > virtqueue_disable_cb_split(_vq); > > This means, without an event index, we don't set avail flags. So the > interrupt is not disabled actually in this case. > > Thanks Only if event_triggered is true, which without event index it never is. > > > > > > > > > > > And the > > > event_triggered is somehow duplicated with the > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > > > might be: > > > > > > 1) remove event_triggered > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > > > vring_interrrupt if event index is supported > > > > > > ? > > > > > > Thanks > > > > I am not sure all this is right and I'd rather we focused > > performance/correctness and cleanups separately. > > > > > > > > > > > > > > > But because of the introduction of event_triggered, here, > > > > virtqueue_get_buf_ctx_split cannot be recognized > > > > that the interrupt has been turned off. > > > > > > > > if we want another interrupt for the next entry, We should probably > > > > call virtqueue_enable_cb? > > > > > > > > Thanks > > > > > > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > &vring_used_event(&vq->split.vring), > > > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > > * by writing event index and flush out the write before > > > > > > * the read in the next get_buf call. > > > > > > */ > > > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > > > + && (vq->event_triggered == false)) > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > &vq->packed.vring.driver->off_wrap, > > > > > > cpu_to_le16(vq->last_used_idx)); > > > > > > -- > > > > > > 2.31.1 > > > > > > > > > > > > > > > > >
On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote: > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > > > > > > > typo should be "trigger" > > > > > > > > > > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > > > > or virtqueue_enable_cb_prepare > > > > > > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > > > > buffer is relatively small: > > > > > > > virtio_net->start_xmit: > > > > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > > > > netif_stop_subqueue(dev, qnum); > > > > > > > if (!use_napi && > > > > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > > > > /* More just got used, free them then recheck. */ > > > > > > > free_old_xmit_skbs(sq, false); > > > > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > > > > netif_start_subqueue(dev, qnum); > > > > > > > virtqueue_disable_cb(sq->vq); > > > > > > > } > > > > > > > > > > > > The code example here is out of date, make sure your tree has this: > > > > > > > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > > > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > > > > Author: Jason Wang <jasowang@redhat.com> > > > > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > > > > > > > } > > > > > > > } > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > > > > > > > Can you please post how to test with the performance numbers? > > > > > > > > > > > > > > > > iperf3 tcp stream: > > > > > vm1 -----------------> vm2 > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > > > > there are so > > > > > many tx interruptions in vm2. > > > > > > > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > > > > --- > > > > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > > > > /* If we expect an interrupt for the next entry, tell host > > > > > > > * by writing event index and flush out the write before > > > > > > > * the read in the next get_buf call. */ > > > > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > > > > I'm not sure this can work, when event_triggered is true it means > > > > > > we've got an interrupt, in this case if we want another interrupt for > > > > > > the next entry, we should update used_event otherwise we will lose > > > > > > that interrupt? > > > > > > > > > > > > Thanks > > > > > > > > > > Normally, if we receive an interrupt, we should disable the interrupt > > > > > in the interrupt callback handler. > > > > > > > > So the problem is: > > > > > > > > 1) event_triggered was set to true in vring_interrupt() > > > > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > > > > then it tries to publish new event > > > > > > Oh. Good point! I think when I wrote up > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > I missed this corner case. > > > > > > > > > > > > > This makes me think about whether or not we really need > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems > > > > wrong: > > > > > > > > /* If device triggered an event already it won't trigger one again: > > > > * no need to disable. > > > > */ > > > > if (vq->event_triggered) > > > > return; > > > > > > > > This is wrong if there's no event index support. > > > > > > > > > I don't get it. how does this get triggered? > > > > > > You are talking about device without event index? > > > Here's code from vring_interrupt(): > > > > > > /* Just a hint for performance: so it's ok that this can be racy! */ > > > if (vq->event) > > > vq->event_triggered = true; > > > > But we have the following in virtqueue_disable_cb(): > > > > /* If device triggered an event already it won't trigger one again: > > * no need to disable. > > */ > > if (vq->event_triggered) > > return; > > > > if (vq->packed_ring) > > virtqueue_disable_cb_packed(_vq); > > else > > virtqueue_disable_cb_split(_vq); > > > > This means, without an event index, we don't set avail flags. So the > > interrupt is not disabled actually in this case. > > > > Thanks > > Only if event_triggered is true, which without event index it never is. I'm not sure I will get here. I meant for example the commit suppresses the effort of skb_xmit_done(): static void skb_xmit_done(struct virtqueue *vq) { struct virtnet_info *vi = vq->vdev->priv; struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; /* Suppress further interrupts. */ virtqueue_disable_cb(vq); The virtqueue_disable_cb() doesn't disable further interrupts when the event index is not there. Thanks > > > > > > > > > > > > > > > > > And the > > > > event_triggered is somehow duplicated with the > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > > > > might be: > > > > > > > > 1) remove event_triggered > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > > > > vring_interrrupt if event index is supported > > > > > > > > ? > > > > > > > > Thanks > > > > > > I am not sure all this is right and I'd rather we focused > > > performance/correctness and cleanups separately. > > > > > > > > > > > > > > > > > > > > > But because of the introduction of event_triggered, here, > > > > > virtqueue_get_buf_ctx_split cannot be recognized > > > > > that the interrupt has been turned off. > > > > > > > > > > if we want another interrupt for the next entry, We should probably > > > > > call virtqueue_enable_cb? > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > &vring_used_event(&vq->split.vring), > > > > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > > > * by writing event index and flush out the write before > > > > > > > * the read in the next get_buf call. > > > > > > > */ > > > > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > > > > + && (vq->event_triggered == false)) > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > &vq->packed.vring.driver->off_wrap, > > > > > > > cpu_to_le16(vq->last_used_idx)); > > > > > > > -- > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > >
On Fri, Mar 24, 2023 at 02:47:02PM +0800, Jason Wang wrote: > On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote: > > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > > > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > > > > > > > > > typo should be "trigger" > > > > > > > > > > > > > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > > > > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > > > > > or virtqueue_enable_cb_prepare > > > > > > > > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > > > > > buffer is relatively small: > > > > > > > > virtio_net->start_xmit: > > > > > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > > > > > netif_stop_subqueue(dev, qnum); > > > > > > > > if (!use_napi && > > > > > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > > > > > /* More just got used, free them then recheck. */ > > > > > > > > free_old_xmit_skbs(sq, false); > > > > > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > > > > > netif_start_subqueue(dev, qnum); > > > > > > > > virtqueue_disable_cb(sq->vq); > > > > > > > > } > > > > > > > > > > > > > > The code example here is out of date, make sure your tree has this: > > > > > > > > > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > > > > > > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > > > > > Author: Jason Wang <jasowang@redhat.com> > > > > > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > > > > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > > > > > > > > > } > > > > > > > > } > > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > > > > > > > > > Can you please post how to test with the performance numbers? > > > > > > > > > > > > > > > > > > > iperf3 tcp stream: > > > > > > vm1 -----------------> vm2 > > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > > > > > there are so > > > > > > many tx interruptions in vm2. > > > > > > > > > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > > > > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > > > > > --- > > > > > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > > > > > /* If we expect an interrupt for the next entry, tell host > > > > > > > > * by writing event index and flush out the write before > > > > > > > > * the read in the next get_buf call. */ > > > > > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > > > > > > I'm not sure this can work, when event_triggered is true it means > > > > > > > we've got an interrupt, in this case if we want another interrupt for > > > > > > > the next entry, we should update used_event otherwise we will lose > > > > > > > that interrupt? > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > Normally, if we receive an interrupt, we should disable the interrupt > > > > > > in the interrupt callback handler. > > > > > > > > > > So the problem is: > > > > > > > > > > 1) event_triggered was set to true in vring_interrupt() > > > > > > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so > > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > > > > > then it tries to publish new event > > > > > > > > Oh. Good point! I think when I wrote up > > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > I missed this corner case. > > > > > > > > > > > > > > > > > This makes me think about whether or not we really need > > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems > > > > > wrong: > > > > > > > > > > /* If device triggered an event already it won't trigger one again: > > > > > * no need to disable. > > > > > */ > > > > > if (vq->event_triggered) > > > > > return; > > > > > > > > > > This is wrong if there's no event index support. > > > > > > > > > > > > I don't get it. how does this get triggered? > > > > > > > > You are talking about device without event index? > > > > Here's code from vring_interrupt(): > > > > > > > > /* Just a hint for performance: so it's ok that this can be racy! */ > > > > if (vq->event) > > > > vq->event_triggered = true; > > > > > > But we have the following in virtqueue_disable_cb(): > > > > > > /* If device triggered an event already it won't trigger one again: > > > * no need to disable. > > > */ > > > if (vq->event_triggered) > > > return; > > > > > > if (vq->packed_ring) > > > virtqueue_disable_cb_packed(_vq); > > > else > > > virtqueue_disable_cb_split(_vq); > > > > > > This means, without an event index, we don't set avail flags. So the > > > interrupt is not disabled actually in this case. > > > > > > Thanks > > > > Only if event_triggered is true, which without event index it never is. > > I'm not sure I will get here. I meant for example the commit > suppresses the effort of skb_xmit_done(): > > static void skb_xmit_done(struct virtqueue *vq) > { > struct virtnet_info *vi = vq->vdev->priv; > struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; > > /* Suppress further interrupts. */ > virtqueue_disable_cb(vq); > > The virtqueue_disable_cb() doesn't disable further interrupts when the > event index is not there. > > Thanks Check what can set event_triggered, you will see. > > > > > > > > > > > > > > > > > > > > > > > And the > > > > > event_triggered is somehow duplicated with the > > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > > > > > might be: > > > > > > > > > > 1) remove event_triggered > > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > > > > > vring_interrrupt if event index is supported > > > > > > > > > > ? > > > > > > > > > > Thanks > > > > > > > > I am not sure all this is right and I'd rather we focused > > > > performance/correctness and cleanups separately. > > > > > > > > > > > > > > > > > > > > > > > > > > > But because of the introduction of event_triggered, here, > > > > > > virtqueue_get_buf_ctx_split cannot be recognized > > > > > > that the interrupt has been turned off. > > > > > > > > > > > > if we want another interrupt for the next entry, We should probably > > > > > > call virtqueue_enable_cb? > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > > &vring_used_event(&vq->split.vring), > > > > > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > > > > * by writing event index and flush out the write before > > > > > > > > * the read in the next get_buf call. > > > > > > > > */ > > > > > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > > &vq->packed.vring.driver->off_wrap, > > > > > > > > cpu_to_le16(vq->last_used_idx)); > > > > > > > > -- > > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Fri, Mar 24, 2023 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Mar 24, 2023 at 02:47:02PM +0800, Jason Wang wrote: > > On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote: > > > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > > > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > > > > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > > > > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > > > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > > > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > > > > > > > > > > > typo should be "trigger" > > > > > > > > > > > > > > > > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > > > > > > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > > > > > > or virtqueue_enable_cb_prepare > > > > > > > > > > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > > > > > > buffer is relatively small: > > > > > > > > > virtio_net->start_xmit: > > > > > > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > > > > > > netif_stop_subqueue(dev, qnum); > > > > > > > > > if (!use_napi && > > > > > > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > > > > > > /* More just got used, free them then recheck. */ > > > > > > > > > free_old_xmit_skbs(sq, false); > > > > > > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > > > > > > netif_start_subqueue(dev, qnum); > > > > > > > > > virtqueue_disable_cb(sq->vq); > > > > > > > > > } > > > > > > > > > > > > > > > > The code example here is out of date, make sure your tree has this: > > > > > > > > > > > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > > > > > > > > > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > > > > > > Author: Jason Wang <jasowang@redhat.com> > > > > > > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > > > > > > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > > > > > > > > > > > } > > > > > > > > > } > > > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > > > > > > > > > > > Can you please post how to test with the performance numbers? > > > > > > > > > > > > > > > > > > > > > > iperf3 tcp stream: > > > > > > > vm1 -----------------> vm2 > > > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > > > > > > there are so > > > > > > > many tx interruptions in vm2. > > > > > > > > > > > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > > > > > > > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > > > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > > > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > > > > > > --- > > > > > > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > > > > > > /* If we expect an interrupt for the next entry, tell host > > > > > > > > > * by writing event index and flush out the write before > > > > > > > > > * the read in the next get_buf call. */ > > > > > > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > > > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > > > > > > > > I'm not sure this can work, when event_triggered is true it means > > > > > > > > we've got an interrupt, in this case if we want another interrupt for > > > > > > > > the next entry, we should update used_event otherwise we will lose > > > > > > > > that interrupt? > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > Normally, if we receive an interrupt, we should disable the interrupt > > > > > > > in the interrupt callback handler. > > > > > > > > > > > > So the problem is: > > > > > > > > > > > > 1) event_triggered was set to true in vring_interrupt() > > > > > > > > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so > > > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > > > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > > > > > > then it tries to publish new event > > > > > > > > > > Oh. Good point! I think when I wrote up > > > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > I missed this corner case. > > > > > > > > > > > > > > > > > > > > > This makes me think about whether or not we really need > > > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems > > > > > > wrong: > > > > > > > > > > > > /* If device triggered an event already it won't trigger one again: > > > > > > * no need to disable. > > > > > > */ > > > > > > if (vq->event_triggered) > > > > > > return; > > > > > > > > > > > > This is wrong if there's no event index support. > > > > > > > > > > > > > > > I don't get it. how does this get triggered? > > > > > > > > > > You are talking about device without event index? > > > > > Here's code from vring_interrupt(): > > > > > > > > > > /* Just a hint for performance: so it's ok that this can be racy! */ > > > > > if (vq->event) > > > > > vq->event_triggered = true; > > > > > > > > But we have the following in virtqueue_disable_cb(): > > > > > > > > /* If device triggered an event already it won't trigger one again: > > > > * no need to disable. > > > > */ > > > > if (vq->event_triggered) > > > > return; > > > > > > > > if (vq->packed_ring) > > > > virtqueue_disable_cb_packed(_vq); > > > > else > > > > virtqueue_disable_cb_split(_vq); > > > > > > > > This means, without an event index, we don't set avail flags. So the > > > > interrupt is not disabled actually in this case. > > > > > > > > Thanks > > > > > > Only if event_triggered is true, which without event index it never is. > > > > I'm not sure I will get here. I meant for example the commit > > suppresses the effort of skb_xmit_done(): > > > > static void skb_xmit_done(struct virtqueue *vq) > > { > > struct virtnet_info *vi = vq->vdev->priv; > > struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; > > > > /* Suppress further interrupts. */ > > virtqueue_disable_cb(vq); > > > > The virtqueue_disable_cb() doesn't disable further interrupts when the > > event index is not there. > > > > Thanks > > Check what can set event_triggered, you will see. Set to truth by vring_interrupt() Set to false by virtqueue_init(), virtqueue_enable_cb_prepare(), virtqueue_enable_cb_delayed() Assuming NAPI TX is enabled and the device doesn't support event index. 1) driver sends packets 1-10 2) the start_xmit() for the last packet will call virtqueue_enable_cb_delayed() which set event_triggered = false 3) 1st packet were sent, vring_interrupt set event_triggered = true 4) skb_xmit_done() won't disable virtqueue_disable_cb() in this case 5) so we will get the interrupts for 2nd to 10th packet Anything I missed here? Note the comment said it's used for event index: /* Hint for event idx: already triggered no need to disable. */ bool event_triggered; I guess what you meant is that if we don't publish a new event, we will get at most 1 interrupt for everything $queue_size used buffers. But this is not the case without event index. Btw, it may supress the effort of: vring_used_event(&vq->split.vring) = 0x0; Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > And the > > > > > > event_triggered is somehow duplicated with the > > > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > > > > > > might be: > > > > > > > > > > > > 1) remove event_triggered > > > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > > > > > > vring_interrrupt if event index is supported > > > > > > > > > > > > ? > > > > > > > > > > > > Thanks > > > > > > > > > > I am not sure all this is right and I'd rather we focused > > > > > performance/correctness and cleanups separately. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > But because of the introduction of event_triggered, here, > > > > > > > virtqueue_get_buf_ctx_split cannot be recognized > > > > > > > that the interrupt has been turned off. > > > > > > > > > > > > > > if we want another interrupt for the next entry, We should probably > > > > > > > call virtqueue_enable_cb? > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > > > &vring_used_event(&vq->split.vring), > > > > > > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > > > > > * by writing event index and flush out the write before > > > > > > > > > * the read in the next get_buf call. > > > > > > > > > */ > > > > > > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > > > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > > > &vq->packed.vring.driver->off_wrap, > > > > > > > > > cpu_to_le16(vq->last_used_idx)); > > > > > > > > > -- > > > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
On Fri, Mar 24, 2023 at 03:37:04PM +0800, Jason Wang wrote: > On Fri, Mar 24, 2023 at 3:00 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Fri, Mar 24, 2023 at 02:47:02PM +0800, Jason Wang wrote: > > > On Fri, Mar 24, 2023 at 2:42 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Fri, Mar 24, 2023 at 02:32:40PM +0800, Jason Wang wrote: > > > > > On Fri, Mar 24, 2023 at 1:59 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Fri, Mar 24, 2023 at 11:41:12AM +0800, Jason Wang wrote: > > > > > > > On Thu, Mar 23, 2023 at 4:01 PM 黄杰 <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > > > > > Jason Wang <jasowang@redhat.com> 于2023年3月22日周三 10:37写道: > > > > > > > > > > > > > > > > > > On Tue, Mar 21, 2023 at 5:00 PM Albert Huang > > > > > > > > > <huangjie.albert@bytedance.com> wrote: > > > > > > > > > > > > > > > > > > > > From: "huangjie.albert" <huangjie.albert@bytedance.com> > > > > > > > > > > > > > > > > > > > > fix commit 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > > > > > > > > > > > > > > > if we disable the napi_tx. when we triger a tx interrupt, the > > > > > > > > > > > > > > > > > > typo should be "trigger" > > > > > > > > > > > > > > > > > > > > > > > > > OK, thanks for this. I will correct it in the next version > > > > > > > > > > > > > > > > > > vq->event_triggered will be set to true. It will no longer be > > > > > > > > > > set to false. Unless we explicitly call virtqueue_enable_cb_delayed > > > > > > > > > > or virtqueue_enable_cb_prepare > > > > > > > > > > > > > > > > > > > > if we disable the napi_tx, It will only be called when the tx ring > > > > > > > > > > buffer is relatively small: > > > > > > > > > > virtio_net->start_xmit: > > > > > > > > > > if (sq->vq->num_free < 2+MAX_SKB_FRAGS) { > > > > > > > > > > netif_stop_subqueue(dev, qnum); > > > > > > > > > > if (!use_napi && > > > > > > > > > > unlikely(!virtqueue_enable_cb_delayed(sq->vq))) { > > > > > > > > > > /* More just got used, free them then recheck. */ > > > > > > > > > > free_old_xmit_skbs(sq, false); > > > > > > > > > > if (sq->vq->num_free >= 2+MAX_SKB_FRAGS) { > > > > > > > > > > netif_start_subqueue(dev, qnum); > > > > > > > > > > virtqueue_disable_cb(sq->vq); > > > > > > > > > > } > > > > > > > > > > > > > > > > > > The code example here is out of date, make sure your tree has this: > > > > > > > > > > > > > > > > also, I will correct it in the next version,this is from kernel 5.15. > > > > > > > > > > > > > > > > > > > > > > > > > > commit d71ebe8114b4bf622804b810f5e274069060a174 > > > > > > > > > Author: Jason Wang <jasowang@redhat.com> > > > > > > > > > Date: Tue Jan 17 11:47:07 2023 +0800 > > > > > > > > > > > > > > > > > > virtio-net: correctly enable callback during start_xmit > > > > > > > > > > > > > > > > > > > } > > > > > > > > > > } > > > > > > > > > > Because event_triggered is true.Therefore, VRING_AVAIL_F_NO_INTERRUPT or > > > > > > > > > > VRING_PACKED_EVENT_FLAG_DISABLE will not be set.So we update > > > > > > > > > > vring_used_event(&vq->split.vring) or vq->packed.vring.driver->off_wrap > > > > > > > > > > every time we call virtqueue_get_buf_ctx.This will bring more interruptions. > > > > > > > > > > > > > > > > > > Can you please post how to test with the performance numbers? > > > > > > > > > > > > > > > > > > > > > > > > > iperf3 tcp stream: > > > > > > > > vm1 -----------------> vm2 > > > > > > > > vm2 just receive tcp data stream from vm1, and send the ack to vm1, > > > > > > > > there are so > > > > > > > > many tx interruptions in vm2. > > > > > > > > > > > > > > > > but without event_triggered there are just a few tx interruptions. > > > > > > > > > > > > > > > > > > > > > > > > > > > > if event_triggered is set to true, do not update vring_used_event(&vq->split.vring) > > > > > > > > > > or vq->packed.vring.driver->off_wrap > > > > > > > > > > > > > > > > > > > > Signed-off-by: huangjie.albert <huangjie.albert@bytedance.com> > > > > > > > > > > --- > > > > > > > > > > drivers/virtio/virtio_ring.c | 6 ++++-- > > > > > > > > > > 1 file changed, 4 insertions(+), 2 deletions(-) > > > > > > > > > > > > > > > > > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c > > > > > > > > > > index 307e139cb11d..f486cccadbeb 100644 > > > > > > > > > > --- a/drivers/virtio/virtio_ring.c > > > > > > > > > > +++ b/drivers/virtio/virtio_ring.c > > > > > > > > > > @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, > > > > > > > > > > /* If we expect an interrupt for the next entry, tell host > > > > > > > > > > * by writing event index and flush out the write before > > > > > > > > > > * the read in the next get_buf call. */ > > > > > > > > > > - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) > > > > > > > > > > + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) > > > > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > > > > > > > > > > I'm not sure this can work, when event_triggered is true it means > > > > > > > > > we've got an interrupt, in this case if we want another interrupt for > > > > > > > > > the next entry, we should update used_event otherwise we will lose > > > > > > > > > that interrupt? > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > Normally, if we receive an interrupt, we should disable the interrupt > > > > > > > > in the interrupt callback handler. > > > > > > > > > > > > > > So the problem is: > > > > > > > > > > > > > > 1) event_triggered was set to true in vring_interrupt() > > > > > > > > > > > > > > 2) after this nothing will happen for virtqueue_disable_cb() so > > > > > > > VRING_AVAIL_F_NO_INTERRUPT is not set in avail_flags_shadow > > > > > > > 3) virtqueue_get_buf_ctx_split() will still think the cb is enabled > > > > > > > then it tries to publish new event > > > > > > > > > > > > Oh. Good point! I think when I wrote up > > > > > > 8d622d21d248 ("virtio: fix up virtio_disable_cb") > > > > > > I missed this corner case. > > > > > > > > > > > > > > > > > > > > > > > > > This makes me think about whether or not we really need > > > > > > > event_triggered. The assumption in the virtqueue_disable_cb() seems > > > > > > > wrong: > > > > > > > > > > > > > > /* If device triggered an event already it won't trigger one again: > > > > > > > * no need to disable. > > > > > > > */ > > > > > > > if (vq->event_triggered) > > > > > > > return; > > > > > > > > > > > > > > This is wrong if there's no event index support. > > > > > > > > > > > > > > > > > > I don't get it. how does this get triggered? > > > > > > > > > > > > You are talking about device without event index? > > > > > > Here's code from vring_interrupt(): > > > > > > > > > > > > /* Just a hint for performance: so it's ok that this can be racy! */ > > > > > > if (vq->event) > > > > > > vq->event_triggered = true; > > > > > > > > > > But we have the following in virtqueue_disable_cb(): > > > > > > > > > > /* If device triggered an event already it won't trigger one again: > > > > > * no need to disable. > > > > > */ > > > > > if (vq->event_triggered) > > > > > return; > > > > > > > > > > if (vq->packed_ring) > > > > > virtqueue_disable_cb_packed(_vq); > > > > > else > > > > > virtqueue_disable_cb_split(_vq); > > > > > > > > > > This means, without an event index, we don't set avail flags. So the > > > > > interrupt is not disabled actually in this case. > > > > > > > > > > Thanks > > > > > > > > Only if event_triggered is true, which without event index it never is. > > > > > > I'm not sure I will get here. I meant for example the commit > > > suppresses the effort of skb_xmit_done(): > > > > > > static void skb_xmit_done(struct virtqueue *vq) > > > { > > > struct virtnet_info *vi = vq->vdev->priv; > > > struct napi_struct *napi = &vi->sq[vq2txq(vq)].napi; > > > > > > /* Suppress further interrupts. */ > > > virtqueue_disable_cb(vq); > > > > > > The virtqueue_disable_cb() doesn't disable further interrupts when the > > > event index is not there. > > > > > > Thanks > > > > Check what can set event_triggered, you will see. > > Set to truth by vring_interrupt() vring_interrupt only sets it to true if vq->event is true > Set to false by virtqueue_init(), virtqueue_enable_cb_prepare(), > virtqueue_enable_cb_delayed() > > Assuming NAPI TX is enabled and the device doesn't support event index. > > 1) driver sends packets 1-10 > 2) the start_xmit() for the last packet will call > virtqueue_enable_cb_delayed() which set event_triggered = false > 3) 1st packet were sent, vring_interrupt set event_triggered = true > 4) skb_xmit_done() won't disable virtqueue_disable_cb() in this case > 5) so we will get the interrupts for 2nd to 10th packet > > Anything I missed here? 3 does not happen if event index is off. > Note the comment said it's used for event index: > > /* Hint for event idx: already triggered no need to disable. */ > bool event_triggered; > > I guess what you meant is that if we don't publish a new event, we > will get at most 1 interrupt for everything $queue_size used buffers. > But this is not the case without event index. Btw, it may supress the > effort of: > > vring_used_event(&vq->split.vring) = 0x0; > > Thanks Because it's not necessary then. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > And the > > > > > > > event_triggered is somehow duplicated with the > > > > > > > VRING_AVAIL_F_NO_INTERRUPT in the case of event index. The correct fix > > > > > > > might be: > > > > > > > > > > > > > > 1) remove event_triggered > > > > > > > 2) set VRING_AVAIL_F_NO_INTERRUPT in avail_flags_shadow in > > > > > > > vring_interrrupt if event index is supported > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > I am not sure all this is right and I'd rather we focused > > > > > > performance/correctness and cleanups separately. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > But because of the introduction of event_triggered, here, > > > > > > > > virtqueue_get_buf_ctx_split cannot be recognized > > > > > > > > that the interrupt has been turned off. > > > > > > > > > > > > > > > > if we want another interrupt for the next entry, We should probably > > > > > > > > call virtqueue_enable_cb? > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > > > > &vring_used_event(&vq->split.vring), > > > > > > > > > > cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); > > > > > > > > > > @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, > > > > > > > > > > * by writing event index and flush out the write before > > > > > > > > > > * the read in the next get_buf call. > > > > > > > > > > */ > > > > > > > > > > - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) > > > > > > > > > > + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC > > > > > > > > > > + && (vq->event_triggered == false)) > > > > > > > > > > virtio_store_mb(vq->weak_barriers, > > > > > > > > > > &vq->packed.vring.driver->off_wrap, > > > > > > > > > > cpu_to_le16(vq->last_used_idx)); > > > > > > > > > > -- > > > > > > > > > > 2.31.1 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c index 307e139cb11d..f486cccadbeb 100644 --- a/drivers/virtio/virtio_ring.c +++ b/drivers/virtio/virtio_ring.c @@ -795,7 +795,8 @@ static void *virtqueue_get_buf_ctx_split(struct virtqueue *_vq, /* If we expect an interrupt for the next entry, tell host * by writing event index and flush out the write before * the read in the next get_buf call. */ - if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT)) + if (!(vq->split.avail_flags_shadow & VRING_AVAIL_F_NO_INTERRUPT) + && (vq->event_triggered == false)) virtio_store_mb(vq->weak_barriers, &vring_used_event(&vq->split.vring), cpu_to_virtio16(_vq->vdev, vq->last_used_idx)); @@ -1529,7 +1530,8 @@ static void *virtqueue_get_buf_ctx_packed(struct virtqueue *_vq, * by writing event index and flush out the write before * the read in the next get_buf call. */ - if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC) + if (vq->packed.event_flags_shadow == VRING_PACKED_EVENT_FLAG_DESC + && (vq->event_triggered == false)) virtio_store_mb(vq->weak_barriers, &vq->packed.vring.driver->off_wrap, cpu_to_le16(vq->last_used_idx));