在 2024/3/28 下午10:37, Breno Leitao 写道:
On Wed, Mar 27, 2024 at 09:37:43AM +0800, Xuan Zhuo wrote:
On Tue, 26 Mar 2024 08:19:08 -0700, Breno Leitao <leitao@xxxxxxxxxx> wrote:
Do not set virtnet_info->rss_indir_table_size if RSS is not available
for the device.
Currently, rss_indir_table_size is set if either has_rss or
has_rss_hash_report is available, but, it should only be set if has_rss
is set.
On the virtnet_set_rxfh(), return an invalid command if the request has
indirection table set, but virtnet does not support RSS.
Suggested-by: Heng Qi <hengqi@xxxxxxxxxxxxxxxxx>
Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
---
drivers/net/virtio_net.c | 9 +++++++--
1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c22d1118a133..c640fdf28fc5 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3813,6 +3813,9 @@ static int virtnet_set_rxfh(struct net_device *dev,
rxfh->hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
+ if (rxfh->indir && !vi->has_rss)
+ return -EINVAL;
+
if (rxfh->indir) {
Put !vi->has_rss here?
I am not sure I understand the suggestion. Where do you suggest we have
!vi->has_rss?
If we got this far, we either have:
a) rxfh->indir set and vi->has_rss is also set
b) rxfh->indir not set. (vi->has_rss could be set or not).
This function does two tasks.
1. update indir table
2. update rss key
#1 only for has_rss
#2 for has_rss or has_rss_hash_report
So I would code:
bool update = false
if (rxfh->indir) {
if (!vi->has_rss)
return -EINVAL;
for (i = 0; i < vi->rss_indir_table_size; ++i)
vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];
update = true
}
if (rxfh->key) {
if (!vi->has_rss && !vi->has_rss_hash_report)
return -EINVAL;
memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);
update = true
}
if (update)
virtnet_commit_rss_command(vi);
Thanks.
This is a bit different from the previous proposal, but, I like this one
approach better. It makes the code easier to read.
How does the full patch looks like?
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index c22d1118a133..180f342f1898 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -3807,20 +3807,35 @@ static int virtnet_set_rxfh(struct net_device *dev,
struct netlink_ext_ack *extack)
{
struct virtnet_info *vi = netdev_priv(dev);
+ bool update = false;
int i;
+ if (!vi->has_rss && !vi->has_rss_hash_report)
+ return -EOPNOTSUPP;
+
I think this two modifications are no longer needed as they have been
embedded below.
if (rxfh->hfunc != ETH_RSS_HASH_NO_CHANGE &&
rxfh->hfunc != ETH_RSS_HASH_TOP)
return -EOPNOTSUPP;
if (rxfh->indir) {
+ if (!vi->has_rss)
+ return -EINVAL;
-EOPNOTSUPP seems better.
+
for (i = 0; i < vi->rss_indir_table_size; ++i)
vi->ctrl->rss.indirection_table[i] = rxfh->indir[i];
+ update = true;
}
- if (rxfh->key)
+
+ if (rxfh->key) {
+ if (!vi->has_rss && !vi->has_rss_hash_report)
+ return -EINVAL;
Same as above. return -EOPNOTSUPP.
Others look good.
Thanks.
+
memcpy(vi->ctrl->rss.key, rxfh->key, vi->rss_key_size);
+ update = true;
+ }
- virtnet_commit_rss_command(vi);
+ if (update)
+ virtnet_commit_rss_command(vi);
return 0;
}
@@ -4729,13 +4744,15 @@ static int virtnet_probe(struct virtio_device *vdev)
if (virtio_has_feature(vdev, VIRTIO_NET_F_HASH_REPORT))
vi->has_rss_hash_report = true;
- if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS))
+ if (virtio_has_feature(vdev, VIRTIO_NET_F_RSS)) {
vi->has_rss = true;
- if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_indir_table_size =
virtio_cread16(vdev, offsetof(struct virtio_net_config,
rss_max_indirection_table_length));
+ }
+
+ if (vi->has_rss || vi->has_rss_hash_report) {
vi->rss_key_size =
virtio_cread8(vdev, offsetof(struct virtio_net_config, rss_max_key_size));
