Thanks Darrell for testing this patch. I will send on v2 with your suggestions below.
- Bhanuprakash. >-----Original Message----- >From: Darrell Ball [mailto:[email protected]] >Sent: Sunday, July 9, 2017 9:44 PM >To: Bodireddy, Bhanuprakash <[email protected]>; >[email protected] >Subject: Re: [ovs-dev] [PATCH] packets: Do not initialize ct_orig_tuple. > >I tested and found similar ballpark performance increase (approx. 10%) in the >most simple case with decreasing benefit as the pipeline gets more realistic >and useful. > >This ideal case incremental beyond the original fix patch (from Daniele) shows >a small decrease in performance of approx 100k pps (0.7 %). I cannot explain >that right now. >However, I think there is advantage in clearly defining what needs >initialization >and was does not and the additional incremental by Bhanu does that. >In realistic cases, I expect the 0.7 % difference, if it is real, to be much >less >anyways. > >Hence, this patch looks fine except for some comments inline. > > > >On 6/22/17, 2:08 PM, "[email protected] on behalf of >Bhanuprakash Bodireddy" <[email protected] on behalf of >[email protected]> wrote: > > Commit "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. > > It's discussed at: > https://urldefense.proofpoint.com/v2/url?u=https- >3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017- >2DMay_332419.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09C >GX7JQ5Ih- >uZnsw&m=8ZtjBQ0W_pVzaasUyrsZM_cp_rmDylXtUvA7D7F2UOw&s=min0TA >xgYGI118BI-LEDaDap8G8XuU4C9xJXtLLqIaE&e= > > Fixes: daf4d3c18da4("odp: Support conntrack orig tuple key.") > Signed-off-by: Daniele Di Proietto <[email protected]> > Signed-off-by: Bhanuprakash Bodireddy ><[email protected]> > Co-authored-by: Bhanuprakash Bodireddy ><[email protected]> > --- > Original RFC was posted by Daniele here: > https://urldefense.proofpoint.com/v2/url?u=https- >3A__mail.openvswitch.org_pipermail_ovs-2Ddev_2017- >2DMarch_329679.html&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA0 >9CGX7JQ5Ih- >uZnsw&m=8ZtjBQ0W_pVzaasUyrsZM_cp_rmDylXtUvA7D7F2UOw&s=QivDZb >3mJt6__TUIssDosbGfZlpAtRN7P_4p-lc4Zls&e= > > In this patch moved the offset from ct_orig_tuple to 'ct_orig_tuple_ipv6'. > This patch fixes the performance drop(~2.3Mpps for P2P - 64 byte pkts) > with OvS-DPDK on Master. > > lib/packets.h | 11 ++++++++++- > 1 file changed, 10 insertions(+), 1 deletion(-) > > diff --git a/lib/packets.h b/lib/packets.h > index a9d5e84..94c3dcc 100644 > --- a/lib/packets.h > +++ b/lib/packets.h > @@ -126,10 +126,19 @@ pkt_metadata_init_tnl(struct pkt_metadata *md) > static inline void > pkt_metadata_init(struct pkt_metadata *md, odp_port_t port) > { > + /* This is called for every packet in userspace datapath and affects > + * performance if all the metadata is initialized. > >I think you meant the sentence > > “Hence absolutely > + * necessary fields should be zeroed out.” > >to be something like > >“Hence, fields should only be zeroed out when necessary.” > > > + * > + * Initialize only the first 17 bytes of metadata (till ct_state). > >Do we really need to discuss “17 bytes” ? >Can the sentence be: >Initialize only till ct_state. > > + * Once the ct_state is zeroed out rest of ct fields will not be > looked > + * at unless ct_state != 0. > + */ > + memset(md, 0, offsetof(struct pkt_metadata, ct_orig_tuple_ipv6)); > + > /* 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)); > md->tunnel.ip_dst = 0; > md->tunnel.ipv6_dst = in6addr_any; > md->in_port.odp_port = port; > -- > 2.4.11 > > _______________________________________________ > dev mailing list > [email protected] > https://urldefense.proofpoint.com/v2/url?u=https- >3A__mail.openvswitch.org_mailman_listinfo_ovs- >2Ddev&d=DwICAg&c=uilaK90D4TOVoH58JNXRgQ&r=BVhFA09CGX7JQ5Ih- >uZnsw&m=8ZtjBQ0W_pVzaasUyrsZM_cp_rmDylXtUvA7D7F2UOw&s=pGKK0P >BsxExr3Bxhim-OKiWgLg3rhYtj2MqiosQ2Rfc&e= > > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
