On 17 May 2017 at 17:54, Joe Stringer <[email protected]> wrote: > 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.
Perhaps this is actually OK, because the ct_state would be initialized to zero in pkt_metadata_init(), and the classifier would never look at ct_orig_tuple unless the ct_state is valid (according to the ovs-fields(7) documentation). In which case, it shouldn't(?) matter that garbage is written to the miniflow. Alternatively if md_is_valid (eg, because it was recirculated), then it's up to the other callers to ensure that md.ct_state was properly initialized. Effectively this is done by the write_ct_md() function. I wonder though if actually the miniflow_extract() "ct_nw_proto_p" initialization should only occur if the md->ct_state is valid. That could save a few extra cycles in a similar vein to how this patch proposes. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
