| Message ID | 1498165721-19086-1-git-send-email-bhanuprakash.bodireddy@intel.com |
|---|---|
| State | Superseded |
| Headers | show |
> -----Original Message----- > From: ovs-dev-bounces@openvswitch.org [mailto:ovs-dev- > bounces@openvswitch.org] On Behalf Of Bhanuprakash Bodireddy > Sent: Thursday, June 22, 2017 10:09 PM > To: dev@openvswitch.org > Subject: [ovs-dev] [PATCH] packets: Do not initialize ct_orig_tuple. > > 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://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332419.html > > Fixes: daf4d3c18da4("odp: Support conntrack orig tuple key.") > Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> > Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > Co-authored-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> > --- > Original RFC was posted by Daniele here: > https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329679.html > > 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. Hence absolutely > + * necessary fields should be zeroed out. > + * > + * Initialize only the first 17 bytes of metadata (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; Thanks for the patch Bhanu, I've tested this and can confirm the performance improvement. I'm not an expert on conntrack so not sure of the knock on effect (if any) it might have with that regard, maybe Darrell could comment on that aspect, but I would like to see this performance fix pushed. Tested-by: Ian Stokes <ian.stokes@intel.com> > -- > 2.4.11 > > _______________________________________________ > dev mailing list > dev@openvswitch.org > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
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, "ovs-dev-bounces@openvswitch.org on behalf of Bhanuprakash Bodireddy" <ovs-dev-bounces@openvswitch.org on behalf of bhanuprakash.bodireddy@intel.com> 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=BVhFA09CGX7JQ5Ih-uZnsw&m=8ZtjBQ0W_pVzaasUyrsZM_cp_rmDylXtUvA7D7F2UOw&s=min0TAxgYGI118BI-LEDaDap8G8XuU4C9xJXtLLqIaE&e=
Fixes: daf4d3c18da4("odp: Support conntrack orig tuple key.")
Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com>
Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
Co-authored-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com>
---
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=BVhFA09CGX7JQ5Ih-uZnsw&m=8ZtjBQ0W_pVzaasUyrsZM_cp_rmDylXtUvA7D7F2UOw&s=QivDZb3mJt6__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
dev@openvswitch.org
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=pGKK0PBsxExr3Bxhim-OKiWgLg3rhYtj2MqiosQ2Rfc&e=
Thanks Darrell for testing this patch. I will send on v2 with your suggestions below. - Bhanuprakash. >-----Original Message----- >From: Darrell Ball [mailto:dball@vmware.com] >Sent: Sunday, July 9, 2017 9:44 PM >To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy@intel.com>; >dev@openvswitch.org >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, "ovs-dev-bounces@openvswitch.org on behalf of >Bhanuprakash Bodireddy" <ovs-dev-bounces@openvswitch.org on behalf of >bhanuprakash.bodireddy@intel.com> 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 <diproiettod@vmware.com> > Signed-off-by: Bhanuprakash Bodireddy ><bhanuprakash.bodireddy@intel.com> > Co-authored-by: Bhanuprakash Bodireddy ><bhanuprakash.bodireddy@intel.com> > --- > 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 > dev@openvswitch.org > 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= > > >
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. Hence absolutely + * necessary fields should be zeroed out. + * + * Initialize only the first 17 bytes of metadata (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;
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://mail.openvswitch.org/pipermail/ovs-dev/2017-May/332419.html Fixes: daf4d3c18da4("odp: Support conntrack orig tuple key.") Signed-off-by: Daniele Di Proietto <diproiettod@vmware.com> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> Co-authored-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy@intel.com> --- Original RFC was posted by Daniele here: https://mail.openvswitch.org/pipermail/ovs-dev/2017-March/329679.html 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(-)