Re: [dpdk-stable] [PATCH v5] app/testpmd: fix setting maximum packet length

[Ferruh Yigit <ferruh.yigit@intel.com>]

On 1/26/2021 3:45 AM, Lance Richardson wrote:
> On Mon, Jan 25, 2021 at 7:44 PM Ferruh Yigit <ferruh.yigit@intel.com> wrote:
>>
>>>> +       if (rx_offloads != port->dev_conf.rxmode.offloads) {
>>>> +               uint16_t qid;
>>>> +
>>>> +               port->dev_conf.rxmode.offloads = rx_offloads;
>>>> +
>>>> +               /* Apply JUMBO_FRAME offload configuration to Rx queue(s) */
>>>> +               for (qid = 0; qid < port->dev_info.nb_rx_queues; qid++) {
>>>> +                       if (on)
>>>> +                               port->rx_conf[qid].offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>> +                       else
>>>> +                               port->rx_conf[qid].offloads &= ~DEV_RX_OFFLOAD_JUMBO_FRAME;
>>>> +               }
>>>
>>> Is it correct to set per-queue offloads that aren't advertised by the PMD
>>> as supported in rx_queue_offload_capa?
>>>
>>
>> 'port->rx_conf[]' is testpmd struct, and 'port->dev_conf.rxmode.offloads' values
>> are reflected to 'port->rx_conf[].offloads' for all queues.
>>
>> We should set the offload in 'port->rx_conf[].offloads' if it is set in
>> 'port->dev_conf.rxmode.offloads'.
>>
>> If a port has capability for 'JUMBO_FRAME', 'port->rx_conf[].offloads' can have
>> it. And the port level capability is already checked above.
>>
> 
> I'm still not 100% clear about the per-queue offload question.
> 
> With this patch, and jumbo max packet size configured (on the command
> line in this case), I see:
> 
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
>    Port : JUMBO_FRAME
>    Queue[ 0] : JUMBO_FRAME
> 
> testpmd> show port 0 rx_offload capabilities
> Rx Offloading Capabilities of port 0 :
>    Per Queue :
>    Per Port  : VLAN_STRIP IPV4_CKSUM UDP_CKSUM TCP_CKSUM TCP_LRO
> OUTER_IPV4_CKSUM VLAN_FILTER VLAN_EXTEND JUMBO_FRAME SCATTER TIMESTAMP
> KEEP_CRC OUTER_UDP_CKSUM RSS_HASH
> 

The port level offload is applied to all queues on the port, testpmd config 
structure reflects this logic in implementation.
If Rx offload X is set for a port, it is set for all Rx queue offloads, this is 
not new behavior and not related to this patch.

In the ethdev, lets assume X & Y are port level offloads,
after X, Y are set via 'rte_eth_dev_configure()'
if user calls 'rte_eth_rx_queue_setup()' with X & Y offload, this is a valid 
call and API will return success, since those offloads already enabled in port 
level means they are enabled for all queues.

Because of above ethdev behavior, testpmd keeps all enabled port level offload 
in the queue level offload too, and display them as enabled offloads for the queue.

To request a queue specific offload, it is added to specific queue's config 
before calling queue setup. Lets say that queue specific offload is Z, after 
setup testpmd config struct will show that specific queue has X, Y & Z offloads.

I hope it is more clear now.

> Yet if I configure a jumbo MTU starting with standard max packet size,
> jumbo is only enabled at the port level:
> testpmd> port config mtu 0 9000
> testpmd> port start all
> 
> testpmd> show port 0 rx_offload configuration
> Rx Offloading Configuration of port 0 :
>    Port : JUMBO_FRAME
>    Queue[ 0] :
> 
> It still seems odd for a per-queue offload to be enabled on a PMD that
> doesn't support per-queue receive offloads.
> 

"port config mtu" should take queue offloads into account, it looks wrong right now.