Re: [PATCH net-next v9 6/6] vhost/net: Support VIRTIO_NET_F_HASH_REPORT
From: Akihiko Odaki
Date: Wed Mar 19 2025 - 00:44:14 EST
On 2025/03/17 10:15, Jason Wang wrote:
On Wed, Mar 12, 2025 at 1:59 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
On 2025/03/12 12:36, Jason Wang wrote:
On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
On 2025/03/11 9:42, Jason Wang wrote:
On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
On 2025/03/10 13:43, Jason Wang wrote:
On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki <akihiko.odaki@xxxxxxxxxx> wrote:
VIRTIO_NET_F_HASH_REPORT allows to report hash values calculated on the
host. When VHOST_NET_F_VIRTIO_NET_HDR is employed, it will report no
hash values (i.e., the hash_report member is always set to
VIRTIO_NET_HASH_REPORT_NONE). Otherwise, the values reported by the
underlying socket will be reported.
VIRTIO_NET_F_HASH_REPORT requires VIRTIO_F_VERSION_1.
Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx>
Tested-by: Lei Yang <leiyang@xxxxxxxxxx>
---
drivers/vhost/net.c | 49 +++++++++++++++++++++++++++++--------------------
1 file changed, 29 insertions(+), 20 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index b9b9e9d40951856d881d77ac74331d914473cd56..16b241b44f89820a42c302f3586ea6bb5e0d4289 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -73,6 +73,7 @@ enum {
VHOST_NET_FEATURES = VHOST_FEATURES |
(1ULL << VHOST_NET_F_VIRTIO_NET_HDR) |
(1ULL << VIRTIO_NET_F_MRG_RXBUF) |
+ (1ULL << VIRTIO_NET_F_HASH_REPORT) |
(1ULL << VIRTIO_F_ACCESS_PLATFORM) |
(1ULL << VIRTIO_F_RING_RESET)
};
@@ -1097,9 +1098,11 @@ static void handle_rx(struct vhost_net *net)
.msg_controllen = 0,
.msg_flags = MSG_DONTWAIT,
};
- struct virtio_net_hdr hdr = {
- .flags = 0,
- .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ struct virtio_net_hdr_v1_hash hdr = {
+ .hdr = {
+ .flags = 0,
+ .gso_type = VIRTIO_NET_HDR_GSO_NONE
+ }
};
size_t total_len = 0;
int err, mergeable;
@@ -1110,7 +1113,6 @@ static void handle_rx(struct vhost_net *net)
bool set_num_buffers;
struct socket *sock;
struct iov_iter fixup;
- __virtio16 num_buffers;
int recv_pkts = 0;
mutex_lock_nested(&vq->mutex, VHOST_NET_VQ_RX);
@@ -1191,30 +1193,30 @@ static void handle_rx(struct vhost_net *net)
vhost_discard_vq_desc(vq, headcount);
continue;
}
+ hdr.hdr.num_buffers = cpu_to_vhost16(vq, headcount);
/* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
if (unlikely(vhost_hlen)) {
- if (copy_to_iter(&hdr, sizeof(hdr),
- &fixup) != sizeof(hdr)) {
+ if (copy_to_iter(&hdr, vhost_hlen,
+ &fixup) != vhost_hlen) {
vq_err(vq, "Unable to write vnet_hdr "
"at addr %p\n", vq->iov->iov_base);
goto out;
Is this an "issue" specific to RSS/HASH? If it's not, we need a separate patch.
Honestly, I'm not sure if it's too late to fix this.
There is nothing wrong with the current implementation.
Note that I meant the vhost_hlen part, and the current code is tricky.
The comment said:
"""
/* Supply virtio_net_hdr if VHOST_NET_F_VIRTIO_NET_HDR */
"""
So it tries to only offer virtio_net_hdr even if vhost_hlen is the set
to mrg_rxbuf len.
And this patch changes this behaviour.
mrg_rxbuf only adds the num_buffers field, which is always set for
mrg_rxbuf.
The num_buffers was not set for VIRTIO_F_VERSION_1 in the past, but this
was also fixed with commit a3b9c053d82a ("vhost/net: Set num_buffers for
virtio 1.0")
So there is no behavioral change for existing features with this patch.
I meant this part.
+ if (copy_to_iter(&hdr, vhost_hlen,
+ &fixup) != vhost_hlen) {
We should copy only sizeof(hdr) instead of vhost_hlen.> > Anything I miss?
sizeof(hdr) will be greater than vhost_hlen when neither
VIRTIO_NET_F_MRG_RXBUF or VIRTIO_F_VERSION_1 is negotiated.
Just to make sure we are on the same page I meant we should only
advance sizeof(struct virtio_net_hdr) here to make sure we can set the
num_buffers correctly.
But this is not what the code did here.
This code wrote num_buffers in hdr instead of setting it later.
However, with v10, I changed it again to fill the whole header with zero
and to set num_buffers later. The end result is identical either way;
num_buffers has the correct value and the other fields are filled with zero.
Regards,
Akihiko Odaki
Thanks
Regards,
Akihiko Odaki
Thanks
Regards,
Akihiko Odaki
Thanks
The current
implementation fills the header with zero except num_buffers, which it
fills some real value. This functionality is working fine with
VIRTIO_NET_F_MRG_RXBUF and VIRTIO_F_VERSION_1, which change the header size.
Now I'm adding VIRTIO_NET_F_HASH_REPORT and it adds the hash_report
field, which also needs to be initialized with zero, so I'm making sure
vhost_net will also initialize it.
Regards,
Akihiko Odaki
Others look fine.
Thanks