* [PATCH] virtio_net: roll back RSS state on control failure
@ 2026-08-04 7:36 Xiong Weimin
2026-08-08 1:19 ` Jakub Kicinski
0 siblings, 1 reply; 3+ messages in thread
From: Xiong Weimin @ 2026-08-04 7:36 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
Cc: netdev, virtualization, linux-kernel, Xiong Weimin
The ethtool RSS and RXHASH paths update the driver's cached RSS state
before committing the change to the device. If the control virtqueue
command fails, the cached hash types, key or indirection table can then
report a configuration that the device did not accept.
Preserve the previous local state around RSS/hash control commands and
restore it when the device update fails, while propagating the error to
the caller.
Signed-off-by: Xiong Weimin <xiongweimin@kylinos.cn>
---
drivers/net/virtio_net.c | 54 ++++++++++++++++++++++++++++++----------
1 file changed, 41 insertions(+), 13 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 3e2a5876c..ccd96315a 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
struct netlink_ext_ack *extack)
{
struct virtnet_info *vi = netdev_priv(dev);
+ u32 old_hashtypes = vi->rss_hash_types_saved;
u32 new_hashtypes = vi->rss_hash_types_saved;
bool is_disable = info->data & RXH_DISCARD;
bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
@@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
if (new_hashtypes != vi->rss_hash_types_saved) {
vi->rss_hash_types_saved = new_hashtypes;
vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
- if (vi->dev->features & NETIF_F_RXHASH)
- if (!virtnet_commit_rss_command(vi))
+ if (vi->dev->features & NETIF_F_RXHASH) {
+ if (!virtnet_commit_rss_command(vi)) {
+ vi->rss_hash_types_saved = old_hashtypes;
+ vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
return -EINVAL;
+ }
+ }
}
return 0;
@@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
struct netlink_ext_ack *extack)
{
struct virtnet_info *vi = netdev_priv(dev);
+ struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
+ u8 old_rss_key[NETDEV_RSS_KEY_LEN];
bool update = false;
int i;
@@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
rxfh->hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
- if (rxfh->indir) {
- if (!vi->has_rss)
- return -EOPNOTSUPP;
-
- for (i = 0; i < vi->rss_indir_table_size; ++i)
- vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
- update = true;
- }
+ if (rxfh->indir && !vi->has_rss)
+ return -EOPNOTSUPP;
if (rxfh->key) {
/* If either _F_HASH_REPORT or _F_RSS are negotiated, the
@@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
*/
if (!vi->has_rss && !vi->has_rss_hash_report)
return -EOPNOTSUPP;
+ }
+
+ if (rxfh->indir) {
+ old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
+ GFP_KERNEL);
+ if (!old_rss_hdr)
+ return -ENOMEM;
+
+ for (i = 0; i < vi->rss_indir_table_size; ++i)
+ vi->rss_hdr->indirection_table[i] =
+ cpu_to_le16(rxfh->indir[i]);
+ update = true;
+ }
+ if (rxfh->key) {
+ memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
update = true;
}
- if (update)
- virtnet_commit_rss_command(vi);
+ if (update && !virtnet_commit_rss_command(vi)) {
+ if (old_rss_hdr)
+ memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
+ if (rxfh->key)
+ memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
+ kfree(old_rss_hdr);
+ return -EINVAL;
+ }
+
+ kfree(old_rss_hdr);
return 0;
}
@@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
}
if ((dev->features ^ features) & NETIF_F_RXHASH) {
+ __le32 hash_types = vi->rss_hdr->hash_types;
+
if (features & NETIF_F_RXHASH)
vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
else
vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
- if (!virtnet_commit_rss_command(vi))
+ if (!virtnet_commit_rss_command(vi)) {
+ vi->rss_hdr->hash_types = hash_types;
return -EINVAL;
+ }
}
return 0;
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] virtio_net: roll back RSS state on control failure
2026-08-04 7:36 [PATCH] virtio_net: roll back RSS state on control failure Xiong Weimin
@ 2026-08-08 1:19 ` Jakub Kicinski
2026-08-08 8:32 ` Michael S. Tsirkin
0 siblings, 1 reply; 3+ messages in thread
From: Jakub Kicinski @ 2026-08-08 1:19 UTC (permalink / raw)
To: Xiong Weimin
Cc: Michael S. Tsirkin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni, netdev,
virtualization, linux-kernel
On Tue, 4 Aug 2026 15:36:54 +0800 Xiong Weimin wrote:
> The ethtool RSS and RXHASH paths update the driver's cached RSS state
> before committing the change to the device. If the control virtqueue
> command fails, the cached hash types, key or indirection table can then
> report a configuration that the device did not accept.
>
> Preserve the previous local state around RSS/hash control commands and
> restore it when the device update fails, while propagating the error to
> the caller.
I guess.. saving the data and then copying it back doesn't seem super
clean but I guess since we DMA directly from the info struct..
Michael, Jason, looks okay?
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 3e2a5876c..ccd96315a 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
> struct netlink_ext_ack *extack)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> + u32 old_hashtypes = vi->rss_hash_types_saved;
> u32 new_hashtypes = vi->rss_hash_types_saved;
> bool is_disable = info->data & RXH_DISCARD;
> bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
> @@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
> if (new_hashtypes != vi->rss_hash_types_saved) {
> vi->rss_hash_types_saved = new_hashtypes;
> vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> - if (vi->dev->features & NETIF_F_RXHASH)
> - if (!virtnet_commit_rss_command(vi))
> + if (vi->dev->features & NETIF_F_RXHASH) {
> + if (!virtnet_commit_rss_command(vi)) {
> + vi->rss_hash_types_saved = old_hashtypes;
> + vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
> return -EINVAL;
> + }
> + }
> }
>
> return 0;
> @@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
> struct netlink_ext_ack *extack)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> + struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
> + u8 old_rss_key[NETDEV_RSS_KEY_LEN];
> bool update = false;
> int i;
>
> @@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
> rxfh->hfunc != ETH_RSS_HASH_TOP)
> return -EOPNOTSUPP;
>
> - if (rxfh->indir) {
> - if (!vi->has_rss)
> - return -EOPNOTSUPP;
> -
> - for (i = 0; i < vi->rss_indir_table_size; ++i)
> - vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
> - update = true;
> - }
> + if (rxfh->indir && !vi->has_rss)
> + return -EOPNOTSUPP;
>
> if (rxfh->key) {
> /* If either _F_HASH_REPORT or _F_RSS are negotiated, the
> @@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
> */
> if (!vi->has_rss && !vi->has_rss_hash_report)
> return -EOPNOTSUPP;
> + }
> +
> + if (rxfh->indir) {
> + old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
> + GFP_KERNEL);
> + if (!old_rss_hdr)
> + return -ENOMEM;
> +
> + for (i = 0; i < vi->rss_indir_table_size; ++i)
> + vi->rss_hdr->indirection_table[i] =
> + cpu_to_le16(rxfh->indir[i]);
> + update = true;
> + }
>
> + if (rxfh->key) {
> + memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
> memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
> update = true;
> }
>
> - if (update)
> - virtnet_commit_rss_command(vi);
> + if (update && !virtnet_commit_rss_command(vi)) {
> + if (old_rss_hdr)
> + memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
> + if (rxfh->key)
> + memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
> + kfree(old_rss_hdr);
> + return -EINVAL;
> + }
> +
> + kfree(old_rss_hdr);
>
> return 0;
> }
> @@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
> }
>
> if ((dev->features ^ features) & NETIF_F_RXHASH) {
> + __le32 hash_types = vi->rss_hdr->hash_types;
> +
> if (features & NETIF_F_RXHASH)
> vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> else
> vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
>
> - if (!virtnet_commit_rss_command(vi))
> + if (!virtnet_commit_rss_command(vi)) {
> + vi->rss_hdr->hash_types = hash_types;
> return -EINVAL;
> + }
> }
>
> return 0;
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] virtio_net: roll back RSS state on control failure
2026-08-08 1:19 ` Jakub Kicinski
@ 2026-08-08 8:32 ` Michael S. Tsirkin
0 siblings, 0 replies; 3+ messages in thread
From: Michael S. Tsirkin @ 2026-08-08 8:32 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Xiong Weimin, Jason Wang, Xuan Zhuo, Eugenio Pérez,
Andrew Lunn, David S. Miller, Eric Dumazet, Paolo Abeni, netdev,
virtualization, linux-kernel
On Fri, Aug 07, 2026 at 06:19:16PM -0700, Jakub Kicinski wrote:
> On Tue, 4 Aug 2026 15:36:54 +0800 Xiong Weimin wrote:
> > The ethtool RSS and RXHASH paths update the driver's cached RSS state
> > before committing the change to the device. If the control virtqueue
> > command fails, the cached hash types, key or indirection table can then
> > report a configuration that the device did not accept.
> >
> > Preserve the previous local state around RSS/hash control commands and
> > restore it when the device update fails, while propagating the error to
> > the caller.
>
> I guess.. saving the data and then copying it back doesn't seem super
> clean but I guess since we DMA directly from the info struct..
>
> Michael, Jason, looks okay?
>
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 3e2a5876c..ccd96315a 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -4295,6 +4295,7 @@ static int virtnet_set_hashflow(struct net_device *dev,
> > struct netlink_ext_ack *extack)
> > {
> > struct virtnet_info *vi = netdev_priv(dev);
> > + u32 old_hashtypes = vi->rss_hash_types_saved;
> > u32 new_hashtypes = vi->rss_hash_types_saved;
> > bool is_disable = info->data & RXH_DISCARD;
> > bool is_l4 = info->data == (RXH_IP_SRC | RXH_IP_DST | RXH_L4_B_0_1 | RXH_L4_B_2_3);
> > @@ -4350,9 +4351,13 @@ static int virtnet_set_hashflow(struct net_device *dev,
> > if (new_hashtypes != vi->rss_hash_types_saved) {
> > vi->rss_hash_types_saved = new_hashtypes;
> > vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> > - if (vi->dev->features & NETIF_F_RXHASH)
> > - if (!virtnet_commit_rss_command(vi))
> > + if (vi->dev->features & NETIF_F_RXHASH) {
> > + if (!virtnet_commit_rss_command(vi)) {
> > + vi->rss_hash_types_saved = old_hashtypes;
> > + vi->rss_hdr->hash_types = cpu_to_le32(old_hashtypes);
> > return -EINVAL;
> > + }
> > + }
> > }
> >
> > return 0;
> > @@ -5546,6 +5551,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
> > struct netlink_ext_ack *extack)
> > {
> > struct virtnet_info *vi = netdev_priv(dev);
> > + struct virtio_net_rss_config_hdr *old_rss_hdr = NULL;
> > + u8 old_rss_key[NETDEV_RSS_KEY_LEN];
> > bool update = false;
> > int i;
> >
> > @@ -5553,14 +5560,8 @@ static int virtnet_set_rxfh(struct net_device *dev,
> > rxfh->hfunc != ETH_RSS_HASH_TOP)
> > return -EOPNOTSUPP;
> >
> > - if (rxfh->indir) {
> > - if (!vi->has_rss)
> > - return -EOPNOTSUPP;
> > -
> > - for (i = 0; i < vi->rss_indir_table_size; ++i)
> > - vi->rss_hdr->indirection_table[i] = cpu_to_le16(rxfh->indir[i]);
> > - update = true;
> > - }
> > + if (rxfh->indir && !vi->has_rss)
> > + return -EOPNOTSUPP;
> >
> > if (rxfh->key) {
> > /* If either _F_HASH_REPORT or _F_RSS are negotiated, the
> > @@ -5569,13 +5570,36 @@ static int virtnet_set_rxfh(struct net_device *dev,
> > */
> > if (!vi->has_rss && !vi->has_rss_hash_report)
> > return -EOPNOTSUPP;
> > + }
> > +
> > + if (rxfh->indir) {
> > + old_rss_hdr = kmemdup(vi->rss_hdr, virtnet_rss_hdr_size(vi),
> > + GFP_KERNEL);
kvmemdup maybe, just in case - I think it can get as high as 128k after all.
> > + if (!old_rss_hdr)
> > + return -ENOMEM;
> > +
> > + for (i = 0; i < vi->rss_indir_table_size; ++i)
> > + vi->rss_hdr->indirection_table[i] =
> > + cpu_to_le16(rxfh->indir[i]);
> > + update = true;
> > + }
> >
> > + if (rxfh->key) {
> > + memcpy(old_rss_key, vi->rss_hash_key_data, vi->rss_key_size);
> > memcpy(vi->rss_hash_key_data, rxfh->key, vi->rss_key_size);
> > update = true;
> > }
> >
> > - if (update)
> > - virtnet_commit_rss_command(vi);
> > + if (update && !virtnet_commit_rss_command(vi)) {
> > + if (old_rss_hdr)
> > + memcpy(vi->rss_hdr, old_rss_hdr, virtnet_rss_hdr_size(vi));
> > + if (rxfh->key)
> > + memcpy(vi->rss_hash_key_data, old_rss_key, vi->rss_key_size);
> > + kfree(old_rss_hdr);
> > + return -EINVAL;
> > + }
> > +
> > + kfree(old_rss_hdr);
> >
> > return 0;
> > }
> > @@ -6171,13 +6195,17 @@ static int virtnet_set_features(struct net_device *dev,
> > }
> >
> > if ((dev->features ^ features) & NETIF_F_RXHASH) {
> > + __le32 hash_types = vi->rss_hdr->hash_types;
> > +
> > if (features & NETIF_F_RXHASH)
> > vi->rss_hdr->hash_types = cpu_to_le32(vi->rss_hash_types_saved);
> > else
> > vi->rss_hdr->hash_types = cpu_to_le32(VIRTIO_NET_HASH_REPORT_NONE);
> >
> > - if (!virtnet_commit_rss_command(vi))
> > + if (!virtnet_commit_rss_command(vi)) {
> > + vi->rss_hdr->hash_types = hash_types;
> > return -EINVAL;
> > + }
> > }
> >
> > return 0;
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-08-08 8:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-04 7:36 [PATCH] virtio_net: roll back RSS state on control failure Xiong Weimin
2026-08-08 1:19 ` Jakub Kicinski
2026-08-08 8:32 ` 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