[ovs-dev] [PATCH v10 00/14] Support multi-segment mbufs

Lam, Tiago tiago.lam at intel.com
Tue Oct 9 13:01:10 UTC 2018


On 08/10/2018 18:04, Ilya Maximets wrote:
> Current patch-set breaks a lot of STP unit tests:

Thanks for pointing that out, I've fixed them now. I wasn't aware that
those tests were running as part of that testsuite, so I'll start
running that as well from now on.

> 
> ## ------------------------------- ##
> ## openvswitch 2.10.90 test suite. ##
> ## ------------------------------- ##
> 
> ofproto-dpif
> 
> 1149: ofproto-dpif - MPLS handling                    FAILED (ofproto-dpif.at:2068)
> 1164: ofproto-dpif - VLAN+MPLS handling               FAILED (ofproto-dpif.at:3921)
> 
> Spanning Tree Protocol unit tests
> 
> 2534: STP example from IEEE 802.1D-1998               FAILED (stp.at:18)
> 2535: STP example from IEEE 802.1D-2004 figures 17.4 and 17.5 FAILED (stp.at:61)
> 2536: STP example from IEEE 802.1D-2004 figure 17.6   FAILED (stp.at:87)
> 2537: STP example from IEEE 802.1D-2004 figure 17.7   FAILED (stp.at:116)
> 2538: STP.io.1.1: Link Failure                        FAILED (stp.at:155)
> 2539: STP.io.1.2: Repeated Network                    FAILED (stp.at:181)
> 2540: STP.io.1.4: Network Initialization              FAILED (stp.at:205)
> 2541: STP.io.1.5: Topology Change                     FAILED (stp.at:258)
> 2545: STP.op.3.3: Root Bridge Selection: Bridge ID Values FAILED (stp.at:337)
> 2546: STP.op.3.3: Root Bridge Selection: Bridge ID Values FAILED (stp.at:360)
> 
> ## ------------- ##
> ## Test results. ##
> ## ------------- ##
> 
> ERROR: All 12 tests were run,
> 12 failed unexpectedly.
> ## -------------------------- ##
> ## testsuite.log was created. ##
> ## -------------------------- ##
> 
> Please send `tests/testsuite.log' and all information you think might help:
> 
>    To: <bugs at openvswitch.org>
>    Subject: [openvswitch 2.10.90] testsuite: 1149 1164 2534 2535 2536 2537 2538 2539 2540 2541 2545 2546 failed
> 
> ----------------------------------------------------------------------
> 
> Meanwhile, I still have a few concerns about this patch-set
> in general:
> 
> 1. Most of dp_packet APIs are now able to fail.
>    But callers doesn't check results in most cases.

Could you be a bit more specific here? Or do you mean the dp_packet
*layer* APIs, such as dp_packet_l3() and variants? As I understand,
while it is true that they may now return NULL in new cases where it
wouldn't happen before, it will happen very rarely (not only your
headers would have to exceed the 1920B, but you'd have to opt-in and use
multi-seg mbufs). Thus, I would rather fix this on a case by case basis
than polluting the code with NULL checks. As a good example there's OVN,
where the packets processed are always linear, and thus placing NULL
checks is almost pointless. What are your thoughts?

> 
> 2. Many drivers has its own restrictions on the number of segments
>    they able to handle in a single packet. For example, igb driver
>    supports only up to 8 segments. I think, that we have to call
>    tx preparation API and check the result before sending segmented
>    packets. It's unclear, what to do with such packets. Will we
>    have to have large sized mempool anyway for this case?
> 
> 2.1 OVS will migrate to DPDK 18.11 as soon as it will be released,
>     but there are few PMDs that doesn't support segmented packets
>     at all (octeontx and axgbe, for example). Maybe it'll be better
>     to work on this patch set on top of ovs/dpdk-latest branch and
>     include all the required checks for DEV_TX_OFFLOAD_MULTI_SEGS
>     flag in order to not block upgrade to 18.11 ?

I wouldn't oppose to that, depending on the timelines it could work. But
my understanding is that one can do that already on v17.11 with the
"old" `ETH_TXQ_FLAGS_NOMULTSEGS` flag. It is a valid point that the
`tx_offload_capa` of each device is not being checked at the moment though.

> 
> One code specific thing (I did not review the whole code. This just
> one thing that I noticed accidentally.):
> 
> 3. dp_packet_equal should not fail in case of different memory
>    layout, but equal data.

Sure. Done. I'll send that in the next iteration.

Tiago.


More information about the dev mailing list