app/testpmd: fix offloads default set error
Checks
Commit Message
There is no need to use default offloads configuration
if offloads configuration has been pass down from uplayer.
Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
---
app/test-pmd/testpmd.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Comments
On 10/06/2019 07:45, Wei Zhao wrote:
> There is no need to use default offloads configuration
> if offloads configuration has been pass down from uplayer.
>
It doesn't seem like the right fix to me. It means if a user
enable/disable one offload in the command line they are silently
changing from the default behaviour of other offloads? I could
understand if the user set all offloads in one go through a mask but not
when they are set seemingly independent.
It looks like you need to track which ones have been enabled/disabled by
the user, and use the default for for the other offloads.
> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port config")
>
> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> ---
> app/test-pmd/testpmd.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 0148b0a..b5f5801 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2815,7 +2815,8 @@ rxtx_port_config(struct rte_port *port)
> 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;
> + if (offloads)
> + port->rx_conf[qid].offloads = offloads;
>
> /* Check if any Rx parameters have been passed */
> if (rx_pthresh != RTE_PMD_PARAM_UNSET)
> @@ -2839,7 +2840,8 @@ 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;
> + if (offloads)
> + port->tx_conf[qid].offloads = offloads;
>
> /* Check if any Tx parameters have been passed */
> if (tx_pthresh != RTE_PMD_PARAM_UNSET)
>
Hi, Kevin Traynor
This patch is based on Yigit Ferruh's suggestion in the following mail.
https://mails.dpdk.org/archives/dev/2019-May/132178.html
If we take your propose, it means we do need any patch at all.
Because the code implement now is just that scheme, we will do "or" for offloads.
Hello Ferruh,What is your comment?
> -----Original Message-----
> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> Sent: Monday, June 10, 2019 6:10 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set error
>
> On 10/06/2019 07:45, Wei Zhao wrote:
> > There is no need to use default offloads configuration if offloads
> > configuration has been pass down from uplayer.
> >
>
> It doesn't seem like the right fix to me. It means if a user enable/disable one
> offload in the command line they are silently changing from the default
> behaviour of other offloads? I could understand if the user set all offloads in
> one go through a mask but not when they are set seemingly independent.
>
> It looks like you need to track which ones have been enabled/disabled by the
> user, and use the default for for the other offloads.
>
> > Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port
> > config")
> >
> > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > ---
> > app/test-pmd/testpmd.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 0148b0a..b5f5801 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2815,7 +2815,8 @@ rxtx_port_config(struct rte_port *port)
> > 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;
> > + if (offloads)
> > + port->rx_conf[qid].offloads = offloads;
> >
> > /* Check if any Rx parameters have been passed */
> > if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2839,7
> +2840,8 @@
> > 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;
> > + if (offloads)
> > + port->tx_conf[qid].offloads = offloads;
> >
> > /* Check if any Tx parameters have been passed */
> > if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >
"If we take that propose, it means we do not need any patch at all."
Sorry for typo.
> -----Original Message-----
> From: Zhao1, Wei
> Sent: Tuesday, June 11, 2019 9:29 AM
> To: 'Kevin Traynor' <ktraynor@redhat.com>; dev@dpdk.org
> Cc: stable@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
> Subject: RE: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set error
>
> Hi, Kevin Traynor
> This patch is based on Yigit Ferruh's suggestion in the following mail.
> https://mails.dpdk.org/archives/dev/2019-May/132178.html
> If we take your propose, it means we do need any patch at all.
> Because the code implement now is just that scheme, we will do "or" for
> offloads.
> Hello Ferruh,What is your comment?
>
> > -----Original Message-----
> > From: Kevin Traynor [mailto:ktraynor@redhat.com]
> > Sent: Monday, June 10, 2019 6:10 PM
> > To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> > Cc: stable@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
> > Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set
> > error
> >
> > On 10/06/2019 07:45, Wei Zhao wrote:
> > > There is no need to use default offloads configuration if offloads
> > > configuration has been pass down from uplayer.
> > >
> >
> > It doesn't seem like the right fix to me. It means if a user
> > enable/disable one offload in the command line they are silently
> > changing from the default behaviour of other offloads? I could
> > understand if the user set all offloads in one go through a mask but not when
> they are set seemingly independent.
> >
> > It looks like you need to track which ones have been enabled/disabled
> > by the user, and use the default for for the other offloads.
> >
> > > Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port
> > > config")
> > >
> > > Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> > > ---
> > > app/test-pmd/testpmd.c | 6 ++++--
> > > 1 file changed, 4 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > > 0148b0a..b5f5801 100644
> > > --- a/app/test-pmd/testpmd.c
> > > +++ b/app/test-pmd/testpmd.c
> > > @@ -2815,7 +2815,8 @@ rxtx_port_config(struct rte_port *port)
> > > 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;
> > > + if (offloads)
> > > + port->rx_conf[qid].offloads = offloads;
> > >
> > > /* Check if any Rx parameters have been passed */
> > > if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2839,7
> > +2840,8 @@
> > > 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;
> > > + if (offloads)
> > > + port->tx_conf[qid].offloads = offloads;
> > >
> > > /* Check if any Tx parameters have been passed */
> > > if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> > >
On 6/11/2019 2:28 AM, Zhao1, Wei wrote:
> Hi, Kevin Traynor
> This patch is based on Yigit Ferruh's suggestion in the following mail.
> https://mails.dpdk.org/archives/dev/2019-May/132178.html
> If we take your propose, it means we do need any patch at all.
> Because the code implement now is just that scheme, we will do "or" for offloads.
> Hello Ferruh,What is your comment?
My comment was if the user not set the queue offload use the default one, so I
think we don't need to track individual offload options.
But also there is slightly difference between user not set the value and
'offloads' being zero. What if user set the offloads value to '0' to disable all
offloads, it will use default values in this case.
>
>> -----Original Message-----
>> From: Kevin Traynor [mailto:ktraynor@redhat.com]
>> Sent: Monday, June 10, 2019 6:10 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
>> Cc: stable@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set error
>>
>> On 10/06/2019 07:45, Wei Zhao wrote:
>>> There is no need to use default offloads configuration if offloads
>>> configuration has been pass down from uplayer.
>>>
>>
>> It doesn't seem like the right fix to me. It means if a user enable/disable one
>> offload in the command line they are silently changing from the default
>> behaviour of other offloads? I could understand if the user set all offloads in
>> one go through a mask but not when they are set seemingly independent.
>>
>> It looks like you need to track which ones have been enabled/disabled by the
>> user, and use the default for for the other offloads.
>>
>>> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port
>>> config")
>>>
>>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
>>> ---
>>> app/test-pmd/testpmd.c | 6 ++++--
>>> 1 file changed, 4 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 0148b0a..b5f5801 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2815,7 +2815,8 @@ rxtx_port_config(struct rte_port *port)
>>> 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;
>>> + if (offloads)
>>> + port->rx_conf[qid].offloads = offloads;
>>>
>>> /* Check if any Rx parameters have been passed */
>>> if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2839,7
>> +2840,8 @@
>>> 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;
>>> + if (offloads)
>>> + port->tx_conf[qid].offloads = offloads;
>>>
>>> /* Check if any Tx parameters have been passed */
>>> if (tx_pthresh != RTE_PMD_PARAM_UNSET)
>>>
>
> -----Original Message-----
> From: Yigit, Ferruh
> Sent: Tuesday, June 11, 2019 10:42 PM
> To: Zhao1, Wei <wei.zhao1@intel.com>; Kevin Traynor
> <ktraynor@redhat.com>; dev@dpdk.org
> Cc: stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set error
>
> On 6/11/2019 2:28 AM, Zhao1, Wei wrote:
> > Hi, Kevin Traynor
> > This patch is based on Yigit Ferruh's suggestion in the following mail.
> > https://mails.dpdk.org/archives/dev/2019-May/132178.html
> > If we take your propose, it means we do need any patch at all.
> > Because the code implement now is just that scheme, we will do "or" for
> offloads.
> > Hello Ferruh,What is your comment?
>
> My comment was if the user not set the queue offload use the default one, so I
> think we don't need to track individual offload options.
>
> But also there is slightly difference between user not set the value and
> 'offloads' being zero. What if user set the offloads value to '0' to disable all
> offloads, it will use default values in this case.
>
Yes, we can not distinguish between not set the value and using 0 to disable it, so if the bit is 0 how can we
Know the actual purpose of users? I think it is hard track individual offload as Kevin Traynor said.
> >
> >> -----Original Message-----
> >> From: Kevin Traynor [mailto:ktraynor@redhat.com]
> >> Sent: Monday, June 10, 2019 6:10 PM
> >> To: Zhao1, Wei <wei.zhao1@intel.com>; dev@dpdk.org
> >> Cc: stable@dpdk.org; Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set
> >> error
> >>
> >> On 10/06/2019 07:45, Wei Zhao wrote:
> >>> There is no need to use default offloads configuration if offloads
> >>> configuration has been pass down from uplayer.
> >>>
> >>
> >> It doesn't seem like the right fix to me. It means if a user
> >> enable/disable one offload in the command line they are silently
> >> changing from the default behaviour of other offloads? I could
> >> understand if the user set all offloads in one go through a mask but not
> when they are set seemingly independent.
> >>
> >> It looks like you need to track which ones have been enabled/disabled
> >> by the user, and use the default for for the other offloads.
> >>
> >>> Fixes: 5e91aeef218c ("app/testpmd: fix offload flags after port
> >>> config")
> >>>
> >>> Signed-off-by: Wei Zhao <wei.zhao1@intel.com>
> >>> ---
> >>> app/test-pmd/testpmd.c | 6 ++++--
> >>> 1 file changed, 4 insertions(+), 2 deletions(-)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> 0148b0a..b5f5801 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -2815,7 +2815,8 @@ rxtx_port_config(struct rte_port *port)
> >>> 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;
> >>> + if (offloads)
> >>> + port->rx_conf[qid].offloads = offloads;
> >>>
> >>> /* Check if any Rx parameters have been passed */
> >>> if (rx_pthresh != RTE_PMD_PARAM_UNSET) @@ -2839,7
> >> +2840,8 @@
> >>> 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;
> >>> + if (offloads)
> >>> + port->tx_conf[qid].offloads = offloads;
> >>>
> >>> /* Check if any Tx parameters have been passed */
> >>> if (tx_pthresh != RTE_PMD_PARAM_UNSET)
> >>>
> >
On 6/12/2019 2:04 AM, Zhao1, Wei wrote:
>
>
>> -----Original Message-----
>> From: Yigit, Ferruh
>> Sent: Tuesday, June 11, 2019 10:42 PM
>> To: Zhao1, Wei <wei.zhao1@intel.com>; Kevin Traynor
>> <ktraynor@redhat.com>; dev@dpdk.org
>> Cc: stable@dpdk.org
>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set error
>>
>> On 6/11/2019 2:28 AM, Zhao1, Wei wrote:
>>> Hi, Kevin Traynor
>>> This patch is based on Yigit Ferruh's suggestion in the following mail.
>>> https://mails.dpdk.org/archives/dev/2019-May/132178.html
>>> If we take your propose, it means we do need any patch at all.
>>> Because the code implement now is just that scheme, we will do "or" for
>> offloads.
>>> Hello Ferruh,What is your comment?
>>
>> My comment was if the user not set the queue offload use the default one, so I
>> think we don't need to track individual offload options.
>>
>> But also there is slightly difference between user not set the value and
>> 'offloads' being zero. What if user set the offloads value to '0' to disable all
>> offloads, it will use default values in this case.
>>
>
> Yes, we can not distinguish between not set the value and using 0 to disable it, so if the bit is 0 how can we
> Know the actual purpose of users? I think it is hard track individual offload as Kevin Traynor said.
Yes it is hard to track, and I believe this is better than the existing code, so:
Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
On 7/2/2019 6:56 PM, Ferruh Yigit wrote:
> On 6/12/2019 2:04 AM, Zhao1, Wei wrote:
>>
>>
>>> -----Original Message-----
>>> From: Yigit, Ferruh
>>> Sent: Tuesday, June 11, 2019 10:42 PM
>>> To: Zhao1, Wei <wei.zhao1@intel.com>; Kevin Traynor
>>> <ktraynor@redhat.com>; dev@dpdk.org
>>> Cc: stable@dpdk.org
>>> Subject: Re: [dpdk-dev] [PATCH] app/testpmd: fix offloads default set error
>>>
>>> On 6/11/2019 2:28 AM, Zhao1, Wei wrote:
>>>> Hi, Kevin Traynor
>>>> This patch is based on Yigit Ferruh's suggestion in the following mail.
>>>> https://mails.dpdk.org/archives/dev/2019-May/132178.html
>>>> If we take your propose, it means we do need any patch at all.
>>>> Because the code implement now is just that scheme, we will do "or" for
>>> offloads.
>>>> Hello Ferruh,What is your comment?
>>>
>>> My comment was if the user not set the queue offload use the default one, so I
>>> think we don't need to track individual offload options.
>>>
>>> But also there is slightly difference between user not set the value and
>>> 'offloads' being zero. What if user set the offloads value to '0' to disable all
>>> offloads, it will use default values in this case.
>>>
>>
>> Yes, we can not distinguish between not set the value and using 0 to disable it, so if the bit is 0 how can we
>> Know the actual purpose of users? I think it is hard track individual offload as Kevin Traynor said.
>
> Yes it is hard to track, and I believe this is better than the existing code, so:
>
> Reviewed-by: Ferruh Yigit <ferruh.yigit@intel.com>
>
Applied to dpdk-next-net/master, thanks.
@@ -2815,7 +2815,8 @@ rxtx_port_config(struct rte_port *port)
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;
+ if (offloads)
+ port->rx_conf[qid].offloads = offloads;
/* Check if any Rx parameters have been passed */
if (rx_pthresh != RTE_PMD_PARAM_UNSET)
@@ -2839,7 +2840,8 @@ 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;
+ if (offloads)
+ port->tx_conf[qid].offloads = offloads;
/* Check if any Tx parameters have been passed */
if (tx_pthresh != RTE_PMD_PARAM_UNSET)