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

[Shahaf Shuler <shahafs@mellanox.com>]

--Shahaf


> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ferruh Yigit
> Sent: Friday, March 2, 2018 11:44 PM
> To: Mordechay Haimovsky <motih@mellanox.com>;
> pascal.mazon@6wind.com
> Cc: dev@dpdk.org; Shahaf Shuler <shahafs@mellanox.com>
> Subject: Re: [dpdk-dev] [PATCH V5 2/2] net/tap: use new Rx offloads API
> 
> On 1/17/2018 2:04 PM, Moti Haimovsky wrote:
> > Ethdev Rx offloads API has changed since:
> > commit ce17eddefc20 ("ethdev: introduce Rx queue offloads API") This
> > commit adds support for the new Rx offloads API.
> >
> > Signed-off-by: Moti Haimovsky <motih@mellanox.com>
> 
> <...>
> 
> > +static bool
> > +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 ((offloads & (queue_supp_offloads | port_supp_offloads)) !=
> > +	    offloads)
> > +		return false;
> > +	if ((port_offloads ^ offloads) & port_supp_offloads)
> > +		return false;
> 
> Hi Moti,
> 
> I am getting following error when tried to use tap with bonding:
> "Rx queue offloads 0x0 don't match port offloads 0x1000 or supported
> offloads 0x300e"
> 
> What is the intention here? I guess it tries to be sure requested queue
> offloads is subsets of port_offloads and offload_capability.
> If so not requesting any queue offload should be valid, isn't it?


 Port offload should be enabled on both port and queue configuration. This is part of the documentation [1], section "8.4.7.1. Per-Port and Per-Queue Offloads"
 On the above case, you try to enable the DEV_RX_OFFLOAD_CRC_STRIP on the port without enabling it on the queues. I guess DEV_RX_OFFLOAD_CRC_STRIP is reported by the tap PMD as port (and not queue) offload.

To answer some question asked privately by Ferruh:
>Why queue offload values should be set to enable per-port offload? I think this is wrong. We don't do queue offload at all, don't use that value, but this suggest that value should be some specific value to work, why?

The logic behind it was to make the offloads enablement more explicit.
Consider port offload is set on device configuration, but not set on the queue. there is an uncertainty whether the application wanted to "not enable" this offload or it was just not set (like in your case).

There other way around also makes sense: if offload was set on the port configuration then it is applied to each of the queues, with no way for the application to disable it through the queue configuration. 

The current API choose option #1, we can discuss to do small change to move to #2. 

>
>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]
http://dpdk.org/doc/guides/prog_guide/poll_mode_drv.html


> 
> <...>