[dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Wei Zhao]

There is an error in function rxtx_port_config(), which may overwrite
offloads configuration get from function launch_args_parse() when run
testpmd app. So rxtx_port_config() should do "or" for port offloads.

Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
cc: stable@dpdk.org

Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
 app/test-pmd/testpmd.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
index 6fbfd29..f0061d9 100644
--- a/app/test-pmd/testpmd.c
+++ b/app/test-pmd/testpmd.c
@@ -2809,9 +2809,12 @@ static void
 rxtx_port_config(struct rte_port *port)
 {
 	uint16_t qid;
+	uint64_t offloads;
 
 	for (qid = 0; qid < nb_rxq; qid++) {
+		offloads = port->rx_conf[qid].offloads;
 		port->rx_conf[qid] = port->dev_info.default_rxconf;
+		port->rx_conf[qid].offloads |= offloads;
 
 		/* Check if any Rx parameters have been passed */
 		if (rx_pthresh != RTE_PMD_PARAM_UNSET)
@@ -2833,7 +2836,9 @@ rxtx_port_config(struct rte_port *port)
 	}
 
 	for (qid = 0; qid < nb_txq; qid++) {
+		offloads = port->tx_conf[qid].offloads;
 		port->tx_conf[qid] = port->dev_info.default_txconf;
+		port->tx_conf[qid].offloads |= offloads;
 
 		/* Check if any Tx parameters have been passed */
 		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
-- 
2.7.5

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

Tested-by: Peng Yuan <yuan.peng@intel.com>


> -----Original Message-----
> From: Zhao1, Wei
> Sent: Thursday, May 9, 2019 3:21 PM
> To: dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Yigit, Ferruh
> <ferruh.yigit@intel.com>; Lu, Wenzhuo <wenzhuo.lu@intel.com>; Zhao1, Wei
> <wei.zhao1@intel.com>
> Subject: [PATCH] app/testpmd: fix offloads overwrite by default configuration
> 
> There is an error in function rxtx_port_config(), which may overwrite offloads
> configuration get from function launch_args_parse() when run testpmd app. So
> rxtx_port_config() should do "or" for port offloads.
> 
> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> cc: stable@dpdk.org
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  app/test-pmd/testpmd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> 6fbfd29..f0061d9 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2809,9 +2809,12 @@ static void
>  rxtx_port_config(struct rte_port *port)  {
>  	uint16_t qid;
> +	uint64_t offloads;
> 
>  	for (qid = 0; qid < nb_rxq; qid++) {
> +		offloads = port->rx_conf[qid].offloads;
>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> +		port->rx_conf[qid].offloads |= offloads;
> 
>  		/* Check if any Rx parameters have been passed */
>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
> +2836,9 @@ rxtx_port_config(struct rte_port *port)
>  	}
> 
>  	for (qid = 0; qid < nb_txq; qid++) {
> +		offloads = port->tx_conf[qid].offloads;
>  		port->tx_conf[qid] = port->dev_info.default_txconf;
> +		port->tx_conf[qid].offloads |= offloads;
> 
>  		/* Check if any Tx parameters have been passed */
>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> --
> 2.7.5

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Thomas Monjalon]

13/05/2019 05:30, Zhao1, Wei:
> From: Zhao1, Wei
> > There is an error in function rxtx_port_config(), which may overwrite offloads
> > configuration get from function launch_args_parse() when run testpmd app. So
> > rxtx_port_config() should do "or" for port offloads.
> > 
> > Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> > cc: stable@dpdk.org
> > 
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> Tested-by: Peng Yuan <yuan.peng@intel.com>

Applied, thanks

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 5/9/2019 8:20 AM, Wei Zhao wrote:
> There is an error in function rxtx_port_config(), which may overwrite
> offloads configuration get from function launch_args_parse() when run
> testpmd app. So rxtx_port_config() should do "or" for port offloads.
> 
> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> cc: stable@dpdk.org
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  app/test-pmd/testpmd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 6fbfd29..f0061d9 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2809,9 +2809,12 @@ static void
>  rxtx_port_config(struct rte_port *port)
>  {
>  	uint16_t qid;
> +	uint64_t offloads;
>  
>  	for (qid = 0; qid < nb_rxq; qid++) {
> +		offloads = port->rx_conf[qid].offloads;
>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> +		port->rx_conf[qid].offloads |= offloads;

OK to this changes as a fix for this release.

But I think intention is, if no offload information is provided by user to use
use the driver provided defaults, if user explicitly provided some values to use
them, instead of OR these two.

With this approach it is not possible to disable a driver default value, so it
becomes mandatory offload instead of default offload values.

Wei, what do you think, does it make sense?

>  
>  		/* Check if any Rx parameters have been passed */
>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET)
> @@ -2833,7 +2836,9 @@ rxtx_port_config(struct rte_port *port)
>  	}
>  
>  	for (qid = 0; qid < nb_txq; qid++) {
> +		offloads = port->tx_conf[qid].offloads;
>  		port->tx_conf[qid] = port->dev_info.default_txconf;
> +		port->tx_conf[qid].offloads |= offloads;
>  
>  		/* Check if any Tx parameters have been passed */
>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> 

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, May 14, 2019 12:36 AM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> > There is an error in function rxtx_port_config(), which may overwrite
> > offloads configuration get from function launch_args_parse() when run
> > testpmd app. So rxtx_port_config() should do "or" for port offloads.
> >
> > Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> > cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 6fbfd29..f0061d9 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2809,9 +2809,12 @@ static void
> >  rxtx_port_config(struct rte_port *port)  {
> >  	uint16_t qid;
> > +	uint64_t offloads;
> >
> >  	for (qid = 0; qid < nb_rxq; qid++) {
> > +		offloads = port->rx_conf[qid].offloads;
> >  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> > +		port->rx_conf[qid].offloads |= offloads;
> 
> OK to this changes as a fix for this release.
> 
> But I think intention is, if no offload information is provided by user to use use
> the driver provided defaults, if user explicitly provided some values to use them,
> instead of OR these two.
> 
> With this approach it is not possible to disable a driver default value, so it
> becomes mandatory offload instead of default offload values.
> 
> Wei, what do you think, does it make sense?


I agree with you, but it is sure that the original code has offloads overwrite issue.
What is your suggestion for code implement?
I find that Thomas has apply it, if you has other idea, maybe you has to commit patch base to this patch.

> 
> >
> >  		/* Check if any Rx parameters have been passed */
> >  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
> +2836,9 @@
> > rxtx_port_config(struct rte_port *port)
> >  	}
> >
> >  	for (qid = 0; qid < nb_txq; qid++) {
> > +		offloads = port->tx_conf[qid].offloads;
> >  		port->tx_conf[qid] = port->dev_info.default_txconf;
> > +		port->tx_conf[qid].offloads |= offloads;
> >
> >  		/* Check if any Tx parameters have been passed */
> >  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
> Hi,  Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, May 14, 2019 12:36 AM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
>> default configuration
>>
>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
>>> There is an error in function rxtx_port_config(), which may overwrite
>>> offloads configuration get from function launch_args_parse() when run
>>> testpmd app. So rxtx_port_config() should do "or" for port offloads.
>>>
>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
>>> cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>> ---
>>>  app/test-pmd/testpmd.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 6fbfd29..f0061d9 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2809,9 +2809,12 @@ static void
>>>  rxtx_port_config(struct rte_port *port)  {
>>>  	uint16_t qid;
>>> +	uint64_t offloads;
>>>
>>>  	for (qid = 0; qid < nb_rxq; qid++) {
>>> +		offloads = port->rx_conf[qid].offloads;
>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
>>> +		port->rx_conf[qid].offloads |= offloads;
>>
>> OK to this changes as a fix for this release.
>>
>> But I think intention is, if no offload information is provided by user to use use
>> the driver provided defaults, if user explicitly provided some values to use them,
>> instead of OR these two.
>>
>> With this approach it is not possible to disable a driver default value, so it
>> becomes mandatory offload instead of default offload values.
>>
>> Wei, what do you think, does it make sense?
> 
> 
> I agree with you, but it is sure that the original code has offloads overwrite issue.
> What is your suggestion for code implement?
> I find that Thomas has apply it, if you has other idea, maybe you has to commit patch base to this patch.

Hi Wei,

Yes this needs to be incremental fix to existing code.

Queue specific offload can be altered either by providing Rx/Tx offload as
command line argument [1] (port configs set to each queues) or via testpmd
commands [2].
Does it make sense to set a global flag when one of above occurs and use default
config only if it is not set?

[1]
Tx
  tx-offloads
Rx
  disable-crc-strip
  enable-lro
  enable-scatter
  enable-rx-cksum
  enable-rx-timestamp
  enable-hw-vlan
  enable-hw-vlan-filter
  enable-hw-vlan-strip
  enable-hw-vlan-extend

[2]
"port config <port_id> rx_offload ..."
"port <port_id> rxq <queue_id> rx_offload ..."
"port config <port_id> tx_offload ..."
"port <port_id> txq <queue_id> tx_offload ..."

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

Hi, Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Monday, May 20, 2019 11:23 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
> > Hi,  Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, May 14, 2019 12:36 AM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >> overwrite by default configuration
> >>
> >> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> >>> There is an error in function rxtx_port_config(), which may
> >>> overwrite offloads configuration get from function
> >>> launch_args_parse() when run testpmd app. So rxtx_port_config() should
> do "or" for port offloads.
> >>>
> >>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> >>> cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> ---
> >>>  app/test-pmd/testpmd.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> 6fbfd29..f0061d9 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -2809,9 +2809,12 @@ static void
> >>>  rxtx_port_config(struct rte_port *port)  {
> >>>  	uint16_t qid;
> >>> +	uint64_t offloads;
> >>>
> >>>  	for (qid = 0; qid < nb_rxq; qid++) {
> >>> +		offloads = port->rx_conf[qid].offloads;
> >>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> >>> +		port->rx_conf[qid].offloads |= offloads;
> >>
> >> OK to this changes as a fix for this release.
> >>
> >> But I think intention is, if no offload information is provided by
> >> user to use use the driver provided defaults, if user explicitly
> >> provided some values to use them, instead of OR these two.
> >>
> >> With this approach it is not possible to disable a driver default
> >> value, so it becomes mandatory offload instead of default offload values.
> >>
> >> Wei, what do you think, does it make sense?
> >
> >
> > I agree with you, but it is sure that the original code has offloads overwrite
> issue.
> > What is your suggestion for code implement?
> > I find that Thomas has apply it, if you has other idea, maybe you has to
> commit patch base to this patch.
> 
> Hi Wei,
> 
> Yes this needs to be incremental fix to existing code.
> 
> Queue specific offload can be altered either by providing Rx/Tx offload as
> command line argument [1] (port configs set to each queues) or via testpmd
> commands [2].
> Does it make sense to set a global flag when one of above occurs and use
> default config only if it is not set?

I  AGREE with you to submit an incremental fix, and it make sense to set a global flag when one of above occurs and use
 default config only if it is not set when implement code, but I do not have time to prepare such a patch by now, so maybe later or some else.

> 
> [1]
> Tx
>   tx-offloads
> Rx
>   disable-crc-strip
>   enable-lro
>   enable-scatter
>   enable-rx-cksum
>   enable-rx-timestamp
>   enable-hw-vlan
>   enable-hw-vlan-filter
>   enable-hw-vlan-strip
>   enable-hw-vlan-extend
> 
> [2]
> "port config <port_id> rx_offload ..."
> "port <port_id> rxq <queue_id> rx_offload ..."
> "port config <port_id> tx_offload ..."
> "port <port_id> txq <queue_id> tx_offload ..."
> 

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 5/21/2019 2:30 AM, Zhao1, Wei wrote:
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Monday, May 20, 2019 11:23 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
>> default configuration
>>
>> On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
>>> Hi,  Ferruh
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, May 14, 2019 12:36 AM
>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>
>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
>>>> overwrite by default configuration
>>>>
>>>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
>>>>> There is an error in function rxtx_port_config(), which may
>>>>> overwrite offloads configuration get from function
>>>>> launch_args_parse() when run testpmd app. So rxtx_port_config() should
>> do "or" for port offloads.
>>>>>
>>>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
>>>>> cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>>> ---
>>>>>  app/test-pmd/testpmd.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> 6fbfd29..f0061d9 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2809,9 +2809,12 @@ static void
>>>>>  rxtx_port_config(struct rte_port *port)  {
>>>>>  	uint16_t qid;
>>>>> +	uint64_t offloads;
>>>>>
>>>>>  	for (qid = 0; qid < nb_rxq; qid++) {
>>>>> +		offloads = port->rx_conf[qid].offloads;
>>>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
>>>>> +		port->rx_conf[qid].offloads |= offloads;
>>>>
>>>> OK to this changes as a fix for this release.
>>>>
>>>> But I think intention is, if no offload information is provided by
>>>> user to use use the driver provided defaults, if user explicitly
>>>> provided some values to use them, instead of OR these two.
>>>>
>>>> With this approach it is not possible to disable a driver default
>>>> value, so it becomes mandatory offload instead of default offload values.
>>>>
>>>> Wei, what do you think, does it make sense?
>>>
>>>
>>> I agree with you, but it is sure that the original code has offloads overwrite
>> issue.
>>> What is your suggestion for code implement?
>>> I find that Thomas has apply it, if you has other idea, maybe you has to
>> commit patch base to this patch.
>>
>> Hi Wei,
>>
>> Yes this needs to be incremental fix to existing code.
>>
>> Queue specific offload can be altered either by providing Rx/Tx offload as
>> command line argument [1] (port configs set to each queues) or via testpmd
>> commands [2].
>> Does it make sense to set a global flag when one of above occurs and use
>> default config only if it is not set?
> 
> I  AGREE with you to submit an incremental fix, and it make sense to set a global flag when one of above occurs and use
>  default config only if it is not set when implement code, but I do not have time to prepare such a patch by now, so maybe later or some else.

I see, can you submit a public defect to record the issue, so it can be
addressed later without forgotten?

> 
>>
>> [1]
>> Tx
>>   tx-offloads
>> Rx
>>   disable-crc-strip
>>   enable-lro
>>   enable-scatter
>>   enable-rx-cksum
>>   enable-rx-timestamp
>>   enable-hw-vlan
>>   enable-hw-vlan-filter
>>   enable-hw-vlan-strip
>>   enable-hw-vlan-extend
>>
>> [2]
>> "port config <port_id> rx_offload ..."
>> "port <port_id> rxq <queue_id> rx_offload ..."
>> "port config <port_id> tx_offload ..."
>> "port <port_id> txq <queue_id> tx_offload ..."
>>
> 

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

Hi, Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, May 21, 2019 11:43 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 5/21/2019 2:30 AM, Zhao1, Wei wrote:
> > Hi, Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Monday, May 20, 2019 11:23 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >> overwrite by default configuration
> >>
> >> On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
> >>> Hi,  Ferruh
> >>>
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Tuesday, May 14, 2019 12:36 AM
> >>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >>>> <wenzhuo.lu@intel.com>
> >>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >>>> overwrite by default configuration
> >>>>
> >>>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> >>>>> There is an error in function rxtx_port_config(), which may
> >>>>> overwrite offloads configuration get from function
> >>>>> launch_args_parse() when run testpmd app. So rxtx_port_config()
> >>>>> should
> >> do "or" for port offloads.
> >>>>>
> >>>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> >>>>> cc: stable@dpdk.org
> >>>>>
> >>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>>>> ---
> >>>>>  app/test-pmd/testpmd.c | 5 +++++
> >>>>>  1 file changed, 5 insertions(+)
> >>>>>
> >>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>>>> 6fbfd29..f0061d9 100644
> >>>>> --- a/app/test-pmd/testpmd.c
> >>>>> +++ b/app/test-pmd/testpmd.c
> >>>>> @@ -2809,9 +2809,12 @@ static void  rxtx_port_config(struct
> >>>>> rte_port *port)  {
> >>>>>  	uint16_t qid;
> >>>>> +	uint64_t offloads;
> >>>>>
> >>>>>  	for (qid = 0; qid < nb_rxq; qid++) {
> >>>>> +		offloads = port->rx_conf[qid].offloads;
> >>>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> >>>>> +		port->rx_conf[qid].offloads |= offloads;
> >>>>
> >>>> OK to this changes as a fix for this release.
> >>>>
> >>>> But I think intention is, if no offload information is provided by
> >>>> user to use use the driver provided defaults, if user explicitly
> >>>> provided some values to use them, instead of OR these two.
> >>>>
> >>>> With this approach it is not possible to disable a driver default
> >>>> value, so it becomes mandatory offload instead of default offload values.
> >>>>
> >>>> Wei, what do you think, does it make sense?
> >>>
> >>>
> >>> I agree with you, but it is sure that the original code has offloads
> >>> overwrite
> >> issue.
> >>> What is your suggestion for code implement?
> >>> I find that Thomas has apply it, if you has other idea, maybe you
> >>> has to
> >> commit patch base to this patch.
> >>
> >> Hi Wei,
> >>
> >> Yes this needs to be incremental fix to existing code.
> >>
> >> Queue specific offload can be altered either by providing Rx/Tx
> >> offload as command line argument [1] (port configs set to each
> >> queues) or via testpmd commands [2].
> >> Does it make sense to set a global flag when one of above occurs and
> >> use default config only if it is not set?
> >
> > I  AGREE with you to submit an incremental fix, and it make sense to
> > set a global flag when one of above occurs and use  default config only if it is
> not set when implement code, but I do not have time to prepare such a patch
> by now, so maybe later or some else.
> 
> I see, can you submit a public defect to record the issue, so it can be addressed
> later without forgotten?

Sure, but what is a public defect patch? Do you mean I need to update some doc? Can you give me a link as an example ?


> 
> >
> >>
> >> [1]
> >> Tx
> >>   tx-offloads
> >> Rx
> >>   disable-crc-strip
> >>   enable-lro
> >>   enable-scatter
> >>   enable-rx-cksum
> >>   enable-rx-timestamp
> >>   enable-hw-vlan
> >>   enable-hw-vlan-filter
> >>   enable-hw-vlan-strip
> >>   enable-hw-vlan-extend
> >>
> >> [2]
> >> "port config <port_id> rx_offload ..."
> >> "port <port_id> rxq <queue_id> rx_offload ..."
> >> "port config <port_id> tx_offload ..."
> >> "port <port_id> txq <queue_id> tx_offload ..."
> >>
> >

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 5/24/2019 2:55 AM, Zhao1, Wei wrote:
> 
> Hi, Ferruh
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, May 21, 2019 11:43 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
>> default configuration
>>
>> On 5/21/2019 2:30 AM, Zhao1, Wei wrote:
>>> Hi, Ferruh
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Monday, May 20, 2019 11:23 PM
>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>
>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
>>>> overwrite by default configuration
>>>>
>>>> On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
>>>>> Hi,  Ferruh
>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Yigit, Ferruh
>>>>>> Sent: Tuesday, May 14, 2019 12:36 AM
>>>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>>>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>>>>>> <wenzhuo.lu@intel.com>
>>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
>>>>>> overwrite by default configuration
>>>>>>
>>>>>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
>>>>>>> There is an error in function rxtx_port_config(), which may
>>>>>>> overwrite offloads configuration get from function
>>>>>>> launch_args_parse() when run testpmd app. So rxtx_port_config()
>>>>>>> should
>>>> do "or" for port offloads.
>>>>>>>
>>>>>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
>>>>>>> cc: stable@dpdk.org
>>>>>>>
>>>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>>>>> ---
>>>>>>>  app/test-pmd/testpmd.c | 5 +++++
>>>>>>>  1 file changed, 5 insertions(+)
>>>>>>>
>>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>>>> 6fbfd29..f0061d9 100644
>>>>>>> --- a/app/test-pmd/testpmd.c
>>>>>>> +++ b/app/test-pmd/testpmd.c
>>>>>>> @@ -2809,9 +2809,12 @@ static void  rxtx_port_config(struct
>>>>>>> rte_port *port)  {
>>>>>>>  	uint16_t qid;
>>>>>>> +	uint64_t offloads;
>>>>>>>
>>>>>>>  	for (qid = 0; qid < nb_rxq; qid++) {
>>>>>>> +		offloads = port->rx_conf[qid].offloads;
>>>>>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
>>>>>>> +		port->rx_conf[qid].offloads |= offloads;
>>>>>>
>>>>>> OK to this changes as a fix for this release.
>>>>>>
>>>>>> But I think intention is, if no offload information is provided by
>>>>>> user to use use the driver provided defaults, if user explicitly
>>>>>> provided some values to use them, instead of OR these two.
>>>>>>
>>>>>> With this approach it is not possible to disable a driver default
>>>>>> value, so it becomes mandatory offload instead of default offload values.
>>>>>>
>>>>>> Wei, what do you think, does it make sense?
>>>>>
>>>>>
>>>>> I agree with you, but it is sure that the original code has offloads
>>>>> overwrite
>>>> issue.
>>>>> What is your suggestion for code implement?
>>>>> I find that Thomas has apply it, if you has other idea, maybe you
>>>>> has to
>>>> commit patch base to this patch.
>>>>
>>>> Hi Wei,
>>>>
>>>> Yes this needs to be incremental fix to existing code.
>>>>
>>>> Queue specific offload can be altered either by providing Rx/Tx
>>>> offload as command line argument [1] (port configs set to each
>>>> queues) or via testpmd commands [2].
>>>> Does it make sense to set a global flag when one of above occurs and
>>>> use default config only if it is not set?
>>>
>>> I  AGREE with you to submit an incremental fix, and it make sense to
>>> set a global flag when one of above occurs and use  default config only if it is
>> not set when implement code, but I do not have time to prepare such a patch
>> by now, so maybe later or some else.
>>
>> I see, can you submit a public defect to record the issue, so it can be addressed
>> later without forgotten?
> 
> Sure, but what is a public defect patch? Do you mean I need to update some doc? Can you give me a link as an example 

No documentation, please create an issue in public DPDK bug tracker:
https://bugs.dpdk.org/


> 
> 
>>
>>>
>>>>
>>>> [1]
>>>> Tx
>>>>   tx-offloads
>>>> Rx
>>>>   disable-crc-strip
>>>>   enable-lro
>>>>   enable-scatter
>>>>   enable-rx-cksum
>>>>   enable-rx-timestamp
>>>>   enable-hw-vlan
>>>>   enable-hw-vlan-filter
>>>>   enable-hw-vlan-strip
>>>>   enable-hw-vlan-extend
>>>>
>>>> [2]
>>>> "port config <port_id> rx_offload ..."
>>>> "port <port_id> rxq <queue_id> rx_offload ..."
>>>> "port config <port_id> tx_offload ..."
>>>> "port <port_id> txq <queue_id> tx_offload ..."
>>>>
>>>
> 

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

Hi,  Ferruh

A patch has been commit for this issue by me, so no need for bug tracker
https://patches.dpdk.org/patch/54584/


> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, May 24, 2019 9:04 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 5/24/2019 2:55 AM, Zhao1, Wei wrote:
> >
> > Hi, Ferruh
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, May 21, 2019 11:43 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >> overwrite by default configuration
> >>
> >> On 5/21/2019 2:30 AM, Zhao1, Wei wrote:
> >>> Hi, Ferruh
> >>>
> >>>> -----Original Message-----
> >>>> From: Yigit, Ferruh
> >>>> Sent: Monday, May 20, 2019 11:23 PM
> >>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >>>> <wenzhuo.lu@intel.com>
> >>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >>>> overwrite by default configuration
> >>>>
> >>>> On 5/14/2019 2:56 AM, Zhao1, Wei wrote:
> >>>>> Hi,  Ferruh
> >>>>>
> >>>>>> -----Original Message-----
> >>>>>> From: Yigit, Ferruh
> >>>>>> Sent: Tuesday, May 14, 2019 12:36 AM
> >>>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >>>>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu,
> >>>>>> Wenzhuo <wenzhuo.lu@intel.com>
> >>>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >>>>>> overwrite by default configuration
> >>>>>>
> >>>>>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> >>>>>>> There is an error in function rxtx_port_config(), which may
> >>>>>>> overwrite offloads configuration get from function
> >>>>>>> launch_args_parse() when run testpmd app. So rxtx_port_config()
> >>>>>>> should
> >>>> do "or" for port offloads.
> >>>>>>>
> >>>>>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> >>>>>>> cc: stable@dpdk.org
> >>>>>>>
> >>>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>>>>>> ---
> >>>>>>>  app/test-pmd/testpmd.c | 5 +++++
> >>>>>>>  1 file changed, 5 insertions(+)
> >>>>>>>
> >>>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> >>>>>>> index
> >>>>>>> 6fbfd29..f0061d9 100644
> >>>>>>> --- a/app/test-pmd/testpmd.c
> >>>>>>> +++ b/app/test-pmd/testpmd.c
> >>>>>>> @@ -2809,9 +2809,12 @@ static void  rxtx_port_config(struct
> >>>>>>> rte_port *port)  {
> >>>>>>>  	uint16_t qid;
> >>>>>>> +	uint64_t offloads;
> >>>>>>>
> >>>>>>>  	for (qid = 0; qid < nb_rxq; qid++) {
> >>>>>>> +		offloads = port->rx_conf[qid].offloads;
> >>>>>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> >>>>>>> +		port->rx_conf[qid].offloads |= offloads;
> >>>>>>
> >>>>>> OK to this changes as a fix for this release.
> >>>>>>
> >>>>>> But I think intention is, if no offload information is provided
> >>>>>> by user to use use the driver provided defaults, if user
> >>>>>> explicitly provided some values to use them, instead of OR these two.
> >>>>>>
> >>>>>> With this approach it is not possible to disable a driver default
> >>>>>> value, so it becomes mandatory offload instead of default offload
> values.
> >>>>>>
> >>>>>> Wei, what do you think, does it make sense?
> >>>>>
> >>>>>
> >>>>> I agree with you, but it is sure that the original code has
> >>>>> offloads overwrite
> >>>> issue.
> >>>>> What is your suggestion for code implement?
> >>>>> I find that Thomas has apply it, if you has other idea, maybe you
> >>>>> has to
> >>>> commit patch base to this patch.
> >>>>
> >>>> Hi Wei,
> >>>>
> >>>> Yes this needs to be incremental fix to existing code.
> >>>>
> >>>> Queue specific offload can be altered either by providing Rx/Tx
> >>>> offload as command line argument [1] (port configs set to each
> >>>> queues) or via testpmd commands [2].
> >>>> Does it make sense to set a global flag when one of above occurs
> >>>> and use default config only if it is not set?
> >>>
> >>> I  AGREE with you to submit an incremental fix, and it make sense to
> >>> set a global flag when one of above occurs and use  default config
> >>> only if it is
> >> not set when implement code, but I do not have time to prepare such a
> >> patch by now, so maybe later or some else.
> >>
> >> I see, can you submit a public defect to record the issue, so it can
> >> be addressed later without forgotten?
> >
> > Sure, but what is a public defect patch? Do you mean I need to update
> > some doc? Can you give me a link as an example
> 
> No documentation, please create an issue in public DPDK bug tracker:
> https://bugs.dpdk.org/
> 
> 
> >
> >
> >>
> >>>
> >>>>
> >>>> [1]
> >>>> Tx
> >>>>   tx-offloads
> >>>> Rx
> >>>>   disable-crc-strip
> >>>>   enable-lro
> >>>>   enable-scatter
> >>>>   enable-rx-cksum
> >>>>   enable-rx-timestamp
> >>>>   enable-hw-vlan
> >>>>   enable-hw-vlan-filter
> >>>>   enable-hw-vlan-strip
> >>>>   enable-hw-vlan-extend
> >>>>
> >>>> [2]
> >>>> "port config <port_id> rx_offload ..."
> >>>> "port <port_id> rxq <queue_id> rx_offload ..."
> >>>> "port config <port_id> tx_offload ..."
> >>>> "port <port_id> txq <queue_id> tx_offload ..."
> >>>>
> >>>
> >

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 5/9/2019 8:20 AM, Wei Zhao wrote:
> There is an error in function rxtx_port_config(), which may overwrite
> offloads configuration get from function launch_args_parse() when run
> testpmd app. So rxtx_port_config() should do "or" for port offloads.
> 
> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> cc: stable@dpdk.org
> 
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
>  app/test-pmd/testpmd.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 6fbfd29..f0061d9 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2809,9 +2809,12 @@ static void
>  rxtx_port_config(struct rte_port *port)
>  {
>  	uint16_t qid;
> +	uint64_t offloads;
>  
>  	for (qid = 0; qid < nb_rxq; qid++) {
> +		offloads = port->rx_conf[qid].offloads;
>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> +		port->rx_conf[qid].offloads |= offloads;

While talking with Kevin, he pointed out the error in this code.

We are updating queue level offloads, with whatever in the 'offloads' and it can
be non-queue level offloads in it, next time ethdev API called these values are
caught by the API checks and causing an error.

It looks like port level offload flags needs to be masted out before writing to
queue level 'offloads' variable.

>  
>  		/* Check if any Rx parameters have been passed */
>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET)
> @@ -2833,7 +2836,9 @@ rxtx_port_config(struct rte_port *port)
>  	}
>  
>  	for (qid = 0; qid < nb_txq; qid++) {
> +		offloads = port->tx_conf[qid].offloads;
>  		port->tx_conf[qid] = port->dev_info.default_txconf;
> +		port->tx_conf[qid].offloads |= offloads;
>  
>  		/* Check if any Tx parameters have been passed */
>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> 

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

Hi,  Ferruh

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, June 11, 2019 10:37 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> > There is an error in function rxtx_port_config(), which may overwrite
> > offloads configuration get from function launch_args_parse() when run
> > testpmd app. So rxtx_port_config() should do "or" for port offloads.
> >
> > Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> > cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 6fbfd29..f0061d9 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2809,9 +2809,12 @@ static void
> >  rxtx_port_config(struct rte_port *port)  {
> >  	uint16_t qid;
> > +	uint64_t offloads;
> >
> >  	for (qid = 0; qid < nb_rxq; qid++) {
> > +		offloads = port->rx_conf[qid].offloads;
> >  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> > +		port->rx_conf[qid].offloads |= offloads;
> 
> While talking with Kevin, he pointed out the error in this code.
> 
> We are updating queue level offloads, with whatever in the 'offloads' and it can
> be non-queue level offloads in it, next time ethdev API called these values are
> caught by the API checks and causing an error.
> 
> It looks like port level offload flags needs to be masted out before writing to
> queue level 'offloads' variable.

By now, the default offloads value of i40e and ixgbe is 0, so it do not cause error when set port offloads to queue.
But if it is not 0, maybe that will happen as you said.
  
> 
> >
> >  		/* Check if any Rx parameters have been passed */
> >  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
> +2836,9 @@
> > rxtx_port_config(struct rte_port *port)
> >  	}
> >
> >  	for (qid = 0; qid < nb_txq; qid++) {
> > +		offloads = port->tx_conf[qid].offloads;
> >  		port->tx_conf[qid] = port->dev_info.default_txconf;
> > +		port->tx_conf[qid].offloads |= offloads;
> >
> >  		/* Check if any Tx parameters have been passed */
> >  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, June 11, 2019 10:37 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> > There is an error in function rxtx_port_config(), which may overwrite
> > offloads configuration get from function launch_args_parse() when run
> > testpmd app. So rxtx_port_config() should do "or" for port offloads.
> >
> > Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> > cc: stable@dpdk.org
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > ---
> >  app/test-pmd/testpmd.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 6fbfd29..f0061d9 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2809,9 +2809,12 @@ static void
> >  rxtx_port_config(struct rte_port *port)  {
> >  	uint16_t qid;
> > +	uint64_t offloads;
> >
> >  	for (qid = 0; qid < nb_rxq; qid++) {
> > +		offloads = port->rx_conf[qid].offloads;
> >  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> > +		port->rx_conf[qid].offloads |= offloads;
> 
> While talking with Kevin, he pointed out the error in this code.
> 
> We are updating queue level offloads, with whatever in the 'offloads' and it can
> be non-queue level offloads in it, next time ethdev API called these values are
> caught by the API checks and causing an error.
> 
> It looks like port level offload flags needs to be masted out before writing to
> queue level 'offloads' variable.


By the way, this error in not introduced in this patch, it seems has exist long before this patch.
This patch is just fix for overwrite problem.



> 
> >
> >  		/* Check if any Rx parameters have been passed */
> >  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
> +2836,9 @@
> > rxtx_port_config(struct rte_port *port)
> >  	}
> >
> >  	for (qid = 0; qid < nb_txq; qid++) {
> > +		offloads = port->tx_conf[qid].offloads;
> >  		port->tx_conf[qid] = port->dev_info.default_txconf;
> > +		port->tx_conf[qid].offloads |= offloads;
> >
> >  		/* Check if any Tx parameters have been passed */
> >  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 6/12/2019 2:17 AM, Zhao1, Wei wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, June 11, 2019 10:37 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
>> default configuration
>>
>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
>>> There is an error in function rxtx_port_config(), which may overwrite
>>> offloads configuration get from function launch_args_parse() when run
>>> testpmd app. So rxtx_port_config() should do "or" for port offloads.
>>>
>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
>>> cc: stable@dpdk.org
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>> ---
>>>  app/test-pmd/testpmd.c | 5 +++++
>>>  1 file changed, 5 insertions(+)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 6fbfd29..f0061d9 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2809,9 +2809,12 @@ static void
>>>  rxtx_port_config(struct rte_port *port)  {
>>>  	uint16_t qid;
>>> +	uint64_t offloads;
>>>
>>>  	for (qid = 0; qid < nb_rxq; qid++) {
>>> +		offloads = port->rx_conf[qid].offloads;
>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
>>> +		port->rx_conf[qid].offloads |= offloads;
>>
>> While talking with Kevin, he pointed out the error in this code.
>>
>> We are updating queue level offloads, with whatever in the 'offloads' and it can
>> be non-queue level offloads in it, next time ethdev API called these values are
>> caught by the API checks and causing an error.
>>
>> It looks like port level offload flags needs to be masted out before writing to
>> queue level 'offloads' variable.
> 
> 
> By the way, this error in not introduced in this patch, it seems has exist long before this patch.
> This patch is just fix for overwrite problem.

I disagree, writing 'offloads' to "rx_conf[].offloads" without checking if they
queue offloads or not causing this problem. And that write introduced in this patch.


> 
> 
> 
>>
>>>
>>>  		/* Check if any Rx parameters have been passed */
>>>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
>> +2836,9 @@
>>> rxtx_port_config(struct rte_port *port)
>>>  	}
>>>
>>>  	for (qid = 0; qid < nb_txq; qid++) {
>>> +		offloads = port->tx_conf[qid].offloads;
>>>  		port->tx_conf[qid] = port->dev_info.default_txconf;
>>> +		port->tx_conf[qid].offloads |= offloads;
>>>
>>>  		/* Check if any Tx parameters have been passed */
>>>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
>>>
> 

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Zhao1, Wei]

> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Friday, June 14, 2019 11:42 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
> default configuration
> 
> On 6/12/2019 2:17 AM, Zhao1, Wei wrote:
> >
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh
> >> Sent: Tuesday, June 11, 2019 10:37 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
> >> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
> >> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
> >> overwrite by default configuration
> >>
> >> On 5/9/2019 8:20 AM, Wei Zhao wrote:
> >>> There is an error in function rxtx_port_config(), which may
> >>> overwrite offloads configuration get from function
> >>> launch_args_parse() when run testpmd app. So rxtx_port_config() should
> do "or" for port offloads.
> >>>
> >>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
> >>> cc: stable@dpdk.org
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> ---
> >>>  app/test-pmd/testpmd.c | 5 +++++
> >>>  1 file changed, 5 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> 6fbfd29..f0061d9 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -2809,9 +2809,12 @@ static void
> >>>  rxtx_port_config(struct rte_port *port)  {
> >>>  	uint16_t qid;
> >>> +	uint64_t offloads;
> >>>
> >>>  	for (qid = 0; qid < nb_rxq; qid++) {
> >>> +		offloads = port->rx_conf[qid].offloads;
> >>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
> >>> +		port->rx_conf[qid].offloads |= offloads;
> >>
> >> While talking with Kevin, he pointed out the error in this code.
> >>
> >> We are updating queue level offloads, with whatever in the 'offloads'
> >> and it can be non-queue level offloads in it, next time ethdev API
> >> called these values are caught by the API checks and causing an error.
> >>
> >> It looks like port level offload flags needs to be masted out before
> >> writing to queue level 'offloads' variable.
> >
> >
> > By the way, this error in not introduced in this patch, it seems has exist long
> before this patch.
> > This patch is just fix for overwrite problem.
> 
> I disagree, writing 'offloads' to "rx_conf[].offloads" without checking if they
> queue offloads or not causing this problem. And that write introduced in this
> patch.
> 

But if delete the patch I submitted, this bug still exists there.

> 
> >
> >
> >
> >>
> >>>
> >>>  		/* Check if any Rx parameters have been passed */
> >>>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
> >> +2836,9 @@
> >>> rxtx_port_config(struct rte_port *port)
> >>>  	}
> >>>
> >>>  	for (qid = 0; qid < nb_txq; qid++) {
> >>> +		offloads = port->tx_conf[qid].offloads;
> >>>  		port->tx_conf[qid] = port->dev_info.default_txconf;
> >>> +		port->tx_conf[qid].offloads |= offloads;
> >>>
> >>>  		/* Check if any Tx parameters have been passed */
> >>>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >>>
> >

Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by default configuration

[Ferruh Yigit]

On 6/17/2019 2:51 AM, Zhao1, Wei wrote:
> 
> 
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Friday, June 14, 2019 11:42 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads overwrite by
>> default configuration
>>
>> On 6/12/2019 2:17 AM, Zhao1, Wei wrote:
>>>
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh
>>>> Sent: Tuesday, June 11, 2019 10:37 PM
>>>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>>>> Cc: stable@dpdk.org; Peng, Yuan <yuan.peng@intel.com>; Lu, Wenzhuo
>>>> <wenzhuo.lu@intel.com>; Kevin Traynor <ktraynor@redhat.com>
>>>> Subject: Re: [dpdk-stable] [PATCH] app/testpmd: fix offloads
>>>> overwrite by default configuration
>>>>
>>>> On 5/9/2019 8:20 AM, Wei Zhao wrote:
>>>>> There is an error in function rxtx_port_config(), which may
>>>>> overwrite offloads configuration get from function
>>>>> launch_args_parse() when run testpmd app. So rxtx_port_config() should
>> do "or" for port offloads.
>>>>>
>>>>> Fixes: d44f8a485f5d ("app/testpmd: enable per queue configure")
>>>>> cc: stable@dpdk.org
>>>>>
>>>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>>>> ---
>>>>>  app/test-pmd/testpmd.c | 5 +++++
>>>>>  1 file changed, 5 insertions(+)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> 6fbfd29..f0061d9 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2809,9 +2809,12 @@ static void
>>>>>  rxtx_port_config(struct rte_port *port)  {
>>>>>  	uint16_t qid;
>>>>> +	uint64_t offloads;
>>>>>
>>>>>  	for (qid = 0; qid < nb_rxq; qid++) {
>>>>> +		offloads = port->rx_conf[qid].offloads;
>>>>>  		port->rx_conf[qid] = port->dev_info.default_rxconf;
>>>>> +		port->rx_conf[qid].offloads |= offloads;
>>>>
>>>> While talking with Kevin, he pointed out the error in this code.
>>>>
>>>> We are updating queue level offloads, with whatever in the 'offloads'
>>>> and it can be non-queue level offloads in it, next time ethdev API
>>>> called these values are caught by the API checks and causing an error.
>>>>
>>>> It looks like port level offload flags needs to be masted out before
>>>> writing to queue level 'offloads' variable.
>>>
>>>
>>> By the way, this error in not introduced in this patch, it seems has exist long
>> before this patch.
>>> This patch is just fix for overwrite problem.
>>
>> I disagree, writing 'offloads' to "rx_conf[].offloads" without checking if they
>> queue offloads or not causing this problem. And that write introduced in this
>> patch.
>>
> 
> But if delete the patch I submitted, this bug still exists there.

OK, after checking again, the defect was there as you said, but your patch makes
it visible.

How to reproduce the problem:

testpmd> port stop all
testpmd> port config all crc-strip on
testpmd> port start all
Ethdev port_id=0 rx_queue_id=0, new added offloads 0x10000 must be within
per-queue offload capabilities 0x0 in rte_eth_rx_queue_setup()

The failure is coming from 'rte_eth_rx_queue_setup()' and a valid error, a "port
level offload" can't be set to individual queues without setting in port level,
that is what happening here.
"port config all crc-strip on" removes the 'keep_crc' offload from port but not
from queues, so when 'rte_eth_rx_queue_setup()' called it complains about it.

Before your patch "queue offloads" were overwritten and this error was not observed.

The solution can be updating "port config all xxxx on|off" command to updates
queue offload values too, but even better we can remove it completely:

There are already different testpmd commands for same thing:
"port config <port_id> rx_offload <offload>"
"port config <port_id> tx_offload <offload>"
"port <port_id> txq <queue_id> rx_offload <offload>"
"port <port_id> txq <queue_id> tx_offload <offload>"

For example using "port config 0 rx_offload crc-strip on" will work without
error since it sets the port and queue offload properly.

Only missing thing in above commands are "all" parameter. I suggest removing
"port config all <offload> on|off" and adding "all" support to above commands.


> 
>>
>>>
>>>
>>>
>>>>
>>>>>
>>>>>  		/* Check if any Rx parameters have been passed */
>>>>>  		if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2833,7
>>>> +2836,9 @@
>>>>> rxtx_port_config(struct rte_port *port)
>>>>>  	}
>>>>>
>>>>>  	for (qid = 0; qid < nb_txq; qid++) {
>>>>> +		offloads = port->tx_conf[qid].offloads;
>>>>>  		port->tx_conf[qid] = port->dev_info.default_txconf;
>>>>> +		port->tx_conf[qid].offloads |= offloads;
>>>>>
>>>>>  		/* Check if any Tx parameters have been passed */
>>>>>  		if (tx_pthresh != RTE_PMD_PARAM_UNSET)
>>>>>
>>>
>