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

[Bodireddy, Bhanuprakash]

Thanks Darrell for testing this patch. I will send on v2 with your suggestions below.

- Bhanuprakash.

>-----Original Message-----
>From: Darrell Ball [mailto:dball at vmware.com]
>Sent: Sunday, July 9, 2017 9:44 PM
>To: Bodireddy, Bhanuprakash <bhanuprakash.bodireddy at intel.com>;
>dev at 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 at openvswitch.org on behalf of
>Bhanuprakash Bodireddy" <ovs-dev-bounces at openvswitch.org on behalf of
>bhanuprakash.bodireddy at 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 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://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 at 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=
>
>
>