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

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

On 3/14/2018 5:49 AM, Shahaf Shuler wrote:
> Tuesday, March 13, 2018 1:57 PM, Ferruh Yigit:
>>>
>>> Again - the application should follow the API which currently dictates how
>> to set port offload. It is not depends on the rx_queue_offloads capabilities.
>>> For example, PMD which don't support queue offloads can still have
>> verification for the API that each port offload is set also on the queue
>> offloads.
>>
>> I am not agree with this part, why to dictate application to set queue offloads
>> if it already knows device doesn't support queue specific offloads?
> 
> I agree we can make a small change in the API to not force the application to set the port offloads in the queue configuration. It makes sense. 
> The change will be:
> "port offloads should be set on the port configuration. Queue offloads should be set on the queue configuration" 

I am OK to this one, this is more reasonable for devices that support only port
level offloads.

This looks like same as option #2 mentioned in the previous mails.

> 
>>
>> In some of the existing PMD patches, to switch to new offloading API, PMD
>> sets [rt]x_queue_offload_capa as same as [rt]x_offload_capa, 
> 
> Well this is just wrong. Unless those PMDs support all the offloads in a queue level. 
> 
> The logic is "every queue offload can be counted as port offload", because such offload can be set on each and every queue.
> The other way around is not correct, port offload cannot be counted as queue offload.
> 
> So if such PMDs has offloads which are supported only on the port level they cannot be declared as queue offloads. 

Thanks for confirming, it would be great if you can help on the PMD new offload
API patch reviews, to catch these kind of issues.

> 
> 
>> in that case
>> application can't know if queue specific offloads are supported or not and
>> application may try to set queue offloads, this forces PMD to verify them.
>>
>> You confirmed [rt]x_queue_offload_capa is the way for application to know
>> if device supports queue specific offloads or not. If these values always set to
>> [rt]x_offload_capa, application losts this capability.
>>
>> Instead:
>> - PMD that doesn't support queue specific offloads should set
>> [rt]x_queue_offload_capa to 0
>> - When [rt]x_queue_offload_capa is 0, application should be free to set
>> queue offloads whatever it wants
> 
> I don't agree, when queue_offload_capa is 0 the expected behavior from application is not to set any offload (if we do the change in the API that you are pushing to).
> PMDs can verify it or not, but if capability is not set the application should not set the offload. This is how the API should be defined. 

OK for this one.

> 
>> - When [rt]x_queue_offload_capa is 0, PMD should be free to verify queue
>> offloads but most probably shouldn't verify them since we don't know what
>> application will send.
>>
>> - When [rt]x_queue_offload_capa is != 0, applications should set queue
>> offloads at least "[rt]x_queue_offload = [rt]x_offload"
> 
> If we do the change you are pushing it is not needed. 
> Application will set the port offload in the port configuration, and the queue offload in the queue configuration. 
> No need to make special treatment based on the offloads_capa. 

Right.

> 
>> - When [rt]x_queue_offload_capa is != 0, PMD should verify the queue
>> offloads
>>


Back to initial question J, is tap supports queue level offloads?
If not it shouldn't be reporting or checking queue offloads.


Although it will be changed after above suggested change in API, I think check
in existing tap queue_setup, also same in mlx5, is wrong.

tap_rxq_are_offloads_valid(struct rte_eth_dev *dev, uint64_t offloads)
{

        uint64_t port_offloads = dev->data->dev_conf.rxmode.offloads;
        uint64_t queue_supp_offloads = tap_rx_offload_get_queue_capa();
        uint64_t port_supp_offloads = tap_rx_offload_get_port_capa();


<...>
        if ((port_offloads ^ offloads) & port_supp_offloads)
               return false;
        return true;

}


take the example:
port_supp_offloads = 11111
port_offloads = 111
queue_supp_offloads = 1111
offloads = 1111

(port_offloads ^ offloads) & port_supp_offloads = 1000
Which will return false.

This only works if "port_offloads == offloads" which is practically only
supporting port level offloads.