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

[Shahaf Shuler <shahafs@mellanox.com>]

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