[ovs-dev] [PATCH] netdev-dpdk: Fix port init when lacking Tx offloads for TSO.

[Ilya Maximets]

On 2/6/20 1:28 PM, David Marchand wrote:
> On Thu, Feb 6, 2020 at 12:50 PM Flavio Leitner <fbl at sysclose.org> wrote:
>>
>> On Thu, Feb 06, 2020 at 11:48:58AM +0100, Ilya Maximets wrote:
>>> On 2/5/20 7:01 PM, David Marchand wrote:
>>>> On Wed, Feb 5, 2020 at 12:42 PM Flavio Leitner <fbl at sysclose.org> wrote:
>>>>>
>>>>> On Tue, Feb 04, 2020 at 10:28:26PM +0100, David Marchand wrote:
>>>>>> The check on TSO capability did not ensure ip checksum, tcp checksum and
>>>>>> TSO tx offloads were available which resulted in a port init failure
>>>>>> (example below with a ena device):
>>>>>>
>>>>>> *2020-02-04T17:42:52.976Z|00084|dpdk|ERR|Ethdev port_id=0 requested Tx
>>>>>> offloads 0x2a doesn't match Tx offloads capabilities 0xe in
>>>>>> rte_eth_dev_configure()*
>>>>>>
>>>>>> Fixes: 29cf9c1b3b9c ("userspace: Add TCP Segmentation Offload support")
>>>>>>
>>>>>> Reported-by: Ravi Kerur <rkerur at gmail.com>
>>>>>> Signed-off-by: David Marchand <david.marchand at redhat.com>
>>>>>> ---
>>>>>
>>>>> Works for me.
>>>>> Acked-by: Flavio Leitner <fbl at sysclose.org>
>>>>>
>>>>> This needs to go to master and branch-2.13.
>>>>
>>>> Thinking again about this, don't we have an issue in that we always
>>>> ask drivers for offloads even if TSO is disabled?
>>>>
>>>> I would expect a performance impact, since dpdk pmds (eyeing ixgbe
>>>> driver which checks txq->offloads == 0) love to select optimised tx
>>>> function based on offloads configuration.
>>>>
>>>>
>>>
>>>
>>>
>>> Yes. There will be significant performance impact. Last time I compared
>>>
>>> different i40e tx functions there was performance difference ~20-25% in
>>>
>>> phy-phy test.
>>>   It was a long time ago, however we should not change the
>>> default behavior anyway.
>>>
>>>
>>> Suggesting following incremental:
>>>
>>>
>>>
>>> ---
>>>
>>>
>>> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
>>> index eb1a7af94..6187129c0 100644
>>> --- a/lib/netdev-dpdk.c
>>> +++ b/lib/netdev-dpdk.c
>>> @@ -1132,12 +1132,15 @@ dpdk_eth_dev_init(struct netdev_dpdk *dev)
>>>          dev->hw_ol_features &= ~NETDEV_RX_HW_SCATTER;
>>>      }
>>>
>>> -    if ((info.tx_offload_capa & tx_tso_offload_capa) == tx_tso_offload_capa) {
>>> -        dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD;
>>> -    } else {
>>> -        dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD;
>>> -        VLOG_WARN("Tx TSO offload is not supported on %s port "
>>> -                  DPDK_PORT_ID_FMT, netdev_get_name(&dev->up), dev->port_id);
>>> +    dev->hw_ol_features &= ~NETDEV_TX_TSO_OFFLOAD;
>>> +    if (userspace_tso_enabled()) {
>>> +        if ((info.tx_offload_capa & tx_tso_offload_capa)
>>> +            == tx_tso_offload_capa) {
>>> +            dev->hw_ol_features |= NETDEV_TX_TSO_OFFLOAD;
>>> +        } else {
>>> +            VLOG_WARN("%s: Tx TSO offload is not supported.",
>>> +                      netdev_get_name(&dev->up));
>>> +        }
>>>      }
>>>
>>>      n_rxq = MIN(info.max_rx_queues, dev->up.n_rxq);
>>> ---
>>>
>>> Could anyone, please, test this?
>>
>> I had the same patch under testing during the night:
>> today: 13252499.10 +-71996.09
>> patch: 13732079.60 +-97931.85 pps
> 
> I am fine with taking my original patch as is.
> Then either Ilya or you send a follow up patch.

I applied this patch to master and branch-2.13 as is and sent above diff
as a separate patch.

Best regards, Ilya Maximets.