[ovs-dev,v1] flow: miniflow_extract metadata branchless optimization
diff mbox series

Message ID 1566466200-45281-1-git-send-email-Yanqin.Wei@arm.com
State New
Headers show
Series
  • [ovs-dev,v1] flow: miniflow_extract metadata branchless optimization
Related show

Commit Message

Yanqin Wei Aug. 22, 2019, 9:30 a.m. UTC
"miniflow_extract" is a branch heavy implementation for packet header and
metadata parsing. There is a lot of meta data handling for all traffic.
But this should not be applicable for packets from interface.
This patch adds a layer of inline encapsulation to miniflow_extract and
introduces constant "md_valid" input parameter as a branch condition.
The new branch will be removed by the compiler at compile time. Two
instances of miniflow_extract with different branches will be generated.

This patch is tested on an arm64 platform. It improves more than 3.5%
performance in P2P forwarding cases.

Change-Id: I5d606afb52bfa68e8afa6f886d69b9665cdad51a
Reviewed-by: Gavin Hu <Gavin.Hu@arm.com>
Signed-off-by: Yanqin Wei <Yanqin.Wei@arm.com>
---
 lib/dpif-netdev.c |  13 +++---
 lib/flow.c        | 116 ++++++++++++++++++++++++++++++++----------------------
 lib/flow.h        |   2 +
 3 files changed, 79 insertions(+), 52 deletions(-)

Comments

0-day Robot Aug. 22, 2019, 9:59 a.m. UTC | #1
Bleep bloop.  Greetings Yanqin Wei, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
ERROR: Remove Gerrit Change-Id's before submitting upstream.
16: Change-Id: I5d606afb52bfa68e8afa6f886d69b9665cdad51a

Lines checked: 232, Warnings: 0, Errors: 1


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot

Patch
diff mbox series

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index d0a1c58..6686b14 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -6508,12 +6508,15 @@  dfc_processing(struct dp_netdev_pmd_thread *pmd,
             }
         }
 
-        miniflow_extract(packet, &key->mf);
+        if (!md_is_valid) {
+            miniflow_extract_firstpass(packet, &key->mf);
+            key->hash =
+                dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf);
+        } else {
+            miniflow_extract(packet, &key->mf);
+            key->hash = dpif_netdev_packet_get_rss_hash(packet, &key->mf);
+        }
         key->len = 0; /* Not computed yet. */
-        key->hash =
-                (md_is_valid == false)
-                ? dpif_netdev_packet_get_rss_hash_orig_pkt(packet, &key->mf)
-                : dpif_netdev_packet_get_rss_hash(packet, &key->mf);
 
         /* If EMC is disabled skip emc_lookup */
         flow = (cur_min != 0) ? emc_lookup(&cache->emc_cache, key) : NULL;
diff --git a/lib/flow.c b/lib/flow.c
index e54fd2e..e5b554b 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -707,7 +707,8 @@  ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
 }
 
 /* Initializes 'dst' from 'packet' and 'md', taking the packet type into
- * account.  'dst' must have enough space for FLOW_U64S * 8 bytes.
+ * account.  'dst' must have enough space for FLOW_U64S * 8 bytes. Metadata
+ * initialization should be bypassed if "md_valid" is false.
  *
  * Initializes the layer offsets as follows:
  *
@@ -732,8 +733,9 @@  ipv6_sanity_check(const struct ovs_16aligned_ip6_hdr *nh, size_t size)
  *      present and the packet has at least the content used for the fields
  *      of interest for the flow, otherwise UINT16_MAX.
  */
-void
-miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
+static inline ALWAYS_INLINE void
+miniflow_extract__(struct dp_packet *packet, struct miniflow *dst,
+                    const bool md_valid)
 {
     /* Add code to this function (or its callees) to extract new fields. */
     BUILD_ASSERT_DECL(FLOW_WC_SEQ == 41);
@@ -752,54 +754,60 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
     ovs_be16 ct_tp_src = 0, ct_tp_dst = 0;
 
     /* Metadata. */
-    if (flow_tnl_dst_is_set(&md->tunnel)) {
-        miniflow_push_words(mf, tunnel, &md->tunnel,
-                            offsetof(struct flow_tnl, metadata) /
-                            sizeof(uint64_t));
-
-        if (!(md->tunnel.flags & FLOW_TNL_F_UDPIF)) {
-            if (md->tunnel.metadata.present.map) {
-                miniflow_push_words(mf, tunnel.metadata, &md->tunnel.metadata,
-                                    sizeof md->tunnel.metadata /
-                                    sizeof(uint64_t));
-            }
-        } else {
-            if (md->tunnel.metadata.present.len) {
-                miniflow_push_words(mf, tunnel.metadata.present,
-                                    &md->tunnel.metadata.present, 1);
-                miniflow_push_words(mf, tunnel.metadata.opts.gnv,
-                                    md->tunnel.metadata.opts.gnv,
-                                    DIV_ROUND_UP(md->tunnel.metadata.present.len,
-                                                 sizeof(uint64_t)));
+    if (md_valid) {
+        if (flow_tnl_dst_is_set(&md->tunnel)) {
+            miniflow_push_words(mf, tunnel, &md->tunnel,
+                                offsetof(struct flow_tnl, metadata) /
+                                sizeof(uint64_t));
+
+            if (!(md->tunnel.flags & FLOW_TNL_F_UDPIF)) {
+                if (md->tunnel.metadata.present.map) {
+                    miniflow_push_words(mf, tunnel.metadata,
+                                        &md->tunnel.metadata,
+                                        sizeof md->tunnel.metadata /
+                                        sizeof(uint64_t));
+                }
+            } else {
+                if (md->tunnel.metadata.present.len) {
+                    miniflow_push_words(mf, tunnel.metadata.present,
+                                        &md->tunnel.metadata.present, 1);
+                    miniflow_push_words(mf, tunnel.metadata.opts.gnv,
+                                        md->tunnel.metadata.opts.gnv,
+                                        DIV_ROUND_UP(
+                                               md->tunnel.metadata.present.len,
+                                               sizeof(uint64_t)));
+                }
             }
         }
-    }
-    if (md->skb_priority || md->pkt_mark) {
-        miniflow_push_uint32(mf, skb_priority, md->skb_priority);
-        miniflow_push_uint32(mf, pkt_mark, md->pkt_mark);
-    }
-    miniflow_push_uint32(mf, dp_hash, md->dp_hash);
-    miniflow_push_uint32(mf, in_port, odp_to_u32(md->in_port.odp_port));
-    if (md->ct_state) {
-        miniflow_push_uint32(mf, recirc_id, md->recirc_id);
-        miniflow_push_uint8(mf, ct_state, md->ct_state);
-        ct_nw_proto_p = miniflow_pointer(mf, ct_nw_proto);
-        miniflow_push_uint8(mf, ct_nw_proto, 0);
-        miniflow_push_uint16(mf, ct_zone, md->ct_zone);
-    } else if (md->recirc_id) {
-        miniflow_push_uint32(mf, recirc_id, md->recirc_id);
-        miniflow_pad_to_64(mf, recirc_id);
-    }
-
-    if (md->ct_state) {
-        miniflow_push_uint32(mf, ct_mark, md->ct_mark);
-        miniflow_push_be32(mf, packet_type, packet_type);
-
-        if (!ovs_u128_is_zero(md->ct_label)) {
-            miniflow_push_words(mf, ct_label, &md->ct_label,
-                                sizeof md->ct_label / sizeof(uint64_t));
+        if (md->skb_priority || md->pkt_mark) {
+            miniflow_push_uint32(mf, skb_priority, md->skb_priority);
+            miniflow_push_uint32(mf, pkt_mark, md->pkt_mark);
+        }
+        miniflow_push_uint32(mf, dp_hash, md->dp_hash);
+        miniflow_push_uint32(mf, in_port, odp_to_u32(md->in_port.odp_port));
+        if (md->ct_state) {
+            miniflow_push_uint32(mf, recirc_id, md->recirc_id);
+            miniflow_push_uint8(mf, ct_state, md->ct_state);
+            ct_nw_proto_p = miniflow_pointer(mf, ct_nw_proto);
+            miniflow_push_uint8(mf, ct_nw_proto, 0);
+            miniflow_push_uint16(mf, ct_zone, md->ct_zone);
+        } else if (md->recirc_id) {
+            miniflow_push_uint32(mf, recirc_id, md->recirc_id);
+            miniflow_pad_to_64(mf, recirc_id);
+        }
+
+        if (md->ct_state) {
+            miniflow_push_uint32(mf, ct_mark, md->ct_mark);
+            miniflow_push_be32(mf, packet_type, packet_type);
+
+            if (!ovs_u128_is_zero(md->ct_label)) {
+                miniflow_push_words(mf, ct_label, &md->ct_label,
+                                    sizeof md->ct_label / sizeof(uint64_t));
+            }
         }
     } else {
+        miniflow_push_uint32(mf, dp_hash, md->dp_hash);
+        miniflow_push_uint32(mf, in_port, odp_to_u32(md->in_port.odp_port));
         miniflow_pad_from_64(mf, packet_type);
         miniflow_push_be32(mf, packet_type, packet_type);
     }
@@ -865,6 +873,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
 
         /* Push both source and destination address at once. */
         miniflow_push_words(mf, nw_src, &nh->ip_src, 1);
+
         if (ct_nw_proto_p && !md->ct_orig_tuple_ipv6) {
             *ct_nw_proto_p = md->ct_orig_tuple.ipv4.ipv4_proto;
             if (*ct_nw_proto_p) {
@@ -900,6 +909,7 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
                             sizeof nh->ip6_src / 8);
         miniflow_push_words(mf, ipv6_dst, &nh->ip6_dst,
                             sizeof nh->ip6_dst / 8);
+
         if (ct_nw_proto_p && md->ct_orig_tuple_ipv6) {
             *ct_nw_proto_p = md->ct_orig_tuple.ipv6.ipv6_proto;
             if (*ct_nw_proto_p) {
@@ -1076,6 +1086,18 @@  miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
     dst->map = mf.map;
 }
 
+void
+miniflow_extract(struct dp_packet *packet, struct miniflow *dst)
+{
+    miniflow_extract__(packet, dst, true);
+}
+
+void
+miniflow_extract_firstpass(struct dp_packet *packet, struct miniflow *dst)
+{
+    miniflow_extract__(packet, dst, false);
+}
+
 ovs_be16
 parse_dl_type(const struct eth_header *data_, size_t size)
 {
diff --git a/lib/flow.h b/lib/flow.h
index 7298c71..2d38574 100644
--- a/lib/flow.h
+++ b/lib/flow.h
@@ -541,6 +541,8 @@  struct pkt_metadata;
  * 'dst->map' is ignored on input and set on output to indicate which fields
  * were extracted. */
 void miniflow_extract(struct dp_packet *packet, struct miniflow *dst);
+void miniflow_extract_firstpass(struct dp_packet *packet,
+                                struct miniflow *dst);
 void miniflow_map_init(struct miniflow *, const struct flow *);
 void flow_wc_map(const struct flow *, struct flowmap *);
 size_t miniflow_alloc(struct miniflow *dsts[], size_t n,