Re: [dpdk-dev] Rx/Tx offloads checks behaviour in 18.05

[Thomas Monjalon <thomas@monjalon.net>]

14/05/2018 16:10, Ferruh Yigit:
> On 5/14/2018 11:19 AM, Shahaf Shuler wrote:
> > Monday, May 14, 2018 1:00 PM, Ferruh Yigit:
> >>>
> >>> It depends on which PMD is used. Yes, it was no checks in ethdev before.
> >>> If PMD does not support multi-segment Tx, some checksum or VLAN
> >>> insertion offload, but application requests it and rely on it, it will
> >>> result in invalid packets sent to network.
> >>>
> >>> I realize that some applications may simply use empty txq_flags, but
> >>> do not use any offloads in fact. If so, such PMDs will fail to setup
> >>> TxQ if checks are made fatal, return error and underlying PMD does not
> >>>  support these offloads.
> >>>
> >>> At least it is safer behaviour than transmitting garbage.
> >>> Yes, not easy decision.
> >>>
> >>> I will publish my patches which passed our tests.
> >>>
> >>>
> >>>
> >>> I agree with Andrew here. Even though there is a concern about the
> >>> existing application we cannot use only logging.
> >>>
> >>> It is better to enforce the right behavior rather than having wrong
> >>> configuration silently accepted by the PMD.
> >>
> >> What do you think send an error for the application that switch to new
> >> offloading API and only print error log for the old ones. I believe we can
> >> detect this via ETH_TXQ_FLAGS_IGNORE and ignore_offload_bitfield.
> >>
> >> And we will already force old applications to switch to new API next release,
> >> next release we can introduce return error without condition.
> >> Does it make sense?
> > 
> > The issue is currently none of the PMDs verify the offloads v.s. the caps as ethdev layer does it. 
> > We cannot have offload to be set if it is not supported, even under the old API. allowing such breach on ethdev means we need to enforce it back again on each PMD. 
> 
> Hi Shahaf, Andrew,
> 
> Thank you for sharing your concern I agree this is a valid one and thanks for
> the solution provided.
> 
> Ethdev configure() and queue_setup() are two common functions used for almost
> all (if not all) DPDK networking applications, independent from they are
> interested in offloading or not.
> I am not comfortable on possibility to break them when we already scheduled a
> break next release withing the process.
> 
> With Wei's latest updates within comment it mentions return error will be added
> in next release, this is good from documenting intention point of view.
> 
> 
> With taking the pressure to close the v18.05 I am for getting offloading patch
> as it is and reject Andrew's update.
> And do new offloading API related work, including adding these returns, early in
> next release (18.08)

It is a really difficult decision, but I agree with Ferruh.
We discussed it and we cannot take more risk in 18.05, even if it is
a bad choice from API perspective.

Thank you Andrew, and let's fix it in early days of 18.08.