Messages in this thread | | | Date | Fri, 29 Mar 2024 10:10:10 +0800 | Subject | Re: [PATCH net v2 1/2] virtio_net: Do not set rss_indir if RSS is not supported | From | Heng Qi <> |
| |
在 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@debian.org> 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@linux.alibaba.com> >>>>> Signed-off-by: Breno Leitao <leitao@debian.org> >>>>> --- >>>>> 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)); >
| |