RE: [PATCH v2] app/testpmd: use Tx preparation in txonly engine

[Konstantin Ananyev <konstantin.ananyev@huawei.com>]

> Subject: RE: [PATCH v2] app/testpmd: use Tx preparation in txonly engine
> 
> > > >>>>>>> TSO breaks when MSS spans more than 8 data fragments. Those
> > > >>>>>>> packets will be dropped by Tx preparation API, but it will
> > > >>> cause
> > > >>>>>>> MDD event if txonly forwarding engine does not call the Tx
> > > >>>>> preparation
> > > >>>>>>> API before transmitting packets.
> > > >>>>>>>
> > > >>>>>>
> > > >>>>>> txonly is used commonly, adding Tx prepare for a specific case
> > > >>> may
> > > >>>>>> impact performance for users.
> > > >>>>>>
> > > >>>>>> What happens when driver throws MDD (Malicious Driver
> > Detection)
> > > >>>>> event,
> > > >>>>>> can't it be ignored? As you are already OK to drop the packet,
> > > >>> can
> > > >>>>>> device be configured to drop these packages?
> > > >>>>>>
> > > >>>>>>
> > > >>>>>> Or as Jerin suggested adding a new forwarding engine is a
> > > >>> solution,
> > > >>>>> but
> > > >>>>>> that will create code duplication, I prefer to not have it if
> > > >>> this
> > > >>>>> can
> > > >>>>>> be handled in device level.
> > > >>>>>
> > > >>>>> Actually I am agree with the author of the patch - when TX
> > offloads
> > > >>>>> and/or multisegs are enabled,
> > > >>>>> user supposed to invoke eth_tx_prepare().
> > > >>>>> Not doing that seems like a bug to me.
> > > >>>>
> > > >>>> I strongly disagree with that statement, Konstantin!
> > > >>>> It is not documented anywhere that using TX offloads and/or
> > multisegs
> > > >>> requires calling rte_eth_tx_prepare() before
> > > >>>> rte_eth_tx_burst(). And none of the examples do it.
> > > >>>
> > > >>> In fact, we do use it for test-pmd/csumonly.c.
> > > >>> About other sample apps:
> > > >>> AFAIK, not many of other DPDK apps do use L4 offloads.
> > > >>> Right now special treatment (pseudo-header cksum calculation) is
> > needed
> > > >>> only for L4 offloads (CKSUM, SEG).
> > > >>> So, majority of our apps who rely on other TX offloads (multi-seg,
> > ipv4
> > > >>> cksum, vlan insertion) happily run without
> > > >>> calling tx_prepare(), even though it is not the safest way.
> > > >>>
> > > >>>>
> > > >>>> In my opinion:
> > > >>>> If some driver has limitations for a feature, e.g. max 8
> > fragments,
> > > >>> it should be documented for that driver, so the application
> > > >>>> developer can make the appropriate decisions when designing the
> > > >>> application.
> > > >>>> Furthermore, we have APIs for the drivers to expose to the
> > > >>> applications what the driver supports, so the application can
> > configure
> > > >>>> itself optimally at startup. Perhaps those APIs need to be
> > expanded.
> > > >>>> And if a feature limitation is common across the majority of
> > drivers,
> > > >>> that limitation should be mentioned in the documentation of the
> > > >>>> feature itself.
> > > >>>
> > > >>> Many of such limitations *are* documented and in fact we do have
> > an API
> > > >>> to check max segments that each driver support,
> > > >>> see struct rte_eth_desc_lim.
> > > >>
> > > >> Yes, this is the kind of API we should provide, so the application
> > can configure itself appropriately.
> > > >>
> > > >>> The problem is:
> > > >>> - none of our sample app does proper check on these values, so
> > users
> > > >>> don't have a good example how to do it.
> > > >>
> > > >> Agreed.
> > > >> Adding an example showing how to do it properly would be the best
> > solution.
> > > >> Calling tx_prepare() in the examples is certainly not the solution.
> > > >>
> > > >>> - with current DPDK API not all of HW/PMD requirements could be
> > > >>> extracted programmatically:
> > > >>>    let say majority of Intel PMDs for TCP offloads expect pseudo-
> > header
> > > >>> cksum to be pre-calculated by the SW.
> > > >>
> > > >> I hope this requirement is documented somewhere.
> > > >>
> > > >>>    another example, some HW expects pkt_len to be bigger then some
> > > >>> threshold value, otherwise HW hang may appear.
> > > >>
> > > >> I hope this requirement is also documented somewhere.
> > > >
> > > > No idea, I found it only in the code.
> > >
> > > IMHO Tx burst must check such limitations. If you made your HW simpler
> > > (or just lost it on initial testing), pay in your drivers (or your
> > > HW+driver will be unusable because of such problems).
> > >
> > > >> Generally, if the requirements cannot be extracted
> > programmatically, they must be prominently documented, like this note to
> > > >> rte_eth_rx_burst():
> > > >
> > > > Obviously, more detailed documentation is always good, but...
> > > > Right now we have 50+ different PMDs from different vendors.
> > > > Even if each and every of them will carefully document all possible
> > limitations and necessary preparation steps,
> > > > how DPDK app developer supposed to  deal with all that?
> > > > Do you expect everyone, to read carefully through all of them, and
> > handle all of them properly oh his own
> > > > in each and every DPDK app he is going to write?
> > > > That seems unrealistic.
> > > > Again what to do with backward compatibility: when new driver (with
> > new limitations) will arise
> > > > *after* your app is already written and tested?
> > >
> > > +1
> > >
> > > >>
> > > >>   * @note
> > > >>   *   Some drivers using vector instructions require that *nb_pkts*
> > is
> > > >>   *   divisible by 4 or 8, depending on the driver implementation.
> > >
> > > I'm wondering what application should do if it needs to send just one
> > > packet and do it now. IMHO, such limitations are not acceptable.
> 
> This common limitation for vector drivers is for RX burst, not TX burst.
> I agree such a limitation would be unacceptable for TX.
> 
> > >
> > > >>
> > > >>> - As new HW and PMD keep appearing it is hard to predict what
> > extra
> > > >>> limitations/requirements will arise,
> > > >>>    that's why tx_prepare() was introduced as s driver op.
> > > >>>
> > > >>>>
> > > >>>> We don't want to check in the fast path what can be checked at
> > > >>> startup or build time!
> > > >>>
> > > >>> If your app supposed to work with just a few, known in advance,
> > NIC
> > > >>> models, then sure, you can do that.
> > > >>> For apps that supposed to work 'in general'  with any possible
> > PMDs
> > > >>> that DPDK supports - that might be a problem.
> > > >>> That's why tx_prepare() was introduced and it is strongly
> > recommended
> > > >>> to use it by the apps that do use TX offloads.
> > > >>> Probably tx_prepare() is not the best possible approach, but right
> > now
> > > >>> there are not many alternatives within DPDK.
> > > >>
> > > >> What exactly is an application supposed to do if tx_prepare()
> > doesn't accept the full burst? It doesn't return information about
> > > what is
> > > >> wrong.
> > > >
> > > > It provides some information, but it is *very* limited: just index
> > of 'bad' packet and error code.
> > > > In theory app can try to handle it in a 'smart' way: let say if
> > ENOTSUP is returned, then try to disable all HW offloads
> > > > and do all in SW. But again, it is much better to do so *before*
> > submitting packets for TX, so in practice everyone
> > > > just drop such 'bad' packets.
> > > >
> > > >   Dropping the packets might not be an option, e.g. for applications
> > used in life support or tele-medicine.
> > >
> > > Critical applications should be able to do all Tx offloads in SW and
> > > retry. Of course, various statistics and logs should help to improve
> > the
> > > application.
> > >
> > > > If the packet is 'bad', then it is much better to drop it, then TX
> > corrupted packet or even hang NIC HW completely.
> > >
> > > IMHO Tx burst must drop packet which could hang NIC HW completely. I
> > > realize that it is an extra checks and performance drop, but vendor
> > > should pay in performance if HW is not good enough.
> > >
> > > > Though off-course it is much better to have an app that would check
> > for limitations that can be checked by API provided
> > > > and enable only supported offloads.
> > >
> > > Yes, that's why API to get limitation is much better than
> > documentation.
> > >
> > > >> If limitations are documented, an application can use the lowest
> > common denominator of the NICs it supports. And if the
> > > application is
> > > >> supposed to work in general, that becomes the lowest common
> > denominator of all NICs.
> > > >
> > > > I agree: for limitations that can be extracted with generic API,
> > like:
> > > > number of segments per packet, supported TX offloads, mbuf fileds
> > that must be provided for each TX offload,  etc. -
> > > > it is responsibility of well-written application to obey all of
> > them.
> > > > Yes, many tx_prepare() implementations do  such checks anyway, but
> > from my perspective it is sort of last-line of defense.
> > > > For well written application that should just never happen.
> > > > But there is one more important responsibility of tx_prepare() -  it
> > performs PMD specific packet modifications for requested
> > > offloads.
> > > > As I already mentioned for Intel NICs - it does pseudo-header cksum
> > calcucation, for packets with size less then minimal, it can
> > > > probably do padding (even if doesn't do it right now), for some
> > other PMDs - might be something else, I didn't check.
> > > > Obviously it saves app developer from a burden to do all these
> > things on his own.
> > > >
> > > >> It looks like tx_prepare() has become a horrible workaround for
> > undocumented limitations.
> > >
> > > I strongly disagree. Documentation is never a solution for a generic
> > > application which is intended to work on any HW and IMHO it is the
> > > goal to have more and more applications which work on any HW.
> > >
> > > >> Limitations due to hardware and/or software tradeoffs are
> > unavoidable, so we have to live with them; but we should not accept
> > > >> PMDs with undocumented limitations.
> > > >
> > > > As I already said, more documentation never hurts, but for that
> > case, I think it is not enough.
> > > > I expect PMD to provide a tx_prepare() implementation that would
> > deal with such specific things.
> > >
> > > +1
> > > > Anyway, back to the original patch - I looked at it once again, and
> > realized that the problem
> > > > is just in the unsupported number of segments.
> > > > As we discussed above - such limitations should be handled by well
> > written app,
> > > > but none of DPDK apps does it right now.
> > > > So probably it is good opportunity to do things in a proper way and
> > introduce such checks
> > > > in testpmd ;)
> > >
> > > +1 since we already have fields in device information to report such
> > > limitations, but it does not say that Tx prepare should be dropped.
> > > Drivers which don't need Tx prepare keep it NULL and it returns
> > > immediately from ethdev. Since it is done per-burst, it should not
> > > affect performance a lot.
> > >
> >
> > 100% agree. I think tx_prepare needs to stay, and in general has to be
> > strongly recommended
> > for apps that do use TX offloads/multi-segs.
> 
> <irony>
> Then tx_prepare should randomly reject packets and segments, to ensure the application has sufficient SW fallback implemented.
> </irony>
> 
> No, seriously, considering the above arguments, I think PMD conformance requirements would be a better solution.
> If a PMD claims support for some feature, it must conform to some minimum requirements for that feature.
> 
> E.g. support for multi-seg TX must be able to handle min. 8 segments (or some other reasonable number).

Hmm... why is that?
I thought we had a consensus here that good behaving app should use existing API (rte_eth_desc_lim) to deduce min common denominator,
and that we need some code example, or even better a lib function that would do such thing. 

> 
> Common limitations and preconditions, such as minimum RX burst size for vector drivers and pseudo-header checksum precalculation
> for TCP offload, should be accepted and prominently documented.

I still don't understand why these preconditions can't be done by tx_prepare().

> Unusual HW limitations, such as inability to pad short Ethernet frames, should be handled by the driver's TX function as workarounds
> for unacceptable limitations (in DPDK API context) in the HW.

With current design, rte_eth_tx_burst() is not allowed to modify mbufs whose refcnt > 1.
That's one of the reason why tx_prepare() was introduced.