Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API

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

On 3/12/2018 5:58 PM, Shahaf Shuler wrote:
> Monday, March 12, 2018 7:00 PM, Ferruh Yigit:
>> There are some devices supports queue level offloads and there are some
>> devices supports port level offloads.
>>
>> Values queue offload = 0x0 and port offload = 0x1000, for the device that
>> support queue level offloads may mean disabling all offloads for that specific
>> queue and this is valid value. For the device that support port level offloads
>> this is an error.
> 
> device which don't support port level offloads should not be configured with port offloads. 
> Well implemented PMDs will fail the configuration with port offload = 0x1000.

For this particular error in tap:
Rx queue offloads = 0x0,
requested port offloads = 0x1000,
supported offloads = 0x300e

Since supported offloads reported, and requested is subset of it I think port
level offloads are OK, problem is in queue level offloads.

> 
>>
>> What about using "rx/tx_queue_offload_capa" field to help application to
>> detect if device supports queue level or port level offloads?
> 
> Yes. this is their purpose. 
> 
>>
>> If device reports 0x0 "rx_queue_offload_capa", application will know device
>> doesn't support queue level offloads and PMD just ignore all provided queue
>> offloads.
>>
>> If device reports a "rx_queue_offload_capa" other than 0x0, app will know
>> that device _supports_ queue offloads and will set offloads according and
>> PMD will verify and apply queue specific offloads according.
>>
>> For above tap specific case, is tap PMD supports queue specific offloads?
> 
> Am not sure. Moty? 
> 
>>
>> And there should be some restrictions on offloading values:
>> 1- requested port offloads should be subset of supported port offloads
>> 2- supported queue offloads should be subset of supported port offloads
>> 3- requested queue offloads should be subset of supported queue offloads
> 
> This is correct. 
> 
>>
>> And since these information is part of dev_info, these can be managed in the
>> ethdev layer, instead of checked in each PMD (as done in tap).
>>
>>
>> According above, would you mind walk-trough how application can set
>> offloads:
>>
>> A) New application that implements new offloading APIs:
>> - Get dev_info
>> - Configure Rx/Tx offloads based on rx_offload_capa / tx_offload_capa
>> - If rx_queue_offload_capa / tx_queue_offload_capa is other than 0, setup
>> RxQ / TxQ offloads based on these values.
>> - If rx_queue_offload_capa / tx_queue_offload_capa is 0, queue level
>> offlaods are not supported by this device, ignore offloads during RxQ / TxQ
>> setup.
> 
> With the current API it is not correct. queue offloads should be at least the port offloads. If the application wants to enable more queue specific offloads it can as long as those are supported.

So above statement 2 is wrong?

Just to confirm, a queue can not disable an offload configured for port but can
enable more offloads?

> The following pseudo code demonstrate above:
> 
> Dev_configure(port, port_offloads)
> Rx_queue_offloads = port_offloads
> If (rx_queue_offload_capa != 0)
> 	Rx_queue_offloads |= rx_queue_offload_capa

Again to confirm, a device that supports port level offloads free to ignore TxQ
/ RxQ offload values, right? So why we need "Rx_queue_offloads = port_offloads",
will following be true?

Dev_configure(port, port_offloads)
If (rx_queue_offload_capa != 0)
        Rx_queue_offloads = port_offloads | rx_queue_offload_capa

> 
>>
>> All look OK here.
>>
>>
>> B) Old application with old offloading API
>> - Get dev_info, which provides only rx_offload_capa, tx_offload_capa and
>> txq_flags
>> - set rte_eth_rxmode->bitfield_values  ==> ethdev will convert them to port
>> level Rx offloads.
>> - port level offloads are empty!!
>> - ethdev will set queue level Rx offloads to be same as port level Rx offloads.
>> - ethdev will set txq_flags values for Tx offloads to queue level Tx offloads.
>>
>> Things should work well for PMDs with old offloading API.
>>
>> For the PMDs that support new offloading API, port level Tx offload values
>> are missing and Queue level and Port level Tx offloads mismatch. Am I
>> missing something here, if not how can we solve this issue in PMDs?
> 
> Those PMDs (new PMD for old application) can use the  ETH_TXQ_FLAGS_IGNORE which must be set for application which uses the new API..
> see snipped code from mlx5 PMD:
> 
> [1]
> /*                                                                      
>  * Don't verify port offloads for application which                     
>  * use the old API.                                                     
>  */                                                                     
> if (!!(conf->txq_flags & ETH_TXQ_FLAGS_IGNORE) &&                       
>     !priv_is_tx_queue_offloads_allowed(priv, conf->offloads)) {         
>         ret = ENOTSUP;                                                  
>         ERROR("%p: Tx queue offloads 0x%" PRIx64 " don't match port "   
>               "offloads 0x%" PRIx64 " or supported offloads 0x%" PRIx64,
>               (void *)dev, conf->offloads,                              
>               dev->data->dev_conf.txmode.offloads,                      
>               mlx5_priv_get_tx_port_offloads(priv));                    
>         goto out;                                                       
> } 

What this code does is: "if new offload API is used and queue offload is not
valid, return error", this is completely different than what I say.

My concern is how new PMD will handle old application because of missing port
level Tx offloads.

I guess each time PMD needs to use a Tx offload, it needs to check
ETH_TXQ_FLAGS_IGNORE. If IGNORE is set PMD will use  txmode->offloads, if not it
will use txq->offloads. Do you think this solves the issue?

> 
> 
> 
> 
> 
>>
>>
>>>
>>>>
>>>> I believe other way around makes sense, to be able to set queue offload
>> param, device should announce that offloading as supported. Queue is
>> subset of the device, why you ignore device offload param to set queue
>> offload param?
>>>>
>>>> What makes sense to me:
>>>> "queue offload" is subset of "device offload" is subset of "device
>> supported offload"
>>>
>>>
>>>
>>>
>>> [1]
>>>
>> https://emea01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fdpd
>> k
>>>
>> .org%2Fdoc%2Fguides%2Fprog_guide%2Fpoll_mode_drv.html&data=02%7C
>> 01%7Cs
>>>
>> hahafs%40mellanox.com%7C2fc3763767f041846bdd08d5883aa76a%7Ca65297
>> 1c7d2
>>>
>> e4d9ba6a4d149256f461b%7C0%7C0%7C636564707857610335&sdata=Uavzne
>> YAsXf2M
>>> SdJWSp6i1fvRmCRyx6pWwLuzUCqOLA%3D&reserved=0
>>>
>>>
>>>>
>>>> <...>
>