| Message ID | 20240403-rss-v9-16-c6d87e69d38b@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-net RSS/hash report fixes and improvements | expand |
On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > The peer buffer is qualified with const and not meant to be modified. IMHO, this buffer is not so 'const' (although the prototype states so), it is allocated in net.c btw, another procedure in this file also modifies the buffer (work_around_broken_dhclient) > It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without > virtio-net header support. Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without virtio-net header support? Where? > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > hw/net/virtio-net.c | 36 +++++++++++++++++------------------- > 1 file changed, 17 insertions(+), 19 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 2de073ce18fd..ff1884564d0d 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1823,16 +1823,9 @@ static uint8_t virtio_net_get_hash_type(bool hasip4, > return 0xff; > } > > -static void virtio_set_packet_hash(const uint8_t *buf, uint8_t report, > - uint32_t hash) > -{ > - struct virtio_net_hdr_v1_hash *hdr = (void *)buf; > - hdr->hash_value = hash; > - hdr->hash_report = report; > -} > - > static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf, > - size_t size) > + size_t size, > + struct virtio_net_hdr_v1_hash *hdr) > { > VirtIONet *n = qemu_get_nic_opaque(nc); > unsigned int index = nc->queue_index, new_index = index; > @@ -1863,7 +1856,8 @@ static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf, > n->rss_data.hash_types); > if (net_hash_type > NetPktRssIpV6UdpEx) { > if (n->rss_data.populate_hash) { > - virtio_set_packet_hash(buf, VIRTIO_NET_HASH_REPORT_NONE, 0); > + hdr->hash_value = VIRTIO_NET_HASH_REPORT_NONE; > + hdr->hash_report = 0; > } > return n->rss_data.redirect ? n->rss_data.default_queue : -1; > } > @@ -1871,7 +1865,8 @@ static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf, > hash = net_rx_pkt_calc_rss_hash(pkt, net_hash_type, n->rss_data.key); > > if (n->rss_data.populate_hash) { > - virtio_set_packet_hash(buf, reports[net_hash_type], hash); > + hdr->hash_value = hash; > + hdr->hash_report = reports[net_hash_type]; > } > > if (n->rss_data.redirect) { > @@ -1891,7 +1886,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE]; > size_t lens[VIRTQUEUE_MAX_SIZE]; > struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE]; > - struct virtio_net_hdr_mrg_rxbuf mhdr; > + struct virtio_net_hdr_v1_hash extra_hdr; > unsigned mhdr_cnt = 0; > size_t offset, i, guest_offset, j; > ssize_t err; > @@ -1901,7 +1896,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > } > > if (!no_rss && n->rss_data.enabled && n->rss_data.enabled_software_rss) { > - int index = virtio_net_process_rss(nc, buf, size); > + int index = virtio_net_process_rss(nc, buf, size, &extra_hdr); > if (index >= 0) { > NetClientState *nc2 = qemu_get_subqueue(n->nic, index); > return virtio_net_receive_rcu(nc2, buf, size, true); > @@ -1961,15 +1956,17 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > if (n->mergeable_rx_bufs) { > mhdr_cnt = iov_copy(mhdr_sg, ARRAY_SIZE(mhdr_sg), > sg, elem->in_num, > - offsetof(typeof(mhdr), num_buffers), > - sizeof(mhdr.num_buffers)); > + offsetof(typeof(extra_hdr), hdr.num_buffers), > + sizeof(extra_hdr.hdr.num_buffers)); > } > > receive_header(n, sg, elem->in_num, buf, size); > if (n->rss_data.populate_hash) { > - offset = sizeof(mhdr); > + offset = offsetof(typeof(extra_hdr), hash_value); > iov_from_buf(sg, elem->in_num, offset, > - buf + offset, n->host_hdr_len - sizeof(mhdr)); > + (char *)&extra_hdr + offset, > + sizeof(extra_hdr.hash_value) + > + sizeof(extra_hdr.hash_report)); > } > offset = n->host_hdr_len; > total += n->guest_hdr_len; > @@ -2015,10 +2012,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, > } > > if (mhdr_cnt) { > - virtio_stw_p(vdev, &mhdr.num_buffers, i); > + virtio_stw_p(vdev, &extra_hdr.hdr.num_buffers, i); > iov_from_buf(mhdr_sg, mhdr_cnt, > 0, > - &mhdr.num_buffers, sizeof mhdr.num_buffers); > + &extra_hdr.hdr.num_buffers, > + sizeof extra_hdr.hdr.num_buffers); > } > > for (j = 0; j < i; j++) { > > -- > 2.44.0 >
On 2024/04/08 7:09, Yuri Benditovich wrote: > On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> The peer buffer is qualified with const and not meant to be modified. > > IMHO, this buffer is not so 'const' (although the prototype states so), > it is allocated in net.c > btw, another procedure in this file also modifies the buffer > (work_around_broken_dhclient) Right but it has a FIXME comment. > >> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without >> virtio-net header support. > > Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT > for peers without > virtio-net header support? Where? No, but I meant that this patch fixes such a problem. Regards, Akihiko Odaki
On Mon, Apr 8, 2024 at 4:30 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/04/08 7:09, Yuri Benditovich wrote: > > On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> The peer buffer is qualified with const and not meant to be modified. > > > > IMHO, this buffer is not so 'const' (although the prototype states so), > > it is allocated in net.c > > btw, another procedure in this file also modifies the buffer > > (work_around_broken_dhclient) > > Right but it has a FIXME comment. > > > > >> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without > >> virtio-net header support. > > > > Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT > > for peers without > > virtio-net header support? Where? > > No, but I meant that this patch fixes such a problem. No, it does not. Such a problem does not exist in the master, the hash_report feature is silently dropped in such case: https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L816 > > Regards, > Akihiko Odaki
On 2024/04/08 16:40, Yuri Benditovich wrote: > On Mon, Apr 8, 2024 at 4:30 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2024/04/08 7:09, Yuri Benditovich wrote: >>> On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>> The peer buffer is qualified with const and not meant to be modified. >>> >>> IMHO, this buffer is not so 'const' (although the prototype states so), >>> it is allocated in net.c >>> btw, another procedure in this file also modifies the buffer >>> (work_around_broken_dhclient) >> >> Right but it has a FIXME comment. >> >>> >>>> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without >>>> virtio-net header support. >>> >>> Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT >>> for peers without >>> virtio-net header support? Where? >> >> No, but I meant that this patch fixes such a problem. > > No, it does not. Such a problem does not exist in the master, the > hash_report feature > is silently dropped in such case: > https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L816 Well, silently dropping VIRTIO_NET_F_HASH_REPORT is not different from preventing enabling VIRTIO_NET_F_HASH_REPORT, is it? Regards, Akihiko Odaki
On Mon, Apr 8, 2024 at 10:42 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/04/08 16:40, Yuri Benditovich wrote: > > On Mon, Apr 8, 2024 at 4:30 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> On 2024/04/08 7:09, Yuri Benditovich wrote: > >>> On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>> > >>>> The peer buffer is qualified with const and not meant to be modified. > >>> > >>> IMHO, this buffer is not so 'const' (although the prototype states so), > >>> it is allocated in net.c > >>> btw, another procedure in this file also modifies the buffer > >>> (work_around_broken_dhclient) > >> > >> Right but it has a FIXME comment. > >> > >>> > >>>> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without > >>>> virtio-net header support. > >>> > >>> Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT > >>> for peers without > >>> virtio-net header support? Where? > >> > >> No, but I meant that this patch fixes such a problem. > > > > No, it does not. Such a problem does not exist in the master, the > > hash_report feature > > is silently dropped in such case: > > https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L816 > > Well, silently dropping VIRTIO_NET_F_HASH_REPORT is not different from > preventing enabling VIRTIO_NET_F_HASH_REPORT, is it? > But how is your patch involved in it? Should this line be removed from the commit message? > Regards, > Akihiko Odaki
On 2024/04/08 16:54, Yuri Benditovich wrote: > On Mon, Apr 8, 2024 at 10:42 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2024/04/08 16:40, Yuri Benditovich wrote: >>> On Mon, Apr 8, 2024 at 4:30 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>> On 2024/04/08 7:09, Yuri Benditovich wrote: >>>>> On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>>>> >>>>>> The peer buffer is qualified with const and not meant to be modified. >>>>> >>>>> IMHO, this buffer is not so 'const' (although the prototype states so), >>>>> it is allocated in net.c >>>>> btw, another procedure in this file also modifies the buffer >>>>> (work_around_broken_dhclient) >>>> >>>> Right but it has a FIXME comment. >>>> >>>>> >>>>>> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without >>>>>> virtio-net header support. >>>>> >>>>> Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT >>>>> for peers without >>>>> virtio-net header support? Where? >>>> >>>> No, but I meant that this patch fixes such a problem. >>> >>> No, it does not. Such a problem does not exist in the master, the >>> hash_report feature >>> is silently dropped in such case: >>> https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L816 >> >> Well, silently dropping VIRTIO_NET_F_HASH_REPORT is not different from >> preventing enabling VIRTIO_NET_F_HASH_REPORT, is it? >> > But how is your patch involved in it? Should this line be removed from > the commit message? In the master, VIRTIO_NET_F_HASH_REPORT is silently dropped, but this patch will change to work without dropping it, which is worth to mention. Regards, Akihiko Odaki
On Mon, Apr 8, 2024 at 10:57 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > On 2024/04/08 16:54, Yuri Benditovich wrote: > > On Mon, Apr 8, 2024 at 10:42 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >> > >> On 2024/04/08 16:40, Yuri Benditovich wrote: > >>> On Mon, Apr 8, 2024 at 4:30 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>> > >>>> On 2024/04/08 7:09, Yuri Benditovich wrote: > >>>>> On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > >>>>>> > >>>>>> The peer buffer is qualified with const and not meant to be modified. > >>>>> > >>>>> IMHO, this buffer is not so 'const' (although the prototype states so), > >>>>> it is allocated in net.c > >>>>> btw, another procedure in this file also modifies the buffer > >>>>> (work_around_broken_dhclient) > >>>> > >>>> Right but it has a FIXME comment. > >>>> > >>>>> > >>>>>> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without > >>>>>> virtio-net header support. > >>>>> > >>>>> Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT > >>>>> for peers without > >>>>> virtio-net header support? Where? > >>>> > >>>> No, but I meant that this patch fixes such a problem. > >>> > >>> No, it does not. Such a problem does not exist in the master, the > >>> hash_report feature > >>> is silently dropped in such case: > >>> https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L816 > >> > >> Well, silently dropping VIRTIO_NET_F_HASH_REPORT is not different from > >> preventing enabling VIRTIO_NET_F_HASH_REPORT, is it? > >> > > But how is your patch involved in it? Should this line be removed from > > the commit message? > > In the master, VIRTIO_NET_F_HASH_REPORT is silently dropped, but this > patch will change to work without dropping it, which is worth to mention. After applying this series of patches the VIRTIO_NET_F_HASH_REPORT is dropped _the same way_ as in the master > > Regards, > Akihiko Odaki
On 2024/04/08 17:06, Yuri Benditovich wrote: > On Mon, Apr 8, 2024 at 10:57 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> On 2024/04/08 16:54, Yuri Benditovich wrote: >>> On Mon, Apr 8, 2024 at 10:42 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>> >>>> On 2024/04/08 16:40, Yuri Benditovich wrote: >>>>> On Mon, Apr 8, 2024 at 4:30 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>>>> >>>>>> On 2024/04/08 7:09, Yuri Benditovich wrote: >>>>>>> On Wed, Apr 3, 2024 at 2:12 PM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >>>>>>>> >>>>>>>> The peer buffer is qualified with const and not meant to be modified. >>>>>>> >>>>>>> IMHO, this buffer is not so 'const' (although the prototype states so), >>>>>>> it is allocated in net.c >>>>>>> btw, another procedure in this file also modifies the buffer >>>>>>> (work_around_broken_dhclient) >>>>>> >>>>>> Right but it has a FIXME comment. >>>>>> >>>>>>> >>>>>>>> It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without >>>>>>>> virtio-net header support. >>>>>>> >>>>>>> Does it mean _this commit_ prevents enabling VIRTIO_NET_F_HASH_REPORT >>>>>>> for peers without >>>>>>> virtio-net header support? Where? >>>>>> >>>>>> No, but I meant that this patch fixes such a problem. >>>>> >>>>> No, it does not. Such a problem does not exist in the master, the >>>>> hash_report feature >>>>> is silently dropped in such case: >>>>> https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L816 >>>> >>>> Well, silently dropping VIRTIO_NET_F_HASH_REPORT is not different from >>>> preventing enabling VIRTIO_NET_F_HASH_REPORT, is it? >>>> >>> But how is your patch involved in it? Should this line be removed from >>> the commit message? >> >> In the master, VIRTIO_NET_F_HASH_REPORT is silently dropped, but this >> patch will change to work without dropping it, which is worth to mention. > After applying this series of patches the VIRTIO_NET_F_HASH_REPORT is > dropped _the same way_ as in the master You are right. I forgot that I dropped patch "virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT" with v7. I'll drop the line in the next version accordingly. Thanks for pointing out that. Regards, Akihiko Odaki
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 2de073ce18fd..ff1884564d0d 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1823,16 +1823,9 @@ static uint8_t virtio_net_get_hash_type(bool hasip4, return 0xff; } -static void virtio_set_packet_hash(const uint8_t *buf, uint8_t report, - uint32_t hash) -{ - struct virtio_net_hdr_v1_hash *hdr = (void *)buf; - hdr->hash_value = hash; - hdr->hash_report = report; -} - static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf, - size_t size) + size_t size, + struct virtio_net_hdr_v1_hash *hdr) { VirtIONet *n = qemu_get_nic_opaque(nc); unsigned int index = nc->queue_index, new_index = index; @@ -1863,7 +1856,8 @@ static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf, n->rss_data.hash_types); if (net_hash_type > NetPktRssIpV6UdpEx) { if (n->rss_data.populate_hash) { - virtio_set_packet_hash(buf, VIRTIO_NET_HASH_REPORT_NONE, 0); + hdr->hash_value = VIRTIO_NET_HASH_REPORT_NONE; + hdr->hash_report = 0; } return n->rss_data.redirect ? n->rss_data.default_queue : -1; } @@ -1871,7 +1865,8 @@ static int virtio_net_process_rss(NetClientState *nc, const uint8_t *buf, hash = net_rx_pkt_calc_rss_hash(pkt, net_hash_type, n->rss_data.key); if (n->rss_data.populate_hash) { - virtio_set_packet_hash(buf, reports[net_hash_type], hash); + hdr->hash_value = hash; + hdr->hash_report = reports[net_hash_type]; } if (n->rss_data.redirect) { @@ -1891,7 +1886,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, VirtQueueElement *elems[VIRTQUEUE_MAX_SIZE]; size_t lens[VIRTQUEUE_MAX_SIZE]; struct iovec mhdr_sg[VIRTQUEUE_MAX_SIZE]; - struct virtio_net_hdr_mrg_rxbuf mhdr; + struct virtio_net_hdr_v1_hash extra_hdr; unsigned mhdr_cnt = 0; size_t offset, i, guest_offset, j; ssize_t err; @@ -1901,7 +1896,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, } if (!no_rss && n->rss_data.enabled && n->rss_data.enabled_software_rss) { - int index = virtio_net_process_rss(nc, buf, size); + int index = virtio_net_process_rss(nc, buf, size, &extra_hdr); if (index >= 0) { NetClientState *nc2 = qemu_get_subqueue(n->nic, index); return virtio_net_receive_rcu(nc2, buf, size, true); @@ -1961,15 +1956,17 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, if (n->mergeable_rx_bufs) { mhdr_cnt = iov_copy(mhdr_sg, ARRAY_SIZE(mhdr_sg), sg, elem->in_num, - offsetof(typeof(mhdr), num_buffers), - sizeof(mhdr.num_buffers)); + offsetof(typeof(extra_hdr), hdr.num_buffers), + sizeof(extra_hdr.hdr.num_buffers)); } receive_header(n, sg, elem->in_num, buf, size); if (n->rss_data.populate_hash) { - offset = sizeof(mhdr); + offset = offsetof(typeof(extra_hdr), hash_value); iov_from_buf(sg, elem->in_num, offset, - buf + offset, n->host_hdr_len - sizeof(mhdr)); + (char *)&extra_hdr + offset, + sizeof(extra_hdr.hash_value) + + sizeof(extra_hdr.hash_report)); } offset = n->host_hdr_len; total += n->guest_hdr_len; @@ -2015,10 +2012,11 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf, } if (mhdr_cnt) { - virtio_stw_p(vdev, &mhdr.num_buffers, i); + virtio_stw_p(vdev, &extra_hdr.hdr.num_buffers, i); iov_from_buf(mhdr_sg, mhdr_cnt, 0, - &mhdr.num_buffers, sizeof mhdr.num_buffers); + &extra_hdr.hdr.num_buffers, + sizeof extra_hdr.hdr.num_buffers); } for (j = 0; j < i; j++) {
The peer buffer is qualified with const and not meant to be modified. It also prevents enabling VIRTIO_NET_F_HASH_REPORT for peers without virtio-net header support. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- hw/net/virtio-net.c | 36 +++++++++++++++++------------------- 1 file changed, 17 insertions(+), 19 deletions(-)