[1/2] app/testpmd: update Rx RSS HASH offload when setting MQ RSS
Checks
Commit Message
From: Huisong Li <lihuisong@huawei.com>
Currently, when starting testpmd application without '--disable-rss' and
the number of Rx queue configured is greater than 1, ETH_MQ_RX_RSS flag
is set in port->dev_conf.rxmode.mq_mode in testpmd application, and
DEV_RX_OFFLOAD_RSS_HASH flag is set in rx_offloads
(dev->data->dev_conf.rxmode.offloads) according to the ETH_MQ_RX_RSS
flag of rxmode.mq_mode in PMD drivers.
However, DEV_RX_OFFLOAD_RSS_HASH is not set to rx_offloads maintained
in testpmd application, this will cause the inconsistent problem that
rx_offloads is different for testpmd and PMD drivers.
Signed-off-by: Huisong Li <lihuisong@huawei.com>
Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
---
app/test-pmd/testpmd.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
Comments
On 9/8/2020 3:16 AM, Wei Hu (Xavier) wrote:
> From: Huisong Li <lihuisong@huawei.com>
>
> Currently, when starting testpmd application without '--disable-rss' and
> the number of Rx queue configured is greater than 1, ETH_MQ_RX_RSS flag
> is set in port->dev_conf.rxmode.mq_mode in testpmd application, and
> DEV_RX_OFFLOAD_RSS_HASH flag is set in rx_offloads
> (dev->data->dev_conf.rxmode.offloads) according to the ETH_MQ_RX_RSS
> flag of rxmode.mq_mode in PMD drivers.
>
> However, DEV_RX_OFFLOAD_RSS_HASH is not set to rx_offloads maintained
> in testpmd application, this will cause the inconsistent problem that
> rx_offloads is different for testpmd and PMD drivers.
Yes for DEV_RX_OFFLOAD_RSS_HASH, application rx_offload config and PMD
one diverges, for *some* PMDs.
This is done to have the backward compatibility of the PMD behavior.
The PMDs that would like to write the provide the calculated hash value
back, overwrites the offload config to enable 'DEV_RX_OFFLOAD_RSS_HASH'.
And does more work than user requested, this shouldn't have any side affect.
Some doesn't provide the hash unless user explicitly requests it.
Applications shouldn't set it blindly, at least first check if PMD
supports it but even that case, unless it is needed I think HASH offload
shouldn't be requested by default.
>
> Signed-off-by: Huisong Li <lihuisong@huawei.com>
> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.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 7842c3b..73543bb 100644
> --- a/app/test-pmd/testpmd.c
> +++ b/app/test-pmd/testpmd.c
> @@ -3356,11 +3356,13 @@ init_port_config(void)
> }
>
> if (port->dcb_flag == 0) {
> - if( port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0)
> + if (port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0) {
> port->dev_conf.rxmode.mq_mode =
> (enum rte_eth_rx_mq_mode)
> (rx_mq_mode & ETH_MQ_RX_RSS);
> - else
> + port->dev_conf.rxmode.offloads |=
> + DEV_RX_OFFLOAD_RSS_HASH;
> + } else
> port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
> }
>
>
Hi, Ferruh Yigit
On 2020/9/23 0:21, Ferruh Yigit wrote:
> On 9/8/2020 3:16 AM, Wei Hu (Xavier) wrote:
>> From: Huisong Li <lihuisong@huawei.com>
>>
>> Currently, when starting testpmd application without '--disable-rss' and
>> the number of Rx queue configured is greater than 1, ETH_MQ_RX_RSS flag
>> is set in port->dev_conf.rxmode.mq_mode in testpmd application, and
>> DEV_RX_OFFLOAD_RSS_HASH flag is set in rx_offloads
>> (dev->data->dev_conf.rxmode.offloads) according to the ETH_MQ_RX_RSS
>> flag of rxmode.mq_mode in PMD drivers.
>>
>> However, DEV_RX_OFFLOAD_RSS_HASH is not set to rx_offloads maintained
>> in testpmd application, this will cause the inconsistent problem that
>> rx_offloads is different for testpmd and PMD drivers.
>
> Yes for DEV_RX_OFFLOAD_RSS_HASH, application rx_offload config and PMD
> one diverges, for *some* PMDs.
>
> This is done to have the backward compatibility of the PMD behavior.
>
> The PMDs that would like to write the provide the calculated hash value
> back, overwrites the offload config to enable 'DEV_RX_OFFLOAD_RSS_HASH'.
> And does more work than user requested, this shouldn't have any side
> affect.
> Some doesn't provide the hash unless user explicitly requests it.
>
> Applications shouldn't set it blindly, at least first check if PMD
> supports it but even that case, unless it is needed I think HASH offload
> shouldn't be requested by default.
OK, we are going to do modification to add check if PMD support
DEV_RX_OFFLOAD_RSS_HASH before setting it as below:
@@ -3356,11 +3356,13 @@ init_port_config(void)
}
if (port->dcb_flag == 0) {
if (port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0) {
port->dev_conf.rxmode.mq_mode =
(enum rte_eth_rx_mq_mode)
(rx_mq_mode & ETH_MQ_RX_RSS);
if (port->dev_info.rx_offload_capa &
DEV_RX_OFFLOAD_RSS_HASH)
port->dev_conf.rxmode.offloads |=
DEV_RX_OFFLOAD_RSS_HASH;
} else
port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
}
Thanks, Xavier
>
>>
>> Signed-off-by: Huisong Li <lihuisong@huawei.com>
>> Signed-off-by: Wei Hu (Xavier) <xavier.huwei@huawei.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 7842c3b..73543bb 100644
>> --- a/app/test-pmd/testpmd.c
>> +++ b/app/test-pmd/testpmd.c
>> @@ -3356,11 +3356,13 @@ init_port_config(void)
>> }
>> if (port->dcb_flag == 0) {
>> - if( port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0)
>> + if (port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0) {
>> port->dev_conf.rxmode.mq_mode =
>> (enum rte_eth_rx_mq_mode)
>> (rx_mq_mode & ETH_MQ_RX_RSS);
>> - else
>> + port->dev_conf.rxmode.offloads |=
>> + DEV_RX_OFFLOAD_RSS_HASH;
>> + } else
>> port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
>> }
>>
On 9/23/2020 8:04 AM, Wei Hu (Xavier) wrote:
> Hi, Ferruh Yigit
>
> On 2020/9/23 0:21, Ferruh Yigit wrote:
>> On 9/8/2020 3:16 AM, Wei Hu (Xavier) wrote:
>>> From: Huisong Li <lihuisong@huawei.com>
>>>
>>> Currently, when starting testpmd application without '--disable-rss' and
>>> the number of Rx queue configured is greater than 1, ETH_MQ_RX_RSS flag
>>> is set in port->dev_conf.rxmode.mq_mode in testpmd application, and
>>> DEV_RX_OFFLOAD_RSS_HASH flag is set in rx_offloads
>>> (dev->data->dev_conf.rxmode.offloads) according to the ETH_MQ_RX_RSS
>>> flag of rxmode.mq_mode in PMD drivers.
>>>
>>> However, DEV_RX_OFFLOAD_RSS_HASH is not set to rx_offloads maintained
>>> in testpmd application, this will cause the inconsistent problem that
>>> rx_offloads is different for testpmd and PMD drivers.
>>
>> Yes for DEV_RX_OFFLOAD_RSS_HASH, application rx_offload config and PMD
>> one diverges, for *some* PMDs.
>>
>> This is done to have the backward compatibility of the PMD behavior.
>>
>> The PMDs that would like to write the provide the calculated hash
>> value back, overwrites the offload config to enable
>> 'DEV_RX_OFFLOAD_RSS_HASH'. And does more work than user requested,
>> this shouldn't have any side affect.
>> Some doesn't provide the hash unless user explicitly requests it.
>>
>> Applications shouldn't set it blindly, at least first check if PMD
>> supports it but even that case, unless it is needed I think HASH
>> offload shouldn't be requested by default.
>
> OK, we are going to do modification to add check if PMD support
> DEV_RX_OFFLOAD_RSS_HASH before setting it as below:
>
> @@ -3356,11 +3356,13 @@ init_port_config(void)
> }
> if (port->dcb_flag == 0) {
> if (port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0) {
> port->dev_conf.rxmode.mq_mode =
> (enum rte_eth_rx_mq_mode)
> (rx_mq_mode & ETH_MQ_RX_RSS);
> if (port->dev_info.rx_offload_capa &
> DEV_RX_OFFLOAD_RSS_HASH)
> port->dev_conf.rxmode.offloads |=
> DEV_RX_OFFLOAD_RSS_HASH;
> } else
> port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
> }
>
Hi Xavier,
The capability check is correct thing to do, but even in that case I am
not sure if we should set the config by default.
The reason to extract the 'DEV_RX_OFFLOAD_RSS_HASH' offload is to gain
performance for some NICs, enabling it by default defeats the purpose.
The offload should be enabled when application needs the provided hash
value.
I can see you are trying to remove the divergence between PMD and
application config, this can be fixed when all PMDs take this user
offload request into account instead of overwriting, but for some PMDs
it is cheaper to provide the hash value instead of checks, so this
divergence is not easy to address.
Hi, Ferruh Yigit
On 2020/9/23 17:35, Ferruh Yigit wrote:
> On 9/23/2020 8:04 AM, Wei Hu (Xavier) wrote:
>> Hi, Ferruh Yigit
>>
>> On 2020/9/23 0:21, Ferruh Yigit wrote:
>>> On 9/8/2020 3:16 AM, Wei Hu (Xavier) wrote:
>>>> From: Huisong Li <lihuisong@huawei.com>
>>>>
>>>> Currently, when starting testpmd application without
>>>> '--disable-rss' and
>>>> the number of Rx queue configured is greater than 1, ETH_MQ_RX_RSS
>>>> flag
>>>> is set in port->dev_conf.rxmode.mq_mode in testpmd application, and
>>>> DEV_RX_OFFLOAD_RSS_HASH flag is set in rx_offloads
>>>> (dev->data->dev_conf.rxmode.offloads) according to the ETH_MQ_RX_RSS
>>>> flag of rxmode.mq_mode in PMD drivers.
>>>>
>>>> However, DEV_RX_OFFLOAD_RSS_HASH is not set to rx_offloads maintained
>>>> in testpmd application, this will cause the inconsistent problem that
>>>> rx_offloads is different for testpmd and PMD drivers.
>>>
>>> Yes for DEV_RX_OFFLOAD_RSS_HASH, application rx_offload config and
>>> PMD one diverges, for *some* PMDs.
>>>
>>> This is done to have the backward compatibility of the PMD behavior.
>>>
>>> The PMDs that would like to write the provide the calculated hash
>>> value back, overwrites the offload config to enable
>>> 'DEV_RX_OFFLOAD_RSS_HASH'. And does more work than user requested,
>>> this shouldn't have any side affect.
>>> Some doesn't provide the hash unless user explicitly requests it.
>>>
>>> Applications shouldn't set it blindly, at least first check if PMD
>>> supports it but even that case, unless it is needed I think HASH
>>> offload shouldn't be requested by default.
>>
>> OK, we are going to do modification to add check if PMD support
>> DEV_RX_OFFLOAD_RSS_HASH before setting it as below:
>>
>> @@ -3356,11 +3356,13 @@ init_port_config(void)
>> }
>> if (port->dcb_flag == 0) {
>> if (port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0) {
>> port->dev_conf.rxmode.mq_mode =
>> (enum rte_eth_rx_mq_mode)
>> (rx_mq_mode & ETH_MQ_RX_RSS);
>> if (port->dev_info.rx_offload_capa &
>> DEV_RX_OFFLOAD_RSS_HASH)
>> port->dev_conf.rxmode.offloads |=
>> DEV_RX_OFFLOAD_RSS_HASH;
>> } else
>> port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
>> }
>>
>
> Hi Xavier,
>
> The capability check is correct thing to do, but even in that case I
> am not sure if we should set the config by default.
>
> The reason to extract the 'DEV_RX_OFFLOAD_RSS_HASH' offload is to gain
> performance for some NICs, enabling it by default defeats the purpose.
>
> The offload should be enabled when application needs the provided hash
> value.
>
> I can see you are trying to remove the divergence between PMD and
> application config, this can be fixed when all PMDs take this user
> offload request into account instead of overwriting, but for some PMDs
> it is cheaper to provide the hash value instead of checks, so this
> divergence is not easy to address.
>
OK, please ignore this patch and the V2 patch.
http://patches.dpdk.org/patch/78502/
Thanks.
Regards
Xavier
@@ -3356,11 +3356,13 @@ init_port_config(void)
}
if (port->dcb_flag == 0) {
- if( port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0)
+ if (port->dev_conf.rx_adv_conf.rss_conf.rss_hf != 0) {
port->dev_conf.rxmode.mq_mode =
(enum rte_eth_rx_mq_mode)
(rx_mq_mode & ETH_MQ_RX_RSS);
- else
+ port->dev_conf.rxmode.offloads |=
+ DEV_RX_OFFLOAD_RSS_HASH;
+ } else
port->dev_conf.rxmode.mq_mode = ETH_MQ_RX_NONE;
}