On 2025/03/17 10:15, Jason Wang wrote:
On Wed, Mar 12, 2025 at 1:59 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:
On 2025/03/12 12:36, Jason Wang wrote:
On Tue, Mar 11, 2025 at 2:24 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:
On 2025/03/11 9:42, Jason Wang wrote:
On Mon, Mar 10, 2025 at 3:04 PM Akihiko Odaki akihiko.odaki@daynix.com wrote:
On 2025/03/10 13:43, Jason Wang wrote: > On Fri, Mar 7, 2025 at 7:02 PM Akihiko Odaki akihiko.odaki@daynix.com 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@daynix.com >> Tested-by: Lei Yang leiyang@redhat.com >> --- >> 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 >