[ovs-dev] [PATCH] packets: Do not initialize ct_orig_tuple.

[Stokes, Ian]

> -----Original Message-----
> From: ovs-dev-bounces at openvswitch.org [mailto:ovs-dev-
> bounces at openvswitch.org] On Behalf Of Bhanuprakash Bodireddy
> Sent: Thursday, June 22, 2017 10:09 PM
> To: dev at 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 at vmware.com>
> Signed-off-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at intel.com>
> Co-authored-by: Bhanuprakash Bodireddy <bhanuprakash.bodireddy at 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 at intel.com>
> --
> 2.4.11
> 
> _______________________________________________
> dev mailing list
> dev at openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev