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> --- drivers/net/mlx5/mlx5_rxtx.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index a7f3bff..57804f5 100644 --- a/drivers/net/mlx5/mlx5_rxtx.c +++ b/drivers/net/mlx5/mlx5_rxtx.c @@ -4984,6 +4984,10 @@ enum mlx5_txcmp_code { MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) +MLX5_TXOFF_DECL(mc_mpw, + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) + MLX5_TXOFF_DECL(i_mpw, MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) @@ -5140,6 +5144,10 @@ enum mlx5_txcmp_code { MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) +MLX5_TXOFF_INFO(mc_mpw, + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) + MLX5_TXOFF_INFO(i_mpw, MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) @@ -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); return NULL; } DRV_LOG(DEBUG, "port %u has selected Tx function" -- 1.8.3.1
From: Viacheslav Ovsiienko
> 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>
Acked-by: Matan Azrad <matan@mellanox.com>
Hi,
> -----Original Message-----
> From: Viacheslav Ovsiienko <viacheslavo@mellanox.com>
> Sent: Friday, December 20, 2019 12:48 PM
> To: dev@dpdk.org
> Cc: Matan Azrad <matan@mellanox.com>; Raslan Darawsheh
> <rasland@mellanox.com>; Ori Kam <orika@mellanox.com>;
> stable@dpdk.org
> Subject: [PATCH] net/mlx5: fix ConnectX-4LX Tx burst routines set
>
> 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>
> ---
> drivers/net/mlx5/mlx5_rxtx.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c
> index a7f3bff..57804f5 100644
> --- a/drivers/net/mlx5/mlx5_rxtx.c
> +++ b/drivers/net/mlx5/mlx5_rxtx.c
> @@ -4984,6 +4984,10 @@ enum mlx5_txcmp_code {
> MLX5_TXOFF_CONFIG_INLINE |
> MLX5_TXOFF_CONFIG_EMPW |
> MLX5_TXOFF_CONFIG_MPW)
>
> +MLX5_TXOFF_DECL(mc_mpw,
> + MLX5_TXOFF_CONFIG_MULTI |
> MLX5_TXOFF_CONFIG_CSUM |
> + MLX5_TXOFF_CONFIG_EMPW |
> MLX5_TXOFF_CONFIG_MPW)
> +
> MLX5_TXOFF_DECL(i_mpw,
> MLX5_TXOFF_CONFIG_INLINE |
> MLX5_TXOFF_CONFIG_EMPW |
> MLX5_TXOFF_CONFIG_MPW)
> @@ -5140,6 +5144,10 @@ enum mlx5_txcmp_code {
> MLX5_TXOFF_CONFIG_INLINE |
> MLX5_TXOFF_CONFIG_EMPW |
> MLX5_TXOFF_CONFIG_MPW)
>
> +MLX5_TXOFF_INFO(mc_mpw,
> + MLX5_TXOFF_CONFIG_MULTI |
> MLX5_TXOFF_CONFIG_CSUM |
> + MLX5_TXOFF_CONFIG_EMPW |
> MLX5_TXOFF_CONFIG_MPW)
> +
> MLX5_TXOFF_INFO(i_mpw,
> MLX5_TXOFF_CONFIG_INLINE |
> MLX5_TXOFF_CONFIG_EMPW |
> MLX5_TXOFF_CONFIG_MPW)
> @@ -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);
> return NULL;
> }
> DRV_LOG(DEBUG, "port %u has selected Tx function"
> --
> 1.8.3.1
Patch applied to next-net-mlx,
Kindest regards,
Raslan Darawsheh
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> > --- > drivers/net/mlx5/mlx5_rxtx.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c > index a7f3bff..57804f5 100644 > --- a/drivers/net/mlx5/mlx5_rxtx.c > +++ b/drivers/net/mlx5/mlx5_rxtx.c > @@ -4984,6 +4984,10 @@ enum mlx5_txcmp_code { > MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > MLX5_TXOFF_CONFIG_MPW) > > +MLX5_TXOFF_DECL(mc_mpw, > + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | > + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) > + > MLX5_TXOFF_DECL(i_mpw, > MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > MLX5_TXOFF_CONFIG_MPW) > @@ -5140,6 +5144,10 @@ enum mlx5_txcmp_code { > MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > MLX5_TXOFF_CONFIG_MPW) > > +MLX5_TXOFF_INFO(mc_mpw, > + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | > + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) > + > MLX5_TXOFF_INFO(i_mpw, > MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > MLX5_TXOFF_CONFIG_MPW) > @@ -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); Hi Slave, 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" >
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> >> --- >> drivers/net/mlx5/mlx5_rxtx.c | 9 +++++++++ >> 1 file changed, 9 insertions(+) >> >> diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c >> index a7f3bff..57804f5 100644 >> --- a/drivers/net/mlx5/mlx5_rxtx.c >> +++ b/drivers/net/mlx5/mlx5_rxtx.c >> @@ -4984,6 +4984,10 @@ enum mlx5_txcmp_code { >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | >> MLX5_TXOFF_CONFIG_MPW) >> >> +MLX5_TXOFF_DECL(mc_mpw, >> + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | >> + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) >> + >> MLX5_TXOFF_DECL(i_mpw, >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | >> MLX5_TXOFF_CONFIG_MPW) >> @@ -5140,6 +5144,10 @@ enum mlx5_txcmp_code { >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | >> MLX5_TXOFF_CONFIG_MPW) >> >> +MLX5_TXOFF_INFO(mc_mpw, >> + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | >> + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) >> + >> MLX5_TXOFF_INFO(i_mpw, >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | >> MLX5_TXOFF_CONFIG_MPW) >> @@ -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); > > Hi Slave, Sorry Slava, it must be auto-correction, I recognized a few milliseconds too late. > > 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" >> >
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> > >> --- > >> drivers/net/mlx5/mlx5_rxtx.c | 9 +++++++++ > >> 1 file changed, 9 insertions(+) > >> > >> diff --git a/drivers/net/mlx5/mlx5_rxtx.c > >> b/drivers/net/mlx5/mlx5_rxtx.c index a7f3bff..57804f5 100644 > >> --- a/drivers/net/mlx5/mlx5_rxtx.c > >> +++ b/drivers/net/mlx5/mlx5_rxtx.c > >> @@ -4984,6 +4984,10 @@ enum mlx5_txcmp_code { > >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > >> MLX5_TXOFF_CONFIG_MPW) > >> > >> +MLX5_TXOFF_DECL(mc_mpw, > >> + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | > >> + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) > >> + > >> MLX5_TXOFF_DECL(i_mpw, > >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > >> MLX5_TXOFF_CONFIG_MPW) > >> @@ -5140,6 +5144,10 @@ enum mlx5_txcmp_code { > >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > >> MLX5_TXOFF_CONFIG_MPW) > >> > >> +MLX5_TXOFF_INFO(mc_mpw, > >> + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | > >> + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) > >> + > >> MLX5_TXOFF_INFO(i_mpw, > >> MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | > >> MLX5_TXOFF_CONFIG_MPW) > >> @@ -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); > > > > Hi Slave, > > Sorry Slava, it must be auto-correction, I recognized a few milliseconds too > late. Just forget, it is not a problem. This typo happens from time to time 😊 And it seems to be a smaller evil than a permanent torturing my colleagues with my full name "Viacheslav" 😊 > > > > > 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). With best regards, Slava
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'?
> -----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?
With best regards, Slava
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? Or perhaps a RTE_ASSERT() which is for debug.
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> --- v2: removes ambigous assert() to address the comment v1: http://patches.dpdk.org/patch/64074/ drivers/net/mlx5/mlx5_rxtx.c | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/drivers/net/mlx5/mlx5_rxtx.c b/drivers/net/mlx5/mlx5_rxtx.c index a7f3bff..e39c5b9 100644 --- a/drivers/net/mlx5/mlx5_rxtx.c +++ b/drivers/net/mlx5/mlx5_rxtx.c @@ -4984,6 +4984,10 @@ enum mlx5_txcmp_code { MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) +MLX5_TXOFF_DECL(mc_mpw, + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) + MLX5_TXOFF_DECL(i_mpw, MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) @@ -5140,6 +5144,10 @@ enum mlx5_txcmp_code { MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) +MLX5_TXOFF_INFO(mc_mpw, + MLX5_TXOFF_CONFIG_MULTI | MLX5_TXOFF_CONFIG_CSUM | + MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) + MLX5_TXOFF_INFO(i_mpw, MLX5_TXOFF_CONFIG_INLINE | MLX5_TXOFF_CONFIG_EMPW | MLX5_TXOFF_CONFIG_MPW) -- 1.8.3.1
> -----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 😊
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