Hi Olivier, > > Hi, > > Jumping into this thread, it looks it's the last pending patch remaining for > the release. > > For reference, the idea of tx_prep() was mentionned some time ago in > http://dpdk.org/ml/archives/dev/2014-May/002504.html > > Few comments below. > > On 07/28/2016 03:01 PM, Ananyev, Konstantin wrote: > > Right now to make HW TX offloads to work user is required to do particular > > actions: > > 1. set mbuf.ol_flags properly. > > 2. setup mbuf.tx_offload fields properly. > > 3. update L3/L4 header fields in a particular way. > > > > We move #3 into tx_prep(), to hide that complexity from the user simplify > > things for him. > > Though if he still prefers to do #3 by himself - that's ok too. > > I think moving #3 out of the application is a good idea. Today, for TSO, the > offload dpdk API requires to set a specific pseudo header > checksum (which does not include the ip len, as expected by Intel drivers), > and set the IP checksum to 0. > > In our app, the network stack sets the TCP checksum to the standard pseudo > header checksum, and before sending the mbuf: > - packets are split in sw if the driver does not support tso > - the tcp csum is patched to match dpdk api if the driver supports tso > > In the patchs I've recently sent adding tso support for virtio-pmd, it > conforms to the dpdk API (phdr csum without ip len), so the tx function > need to patch the mbuf data inside the driver, which is something what we > want to avoid, for some good reasons explained by Konstantin.
Yep, that would be another good use-case for tx_prep() I suppose. > > So, I think having a tx_prep would also be the occasion to standardize a bit > the dpdk offload api, and let the driver-specific stuff in tx_prep(). > > Konstantin, any opinion about this? Yes, that sounds like a good thing to me. > > >>> Another main purpose of tx_prep(): for multi-segment packets is to > >>> check that number of segments doesn't exceed HW limit. > >>> Again right now users have to do that on their own. > > If calling tx_prep() is optional, does it mean that this check may be done > twice? (once in tx_prep() and once in tx_burst()) I meant 'optional' in a way, that if user doesn't want to use tx_prep() and do step #3 from the above on his own (what happens now) that is still ok. But I think step #3 (modify packet's data) still needs to be done before tx_burst() is called for the packets. > > What would be the advantage of doing this check in tx_prep() instead of > keeping it in tx_burst(), as it does not touch the mbuf data? > > >>> 3. Having it a s separate function would allow user control when/where > >>> to call it, let say only for some packets, or probably call > >>> tx_prep() > >>> on one core, and do actual tx_burst() for these packets on the > >>> other. > > Yes, from what I remember, the pipeline model was the main reason why we do > not just modify the packet in tx_burst(). Right? Yes. > > >>>>> If you refer as lost cycles here something like: > >>>>> RTE_FUNC_PTR_OR_ERR_RET(*dev->dev_ops->tx_prep, -ENOTSUP); then > >>>>> yes. > >>>>> Though comparing to actual work need to be done for most HW TX > >>>>> offloads, I think it is neglectable. > >>>> > >>>> Not sure. > > I think doing this test on a per-bulk basis should not impact performance. > > > To be honest, I don't understand what is your concern here. > > That proposed change doesn't break any existing functionality, doesn't > > introduce any new requirements to the existing API, and wouldn't > > introduce any performance regression for existing apps. > > It is a an extension, and user is free not to use it, if it doesn't fit his > > needs. > > From other side there are users who are interested in that > > functionality, and they do have use-cases for it. > > In my opinion, using tx_prep() will implicitly become mandatory as soon as > the application want to do offload. An application that won't use > it will have to prepare the mbuf, and this preparation will depend on the > device, which is not acceptable inside an application. Yes, I also hope that most apps that do use TX offloads will start to use it, as I think it will be much more convenient way, then what we have right now. I just liked to emphasis that user wouldn't be forced to. Konstantin > > > So, to conclude, the api change notification looks good to me, even if there > is still some room to discuss the implementation details. > > > Acked-by: Olivier Matz <olivier.matz at 6wind.com>
