On 13/11/2024 08:57, David Marchand wrote:
> Since DPDK v19.11, a couple of ethdev API have been reporting errors in
> case of invalid port id or other error conditions in drivers.
> So far, OVS did not check for those error cases.
>
> Starting v24.11 future release, the ethdev API warns for unchecked
> returned values, so let's prepare for this.
>
> Link: https://git.dpdk.org/dpdk/commit/?id=4f25d7d2252f
> Link: https://git.dpdk.org/dpdk/commit/?id=4633c3b2ebf2
> Link: https://git.dpdk.org/dpdk/commit/?id=1ff8b9a6ef24
> Signed-off-by: David Marchand <[email protected]>
> ---
> lib/netdev-dpdk.c | 47 +++++++++++++++++++++++++++++++++++------------
> 1 file changed, 35 insertions(+), 12 deletions(-)
>
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 30e9d98400..562d2efeb1 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1012,7 +1012,9 @@ check_link_status(struct netdev_dpdk *dev)
> {
> struct rte_eth_link link;
>
> - rte_eth_link_get_nowait(dev->port_id, &link);
> + if (rte_eth_link_get_nowait(dev->port_id, &link) < 0) {
> + memset(&link, 0, sizeof link);
I'm not sure about just setting to 0 on error, because it gives a link
down meaning to link_status. That could lead to a false carrier status
for the device, false log below etc.
> + }
>
> if (dev->link.link_status != link.link_status) {
> netdev_change_seq_changed(&dev->up);
> @@ -1319,7 +1321,12 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> dpdk_eth_dev_init_rx_metadata(dev);
> }
>
> - rte_eth_dev_info_get(dev->port_id, &info);
> + diag = rte_eth_dev_info_get(dev->port_id, &info);
> + if (diag < 0) {
> + VLOG_ERR("Interface %s rte_eth_dev_info_get error: %s",
> + dev->up.name, rte_strerror(-diag));
> + return -diag;
> + }
>
> dev->is_representor = (*info.dev_flags) & RTE_ETH_DEV_REPRESENTOR;
>
> @@ -1461,7 +1468,9 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
> dev->port_id, ETH_ADDR_BYTES_ARGS(eth_addr.addr_bytes));
>
> memcpy(dev->hwaddr.ea, eth_addr.addr_bytes, ETH_ADDR_LEN);
> - rte_eth_link_get_nowait(dev->port_id, &dev->link);
> + if (rte_eth_link_get_nowait(dev->port_id, &dev->link) < 0) {
> + memset(&dev->link, 0, sizeof dev->link);
> + }
>
> mbp_priv = rte_mempool_get_priv(dev->dpdk_mp->mp);
> dev->buf_size = mbp_priv->mbuf_data_room_size - RTE_PKTMBUF_HEADROOM;
> @@ -1745,6 +1754,7 @@ netdev_dpdk_destruct(struct netdev *netdev)
> bool dpdk_resources_still_used = false;
> struct rte_eth_dev_info dev_info;
> dpdk_port_t sibling_port_id;
> + int diag;
>
> /* Check if this netdev has siblings (i.e. shares DPDK resources)
> among
> * other OVS netdevs. */
> @@ -1769,7 +1779,7 @@ netdev_dpdk_destruct(struct netdev *netdev)
> }
>
> /* Retrieve eth device data before closing it. */
> - rte_eth_dev_info_get(dev->port_id, &dev_info);
> + diag = rte_eth_dev_info_get(dev->port_id, &dev_info);
>
> /* Remove the eth device. */
> rte_eth_dev_close(dev->port_id);
> @@ -1778,11 +1788,13 @@ netdev_dpdk_destruct(struct netdev *netdev)
> * Note: any remaining eth devices associated to this rte device are
> * closed by DPDK ethdev layer. */
> if (!dpdk_resources_still_used) {
> - int ret = rte_dev_remove(dev_info.device);
> + if (!diag) {
> + diag = rte_dev_remove(dev_info.device);
> + }
>
> - if (ret < 0) {
> + if (diag < 0) {
> VLOG_ERR("Device '%s' can not be detached: %s.",
> - dev->devargs, rte_strerror(-ret));
> + dev->devargs, rte_strerror(-diag));
> } else {
> /* Device was closed and detached. */
> VLOG_INFO("Device '%s' has been removed and detached",
> @@ -4046,7 +4058,9 @@ netdev_dpdk_get_speed(const struct netdev *netdev,
> uint32_t *current,
>
> ovs_mutex_lock(&dev->mutex);
> link = dev->link;
> - rte_eth_dev_info_get(dev->port_id, &dev_info);
> + if (rte_eth_dev_info_get(dev->port_id, &dev_info) < 0) {
> + memset(&dev_info, 0, sizeof dev_info);
It seems 0 is reported for unknown speed, so in a way this fn still
works with the memset. Perhaps better to set to
RTE_ETH_SPEED_NUM_UNKNOWN explicitly here though as the current is set
below to 0 by reporting link_speed as opposed that the link speed was
UNKNOWN.
> + }
> ovs_mutex_unlock(&dev->mutex);
>
> *current = link.link_speed != RTE_ETH_SPEED_NUM_UNKNOWN
> @@ -4422,7 +4436,9 @@ netdev_dpdk_get_status(const struct netdev *netdev,
> struct smap *args)
>
> ovs_mutex_lock(&dpdk_mutex);
> ovs_mutex_lock(&dev->mutex);
> - rte_eth_dev_info_get(dev->port_id, &dev_info);
> + if (rte_eth_dev_info_get(dev->port_id, &dev_info) < 0) {
> + memset(&dev_info, 0, sizeof dev_info);
> + }
Maybe we should just report port_no, numa_id etc and not what info was
needed for in the event of error.
> link_speed = dev->link.link_speed;
> bus_info = rte_dev_bus_info(dev_info.device);
> rx_steer_flags = dev->rx_steer_flags;
> @@ -4566,6 +4582,7 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> dpdk_port_t port_id;
> bool used = false;
> char *response;
> + int diag;
>
> ovs_mutex_lock(&dpdk_mutex);
>
> @@ -4601,9 +4618,9 @@ netdev_dpdk_detach(struct unixctl_conn *conn, int argc
> OVS_UNUSED,
> }
> ds_destroy(&used_interfaces);
>
> - rte_eth_dev_info_get(port_id, &dev_info);
> + diag = rte_eth_dev_info_get(port_id, &dev_info);
> rte_eth_dev_close(port_id);
> - if (rte_dev_remove(dev_info.device) < 0) {
> + if (diag < 0 || rte_dev_remove(dev_info.device) < 0) {
> response = xasprintf("Device '%s' can not be detached", argv[1]);
> goto error;
> }
> @@ -5946,7 +5963,12 @@ dpdk_rx_steer_rss_configure(struct netdev_dpdk *dev,
> int rss_n_rxq)
> struct rte_eth_dev_info info;
> int err;
>
> - rte_eth_dev_info_get(dev->port_id, &info);
> + err = rte_eth_dev_info_get(dev->port_id, &info);
> + if (err < 0) {
> + VLOG_WARN("%s: failed to query RSS info: %s",
> + netdev_get_name(&dev->up), rte_strerror(-err));
> + goto error;
> + }
>
> if (info.reta_size % rss_n_rxq != 0 &&
> info.reta_size < RTE_ETH_RSS_RETA_SIZE_128) {
> @@ -5992,6 +6014,7 @@ dpdk_rx_steer_rss_configure(struct netdev_dpdk *dev,
> int rss_n_rxq)
> netdev_get_name(&dev->up), err);
> }
>
> +error:
> return err;
> }
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
