On 17 May 2017 at 16:26, Darrell Ball <[email protected]> wrote: > > > On 5/17/17, 2:19 PM, "Joe Stringer" <[email protected]> wrote: > > On 16 May 2017 at 21:01, Darrell Ball <[email protected]> wrote: > > > > > > On 5/15/17, 6:00 AM, "[email protected] on behalf of > Bodireddy, Bhanuprakash" <[email protected] on behalf of > [email protected]> wrote: > > > > >Commit daf4d3c18da4("odp: Support conntrack orig tuple key.") > introduced > > >new fields in struct 'pkt_metadata'. pkt_metadata_init() is > called for every > > >packet in the userspace datapath. When testing a simple single > flow case with > > >DPDK, we observe a lower throughput after the above commit (it was > 14.88 > > >Mpps before, it is 13 Mpps after). > > > > > >This patch skips initializing ct_orig_tuple in pkt_metadata_init(). > > >It should be enough to initialize ct_state, because nobody should > look at > > >ct_orig_tuple unless ct_state is != 0. > > > > > >CC: Jarno Rajahalme <[email protected]> > > >Signed-off-by: Daniele Di Proietto <[email protected]> > > >--- > > >I'm sending this as an RFC because I didn't check very carefully > if we can really > > >avoid initializing ct_orig_tuple. > > > > > >Maybe there are better solutions to this problem. > > >--- > > > lib/packets.h | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > >diff --git a/lib/packets.h b/lib/packets.h index > a5a483bc8..6f1791c7a 100644 > > >--- a/lib/packets.h > > >+++ b/lib/packets.h > > >@@ -129,7 +129,7 @@ pkt_metadata_init(struct pkt_metadata *md, > > >odp_port_t port) > > > /* It can be expensive to zero out all of the tunnel > metadata. However, > > > * we can just zero out ip_dst and the rest of the data will > never be > > > * looked at. */ > > >- memset(md, 0, offsetof(struct pkt_metadata, in_port)); > > >+ memset(md, 0, offsetof(struct pkt_metadata, ct_orig_tuple)); > > > md->tunnel.ip_dst = 0; > > > md->tunnel.ipv6_dst = in6addr_any; > > > > > > > It's been a while this RFC patch has been submitted to fix > performance drop on Master. This indeed fixes the OvS-DPDK performance drop > that was introduced by the conntrack commit. > > Is there a better fix than what is suggested above? > > > > > > This affects both kernel and userspace. > > I tested this on both datapaths. > > LGTM > > How do we make sure that ct_orig_tuple isn't used uninitialized? Do we > need to zero out the ct_orig_tuple proto? > > There are a couple places where we explicitly set or reset the pkt metadata > ct_orig_tuple; > one is in pkt_metadata_from_flow(). > > I know there is a check for ct_orig_tuple proto in odp_key_from_pkt_metadata() > Can you find a case where can run this code without a set or reset of > ct_orig_tuple pkt md > or you are not sure ?
I wasn't sure, and I didn't see a response to the question that Daniele asked below the dashes in the original submission. Is miniflow_extract() safe wrt this? Seems like plausibly if the recirc_id is nonzero but there is no CT state (because, eg, bonds, or MPLS pop to IP, etc) it could end up populating the miniflow with garbage. In particular the path from emc_processing() seems suspect. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
