| Message ID | 20230516115958.287305-3-ktraynor@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | netdev-dpdk: Apply device rx/tx descriptor limits. | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | fail | github build: failed |
On Tue, May 16, 2023 at 2:00 PM Kevin Traynor <ktraynor@redhat.com> wrote: > > By default OVS configures 2048 descriptors for tx and rx queues > on DPDK devices. It also allows the user to configure those values. > > If the values used are not acceptable to the device then queue setup > would fail. > > The device exposes it's max/min/alignment requirements and OVS > applies some limits also. Use these to ensure an acceptable value > is used for the number of descriptors on a device tx/rx. > > If the default or user value is not acceptable, adjust to a suitable > value and log. > > Reported-at: https://bugzilla.redhat.com/2119876 > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> Lgtm. Reviewed-by: David Marchand <david.marchand@redhat.com>
On Tue, May 16, 2023 at 12:59:58PM +0100, Kevin Traynor wrote: > By default OVS configures 2048 descriptors for tx and rx queues > on DPDK devices. It also allows the user to configure those values. > > If the values used are not acceptable to the device then queue setup > would fail. > > The device exposes it's max/min/alignment requirements and OVS > applies some limits also. Use these to ensure an acceptable value > is used for the number of descriptors on a device tx/rx. > > If the default or user value is not acceptable, adjust to a suitable > value and log. > > Reported-at: https://bugzilla.redhat.com/2119876 > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> Reviewed-by: Simon Horman <simon.horman@corigine.com>
On 5/16/23 13:59, Kevin Traynor wrote: > By default OVS configures 2048 descriptors for tx and rx queues > on DPDK devices. It also allows the user to configure those values. > > If the values used are not acceptable to the device then queue setup > would fail. > > The device exposes it's max/min/alignment requirements and OVS > applies some limits also. Use these to ensure an acceptable value > is used for the number of descriptors on a device tx/rx. > > If the default or user value is not acceptable, adjust to a suitable > value and log. > > Reported-at: https://bugzilla.redhat.com/2119876 > Signed-off-by: Kevin Traynor <ktraynor@redhat.com> > --- > lib/netdev-dpdk.c | 61 ++++++++++++++++++++++++++++++++++++++--------- > 1 file changed, 50 insertions(+), 11 deletions(-) > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c > index 2d9afc493..e5fc1aa30 100644 > --- a/lib/netdev-dpdk.c > +++ b/lib/netdev-dpdk.c > @@ -1911,8 +1911,30 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args) > static void > dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, > - const char *flag, int default_size, int *new_size) > + struct rte_eth_dev_info *info, bool is_rx) > { > - int queue_size = smap_get_int(args, flag, default_size); > + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); > + struct rte_eth_desc_lim *lim; > + int default_size, queue_size, cur_size, new_requested_size; > + int *cur_requested_size; > + bool reconfig = false; > > + if (is_rx) { > + default_size = NIC_PORT_DEFAULT_RXQ_SIZE; > + new_requested_size = smap_get_int(args, "n_rxq_desc", default_size); > + cur_size = dev->rxq_size; > + cur_requested_size = &dev->requested_rxq_size; > + lim = info ? &info->rx_desc_lim : NULL; > + The empty line here seems unnecessary. > + } else { > + default_size = NIC_PORT_DEFAULT_TXQ_SIZE; > + new_requested_size = smap_get_int(args, "n_txq_desc", default_size); > + cur_size = dev->txq_size; > + cur_requested_size = &dev->requested_txq_size; > + lim = info ? &info->tx_desc_lim : NULL; > + } > + > + queue_size = new_requested_size; > + > + /* Check for OVS limits. */ > if (queue_size <= 0 || queue_size > NIC_PORT_MAX_Q_SIZE > || !is_pow2(queue_size)) { > @@ -1920,7 +1942,24 @@ dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, > } > > - if (queue_size != *new_size) { > - *new_size = queue_size; > + if (lim) { > + /* Check for device limits. */ > + if (lim->nb_align) { > + queue_size = ROUND_UP(queue_size, lim->nb_align); > + } > + queue_size = MIN(queue_size, lim->nb_max); > + queue_size = MAX(queue_size, lim->nb_min); > + } > + > + *cur_requested_size = queue_size; > + > + if (cur_size != queue_size) { > netdev_request_reconfigure(netdev); > + reconfig = true; > + } > + if (new_requested_size != queue_size) { > + VLOG(reconfig ? VLL_INFO : VLL_DBG, > + "Interface %s cannot set %s descriptor size to %d. " > + "Adjusted to %d.", dev->up.name, is_rx ? "rx": "tx" , > + new_requested_size, queue_size); This message is a bit misleading. It says that it can't set the descriptor size. But it's not the descriptor size, it's a number of descriptors. Or a descriptor ring size, but that's too much of the unnecessary terminology. And we should use netdev_get_name instead of accessing the name directly. How about this: VLOG(reconfig ? VLL_INFO : VLL_DBG, "%s: Unable to set the number of %s descriptors to %d. " "Adjusted to %d.", netdev_get_name(netdev), is_rx ? "rx": "tx", new_requested_size, queue_size); ? > } > } > @@ -1938,7 +1977,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > {RTE_ETH_FC_RX_PAUSE, RTE_ETH_FC_FULL } > }; > + struct rte_eth_dev_info info; > const char *new_devargs; > const char *vf_mac; > int err = 0; > + int ret; > > ovs_mutex_lock(&dpdk_mutex); > @@ -1947,11 +1988,4 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > dpdk_set_rxq_config(dev, args); > > - dpdk_process_queue_size(netdev, args, "n_rxq_desc", > - NIC_PORT_DEFAULT_RXQ_SIZE, > - &dev->requested_rxq_size); > - dpdk_process_queue_size(netdev, args, "n_txq_desc", > - NIC_PORT_DEFAULT_TXQ_SIZE, > - &dev->requested_txq_size); > - > new_devargs = smap_get(args, "dpdk-devargs"); > > @@ -2073,4 +2107,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, > } > > + ret = rte_eth_dev_info_get(dev->port_id, &info); > + > + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, true); > + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, false); Any reason to move this after the flow control? If flow control is not supported we jump over this part of the code and not configuring the queue size. You can test that by adding a net_null device. Best regards, Ilya Maximets.
On 24/05/2023 19:15, Ilya Maximets wrote: > On 5/16/23 13:59, Kevin Traynor wrote: >> By default OVS configures 2048 descriptors for tx and rx queues >> on DPDK devices. It also allows the user to configure those values. >> >> If the values used are not acceptable to the device then queue setup >> would fail. >> >> The device exposes it's max/min/alignment requirements and OVS >> applies some limits also. Use these to ensure an acceptable value >> is used for the number of descriptors on a device tx/rx. >> >> If the default or user value is not acceptable, adjust to a suitable >> value and log. >> >> Reported-at: https://bugzilla.redhat.com/2119876 >> Signed-off-by: Kevin Traynor <ktraynor@redhat.com> >> --- >> lib/netdev-dpdk.c | 61 ++++++++++++++++++++++++++++++++++++++--------- >> 1 file changed, 50 insertions(+), 11 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index 2d9afc493..e5fc1aa30 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -1911,8 +1911,30 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args) >> static void >> dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, >> - const char *flag, int default_size, int *new_size) >> + struct rte_eth_dev_info *info, bool is_rx) >> { >> - int queue_size = smap_get_int(args, flag, default_size); >> + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); >> + struct rte_eth_desc_lim *lim; >> + int default_size, queue_size, cur_size, new_requested_size; >> + int *cur_requested_size; >> + bool reconfig = false; >> >> + if (is_rx) { >> + default_size = NIC_PORT_DEFAULT_RXQ_SIZE; >> + new_requested_size = smap_get_int(args, "n_rxq_desc", default_size); >> + cur_size = dev->rxq_size; >> + cur_requested_size = &dev->requested_rxq_size; >> + lim = info ? &info->rx_desc_lim : NULL; >> + > > The empty line here seems unnecessary. > yep, I think I removed an assignment at some stage and forgot to remove the blank line. >> + } else { >> + default_size = NIC_PORT_DEFAULT_TXQ_SIZE; >> + new_requested_size = smap_get_int(args, "n_txq_desc", default_size); >> + cur_size = dev->txq_size; >> + cur_requested_size = &dev->requested_txq_size; >> + lim = info ? &info->tx_desc_lim : NULL; >> + } >> + >> + queue_size = new_requested_size; >> + >> + /* Check for OVS limits. */ >> if (queue_size <= 0 || queue_size > NIC_PORT_MAX_Q_SIZE >> || !is_pow2(queue_size)) { >> @@ -1920,7 +1942,24 @@ dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, >> } >> >> - if (queue_size != *new_size) { >> - *new_size = queue_size; >> + if (lim) { >> + /* Check for device limits. */ >> + if (lim->nb_align) { >> + queue_size = ROUND_UP(queue_size, lim->nb_align); >> + } >> + queue_size = MIN(queue_size, lim->nb_max); >> + queue_size = MAX(queue_size, lim->nb_min); >> + } >> + >> + *cur_requested_size = queue_size; >> + >> + if (cur_size != queue_size) { >> netdev_request_reconfigure(netdev); >> + reconfig = true; >> + } >> + if (new_requested_size != queue_size) { >> + VLOG(reconfig ? VLL_INFO : VLL_DBG, >> + "Interface %s cannot set %s descriptor size to %d. " >> + "Adjusted to %d.", dev->up.name, is_rx ? "rx": "tx" , >> + new_requested_size, queue_size); > > This message is a bit misleading. It says that it can't set the > descriptor size. But it's not the descriptor size, it's a number > of descriptors. Or a descriptor ring size, but that's too much > of the unnecessary terminology. > And we should use netdev_get_name instead of accessing the name > directly. > > How about this: > > VLOG(reconfig ? VLL_INFO : VLL_DBG, > "%s: Unable to set the number of %s descriptors to %d. " > "Adjusted to %d.", netdev_get_name(netdev), > is_rx ? "rx": "tx", new_requested_size, queue_size); > > ? > That reads better, thanks. >> } >> } >> @@ -1938,7 +1977,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, >> {RTE_ETH_FC_RX_PAUSE, RTE_ETH_FC_FULL } >> }; >> + struct rte_eth_dev_info info; >> const char *new_devargs; >> const char *vf_mac; >> int err = 0; >> + int ret; >> >> ovs_mutex_lock(&dpdk_mutex); >> @@ -1947,11 +1988,4 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, >> dpdk_set_rxq_config(dev, args); >> >> - dpdk_process_queue_size(netdev, args, "n_rxq_desc", >> - NIC_PORT_DEFAULT_RXQ_SIZE, >> - &dev->requested_rxq_size); >> - dpdk_process_queue_size(netdev, args, "n_txq_desc", >> - NIC_PORT_DEFAULT_TXQ_SIZE, >> - &dev->requested_txq_size); >> - >> new_devargs = smap_get(args, "dpdk-devargs"); >> >> @@ -2073,4 +2107,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, >> } >> >> + ret = rte_eth_dev_info_get(dev->port_id, &info); >> + >> + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, true); >> + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, false); > > Any reason to move this after the flow control? > If flow control is not supported we jump over this part of the code and not > configuring the queue size. You can test that by adding a net_null device. > That's a good point. IIRC, I moved it because I added the call to info_get(), so I wanted to give the device a chance to be attached first. I guess I can move it to after the devargs stuff but before the other code to avoid this issue. Thanks for the comments, I'll respin a v5. Kevin. > Best regards, Ilya Maximets. >
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 2d9afc493..e5fc1aa30 100644 --- a/lib/netdev-dpdk.c +++ b/lib/netdev-dpdk.c @@ -1911,8 +1911,30 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const struct smap *args) static void dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, - const char *flag, int default_size, int *new_size) + struct rte_eth_dev_info *info, bool is_rx) { - int queue_size = smap_get_int(args, flag, default_size); + struct netdev_dpdk *dev = netdev_dpdk_cast(netdev); + struct rte_eth_desc_lim *lim; + int default_size, queue_size, cur_size, new_requested_size; + int *cur_requested_size; + bool reconfig = false; + if (is_rx) { + default_size = NIC_PORT_DEFAULT_RXQ_SIZE; + new_requested_size = smap_get_int(args, "n_rxq_desc", default_size); + cur_size = dev->rxq_size; + cur_requested_size = &dev->requested_rxq_size; + lim = info ? &info->rx_desc_lim : NULL; + + } else { + default_size = NIC_PORT_DEFAULT_TXQ_SIZE; + new_requested_size = smap_get_int(args, "n_txq_desc", default_size); + cur_size = dev->txq_size; + cur_requested_size = &dev->requested_txq_size; + lim = info ? &info->tx_desc_lim : NULL; + } + + queue_size = new_requested_size; + + /* Check for OVS limits. */ if (queue_size <= 0 || queue_size > NIC_PORT_MAX_Q_SIZE || !is_pow2(queue_size)) { @@ -1920,7 +1942,24 @@ dpdk_process_queue_size(struct netdev *netdev, const struct smap *args, } - if (queue_size != *new_size) { - *new_size = queue_size; + if (lim) { + /* Check for device limits. */ + if (lim->nb_align) { + queue_size = ROUND_UP(queue_size, lim->nb_align); + } + queue_size = MIN(queue_size, lim->nb_max); + queue_size = MAX(queue_size, lim->nb_min); + } + + *cur_requested_size = queue_size; + + if (cur_size != queue_size) { netdev_request_reconfigure(netdev); + reconfig = true; + } + if (new_requested_size != queue_size) { + VLOG(reconfig ? VLL_INFO : VLL_DBG, + "Interface %s cannot set %s descriptor size to %d. " + "Adjusted to %d.", dev->up.name, is_rx ? "rx": "tx" , + new_requested_size, queue_size); } } @@ -1938,7 +1977,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, {RTE_ETH_FC_RX_PAUSE, RTE_ETH_FC_FULL } }; + struct rte_eth_dev_info info; const char *new_devargs; const char *vf_mac; int err = 0; + int ret; ovs_mutex_lock(&dpdk_mutex); @@ -1947,11 +1988,4 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, dpdk_set_rxq_config(dev, args); - dpdk_process_queue_size(netdev, args, "n_rxq_desc", - NIC_PORT_DEFAULT_RXQ_SIZE, - &dev->requested_rxq_size); - dpdk_process_queue_size(netdev, args, "n_txq_desc", - NIC_PORT_DEFAULT_TXQ_SIZE, - &dev->requested_txq_size); - new_devargs = smap_get(args, "dpdk-devargs"); @@ -2073,4 +2107,9 @@ netdev_dpdk_set_config(struct netdev *netdev, const struct smap *args, } + ret = rte_eth_dev_info_get(dev->port_id, &info); + + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, true); + dpdk_process_queue_size(netdev, args, !ret ? &info : NULL, false); + out: ovs_mutex_unlock(&dev->mutex);
By default OVS configures 2048 descriptors for tx and rx queues on DPDK devices. It also allows the user to configure those values. If the values used are not acceptable to the device then queue setup would fail. The device exposes it's max/min/alignment requirements and OVS applies some limits also. Use these to ensure an acceptable value is used for the number of descriptors on a device tx/rx. If the default or user value is not acceptable, adjust to a suitable value and log. Reported-at: https://bugzilla.redhat.com/2119876 Signed-off-by: Kevin Traynor <ktraynor@redhat.com> --- lib/netdev-dpdk.c | 61 ++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 50 insertions(+), 11 deletions(-)