On 9/18/2021 3:18 AM, Li, Xiaoyun wrote:
> Hi
>
>> -----Original Message-----
>> From: Yigit, Ferruh <ferruh.yi...@intel.com>
>> Sent: Friday, September 17, 2021 18:20
>> To: Li, Xiaoyun <xiaoyun...@intel.com>; Wang, Jie1X <jie1x.w...@intel.com>;
>> dev@dpdk.org
>> Cc: andrew.rybche...@oktetlabs.ru; tho...@monjalon.net;
>> jer...@marvell.com; Ananyev, Konstantin <konstantin.anan...@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.yi...@intel.com>
>>>> Sent: Thursday, September 9, 2021 00:51
>>>> To: Wang, Jie1X <jie1x.w...@intel.com>; dev@dpdk.org; Li, Xiaoyun
>>>> <xiaoyun...@intel.com>
>>>> Cc: andrew.rybche...@oktetlabs.ru; tho...@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.w...@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.
