| Message ID | 20250321-vq-v1-1-6d6d285e5cbc@daynix.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-net: Fix the interpretation of max_tx_vq | expand |
On Fri, Mar 21, 2025 at 11:56 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: > > virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to > determine the number of queue pairs and emits an error message saying > "Can't get queue_pairs". However, the field tells only about tx. > > Examine the indirection table to determine the number of queues required > for rx, and correct the name of field in the error message, clarifying > its correct semantics. > > Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> > --- > hw/net/virtio-net.c | 25 +++++++++++++++---------- > 1 file changed, 15 insertions(+), 10 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index de87cfadffe1..d9ab9e1eb74d 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1450,23 +1450,28 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, > err_value = (uint32_t)s; > goto error; > } > - for (i = 0; i < n->rss_data.indirections_len; ++i) { > - uint16_t val = n->rss_data.indirections_table[i]; > - n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val); > - } > offset += size_get; > size_get = sizeof(temp); > s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get); > if (s != size_get) { > - err_msg = "Can't get queue_pairs"; > + err_msg = "Can't get max_tx_vq"; > err_value = (uint32_t)s; > goto error; > } > - queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs; > - if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) { > - err_msg = "Invalid number of queue_pairs"; > - err_value = queue_pairs; > - goto error; > + if (do_rss) { > + queue_pairs = virtio_lduw_p(vdev, &temp.us); > + for (i = 0; i < n->rss_data.indirections_len; ++i) { > + uint16_t val = n->rss_data.indirections_table[i]; > + n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val); > + queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]); > + } I think this change will create a problem rather than improve something. curr_queue_pairs is used in virtio_net_can_receive. Let's say the device has 4 queues and 4 CPUs. If the OS currently configures the RSS for first 2 queues only the curr_queue_pairs becomes 2. The packets that come to queues 2 and 3 will be dropped, see virtio_net_receive_rcu and virtio_net_can_receive IMO the curr_queue_pairs should not be changed after it was set by VIRTIO_NET_CTRL_MQ / VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET > + if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) { > + err_msg = "Invalid number of queue_pairs"; > + err_value = queue_pairs; > + goto error; > + } > + } else { > + queue_pairs = n->curr_queue_pairs; > } > if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) { > err_msg = "Invalid key size"; > > --- > base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 > change-id: 20250321-vq-87aff4f531bf > > Best regards, > -- > Akihiko Odaki <akihiko.odaki@daynix.com> >
On 2025/03/21 19:44, Yuri Benditovich wrote: > On Fri, Mar 21, 2025 at 11:56 AM Akihiko Odaki <akihiko.odaki@daynix.com> wrote: >> >> virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to >> determine the number of queue pairs and emits an error message saying >> "Can't get queue_pairs". However, the field tells only about tx. >> >> Examine the indirection table to determine the number of queues required >> for rx, and correct the name of field in the error message, clarifying >> its correct semantics. >> >> Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> >> --- >> hw/net/virtio-net.c | 25 +++++++++++++++---------- >> 1 file changed, 15 insertions(+), 10 deletions(-) >> >> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >> index de87cfadffe1..d9ab9e1eb74d 100644 >> --- a/hw/net/virtio-net.c >> +++ b/hw/net/virtio-net.c >> @@ -1450,23 +1450,28 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, >> err_value = (uint32_t)s; >> goto error; >> } >> - for (i = 0; i < n->rss_data.indirections_len; ++i) { >> - uint16_t val = n->rss_data.indirections_table[i]; >> - n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val); >> - } >> offset += size_get; >> size_get = sizeof(temp); >> s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get); >> if (s != size_get) { >> - err_msg = "Can't get queue_pairs"; >> + err_msg = "Can't get max_tx_vq"; >> err_value = (uint32_t)s; >> goto error; >> } >> - queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs; >> - if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) { >> - err_msg = "Invalid number of queue_pairs"; >> - err_value = queue_pairs; >> - goto error; >> + if (do_rss) { >> + queue_pairs = virtio_lduw_p(vdev, &temp.us); >> + for (i = 0; i < n->rss_data.indirections_len; ++i) { >> + uint16_t val = n->rss_data.indirections_table[i]; >> + n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val); >> + queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]); >> + } > > I think this change will create a problem rather than improve something. > curr_queue_pairs is used in virtio_net_can_receive. > Let's say the device has 4 queues and 4 CPUs. > If the OS currently configures the RSS for first 2 queues only the > curr_queue_pairs becomes 2. > The packets that come to queues 2 and 3 will be dropped, see > virtio_net_receive_rcu and virtio_net_can_receive > > IMO the curr_queue_pairs should not be changed after it was set by > VIRTIO_NET_CTRL_MQ / VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET VIRTIO_NET_CTRL_MQ_RSS_CONFIG is part of class VIRTIO_NET_CTRL_MQ and should be used for configurable receive steering while VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET should be used for automatic receive steering. 5.1.6.5.5 "Device operation in multiqueue mode" says: > The driver enables multiqueue by sending a command using class > VIRTIO_NET_CTRL_MQ. The command selects the mode of multiqueue > operation, as follows: > > #define VIRTIO_NET_CTRL_MQ 4 > #define VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET 0 (for automatic > receive steering) > #define VIRTIO_NET_CTRL_MQ_RSS_CONFIG 1 (for configurable > receive steering) > #define VIRTIO_NET_CTRL_MQ_HASH_CONFIG 2 (for configurable > hash calculation) > > If more than one multiqueue mode is negotiated, the resulting device > configuration is defined by the last command sent by the driver. VIRTIO_NET_CTRL_MQ_VQ_PAIRS_SET may not be even available if VIRTIO_NET_F_RSS is negotiated as VIRTIO_NET_F_RSS does not depend on VIRTIO_NET_F_MQ. Jason, as you are participating in a relevant discussion on LKML, I'd also like to ask you if you have anything to add. Regards, Akihiko Odaki > >> + if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) { >> + err_msg = "Invalid number of queue_pairs"; >> + err_value = queue_pairs; >> + goto error; >> + } >> + } else { >> + queue_pairs = n->curr_queue_pairs; >> } >> if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) { >> err_msg = "Invalid key size"; >> >> --- >> base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 >> change-id: 20250321-vq-87aff4f531bf >> >> Best regards, >> -- >> Akihiko Odaki <akihiko.odaki@daynix.com> >>
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index de87cfadffe1..d9ab9e1eb74d 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -1450,23 +1450,28 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n, err_value = (uint32_t)s; goto error; } - for (i = 0; i < n->rss_data.indirections_len; ++i) { - uint16_t val = n->rss_data.indirections_table[i]; - n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val); - } offset += size_get; size_get = sizeof(temp); s = iov_to_buf(iov, iov_cnt, offset, &temp, size_get); if (s != size_get) { - err_msg = "Can't get queue_pairs"; + err_msg = "Can't get max_tx_vq"; err_value = (uint32_t)s; goto error; } - queue_pairs = do_rss ? virtio_lduw_p(vdev, &temp.us) : n->curr_queue_pairs; - if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) { - err_msg = "Invalid number of queue_pairs"; - err_value = queue_pairs; - goto error; + if (do_rss) { + queue_pairs = virtio_lduw_p(vdev, &temp.us); + for (i = 0; i < n->rss_data.indirections_len; ++i) { + uint16_t val = n->rss_data.indirections_table[i]; + n->rss_data.indirections_table[i] = virtio_lduw_p(vdev, &val); + queue_pairs = MAX(queue_pairs, n->rss_data.indirections_table[i]); + } + if (queue_pairs == 0 || queue_pairs > n->max_queue_pairs) { + err_msg = "Invalid number of queue_pairs"; + err_value = queue_pairs; + goto error; + } + } else { + queue_pairs = n->curr_queue_pairs; } if (temp.b > VIRTIO_NET_RSS_MAX_KEY_SIZE) { err_msg = "Invalid key size";
virtio-net uses the max_tx_vq field of struct virtio_net_rss_config to determine the number of queue pairs and emits an error message saying "Can't get queue_pairs". However, the field tells only about tx. Examine the indirection table to determine the number of queues required for rx, and correct the name of field in the error message, clarifying its correct semantics. Signed-off-by: Akihiko Odaki <akihiko.odaki@daynix.com> --- hw/net/virtio-net.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) --- base-commit: 825b96dbcee23d134b691fc75618b59c5f53da32 change-id: 20250321-vq-87aff4f531bf Best regards,