Re: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API

[Shahaf Shuler <shahafs@mellanox.com>]

Hi Konstantin,

Monday, September 4, 2017 4:25 PM, Ananyev, Konstantin:
> 
> Hi Shahaf,
> 
> >  }
> >
> > +/**
> > + * A conversion function from rxmode offloads API to rte_eth_rxq_conf
> > + * offloads API.
> > + */
> > +static void
> > +rte_eth_convert_rxmode_offloads(struct rte_eth_rxmode *rxmode,
> > +				struct rte_eth_rxq_conf *rxq_conf) {
> > +	if (rxmode->header_split == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_HEADER_SPLIT;
> > +	if (rxmode->hw_ip_checksum == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CHECKSUM;
> > +	if (rxmode->hw_vlan_filter == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_FILTER;
> 
> Thinking on it a bit more:
> VLAN_FILTER is definitely one per device, as it would affect VFs also.
> At least that's what we have for Intel devices (ixgbe, i40e) right now.

This is vendor specific. For Mellanox this is one per device (regardless if it is a vf/pf).

> For Intel devices VLAN_STRIP is also per device and will also be  applied to all
> corresponding VFs.

Again - vendor specific. For Mellanox is per queue.

> In fact, right now it is possible to query/change these 3 vlan offload flags on
> the fly (after dev_start) on  port basis by
> rte_eth_dev_(get|set)_vlan_offload API.
> So, I think at least these 3 flags need to be remained on a port basis.

Am not sure I agree. 
Why, for example, block from application the option to set some queues with vlan strip and some without if device allows?
Also how will we decide which offloads should stay per port and which are allowed to move per queue? this much depends on the underlying PMD.

Looks like i missed that part on ethdev, and if Rx offload will be per queue I will need to change it also.


> In fact, why can't we have both per port and per queue RX offload:
> - dev_configure() will accept RX_OFFLOAD_* flags and apply them on a port
> basis.
> - rx_queue_setup() will also accept RX_OFFLOAD_* flags and apply them on
> a queue basis.
> - if particular RX_OFFLOAD flag for that device couldn't be setup on a queue
> basis  -
>    rx_queue_setup() will return an error.

Why not taking the per port configuration as a sub-case of per queue configuration?
For per-port offloads as long as the same configuration applies the queue setup succeeds.


> - rte_eth_rxq_info can be extended to provide information which
> RX_OFFLOADs
>   can be configured on a per queue basis.

I am OK with the info suggestion. 

> BTW - in that case we probably wouldn't need ignore flag inside rx_conf
> anymore.
> 
> 
> > +	if (rxmode->hw_vlan_strip == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_STRIP;
> > +	if (rxmode->hw_vlan_extend == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_VLAN_EXTEND;
> > +	if (rxmode->jumbo_frame == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_JUMBO_FRAME;
> 
> There are some extra checks for that flag inside rte_eth_dev_configure().
> If we going so support it per queue - then it probably need to be updated.
> 
> > +	if (rxmode->hw_strip_crc == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_CRC_STRIP;
> > +	if (rxmode->enable_scatter == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_SCATTER;
> > +	if (rxmode->enable_lro == 1)
> > +		rxq_conf->offloads |= DEV_RX_OFFLOAD_TCP_LRO; }
> > +
> 
> Konstantin