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.
