Re: [dpdk-stable] [PATCH] net/mlx5: fix ConnectX-4LX Tx burst routines set

[Raslan Darawsheh <rasland@mellanox.com>]

Hi,

> -----Original Message-----
> From: Slava Ovsiienko <viacheslavo@mellanox.com>
> Sent: Thursday, January 9, 2020 1:10 PM
> To: Ferruh Yigit <ferruh.yigit@intel.com>; dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>;
> stable@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> Subject: RE: [dpdk-stable] [PATCH] net/mlx5: fix ConnectX-4LX Tx burst
> routines set
> 
> > -----Original Message-----
> > From: Ferruh Yigit <ferruh.yigit@intel.com>
> > Sent: Thursday, January 9, 2020 12:50
> > To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>;
> stable@dpdk.org;
> > Thomas Monjalon <thomas@monjalon.net>
> > Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix ConnectX-4LX Tx burst
> > routines set
> >
> > On 1/9/2020 9:03 AM, Slava Ovsiienko wrote:
> > >> -----Original Message-----
> > >> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > >> Sent: Wednesday, January 8, 2020 17:55
> > >> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > >> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > >> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>;
> > >> stable@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> > >> Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix ConnectX-4LX Tx
> > >> burst routines set
> > >>
> > >> On 1/8/2020 3:50 PM, Slava Ovsiienko wrote:
> > >>> Hi, Ferruh
> > >>>
> > >>>> -----Original Message-----
> > >>>> From: Ferruh Yigit <ferruh.yigit@intel.com>
> > >>>> Sent: Wednesday, January 8, 2020 16:55
> > >>>> To: Slava Ovsiienko <viacheslavo@mellanox.com>; dev@dpdk.org
> > >>>> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> > >>>> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>;
> > >>>> stable@dpdk.org; Thomas Monjalon <thomas@monjalon.net>
> > >>>> Subject: Re: [dpdk-stable] [PATCH] net/mlx5: fix ConnectX-4LX Tx
> > >>>> burst routines set
> > >>>>
> > >>>> On 1/8/2020 2:53 PM, Ferruh Yigit wrote:
> > >>>>> On 12/20/2019 10:48 AM, Viacheslav Ovsiienko wrote:
> > >>>>>> The tx_burst routine supporting multi-segment packets with
> > >>>>>> legacy MPW and without inline was missed, and there was no
> > >>>>>> valid selection for these options, patch adds the missing routine.
> > >>>>>>
> > >>>>>> Fixes: 82e75f8323bf ("net/mlx5: fix legacy multi-packet Tx
> > >>>>>> descriptors")
> > >>>>>> Cc: stable@dpdk.org
> > >>>>>>
> > >>>>>> Signed-off-by: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> > >>
> > >> <...>
> > >>
> > >>>>>> @@ -5297,6 +5305,7 @@ enum mlx5_txcmp_code {
> > >>>>>>  		DRV_LOG(DEBUG, "port %u has no selected Tx
> > function"
> > >>>>>>  			       " for requested offloads %04X",
> > >>>>>>  				dev->data->port_id, olx);
> > >>>>>> +		assert(false);
> > >>
> > >> <...>
> > >>
> > >>>
> > >>>>
> > >>>>>
> > >>>>> I think we should avoid PMDs calling the assert unconditionally,
> > >>>>> specially in a code that debug level log is printed.
> > >>>>>
> > >>>>>>  		return NULL;
> > >>>>>>  	}
> > >>>>>>  	DRV_LOG(DEBUG, "port %u has selected Tx function"
> > >>>
> > >>> Yes, I agree. We just do not have the check for the result
> > >>> returned by mlx5_select_tx_function(). I think we should check
> > >>> against NULL and report an error.  "assert" is a temporary
> > >>> solution till this upgrade (in debug mode we have a lot of
> > >>> messages and break on assert helps to locate the problem quickly,
> reporting error will do the same).
> > >>>
> > >>
> > >> Can it be possible to drop the patch from mlx tree and prepare a
> > >> new version without 'assert'?
> > > The selection routine error handling is rather generic and is not
> > > merely
> > related to ConnectX-4LX.
> > > I propose to prepare the dedicated patch, what do you  think?
> > >
> >
> > My concern is with the assert, the error handling can be another
> > patch, but can we have this change without an assert?
> 
> Yes, please: http://patches.dpdk.org/patch/64340/
> 
> With best regards, Slava
> 
> PS. Removing this assert urges me to add error handling ASAP 😊

This patch has been removed from next-net-mlx,
I'll apply the v2

Kindest regards,
Raslan Darawsheh