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.laranje...@6wind.com>
--
Nélio Laranjeiro
6WIND
