Message ID | 20230413064027.13267-2-jasowang@redhat.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a59:b0ea:0:b0:3b6:4342:cba0 with SMTP id b10csp837048vqo; Thu, 13 Apr 2023 00:01:15 -0700 (PDT) X-Google-Smtp-Source: AKy350YfLIZuuC9pGtCMn/5tqcAel0ByT/pcxqJodXDTKWUYfbdPkTB30yKCPca0oqFHmYxUZeQ+ X-Received: by 2002:a05:6402:1b0a:b0:504:b993:7dba with SMTP id by10-20020a0564021b0a00b00504b9937dbamr1576077edb.41.1681369275386; Thu, 13 Apr 2023 00:01:15 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1681369275; cv=none; d=google.com; s=arc-20160816; b=cLOQqXJRqei7MnHGZPunOCC5I95Lhtm5hNq40whR9NKgSadxHi6+Rh8F8FW98OdNDP RDOCO6c/z/35H/fetd+oXtYrlAbBIuLEWOpU/LOQR8QL4WLOOMl/R2R18G9c/bHs2Jp7 YRHxtv6vKSrTxY6YeODuoQK4klfCTQJYQFac06OPM+F5fHZsUo547rdFp3HoWkIYZPU9 pNZqKNiQne8UiCCNjeNcy+aQGAsAy6+Ig0Mjt1pLX4fhqd4uEShn0OY2TTsrQ0o6JQUb bLK1NvjuWxajgrADnIvUxuWaDN9A8AEFkv08cDDnUl9tdBIaK/j1jt0SDKZjB8AUaMu5 Sz8Q== 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 :references:in-reply-to:message-id:date:subject:cc:to:from :dkim-signature; bh=WTB3CgJRk7J5IzkSOYPigSPFDZTD53vvUVJW3ncH3HA=; b=j9xCEX84GoXvjc+Eg4TeCTmJIbJUlFOqQrAdJ+QHRlJlcnsLYEw1v9ABaL6bQRZj+L 3OT5opLrjCsRE7XGqUHYhFiHqcippLUDFZVgVxREVDgsPvYDz0s6uuQwbWSZkdoPO5i1 yScfomUZy6d3ChyDmkz4N63nVlQGhGXkLdlA8KBVZeTWTogMwwHlIkkNXWn+a9R9+zNF prpmjsr2n/BkP2bewsw06TUyedP6n0i2Gs7htMftHpCojAsIIZYY1Z0nxQYBJQhhboLI rhsFXfQ65hYa/RbYoYLdyC2wTYFoQ4AUZn6holFWUvPSilD6R8zy22T7Z8Q9Zk2VOssk 0xxw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@redhat.com header.s=mimecast20190719 header.b=GceBONcl; 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=NONE sp=NONE dis=NONE) header.from=redhat.com Received: from out1.vger.email (out1.vger.email. [2620:137:e000::1:20]) by mx.google.com with ESMTP id s15-20020aa7c54f000000b00501e59e6198si1151294edr.613.2023.04.13.00.00.51; Thu, 13 Apr 2023 00:01:15 -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=@redhat.com header.s=mimecast20190719 header.b=GceBONcl; 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=NONE sp=NONE dis=NONE) header.from=redhat.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229864AbjDMGll (ORCPT <rfc822;peter110.wang@gmail.com> + 99 others); Thu, 13 Apr 2023 02:41:41 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:48842 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229845AbjDMGld (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Thu, 13 Apr 2023 02:41:33 -0400 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5ED938A58 for <linux-kernel@vger.kernel.org>; Wed, 12 Apr 2023 23:40:49 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1681368048; 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=WTB3CgJRk7J5IzkSOYPigSPFDZTD53vvUVJW3ncH3HA=; b=GceBONcl/ryI8DyMdPXrzKfGaMsWQsq//v0dQh8OOj6j6AK4qa1FDrHxGezsQnHUVAAurl nkBXLs1rKLpPwCgrlQap8MLP93P1TBtg6GknDrXjfdnHZHD45o/QIQ+VJXw7BoiM6AN9lM rk8dMqHBMbS4Tda82JeuLh0X4JMfOiY= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-411-tcTRVZCjPderlQhfr7Cpkg-1; Thu, 13 Apr 2023 02:40:42 -0400 X-MC-Unique: tcTRVZCjPderlQhfr7Cpkg-1 Received: from smtp.corp.redhat.com (int-mx02.intmail.prod.int.rdu2.redhat.com [10.11.54.2]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 5B6823C1068B; Thu, 13 Apr 2023 06:40:42 +0000 (UTC) Received: from localhost.localdomain (ovpn-12-72.pek2.redhat.com [10.72.12.72]) by smtp.corp.redhat.com (Postfix) with ESMTP id B878B40C6E20; Thu, 13 Apr 2023 06:40:36 +0000 (UTC) From: Jason Wang <jasowang@redhat.com> To: mst@redhat.com, jasowang@redhat.com Cc: davem@davemloft.net, edumazet@google.com, kuba@kernel.org, pabeni@redhat.com, virtualization@lists.linux-foundation.org, linux-kernel@vger.kernel.org, maxime.coquelin@redhat.com, alvaro.karsz@solid-run.com, eperezma@redhat.com, xuanzhuo@linux.alibaba.com, david.marchand@redhat.com Subject: [PATCH net-next V2 1/2] virtio-net: convert rx mode setting to use workqueue Date: Thu, 13 Apr 2023 14:40:26 +0800 Message-Id: <20230413064027.13267-2-jasowang@redhat.com> In-Reply-To: <20230413064027.13267-1-jasowang@redhat.com> References: <20230413064027.13267-1-jasowang@redhat.com> MIME-Version: 1.0 Content-type: text/plain Content-Transfer-Encoding: 8bit X-Scanned-By: MIMEDefang 3.1 on 10.11.54.2 X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_NONE, RCVD_IN_MSPIKE_H2,SPF_HELO_NONE,SPF_NONE 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?1763043469048454501?= X-GMAIL-MSGID: =?utf-8?q?1763043469048454501?= |
Series |
virtio-net: don't busy poll for cvq command
|
|
Commit Message
Jason Wang
April 13, 2023, 6:40 a.m. UTC
This patch convert rx mode setting to be done in a workqueue, this is
a must for allow to sleep when waiting for the cvq command to
response since current code is executed under addr spin lock.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes since V1:
- use RTNL to synchronize rx mode worker
---
drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++---
1 file changed, 52 insertions(+), 3 deletions(-)
Comments
On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > This patch convert rx mode setting to be done in a workqueue, this is > a must for allow to sleep when waiting for the cvq command to > response since current code is executed under addr spin lock. > > Signed-off-by: Jason Wang <jasowang@redhat.com> I don't like this frankly. This means that setting RX mode which would previously be reliable, now becomes unreliable. - first of all configuration is no longer immediate and there is no way for driver to find out when it actually took effect - second, if device fails command, this is also not propagated to driver, again no way for driver to find out VDUSE needs to be fixed to do tricks to fix this without breaking normal drivers. > --- > Changes since V1: > - use RTNL to synchronize rx mode worker > --- > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > 1 file changed, 52 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > index e2560b6f7980..2e56bbf86894 100644 > --- a/drivers/net/virtio_net.c > +++ b/drivers/net/virtio_net.c > @@ -265,6 +265,12 @@ struct virtnet_info { > /* Work struct for config space updates */ > struct work_struct config_work; > > + /* Work struct for config rx mode */ > + struct work_struct rx_mode_work; > + > + /* Is rx mode work enabled? */ > + bool rx_mode_work_enabled; > + > /* Does the affinity hint is set for virtqueues? */ > bool affinity_hint_set; > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > spin_unlock_bh(&vi->refill_lock); > } > > +static void enable_rx_mode_work(struct virtnet_info *vi) > +{ > + rtnl_lock(); > + vi->rx_mode_work_enabled = true; > + rtnl_unlock(); > +} > + > +static void disable_rx_mode_work(struct virtnet_info *vi) > +{ > + rtnl_lock(); > + vi->rx_mode_work_enabled = false; > + rtnl_unlock(); > +} > + > static void virtqueue_napi_schedule(struct napi_struct *napi, > struct virtqueue *vq) > { > @@ -2310,9 +2330,11 @@ static int virtnet_close(struct net_device *dev) > return 0; > } > > -static void virtnet_set_rx_mode(struct net_device *dev) > +static void virtnet_rx_mode_work(struct work_struct *work) > { > - struct virtnet_info *vi = netdev_priv(dev); > + struct virtnet_info *vi = > + container_of(work, struct virtnet_info, rx_mode_work); > + struct net_device *dev = vi->dev; > struct scatterlist sg[2]; > struct virtio_net_ctrl_mac *mac_data; > struct netdev_hw_addr *ha; > @@ -2325,6 +2347,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > return; > > + rtnl_lock(); > + > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > @@ -2342,14 +2366,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > vi->ctrl->allmulti ? "en" : "dis"); > > + netif_addr_lock_bh(dev); > + > uc_count = netdev_uc_count(dev); > mc_count = netdev_mc_count(dev); > /* MAC filter - use one buffer for both lists */ > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > mac_data = buf; > - if (!buf) > + if (!buf) { > + netif_addr_unlock_bh(dev); > + rtnl_unlock(); > return; > + } > > sg_init_table(sg, 2); > > @@ -2370,6 +2399,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > netdev_for_each_mc_addr(ha, dev) > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > + netif_addr_unlock_bh(dev); > + > sg_set_buf(&sg[1], mac_data, > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > @@ -2377,9 +2408,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > + rtnl_unlock(); > + > kfree(buf); > } > > +static void virtnet_set_rx_mode(struct net_device *dev) > +{ > + struct virtnet_info *vi = netdev_priv(dev); > + > + if (vi->rx_mode_work_enabled) > + schedule_work(&vi->rx_mode_work); > +} > + > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > __be16 proto, u16 vid) > { > @@ -3150,6 +3191,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > /* Make sure no work handler is accessing the device */ > flush_work(&vi->config_work); > + disable_rx_mode_work(vi); > + flush_work(&vi->rx_mode_work); > > netif_tx_lock_bh(vi->dev); > netif_device_detach(vi->dev); So now configuration is not propagated to device. Won't device later wake up in wrong state? > @@ -3172,6 +3215,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > virtio_device_ready(vdev); > > enable_delayed_refill(vi); > + enable_rx_mode_work(vi); > > if (netif_running(vi->dev)) { > err = virtnet_open(vi->dev); > @@ -3969,6 +4013,7 @@ static int virtnet_probe(struct virtio_device *vdev) > vdev->priv = vi; > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > spin_lock_init(&vi->refill_lock); > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > @@ -4077,6 +4122,8 @@ static int virtnet_probe(struct virtio_device *vdev) > if (vi->has_rss || vi->has_rss_hash_report) > virtnet_init_default_rss(vi); > > + enable_rx_mode_work(vi); > + > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > rtnl_lock(); > > @@ -4174,6 +4221,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > /* Make sure no work handler is accessing the device. */ > flush_work(&vi->config_work); > + disable_rx_mode_work(vi); > + flush_work(&vi->rx_mode_work); > > unregister_netdev(vi->dev); > > -- > 2.25.1
Forget to cc netdev, adding. On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > This patch convert rx mode setting to be done in a workqueue, this is > > a must for allow to sleep when waiting for the cvq command to > > response since current code is executed under addr spin lock. > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > I don't like this frankly. This means that setting RX mode which would > previously be reliable, now becomes unreliable. It is "unreliable" by design: void (*ndo_set_rx_mode)(struct net_device *dev); > - first of all configuration is no longer immediate Is immediate a hard requirement? I can see a workqueue is used at least: mlx5e, ipoib, efx, ... > and there is no way for driver to find out when > it actually took effect But we know rx mode is best effort e.g it doesn't support vhost and we survive from this for years. > - second, if device fails command, this is also not > propagated to driver, again no way for driver to find out > > VDUSE needs to be fixed to do tricks to fix this > without breaking normal drivers. It's not specific to VDUSE. For example, when using virtio-net in the UP environment with any software cvq (like mlx5 via vDPA or cma transport). Thanks > > > > --- > > Changes since V1: > > - use RTNL to synchronize rx mode worker > > --- > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > index e2560b6f7980..2e56bbf86894 100644 > > --- a/drivers/net/virtio_net.c > > +++ b/drivers/net/virtio_net.c > > @@ -265,6 +265,12 @@ struct virtnet_info { > > /* Work struct for config space updates */ > > struct work_struct config_work; > > > > + /* Work struct for config rx mode */ > > + struct work_struct rx_mode_work; > > + > > + /* Is rx mode work enabled? */ > > + bool rx_mode_work_enabled; > > + > > /* Does the affinity hint is set for virtqueues? */ > > bool affinity_hint_set; > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > spin_unlock_bh(&vi->refill_lock); > > } > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > +{ > > + rtnl_lock(); > > + vi->rx_mode_work_enabled = true; > > + rtnl_unlock(); > > +} > > + > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > +{ > > + rtnl_lock(); > > + vi->rx_mode_work_enabled = false; > > + rtnl_unlock(); > > +} > > + > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > struct virtqueue *vq) > > { > > @@ -2310,9 +2330,11 @@ static int virtnet_close(struct net_device *dev) > > return 0; > > } > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > +static void virtnet_rx_mode_work(struct work_struct *work) > > { > > - struct virtnet_info *vi = netdev_priv(dev); > > + struct virtnet_info *vi = > > + container_of(work, struct virtnet_info, rx_mode_work); > > + struct net_device *dev = vi->dev; > > struct scatterlist sg[2]; > > struct virtio_net_ctrl_mac *mac_data; > > struct netdev_hw_addr *ha; > > @@ -2325,6 +2347,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > return; > > > > + rtnl_lock(); > > + > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > @@ -2342,14 +2366,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > vi->ctrl->allmulti ? "en" : "dis"); > > > > + netif_addr_lock_bh(dev); > > + > > uc_count = netdev_uc_count(dev); > > mc_count = netdev_mc_count(dev); > > /* MAC filter - use one buffer for both lists */ > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > mac_data = buf; > > - if (!buf) > > + if (!buf) { > > + netif_addr_unlock_bh(dev); > > + rtnl_unlock(); > > return; > > + } > > > > sg_init_table(sg, 2); > > > > @@ -2370,6 +2399,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > netdev_for_each_mc_addr(ha, dev) > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > + netif_addr_unlock_bh(dev); > > + > > sg_set_buf(&sg[1], mac_data, > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > @@ -2377,9 +2408,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > + rtnl_unlock(); > > + > > kfree(buf); > > } > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > +{ > > + struct virtnet_info *vi = netdev_priv(dev); > > + > > + if (vi->rx_mode_work_enabled) > > + schedule_work(&vi->rx_mode_work); > > +} > > + > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > __be16 proto, u16 vid) > > { > > @@ -3150,6 +3191,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > /* Make sure no work handler is accessing the device */ > > flush_work(&vi->config_work); > > + disable_rx_mode_work(vi); > > + flush_work(&vi->rx_mode_work); > > > > netif_tx_lock_bh(vi->dev); > > netif_device_detach(vi->dev); > > So now configuration is not propagated to device. > Won't device later wake up in wrong state? > > > > @@ -3172,6 +3215,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > virtio_device_ready(vdev); > > > > enable_delayed_refill(vi); > > + enable_rx_mode_work(vi); > > > > if (netif_running(vi->dev)) { > > err = virtnet_open(vi->dev); > > @@ -3969,6 +4013,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > vdev->priv = vi; > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > spin_lock_init(&vi->refill_lock); > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > @@ -4077,6 +4122,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > if (vi->has_rss || vi->has_rss_hash_report) > > virtnet_init_default_rss(vi); > > > > + enable_rx_mode_work(vi); > > + > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > rtnl_lock(); > > > > @@ -4174,6 +4221,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > /* Make sure no work handler is accessing the device. */ > > flush_work(&vi->config_work); > > + disable_rx_mode_work(vi); > > + flush_work(&vi->rx_mode_work); > > > > unregister_netdev(vi->dev); > > > > -- > > 2.25.1 >
On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > Forget to cc netdev, adding. > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > This patch convert rx mode setting to be done in a workqueue, this is > > > a must for allow to sleep when waiting for the cvq command to > > > response since current code is executed under addr spin lock. > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > I don't like this frankly. This means that setting RX mode which would > > previously be reliable, now becomes unreliable. > > It is "unreliable" by design: > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > - first of all configuration is no longer immediate > > Is immediate a hard requirement? I can see a workqueue is used at least: > > mlx5e, ipoib, efx, ... > > > and there is no way for driver to find out when > > it actually took effect > > But we know rx mode is best effort e.g it doesn't support vhost and we > survive from this for years. > > > - second, if device fails command, this is also not > > propagated to driver, again no way for driver to find out > > > > VDUSE needs to be fixed to do tricks to fix this > > without breaking normal drivers. > > It's not specific to VDUSE. For example, when using virtio-net in the > UP environment with any software cvq (like mlx5 via vDPA or cma > transport). > > Thanks Hmm. Can we differentiate between these use-cases? > > > > > > > --- > > > Changes since V1: > > > - use RTNL to synchronize rx mode worker > > > --- > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > index e2560b6f7980..2e56bbf86894 100644 > > > --- a/drivers/net/virtio_net.c > > > +++ b/drivers/net/virtio_net.c > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > /* Work struct for config space updates */ > > > struct work_struct config_work; > > > > > > + /* Work struct for config rx mode */ > > > + struct work_struct rx_mode_work; > > > + > > > + /* Is rx mode work enabled? */ > > > + bool rx_mode_work_enabled; > > > + > > > /* Does the affinity hint is set for virtqueues? */ > > > bool affinity_hint_set; > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > spin_unlock_bh(&vi->refill_lock); > > > } > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > +{ > > > + rtnl_lock(); > > > + vi->rx_mode_work_enabled = true; > > > + rtnl_unlock(); > > > +} > > > + > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > +{ > > > + rtnl_lock(); > > > + vi->rx_mode_work_enabled = false; > > > + rtnl_unlock(); > > > +} > > > + > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > struct virtqueue *vq) > > > { > > > @@ -2310,9 +2330,11 @@ static int virtnet_close(struct net_device *dev) > > > return 0; > > > } > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > { > > > - struct virtnet_info *vi = netdev_priv(dev); > > > + struct virtnet_info *vi = > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > + struct net_device *dev = vi->dev; > > > struct scatterlist sg[2]; > > > struct virtio_net_ctrl_mac *mac_data; > > > struct netdev_hw_addr *ha; > > > @@ -2325,6 +2347,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > return; > > > > > > + rtnl_lock(); > > > + > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > @@ -2342,14 +2366,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > + netif_addr_lock_bh(dev); > > > + > > > uc_count = netdev_uc_count(dev); > > > mc_count = netdev_mc_count(dev); > > > /* MAC filter - use one buffer for both lists */ > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > mac_data = buf; > > > - if (!buf) > > > + if (!buf) { > > > + netif_addr_unlock_bh(dev); > > > + rtnl_unlock(); > > > return; > > > + } > > > > > > sg_init_table(sg, 2); > > > > > > @@ -2370,6 +2399,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > netdev_for_each_mc_addr(ha, dev) > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > + netif_addr_unlock_bh(dev); > > > + > > > sg_set_buf(&sg[1], mac_data, > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > @@ -2377,9 +2408,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > + rtnl_unlock(); > > > + > > > kfree(buf); > > > } > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > +{ > > > + struct virtnet_info *vi = netdev_priv(dev); > > > + > > > + if (vi->rx_mode_work_enabled) > > > + schedule_work(&vi->rx_mode_work); > > > +} > > > + > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > __be16 proto, u16 vid) > > > { > > > @@ -3150,6 +3191,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > /* Make sure no work handler is accessing the device */ > > > flush_work(&vi->config_work); > > > + disable_rx_mode_work(vi); > > > + flush_work(&vi->rx_mode_work); > > > > > > netif_tx_lock_bh(vi->dev); > > > netif_device_detach(vi->dev); > > > > So now configuration is not propagated to device. > > Won't device later wake up in wrong state? > > > > > > > @@ -3172,6 +3215,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > virtio_device_ready(vdev); > > > > > > enable_delayed_refill(vi); > > > + enable_rx_mode_work(vi); > > > > > > if (netif_running(vi->dev)) { > > > err = virtnet_open(vi->dev); > > > @@ -3969,6 +4013,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > vdev->priv = vi; > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > spin_lock_init(&vi->refill_lock); > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > @@ -4077,6 +4122,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > if (vi->has_rss || vi->has_rss_hash_report) > > > virtnet_init_default_rss(vi); > > > > > > + enable_rx_mode_work(vi); > > > + > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > rtnl_lock(); > > > > > > @@ -4174,6 +4221,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > /* Make sure no work handler is accessing the device. */ > > > flush_work(&vi->config_work); > > > + disable_rx_mode_work(vi); > > > + flush_work(&vi->rx_mode_work); > > > > > > unregister_netdev(vi->dev); > > > > > > -- > > > 2.25.1 > >
On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > Forget to cc netdev, adding. > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > a must for allow to sleep when waiting for the cvq command to > > > > response since current code is executed under addr spin lock. > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > I don't like this frankly. This means that setting RX mode which would > > > previously be reliable, now becomes unreliable. > > > > It is "unreliable" by design: > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > - first of all configuration is no longer immediate > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > mlx5e, ipoib, efx, ... > > > > > and there is no way for driver to find out when > > > it actually took effect > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > survive from this for years. > > > > > - second, if device fails command, this is also not > > > propagated to driver, again no way for driver to find out > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > without breaking normal drivers. > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > UP environment with any software cvq (like mlx5 via vDPA or cma > > transport). > > > > Thanks > > Hmm. Can we differentiate between these use-cases? It doesn't look easy since we are drivers for virtio bus. Underlayer details were hidden from virtio-net. Or do you have any ideas on this? Thanks > > > > > > > > > > > --- > > > > Changes since V1: > > > > - use RTNL to synchronize rx mode worker > > > > --- > > > > drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- > > > > 1 file changed, 52 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c > > > > index e2560b6f7980..2e56bbf86894 100644 > > > > --- a/drivers/net/virtio_net.c > > > > +++ b/drivers/net/virtio_net.c > > > > @@ -265,6 +265,12 @@ struct virtnet_info { > > > > /* Work struct for config space updates */ > > > > struct work_struct config_work; > > > > > > > > + /* Work struct for config rx mode */ > > > > + struct work_struct rx_mode_work; > > > > + > > > > + /* Is rx mode work enabled? */ > > > > + bool rx_mode_work_enabled; > > > > + > > > > /* Does the affinity hint is set for virtqueues? */ > > > > bool affinity_hint_set; > > > > > > > > @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) > > > > spin_unlock_bh(&vi->refill_lock); > > > > } > > > > > > > > +static void enable_rx_mode_work(struct virtnet_info *vi) > > > > +{ > > > > + rtnl_lock(); > > > > + vi->rx_mode_work_enabled = true; > > > > + rtnl_unlock(); > > > > +} > > > > + > > > > +static void disable_rx_mode_work(struct virtnet_info *vi) > > > > +{ > > > > + rtnl_lock(); > > > > + vi->rx_mode_work_enabled = false; > > > > + rtnl_unlock(); > > > > +} > > > > + > > > > static void virtqueue_napi_schedule(struct napi_struct *napi, > > > > struct virtqueue *vq) > > > > { > > > > @@ -2310,9 +2330,11 @@ static int virtnet_close(struct net_device *dev) > > > > return 0; > > > > } > > > > > > > > -static void virtnet_set_rx_mode(struct net_device *dev) > > > > +static void virtnet_rx_mode_work(struct work_struct *work) > > > > { > > > > - struct virtnet_info *vi = netdev_priv(dev); > > > > + struct virtnet_info *vi = > > > > + container_of(work, struct virtnet_info, rx_mode_work); > > > > + struct net_device *dev = vi->dev; > > > > struct scatterlist sg[2]; > > > > struct virtio_net_ctrl_mac *mac_data; > > > > struct netdev_hw_addr *ha; > > > > @@ -2325,6 +2347,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) > > > > return; > > > > > > > > + rtnl_lock(); > > > > + > > > > vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); > > > > vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); > > > > > > > > @@ -2342,14 +2366,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", > > > > vi->ctrl->allmulti ? "en" : "dis"); > > > > > > > > + netif_addr_lock_bh(dev); > > > > + > > > > uc_count = netdev_uc_count(dev); > > > > mc_count = netdev_mc_count(dev); > > > > /* MAC filter - use one buffer for both lists */ > > > > buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + > > > > (2 * sizeof(mac_data->entries)), GFP_ATOMIC); > > > > mac_data = buf; > > > > - if (!buf) > > > > + if (!buf) { > > > > + netif_addr_unlock_bh(dev); > > > > + rtnl_unlock(); > > > > return; > > > > + } > > > > > > > > sg_init_table(sg, 2); > > > > > > > > @@ -2370,6 +2399,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > netdev_for_each_mc_addr(ha, dev) > > > > memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); > > > > > > > > + netif_addr_unlock_bh(dev); > > > > + > > > > sg_set_buf(&sg[1], mac_data, > > > > sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); > > > > > > > > @@ -2377,9 +2408,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) > > > > VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) > > > > dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); > > > > > > > > + rtnl_unlock(); > > > > + > > > > kfree(buf); > > > > } > > > > > > > > +static void virtnet_set_rx_mode(struct net_device *dev) > > > > +{ > > > > + struct virtnet_info *vi = netdev_priv(dev); > > > > + > > > > + if (vi->rx_mode_work_enabled) > > > > + schedule_work(&vi->rx_mode_work); > > > > +} > > > > + > > > > static int virtnet_vlan_rx_add_vid(struct net_device *dev, > > > > __be16 proto, u16 vid) > > > > { > > > > @@ -3150,6 +3191,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) > > > > > > > > /* Make sure no work handler is accessing the device */ > > > > flush_work(&vi->config_work); > > > > + disable_rx_mode_work(vi); > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > netif_tx_lock_bh(vi->dev); > > > > netif_device_detach(vi->dev); > > > > > > So now configuration is not propagated to device. > > > Won't device later wake up in wrong state? > > > > > > > > > > @@ -3172,6 +3215,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) > > > > virtio_device_ready(vdev); > > > > > > > > enable_delayed_refill(vi); > > > > + enable_rx_mode_work(vi); > > > > > > > > if (netif_running(vi->dev)) { > > > > err = virtnet_open(vi->dev); > > > > @@ -3969,6 +4013,7 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > vdev->priv = vi; > > > > > > > > INIT_WORK(&vi->config_work, virtnet_config_changed_work); > > > > + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); > > > > spin_lock_init(&vi->refill_lock); > > > > > > > > if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { > > > > @@ -4077,6 +4122,8 @@ static int virtnet_probe(struct virtio_device *vdev) > > > > if (vi->has_rss || vi->has_rss_hash_report) > > > > virtnet_init_default_rss(vi); > > > > > > > > + enable_rx_mode_work(vi); > > > > + > > > > /* serialize netdev register + virtio_device_ready() with ndo_open() */ > > > > rtnl_lock(); > > > > > > > > @@ -4174,6 +4221,8 @@ static void virtnet_remove(struct virtio_device *vdev) > > > > > > > > /* Make sure no work handler is accessing the device. */ > > > > flush_work(&vi->config_work); > > > > + disable_rx_mode_work(vi); > > > > + flush_work(&vi->rx_mode_work); > > > > > > > > unregister_netdev(vi->dev); > > > > > > > > -- > > > > 2.25.1 > > > >
在 2023/4/17 11:40, Jason Wang 写道: > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: >> On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: >>> Forget to cc netdev, adding. >>> >>> On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: >>>> On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: >>>>> This patch convert rx mode setting to be done in a workqueue, this is >>>>> a must for allow to sleep when waiting for the cvq command to >>>>> response since current code is executed under addr spin lock. >>>>> >>>>> Signed-off-by: Jason Wang <jasowang@redhat.com> >>>> I don't like this frankly. This means that setting RX mode which would >>>> previously be reliable, now becomes unreliable. >>> It is "unreliable" by design: >>> >>> void (*ndo_set_rx_mode)(struct net_device *dev); >>> >>>> - first of all configuration is no longer immediate >>> Is immediate a hard requirement? I can see a workqueue is used at least: >>> >>> mlx5e, ipoib, efx, ... >>> >>>> and there is no way for driver to find out when >>>> it actually took effect >>> But we know rx mode is best effort e.g it doesn't support vhost and we >>> survive from this for years. >>> >>>> - second, if device fails command, this is also not >>>> propagated to driver, again no way for driver to find out >>>> >>>> VDUSE needs to be fixed to do tricks to fix this >>>> without breaking normal drivers. >>> It's not specific to VDUSE. For example, when using virtio-net in the >>> UP environment with any software cvq (like mlx5 via vDPA or cma >>> transport). >>> >>> Thanks >> Hmm. Can we differentiate between these use-cases? > It doesn't look easy since we are drivers for virtio bus. Underlayer > details were hidden from virtio-net. > > Or do you have any ideas on this? Michael, any thought on this? Thanks > > Thanks > >>>> >>>>> --- >>>>> Changes since V1: >>>>> - use RTNL to synchronize rx mode worker >>>>> --- >>>>> drivers/net/virtio_net.c | 55 +++++++++++++++++++++++++++++++++++++--- >>>>> 1 file changed, 52 insertions(+), 3 deletions(-) >>>>> >>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c >>>>> index e2560b6f7980..2e56bbf86894 100644 >>>>> --- a/drivers/net/virtio_net.c >>>>> +++ b/drivers/net/virtio_net.c >>>>> @@ -265,6 +265,12 @@ struct virtnet_info { >>>>> /* Work struct for config space updates */ >>>>> struct work_struct config_work; >>>>> >>>>> + /* Work struct for config rx mode */ >>>>> + struct work_struct rx_mode_work; >>>>> + >>>>> + /* Is rx mode work enabled? */ >>>>> + bool rx_mode_work_enabled; >>>>> + >>>>> /* Does the affinity hint is set for virtqueues? */ >>>>> bool affinity_hint_set; >>>>> >>>>> @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) >>>>> spin_unlock_bh(&vi->refill_lock); >>>>> } >>>>> >>>>> +static void enable_rx_mode_work(struct virtnet_info *vi) >>>>> +{ >>>>> + rtnl_lock(); >>>>> + vi->rx_mode_work_enabled = true; >>>>> + rtnl_unlock(); >>>>> +} >>>>> + >>>>> +static void disable_rx_mode_work(struct virtnet_info *vi) >>>>> +{ >>>>> + rtnl_lock(); >>>>> + vi->rx_mode_work_enabled = false; >>>>> + rtnl_unlock(); >>>>> +} >>>>> + >>>>> static void virtqueue_napi_schedule(struct napi_struct *napi, >>>>> struct virtqueue *vq) >>>>> { >>>>> @@ -2310,9 +2330,11 @@ static int virtnet_close(struct net_device *dev) >>>>> return 0; >>>>> } >>>>> >>>>> -static void virtnet_set_rx_mode(struct net_device *dev) >>>>> +static void virtnet_rx_mode_work(struct work_struct *work) >>>>> { >>>>> - struct virtnet_info *vi = netdev_priv(dev); >>>>> + struct virtnet_info *vi = >>>>> + container_of(work, struct virtnet_info, rx_mode_work); >>>>> + struct net_device *dev = vi->dev; >>>>> struct scatterlist sg[2]; >>>>> struct virtio_net_ctrl_mac *mac_data; >>>>> struct netdev_hw_addr *ha; >>>>> @@ -2325,6 +2347,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) >>>>> if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) >>>>> return; >>>>> >>>>> + rtnl_lock(); >>>>> + >>>>> vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); >>>>> vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); >>>>> >>>>> @@ -2342,14 +2366,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) >>>>> dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", >>>>> vi->ctrl->allmulti ? "en" : "dis"); >>>>> >>>>> + netif_addr_lock_bh(dev); >>>>> + >>>>> uc_count = netdev_uc_count(dev); >>>>> mc_count = netdev_mc_count(dev); >>>>> /* MAC filter - use one buffer for both lists */ >>>>> buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + >>>>> (2 * sizeof(mac_data->entries)), GFP_ATOMIC); >>>>> mac_data = buf; >>>>> - if (!buf) >>>>> + if (!buf) { >>>>> + netif_addr_unlock_bh(dev); >>>>> + rtnl_unlock(); >>>>> return; >>>>> + } >>>>> >>>>> sg_init_table(sg, 2); >>>>> >>>>> @@ -2370,6 +2399,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) >>>>> netdev_for_each_mc_addr(ha, dev) >>>>> memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); >>>>> >>>>> + netif_addr_unlock_bh(dev); >>>>> + >>>>> sg_set_buf(&sg[1], mac_data, >>>>> sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); >>>>> >>>>> @@ -2377,9 +2408,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) >>>>> VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) >>>>> dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); >>>>> >>>>> + rtnl_unlock(); >>>>> + >>>>> kfree(buf); >>>>> } >>>>> >>>>> +static void virtnet_set_rx_mode(struct net_device *dev) >>>>> +{ >>>>> + struct virtnet_info *vi = netdev_priv(dev); >>>>> + >>>>> + if (vi->rx_mode_work_enabled) >>>>> + schedule_work(&vi->rx_mode_work); >>>>> +} >>>>> + >>>>> static int virtnet_vlan_rx_add_vid(struct net_device *dev, >>>>> __be16 proto, u16 vid) >>>>> { >>>>> @@ -3150,6 +3191,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) >>>>> >>>>> /* Make sure no work handler is accessing the device */ >>>>> flush_work(&vi->config_work); >>>>> + disable_rx_mode_work(vi); >>>>> + flush_work(&vi->rx_mode_work); >>>>> >>>>> netif_tx_lock_bh(vi->dev); >>>>> netif_device_detach(vi->dev); >>>> So now configuration is not propagated to device. >>>> Won't device later wake up in wrong state? >>>> >>>> >>>>> @@ -3172,6 +3215,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) >>>>> virtio_device_ready(vdev); >>>>> >>>>> enable_delayed_refill(vi); >>>>> + enable_rx_mode_work(vi); >>>>> >>>>> if (netif_running(vi->dev)) { >>>>> err = virtnet_open(vi->dev); >>>>> @@ -3969,6 +4013,7 @@ static int virtnet_probe(struct virtio_device *vdev) >>>>> vdev->priv = vi; >>>>> >>>>> INIT_WORK(&vi->config_work, virtnet_config_changed_work); >>>>> + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); >>>>> spin_lock_init(&vi->refill_lock); >>>>> >>>>> if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { >>>>> @@ -4077,6 +4122,8 @@ static int virtnet_probe(struct virtio_device *vdev) >>>>> if (vi->has_rss || vi->has_rss_hash_report) >>>>> virtnet_init_default_rss(vi); >>>>> >>>>> + enable_rx_mode_work(vi); >>>>> + >>>>> /* serialize netdev register + virtio_device_ready() with ndo_open() */ >>>>> rtnl_lock(); >>>>> >>>>> @@ -4174,6 +4221,8 @@ static void virtnet_remove(struct virtio_device *vdev) >>>>> >>>>> /* Make sure no work handler is accessing the device. */ >>>>> flush_work(&vi->config_work); >>>>> + disable_rx_mode_work(vi); >>>>> + flush_work(&vi->rx_mode_work); >>>>> >>>>> unregister_netdev(vi->dev); >>>>> >>>>> -- >>>>> 2.25.1
On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > Forget to cc netdev, adding. > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > previously be reliable, now becomes unreliable. > > > > > > It is "unreliable" by design: > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > - first of all configuration is no longer immediate > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > mlx5e, ipoib, efx, ... > > > > > > > and there is no way for driver to find out when > > > > it actually took effect > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > survive from this for years. > > > > > > > - second, if device fails command, this is also not > > > > propagated to driver, again no way for driver to find out > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > without breaking normal drivers. > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > transport). > > > > > > Thanks > > > > Hmm. Can we differentiate between these use-cases? > > It doesn't look easy since we are drivers for virtio bus. Underlayer > details were hidden from virtio-net. > > Or do you have any ideas on this? > > Thanks I don't know, pass some kind of flag in struct virtqueue? "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" ?
On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > Forget to cc netdev, adding. > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > previously be reliable, now becomes unreliable. > > > > > > > > It is "unreliable" by design: > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > and there is no way for driver to find out when > > > > > it actually took effect > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > survive from this for years. > > > > > > > > > - second, if device fails command, this is also not > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > without breaking normal drivers. > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > transport). > > > > > > > > Thanks > > > > > > Hmm. Can we differentiate between these use-cases? > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > details were hidden from virtio-net. > > > > Or do you have any ideas on this? > > > > Thanks > > I don't know, pass some kind of flag in struct virtqueue? > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > ? > So if it's slow, sleep, otherwise poll? I feel setting this flag might be tricky, since the driver doesn't know whether or not it's really slow. E.g smartNIC vendor may allow virtio-net emulation over PCI. Thanks > -- > MST >
On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > Forget to cc netdev, adding. > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > and there is no way for driver to find out when > > > > > > it actually took effect > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > survive from this for years. > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > without breaking normal drivers. > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > transport). > > > > > > > > > > Thanks > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > details were hidden from virtio-net. > > > > > > Or do you have any ideas on this? > > > > > > Thanks > > > > I don't know, pass some kind of flag in struct virtqueue? > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > ? > > > > So if it's slow, sleep, otherwise poll? > > I feel setting this flag might be tricky, since the driver doesn't > know whether or not it's really slow. E.g smartNIC vendor may allow > virtio-net emulation over PCI. > > Thanks driver will have the choice, depending on whether vq is deterministic or not. > > -- > > MST > >
On Mon, May 15, 2023 at 12:45 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > > Forget to cc netdev, adding. > > > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > > > and there is no way for driver to find out when > > > > > > > it actually took effect > > > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > > survive from this for years. > > > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > > without breaking normal drivers. > > > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > > transport). > > > > > > > > > > > > Thanks > > > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > > details were hidden from virtio-net. > > > > > > > > Or do you have any ideas on this? > > > > > > > > Thanks > > > > > > I don't know, pass some kind of flag in struct virtqueue? > > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > > > ? > > > > > > > So if it's slow, sleep, otherwise poll? > > > > I feel setting this flag might be tricky, since the driver doesn't > > know whether or not it's really slow. E.g smartNIC vendor may allow > > virtio-net emulation over PCI. > > > > Thanks > > driver will have the choice, depending on whether > vq is deterministic or not. Ok, but the problem is, such booleans are only useful for virtio ring codes. But in this case, virtio-net knows what to do for cvq. So I'm not sure who the user is. Thanks > > > > > -- > > > MST > > > >
On Mon, May 15, 2023 at 01:13:33PM +0800, Jason Wang wrote: > On Mon, May 15, 2023 at 12:45 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > > > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > > > Forget to cc netdev, adding. > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > > > > > and there is no way for driver to find out when > > > > > > > > it actually took effect > > > > > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > > > survive from this for years. > > > > > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > > > without breaking normal drivers. > > > > > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > > > transport). > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > > > details were hidden from virtio-net. > > > > > > > > > > Or do you have any ideas on this? > > > > > > > > > > Thanks > > > > > > > > I don't know, pass some kind of flag in struct virtqueue? > > > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > > > > > ? > > > > > > > > > > So if it's slow, sleep, otherwise poll? > > > > > > I feel setting this flag might be tricky, since the driver doesn't > > > know whether or not it's really slow. E.g smartNIC vendor may allow > > > virtio-net emulation over PCI. > > > > > > Thanks > > > > driver will have the choice, depending on whether > > vq is deterministic or not. > > Ok, but the problem is, such booleans are only useful for virtio ring > codes. But in this case, virtio-net knows what to do for cvq. So I'm > not sure who the user is. > > Thanks Circling back, what exactly does the architecture you are trying to fix look like? Who is going to introduce unbounded latency? The hypervisor? If so do we not maybe want a new feature bit that documents this? Hypervisor then can detect old guests that spin and decide what to do, e.g. prioritise cvq more, or fail FEATURES_OK. > > > > > > > > -- > > > > MST > > > > > >
On Mon, May 15, 2023 at 6:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Mon, May 15, 2023 at 01:13:33PM +0800, Jason Wang wrote: > > On Mon, May 15, 2023 at 12:45 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > > > > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > > > > Forget to cc netdev, adding. > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > > > > > > > and there is no way for driver to find out when > > > > > > > > > it actually took effect > > > > > > > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > > > > survive from this for years. > > > > > > > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > > > > without breaking normal drivers. > > > > > > > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > > > > transport). > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > > > > details were hidden from virtio-net. > > > > > > > > > > > > Or do you have any ideas on this? > > > > > > > > > > > > Thanks > > > > > > > > > > I don't know, pass some kind of flag in struct virtqueue? > > > > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > > > > > > > ? > > > > > > > > > > > > > So if it's slow, sleep, otherwise poll? > > > > > > > > I feel setting this flag might be tricky, since the driver doesn't > > > > know whether or not it's really slow. E.g smartNIC vendor may allow > > > > virtio-net emulation over PCI. > > > > > > > > Thanks > > > > > > driver will have the choice, depending on whether > > > vq is deterministic or not. > > > > Ok, but the problem is, such booleans are only useful for virtio ring > > codes. But in this case, virtio-net knows what to do for cvq. So I'm > > not sure who the user is. > > > > Thanks > > Circling back, what exactly does the architecture you are trying > to fix look like? Who is going to introduce unbounded latency? > The hypervisor? Hypervisor is one of the possible reason, we have many more: Hardware device that provides virtio-pci emulation. Userspace devices like VDUSE. > If so do we not maybe want a new feature bit > that documents this? Hypervisor then can detect old guests > that spin and decide what to do, e.g. prioritise cvq more, > or fail FEATURES_OK. We suffer from this for bare metal as well. But a question is what's wrong with the approach that is used in this patch? I've answered that set_rx_mode is not reliable, so it should be fine to use workqueue. Except for this, any other thing that worries you? Thanks > > > > > > > > > > > > -- > > > > > MST > > > > > > > > >
On Tue, May 16, 2023 at 10:44:45AM +0800, Jason Wang wrote: > On Mon, May 15, 2023 at 6:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Mon, May 15, 2023 at 01:13:33PM +0800, Jason Wang wrote: > > > On Mon, May 15, 2023 at 12:45 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > > > > > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > > > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > > > > > Forget to cc netdev, adding. > > > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > > > > > > > > > and there is no way for driver to find out when > > > > > > > > > > it actually took effect > > > > > > > > > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > > > > > survive from this for years. > > > > > > > > > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > > > > > without breaking normal drivers. > > > > > > > > > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > > > > > transport). > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > > > > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > > > > > details were hidden from virtio-net. > > > > > > > > > > > > > > Or do you have any ideas on this? > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > I don't know, pass some kind of flag in struct virtqueue? > > > > > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > So if it's slow, sleep, otherwise poll? > > > > > > > > > > I feel setting this flag might be tricky, since the driver doesn't > > > > > know whether or not it's really slow. E.g smartNIC vendor may allow > > > > > virtio-net emulation over PCI. > > > > > > > > > > Thanks > > > > > > > > driver will have the choice, depending on whether > > > > vq is deterministic or not. > > > > > > Ok, but the problem is, such booleans are only useful for virtio ring > > > codes. But in this case, virtio-net knows what to do for cvq. So I'm > > > not sure who the user is. > > > > > > Thanks > > > > Circling back, what exactly does the architecture you are trying > > to fix look like? Who is going to introduce unbounded latency? > > The hypervisor? > > Hypervisor is one of the possible reason, we have many more: > > Hardware device that provides virtio-pci emulation. > Userspace devices like VDUSE. So let's start by addressing VDUSE maybe? > > If so do we not maybe want a new feature bit > > that documents this? Hypervisor then can detect old guests > > that spin and decide what to do, e.g. prioritise cvq more, > > or fail FEATURES_OK. > > We suffer from this for bare metal as well. > > But a question is what's wrong with the approach that is used in this > patch? I've answered that set_rx_mode is not reliable, so it should be > fine to use workqueue. Except for this, any other thing that worries > you? > > Thanks It's not reliable for other drivers but has been reliable for virtio. I worry some software relied on this. You are making good points though ... could we get some maintainer's feedback on this? > > > > > > > > > > > > > > > > -- > > > > > > MST > > > > > > > > > > > >
On Tue, May 16, 2023 at 12:13 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > On Tue, May 16, 2023 at 10:44:45AM +0800, Jason Wang wrote: > > On Mon, May 15, 2023 at 6:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > On Mon, May 15, 2023 at 01:13:33PM +0800, Jason Wang wrote: > > > > On Mon, May 15, 2023 at 12:45 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > > > > > > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > > > > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > > > > > > Forget to cc netdev, adding. > > > > > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > > > > > > > > > > > and there is no way for driver to find out when > > > > > > > > > > > it actually took effect > > > > > > > > > > > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > > > > > > survive from this for years. > > > > > > > > > > > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > > > > > > without breaking normal drivers. > > > > > > > > > > > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > > > > > > transport). > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > > > > > > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > > > > > > details were hidden from virtio-net. > > > > > > > > > > > > > > > > Or do you have any ideas on this? > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > I don't know, pass some kind of flag in struct virtqueue? > > > > > > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > So if it's slow, sleep, otherwise poll? > > > > > > > > > > > > I feel setting this flag might be tricky, since the driver doesn't > > > > > > know whether or not it's really slow. E.g smartNIC vendor may allow > > > > > > virtio-net emulation over PCI. > > > > > > > > > > > > Thanks > > > > > > > > > > driver will have the choice, depending on whether > > > > > vq is deterministic or not. > > > > > > > > Ok, but the problem is, such booleans are only useful for virtio ring > > > > codes. But in this case, virtio-net knows what to do for cvq. So I'm > > > > not sure who the user is. > > > > > > > > Thanks > > > > > > Circling back, what exactly does the architecture you are trying > > > to fix look like? Who is going to introduce unbounded latency? > > > The hypervisor? > > > > Hypervisor is one of the possible reason, we have many more: > > > > Hardware device that provides virtio-pci emulation. > > Userspace devices like VDUSE. > > So let's start by addressing VDUSE maybe? It's reported by at least one hardware vendor as well. I remember it was Alvaro who reported this first in the past. > > > > If so do we not maybe want a new feature bit > > > that documents this? Hypervisor then can detect old guests > > > that spin and decide what to do, e.g. prioritise cvq more, > > > or fail FEATURES_OK. > > > > We suffer from this for bare metal as well. > > > > But a question is what's wrong with the approach that is used in this > > patch? I've answered that set_rx_mode is not reliable, so it should be > > fine to use workqueue. Except for this, any other thing that worries > > you? > > > > Thanks > > It's not reliable for other drivers but has been reliable for virtio. > I worry some software relied on this. It's probably fine since some device like vhost doesn't support this at all and we manage to survive for several years. > You are making good points though ... could we get some > maintainer's feedback on this? That would be helpful. Jakub, any input on this? Thanks > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > MST > > > > > > > > > > > > > > > >
On Tue, May 16, 2023 at 12:17:50PM +0800, Jason Wang wrote: > On Tue, May 16, 2023 at 12:13 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > On Tue, May 16, 2023 at 10:44:45AM +0800, Jason Wang wrote: > > > On Mon, May 15, 2023 at 6:17 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > On Mon, May 15, 2023 at 01:13:33PM +0800, Jason Wang wrote: > > > > > On Mon, May 15, 2023 at 12:45 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > On Mon, May 15, 2023 at 09:05:54AM +0800, Jason Wang wrote: > > > > > > > On Wed, May 10, 2023 at 1:33 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > On Mon, Apr 17, 2023 at 11:40:58AM +0800, Jason Wang wrote: > > > > > > > > > On Fri, Apr 14, 2023 at 3:21 PM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 01:04:15PM +0800, Jason Wang wrote: > > > > > > > > > > > Forget to cc netdev, adding. > > > > > > > > > > > > > > > > > > > > > > On Fri, Apr 14, 2023 at 12:25 AM Michael S. Tsirkin <mst@redhat.com> wrote: > > > > > > > > > > > > > > > > > > > > > > > > On Thu, Apr 13, 2023 at 02:40:26PM +0800, Jason Wang wrote: > > > > > > > > > > > > > This patch convert rx mode setting to be done in a workqueue, this is > > > > > > > > > > > > > a must for allow to sleep when waiting for the cvq command to > > > > > > > > > > > > > response since current code is executed under addr spin lock. > > > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Jason Wang <jasowang@redhat.com> > > > > > > > > > > > > > > > > > > > > > > > > I don't like this frankly. This means that setting RX mode which would > > > > > > > > > > > > previously be reliable, now becomes unreliable. > > > > > > > > > > > > > > > > > > > > > > It is "unreliable" by design: > > > > > > > > > > > > > > > > > > > > > > void (*ndo_set_rx_mode)(struct net_device *dev); > > > > > > > > > > > > > > > > > > > > > > > - first of all configuration is no longer immediate > > > > > > > > > > > > > > > > > > > > > > Is immediate a hard requirement? I can see a workqueue is used at least: > > > > > > > > > > > > > > > > > > > > > > mlx5e, ipoib, efx, ... > > > > > > > > > > > > > > > > > > > > > > > and there is no way for driver to find out when > > > > > > > > > > > > it actually took effect > > > > > > > > > > > > > > > > > > > > > > But we know rx mode is best effort e.g it doesn't support vhost and we > > > > > > > > > > > survive from this for years. > > > > > > > > > > > > > > > > > > > > > > > - second, if device fails command, this is also not > > > > > > > > > > > > propagated to driver, again no way for driver to find out > > > > > > > > > > > > > > > > > > > > > > > > VDUSE needs to be fixed to do tricks to fix this > > > > > > > > > > > > without breaking normal drivers. > > > > > > > > > > > > > > > > > > > > > > It's not specific to VDUSE. For example, when using virtio-net in the > > > > > > > > > > > UP environment with any software cvq (like mlx5 via vDPA or cma > > > > > > > > > > > transport). > > > > > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > > > > > Hmm. Can we differentiate between these use-cases? > > > > > > > > > > > > > > > > > > It doesn't look easy since we are drivers for virtio bus. Underlayer > > > > > > > > > details were hidden from virtio-net. > > > > > > > > > > > > > > > > > > Or do you have any ideas on this? > > > > > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > > > > > I don't know, pass some kind of flag in struct virtqueue? > > > > > > > > "bool slow; /* This vq can be very slow sometimes. Don't wait for it! */" > > > > > > > > > > > > > > > > ? > > > > > > > > > > > > > > > > > > > > > > So if it's slow, sleep, otherwise poll? > > > > > > > > > > > > > > I feel setting this flag might be tricky, since the driver doesn't > > > > > > > know whether or not it's really slow. E.g smartNIC vendor may allow > > > > > > > virtio-net emulation over PCI. > > > > > > > > > > > > > > Thanks > > > > > > > > > > > > driver will have the choice, depending on whether > > > > > > vq is deterministic or not. > > > > > > > > > > Ok, but the problem is, such booleans are only useful for virtio ring > > > > > codes. But in this case, virtio-net knows what to do for cvq. So I'm > > > > > not sure who the user is. > > > > > > > > > > Thanks > > > > > > > > Circling back, what exactly does the architecture you are trying > > > > to fix look like? Who is going to introduce unbounded latency? > > > > The hypervisor? > > > > > > Hypervisor is one of the possible reason, we have many more: > > > > > > Hardware device that provides virtio-pci emulation. > > > Userspace devices like VDUSE. > > > > So let's start by addressing VDUSE maybe? > > It's reported by at least one hardware vendor as well. I remember it > was Alvaro who reported this first in the past. > > > > > > > If so do we not maybe want a new feature bit > > > > that documents this? Hypervisor then can detect old guests > > > > that spin and decide what to do, e.g. prioritise cvq more, > > > > or fail FEATURES_OK. > > > > > > We suffer from this for bare metal as well. > > > > > > But a question is what's wrong with the approach that is used in this > > > patch? I've answered that set_rx_mode is not reliable, so it should be > > > fine to use workqueue. Except for this, any other thing that worries > > > you? > > > > > > Thanks > > > > It's not reliable for other drivers but has been reliable for virtio. > > I worry some software relied on this. > > It's probably fine since some device like vhost doesn't support this > at all and we manage to survive for several years. vhost is often connected to a clever learning backend such as a bridge which will DTRT without guest configuring anything at all though, this could be why it works. > > You are making good points though ... could we get some > > maintainer's feedback on this? > > That would be helpful. Jakub, any input on this? > > Thanks > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > MST > > > > > > > > > > > > > > > > > > > >
On Tue, 16 May 2023 12:17:50 +0800 Jason Wang wrote: > > It's not reliable for other drivers but has been reliable for virtio. > > I worry some software relied on this. > > It's probably fine since some device like vhost doesn't support this > at all and we manage to survive for several years. > > > You are making good points though ... could we get some > > maintainer's feedback on this? > > That would be helpful. Jakub, any input on this? AFAIU the question is whether .ndo_set_rx_mode needs to be reliable and instantaneous? I haven't heard any complaints for it not being immediate, and most 10G+ NICs do the config via a workqueue. I even have an "intern task" to implement a workqueue in the core, for this to save the boilerplate code in the drivers.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c index e2560b6f7980..2e56bbf86894 100644 --- a/drivers/net/virtio_net.c +++ b/drivers/net/virtio_net.c @@ -265,6 +265,12 @@ struct virtnet_info { /* Work struct for config space updates */ struct work_struct config_work; + /* Work struct for config rx mode */ + struct work_struct rx_mode_work; + + /* Is rx mode work enabled? */ + bool rx_mode_work_enabled; + /* Does the affinity hint is set for virtqueues? */ bool affinity_hint_set; @@ -388,6 +394,20 @@ static void disable_delayed_refill(struct virtnet_info *vi) spin_unlock_bh(&vi->refill_lock); } +static void enable_rx_mode_work(struct virtnet_info *vi) +{ + rtnl_lock(); + vi->rx_mode_work_enabled = true; + rtnl_unlock(); +} + +static void disable_rx_mode_work(struct virtnet_info *vi) +{ + rtnl_lock(); + vi->rx_mode_work_enabled = false; + rtnl_unlock(); +} + static void virtqueue_napi_schedule(struct napi_struct *napi, struct virtqueue *vq) { @@ -2310,9 +2330,11 @@ static int virtnet_close(struct net_device *dev) return 0; } -static void virtnet_set_rx_mode(struct net_device *dev) +static void virtnet_rx_mode_work(struct work_struct *work) { - struct virtnet_info *vi = netdev_priv(dev); + struct virtnet_info *vi = + container_of(work, struct virtnet_info, rx_mode_work); + struct net_device *dev = vi->dev; struct scatterlist sg[2]; struct virtio_net_ctrl_mac *mac_data; struct netdev_hw_addr *ha; @@ -2325,6 +2347,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_RX)) return; + rtnl_lock(); + vi->ctrl->promisc = ((dev->flags & IFF_PROMISC) != 0); vi->ctrl->allmulti = ((dev->flags & IFF_ALLMULTI) != 0); @@ -2342,14 +2366,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) dev_warn(&dev->dev, "Failed to %sable allmulti mode.\n", vi->ctrl->allmulti ? "en" : "dis"); + netif_addr_lock_bh(dev); + uc_count = netdev_uc_count(dev); mc_count = netdev_mc_count(dev); /* MAC filter - use one buffer for both lists */ buf = kzalloc(((uc_count + mc_count) * ETH_ALEN) + (2 * sizeof(mac_data->entries)), GFP_ATOMIC); mac_data = buf; - if (!buf) + if (!buf) { + netif_addr_unlock_bh(dev); + rtnl_unlock(); return; + } sg_init_table(sg, 2); @@ -2370,6 +2399,8 @@ static void virtnet_set_rx_mode(struct net_device *dev) netdev_for_each_mc_addr(ha, dev) memcpy(&mac_data->macs[i++][0], ha->addr, ETH_ALEN); + netif_addr_unlock_bh(dev); + sg_set_buf(&sg[1], mac_data, sizeof(mac_data->entries) + (mc_count * ETH_ALEN)); @@ -2377,9 +2408,19 @@ static void virtnet_set_rx_mode(struct net_device *dev) VIRTIO_NET_CTRL_MAC_TABLE_SET, sg)) dev_warn(&dev->dev, "Failed to set MAC filter table.\n"); + rtnl_unlock(); + kfree(buf); } +static void virtnet_set_rx_mode(struct net_device *dev) +{ + struct virtnet_info *vi = netdev_priv(dev); + + if (vi->rx_mode_work_enabled) + schedule_work(&vi->rx_mode_work); +} + static int virtnet_vlan_rx_add_vid(struct net_device *dev, __be16 proto, u16 vid) { @@ -3150,6 +3191,8 @@ static void virtnet_freeze_down(struct virtio_device *vdev) /* Make sure no work handler is accessing the device */ flush_work(&vi->config_work); + disable_rx_mode_work(vi); + flush_work(&vi->rx_mode_work); netif_tx_lock_bh(vi->dev); netif_device_detach(vi->dev); @@ -3172,6 +3215,7 @@ static int virtnet_restore_up(struct virtio_device *vdev) virtio_device_ready(vdev); enable_delayed_refill(vi); + enable_rx_mode_work(vi); if (netif_running(vi->dev)) { err = virtnet_open(vi->dev); @@ -3969,6 +4013,7 @@ static int virtnet_probe(struct virtio_device *vdev) vdev->priv = vi; INIT_WORK(&vi->config_work, virtnet_config_changed_work); + INIT_WORK(&vi->rx_mode_work, virtnet_rx_mode_work); spin_lock_init(&vi->refill_lock); if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF)) { @@ -4077,6 +4122,8 @@ static int virtnet_probe(struct virtio_device *vdev) if (vi->has_rss || vi->has_rss_hash_report) virtnet_init_default_rss(vi); + enable_rx_mode_work(vi); + /* serialize netdev register + virtio_device_ready() with ndo_open() */ rtnl_lock(); @@ -4174,6 +4221,8 @@ static void virtnet_remove(struct virtio_device *vdev) /* Make sure no work handler is accessing the device. */ flush_work(&vi->config_work); + disable_rx_mode_work(vi); + flush_work(&vi->rx_mode_work); unregister_netdev(vi->dev);