Re: [PATCH net-next] virtio-net: synchronize operstate with admin state on up/down
On Mon, May 20, 2024 at 09:03:02AM +0800, Jason Wang wrote:
> This patch synchronize operstate with admin state per RFC2863.
>
> This is done by trying to toggle the carrier upon open/close and
> synchronize with the config change work. This allows propagate status
> correctly to stacked devices like:
>
> ip link add link enp0s3 macvlan0 type macvlan
> ip link set link enp0s3 down
> ip link show
>
> Before this patch:
>
> 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode
> DEFAULT group default qlen 1000
> link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> ..
> 5: macvlan0@enp0s3: mtu 1500 qdisc
> noqueue state UP mode DEFAULT group default qlen 1000
> link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
>
> After this patch:
>
> 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode
> DEFAULT group default qlen 1000
> link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff
> ...
> 5: macvlan0@enp0s3: mtu 1500 qdisc
> noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000
> link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff
>
> Cc: Venkat Venkatsubra
> Cc: Gia-Khanh Nguyen
> Signed-off-by: Jason Wang
Acked-by: Michael S. Tsirkin
> ---
> drivers/net/virtio_net.c | 94 +++-
> 1 file changed, 63 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 4e1a0fc0d555..24d880a5023d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -433,6 +433,12 @@ struct virtnet_info {
> /* The lock to synchronize the access to refill_enabled */
> spinlock_t refill_lock;
>
> + /* Is config change enabled? */
> + bool config_change_enabled;
> +
> + /* The lock to synchronize the access to config_change_enabled */
> + spinlock_t config_change_lock;
> +
> /* Work struct for config space updates */
> struct work_struct config_work;
>
> @@ -623,6 +629,20 @@ static void disable_delayed_refill(struct virtnet_info
> *vi)
> spin_unlock_bh(&vi->refill_lock);
> }
>
> +static void enable_config_change(struct virtnet_info *vi)
> +{
> + spin_lock_irq(&vi->config_change_lock);
> + vi->config_change_enabled = true;
> + spin_unlock_irq(&vi->config_change_lock);
> +}
> +
> +static void disable_config_change(struct virtnet_info *vi)
> +{
> + spin_lock_irq(&vi->config_change_lock);
> + vi->config_change_enabled = false;
> + spin_unlock_irq(&vi->config_change_lock);
> +}
> +
> static void enable_rx_mode_work(struct virtnet_info *vi)
> {
> rtnl_lock();
> @@ -2421,6 +2441,25 @@ static int virtnet_enable_queue_pair(struct
> virtnet_info *vi, int qp_index)
> return err;
> }
>
> +static void virtnet_update_settings(struct virtnet_info *vi)
> +{
> + u32 speed;
> + u8 duplex;
> +
> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_SPEED_DUPLEX))
> + return;
> +
> + virtio_cread_le(vi->vdev, struct virtio_net_config, speed, &speed);
> +
> + if (ethtool_validate_speed(speed))
> + vi->speed = speed;
> +
> + virtio_cread_le(vi->vdev, struct virtio_net_config, duplex, &duplex);
> +
> + if (ethtool_validate_duplex(duplex))
> + vi->duplex = duplex;
> +}
> +
> static int virtnet_open(struct net_device *dev)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> @@ -2439,6 +2478,18 @@ static int virtnet_open(struct net_device *dev)
> goto err_enable_qp;
> }
>
> + /* Assume link up if device can't report link status,
> +otherwise get link status from config. */
> + netif_carrier_off(dev);
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_STATUS)) {
> + enable_config_change(vi);
> + schedule_work(&vi->config_work);
> + } else {
> + vi->status = VIRTIO_NET_S_LINK_UP;
> + virtnet_update_settings(vi);
> + netif_carrier_on(dev);
> + }
> +
> return 0;
>
> err_enable_qp:
> @@ -2875,12 +2926,19 @@ static int virtnet_close(struct net_device *dev)
> disable_delayed_refill(vi);
> /* Make sure refill_work doesn't re-enable napi! */
> cancel_delayed_work_sync(&vi->refill);
> + /* Make sure config notification doesn't schedule config work */
> + disable_config_change(vi);
> + /* Make sure status updating is cancelled */
> + cancel_work_sync(&vi->config_work);
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> virtnet_disable_queue_pair(vi, i);
> cancel_work_sync(&vi->rq[i].dim.work);
> }
>
> + vi->status &= ~VIRTIO_NET_S_LINK_UP;
> + netif_carrier_off(dev);
> +
> return 0;
> }
>
> @@ -4583,25 +4641,6 @@ static void virtnet_init_settings(struct net_device
> *dev)
> vi->duplex = DUPLEX_UNKNOWN;
> }
>
> -static void virtnet_update_settings(struct virtnet_info *vi)
> -{
> - u32 speed;
> - u8 duplex;
> -
> - if (!virtio_has
Re: [PATCH net-next] virtio-net: synchronize operstate with admin state on up/down
On Mon, 2024-05-20 at 09:03 +0800, Jason Wang wrote: > This patch synchronize operstate with admin state per RFC2863. > > This is done by trying to toggle the carrier upon open/close and > synchronize with the config change work. This allows propagate status > correctly to stacked devices like: > > ip link add link enp0s3 macvlan0 type macvlan > ip link set link enp0s3 down > ip link show > > Before this patch: > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode > DEFAULT group default qlen 1000 > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff > .. > 5: macvlan0@enp0s3: mtu 1500 qdisc > noqueue state UP mode DEFAULT group default qlen 1000 > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff > > After this patch: > > 3: enp0s3: mtu 1500 qdisc pfifo_fast state DOWN mode > DEFAULT group default qlen 1000 > link/ether 00:00:05:00:00:09 brd ff:ff:ff:ff:ff:ff > ... > 5: macvlan0@enp0s3: mtu 1500 qdisc > noqueue state LOWERLAYERDOWN mode DEFAULT group default qlen 1000 > link/ether b2:a9:c5:04:da:53 brd ff:ff:ff:ff:ff:ff > > Cc: Venkat Venkatsubra > Cc: Gia-Khanh Nguyen > Signed-off-by: Jason Wang ## Form letter - net-next-closed The merge window for v6.10 has begun and we have already posted our pull request. Therefore net-next is closed for new drivers, features, code refactoring and optimizations. We are currently accepting bug fixes only. Please repost when net-next reopens after May 26th. RFC patches sent for review only are obviously welcome at any time. See: https://www.kernel.org/doc/html/next/process/maintainer-netdev.html#development-cycle -- pw-bot: defer
[PATCH net 2/2] Revert "virtio_net: Add a lock for per queue RX coalesce"
This reverts commit 4d4ac2ececd3c42a08dd32a6e3a4aaf25f7efe44.
When the following snippet is run, lockdep will complain[1].
/* Acquire all queues dim_locks */
for (i = 0; i < vi->max_queue_pairs; i++)
mutex_lock(&vi->rq[i].dim_lock);
At the same time, too many queues will cause lockdep to be more irritable,
which can be alleviated by using mutex_lock_nested(), however, there are
still new warning when the number of queues exceeds MAX_LOCKDEP_SUBCLASSES.
So I would like to gently revert this commit, although it brings
unsynchronization that is not so concerned:
1. When dim is enabled, rx_dim_work may modify the coalescing parameters.
Users may read dirty coalescing parameters if querying.
2. When dim is switched from enabled to disabled, a spurious dim worker
maybe scheduled, but this can be handled correctly by rx_dim_work.
[1]
WARNING: possible recursive locking detected
6.9.0-rc7+ #319 Not tainted
ethtool/962 is trying to acquire lock:
but task is already holding lock:
other info that might help us debug this:
Possible unsafe locking scenario:
CPU0
lock(&vi->rq[i].dim_lock);
lock(&vi->rq[i].dim_lock);
*** DEADLOCK ***
May be due to missing lock nesting notation
3 locks held by ethtool/962:
#0: 82dbaab0 (cb_lock){}-{3:3}, at: genl_rcv+0x19/0x40
#1: 82dad0a8 (rtnl_mutex){+.+.}-{3:3}, at:
ethnl_default_set_doit+0xbe/0x1e0
stack backtrace:
CPU: 6 PID: 962 Comm: ethtool Not tainted 6.9.0-rc7+ #319
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS
rel-1.16.0-0-gd239552ce722-prebuilt.qemu.org 04/01/2014
Call Trace:
dump_stack_lvl+0x79/0xb0
check_deadlock+0x130/0x220
__lock_acquire+0x861/0x990
lock_acquire.part.0+0x72/0x1d0
? lock_acquire+0xf8/0x130
__mutex_lock+0x71/0xd50
virtnet_set_coalesce+0x151/0x190
__ethnl_set_coalesce.isra.0+0x3f8/0x4d0
ethnl_set_coalesce+0x34/0x90
ethnl_default_set_doit+0xdd/0x1e0
genl_family_rcv_msg_doit+0xdc/0x130
genl_family_rcv_msg+0x154/0x230
? __pfx_ethnl_default_set_doit+0x10/0x10
genl_rcv_msg+0x4b/0xa0
? __pfx_genl_rcv_msg+0x10/0x10
netlink_rcv_skb+0x5a/0x110
genl_rcv+0x28/0x40
netlink_unicast+0x1af/0x280
netlink_sendmsg+0x20e/0x460
__sys_sendto+0x1fe/0x210
? find_held_lock+0x2b/0x80
? do_user_addr_fault+0x3a2/0x8a0
? __lock_release+0x5e/0x160
? do_user_addr_fault+0x3a2/0x8a0
? lock_release+0x72/0x140
? do_user_addr_fault+0x3a7/0x8a0
__x64_sys_sendto+0x29/0x30
do_syscall_64+0x78/0x180
entry_SYSCALL_64_after_hwframe+0x76/0x7e
Signed-off-by: Heng Qi
---
drivers/net/virtio_net.c | 53 +---
1 file changed, 12 insertions(+), 41 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1cad06cef230..e4a1dff2a64a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -316,9 +316,6 @@ struct receive_queue {
/* Is dynamic interrupt moderation enabled? */
bool dim_enabled;
- /* Used to protect dim_enabled and inter_coal */
- struct mutex dim_lock;
-
/* Dynamic Interrupt Moderation */
struct dim dim;
@@ -2368,10 +2365,6 @@ static int virtnet_poll(struct napi_struct *napi, int
budget)
/* Out of packets? */
if (received < budget) {
napi_complete = virtqueue_napi_complete(napi, rq->vq, received);
- /* Intentionally not taking dim_lock here. This may result in a
-* spurious net_dim call. But if that happens
virtnet_rx_dim_work
-* will not act on the scheduled work.
-*/
if (napi_complete && rq->dim_enabled)
virtnet_rx_dim_update(vi, rq);
}
@@ -3247,11 +3240,9 @@ static int virtnet_set_ringparam(struct net_device *dev,
return err;
/* The reason is same as the transmit virtqueue reset */
- mutex_lock(&vi->rq[i].dim_lock);
err = virtnet_send_rx_ctrl_coal_vq_cmd(vi, i,
vi->intr_coal_rx.max_usecs,
vi->intr_coal_rx.max_packets);
- mutex_unlock(&vi->rq[i].dim_lock);
if (err)
return err;
}
@@ -4257,7 +4248,6 @@ static int virtnet_send_rx_notf_coal_cmds(struct
virtnet_info *vi,
struct virtio_net_ctrl_coal_rx *coal_rx __free(kfree) = NULL;
bool rx_ctrl_dim_on = !!ec->use_adaptive_rx_coalesce;
struct scatterlist sgs_rx;
- int ret = 0;
int i;
if (rx_ctrl_dim_on && !virtio_has_feature(vi->vdev,
VIRTIO_NET_F_VQ_NOTF_COAL))
@@ -4267,22 +4257,16 @@ static int virtnet_send_rx_notf_coal_cmds(struct
virtnet_info
[PATCH net 0/2] virtio_net: fix lock warning and unrecoverable state
Patch 1 describes and fixes an issue where dim cannot return to normal state in certain scenarios. Patch 2 attempts to resolve lockdep's complaints that holding many nested locks and when there is a maximum number of queues may also be problematic, so try to remove the dim_lock. Heng Qi (2): virtio_net: fix possible dim status unrecoverable Revert "virtio_net: Add a lock for per queue RX coalesce" drivers/net/virtio_net.c | 55 ++-- 1 file changed, 13 insertions(+), 42 deletions(-) -- 2.32.0.3.g01195cf9f
[PATCH net 1/2] virtio_net: fix possible dim status unrecoverable
When the dim worker is scheduled, if it no longer needs to issue
commands, dim may not be able to return to the working state later.
For example, the following single queue scenario:
1. The dim worker of rxq0 is scheduled, and the dim status is
changed to DIM_APPLY_NEW_PROFILE;
2. dim is disabled or parameters have not been modified;
3. virtnet_rx_dim_work exits directly;
Then, even if net_dim is invoked again, it cannot work because the
state is not restored to DIM_START_MEASURE.
Fixes: 6208799553a8 ("virtio-net: support rx netdim")
Signed-off-by: Heng Qi
---
drivers/net/virtio_net.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4e1a0fc0d555..1cad06cef230 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4417,9 +4417,9 @@ static void virtnet_rx_dim_work(struct work_struct *work)
if (err)
pr_debug("%s: Failed to send dim parameters on rxq%d\n",
dev->name, qnum);
- dim->state = DIM_START_MEASURE;
}
out:
+ dim->state = DIM_START_MEASURE;
mutex_unlock(&rq->dim_lock);
}
--
2.32.0.3.g01195cf9f
