[v8,2/2] app/testpmd: fix testpmd doesn't show RSS hash offload
Checks
Commit Message
The driver may change offloads info into dev->data->dev_conf
in dev_configure which may cause port->dev_conf and port->rx_conf
contain outdated values.
This patch updates the offloads info if it changes to fix this issue.
Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
Signed-off-by: Jie Wang <jie1x.wang@intel.com>
---
app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
app/test-pmd/testpmd.h | 2 ++
app/test-pmd/util.c | 15 +++++++++++++++
3 files changed, 51 insertions(+)
Comments
> -----Original Message-----
> From: Wang, Jie1X <jie1x.wang@intel.com>
> Sent: Friday, August 27, 2021 16:18
> To: dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru;
> thomas@monjalon.net; Yigit, Ferruh <ferruh.yigit@intel.com>; Wang, Jie1X
> <jie1x.wang@intel.com>
> Subject: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
> offload
>
> The driver may change offloads info into dev->data->dev_conf in dev_configure
> which may cause port->dev_conf and port->rx_conf contain outdated values.
>
> This patch updates the offloads info if it changes to fix this issue.
>
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
> app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
> app/test-pmd/testpmd.h | 2 ++
> app/test-pmd/util.c | 15 +++++++++++++++
> 3 files changed, 51 insertions(+)
>
Acked-by: Xiaoyun Li <xiaoyun.li@intel.com>
On 8/27/2021 9:17 AM, Jie Wang wrote:
> The driver may change offloads info into dev->data->dev_conf
> in dev_configure which may cause port->dev_conf and port->rx_conf
> contain outdated values.
>
> This patch updates the offloads info if it changes to fix this issue.
>
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> ---
> app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
> app/test-pmd/testpmd.h | 2 ++
> app/test-pmd/util.c | 15 +++++++++++++++
> 3 files changed, 51 insertions(+)
>
> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c
> index 6cbe9ba3c8..bd67291160 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -2461,6 +2461,9 @@ start_port(portid_t pid)
> }
>
> if (port->need_reconfig > 0) {
> + struct rte_eth_conf dev_conf_info;
> + int k;
> +
> port->need_reconfig = 0;
>
> if (flow_isolate_all) {
> @@ -2498,6 +2501,37 @@ start_port(portid_t pid)
> port->need_reconfig = 1;
> return -1;
> }
> + /* get rte_eth_conf info */
> + if (0 !=
> + eth_dev_conf_info_get_print_err(pi,
> + &dev_conf_info)) {
> + fprintf(stderr,
> + "port %d can not get device configuration info\n",
> + pi);
> + return -1;
> + }
> + /* Apply Rx offloads configuration */
> + if (dev_conf_info.rxmode.offloads !=
> + port->dev_conf.rxmode.offloads) {
> + port->dev_conf.rxmode.offloads =
> + dev_conf_info.rxmode.offloads;
> + for (k = 0;
> + k < port->dev_info.max_rx_queues;
> + k++)
> + port->rx_conf[k].offloads =
> + dev_conf_info.rxmode.offloads;
> + }
> + /* Apply Tx offloads configuration */
> + if (dev_conf_info.txmode.offloads !=
> + port->dev_conf.txmode.offloads) {
> + port->dev_conf.txmode.offloads =
> + dev_conf_info.txmode.offloads;
> + for (k = 0;
> + k < port->dev_info.max_tx_queues;
> + k++)
> + port->tx_conf[k].offloads =
> + dev_conf_info.txmode.offloads;
> + }
> }
Above implementation gets the configuration from device and applies it to the
testpmd configuration.
Instead, what about a long level target to get rid of testpmd specific copy of
the configuration and rely and the config provided by devices. @Xiaoyun, what do
you think, does this make sense?
So instead of above code, update where RSS hash offload information printed to
use device retrieved config instead of testpmd config, will it work?
Hi
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Thursday, September 9, 2021 00:51
> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org; Li, Xiaoyun
> <xiaoyun.li@intel.com>
> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net
> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
> offload
>
> On 8/27/2021 9:17 AM, Jie Wang wrote:
> > The driver may change offloads info into dev->data->dev_conf in
> > dev_configure which may cause port->dev_conf and port->rx_conf contain
> > outdated values.
> >
> > This patch updates the offloads info if it changes to fix this issue.
> >
> > Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> >
> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> > ---
> > app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
> > app/test-pmd/testpmd.h | 2 ++
> > app/test-pmd/util.c | 15 +++++++++++++++
> > 3 files changed, 51 insertions(+)
> >
> > diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> > 6cbe9ba3c8..bd67291160 100644
> > --- a/app/test-pmd/testpmd.c
> > +++ b/app/test-pmd/testpmd.c
> > @@ -2461,6 +2461,9 @@ start_port(portid_t pid)
> > }
> >
> > if (port->need_reconfig > 0) {
> > + struct rte_eth_conf dev_conf_info;
> > + int k;
> > +
> > port->need_reconfig = 0;
> >
> > if (flow_isolate_all) {
> > @@ -2498,6 +2501,37 @@ start_port(portid_t pid)
> > port->need_reconfig = 1;
> > return -1;
> > }
> > + /* get rte_eth_conf info */
> > + if (0 !=
> > + eth_dev_conf_info_get_print_err(pi,
> > + &dev_conf_info)) {
> > + fprintf(stderr,
> > + "port %d can not get device
> configuration info\n",
> > + pi);
> > + return -1;
> > + }
> > + /* Apply Rx offloads configuration */
> > + if (dev_conf_info.rxmode.offloads !=
> > + port->dev_conf.rxmode.offloads) {
> > + port->dev_conf.rxmode.offloads =
> > + dev_conf_info.rxmode.offloads;
> > + for (k = 0;
> > + k < port->dev_info.max_rx_queues;
> > + k++)
> > + port->rx_conf[k].offloads =
> > +
> dev_conf_info.rxmode.offloads;
> > + }
> > + /* Apply Tx offloads configuration */
> > + if (dev_conf_info.txmode.offloads !=
> > + port->dev_conf.txmode.offloads) {
> > + port->dev_conf.txmode.offloads =
> > + dev_conf_info.txmode.offloads;
> > + for (k = 0;
> > + k < port->dev_info.max_tx_queues;
> > + k++)
> > + port->tx_conf[k].offloads =
> > +
> dev_conf_info.txmode.offloads;
> > + }
> > }
>
> Above implementation gets the configuration from device and applies it to the
> testpmd configuration.
>
> Instead, what about a long level target to get rid of testpmd specific copy of the
> configuration and rely and the config provided by devices. @Xiaoyun, what do
> you think, does this make sense?
You mean remove port->dev_conf and rx/tx_conf completely in the future? Or keep it in initial stage?
Now, port->dev_conf will take global tx/rx_mode, fdir_conf and change some based on dev_info capabilities. And then use dev_configure to apply them for device.
After this, actually, dev->data->dev_conf contains all device configuration.
So It seems it's OK to remove port->dev_conf completely. Just testpmd needs to be refactored a lot and regression test in case of issues.
But from long term view, it's good to keep one source and avoid copy.
As for rx/tx_conf, it takes device default tx/rx_conf in dev_info and some settings in testpmd parameters also offloads from dev_conf.
So keep port->rx/tx_conf? But then it still needs copy from dev_conf since this may change.
>
> So instead of above code, update where RSS hash offload information printed to
> use device retrieved config instead of testpmd config, will it work?
It's OK for device offload configurations.
But queue offloads are a bit tricky since dev->data->dev_conf doesn't include queue conf.
And it's not fair to use device offload configurations for queue offloads since user can use cmdline to config queue offload and that info can only be saved in port->rx/tx_conf and configure the device in setup_queue.
On 9/9/2021 4:31 AM, Li, Xiaoyun wrote:
> Hi
>
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Thursday, September 9, 2021 00:51
>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org; Li, Xiaoyun
>> <xiaoyun.li@intel.com>
>> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net
>> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
>> offload
>>
>> On 8/27/2021 9:17 AM, Jie Wang wrote:
>>> The driver may change offloads info into dev->data->dev_conf in
>>> dev_configure which may cause port->dev_conf and port->rx_conf contain
>>> outdated values.
>>>
>>> This patch updates the offloads info if it changes to fix this issue.
>>>
>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>>>
>>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>>> ---
>>> app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
>>> app/test-pmd/testpmd.h | 2 ++
>>> app/test-pmd/util.c | 15 +++++++++++++++
>>> 3 files changed, 51 insertions(+)
>>>
>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>> 6cbe9ba3c8..bd67291160 100644
>>> --- a/app/test-pmd/testpmd.c
>>> +++ b/app/test-pmd/testpmd.c
>>> @@ -2461,6 +2461,9 @@ start_port(portid_t pid)
>>> }
>>>
>>> if (port->need_reconfig > 0) {
>>> + struct rte_eth_conf dev_conf_info;
>>> + int k;
>>> +
>>> port->need_reconfig = 0;
>>>
>>> if (flow_isolate_all) {
>>> @@ -2498,6 +2501,37 @@ start_port(portid_t pid)
>>> port->need_reconfig = 1;
>>> return -1;
>>> }
>>> + /* get rte_eth_conf info */
>>> + if (0 !=
>>> + eth_dev_conf_info_get_print_err(pi,
>>> + &dev_conf_info)) {
>>> + fprintf(stderr,
>>> + "port %d can not get device
>> configuration info\n",
>>> + pi);
>>> + return -1;
>>> + }
>>> + /* Apply Rx offloads configuration */
>>> + if (dev_conf_info.rxmode.offloads !=
>>> + port->dev_conf.rxmode.offloads) {
>>> + port->dev_conf.rxmode.offloads =
>>> + dev_conf_info.rxmode.offloads;
>>> + for (k = 0;
>>> + k < port->dev_info.max_rx_queues;
>>> + k++)
>>> + port->rx_conf[k].offloads =
>>> +
>> dev_conf_info.rxmode.offloads;
>>> + }
>>> + /* Apply Tx offloads configuration */
>>> + if (dev_conf_info.txmode.offloads !=
>>> + port->dev_conf.txmode.offloads) {
>>> + port->dev_conf.txmode.offloads =
>>> + dev_conf_info.txmode.offloads;
>>> + for (k = 0;
>>> + k < port->dev_info.max_tx_queues;
>>> + k++)
>>> + port->tx_conf[k].offloads =
>>> +
>> dev_conf_info.txmode.offloads;
>>> + }
>>> }
>>
>> Above implementation gets the configuration from device and applies it to the
>> testpmd configuration.
>>
>> Instead, what about a long level target to get rid of testpmd specific copy of the
>> configuration and rely and the config provided by devices. @Xiaoyun, what do
>> you think, does this make sense?
>
> You mean remove port->dev_conf and rx/tx_conf completely in the future? Or keep it in initial stage?
>
> Now, port->dev_conf will take global tx/rx_mode, fdir_conf and change some based on dev_info capabilities. And then use dev_configure to apply them for device.
> After this, actually, dev->data->dev_conf contains all device configuration.
>
> So It seems it's OK to remove port->dev_conf completely. Just testpmd needs to be refactored a lot and regression test in case of issues.
> But from long term view, it's good to keep one source and avoid copy.
>
Yes, this is the intention I have for long term. I expect that testpmd still
will keep some configuration in application level but we can prevent some
duplication.
And the main point is, by cleaning up testpmd we can recognize blockers and fix
them in libraries to help user applications.
> As for rx/tx_conf, it takes device default tx/rx_conf in dev_info and some settings in testpmd parameters also offloads from dev_conf.
> So keep port->rx/tx_conf? But then it still needs copy from dev_conf since this may change.
>
I am not very clear what is suggested above, can you please elaborate?
And 'struct rte_port' seems has following structs that can be get from library:
struct rte_eth_dev_info dev_info;
struct rte_eth_conf dev_conf;
struct rte_eth_rxconf rx_conf[]
struct rte_eth_txconf tx_conf[]
I don't think we can remove them, but perhaps reduce the usage of them, please
see below.
>>
>> So instead of above code, update where RSS hash offload information printed to
>> use device retrieved config instead of testpmd config, will it work?
>
> It's OK for device offload configurations.
> But queue offloads are a bit tricky since dev->data->dev_conf doesn't include queue conf.
> And it's not fair to use device offload configurations for queue offloads since user can use cmdline to config queue offload and that info can only be saved in port->rx/tx_conf and configure the device in setup_queue.
>
It is common in testpmd that, a command changes the application copy of the
configs, and mark as device configuration is required (for port or for queue).
So in later stage this changed configuration is applied to device.
This async approach has its benefits and I don't think we should change it.
(Also has some disadvantages that we hit in the past, like detecting some
configuration can't be applied in later stage when we try to apply the config,
not when command is issued at first place.).
What we can do it, reduce the testpmd config usage for the case to gather user
requests and apply them to device.
But to display device configuration, or to decide based on device configuration
we can user config values get by device by APIs.
What do you think, can above distinction makes sense, or does it work?
And there is still a chance that application copy of config diverge from device
config, and since we provide full config in our APIs (not changes), there is a
chance to overwrite a device configuration.
To prevent this it is possible to read device config and overwrite testpmd
config with that, similar to what this patch does, but I am not sure where this
sync can be done. What do you think about doing this just after device configured?
Thanks,
ferruh
Hi
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Friday, September 17, 2021 18:20
> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wang, Jie1X <jie1x.wang@intel.com>;
> dev@dpdk.org
> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net;
> jerinj@marvell.com; Ananyev, Konstantin <konstantin.ananyev@intel.com>
> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
> offload
>
> On 9/9/2021 4:31 AM, Li, Xiaoyun wrote:
> > Hi
> >
> >> -----Original Message-----
> >> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> >> Sent: Thursday, September 9, 2021 00:51
> >> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org; Li, Xiaoyun
> >> <xiaoyun.li@intel.com>
> >> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net
> >> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS
> >> hash offload
> >>
> >> On 8/27/2021 9:17 AM, Jie Wang wrote:
> >>> The driver may change offloads info into dev->data->dev_conf in
> >>> dev_configure which may cause port->dev_conf and port->rx_conf
> >>> contain outdated values.
> >>>
> >>> This patch updates the offloads info if it changes to fix this issue.
> >>>
> >>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> >>>
> >>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
> >>> ---
> >>> app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
> >>> app/test-pmd/testpmd.h | 2 ++
> >>> app/test-pmd/util.c | 15 +++++++++++++++
> >>> 3 files changed, 51 insertions(+)
> >>>
> >>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
> >>> 6cbe9ba3c8..bd67291160 100644
> >>> --- a/app/test-pmd/testpmd.c
> >>> +++ b/app/test-pmd/testpmd.c
> >>> @@ -2461,6 +2461,9 @@ start_port(portid_t pid)
> >>> }
> >>>
> >>> if (port->need_reconfig > 0) {
> >>> + struct rte_eth_conf dev_conf_info;
> >>> + int k;
> >>> +
> >>> port->need_reconfig = 0;
> >>>
> >>> if (flow_isolate_all) {
> >>> @@ -2498,6 +2501,37 @@ start_port(portid_t pid)
> >>> port->need_reconfig = 1;
> >>> return -1;
> >>> }
> >>> + /* get rte_eth_conf info */
> >>> + if (0 !=
> >>> + eth_dev_conf_info_get_print_err(pi,
> >>> + &dev_conf_info)) {
> >>> + fprintf(stderr,
> >>> + "port %d can not get device
> >> configuration info\n",
> >>> + pi);
> >>> + return -1;
> >>> + }
> >>> + /* Apply Rx offloads configuration */
> >>> + if (dev_conf_info.rxmode.offloads !=
> >>> + port->dev_conf.rxmode.offloads) {
> >>> + port->dev_conf.rxmode.offloads =
> >>> + dev_conf_info.rxmode.offloads;
> >>> + for (k = 0;
> >>> + k < port->dev_info.max_rx_queues;
> >>> + k++)
> >>> + port->rx_conf[k].offloads =
> >>> +
> >> dev_conf_info.rxmode.offloads;
> >>> + }
> >>> + /* Apply Tx offloads configuration */
> >>> + if (dev_conf_info.txmode.offloads !=
> >>> + port->dev_conf.txmode.offloads) {
> >>> + port->dev_conf.txmode.offloads =
> >>> + dev_conf_info.txmode.offloads;
> >>> + for (k = 0;
> >>> + k < port->dev_info.max_tx_queues;
> >>> + k++)
> >>> + port->tx_conf[k].offloads =
> >>> +
> >> dev_conf_info.txmode.offloads;
> >>> + }
> >>> }
> >>
> >> Above implementation gets the configuration from device and applies
> >> it to the testpmd configuration.
> >>
> >> Instead, what about a long level target to get rid of testpmd
> >> specific copy of the configuration and rely and the config provided
> >> by devices. @Xiaoyun, what do you think, does this make sense?
> >
> > You mean remove port->dev_conf and rx/tx_conf completely in the future? Or
> keep it in initial stage?
> >
> > Now, port->dev_conf will take global tx/rx_mode, fdir_conf and change some
> based on dev_info capabilities. And then use dev_configure to apply them for
> device.
> > After this, actually, dev->data->dev_conf contains all device configuration.
> >
> > So It seems it's OK to remove port->dev_conf completely. Just testpmd needs
> to be refactored a lot and regression test in case of issues.
> > But from long term view, it's good to keep one source and avoid copy.
> >
>
> Yes, this is the intention I have for long term. I expect that testpmd still will keep
> some configuration in application level but we can prevent some duplication.
>
> And the main point is, by cleaning up testpmd we can recognize blockers and fix
> them in libraries to help user applications.
>
> > As for rx/tx_conf, it takes device default tx/rx_conf in dev_info and some
> settings in testpmd parameters also offloads from dev_conf.
> > So keep port->rx/tx_conf? But then it still needs copy from dev_conf since this
> may change.
> >
>
> I am not very clear what is suggested above, can you please elaborate?
>
> And 'struct rte_port' seems has following structs that can be get from library:
> struct rte_eth_dev_info dev_info;
> struct rte_eth_conf dev_conf;
> struct rte_eth_rxconf rx_conf[]
> struct rte_eth_txconf tx_conf[]
>
> I don't think we can remove them, but perhaps reduce the usage of them, please
> see below.
>
> >>
> >> So instead of above code, update where RSS hash offload information
> >> printed to use device retrieved config instead of testpmd config, will it work?
> >
> > It's OK for device offload configurations.
> > But queue offloads are a bit tricky since dev->data->dev_conf doesn't include
> queue conf.
> > And it's not fair to use device offload configurations for queue offloads since
> user can use cmdline to config queue offload and that info can only be saved in
> port->rx/tx_conf and configure the device in setup_queue.
> >
>
> It is common in testpmd that, a command changes the application copy of the
> configs, and mark as device configuration is required (for port or for queue).
> So in later stage this changed configuration is applied to device.
>
> This async approach has its benefits and I don't think we should change it.
> (Also has some disadvantages that we hit in the past, like detecting some
> configuration can't be applied in later stage when we try to apply the config, not
> when command is issued at first place.).
>
> What we can do it, reduce the testpmd config usage for the case to gather user
> requests and apply them to device.
> But to display device configuration, or to decide based on device configuration
> we can user config values get by device by APIs.
>
> What do you think, can above distinction makes sense, or does it work?
>
>
> And there is still a chance that application copy of config diverge from device
> config, and since we provide full config in our APIs (not changes), there is a
> chance to overwrite a device configuration.
> To prevent this it is possible to read device config and overwrite testpmd config
> with that, similar to what this patch does, but I am not sure where this sync can
> be done. What do you think about doing this just after device configured?
I'm not sure I fully understand.
So for showing cmd, just use API rte_eth_tx/rx_queue_info_get to get dev queue config and new added API rte_eth_dev_conf_info_get to get dev config.
And for the cases where port->dev_config is used as a right value, replace them with use getting API.
For example: "if (res->value == port->dev_conf.rxmode.max_rx_pkt_len)" will be changed like "if (res->value == rte_eth_dev_conf_info_get().rxmode.max_rx_pkt_len)"
But other things keep the same as what this patch does?
This makes sense to me if I understand it right.
>
> Thanks,
> ferruh
On 9/18/2021 3:18 AM, Li, Xiaoyun wrote:
> Hi
>
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>> Sent: Friday, September 17, 2021 18:20
>> To: Li, Xiaoyun <xiaoyun.li@intel.com>; Wang, Jie1X <jie1x.wang@intel.com>;
>> dev@dpdk.org
>> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net;
>> jerinj@marvell.com; Ananyev, Konstantin <konstantin.ananyev@intel.com>
>> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
>> offload
>>
>> On 9/9/2021 4:31 AM, Li, Xiaoyun wrote:
>>> Hi
>>>
>>>> -----Original Message-----
>>>> From: Yigit, Ferruh <ferruh.yigit@intel.com>
>>>> Sent: Thursday, September 9, 2021 00:51
>>>> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org; Li, Xiaoyun
>>>> <xiaoyun.li@intel.com>
>>>> Cc: andrew.rybchenko@oktetlabs.ru; thomas@monjalon.net
>>>> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS
>>>> hash offload
>>>>
>>>> On 8/27/2021 9:17 AM, Jie Wang wrote:
>>>>> The driver may change offloads info into dev->data->dev_conf in
>>>>> dev_configure which may cause port->dev_conf and port->rx_conf
>>>>> contain outdated values.
>>>>>
>>>>> This patch updates the offloads info if it changes to fix this issue.
>>>>>
>>>>> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>>>>>
>>>>> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>>>>> ---
>>>>> app/test-pmd/testpmd.c | 34 ++++++++++++++++++++++++++++++++++
>>>>> app/test-pmd/testpmd.h | 2 ++
>>>>> app/test-pmd/util.c | 15 +++++++++++++++
>>>>> 3 files changed, 51 insertions(+)
>>>>>
>>>>> diff --git a/app/test-pmd/testpmd.c b/app/test-pmd/testpmd.c index
>>>>> 6cbe9ba3c8..bd67291160 100644
>>>>> --- a/app/test-pmd/testpmd.c
>>>>> +++ b/app/test-pmd/testpmd.c
>>>>> @@ -2461,6 +2461,9 @@ start_port(portid_t pid)
>>>>> }
>>>>>
>>>>> if (port->need_reconfig > 0) {
>>>>> + struct rte_eth_conf dev_conf_info;
>>>>> + int k;
>>>>> +
>>>>> port->need_reconfig = 0;
>>>>>
>>>>> if (flow_isolate_all) {
>>>>> @@ -2498,6 +2501,37 @@ start_port(portid_t pid)
>>>>> port->need_reconfig = 1;
>>>>> return -1;
>>>>> }
>>>>> + /* get rte_eth_conf info */
>>>>> + if (0 !=
>>>>> + eth_dev_conf_info_get_print_err(pi,
>>>>> + &dev_conf_info)) {
>>>>> + fprintf(stderr,
>>>>> + "port %d can not get device
>>>> configuration info\n",
>>>>> + pi);
>>>>> + return -1;
>>>>> + }
>>>>> + /* Apply Rx offloads configuration */
>>>>> + if (dev_conf_info.rxmode.offloads !=
>>>>> + port->dev_conf.rxmode.offloads) {
>>>>> + port->dev_conf.rxmode.offloads =
>>>>> + dev_conf_info.rxmode.offloads;
>>>>> + for (k = 0;
>>>>> + k < port->dev_info.max_rx_queues;
>>>>> + k++)
>>>>> + port->rx_conf[k].offloads =
>>>>> +
>>>> dev_conf_info.rxmode.offloads;
>>>>> + }
>>>>> + /* Apply Tx offloads configuration */
>>>>> + if (dev_conf_info.txmode.offloads !=
>>>>> + port->dev_conf.txmode.offloads) {
>>>>> + port->dev_conf.txmode.offloads =
>>>>> + dev_conf_info.txmode.offloads;
>>>>> + for (k = 0;
>>>>> + k < port->dev_info.max_tx_queues;
>>>>> + k++)
>>>>> + port->tx_conf[k].offloads =
>>>>> +
>>>> dev_conf_info.txmode.offloads;
>>>>> + }
>>>>> }
>>>>
>>>> Above implementation gets the configuration from device and applies
>>>> it to the testpmd configuration.
>>>>
>>>> Instead, what about a long level target to get rid of testpmd
>>>> specific copy of the configuration and rely and the config provided
>>>> by devices. @Xiaoyun, what do you think, does this make sense?
>>>
>>> You mean remove port->dev_conf and rx/tx_conf completely in the future? Or
>> keep it in initial stage?
>>>
>>> Now, port->dev_conf will take global tx/rx_mode, fdir_conf and change some
>> based on dev_info capabilities. And then use dev_configure to apply them for
>> device.
>>> After this, actually, dev->data->dev_conf contains all device configuration.
>>>
>>> So It seems it's OK to remove port->dev_conf completely. Just testpmd needs
>> to be refactored a lot and regression test in case of issues.
>>> But from long term view, it's good to keep one source and avoid copy.
>>>
>>
>> Yes, this is the intention I have for long term. I expect that testpmd still will keep
>> some configuration in application level but we can prevent some duplication.
>>
>> And the main point is, by cleaning up testpmd we can recognize blockers and fix
>> them in libraries to help user applications.
>>
>>> As for rx/tx_conf, it takes device default tx/rx_conf in dev_info and some
>> settings in testpmd parameters also offloads from dev_conf.
>>> So keep port->rx/tx_conf? But then it still needs copy from dev_conf since this
>> may change.
>>>
>>
>> I am not very clear what is suggested above, can you please elaborate?
>>
>> And 'struct rte_port' seems has following structs that can be get from library:
>> struct rte_eth_dev_info dev_info;
>> struct rte_eth_conf dev_conf;
>> struct rte_eth_rxconf rx_conf[]
>> struct rte_eth_txconf tx_conf[]
>>
>> I don't think we can remove them, but perhaps reduce the usage of them, please
>> see below.
>>
>>>>
>>>> So instead of above code, update where RSS hash offload information
>>>> printed to use device retrieved config instead of testpmd config, will it work?
>>>
>>> It's OK for device offload configurations.
>>> But queue offloads are a bit tricky since dev->data->dev_conf doesn't include
>> queue conf.
>>> And it's not fair to use device offload configurations for queue offloads since
>> user can use cmdline to config queue offload and that info can only be saved in
>> port->rx/tx_conf and configure the device in setup_queue.
>>>
>>
>> It is common in testpmd that, a command changes the application copy of the
>> configs, and mark as device configuration is required (for port or for queue).
>> So in later stage this changed configuration is applied to device.
>>
>> This async approach has its benefits and I don't think we should change it.
>> (Also has some disadvantages that we hit in the past, like detecting some
>> configuration can't be applied in later stage when we try to apply the config, not
>> when command is issued at first place.).
>>
>> What we can do it, reduce the testpmd config usage for the case to gather user
>> requests and apply them to device.
>> But to display device configuration, or to decide based on device configuration
>> we can user config values get by device by APIs.
>>
>> What do you think, can above distinction makes sense, or does it work?
>>
>>
>> And there is still a chance that application copy of config diverge from device
>> config, and since we provide full config in our APIs (not changes), there is a
>> chance to overwrite a device configuration.
>> To prevent this it is possible to read device config and overwrite testpmd config
>> with that, similar to what this patch does, but I am not sure where this sync can
>> be done. What do you think about doing this just after device configured?
>
> I'm not sure I fully understand.
> So for showing cmd, just use API rte_eth_tx/rx_queue_info_get to get dev queue config and new added API rte_eth_dev_conf_info_get to get dev config.
>
> And for the cases where port->dev_config is used as a right value, replace them with use getting API.
> For example: "if (res->value == port->dev_conf.rxmode.max_rx_pkt_len)" will be changed like "if (res->value == rte_eth_dev_conf_info_get().rxmode.max_rx_pkt_len)"
>
> But other things keep the same as what this patch does?
>
Yes. (Only I have a small comment on this patch, I will comment on other tread.)
And for this patch I don't suggest any additional change other than RSS show,
rest can be updated gradually.
> This makes sense to me if I understand it right.
On 8/27/2021 9:17 AM, Jie Wang wrote:
> The driver may change offloads info into dev->data->dev_conf
> in dev_configure which may cause port->dev_conf and port->rx_conf
> contain outdated values.
>
> This patch updates the offloads info if it changes to fix this issue.
>
> Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
>
> Signed-off-by: Jie Wang <jie1x.wang@intel.com>
<...>
> + /* Apply Rx offloads configuration */
> + if (dev_conf_info.rxmode.offloads !=
> + port->dev_conf.rxmode.offloads) {
> + port->dev_conf.rxmode.offloads =
> + dev_conf_info.rxmode.offloads;
> + for (k = 0;
> + k < port->dev_info.max_rx_queues;
> + k++)
> + port->rx_conf[k].offloads =
> + dev_conf_info.rxmode.offloads;
If queue specific offloads are used, won't this overwrite it with port offload?
Should we get queue config from device and update queue offloads with it?
> -----Original Message-----
> From: Yigit, Ferruh <ferruh.yigit@intel.com>
> Sent: Monday, September 20, 2021 5:48 PM
> To: Wang, Jie1X <jie1x.wang@intel.com>; dev@dpdk.org
> Cc: Li, Xiaoyun <xiaoyun.li@intel.com>; andrew.rybchenko@oktetlabs.ru;
> thomas@monjalon.net
> Subject: Re: [PATCH v8 2/2] app/testpmd: fix testpmd doesn't show RSS hash
> offload
>
> On 8/27/2021 9:17 AM, Jie Wang wrote:
> > The driver may change offloads info into dev->data->dev_conf in
> > dev_configure which may cause port->dev_conf and port->rx_conf contain
> > outdated values.
> >
> > This patch updates the offloads info if it changes to fix this issue.
> >
> > Fixes: ce8d561418d4 ("app/testpmd: add port configuration settings")
> >
> > Signed-off-by: Jie Wang <jie1x.wang@intel.com>
>
> <...>
>
> > + /* Apply Rx offloads configuration */
> > + if (dev_conf_info.rxmode.offloads !=
> > + port->dev_conf.rxmode.offloads) {
> > + port->dev_conf.rxmode.offloads =
> > + dev_conf_info.rxmode.offloads;
> > + for (k = 0;
> > + k < port->dev_info.max_rx_queues;
> > + k++)
> > + port->rx_conf[k].offloads =
> > +
> dev_conf_info.rxmode.offloads;
>
> If queue specific offloads are used, won't this overwrite it with port offload?
>
> Should we get queue config from device and update queue offloads with it?
Only the first time the driver configures the port, "dev_conf_info.rxmode.offloads" is not equal to "port->dev_conf.rxmode.offloads". So the added code just run 1 time.
But your suggestion is correct, I should update the queue offloads instead of overwriting it.
@@ -2461,6 +2461,9 @@ start_port(portid_t pid)
}
if (port->need_reconfig > 0) {
+ struct rte_eth_conf dev_conf_info;
+ int k;
+
port->need_reconfig = 0;
if (flow_isolate_all) {
@@ -2498,6 +2501,37 @@ start_port(portid_t pid)
port->need_reconfig = 1;
return -1;
}
+ /* get rte_eth_conf info */
+ if (0 !=
+ eth_dev_conf_info_get_print_err(pi,
+ &dev_conf_info)) {
+ fprintf(stderr,
+ "port %d can not get device configuration info\n",
+ pi);
+ return -1;
+ }
+ /* Apply Rx offloads configuration */
+ if (dev_conf_info.rxmode.offloads !=
+ port->dev_conf.rxmode.offloads) {
+ port->dev_conf.rxmode.offloads =
+ dev_conf_info.rxmode.offloads;
+ for (k = 0;
+ k < port->dev_info.max_rx_queues;
+ k++)
+ port->rx_conf[k].offloads =
+ dev_conf_info.rxmode.offloads;
+ }
+ /* Apply Tx offloads configuration */
+ if (dev_conf_info.txmode.offloads !=
+ port->dev_conf.txmode.offloads) {
+ port->dev_conf.txmode.offloads =
+ dev_conf_info.txmode.offloads;
+ for (k = 0;
+ k < port->dev_info.max_tx_queues;
+ k++)
+ port->tx_conf[k].offloads =
+ dev_conf_info.txmode.offloads;
+ }
}
if (port->need_reconfig_queues > 0) {
port->need_reconfig_queues = 0;
@@ -950,6 +950,8 @@ void show_gro(portid_t port_id);
void setup_gso(const char *mode, portid_t port_id);
int eth_dev_info_get_print_err(uint16_t port_id,
struct rte_eth_dev_info *dev_info);
+int eth_dev_conf_info_get_print_err(uint16_t port_id,
+ struct rte_eth_conf *dev_conf_info);
void eth_set_promisc_mode(uint16_t port_id, int enable);
void eth_set_allmulticast_mode(uint16_t port, int enable);
int eth_link_get_nowait_print_err(uint16_t port_id, struct rte_eth_link *link);
@@ -440,6 +440,21 @@ eth_dev_info_get_print_err(uint16_t port_id,
return ret;
}
+int
+eth_dev_conf_info_get_print_err(uint16_t port_id,
+ struct rte_eth_conf *dev_conf_info)
+{
+ int ret;
+
+ ret = rte_eth_dev_conf_info_get(port_id, dev_conf_info);
+ if (ret != 0)
+ fprintf(stderr,
+ "Error during getting device configuration (port %u) info: %s\n",
+ port_id, strerror(-ret));
+
+ return ret;
+}
+
void
eth_set_promisc_mode(uint16_t port, int enable)
{