Re: [dpdk-dev] [PATCH] net/mlx5: fix eagain on admin down

[Slava Ovsiienko <viacheslavo@mellanox.com>]

Hi, Mike

> -----Original Message-----
> From: Mike Manning <mmanning@vyatta.att-mail.com>
> Sent: Friday, December 13, 2019 17:12
> To: Slava Ovsiienko <viacheslavo@mellanox.com>; Thomas Monjalon
> <thomas@monjalon.net>; dev@dpdk.org
> Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>; Matan Azrad
> <matan@mellanox.com>; Shahaf Shuler <shahafs@mellanox.com>; Raslan
> Darawsheh <rasland@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix eagain on admin down
> 
> Hi Slava,
> Thanks, you are right - I retract my patch, as this was fixed in v19.08 by
> 6fd05da9efbd ("net/mlx5: fix link speed info when link is down") as per the
> line you indicate.
> 
> I should explain that we are using Debian 10 which uses v18.11, here we had a
> painful issue with the combination of this -EAGAIN and use of netlink to
> obtain the ifindex resulting in concurrent use of the netlink socket causing the
> calling thread to block indefinitely.

JFYI, there is the patch: http://patches.dpdk.org/patch/56813/
It provides caching for ifindex, netlink is not involved anymore.

With best regards, Slava


> 
> Regards
> Mike
> 
> On 13/12/2019 14:43, Slava Ovsiienko wrote:
> > Hi, Mike
> >
> > In the mlx5_link_update_unlocked_gs() the dev_link.link_speed is set like
> this:
> >
> > dev_link.link_speed = (ecmd->speed == UINT32_MAX) ?
> ETH_SPEED_NUM_NONE
> > :  ecmd->speed;
> >
> > So, dev_link.link_speed can't be assigned with -1. Do I misunderstand you
> commit message?
> >
> > With best regards, Slava
> >
> >> -----Original Message-----
> >> From: Thomas Monjalon <thomas@monjalon.net>
> >> Sent: Friday, December 13, 2019 0:31
> >> To: dev@dpdk.org
> >> Cc: Mike Manning <mmanning@vyatta.att-mail.com>; Nélio Laranjeiro
> >> <nelio.laranjeiro@6wind.com>; Matan Azrad <matan@mellanox.com>;
> Slava
> >> Ovsiienko <viacheslavo@mellanox.com>; Shahaf Shuler
> >> <shahafs@mellanox.com>; Raslan Darawsheh <rasland@mellanox.com>
> >> Subject: Re: [dpdk-dev] [PATCH] net/mlx5: fix eagain on admin down
> >>
> >> +Cc maintainers
> >>
> >> 09/12/2019 19:23, Mike Manning:
> >>> The check in mlx5_link_update_unlocked_gs() returns -EAGAIN if link
> >>> status does not correspond to link speed. If status is DOWN, the
> >>> speed is expected to be ETH_SPEED_NUM_NONE (0). But as the link
> >>> speed is -1 on admin down, modify the check to account for this.
> >>>
> >>> Fixes: cfee94752b8f ("net/mlx5: fix link status to use wait to
> >>> complete")
> >>> Cc: Nélio Laranjeiro <nelio.laranjeiro@6wind.com>
> >>>
> >>> Signed-off-by: Mike Manning <mmanning@vyatta.att-mail.com>
> >>> ---
> >>>  drivers/net/mlx5/mlx5_ethdev.c | 4 ++--
> >>>  1 file changed, 2 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/drivers/net/mlx5/mlx5_ethdev.c
> >>> b/drivers/net/mlx5/mlx5_ethdev.c index d80ae458b..6ef2dfd74 100644
> >>> --- a/drivers/net/mlx5/mlx5_ethdev.c
> >>> +++ b/drivers/net/mlx5/mlx5_ethdev.c
> >>> @@ -1031,8 +1031,8 @@ mlx5_link_update_unlocked_gs(struct
> >> rte_eth_dev *dev,
> >>>  				ETH_LINK_HALF_DUPLEX :
> >> ETH_LINK_FULL_DUPLEX);
> >>>  	dev_link.link_autoneg = !(dev->data->dev_conf.link_speeds &
> >>>  				  ETH_LINK_SPEED_FIXED);
> >>> -	if (((dev_link.link_speed && !dev_link.link_status) ||
> >>> -	     (!dev_link.link_speed && dev_link.link_status))) {
> >>> +	if ((((int)dev_link.link_speed > 0 && !dev_link.link_status) ||
> >>> +	     ((int)dev_link.link_speed <= 0 && dev_link.link_status))) {
> >>>  		rte_errno = EAGAIN;
> >>>  		return -rte_errno;
> >>>  	}
> >>>
> >>
> >>
> >>