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

["Ananyev, Konstantin" <konstantin.ananyev@intel.com>]

> -----Original Message-----
> From: Shahaf Shuler [mailto:shahafs@mellanox.com]
> Sent: Monday, September 4, 2017 3:03 PM
> To: Ananyev, Konstantin <konstantin.ananyev@intel.com>; Thomas Monjalon <thomas@monjalon.net>
> Cc: dev@dpdk.org
> Subject: RE: [dpdk-dev] [PATCH 4/4] ethdev: add helpers to move to the new offloads API
> 
> 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.

Yep, I understand it varies quite a lot from vendor from vendor.
That's why I started to think we need to have to allow user to specify rx_offloads
for both port and queue basis.

> 
> > 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.

Do you mean that as long as queue config for offload X matches port config for the same offload - it is ok,
even if for that particular device offload X is per port not per queue?

Let say:
...
rxconf.offloads = DEV_RX_OFFLOAD_VLAN_STRIP;
...
rte_eth_dev_configure(port, ...);
...

/* queue offloads matches port offloads - always  */
rte_eth_rx_queue_setup(port, ..., &rcxonf);
...
rxconf.offloads &= ~ DEV_RX_OFFLOAD_VLAN_STRIP;

/* ok for devices where vlan_stip per queue is supported,
  * fails for devices with vlan_strip offload is per device.
  */ 
rte_eth_rx_queue_setup(port, ..., &rcxonf);

?

Konstantin





 

> 
> 
> > - 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