[PATCH v2] net: virtio_net: notifications coalescing support

[PATCH v2] net: virtio_net: notifications coalescing support

[Alvaro Karsz]

New VirtIO network feature: VIRTIO_NET_F_NOTF_COAL.

Control a Virtio network device notifications coalescing parameters
using the control virtqueue.

A device that supports this fetature can receive
VIRTIO_NET_CTRL_NOTF_COAL control commands.

- VIRTIO_NET_CTRL_NOTF_COAL_TX_SET:
  Ask the network device to change the following parameters:
  - tx_usecs: Maximum number of usecs to delay a TX notification.
  - tx_max_packets: Maximum number of packets to send before a
    TX notification.

- VIRTIO_NET_CTRL_NOTF_COAL_RX_SET:
  Ask the network device to change the following parameters:
  - rx_usecs: Maximum number of usecs to delay a RX notification.
  - rx_max_packets: Maximum number of packets to receive before a
    RX notification.

VirtIO spec. patch:
https://lists.oasis-open.org/archives/virtio-comment/202206/msg00100.html

Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
---
v2:
	- Fix type assignments warnings found with sparse.
	- Fix a few typos.
---
 drivers/net/virtio_net.c        | 110 ++++++++++++++++++++++++++++----
 include/uapi/linux/virtio_net.h |  34 +++++++++-
 2 files changed, 130 insertions(+), 14 deletions(-)

diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 356cf8dd416..7837db0306f 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -261,6 +261,12 @@ struct virtnet_info {
 	u8 duplex;
 	u32 speed;
 
+	/* Interrupt coalescing settings */
+	u32 tx_usecs;
+	u32 rx_usecs;
+	u32 tx_max_packets;
+	u32 rx_max_packets;
+
 	unsigned long guest_offloads;
 	unsigned long guest_offloads_capable;
 
@@ -2594,19 +2600,76 @@ static int virtnet_set_coalesce(struct net_device *dev,
 {
 	struct virtnet_info *vi = netdev_priv(dev);
 	int i, napi_weight;
+	struct scatterlist sgs_tx, sgs_rx;
+	struct virtio_net_ctrl_coal_tx coal_tx;
+	struct virtio_net_ctrl_coal_rx coal_rx;
+	bool update_napi,
+	notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
+
+	/* rx_coalesce_usecs/tx_coalesce_usecs are supported only
+	 * if VIRTIO_NET_F_NOTF_COAL feature is negotiated.
+	 */
+	if (!notf_coal && (ec->rx_coalesce_usecs || ec->tx_coalesce_usecs))
+		return -EOPNOTSUPP;
+
+	if (notf_coal) {
+		coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
+		coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
+		sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
+
+		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
+					  VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
+					  &sgs_tx))
+			return -EINVAL;
+
+		/* Save parameters */
+		vi->tx_usecs = ec->tx_coalesce_usecs;
+		vi->tx_max_packets = ec->tx_max_coalesced_frames;
+
+		coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
+		coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
+		sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
+
+		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
+					  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
+					  &sgs_rx))
+			return -EINVAL;
+
+		/* Save parameters */
+		vi->rx_usecs = ec->rx_coalesce_usecs;
+		vi->rx_max_packets = ec->rx_max_coalesced_frames;
+	}
+
+	/* Should we update NAPI? */
+	update_napi = ec->tx_max_coalesced_frames <= 1 &&
+			ec->rx_max_coalesced_frames == 1;
 
-	if (ec->tx_max_coalesced_frames > 1 ||
-	    ec->rx_max_coalesced_frames != 1)
+	/* If notifications coalesing feature is not negotiated,
+	 * and we can't update NAPI, return an error
+	 */
+	if (!notf_coal && !update_napi)
 		return -EINVAL;
 
-	napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
-	if (napi_weight ^ vi->sq[0].napi.weight) {
-		if (dev->flags & IFF_UP)
-			return -EBUSY;
-		for (i = 0; i < vi->max_queue_pairs; i++)
-			vi->sq[i].napi.weight = napi_weight;
+	if (update_napi) {
+		napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
+		if (napi_weight ^ vi->sq[0].napi.weight) {
+			if (dev->flags & IFF_UP) {
+				/* If notifications coalescing feature is not negotiated,
+				 * return an error, otherwise exit without changing
+				 * the NAPI parameters.
+				 */
+				if (!notf_coal)
+					return -EBUSY;
+
+				goto exit;
+			}
+
+			for (i = 0; i < vi->max_queue_pairs; i++)
+				vi->sq[i].napi.weight = napi_weight;
+		}
 	}
 
+exit:
 	return 0;
 }
 
@@ -2616,14 +2679,25 @@ static int virtnet_get_coalesce(struct net_device *dev,
 				struct netlink_ext_ack *extack)
 {
 	struct ethtool_coalesce ec_default = {
-		.cmd = ETHTOOL_GCOALESCE,
-		.rx_max_coalesced_frames = 1,
+		.cmd = ETHTOOL_GCOALESCE
 	};
+
 	struct virtnet_info *vi = netdev_priv(dev);
+	bool notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
+
+	/* Add notifications coalescing settings */
+	if (notf_coal) {
+		ec_default.rx_coalesce_usecs = vi->rx_usecs;
+		ec_default.tx_coalesce_usecs = vi->tx_usecs;
+		ec_default.tx_max_coalesced_frames = vi->tx_max_packets;
+		ec_default.rx_max_coalesced_frames = vi->rx_max_packets;
+	} else {
+		ec_default.rx_max_coalesced_frames = 1;
+	}
 
 	memcpy(ec, &ec_default, sizeof(ec_default));
 
-	if (vi->sq[0].napi.weight)
+	if (!notf_coal && vi->sq[0].napi.weight)
 		ec->tx_max_coalesced_frames = 1;
 
 	return 0;
@@ -2743,7 +2817,8 @@ static int virtnet_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info)
 }
 
 static const struct ethtool_ops virtnet_ethtool_ops = {
-	.supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES,
+	.supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES |
+		ETHTOOL_COALESCE_USECS,
 	.get_drvinfo = virtnet_get_drvinfo,
 	.get_link = ethtool_op_get_link,
 	.get_ringparam = virtnet_get_ringparam,
@@ -3411,6 +3486,8 @@ static bool virtnet_validate_features(struct virtio_device *vdev)
 	     VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_RSS,
 			     "VIRTIO_NET_F_CTRL_VQ") ||
 	     VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_HASH_REPORT,
+			     "VIRTIO_NET_F_CTRL_VQ") ||
+	     VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_NOTF_COAL,
 			     "VIRTIO_NET_F_CTRL_VQ"))) {
 		return false;
 	}
@@ -3546,6 +3623,13 @@ static int virtnet_probe(struct virtio_device *vdev)
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
 		vi->mergeable_rx_bufs = true;
 
+	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
+		vi->rx_usecs = 0;
+		vi->tx_usecs = 0;
+		vi->tx_max_packets = 0;
+		vi->rx_max_packets = 0;
+	}
+
 	if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
 		vi->has_rss_hash_report = true;
 
@@ -3780,7 +3864,7 @@ static struct virtio_device_id id_table[] = {
 	VIRTIO_NET_F_CTRL_MAC_ADDR, \
 	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
 	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
-	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT
+	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
 
 static unsigned int features[] = {
 	VIRTNET_FEATURES,
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index 3f55a4215f1..29ced55514d 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -56,7 +56,7 @@
 #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
 					 * Steering */
 #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
-
+#define VIRTIO_NET_F_NOTF_COAL	53	/* Guest can handle notifications coalescing */
 #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
 #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
 #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
@@ -355,4 +355,36 @@ struct virtio_net_hash_config {
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS   5
 #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0
 
+/*
+ * Control notifications coalescing.
+ *
+ * Request the device to change the notifications coalescing parameters.
+ *
+ * Available with the VIRTIO_NET_F_NOTF_COAL feature bit.
+ */
+#define VIRTIO_NET_CTRL_NOTF_COAL		6
+/*
+ * Set the tx-usecs/tx-max-packets patameters.
+ * tx-usecs - Maximum number of usecs to delay a TX notification.
+ * tx-max-packets - Maximum number of packets to send before a TX notification.
+ */
+struct virtio_net_ctrl_coal_tx {
+	__le32 tx_max_packets;
+	__le32 tx_usecs;
+};
+
+#define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET		0
+
+/*
+ * Set the rx-usecs/rx-max-packets patameters.
+ * rx-usecs - Maximum number of usecs to delay a RX notification.
+ * rx-max-frames - Maximum number of packets to receive before a RX notification.
+ */
+struct virtio_net_ctrl_coal_rx {
+	__le32 rx_max_packets;
+	__le32 rx_usecs;
+};
+
+#define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET		1
+
 #endif /* _UAPI_LINUX_VIRTIO_NET_H */
-- 
2.32.0

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Jakub Kicinski]

On Tue, 12 Jul 2022 14:22:09 +0300 Alvaro Karsz wrote:
> @@ -2594,19 +2600,76 @@ static int virtnet_set_coalesce(struct net_device *dev,
>  {
>  	struct virtnet_info *vi = netdev_priv(dev);
>  	int i, napi_weight;
> +	struct scatterlist sgs_tx, sgs_rx;
> +	struct virtio_net_ctrl_coal_tx coal_tx;
> +	struct virtio_net_ctrl_coal_rx coal_rx;
> +	bool update_napi,
> +	notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
> +
> +	/* rx_coalesce_usecs/tx_coalesce_usecs are supported only
> +	 * if VIRTIO_NET_F_NOTF_COAL feature is negotiated.
> +	 */
> +	if (!notf_coal && (ec->rx_coalesce_usecs || ec->tx_coalesce_usecs))
> +		return -EOPNOTSUPP;
> +
> +	if (notf_coal) {
> +		coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> +		coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> +		sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
> +
> +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> +					  VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
> +					  &sgs_tx))
> +			return -EINVAL;
> +
> +		/* Save parameters */
> +		vi->tx_usecs = ec->tx_coalesce_usecs;
> +		vi->tx_max_packets = ec->tx_max_coalesced_frames;
> +
> +		coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> +		coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> +		sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
> +
> +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> +					  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> +					  &sgs_rx))
> +			return -EINVAL;
> +
> +		/* Save parameters */
> +		vi->rx_usecs = ec->rx_coalesce_usecs;
> +		vi->rx_max_packets = ec->rx_max_coalesced_frames;
> +	}
> +
> +	/* Should we update NAPI? */
> +	update_napi = ec->tx_max_coalesced_frames <= 1 &&
> +			ec->rx_max_coalesced_frames == 1;

I'd vote for either interpreting the parameters in the new way (real
coalescing) or old way (NAPI update) but not both.

> -	if (ec->tx_max_coalesced_frames > 1 ||
> -	    ec->rx_max_coalesced_frames != 1)
> +	/* If notifications coalesing feature is not negotiated,
> +	 * and we can't update NAPI, return an error
> +	 */
> +	if (!notf_coal && !update_napi)
>  		return -EINVAL;
>  
> -	napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> -	if (napi_weight ^ vi->sq[0].napi.weight) {
> -		if (dev->flags & IFF_UP)
> -			return -EBUSY;
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> -			vi->sq[i].napi.weight = napi_weight;
> +	if (update_napi) {
> +		napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> +		if (napi_weight ^ vi->sq[0].napi.weight) {
> +			if (dev->flags & IFF_UP) {
> +				/* If notifications coalescing feature is not negotiated,
> +				 * return an error, otherwise exit without changing
> +				 * the NAPI parameters.
> +				 */
> +				if (!notf_coal)
> +					return -EBUSY;
> +
> +				goto exit;
> +			}
> +
> +			for (i = 0; i < vi->max_queue_pairs; i++)
> +				vi->sq[i].napi.weight = napi_weight;
> +		}
>  	}
>  
> +exit:
>  	return 0;
>  }

This function got long now, and large parts of it are under if()s
Feels like you'd be better of factoring out the USEC update and the
existing logic into separate functions and calling them as needed.

> @@ -2616,14 +2679,25 @@ static int virtnet_get_coalesce(struct net_device *dev,
>  				struct netlink_ext_ack *extack)
>  {
>  	struct ethtool_coalesce ec_default = {
> -		.cmd = ETHTOOL_GCOALESCE,
> -		.rx_max_coalesced_frames = 1,
> +		.cmd = ETHTOOL_GCOALESCE
>  	};

I think the structure was here for conciseness, since you're not
populating it now just remove it and write to ec directly. 
ec->cmd does not have to be written it's already set.

> +
>  	struct virtnet_info *vi = netdev_priv(dev);
> +	bool notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
> +
> +	/* Add notifications coalescing settings */
> +	if (notf_coal) {
> +		ec_default.rx_coalesce_usecs = vi->rx_usecs;
> +		ec_default.tx_coalesce_usecs = vi->tx_usecs;
> +		ec_default.tx_max_coalesced_frames = vi->tx_max_packets;
> +		ec_default.rx_max_coalesced_frames = vi->rx_max_packets;
> +	} else {
> +		ec_default.rx_max_coalesced_frames = 1;
> +	}


> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 3f55a4215f1..29ced55514d 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h

Is it typical for virtio to add the structures to uAPI even tho the
kernel does not consume them? I presume so, just wanted to flag it.

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Jason Wang]

在 2022/7/12 19:22, Alvaro Karsz 写道:
> New VirtIO network feature: VIRTIO_NET_F_NOTF_COAL.
>
> Control a Virtio network device notifications coalescing parameters
> using the control virtqueue.
>
> A device that supports this fetature can receive
> VIRTIO_NET_CTRL_NOTF_COAL control commands.
>
> - VIRTIO_NET_CTRL_NOTF_COAL_TX_SET:
>    Ask the network device to change the following parameters:
>    - tx_usecs: Maximum number of usecs to delay a TX notification.
>    - tx_max_packets: Maximum number of packets to send before a
>      TX notification.
>
> - VIRTIO_NET_CTRL_NOTF_COAL_RX_SET:
>    Ask the network device to change the following parameters:
>    - rx_usecs: Maximum number of usecs to delay a RX notification.
>    - rx_max_packets: Maximum number of packets to receive before a
>      RX notification.
>
> VirtIO spec. patch:
> https://lists.oasis-open.org/archives/virtio-comment/202206/msg00100.html


Note that until this is merged, the patch should be tagged as RFC.

And it looks to me we should target this as net-next instead of net.


>
> Signed-off-by: Alvaro Karsz <alvaro.karsz@solid-run.com>
> ---
> v2:
> 	- Fix type assignments warnings found with sparse.
> 	- Fix a few typos.
> ---
>   drivers/net/virtio_net.c        | 110 ++++++++++++++++++++++++++++----
>   include/uapi/linux/virtio_net.h |  34 +++++++++-
>   2 files changed, 130 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 356cf8dd416..7837db0306f 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -261,6 +261,12 @@ struct virtnet_info {
>   	u8 duplex;
>   	u32 speed;
>   
> +	/* Interrupt coalescing settings */
> +	u32 tx_usecs;
> +	u32 rx_usecs;
> +	u32 tx_max_packets;
> +	u32 rx_max_packets;
> +
>   	unsigned long guest_offloads;
>   	unsigned long guest_offloads_capable;
>   
> @@ -2594,19 +2600,76 @@ static int virtnet_set_coalesce(struct net_device *dev,
>   {
>   	struct virtnet_info *vi = netdev_priv(dev);
>   	int i, napi_weight;
> +	struct scatterlist sgs_tx, sgs_rx;
> +	struct virtio_net_ctrl_coal_tx coal_tx;
> +	struct virtio_net_ctrl_coal_rx coal_rx;
> +	bool update_napi,
> +	notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
> +
> +	/* rx_coalesce_usecs/tx_coalesce_usecs are supported only
> +	 * if VIRTIO_NET_F_NOTF_COAL feature is negotiated.
> +	 */
> +	if (!notf_coal && (ec->rx_coalesce_usecs || ec->tx_coalesce_usecs))
> +		return -EOPNOTSUPP;
> +
> +	if (notf_coal) {
> +		coal_tx.tx_usecs = cpu_to_le32(ec->tx_coalesce_usecs);
> +		coal_tx.tx_max_packets = cpu_to_le32(ec->tx_max_coalesced_frames);
> +		sg_init_one(&sgs_tx, &coal_tx, sizeof(coal_tx));
> +
> +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> +					  VIRTIO_NET_CTRL_NOTF_COAL_TX_SET,
> +					  &sgs_tx))
> +			return -EINVAL;
> +
> +		/* Save parameters */
> +		vi->tx_usecs = ec->tx_coalesce_usecs;
> +		vi->tx_max_packets = ec->tx_max_coalesced_frames;
> +
> +		coal_rx.rx_usecs = cpu_to_le32(ec->rx_coalesce_usecs);
> +		coal_rx.rx_max_packets = cpu_to_le32(ec->rx_max_coalesced_frames);
> +		sg_init_one(&sgs_rx, &coal_rx, sizeof(coal_rx));
> +
> +		if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_NOTF_COAL,
> +					  VIRTIO_NET_CTRL_NOTF_COAL_RX_SET,
> +					  &sgs_rx))
> +			return -EINVAL;
> +
> +		/* Save parameters */
> +		vi->rx_usecs = ec->rx_coalesce_usecs;
> +		vi->rx_max_packets = ec->rx_max_coalesced_frames;
> +	}
> +
> +	/* Should we update NAPI? */
> +	update_napi = ec->tx_max_coalesced_frames <= 1 &&
> +			ec->rx_max_coalesced_frames == 1;
>   
> -	if (ec->tx_max_coalesced_frames > 1 ||
> -	    ec->rx_max_coalesced_frames != 1)
> +	/* If notifications coalesing feature is not negotiated,
> +	 * and we can't update NAPI, return an error
> +	 */
> +	if (!notf_coal && !update_napi)
>   		return -EINVAL;
>   
> -	napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> -	if (napi_weight ^ vi->sq[0].napi.weight) {
> -		if (dev->flags & IFF_UP)
> -			return -EBUSY;
> -		for (i = 0; i < vi->max_queue_pairs; i++)
> -			vi->sq[i].napi.weight = napi_weight;
> +	if (update_napi) {
> +		napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> +		if (napi_weight ^ vi->sq[0].napi.weight) {
> +			if (dev->flags & IFF_UP) {
> +				/* If notifications coalescing feature is not negotiated,
> +				 * return an error, otherwise exit without changing
> +				 * the NAPI parameters.
> +				 */
> +				if (!notf_coal)
> +					return -EBUSY;
> +
> +				goto exit;


I think we need to return -EBUSY here regardless whether or not 
interrupt coalescing is negotiated.

This is because the driver can work in two modes:

1) tx interrupt mode (tx_max_coalesced_frames = 1)
2) no tx interrupt mode (will do skb_orphan) (tx_max_coalesced_frames = 0)

Mode switching is more easier to be done when the interface is down.

When the coalescing is neogiated, we can stick the above logic:

1) tx interrupt mode (tx_max_coalesced_frames >= 1)
2) no tx interrupt mode (will do skb_orphan) (tx_max_coalesced_frames = 0)

So if no mode switching (no switching between zero and non zero value of 
tx_max_coalesced_frames), we don't need an update of napi_weight. But if 
it requires a switch, it still needs to be done when the interface is 
down (technically it's not a must but it should be another topic/patch).


> +			}
> +
> +			for (i = 0; i < vi->max_queue_pairs; i++)
> +				vi->sq[i].napi.weight = napi_weight;
> +		}
>   	}
>   
> +exit:
>   	return 0;
>   }
>   
> @@ -2616,14 +2679,25 @@ static int virtnet_get_coalesce(struct net_device *dev,
>   				struct netlink_ext_ack *extack)
>   {
>   	struct ethtool_coalesce ec_default = {
> -		.cmd = ETHTOOL_GCOALESCE,
> -		.rx_max_coalesced_frames = 1,


Nit: we can stick to this then we can remove the following "else".

Thanks


> +		.cmd = ETHTOOL_GCOALESCE
>   	};
> +
>   	struct virtnet_info *vi = netdev_priv(dev);
> +	bool notf_coal = virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL);
> +
> +	/* Add notifications coalescing settings */
> +	if (notf_coal) {
> +		ec_default.rx_coalesce_usecs = vi->rx_usecs;
> +		ec_default.tx_coalesce_usecs = vi->tx_usecs;
> +		ec_default.tx_max_coalesced_frames = vi->tx_max_packets;
> +		ec_default.rx_max_coalesced_frames = vi->rx_max_packets;
> +	} else {
> +		ec_default.rx_max_coalesced_frames = 1;
> +	}
>   
>   	memcpy(ec, &ec_default, sizeof(ec_default));
>   
> -	if (vi->sq[0].napi.weight)
> +	if (!notf_coal && vi->sq[0].napi.weight)
>   		ec->tx_max_coalesced_frames = 1;
>   
>   	return 0;
> @@ -2743,7 +2817,8 @@ static int virtnet_set_rxnfc(struct net_device *dev, struct ethtool_rxnfc *info)
>   }
>   
>   static const struct ethtool_ops virtnet_ethtool_ops = {
> -	.supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES,
> +	.supported_coalesce_params = ETHTOOL_COALESCE_MAX_FRAMES |
> +		ETHTOOL_COALESCE_USECS,
>   	.get_drvinfo = virtnet_get_drvinfo,
>   	.get_link = ethtool_op_get_link,
>   	.get_ringparam = virtnet_get_ringparam,
> @@ -3411,6 +3486,8 @@ static bool virtnet_validate_features(struct virtio_device *vdev)
>   	     VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_RSS,
>   			     "VIRTIO_NET_F_CTRL_VQ") ||
>   	     VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_HASH_REPORT,
> +			     "VIRTIO_NET_F_CTRL_VQ") ||
> +	     VIRTNET_FAIL_ON(vdev, VIRTIO_NET_F_NOTF_COAL,
>   			     "VIRTIO_NET_F_CTRL_VQ"))) {
>   		return false;
>   	}
> @@ -3546,6 +3623,13 @@ static int virtnet_probe(struct virtio_device *vdev)
>   	if (virtio_has_feature(vdev, VIRTIO_NET_F_MRG_RXBUF))
>   		vi->mergeable_rx_bufs = true;
>   
> +	if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_NOTF_COAL)) {
> +		vi->rx_usecs = 0;
> +		vi->tx_usecs = 0;
> +		vi->tx_max_packets = 0;
> +		vi->rx_max_packets = 0;
> +	}
> +
>   	if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
>   		vi->has_rss_hash_report = true;
>   
> @@ -3780,7 +3864,7 @@ static struct virtio_device_id id_table[] = {
>   	VIRTIO_NET_F_CTRL_MAC_ADDR, \
>   	VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS, \
>   	VIRTIO_NET_F_SPEED_DUPLEX, VIRTIO_NET_F_STANDBY, \
> -	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT
> +	VIRTIO_NET_F_RSS, VIRTIO_NET_F_HASH_REPORT, VIRTIO_NET_F_NOTF_COAL
>   
>   static unsigned int features[] = {
>   	VIRTNET_FEATURES,
> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
> index 3f55a4215f1..29ced55514d 100644
> --- a/include/uapi/linux/virtio_net.h
> +++ b/include/uapi/linux/virtio_net.h
> @@ -56,7 +56,7 @@
>   #define VIRTIO_NET_F_MQ	22	/* Device supports Receive Flow
>   					 * Steering */
>   #define VIRTIO_NET_F_CTRL_MAC_ADDR 23	/* Set MAC address */
> -
> +#define VIRTIO_NET_F_NOTF_COAL	53	/* Guest can handle notifications coalescing */
>   #define VIRTIO_NET_F_HASH_REPORT  57	/* Supports hash report */
>   #define VIRTIO_NET_F_RSS	  60	/* Supports RSS RX steering */
>   #define VIRTIO_NET_F_RSC_EXT	  61	/* extended coalescing info */
> @@ -355,4 +355,36 @@ struct virtio_net_hash_config {
>   #define VIRTIO_NET_CTRL_GUEST_OFFLOADS   5
>   #define VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET        0
>   
> +/*
> + * Control notifications coalescing.
> + *
> + * Request the device to change the notifications coalescing parameters.
> + *
> + * Available with the VIRTIO_NET_F_NOTF_COAL feature bit.
> + */
> +#define VIRTIO_NET_CTRL_NOTF_COAL		6
> +/*
> + * Set the tx-usecs/tx-max-packets patameters.
> + * tx-usecs - Maximum number of usecs to delay a TX notification.
> + * tx-max-packets - Maximum number of packets to send before a TX notification.
> + */
> +struct virtio_net_ctrl_coal_tx {
> +	__le32 tx_max_packets;
> +	__le32 tx_usecs;
> +};
> +
> +#define VIRTIO_NET_CTRL_NOTF_COAL_TX_SET		0
> +
> +/*
> + * Set the rx-usecs/rx-max-packets patameters.
> + * rx-usecs - Maximum number of usecs to delay a RX notification.
> + * rx-max-frames - Maximum number of packets to receive before a RX notification.
> + */
> +struct virtio_net_ctrl_coal_rx {
> +	__le32 rx_max_packets;
> +	__le32 rx_usecs;
> +};
> +
> +#define VIRTIO_NET_CTRL_NOTF_COAL_RX_SET		1
> +
>   #endif /* _UAPI_LINUX_VIRTIO_NET_H */

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Jason Wang]

在 2022/7/14 11:02, Jakub Kicinski 写道:
>> diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
>> index 3f55a4215f1..29ced55514d 100644
>> --- a/include/uapi/linux/virtio_net.h
>> +++ b/include/uapi/linux/virtio_net.h
> Is it typical for virtio to add the structures to uAPI even tho the
> kernel does not consume them? I presume so, just wanted to flag it.
>

Yes, Qemu will sync and use Linux uapi headers like this.

Thanks

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Alvaro Karsz]

Thanks Jakub and Jason.

> I think we need to return -EBUSY here regardless whether or not
> interrupt coalescing is negotiated.


The part you are referring to is relevant only if we are going to update NAPI.
Jakub suggested splitting the function into 2 cases.

If interrupt coalescing is negotiated:
 Send control commands to the device.
Otherwise:
 Update NAPI.

So this is not relevant if interrupt coalescing is negotiated.
You don't think that we should separate the function into 2 different cases?


Or maybe I misunderstood you, and you are not referring to the following part:
> +                             if (!notf_coal)
> +                                     return -EBUSY;
> +
> +                             goto exit;

But you are referring to the whole virtnet_set_coalesce function in general.


Alvaro.

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Jason Wang]

On Thu, Jul 14, 2022 at 2:54 PM Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:
>
> Thanks Jakub and Jason.
>
> > I think we need to return -EBUSY here regardless whether or not
> > interrupt coalescing is negotiated.
>
>
> The part you are referring to is relevant only if we are going to update NAPI.
> Jakub suggested splitting the function into 2 cases.
>
> If interrupt coalescing is negotiated:
>  Send control commands to the device.
> Otherwise:
>  Update NAPI.
>
> So this is not relevant if interrupt coalescing is negotiated.
> You don't think that we should separate the function into 2 different cases?

So we use sq->napi.weight as a hint to use tx interrupt or not.

We need a safe switching from tx interrupt and skb_orphan(). The
current code guarantees this by only allowing the switching when the
interface is down.

So what I meant for the above "Update NAPI" is, consider that users
want to switch from tx_max_coalesced_frames from 0 to 100. This needs
to be down when the interface is down, since the driver need to enable
tx interrupt mode, otherwise the coalescing is meaningless.

This would be much easier if we only have tx interrupt mode, but this
requires more work.

>
>
> Or maybe I misunderstood you, and you are not referring to the following part:
> > +                             if (!notf_coal)
> > +                                     return -EBUSY;
> > +
> > +                             goto exit;
>
> But you are referring to the whole virtnet_set_coalesce function in general.

See above.

Thanks

>
>
> Alvaro.
>

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Alvaro Karsz]

> So we use sq->napi.weight as a hint to use tx interrupt or not.
> We need a safe switching from tx interrupt and skb_orphan(). The
> current code guarantees this by only allowing the switching when the
> interface is down.
> So what I meant for the above "Update NAPI" is, consider that users
> want to switch from tx_max_coalesced_frames from 0 to 100. This needs
> to be down when the interface is down, since the driver need to enable
> tx interrupt mode, otherwise the coalescing is meaningless.
> This would be much easier if we only have tx interrupt mode, but this
> requires more work.


So, If I understood correctly, you're suggesting to add the following
part to the
"interrupt coalescing is negotiated" case:

napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
if (napi_weight ^ vi->sq[0].napi.weight) {
   if (dev->flags & IFF_UP)
        return -EBUSY;
    for (i = 0; i < vi->max_queue_pairs; i++)
        vi->sq[i].napi.weight = napi_weight;
}

Before sending the control commands to the device.
Is this right?

Re: [PATCH v2] net: virtio_net: notifications coalescing support

[Jason Wang]

On Thu, Jul 14, 2022 at 3:23 PM Alvaro Karsz <alvaro.karsz@solid-run.com> wrote:
>
> > So we use sq->napi.weight as a hint to use tx interrupt or not.
> > We need a safe switching from tx interrupt and skb_orphan(). The
> > current code guarantees this by only allowing the switching when the
> > interface is down.
> > So what I meant for the above "Update NAPI" is, consider that users
> > want to switch from tx_max_coalesced_frames from 0 to 100. This needs
> > to be down when the interface is down, since the driver need to enable
> > tx interrupt mode, otherwise the coalescing is meaningless.
> > This would be much easier if we only have tx interrupt mode, but this
> > requires more work.
>
>
> So, If I understood correctly, you're suggesting to add the following
> part to the
> "interrupt coalescing is negotiated" case:
>
> napi_weight = ec->tx_max_coalesced_frames ? NAPI_POLL_WEIGHT : 0;
> if (napi_weight ^ vi->sq[0].napi.weight) {
>    if (dev->flags & IFF_UP)
>         return -EBUSY;
>     for (i = 0; i < vi->max_queue_pairs; i++)
>         vi->sq[i].napi.weight = napi_weight;
> }
>
> Before sending the control commands to the device.
> Is this right?

I think it should be after the control commands were sent. Other looks fine.

Thanks

>