* [PATCH net-next 0/4] virtio_net: Fixes and improvements
@ 2025-03-18 9:56 Akihiko Odaki
2025-03-18 9:56 ` [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config Akihiko Odaki
` (5 more replies)
0 siblings, 6 replies; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-18 9:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Melnychenko, Joe Damato, Philo Lu
Cc: virtualization, linux-kernel, netdev, devel, Akihiko Odaki
Jason Wang recently proposed an improvement to struct
virtio_net_rss_config:
https://lore.kernel.org/r/CACGkMEud0Ki8p=z299Q7b4qEDONpYDzbVqhHxCNVk_vo-KdP9A@mail.gmail.com
This patch series implements it and also fixes a few minor bugs I found
when writing patches.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
Akihiko Odaki (4):
virtio_net: Split struct virtio_net_rss_config
virtio_net: Fix endian with virtio_net_ctrl_rss
virtio_net: Use new RSS config structs
virtio_net: Allocate rss_hdr with devres
drivers/net/virtio_net.c | 119 +++++++++++++++-------------------------
include/uapi/linux/virtio_net.h | 13 +++++
2 files changed, 56 insertions(+), 76 deletions(-)
---
base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
change-id: 20250318-virtio-6559d69187db
Best regards,
--
Akihiko Odaki <akihiko.odaki@daynix.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
@ 2025-03-18 9:56 ` Akihiko Odaki
2025-03-19 1:42 ` Jason Wang
2025-03-18 9:56 ` [PATCH net-next 2/4] virtio_net: Fix endian with virtio_net_ctrl_rss Akihiko Odaki
` (4 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-18 9:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Melnychenko, Joe Damato, Philo Lu
Cc: virtualization, linux-kernel, netdev, devel, Akihiko Odaki
struct virtio_net_rss_config was less useful in actual code because of a
flexible array placed in the middle. Add new structures that split it
into two to avoid having a flexible array in the middle.
Suggested-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
include/uapi/linux/virtio_net.h | 13 +++++++++++++
1 file changed, 13 insertions(+)
diff --git a/include/uapi/linux/virtio_net.h b/include/uapi/linux/virtio_net.h
index ac9174717ef1..963540deae66 100644
--- a/include/uapi/linux/virtio_net.h
+++ b/include/uapi/linux/virtio_net.h
@@ -327,6 +327,19 @@ struct virtio_net_rss_config {
__u8 hash_key_data[/* hash_key_length */];
};
+struct virtio_net_rss_config_hdr {
+ __le32 hash_types;
+ __le16 indirection_table_mask;
+ __le16 unclassified_queue;
+ __le16 indirection_table[/* 1 + indirection_table_mask */];
+};
+
+struct virtio_net_rss_config_trailer {
+ __le16 max_tx_vq;
+ __u8 hash_key_length;
+ __u8 hash_key_data[/* hash_key_length */];
+};
+
#define VIRTIO_NET_CTRL_MQ_RSS_CONFIG 1
/*
--
2.48.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 2/4] virtio_net: Fix endian with virtio_net_ctrl_rss
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
2025-03-18 9:56 ` [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config Akihiko Odaki
@ 2025-03-18 9:56 ` Akihiko Odaki
2025-03-19 1:43 ` Jason Wang
2025-03-18 9:56 ` [PATCH net-next 3/4] virtio_net: Use new RSS config structs Akihiko Odaki
` (3 subsequent siblings)
5 siblings, 1 reply; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-18 9:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Melnychenko, Joe Damato, Philo Lu
Cc: virtualization, linux-kernel, netdev, devel, Akihiko Odaki
Mark the fields of struct virtio_net_ctrl_rss as little endian as
they are in struct virtio_net_rss_config, which it follows.
Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
drivers/net/virtio_net.c | 30 +++++++++++++++---------------
1 file changed, 15 insertions(+), 15 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 7646ddd9bef7..d1ed544ba03a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -368,15 +368,15 @@ struct receive_queue {
*/
#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
struct virtio_net_ctrl_rss {
- u32 hash_types;
- u16 indirection_table_mask;
- u16 unclassified_queue;
- u16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
- u16 max_tx_vq;
+ __le32 hash_types;
+ __le16 indirection_table_mask;
+ __le16 unclassified_queue;
+ __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
+ __le16 max_tx_vq;
u8 hash_key_length;
u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
- u16 *indirection_table;
+ __le16 *indirection_table;
};
/* Control VQ buffers: protected by the rtnl lock */
@@ -3576,9 +3576,9 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
for (; i < vi->rss_indir_table_size; ++i) {
indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
- vi->rss.indirection_table[i] = indir_val;
+ vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
}
- vi->rss.max_tx_vq = queue_pairs;
+ vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
}
static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
@@ -4097,10 +4097,10 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
static void virtnet_init_default_rss(struct virtnet_info *vi)
{
- vi->rss.hash_types = vi->rss_hash_types_supported;
+ vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_supported);
vi->rss_hash_types_saved = vi->rss_hash_types_supported;
vi->rss.indirection_table_mask = vi->rss_indir_table_size
- ? vi->rss_indir_table_size - 1 : 0;
+ ? cpu_to_le16(vi->rss_indir_table_size - 1) : 0;
vi->rss.unclassified_queue = 0;
virtnet_rss_update_by_qpairs(vi, vi->curr_queue_pairs);
@@ -4218,7 +4218,7 @@ static bool virtnet_set_hashflow(struct virtnet_info *vi, struct ethtool_rxnfc *
if (new_hashtypes != vi->rss_hash_types_saved) {
vi->rss_hash_types_saved = new_hashtypes;
- vi->rss.hash_types = vi->rss_hash_types_saved;
+ vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
if (vi->dev->features & NETIF_F_RXHASH)
return virtnet_commit_rss_command(vi);
}
@@ -5398,7 +5398,7 @@ static int virtnet_get_rxfh(struct net_device *dev,
if (rxfh->indir) {
for (i = 0; i < vi->rss_indir_table_size; ++i)
- rxfh->indir[i] = vi->rss.indirection_table[i];
+ rxfh->indir[i] = le16_to_cpu(vi->rss.indirection_table[i]);
}
if (rxfh->key)
@@ -5426,7 +5426,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
return -EOPNOTSUPP;
for (i = 0; i < vi->rss_indir_table_size; ++i)
- vi->rss.indirection_table[i] = rxfh->indir[i];
+ vi->rss.indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
update = true;
}
@@ -6044,9 +6044,9 @@ static int virtnet_set_features(struct net_device *dev,
if ((dev->features ^ features) & NETIF_F_RXHASH) {
if (features & NETIF_F_RXHASH)
- vi->rss.hash_types = vi->rss_hash_types_saved;
+ vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
else
- vi->rss.hash_types = VIRTIO_NET_HASH_REPORT_NONE;
+ vi->rss.hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
if (!virtnet_commit_rss_command(vi))
return -EINVAL;
--
2.48.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
2025-03-18 9:56 ` [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config Akihiko Odaki
2025-03-18 9:56 ` [PATCH net-next 2/4] virtio_net: Fix endian with virtio_net_ctrl_rss Akihiko Odaki
@ 2025-03-18 9:56 ` Akihiko Odaki
2025-03-19 1:22 ` Xuan Zhuo
2025-03-19 1:43 ` Jason Wang
2025-03-18 9:56 ` [PATCH net-next 4/4] virtio_net: Allocate rss_hdr with devres Akihiko Odaki
` (2 subsequent siblings)
5 siblings, 2 replies; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-18 9:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Melnychenko, Joe Damato, Philo Lu
Cc: virtualization, linux-kernel, netdev, devel, Akihiko Odaki
The new RSS configuration structures allow easily constructing data for
VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
for the command.
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
1 file changed, 43 insertions(+), 74 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index d1ed544ba03a..4153a0a5f278 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -360,24 +360,7 @@ struct receive_queue {
struct xdp_buff **xsk_buffs;
};
-/* This structure can contain rss message with maximum settings for indirection table and keysize
- * Note, that default structure that describes RSS configuration virtio_net_rss_config
- * contains same info but can't handle table values.
- * In any case, structure would be passed to virtio hw through sg_buf split by parts
- * because table sizes may be differ according to the device configuration.
- */
#define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
-struct virtio_net_ctrl_rss {
- __le32 hash_types;
- __le16 indirection_table_mask;
- __le16 unclassified_queue;
- __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
- __le16 max_tx_vq;
- u8 hash_key_length;
- u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
-
- __le16 *indirection_table;
-};
/* Control VQ buffers: protected by the rtnl lock */
struct control_buf {
@@ -421,7 +404,9 @@ struct virtnet_info {
u16 rss_indir_table_size;
u32 rss_hash_types_supported;
u32 rss_hash_types_saved;
- struct virtio_net_ctrl_rss rss;
+ struct virtio_net_rss_config_hdr *rss_hdr;
+ struct virtio_net_rss_config_trailer rss_trailer;
+ u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
/* Has control virtqueue */
bool has_cvq;
@@ -523,23 +508,16 @@ enum virtnet_xmit_type {
VIRTNET_XMIT_TYPE_XSK,
};
-static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
+static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
{
- if (!indir_table_size) {
- rss->indirection_table = NULL;
- return 0;
- }
+ u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
- rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
- if (!rss->indirection_table)
- return -ENOMEM;
-
- return 0;
+ return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
}
-static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
+static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
{
- kfree(rss->indirection_table);
+ return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
}
/* We use the last two bits of the pointer to distinguish the xmit type. */
@@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
for (; i < vi->rss_indir_table_size; ++i) {
indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
- vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
+ vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
}
- vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
+ vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
}
static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
{
struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
- struct virtio_net_ctrl_rss old_rss;
+ struct virtio_net_rss_config_hdr *old_rss_hdr;
+ struct virtio_net_rss_config_trailer old_rss_trailer;
struct net_device *dev = vi->dev;
struct scatterlist sg;
@@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
* update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
*/
if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
- memcpy(&old_rss, &vi->rss, sizeof(old_rss));
- if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
- vi->rss.indirection_table = old_rss.indirection_table;
+ old_rss_hdr = vi->rss_hdr;
+ old_rss_trailer = vi->rss_trailer;
+ vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
+ if (!vi->rss_hdr) {
+ vi->rss_hdr = old_rss_hdr;
return -ENOMEM;
}
+ *vi->rss_hdr = *old_rss_hdr;
virtnet_rss_update_by_qpairs(vi, queue_pairs);
if (!virtnet_commit_rss_command(vi)) {
/* restore ctrl_rss if commit_rss_command failed */
- rss_indirection_table_free(&vi->rss);
- memcpy(&vi->rss, &old_rss, sizeof(old_rss));
+ kfree(vi->rss_hdr);
+ vi->rss_hdr = old_rss_hdr;
+ vi->rss_trailer = old_rss_trailer;
dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
queue_pairs);
return -EINVAL;
}
- rss_indirection_table_free(&old_rss);
+ kfree(old_rss_hdr);
goto succ;
}
@@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
static bool virtnet_commit_rss_command(struct virtnet_info *vi)
{
struct net_device *dev = vi->dev;
- struct scatterlist sgs[4];
- unsigned int sg_buf_size;
+ struct scatterlist sgs[2];
/* prepare sgs */
- sg_init_table(sgs, 4);
-
- sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
- sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
-
- if (vi->has_rss) {
- sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
- sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
- } else {
- sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
- }
-
- sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
- - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
- sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
-
- sg_buf_size = vi->rss_key_size;
- sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
+ sg_init_table(sgs, 2);
+ sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
+ sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
@@ -4097,17 +4064,17 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
static void virtnet_init_default_rss(struct virtnet_info *vi)
{
- vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_supported);
+ vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_supported);
vi->rss_hash_types_saved = vi->rss_hash_types_supported;
- vi->rss.indirection_table_mask = vi->rss_indir_table_size
+ vi->rss_hdr->indirection_table_mask = vi->rss_indir_table_size
? cpu_to_le16(vi->rss_indir_table_size - 1) : 0;
- vi->rss.unclassified_queue = 0;
+ vi->rss_hdr->unclassified_queue = 0;
virtnet_rss_update_by_qpairs(vi, vi->curr_queue_pairs);
- vi->rss.hash_key_length = vi->rss_key_size;
+ vi->rss_trailer.hash_key_length = vi->rss_key_size;
- netdev_rss_key_fill(vi->rss.key, vi->rss_key_size);
+ netdev_rss_key_fill(vi->rss_hash_key_data, vi->rss_key_size);
}
static void virtnet_get_hashflow(const struct virtnet_info *vi, struct ethtool_rxnfc *info)
@@ -4218,7 +4185,7 @@ static bool virtnet_set_hashflow(struct virtnet_info *vi, struct ethtool_rxnfc *
if (new_hashtypes != vi->rss_hash_types_saved) {
vi->rss_hash_types_saved = new_hashtypes;
- vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
+ vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
if (vi->dev->features & NETIF_F_RXHASH)
return virtnet_commit_rss_command(vi);
}
@@ -5398,11 +5365,11 @@ static int virtnet_get_rxfh(struct net_device *dev,
if (rxfh->indir) {
for (i = 0; i < vi->rss_indir_table_size; ++i)
- rxfh->indir[i] = le16_to_cpu(vi->rss.indirection_table[i]);
+ rxfh->indir[i] = le16_to_cpu(vi->rss_hdr->indirection_table[i]);
}
if (rxfh->key)
- memcpy(rxfh->key, vi->rss.key, vi->rss_key_size);
+ memcpy(rxfh->key, vi->rss_hash_key_data, vi->rss_key_size);
rxfh->hfunc = ETH_RSS_HASH_TOP;
@@ -5426,7 +5393,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
return -EOPNOTSUPP;
for (i = 0; i < vi->rss_indir_table_size; ++i)
- vi->rss.indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
+ vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
update = true;
}
@@ -5438,7 +5405,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
if (!vi->has_rss && !vi->has_rss_hash_report)
return -EOPNOTSUPP;
- memcpy(vi->rss.key, rxfh->key, vi->rss_key_size);
+ memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
update = true;
}
@@ -6044,9 +6011,9 @@ static int virtnet_set_features(struct net_device *dev,
if ((dev->features ^ features) & NETIF_F_RXHASH) {
if (features & NETIF_F_RXHASH)
- vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
+ vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
else
- vi->rss.hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
+ vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
if (!virtnet_commit_rss_command(vi))
return -EINVAL;
@@ -6735,9 +6702,11 @@ static int virtnet_probe(struct virtio_device *vdev)
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
}
- err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size);
- if (err)
+ vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
+ if (!vi->rss_hdr) {
+ err = -ENOMEM;
goto free;
+ }
if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_key_size =
@@ -7016,7 +6985,7 @@ static void virtnet_remove(struct virtio_device *vdev)
remove_vq_common(vi);
- rss_indirection_table_free(&vi->rss);
+ kfree(vi->rss_hdr);
free_netdev(vi->dev);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH net-next 4/4] virtio_net: Allocate rss_hdr with devres
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
` (2 preceding siblings ...)
2025-03-18 9:56 ` [PATCH net-next 3/4] virtio_net: Use new RSS config structs Akihiko Odaki
@ 2025-03-18 9:56 ` Akihiko Odaki
2025-03-19 1:43 ` Jason Wang
2025-03-19 8:17 ` [PATCH net-next 0/4] virtio_net: Fixes and improvements Lei Yang
2025-03-19 9:07 ` Michael S. Tsirkin
5 siblings, 1 reply; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-18 9:56 UTC (permalink / raw)
To: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Melnychenko, Joe Damato, Philo Lu
Cc: virtualization, linux-kernel, netdev, devel, Akihiko Odaki
virtnet_probe() lacks the code to free rss_hdr in its error path.
Allocate rss_hdr with devres so that it will be automatically freed.
Fixes: 86a48a00efdf ("virtio_net: Support dynamic rss indirection table size")
Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
---
drivers/net/virtio_net.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 4153a0a5f278..6cbeba65a4a4 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3580,7 +3580,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
old_rss_hdr = vi->rss_hdr;
old_rss_trailer = vi->rss_trailer;
- vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
+ vi->rss_hdr = devm_kmalloc(&dev->dev, virtnet_rss_hdr_size(vi), GFP_KERNEL);
if (!vi->rss_hdr) {
vi->rss_hdr = old_rss_hdr;
return -ENOMEM;
@@ -3591,7 +3591,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
if (!virtnet_commit_rss_command(vi)) {
/* restore ctrl_rss if commit_rss_command failed */
- kfree(vi->rss_hdr);
+ devm_kfree(&dev->dev, vi->rss_hdr);
vi->rss_hdr = old_rss_hdr;
vi->rss_trailer = old_rss_trailer;
@@ -3599,7 +3599,7 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
queue_pairs);
return -EINVAL;
}
- kfree(old_rss_hdr);
+ devm_kfree(&dev->dev, old_rss_hdr);
goto succ;
}
@@ -6702,7 +6702,7 @@ static int virtnet_probe(struct virtio_device *vdev)
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
}
- vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
+ vi->rss_hdr = devm_kmalloc(&vdev->dev, virtnet_rss_hdr_size(vi), GFP_KERNEL);
if (!vi->rss_hdr) {
err = -ENOMEM;
goto free;
@@ -6985,8 +6985,6 @@ static void virtnet_remove(struct virtio_device *vdev)
remove_vq_common(vi);
- kfree(vi->rss_hdr);
-
free_netdev(vi->dev);
}
--
2.48.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-18 9:56 ` [PATCH net-next 3/4] virtio_net: Use new RSS config structs Akihiko Odaki
@ 2025-03-19 1:22 ` Xuan Zhuo
2025-03-19 4:46 ` Akihiko Odaki
2025-03-19 1:43 ` Jason Wang
1 sibling, 1 reply; 19+ messages in thread
From: Xuan Zhuo @ 2025-03-19 1:22 UTC (permalink / raw)
To: Akihiko Odaki
Cc: virtualization, linux-kernel, netdev, devel, Akihiko Odaki,
Michael S. Tsirkin, Jason Wang, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu
On Tue, 18 Mar 2025 18:56:53 +0900, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> The new RSS configuration structures allow easily constructing data for
> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
> for the command.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
> 1 file changed, 43 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d1ed544ba03a..4153a0a5f278 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -360,24 +360,7 @@ struct receive_queue {
> struct xdp_buff **xsk_buffs;
> };
>
> -/* This structure can contain rss message with maximum settings for indirection table and keysize
> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
> - * contains same info but can't handle table values.
> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
> - * because table sizes may be differ according to the device configuration.
> - */
> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
> -struct virtio_net_ctrl_rss {
> - __le32 hash_types;
> - __le16 indirection_table_mask;
> - __le16 unclassified_queue;
> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
> - __le16 max_tx_vq;
> - u8 hash_key_length;
> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> -
> - __le16 *indirection_table;
> -};
>
> /* Control VQ buffers: protected by the rtnl lock */
> struct control_buf {
> @@ -421,7 +404,9 @@ struct virtnet_info {
> u16 rss_indir_table_size;
> u32 rss_hash_types_supported;
> u32 rss_hash_types_saved;
> - struct virtio_net_ctrl_rss rss;
> + struct virtio_net_rss_config_hdr *rss_hdr;
> + struct virtio_net_rss_config_trailer rss_trailer;
> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
Why put rss_hash_key_data outside of virtio_net_rss_config_trailer?
Thanks.
>
> /* Has control virtqueue */
> bool has_cvq;
> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
> VIRTNET_XMIT_TYPE_XSK,
> };
>
> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
> {
> - if (!indir_table_size) {
> - rss->indirection_table = NULL;
> - return 0;
> - }
> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
>
> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
> - if (!rss->indirection_table)
> - return -ENOMEM;
> -
> - return 0;
> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
> }
>
> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
> {
> - kfree(rss->indirection_table);
> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
> }
>
> /* We use the last two bits of the pointer to distinguish the xmit type. */
> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
>
> for (; i < vi->rss_indir_table_size; ++i) {
> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
> }
> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
> }
>
> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> {
> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> - struct virtio_net_ctrl_rss old_rss;
> + struct virtio_net_rss_config_hdr *old_rss_hdr;
> + struct virtio_net_rss_config_trailer old_rss_trailer;
> struct net_device *dev = vi->dev;
> struct scatterlist sg;
>
> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> */
> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
> - vi->rss.indirection_table = old_rss.indirection_table;
> + old_rss_hdr = vi->rss_hdr;
> + old_rss_trailer = vi->rss_trailer;
> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
> + if (!vi->rss_hdr) {
> + vi->rss_hdr = old_rss_hdr;
> return -ENOMEM;
> }
>
> + *vi->rss_hdr = *old_rss_hdr;
> virtnet_rss_update_by_qpairs(vi, queue_pairs);
>
> if (!virtnet_commit_rss_command(vi)) {
> /* restore ctrl_rss if commit_rss_command failed */
> - rss_indirection_table_free(&vi->rss);
> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
> + kfree(vi->rss_hdr);
> + vi->rss_hdr = old_rss_hdr;
> + vi->rss_trailer = old_rss_trailer;
>
> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
> queue_pairs);
> return -EINVAL;
> }
> - rss_indirection_table_free(&old_rss);
> + kfree(old_rss_hdr);
> goto succ;
> }
>
> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> {
> struct net_device *dev = vi->dev;
> - struct scatterlist sgs[4];
> - unsigned int sg_buf_size;
> + struct scatterlist sgs[2];
>
> /* prepare sgs */
> - sg_init_table(sgs, 4);
> -
> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
> -
> - if (vi->has_rss) {
> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> - } else {
> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
> - }
> -
> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
> -
> - sg_buf_size = vi->rss_key_size;
> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
> + sg_init_table(sgs, 2);
> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
>
> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
> vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
> @@ -4097,17 +4064,17 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>
> static void virtnet_init_default_rss(struct virtnet_info *vi)
> {
> - vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_supported);
> + vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_supported);
> vi->rss_hash_types_saved = vi->rss_hash_types_supported;
> - vi->rss.indirection_table_mask = vi->rss_indir_table_size
> + vi->rss_hdr->indirection_table_mask = vi->rss_indir_table_size
> ? cpu_to_le16(vi->rss_indir_table_size - 1) : 0;
> - vi->rss.unclassified_queue = 0;
> + vi->rss_hdr->unclassified_queue = 0;
>
> virtnet_rss_update_by_qpairs(vi, vi->curr_queue_pairs);
>
> - vi->rss.hash_key_length = vi->rss_key_size;
> + vi->rss_trailer.hash_key_length = vi->rss_key_size;
>
> - netdev_rss_key_fill(vi->rss.key, vi->rss_key_size);
> + netdev_rss_key_fill(vi->rss_hash_key_data, vi->rss_key_size);
> }
>
> static void virtnet_get_hashflow(const struct virtnet_info *vi, struct ethtool_rxnfc *info)
> @@ -4218,7 +4185,7 @@ static bool virtnet_set_hashflow(struct virtnet_info *vi, struct ethtool_rxnfc *
>
> if (new_hashtypes != vi->rss_hash_types_saved) {
> vi->rss_hash_types_saved = new_hashtypes;
> - vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> + vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> if (vi->dev->features & NETIF_F_RXHASH)
> return virtnet_commit_rss_command(vi);
> }
> @@ -5398,11 +5365,11 @@ static int virtnet_get_rxfh(struct net_device *dev,
>
> if (rxfh->indir) {
> for (i = 0; i < vi->rss_indir_table_size; ++i)
> - rxfh->indir[i] = le16_to_cpu(vi->rss.indirection_table[i]);
> + rxfh->indir[i] = le16_to_cpu(vi->rss_hdr->indirection_table[i]);
> }
>
> if (rxfh->key)
> - memcpy(rxfh->key, vi->rss.key, vi->rss_key_size);
> + memcpy(rxfh->key, vi->rss_hash_key_data, vi->rss_key_size);
>
> rxfh->hfunc = ETH_RSS_HASH_TOP;
>
> @@ -5426,7 +5393,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
> return -EOPNOTSUPP;
>
> for (i = 0; i < vi->rss_indir_table_size; ++i)
> - vi->rss.indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
> update = true;
> }
>
> @@ -5438,7 +5405,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
> if (!vi->has_rss && !vi->has_rss_hash_report)
> return -EOPNOTSUPP;
>
> - memcpy(vi->rss.key, rxfh->key, vi->rss_key_size);
> + memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
> update = true;
> }
>
> @@ -6044,9 +6011,9 @@ static int virtnet_set_features(struct net_device *dev,
>
> if ((dev->features ^ features) & NETIF_F_RXHASH) {
> if (features & NETIF_F_RXHASH)
> - vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> + vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> else
> - vi->rss.hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
> + vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
>
> if (!virtnet_commit_rss_command(vi))
> return -EINVAL;
> @@ -6735,9 +6702,11 @@ static int virtnet_probe(struct virtio_device *vdev)
> virtio_cread16(vdev, offsetof(struct virtio_net_config,
> rss_max_indirection_table_length));
> }
> - err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size);
> - if (err)
> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
> + if (!vi->rss_hdr) {
> + err = -ENOMEM;
> goto free;
> + }
>
> if (vi->has_rss || vi->has_rss_hash_report) {
> vi->rss_key_size =
> @@ -7016,7 +6985,7 @@ static void virtnet_remove(struct virtio_device *vdev)
>
> remove_vq_common(vi);
>
> - rss_indirection_table_free(&vi->rss);
> + kfree(vi->rss_hdr);
>
> free_netdev(vi->dev);
> }
>
> --
> 2.48.1
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config
2025-03-18 9:56 ` [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config Akihiko Odaki
@ 2025-03-19 1:42 ` Jason Wang
0 siblings, 0 replies; 19+ messages in thread
From: Jason Wang @ 2025-03-19 1:42 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> struct virtio_net_rss_config was less useful in actual code because of a
> flexible array placed in the middle. Add new structures that split it
> into two to avoid having a flexible array in the middle.
>
> Suggested-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 2/4] virtio_net: Fix endian with virtio_net_ctrl_rss
2025-03-18 9:56 ` [PATCH net-next 2/4] virtio_net: Fix endian with virtio_net_ctrl_rss Akihiko Odaki
@ 2025-03-19 1:43 ` Jason Wang
0 siblings, 0 replies; 19+ messages in thread
From: Jason Wang @ 2025-03-19 1:43 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Mark the fields of struct virtio_net_ctrl_rss as little endian as
> they are in struct virtio_net_rss_config, which it follows.
>
> Fixes: c7114b1249fa ("drivers/net/virtio_net: Added basic RSS support.")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-18 9:56 ` [PATCH net-next 3/4] virtio_net: Use new RSS config structs Akihiko Odaki
2025-03-19 1:22 ` Xuan Zhuo
@ 2025-03-19 1:43 ` Jason Wang
2025-03-19 4:48 ` Akihiko Odaki
1 sibling, 1 reply; 19+ messages in thread
From: Jason Wang @ 2025-03-19 1:43 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> The new RSS configuration structures allow easily constructing data for
> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
> for the command.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
> 1 file changed, 43 insertions(+), 74 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index d1ed544ba03a..4153a0a5f278 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -360,24 +360,7 @@ struct receive_queue {
> struct xdp_buff **xsk_buffs;
> };
>
> -/* This structure can contain rss message with maximum settings for indirection table and keysize
> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
> - * contains same info but can't handle table values.
> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
> - * because table sizes may be differ according to the device configuration.
> - */
> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
> -struct virtio_net_ctrl_rss {
> - __le32 hash_types;
> - __le16 indirection_table_mask;
> - __le16 unclassified_queue;
> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
> - __le16 max_tx_vq;
> - u8 hash_key_length;
> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> -
> - __le16 *indirection_table;
> -};
>
> /* Control VQ buffers: protected by the rtnl lock */
> struct control_buf {
> @@ -421,7 +404,9 @@ struct virtnet_info {
> u16 rss_indir_table_size;
> u32 rss_hash_types_supported;
> u32 rss_hash_types_saved;
> - struct virtio_net_ctrl_rss rss;
> + struct virtio_net_rss_config_hdr *rss_hdr;
> + struct virtio_net_rss_config_trailer rss_trailer;
> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>
> /* Has control virtqueue */
> bool has_cvq;
> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
> VIRTNET_XMIT_TYPE_XSK,
> };
>
> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
> {
> - if (!indir_table_size) {
> - rss->indirection_table = NULL;
> - return 0;
> - }
> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
>
> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
> - if (!rss->indirection_table)
> - return -ENOMEM;
> -
> - return 0;
> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
> }
>
> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
> {
> - kfree(rss->indirection_table);
> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
> }
>
> /* We use the last two bits of the pointer to distinguish the xmit type. */
> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
>
> for (; i < vi->rss_indir_table_size; ++i) {
> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
> }
> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
> }
>
> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> {
> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> - struct virtio_net_ctrl_rss old_rss;
> + struct virtio_net_rss_config_hdr *old_rss_hdr;
> + struct virtio_net_rss_config_trailer old_rss_trailer;
> struct net_device *dev = vi->dev;
> struct scatterlist sg;
>
> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> */
> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
> - vi->rss.indirection_table = old_rss.indirection_table;
> + old_rss_hdr = vi->rss_hdr;
> + old_rss_trailer = vi->rss_trailer;
> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
> + if (!vi->rss_hdr) {
> + vi->rss_hdr = old_rss_hdr;
> return -ENOMEM;
> }
>
> + *vi->rss_hdr = *old_rss_hdr;
> virtnet_rss_update_by_qpairs(vi, queue_pairs);
>
> if (!virtnet_commit_rss_command(vi)) {
> /* restore ctrl_rss if commit_rss_command failed */
> - rss_indirection_table_free(&vi->rss);
> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
> + kfree(vi->rss_hdr);
> + vi->rss_hdr = old_rss_hdr;
> + vi->rss_trailer = old_rss_trailer;
>
> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
> queue_pairs);
> return -EINVAL;
> }
> - rss_indirection_table_free(&old_rss);
> + kfree(old_rss_hdr);
> goto succ;
> }
>
> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> {
> struct net_device *dev = vi->dev;
> - struct scatterlist sgs[4];
> - unsigned int sg_buf_size;
> + struct scatterlist sgs[2];
>
> /* prepare sgs */
> - sg_init_table(sgs, 4);
> -
> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
> -
> - if (vi->has_rss) {
> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> - } else {
> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
> - }
> -
> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
> -
> - sg_buf_size = vi->rss_key_size;
> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
> + sg_init_table(sgs, 2);
> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
So I still see this:
if (vi->has_rss || vi->has_rss_hash_report) {
if (!virtnet_commit_rss_command(vi)) {
Should we introduce a hash config helper instead?
Thanks
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 4/4] virtio_net: Allocate rss_hdr with devres
2025-03-18 9:56 ` [PATCH net-next 4/4] virtio_net: Allocate rss_hdr with devres Akihiko Odaki
@ 2025-03-19 1:43 ` Jason Wang
0 siblings, 0 replies; 19+ messages in thread
From: Jason Wang @ 2025-03-19 1:43 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> virtnet_probe() lacks the code to free rss_hdr in its error path.
> Allocate rss_hdr with devres so that it will be automatically freed.
>
> Fixes: 86a48a00efdf ("virtio_net: Support dynamic rss indirection table size")
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> drivers/net/virtio_net.c | 10 ++++------
> 1 file changed, 4 insertions(+), 6 deletions(-)
Acked-by: Jason Wang <jasowang@redhat.com>
Thanks
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-19 1:22 ` Xuan Zhuo
@ 2025-03-19 4:46 ` Akihiko Odaki
0 siblings, 0 replies; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-19 4:46 UTC (permalink / raw)
To: Xuan Zhuo
Cc: virtualization, linux-kernel, netdev, devel, Michael S. Tsirkin,
Jason Wang, Eugenio Pérez, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Andrew Melnychenko,
Joe Damato, Philo Lu
On 2025/03/19 10:22, Xuan Zhuo wrote:
> On Tue, 18 Mar 2025 18:56:53 +0900, Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>> The new RSS configuration structures allow easily constructing data for
>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
>> for the command.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
>> 1 file changed, 43 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index d1ed544ba03a..4153a0a5f278 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -360,24 +360,7 @@ struct receive_queue {
>> struct xdp_buff **xsk_buffs;
>> };
>>
>> -/* This structure can contain rss message with maximum settings for indirection table and keysize
>> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
>> - * contains same info but can't handle table values.
>> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
>> - * because table sizes may be differ according to the device configuration.
>> - */
>> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
>> -struct virtio_net_ctrl_rss {
>> - __le32 hash_types;
>> - __le16 indirection_table_mask;
>> - __le16 unclassified_queue;
>> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
>> - __le16 max_tx_vq;
>> - u8 hash_key_length;
>> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>> -
>> - __le16 *indirection_table;
>> -};
>>
>> /* Control VQ buffers: protected by the rtnl lock */
>> struct control_buf {
>> @@ -421,7 +404,9 @@ struct virtnet_info {
>> u16 rss_indir_table_size;
>> u32 rss_hash_types_supported;
>> u32 rss_hash_types_saved;
>> - struct virtio_net_ctrl_rss rss;
>> + struct virtio_net_rss_config_hdr *rss_hdr;
>> + struct virtio_net_rss_config_trailer rss_trailer;
>> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>
> Why put rss_hash_key_data outside of virtio_net_rss_config_trailer?
virtio_net_rss_config_trailer has a field named hash_key_data, but it is
a flexible array without storage. The structure is defined in a UAPI
header, and its other users may provide storages of different sizes. So
we need to provide a storage with our own size limit.
Regards,
Akihiko Odaki
>
> Thanks.
>
>
>>
>> /* Has control virtqueue */
>> bool has_cvq;
>> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
>> VIRTNET_XMIT_TYPE_XSK,
>> };
>>
>> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
>> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
>> {
>> - if (!indir_table_size) {
>> - rss->indirection_table = NULL;
>> - return 0;
>> - }
>> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
>>
>> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
>> - if (!rss->indirection_table)
>> - return -ENOMEM;
>> -
>> - return 0;
>> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
>> }
>>
>> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
>> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
>> {
>> - kfree(rss->indirection_table);
>> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
>> }
>>
>> /* We use the last two bits of the pointer to distinguish the xmit type. */
>> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
>>
>> for (; i < vi->rss_indir_table_size; ++i) {
>> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
>> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
>> }
>> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
>> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
>> }
>>
>> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> {
>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>> - struct virtio_net_ctrl_rss old_rss;
>> + struct virtio_net_rss_config_hdr *old_rss_hdr;
>> + struct virtio_net_rss_config_trailer old_rss_trailer;
>> struct net_device *dev = vi->dev;
>> struct scatterlist sg;
>>
>> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>> */
>> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
>> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
>> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
>> - vi->rss.indirection_table = old_rss.indirection_table;
>> + old_rss_hdr = vi->rss_hdr;
>> + old_rss_trailer = vi->rss_trailer;
>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
>> + if (!vi->rss_hdr) {
>> + vi->rss_hdr = old_rss_hdr;
>> return -ENOMEM;
>> }
>>
>> + *vi->rss_hdr = *old_rss_hdr;
>> virtnet_rss_update_by_qpairs(vi, queue_pairs);
>>
>> if (!virtnet_commit_rss_command(vi)) {
>> /* restore ctrl_rss if commit_rss_command failed */
>> - rss_indirection_table_free(&vi->rss);
>> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
>> + kfree(vi->rss_hdr);
>> + vi->rss_hdr = old_rss_hdr;
>> + vi->rss_trailer = old_rss_trailer;
>>
>> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
>> queue_pairs);
>> return -EINVAL;
>> }
>> - rss_indirection_table_free(&old_rss);
>> + kfree(old_rss_hdr);
>> goto succ;
>> }
>>
>> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
>> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>> {
>> struct net_device *dev = vi->dev;
>> - struct scatterlist sgs[4];
>> - unsigned int sg_buf_size;
>> + struct scatterlist sgs[2];
>>
>> /* prepare sgs */
>> - sg_init_table(sgs, 4);
>> -
>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
>> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
>> -
>> - if (vi->has_rss) {
>> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
>> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
>> - } else {
>> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
>> - }
>> -
>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
>> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
>> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
>> -
>> - sg_buf_size = vi->rss_key_size;
>> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
>> + sg_init_table(sgs, 2);
>> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
>> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
>>
>> if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_MQ,
>> vi->has_rss ? VIRTIO_NET_CTRL_MQ_RSS_CONFIG
>> @@ -4097,17 +4064,17 @@ static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>>
>> static void virtnet_init_default_rss(struct virtnet_info *vi)
>> {
>> - vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_supported);
>> + vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_supported);
>> vi->rss_hash_types_saved = vi->rss_hash_types_supported;
>> - vi->rss.indirection_table_mask = vi->rss_indir_table_size
>> + vi->rss_hdr->indirection_table_mask = vi->rss_indir_table_size
>> ? cpu_to_le16(vi->rss_indir_table_size - 1) : 0;
>> - vi->rss.unclassified_queue = 0;
>> + vi->rss_hdr->unclassified_queue = 0;
>>
>> virtnet_rss_update_by_qpairs(vi, vi->curr_queue_pairs);
>>
>> - vi->rss.hash_key_length = vi->rss_key_size;
>> + vi->rss_trailer.hash_key_length = vi->rss_key_size;
>>
>> - netdev_rss_key_fill(vi->rss.key, vi->rss_key_size);
>> + netdev_rss_key_fill(vi->rss_hash_key_data, vi->rss_key_size);
>> }
>>
>> static void virtnet_get_hashflow(const struct virtnet_info *vi, struct ethtool_rxnfc *info)
>> @@ -4218,7 +4185,7 @@ static bool virtnet_set_hashflow(struct virtnet_info *vi, struct ethtool_rxnfc *
>>
>> if (new_hashtypes != vi->rss_hash_types_saved) {
>> vi->rss_hash_types_saved = new_hashtypes;
>> - vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
>> + vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
>> if (vi->dev->features & NETIF_F_RXHASH)
>> return virtnet_commit_rss_command(vi);
>> }
>> @@ -5398,11 +5365,11 @@ static int virtnet_get_rxfh(struct net_device *dev,
>>
>> if (rxfh->indir) {
>> for (i = 0; i < vi->rss_indir_table_size; ++i)
>> - rxfh->indir[i] = le16_to_cpu(vi->rss.indirection_table[i]);
>> + rxfh->indir[i] = le16_to_cpu(vi->rss_hdr->indirection_table[i]);
>> }
>>
>> if (rxfh->key)
>> - memcpy(rxfh->key, vi->rss.key, vi->rss_key_size);
>> + memcpy(rxfh->key, vi->rss_hash_key_data, vi->rss_key_size);
>>
>> rxfh->hfunc = ETH_RSS_HASH_TOP;
>>
>> @@ -5426,7 +5393,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
>> return -EOPNOTSUPP;
>>
>> for (i = 0; i < vi->rss_indir_table_size; ++i)
>> - vi->rss.indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
>> update = true;
>> }
>>
>> @@ -5438,7 +5405,7 @@ static int virtnet_set_rxfh(struct net_device *dev,
>> if (!vi->has_rss && !vi->has_rss_hash_report)
>> return -EOPNOTSUPP;
>>
>> - memcpy(vi->rss.key, rxfh->key, vi->rss_key_size);
>> + memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
>> update = true;
>> }
>>
>> @@ -6044,9 +6011,9 @@ static int virtnet_set_features(struct net_device *dev,
>>
>> if ((dev->features ^ features) & NETIF_F_RXHASH) {
>> if (features & NETIF_F_RXHASH)
>> - vi->rss.hash_types = cpu_to_le32(vi->rss_hash_types_saved);
>> + vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
>> else
>> - vi->rss.hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
>> + vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
>>
>> if (!virtnet_commit_rss_command(vi))
>> return -EINVAL;
>> @@ -6735,9 +6702,11 @@ static int virtnet_probe(struct virtio_device *vdev)
>> virtio_cread16(vdev, offsetof(struct virtio_net_config,
>> rss_max_indirection_table_length));
>> }
>> - err = rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size);
>> - if (err)
>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
>> + if (!vi->rss_hdr) {
>> + err = -ENOMEM;
>> goto free;
>> + }
>>
>> if (vi->has_rss || vi->has_rss_hash_report) {
>> vi->rss_key_size =
>> @@ -7016,7 +6985,7 @@ static void virtnet_remove(struct virtio_device *vdev)
>>
>> remove_vq_common(vi);
>>
>> - rss_indirection_table_free(&vi->rss);
>> + kfree(vi->rss_hdr);
>>
>> free_netdev(vi->dev);
>> }
>>
>> --
>> 2.48.1
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-19 1:43 ` Jason Wang
@ 2025-03-19 4:48 ` Akihiko Odaki
2025-03-20 1:50 ` Jason Wang
0 siblings, 1 reply; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-19 4:48 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On 2025/03/19 10:43, Jason Wang wrote:
> On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> The new RSS configuration structures allow easily constructing data for
>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
>> for the command.
>>
>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>> ---
>> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
>> 1 file changed, 43 insertions(+), 74 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index d1ed544ba03a..4153a0a5f278 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -360,24 +360,7 @@ struct receive_queue {
>> struct xdp_buff **xsk_buffs;
>> };
>>
>> -/* This structure can contain rss message with maximum settings for indirection table and keysize
>> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
>> - * contains same info but can't handle table values.
>> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
>> - * because table sizes may be differ according to the device configuration.
>> - */
>> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
>> -struct virtio_net_ctrl_rss {
>> - __le32 hash_types;
>> - __le16 indirection_table_mask;
>> - __le16 unclassified_queue;
>> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
>> - __le16 max_tx_vq;
>> - u8 hash_key_length;
>> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>> -
>> - __le16 *indirection_table;
>> -};
>>
>> /* Control VQ buffers: protected by the rtnl lock */
>> struct control_buf {
>> @@ -421,7 +404,9 @@ struct virtnet_info {
>> u16 rss_indir_table_size;
>> u32 rss_hash_types_supported;
>> u32 rss_hash_types_saved;
>> - struct virtio_net_ctrl_rss rss;
>> + struct virtio_net_rss_config_hdr *rss_hdr;
>> + struct virtio_net_rss_config_trailer rss_trailer;
>> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>>
>> /* Has control virtqueue */
>> bool has_cvq;
>> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
>> VIRTNET_XMIT_TYPE_XSK,
>> };
>>
>> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
>> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
>> {
>> - if (!indir_table_size) {
>> - rss->indirection_table = NULL;
>> - return 0;
>> - }
>> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
>>
>> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
>> - if (!rss->indirection_table)
>> - return -ENOMEM;
>> -
>> - return 0;
>> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
>> }
>>
>> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
>> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
>> {
>> - kfree(rss->indirection_table);
>> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
>> }
>>
>> /* We use the last two bits of the pointer to distinguish the xmit type. */
>> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
>>
>> for (; i < vi->rss_indir_table_size; ++i) {
>> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
>> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
>> }
>> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
>> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
>> }
>>
>> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> {
>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>> - struct virtio_net_ctrl_rss old_rss;
>> + struct virtio_net_rss_config_hdr *old_rss_hdr;
>> + struct virtio_net_rss_config_trailer old_rss_trailer;
>> struct net_device *dev = vi->dev;
>> struct scatterlist sg;
>>
>> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>> */
>> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
>> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
>> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
>> - vi->rss.indirection_table = old_rss.indirection_table;
>> + old_rss_hdr = vi->rss_hdr;
>> + old_rss_trailer = vi->rss_trailer;
>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
>> + if (!vi->rss_hdr) {
>> + vi->rss_hdr = old_rss_hdr;
>> return -ENOMEM;
>> }
>>
>> + *vi->rss_hdr = *old_rss_hdr;
>> virtnet_rss_update_by_qpairs(vi, queue_pairs);
>>
>> if (!virtnet_commit_rss_command(vi)) {
>> /* restore ctrl_rss if commit_rss_command failed */
>> - rss_indirection_table_free(&vi->rss);
>> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
>> + kfree(vi->rss_hdr);
>> + vi->rss_hdr = old_rss_hdr;
>> + vi->rss_trailer = old_rss_trailer;
>>
>> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
>> queue_pairs);
>> return -EINVAL;
>> }
>> - rss_indirection_table_free(&old_rss);
>> + kfree(old_rss_hdr);
>> goto succ;
>> }
>>
>> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
>> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>> {
>> struct net_device *dev = vi->dev;
>> - struct scatterlist sgs[4];
>> - unsigned int sg_buf_size;
>> + struct scatterlist sgs[2];
>>
>> /* prepare sgs */
>> - sg_init_table(sgs, 4);
>> -
>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
>> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
>> -
>> - if (vi->has_rss) {
>> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
>> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
>> - } else {
>> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
>> - }
>> -
>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
>> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
>> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
>> -
>> - sg_buf_size = vi->rss_key_size;
>> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
>> + sg_init_table(sgs, 2);
>> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
>> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
>
> So I still see this:
>
> if (vi->has_rss || vi->has_rss_hash_report) {
> if (!virtnet_commit_rss_command(vi)) {
>
> Should we introduce a hash config helper instead?
I think it's fine to use virtnet_commit_rss_command() for hash
reporting. struct virtio_net_hash_config and struct
virtio_net_rss_config are defined to have a common layout to allow
sharing this kind of logic.
Regards,
Akihiko Odaki
>
> Thanks
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 0/4] virtio_net: Fixes and improvements
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
` (3 preceding siblings ...)
2025-03-18 9:56 ` [PATCH net-next 4/4] virtio_net: Allocate rss_hdr with devres Akihiko Odaki
@ 2025-03-19 8:17 ` Lei Yang
2025-03-19 9:07 ` Michael S. Tsirkin
5 siblings, 0 replies; 19+ messages in thread
From: Lei Yang @ 2025-03-19 8:17 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Andrew Melnychenko, Joe Damato, Philo Lu,
virtualization, linux-kernel, netdev, devel
QE tested this series of patches with virtio_net regression tests,
everything works fine.
Tested-by: Lei Yang <leiyang@redhat.com>
On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> Jason Wang recently proposed an improvement to struct
> virtio_net_rss_config:
> https://lore.kernel.org/r/CACGkMEud0Ki8p=z299Q7b4qEDONpYDzbVqhHxCNVk_vo-KdP9A@mail.gmail.com
>
> This patch series implements it and also fixes a few minor bugs I found
> when writing patches.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> ---
> Akihiko Odaki (4):
> virtio_net: Split struct virtio_net_rss_config
> virtio_net: Fix endian with virtio_net_ctrl_rss
> virtio_net: Use new RSS config structs
> virtio_net: Allocate rss_hdr with devres
>
> drivers/net/virtio_net.c | 119 +++++++++++++++-------------------------
> include/uapi/linux/virtio_net.h | 13 +++++
> 2 files changed, 56 insertions(+), 76 deletions(-)
> ---
> base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
> change-id: 20250318-virtio-6559d69187db
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 0/4] virtio_net: Fixes and improvements
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
` (4 preceding siblings ...)
2025-03-19 8:17 ` [PATCH net-next 0/4] virtio_net: Fixes and improvements Lei Yang
@ 2025-03-19 9:07 ` Michael S. Tsirkin
5 siblings, 0 replies; 19+ messages in thread
From: Michael S. Tsirkin @ 2025-03-19 9:07 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Jason Wang, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Tue, Mar 18, 2025 at 06:56:50PM +0900, Akihiko Odaki wrote:
> Jason Wang recently proposed an improvement to struct
> virtio_net_rss_config:
> https://lore.kernel.org/r/CACGkMEud0Ki8p=z299Q7b4qEDONpYDzbVqhHxCNVk_vo-KdP9A@mail.gmail.com
>
> This patch series implements it and also fixes a few minor bugs I found
> when writing patches.
>
> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Akihiko Odaki (4):
> virtio_net: Split struct virtio_net_rss_config
> virtio_net: Fix endian with virtio_net_ctrl_rss
> virtio_net: Use new RSS config structs
> virtio_net: Allocate rss_hdr with devres
>
> drivers/net/virtio_net.c | 119 +++++++++++++++-------------------------
> include/uapi/linux/virtio_net.h | 13 +++++
> 2 files changed, 56 insertions(+), 76 deletions(-)
> ---
> base-commit: d082ecbc71e9e0bf49883ee4afd435a77a5101b6
> change-id: 20250318-virtio-6559d69187db
>
> Best regards,
> --
> Akihiko Odaki <akihiko.odaki@daynix.com>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-19 4:48 ` Akihiko Odaki
@ 2025-03-20 1:50 ` Jason Wang
2025-03-20 5:36 ` Akihiko Odaki
0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2025-03-20 1:50 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Wed, Mar 19, 2025 at 12:48 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/19 10:43, Jason Wang wrote:
> > On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> The new RSS configuration structures allow easily constructing data for
> >> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
> >> for the command.
> >>
> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >> ---
> >> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
> >> 1 file changed, 43 insertions(+), 74 deletions(-)
> >>
> >> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >> index d1ed544ba03a..4153a0a5f278 100644
> >> --- a/drivers/net/virtio_net.c
> >> +++ b/drivers/net/virtio_net.c
> >> @@ -360,24 +360,7 @@ struct receive_queue {
> >> struct xdp_buff **xsk_buffs;
> >> };
> >>
> >> -/* This structure can contain rss message with maximum settings for indirection table and keysize
> >> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
> >> - * contains same info but can't handle table values.
> >> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
> >> - * because table sizes may be differ according to the device configuration.
> >> - */
> >> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
> >> -struct virtio_net_ctrl_rss {
> >> - __le32 hash_types;
> >> - __le16 indirection_table_mask;
> >> - __le16 unclassified_queue;
> >> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
> >> - __le16 max_tx_vq;
> >> - u8 hash_key_length;
> >> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >> -
> >> - __le16 *indirection_table;
> >> -};
> >>
> >> /* Control VQ buffers: protected by the rtnl lock */
> >> struct control_buf {
> >> @@ -421,7 +404,9 @@ struct virtnet_info {
> >> u16 rss_indir_table_size;
> >> u32 rss_hash_types_supported;
> >> u32 rss_hash_types_saved;
> >> - struct virtio_net_ctrl_rss rss;
> >> + struct virtio_net_rss_config_hdr *rss_hdr;
> >> + struct virtio_net_rss_config_trailer rss_trailer;
> >> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >>
> >> /* Has control virtqueue */
> >> bool has_cvq;
> >> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
> >> VIRTNET_XMIT_TYPE_XSK,
> >> };
> >>
> >> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
> >> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
> >> {
> >> - if (!indir_table_size) {
> >> - rss->indirection_table = NULL;
> >> - return 0;
> >> - }
> >> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
> >>
> >> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
> >> - if (!rss->indirection_table)
> >> - return -ENOMEM;
> >> -
> >> - return 0;
> >> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
> >> }
> >>
> >> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
> >> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
> >> {
> >> - kfree(rss->indirection_table);
> >> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
> >> }
> >>
> >> /* We use the last two bits of the pointer to distinguish the xmit type. */
> >> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
> >>
> >> for (; i < vi->rss_indir_table_size; ++i) {
> >> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
> >> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
> >> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
> >> }
> >> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
> >> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
> >> }
> >>
> >> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> >> {
> >> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> >> - struct virtio_net_ctrl_rss old_rss;
> >> + struct virtio_net_rss_config_hdr *old_rss_hdr;
> >> + struct virtio_net_rss_config_trailer old_rss_trailer;
> >> struct net_device *dev = vi->dev;
> >> struct scatterlist sg;
> >>
> >> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> >> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> >> */
> >> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> >> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
> >> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
> >> - vi->rss.indirection_table = old_rss.indirection_table;
> >> + old_rss_hdr = vi->rss_hdr;
> >> + old_rss_trailer = vi->rss_trailer;
> >> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
> >> + if (!vi->rss_hdr) {
> >> + vi->rss_hdr = old_rss_hdr;
> >> return -ENOMEM;
> >> }
> >>
> >> + *vi->rss_hdr = *old_rss_hdr;
> >> virtnet_rss_update_by_qpairs(vi, queue_pairs);
> >>
> >> if (!virtnet_commit_rss_command(vi)) {
> >> /* restore ctrl_rss if commit_rss_command failed */
> >> - rss_indirection_table_free(&vi->rss);
> >> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
> >> + kfree(vi->rss_hdr);
> >> + vi->rss_hdr = old_rss_hdr;
> >> + vi->rss_trailer = old_rss_trailer;
> >>
> >> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
> >> queue_pairs);
> >> return -EINVAL;
> >> }
> >> - rss_indirection_table_free(&old_rss);
> >> + kfree(old_rss_hdr);
> >> goto succ;
> >> }
> >>
> >> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
> >> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> >> {
> >> struct net_device *dev = vi->dev;
> >> - struct scatterlist sgs[4];
> >> - unsigned int sg_buf_size;
> >> + struct scatterlist sgs[2];
> >>
> >> /* prepare sgs */
> >> - sg_init_table(sgs, 4);
> >> -
> >> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
> >> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
> >> -
> >> - if (vi->has_rss) {
> >> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
> >> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> >> - } else {
> >> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
> >> - }
> >> -
> >> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
> >> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> >> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
> >> -
> >> - sg_buf_size = vi->rss_key_size;
> >> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
> >> + sg_init_table(sgs, 2);
> >> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
> >> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
> >
> > So I still see this:
> >
> > if (vi->has_rss || vi->has_rss_hash_report) {
> > if (!virtnet_commit_rss_command(vi)) {
> >
> > Should we introduce a hash config helper instead?
>
> I think it's fine to use virtnet_commit_rss_command() for hash
> reporting. struct virtio_net_hash_config and struct
> virtio_net_rss_config are defined to have a common layout to allow
> sharing this kind of logic.
Well, this trick won't work if the reserved field in hash_config is
used in the future.
Thanks
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-20 1:50 ` Jason Wang
@ 2025-03-20 5:36 ` Akihiko Odaki
2025-03-21 0:35 ` Jason Wang
0 siblings, 1 reply; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-20 5:36 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On 2025/03/20 10:50, Jason Wang wrote:
> On Wed, Mar 19, 2025 at 12:48 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/03/19 10:43, Jason Wang wrote:
>>> On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> The new RSS configuration structures allow easily constructing data for
>>>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
>>>> for the command.
>>>>
>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>> ---
>>>> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
>>>> 1 file changed, 43 insertions(+), 74 deletions(-)
>>>>
>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>> index d1ed544ba03a..4153a0a5f278 100644
>>>> --- a/drivers/net/virtio_net.c
>>>> +++ b/drivers/net/virtio_net.c
>>>> @@ -360,24 +360,7 @@ struct receive_queue {
>>>> struct xdp_buff **xsk_buffs;
>>>> };
>>>>
>>>> -/* This structure can contain rss message with maximum settings for indirection table and keysize
>>>> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
>>>> - * contains same info but can't handle table values.
>>>> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
>>>> - * because table sizes may be differ according to the device configuration.
>>>> - */
>>>> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
>>>> -struct virtio_net_ctrl_rss {
>>>> - __le32 hash_types;
>>>> - __le16 indirection_table_mask;
>>>> - __le16 unclassified_queue;
>>>> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
>>>> - __le16 max_tx_vq;
>>>> - u8 hash_key_length;
>>>> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>>>> -
>>>> - __le16 *indirection_table;
>>>> -};
>>>>
>>>> /* Control VQ buffers: protected by the rtnl lock */
>>>> struct control_buf {
>>>> @@ -421,7 +404,9 @@ struct virtnet_info {
>>>> u16 rss_indir_table_size;
>>>> u32 rss_hash_types_supported;
>>>> u32 rss_hash_types_saved;
>>>> - struct virtio_net_ctrl_rss rss;
>>>> + struct virtio_net_rss_config_hdr *rss_hdr;
>>>> + struct virtio_net_rss_config_trailer rss_trailer;
>>>> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>>>>
>>>> /* Has control virtqueue */
>>>> bool has_cvq;
>>>> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
>>>> VIRTNET_XMIT_TYPE_XSK,
>>>> };
>>>>
>>>> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
>>>> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
>>>> {
>>>> - if (!indir_table_size) {
>>>> - rss->indirection_table = NULL;
>>>> - return 0;
>>>> - }
>>>> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
>>>>
>>>> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
>>>> - if (!rss->indirection_table)
>>>> - return -ENOMEM;
>>>> -
>>>> - return 0;
>>>> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
>>>> }
>>>>
>>>> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
>>>> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
>>>> {
>>>> - kfree(rss->indirection_table);
>>>> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
>>>> }
>>>>
>>>> /* We use the last two bits of the pointer to distinguish the xmit type. */
>>>> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
>>>>
>>>> for (; i < vi->rss_indir_table_size; ++i) {
>>>> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
>>>> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
>>>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
>>>> }
>>>> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
>>>> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
>>>> }
>>>>
>>>> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>> {
>>>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>>>> - struct virtio_net_ctrl_rss old_rss;
>>>> + struct virtio_net_rss_config_hdr *old_rss_hdr;
>>>> + struct virtio_net_rss_config_trailer old_rss_trailer;
>>>> struct net_device *dev = vi->dev;
>>>> struct scatterlist sg;
>>>>
>>>> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>>>> */
>>>> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
>>>> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
>>>> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
>>>> - vi->rss.indirection_table = old_rss.indirection_table;
>>>> + old_rss_hdr = vi->rss_hdr;
>>>> + old_rss_trailer = vi->rss_trailer;
>>>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
>>>> + if (!vi->rss_hdr) {
>>>> + vi->rss_hdr = old_rss_hdr;
>>>> return -ENOMEM;
>>>> }
>>>>
>>>> + *vi->rss_hdr = *old_rss_hdr;
>>>> virtnet_rss_update_by_qpairs(vi, queue_pairs);
>>>>
>>>> if (!virtnet_commit_rss_command(vi)) {
>>>> /* restore ctrl_rss if commit_rss_command failed */
>>>> - rss_indirection_table_free(&vi->rss);
>>>> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
>>>> + kfree(vi->rss_hdr);
>>>> + vi->rss_hdr = old_rss_hdr;
>>>> + vi->rss_trailer = old_rss_trailer;
>>>>
>>>> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
>>>> queue_pairs);
>>>> return -EINVAL;
>>>> }
>>>> - rss_indirection_table_free(&old_rss);
>>>> + kfree(old_rss_hdr);
>>>> goto succ;
>>>> }
>>>>
>>>> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
>>>> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>>>> {
>>>> struct net_device *dev = vi->dev;
>>>> - struct scatterlist sgs[4];
>>>> - unsigned int sg_buf_size;
>>>> + struct scatterlist sgs[2];
>>>>
>>>> /* prepare sgs */
>>>> - sg_init_table(sgs, 4);
>>>> -
>>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
>>>> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
>>>> -
>>>> - if (vi->has_rss) {
>>>> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
>>>> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
>>>> - } else {
>>>> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
>>>> - }
>>>> -
>>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
>>>> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
>>>> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
>>>> -
>>>> - sg_buf_size = vi->rss_key_size;
>>>> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
>>>> + sg_init_table(sgs, 2);
>>>> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
>>>> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
>>>
>>> So I still see this:
>>>
>>> if (vi->has_rss || vi->has_rss_hash_report) {
>>> if (!virtnet_commit_rss_command(vi)) {
>>>
>>> Should we introduce a hash config helper instead?
>>
>> I think it's fine to use virtnet_commit_rss_command() for hash
>> reporting. struct virtio_net_hash_config and struct
>> virtio_net_rss_config are defined to have a common layout to allow
>> sharing this kind of logic.
>
> Well, this trick won't work if the reserved field in hash_config is
> used in the future.
Right, but we can add a hash config helper when that happens. It will
only result in a duplication of logic for now.
Regards,
Akihiko Odaki
>
> Thanks
>
>>
>> Regards,
>> Akihiko Odaki
>>
>>>
>>> Thanks
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-20 5:36 ` Akihiko Odaki
@ 2025-03-21 0:35 ` Jason Wang
2025-03-21 6:34 ` Akihiko Odaki
0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2025-03-21 0:35 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Thu, Mar 20, 2025 at 1:36 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/20 10:50, Jason Wang wrote:
> > On Wed, Mar 19, 2025 at 12:48 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/03/19 10:43, Jason Wang wrote:
> >>> On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> The new RSS configuration structures allow easily constructing data for
> >>>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
> >>>> for the command.
> >>>>
> >>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>> ---
> >>>> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
> >>>> 1 file changed, 43 insertions(+), 74 deletions(-)
> >>>>
> >>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>> index d1ed544ba03a..4153a0a5f278 100644
> >>>> --- a/drivers/net/virtio_net.c
> >>>> +++ b/drivers/net/virtio_net.c
> >>>> @@ -360,24 +360,7 @@ struct receive_queue {
> >>>> struct xdp_buff **xsk_buffs;
> >>>> };
> >>>>
> >>>> -/* This structure can contain rss message with maximum settings for indirection table and keysize
> >>>> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
> >>>> - * contains same info but can't handle table values.
> >>>> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
> >>>> - * because table sizes may be differ according to the device configuration.
> >>>> - */
> >>>> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
> >>>> -struct virtio_net_ctrl_rss {
> >>>> - __le32 hash_types;
> >>>> - __le16 indirection_table_mask;
> >>>> - __le16 unclassified_queue;
> >>>> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
> >>>> - __le16 max_tx_vq;
> >>>> - u8 hash_key_length;
> >>>> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >>>> -
> >>>> - __le16 *indirection_table;
> >>>> -};
> >>>>
> >>>> /* Control VQ buffers: protected by the rtnl lock */
> >>>> struct control_buf {
> >>>> @@ -421,7 +404,9 @@ struct virtnet_info {
> >>>> u16 rss_indir_table_size;
> >>>> u32 rss_hash_types_supported;
> >>>> u32 rss_hash_types_saved;
> >>>> - struct virtio_net_ctrl_rss rss;
> >>>> + struct virtio_net_rss_config_hdr *rss_hdr;
> >>>> + struct virtio_net_rss_config_trailer rss_trailer;
> >>>> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >>>>
> >>>> /* Has control virtqueue */
> >>>> bool has_cvq;
> >>>> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
> >>>> VIRTNET_XMIT_TYPE_XSK,
> >>>> };
> >>>>
> >>>> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
> >>>> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
> >>>> {
> >>>> - if (!indir_table_size) {
> >>>> - rss->indirection_table = NULL;
> >>>> - return 0;
> >>>> - }
> >>>> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
> >>>>
> >>>> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
> >>>> - if (!rss->indirection_table)
> >>>> - return -ENOMEM;
> >>>> -
> >>>> - return 0;
> >>>> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
> >>>> }
> >>>>
> >>>> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
> >>>> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
> >>>> {
> >>>> - kfree(rss->indirection_table);
> >>>> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
> >>>> }
> >>>>
> >>>> /* We use the last two bits of the pointer to distinguish the xmit type. */
> >>>> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
> >>>>
> >>>> for (; i < vi->rss_indir_table_size; ++i) {
> >>>> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
> >>>> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
> >>>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
> >>>> }
> >>>> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
> >>>> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
> >>>> }
> >>>>
> >>>> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> >>>> {
> >>>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> >>>> - struct virtio_net_ctrl_rss old_rss;
> >>>> + struct virtio_net_rss_config_hdr *old_rss_hdr;
> >>>> + struct virtio_net_rss_config_trailer old_rss_trailer;
> >>>> struct net_device *dev = vi->dev;
> >>>> struct scatterlist sg;
> >>>>
> >>>> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> >>>> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> >>>> */
> >>>> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> >>>> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
> >>>> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
> >>>> - vi->rss.indirection_table = old_rss.indirection_table;
> >>>> + old_rss_hdr = vi->rss_hdr;
> >>>> + old_rss_trailer = vi->rss_trailer;
> >>>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
> >>>> + if (!vi->rss_hdr) {
> >>>> + vi->rss_hdr = old_rss_hdr;
> >>>> return -ENOMEM;
> >>>> }
> >>>>
> >>>> + *vi->rss_hdr = *old_rss_hdr;
> >>>> virtnet_rss_update_by_qpairs(vi, queue_pairs);
> >>>>
> >>>> if (!virtnet_commit_rss_command(vi)) {
> >>>> /* restore ctrl_rss if commit_rss_command failed */
> >>>> - rss_indirection_table_free(&vi->rss);
> >>>> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
> >>>> + kfree(vi->rss_hdr);
> >>>> + vi->rss_hdr = old_rss_hdr;
> >>>> + vi->rss_trailer = old_rss_trailer;
> >>>>
> >>>> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
> >>>> queue_pairs);
> >>>> return -EINVAL;
> >>>> }
> >>>> - rss_indirection_table_free(&old_rss);
> >>>> + kfree(old_rss_hdr);
> >>>> goto succ;
> >>>> }
> >>>>
> >>>> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
> >>>> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> >>>> {
> >>>> struct net_device *dev = vi->dev;
> >>>> - struct scatterlist sgs[4];
> >>>> - unsigned int sg_buf_size;
> >>>> + struct scatterlist sgs[2];
> >>>>
> >>>> /* prepare sgs */
> >>>> - sg_init_table(sgs, 4);
> >>>> -
> >>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
> >>>> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
> >>>> -
> >>>> - if (vi->has_rss) {
> >>>> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
> >>>> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> >>>> - } else {
> >>>> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
> >>>> - }
> >>>> -
> >>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
> >>>> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> >>>> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
> >>>> -
> >>>> - sg_buf_size = vi->rss_key_size;
> >>>> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
> >>>> + sg_init_table(sgs, 2);
> >>>> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
> >>>> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
> >>>
> >>> So I still see this:
> >>>
> >>> if (vi->has_rss || vi->has_rss_hash_report) {
> >>> if (!virtnet_commit_rss_command(vi)) {
> >>>
> >>> Should we introduce a hash config helper instead?
> >>
> >> I think it's fine to use virtnet_commit_rss_command() for hash
> >> reporting. struct virtio_net_hash_config and struct
> >> virtio_net_rss_config are defined to have a common layout to allow
> >> sharing this kind of logic.
> >
> > Well, this trick won't work if the reserved field in hash_config is
> > used in the future.
>
> Right, but we can add a hash config helper when that happens. It will
> only result in a duplication of logic for now.
>
> Regards,
> Akihiko Odaki
That's tricky as the cvq commands were designed to be used separately.
Let's use a separate helper and virtio_net_hash_config uAPIs now.
Thanks
>
> >
> > Thanks
> >
> >>
> >> Regards,
> >> Akihiko Odaki
> >>
> >>>
> >>> Thanks
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-21 0:35 ` Jason Wang
@ 2025-03-21 6:34 ` Akihiko Odaki
2025-03-24 4:05 ` Jason Wang
0 siblings, 1 reply; 19+ messages in thread
From: Akihiko Odaki @ 2025-03-21 6:34 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On 2025/03/21 9:35, Jason Wang wrote:
> On Thu, Mar 20, 2025 at 1:36 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>
>> On 2025/03/20 10:50, Jason Wang wrote:
>>> On Wed, Mar 19, 2025 at 12:48 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>
>>>> On 2025/03/19 10:43, Jason Wang wrote:
>>>>> On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>>>>>>
>>>>>> The new RSS configuration structures allow easily constructing data for
>>>>>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
>>>>>> for the command.
>>>>>>
>>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
>>>>>> ---
>>>>>> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
>>>>>> 1 file changed, 43 insertions(+), 74 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>>>>>> index d1ed544ba03a..4153a0a5f278 100644
>>>>>> --- a/drivers/net/virtio_net.c
>>>>>> +++ b/drivers/net/virtio_net.c
>>>>>> @@ -360,24 +360,7 @@ struct receive_queue {
>>>>>> struct xdp_buff **xsk_buffs;
>>>>>> };
>>>>>>
>>>>>> -/* This structure can contain rss message with maximum settings for indirection table and keysize
>>>>>> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
>>>>>> - * contains same info but can't handle table values.
>>>>>> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
>>>>>> - * because table sizes may be differ according to the device configuration.
>>>>>> - */
>>>>>> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
>>>>>> -struct virtio_net_ctrl_rss {
>>>>>> - __le32 hash_types;
>>>>>> - __le16 indirection_table_mask;
>>>>>> - __le16 unclassified_queue;
>>>>>> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
>>>>>> - __le16 max_tx_vq;
>>>>>> - u8 hash_key_length;
>>>>>> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>>>>>> -
>>>>>> - __le16 *indirection_table;
>>>>>> -};
>>>>>>
>>>>>> /* Control VQ buffers: protected by the rtnl lock */
>>>>>> struct control_buf {
>>>>>> @@ -421,7 +404,9 @@ struct virtnet_info {
>>>>>> u16 rss_indir_table_size;
>>>>>> u32 rss_hash_types_supported;
>>>>>> u32 rss_hash_types_saved;
>>>>>> - struct virtio_net_ctrl_rss rss;
>>>>>> + struct virtio_net_rss_config_hdr *rss_hdr;
>>>>>> + struct virtio_net_rss_config_trailer rss_trailer;
>>>>>> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
>>>>>>
>>>>>> /* Has control virtqueue */
>>>>>> bool has_cvq;
>>>>>> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
>>>>>> VIRTNET_XMIT_TYPE_XSK,
>>>>>> };
>>>>>>
>>>>>> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
>>>>>> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
>>>>>> {
>>>>>> - if (!indir_table_size) {
>>>>>> - rss->indirection_table = NULL;
>>>>>> - return 0;
>>>>>> - }
>>>>>> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
>>>>>>
>>>>>> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
>>>>>> - if (!rss->indirection_table)
>>>>>> - return -ENOMEM;
>>>>>> -
>>>>>> - return 0;
>>>>>> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
>>>>>> }
>>>>>>
>>>>>> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
>>>>>> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
>>>>>> {
>>>>>> - kfree(rss->indirection_table);
>>>>>> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
>>>>>> }
>>>>>>
>>>>>> /* We use the last two bits of the pointer to distinguish the xmit type. */
>>>>>> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
>>>>>>
>>>>>> for (; i < vi->rss_indir_table_size; ++i) {
>>>>>> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
>>>>>> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
>>>>>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
>>>>>> }
>>>>>> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
>>>>>> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
>>>>>> }
>>>>>>
>>>>>> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>>>> {
>>>>>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
>>>>>> - struct virtio_net_ctrl_rss old_rss;
>>>>>> + struct virtio_net_rss_config_hdr *old_rss_hdr;
>>>>>> + struct virtio_net_rss_config_trailer old_rss_trailer;
>>>>>> struct net_device *dev = vi->dev;
>>>>>> struct scatterlist sg;
>>>>>>
>>>>>> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
>>>>>> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
>>>>>> */
>>>>>> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
>>>>>> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
>>>>>> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
>>>>>> - vi->rss.indirection_table = old_rss.indirection_table;
>>>>>> + old_rss_hdr = vi->rss_hdr;
>>>>>> + old_rss_trailer = vi->rss_trailer;
>>>>>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
>>>>>> + if (!vi->rss_hdr) {
>>>>>> + vi->rss_hdr = old_rss_hdr;
>>>>>> return -ENOMEM;
>>>>>> }
>>>>>>
>>>>>> + *vi->rss_hdr = *old_rss_hdr;
>>>>>> virtnet_rss_update_by_qpairs(vi, queue_pairs);
>>>>>>
>>>>>> if (!virtnet_commit_rss_command(vi)) {
>>>>>> /* restore ctrl_rss if commit_rss_command failed */
>>>>>> - rss_indirection_table_free(&vi->rss);
>>>>>> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
>>>>>> + kfree(vi->rss_hdr);
>>>>>> + vi->rss_hdr = old_rss_hdr;
>>>>>> + vi->rss_trailer = old_rss_trailer;
>>>>>>
>>>>>> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
>>>>>> queue_pairs);
>>>>>> return -EINVAL;
>>>>>> }
>>>>>> - rss_indirection_table_free(&old_rss);
>>>>>> + kfree(old_rss_hdr);
>>>>>> goto succ;
>>>>>> }
>>>>>>
>>>>>> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
>>>>>> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
>>>>>> {
>>>>>> struct net_device *dev = vi->dev;
>>>>>> - struct scatterlist sgs[4];
>>>>>> - unsigned int sg_buf_size;
>>>>>> + struct scatterlist sgs[2];
>>>>>>
>>>>>> /* prepare sgs */
>>>>>> - sg_init_table(sgs, 4);
>>>>>> -
>>>>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
>>>>>> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
>>>>>> -
>>>>>> - if (vi->has_rss) {
>>>>>> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
>>>>>> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
>>>>>> - } else {
>>>>>> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
>>>>>> - }
>>>>>> -
>>>>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
>>>>>> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
>>>>>> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
>>>>>> -
>>>>>> - sg_buf_size = vi->rss_key_size;
>>>>>> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
>>>>>> + sg_init_table(sgs, 2);
>>>>>> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
>>>>>> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
>>>>>
>>>>> So I still see this:
>>>>>
>>>>> if (vi->has_rss || vi->has_rss_hash_report) {
>>>>> if (!virtnet_commit_rss_command(vi)) {
>>>>>
>>>>> Should we introduce a hash config helper instead?
>>>>
>>>> I think it's fine to use virtnet_commit_rss_command() for hash
>>>> reporting. struct virtio_net_hash_config and struct
>>>> virtio_net_rss_config are defined to have a common layout to allow
>>>> sharing this kind of logic.
>>>
>>> Well, this trick won't work if the reserved field in hash_config is
>>> used in the future.
>>
>> Right, but we can add a hash config helper when that happens. It will
>> only result in a duplication of logic for now.
>>
>> Regards,
>> Akihiko Odaki
>
> That's tricky as the cvq commands were designed to be used separately.
> Let's use a separate helper and virtio_net_hash_config uAPIs now.
It's not tricky but is explicitly stated in the spec. 5.1.6.5.6.4 "Hash
calculation" says:
> Field reserved MUST contain zeroes. It is defined to make the
> structure to match the layout of virtio_net_rss_config structure,
> defined in 5.1.6.5.7.
By the way, I found it says field reserved MUST contain zeros but we do
nothing to ensure that. I'll write a fix for that.
Regards,
Akihiko Odaki
>
> Thanks
>
>>
>>>
>>> Thanks
>>>
>>>>
>>>> Regards,
>>>> Akihiko Odaki
>>>>
>>>>>
>>>>> Thanks
>>>>>
>>>>
>>>
>>
>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH net-next 3/4] virtio_net: Use new RSS config structs
2025-03-21 6:34 ` Akihiko Odaki
@ 2025-03-24 4:05 ` Jason Wang
0 siblings, 0 replies; 19+ messages in thread
From: Jason Wang @ 2025-03-24 4:05 UTC (permalink / raw)
To: Akihiko Odaki
Cc: Michael S. Tsirkin, Xuan Zhuo, Eugenio Pérez, Andrew Lunn,
David S. Miller, Eric Dumazet, Jakub Kicinski, Paolo Abeni,
Andrew Melnychenko, Joe Damato, Philo Lu, virtualization,
linux-kernel, netdev, devel
On Fri, Mar 21, 2025 at 2:35 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
>
> On 2025/03/21 9:35, Jason Wang wrote:
> > On Thu, Mar 20, 2025 at 1:36 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>
> >> On 2025/03/20 10:50, Jason Wang wrote:
> >>> On Wed, Mar 19, 2025 at 12:48 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>
> >>>> On 2025/03/19 10:43, Jason Wang wrote:
> >>>>> On Tue, Mar 18, 2025 at 5:57 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote:
> >>>>>>
> >>>>>> The new RSS configuration structures allow easily constructing data for
> >>>>>> VIRTIO_NET_CTRL_MQ_RSS_CONFIG as they strictly follow the order of data
> >>>>>> for the command.
> >>>>>>
> >>>>>> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com>
> >>>>>> ---
> >>>>>> drivers/net/virtio_net.c | 117 +++++++++++++++++------------------------------
> >>>>>> 1 file changed, 43 insertions(+), 74 deletions(-)
> >>>>>>
> >>>>>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> >>>>>> index d1ed544ba03a..4153a0a5f278 100644
> >>>>>> --- a/drivers/net/virtio_net.c
> >>>>>> +++ b/drivers/net/virtio_net.c
> >>>>>> @@ -360,24 +360,7 @@ struct receive_queue {
> >>>>>> struct xdp_buff **xsk_buffs;
> >>>>>> };
> >>>>>>
> >>>>>> -/* This structure can contain rss message with maximum settings for indirection table and keysize
> >>>>>> - * Note, that default structure that describes RSS configuration virtio_net_rss_config
> >>>>>> - * contains same info but can't handle table values.
> >>>>>> - * In any case, structure would be passed to virtio hw through sg_buf split by parts
> >>>>>> - * because table sizes may be differ according to the device configuration.
> >>>>>> - */
> >>>>>> #define VIRTIO_NET_RSS_MAX_KEY_SIZE 40
> >>>>>> -struct virtio_net_ctrl_rss {
> >>>>>> - __le32 hash_types;
> >>>>>> - __le16 indirection_table_mask;
> >>>>>> - __le16 unclassified_queue;
> >>>>>> - __le16 hash_cfg_reserved; /* for HASH_CONFIG (see virtio_net_hash_config for details) */
> >>>>>> - __le16 max_tx_vq;
> >>>>>> - u8 hash_key_length;
> >>>>>> - u8 key[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >>>>>> -
> >>>>>> - __le16 *indirection_table;
> >>>>>> -};
> >>>>>>
> >>>>>> /* Control VQ buffers: protected by the rtnl lock */
> >>>>>> struct control_buf {
> >>>>>> @@ -421,7 +404,9 @@ struct virtnet_info {
> >>>>>> u16 rss_indir_table_size;
> >>>>>> u32 rss_hash_types_supported;
> >>>>>> u32 rss_hash_types_saved;
> >>>>>> - struct virtio_net_ctrl_rss rss;
> >>>>>> + struct virtio_net_rss_config_hdr *rss_hdr;
> >>>>>> + struct virtio_net_rss_config_trailer rss_trailer;
> >>>>>> + u8 rss_hash_key_data[VIRTIO_NET_RSS_MAX_KEY_SIZE];
> >>>>>>
> >>>>>> /* Has control virtqueue */
> >>>>>> bool has_cvq;
> >>>>>> @@ -523,23 +508,16 @@ enum virtnet_xmit_type {
> >>>>>> VIRTNET_XMIT_TYPE_XSK,
> >>>>>> };
> >>>>>>
> >>>>>> -static int rss_indirection_table_alloc(struct virtio_net_ctrl_rss *rss, u16 indir_table_size)
> >>>>>> +static size_t virtnet_rss_hdr_size(const struct virtnet_info *vi)
> >>>>>> {
> >>>>>> - if (!indir_table_size) {
> >>>>>> - rss->indirection_table = NULL;
> >>>>>> - return 0;
> >>>>>> - }
> >>>>>> + u16 indir_table_size = vi->has_rss ? vi->rss_indir_table_size : 1;
> >>>>>>
> >>>>>> - rss->indirection_table = kmalloc_array(indir_table_size, sizeof(u16), GFP_KERNEL);
> >>>>>> - if (!rss->indirection_table)
> >>>>>> - return -ENOMEM;
> >>>>>> -
> >>>>>> - return 0;
> >>>>>> + return struct_size(vi->rss_hdr, indirection_table, indir_table_size);
> >>>>>> }
> >>>>>>
> >>>>>> -static void rss_indirection_table_free(struct virtio_net_ctrl_rss *rss)
> >>>>>> +static size_t virtnet_rss_trailer_size(const struct virtnet_info *vi)
> >>>>>> {
> >>>>>> - kfree(rss->indirection_table);
> >>>>>> + return struct_size(&vi->rss_trailer, hash_key_data, vi->rss_key_size);
> >>>>>> }
> >>>>>>
> >>>>>> /* We use the last two bits of the pointer to distinguish the xmit type. */
> >>>>>> @@ -3576,15 +3554,16 @@ static void virtnet_rss_update_by_qpairs(struct virtnet_info *vi, u16 queue_pair
> >>>>>>
> >>>>>> for (; i < vi->rss_indir_table_size; ++i) {
> >>>>>> indir_val = ethtool_rxfh_indir_default(i, queue_pairs);
> >>>>>> - vi->rss.indirection_table[i] = cpu_to_le16(indir_val);
> >>>>>> + vi->rss_hdr->indirection_table[i] = cpu_to_le16(indir_val);
> >>>>>> }
> >>>>>> - vi->rss.max_tx_vq = cpu_to_le16(queue_pairs);
> >>>>>> + vi->rss_trailer.max_tx_vq = cpu_to_le16(queue_pairs);
> >>>>>> }
> >>>>>>
> >>>>>> static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> >>>>>> {
> >>>>>> struct virtio_net_ctrl_mq *mq __free(kfree) = NULL;
> >>>>>> - struct virtio_net_ctrl_rss old_rss;
> >>>>>> + struct virtio_net_rss_config_hdr *old_rss_hdr;
> >>>>>> + struct virtio_net_rss_config_trailer old_rss_trailer;
> >>>>>> struct net_device *dev = vi->dev;
> >>>>>> struct scatterlist sg;
> >>>>>>
> >>>>>> @@ -3599,24 +3578,28 @@ static int virtnet_set_queues(struct virtnet_info *vi, u16 queue_pairs)
> >>>>>> * update (VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET below) and return directly.
> >>>>>> */
> >>>>>> if (vi->has_rss && !netif_is_rxfh_configured(dev)) {
> >>>>>> - memcpy(&old_rss, &vi->rss, sizeof(old_rss));
> >>>>>> - if (rss_indirection_table_alloc(&vi->rss, vi->rss_indir_table_size)) {
> >>>>>> - vi->rss.indirection_table = old_rss.indirection_table;
> >>>>>> + old_rss_hdr = vi->rss_hdr;
> >>>>>> + old_rss_trailer = vi->rss_trailer;
> >>>>>> + vi->rss_hdr = kmalloc(virtnet_rss_hdr_size(vi), GFP_KERNEL);
> >>>>>> + if (!vi->rss_hdr) {
> >>>>>> + vi->rss_hdr = old_rss_hdr;
> >>>>>> return -ENOMEM;
> >>>>>> }
> >>>>>>
> >>>>>> + *vi->rss_hdr = *old_rss_hdr;
> >>>>>> virtnet_rss_update_by_qpairs(vi, queue_pairs);
> >>>>>>
> >>>>>> if (!virtnet_commit_rss_command(vi)) {
> >>>>>> /* restore ctrl_rss if commit_rss_command failed */
> >>>>>> - rss_indirection_table_free(&vi->rss);
> >>>>>> - memcpy(&vi->rss, &old_rss, sizeof(old_rss));
> >>>>>> + kfree(vi->rss_hdr);
> >>>>>> + vi->rss_hdr = old_rss_hdr;
> >>>>>> + vi->rss_trailer = old_rss_trailer;
> >>>>>>
> >>>>>> dev_warn(&dev->dev, "Fail to set num of queue pairs to %d, because committing RSS failed\n",
> >>>>>> queue_pairs);
> >>>>>> return -EINVAL;
> >>>>>> }
> >>>>>> - rss_indirection_table_free(&old_rss);
> >>>>>> + kfree(old_rss_hdr);
> >>>>>> goto succ;
> >>>>>> }
> >>>>>>
> >>>>>> @@ -4059,28 +4042,12 @@ static int virtnet_set_ringparam(struct net_device *dev,
> >>>>>> static bool virtnet_commit_rss_command(struct virtnet_info *vi)
> >>>>>> {
> >>>>>> struct net_device *dev = vi->dev;
> >>>>>> - struct scatterlist sgs[4];
> >>>>>> - unsigned int sg_buf_size;
> >>>>>> + struct scatterlist sgs[2];
> >>>>>>
> >>>>>> /* prepare sgs */
> >>>>>> - sg_init_table(sgs, 4);
> >>>>>> -
> >>>>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, hash_cfg_reserved);
> >>>>>> - sg_set_buf(&sgs[0], &vi->rss, sg_buf_size);
> >>>>>> -
> >>>>>> - if (vi->has_rss) {
> >>>>>> - sg_buf_size = sizeof(uint16_t) * vi->rss_indir_table_size;
> >>>>>> - sg_set_buf(&sgs[1], vi->rss.indirection_table, sg_buf_size);
> >>>>>> - } else {
> >>>>>> - sg_set_buf(&sgs[1], &vi->rss.hash_cfg_reserved, sizeof(uint16_t));
> >>>>>> - }
> >>>>>> -
> >>>>>> - sg_buf_size = offsetof(struct virtio_net_ctrl_rss, key)
> >>>>>> - - offsetof(struct virtio_net_ctrl_rss, max_tx_vq);
> >>>>>> - sg_set_buf(&sgs[2], &vi->rss.max_tx_vq, sg_buf_size);
> >>>>>> -
> >>>>>> - sg_buf_size = vi->rss_key_size;
> >>>>>> - sg_set_buf(&sgs[3], vi->rss.key, sg_buf_size);
> >>>>>> + sg_init_table(sgs, 2);
> >>>>>> + sg_set_buf(&sgs[0], vi->rss_hdr, virtnet_rss_hdr_size(vi));
> >>>>>> + sg_set_buf(&sgs[1], &vi->rss_trailer, virtnet_rss_trailer_size(vi));
> >>>>>
> >>>>> So I still see this:
> >>>>>
> >>>>> if (vi->has_rss || vi->has_rss_hash_report) {
> >>>>> if (!virtnet_commit_rss_command(vi)) {
> >>>>>
> >>>>> Should we introduce a hash config helper instead?
> >>>>
> >>>> I think it's fine to use virtnet_commit_rss_command() for hash
> >>>> reporting. struct virtio_net_hash_config and struct
> >>>> virtio_net_rss_config are defined to have a common layout to allow
> >>>> sharing this kind of logic.
> >>>
> >>> Well, this trick won't work if the reserved field in hash_config is
> >>> used in the future.
> >>
> >> Right, but we can add a hash config helper when that happens. It will
> >> only result in a duplication of logic for now.
> >>
> >> Regards,
> >> Akihiko Odaki
> >
> > That's tricky as the cvq commands were designed to be used separately.
> > Let's use a separate helper and virtio_net_hash_config uAPIs now.
>
> It's not tricky but is explicitly stated in the spec. 5.1.6.5.6.4 "Hash
> calculation" says:
> > Field reserved MUST contain zeroes. It is defined to make the
> > structure to match the layout of virtio_net_rss_config structure,
> > defined in 5.1.6.5.7.
This is kind of not elegant, but it's too late to fix.
Thanks
>
> By the way, I found it says field reserved MUST contain zeros but we do
> nothing to ensure that. I'll write a fix for that.
>
> Regards,
> Akihiko Odaki
>
> >
> > Thanks
> >
> >>
> >>>
> >>> Thanks
> >>>
> >>>>
> >>>> Regards,
> >>>> Akihiko Odaki
> >>>>
> >>>>>
> >>>>> Thanks
> >>>>>
> >>>>
> >>>
> >>
> >
>
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2025-03-24 4:05 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-03-18 9:56 [PATCH net-next 0/4] virtio_net: Fixes and improvements Akihiko Odaki
2025-03-18 9:56 ` [PATCH net-next 1/4] virtio_net: Split struct virtio_net_rss_config Akihiko Odaki
2025-03-19 1:42 ` Jason Wang
2025-03-18 9:56 ` [PATCH net-next 2/4] virtio_net: Fix endian with virtio_net_ctrl_rss Akihiko Odaki
2025-03-19 1:43 ` Jason Wang
2025-03-18 9:56 ` [PATCH net-next 3/4] virtio_net: Use new RSS config structs Akihiko Odaki
2025-03-19 1:22 ` Xuan Zhuo
2025-03-19 4:46 ` Akihiko Odaki
2025-03-19 1:43 ` Jason Wang
2025-03-19 4:48 ` Akihiko Odaki
2025-03-20 1:50 ` Jason Wang
2025-03-20 5:36 ` Akihiko Odaki
2025-03-21 0:35 ` Jason Wang
2025-03-21 6:34 ` Akihiko Odaki
2025-03-24 4:05 ` Jason Wang
2025-03-18 9:56 ` [PATCH net-next 4/4] virtio_net: Allocate rss_hdr with devres Akihiko Odaki
2025-03-19 1:43 ` Jason Wang
2025-03-19 8:17 ` [PATCH net-next 0/4] virtio_net: Fixes and improvements Lei Yang
2025-03-19 9:07 ` Michael S. Tsirkin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).