[dpdk-dev] [PATCH v2 6/7] net/mlx4: convert to new Tx offloads API

[Nelio Laranjeiro]

Hi Shahaf,

On Thu, Jan 04, 2018 at 11:55:17AM +0000, Shahaf Shuler wrote:
> Hi Adrien and Nelio,
> 
> See below comment regarding your output on the offload check.
> Rest of the comments were accepted.
> 
> Wednesday, January 3, 2018 7:29 PM, Adrien Mazarguil :
> 
> [...]
> 
> > 
> > > +{
> > > +	uint64_t port_offloads = priv->dev->data-
> > >dev_conf.txmode.offloads;
> > > +	uint64_t port_supp_offloads =
> > mlx4_priv_get_tx_port_offloads(priv);
> > 
> > Instead of a redundant "port_", how about clarifying it all as follows:
> > 
> >  offloads -> requested
> >  port_offloads -> mandatory
> >  port_supp_offloads -> supported
> > 
> > > +
> > > +	if ((port_offloads ^ offloads) & port_supp_offloads)
> > > +		return 0;
> > > +	return 1;
> > 
> > And simplify this as:
> > 
> >  return !((mandatory ^ requested) & supported);
> > 
> > Maybe I missed something, but there seems to be an inconsistency, e.g.
> 
> You are correct that the purpose of this function is to check if the offload configuration is correct.
> However the current code being done on mlx4 does not validate if the queue offloads configured are supported. 
> It only validates if the port offloads configuration matches the queue offload configuration.
> 
> The reason it lack the supported offloads check was discussed in internal mail (you both CC I believe). Generally it was due to the fact that CRC and VLAN strip offloads are not supported by the PMD, however set for almost every example/application in DPDK.
> For the complete check look on mlx5 patches on this series. 
> 
> > requesting an unsupported offload does not necessarily fail:
> > 
> >  mandatory = 0x00
> >  requested = 0x40
> >  supported = 0x10
> > 
> >  => valid but shouldn't be
> 
> It should if the offload is per-queue offload.
> 
> 
> > 
> > And requesting a supported offload when there are no mandatory ones
> > should not be a problem:
> > 
> >  mandatory = 0x00
> >  requested = 0x10
> >  supported = 0x10
> > 
> >  => invalid but it should be
> 
> It is invalid indeed. If the application declare some port offload not to be set on dev_configure, it cannot enable it from the queue setup.
> Port offloads can be set only on device configuration, and when set every queue should have them set as well.
> 
> > 
> > A naive translation of the above requirements results in the following
> > expression:
> > 
> >  return (requested | supported) == supported &&
> >         (requested & mandatory) == mandatory;
> > 
> > What's your opinion?
> > 

>From an application point of view, it seems strange to provide an
already configured offload when configuring the queues, i.e.
rte_eth_dev_configure() is called before rte_eth_{tx,rx}_queue_setup().

I think this "mandatory" information should be removed from the API
documentation letting the application the capability to request a null
offload when configuring the queue.
As this modification does not break the API/ABI it only needs eventually
a modification in the driver, it can be done in the future.

For mlx5 part:
Acked-by: Nelio Laranjeiro <nelio.laranjeiro at 6wind.com>

-- 
Nélio Laranjeiro
6WIND