Hi Ian, Thanks, comments in-line.
On 11/01/2019 10:50, Ian Stokes wrote: > On 1/10/2019 4:58 PM, Tiago Lam wrote: >> Given that multi-segment mbufs might be sent between interfaces that >> support different capabilities, and may even support different layouts >> of mbufs, outgoing packets should be validated before sent on the egress >> interface. Thus, netdev_dpdk_eth_tx_burst() now calls DPDK's >> rte_eth_tx_prepare() function, if and only multi-segments is enbaled, in >> order to validate the following (taken from the DPDK documentation), on >> a device specific manner: >> - Check if packet meets devices requirements for tx offloads. >> - Check limitations about number of segments. >> - Check additional requirements when debug is enabled. >> - Update and/or reset required checksums when tx offload is set for >> packet. >> > > Thanks Tiago comments below. > >> Signed-off-by: Tiago Lam <[email protected]> >> --- >> lib/netdev-dpdk.c | 21 +++++++++++++++++++-- >> 1 file changed, 19 insertions(+), 2 deletions(-) >> >> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c >> index d6114ee..77d04fc 100644 >> --- a/lib/netdev-dpdk.c >> +++ b/lib/netdev-dpdk.c >> @@ -2029,6 +2029,10 @@ netdev_dpdk_rxq_dealloc(struct netdev_rxq *rxq) >> >> /* Tries to transmit 'pkts' to txq 'qid' of device 'dev'. Takes ownership >> of >> * 'pkts', even in case of failure. >> + * In case multi-segment mbufs / TSO is being used, it also prepares. In >> such >> + * cases, only the prepared packets will be sent to Tx burst, meaning that >> if >> + * an invalid packet appears in 'pkts'[3] only the validated packets in >> indices >> + * 0, 1 and 2 will be sent. >> * >> * Returns the number of packets that weren't transmitted. */ >> static inline int >> @@ -2036,11 +2040,24 @@ netdev_dpdk_eth_tx_burst(struct netdev_dpdk *dev, >> int qid, >> struct rte_mbuf **pkts, int cnt) >> { >> uint32_t nb_tx = 0; >> + uint16_t nb_prep = cnt; >> >> - while (nb_tx != cnt) { >> + if (dpdk_multi_segment_mbufs) { > As this feature would be experimental and not enabled, by default this > would be false. > > Would it make sense to deal with the default non multiseg case first? > > Extra checks in the datapath can be costly and I'd like to minimize any > impact for the non multi seg traffic which is the default use case > currently. Possibly checking for !dpdk_multi_segment_mbufs first? Or > possibly the use of OVS_LIKELY/OVS_UNLIKELY. Have you given thought to this? Sounds reasonable to me, since this won't be the default. I'll use `OVS_UNLIKELY` for the next iteration. > >> + /* Validate the burst of packets for Tx. */ >> + nb_prep = rte_eth_tx_prepare(dev->port_id, qid, pkts, cnt); > > So one of the gotchas here is that rte_eth_tx_prepare is experimental > although I didn't see any compilation issues (warnings etc.) and it > builds OK with travis. > > Possibly a comment above it to explain it's currently experimental and > subject to change in DPDK would be useful, can be removed once the api > is non experimental in DPDK. It would be one of the conditions to remove > the experimental tag for TSO in OVS DPDK I suspect in the future. Thanks for bringing this up. I can see from the thread that this is pretty much clarified by now, but do we still want to include a comment? Since the docs are also versioned, and David's patch (thanks!) will be for master, I assume we still want a comment here, since people might be looking into DPDK's v18.11 docs specifically. Tiago. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
