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

["Zhao1, Wei" <wei.zhao1@intel.com>]

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 ..."
> >>>>
> >>>
> >