[ovs-dev,v10,2/5] dpif-netdev: move dpcls lookup structures to .h
diff mbox series

Message ID 20190709123440.45519-3-harry.van.haaren@intel.com
State Superseded
Delegated to: Ian Stokes
Headers show
Series
  • dpcls func ptrs & optimizations
Related show

Commit Message

Harry van Haaren July 9, 2019, 12:34 p.m. UTC
This commit moves some data-structures to be available
in the dpif-netdev.h header. This allows specific implementations
of the subtable lookup function to include just that header
file, and not require that the code exists in dpif-netdev.c

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
Tested-by: Malvika Gupta <malvika.gupta@arm.com>

---

v10:
- Rebase updates from previous patch in code that moved.
- Move cmap.h include into alphabetical order (Ian)
- Fix comment and typo (Ian)
- Restructure function typedef to fit in 80 chars

v6:
- Fix double * in code comments (Eelco)
- Reword comment on lookup_func for clarity (Harry)
- Rebase fixups
---
 lib/dpif-netdev.c | 51 -------------------------------------------
 lib/dpif-netdev.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 55 insertions(+), 51 deletions(-)

Comments

Stokes, Ian July 10, 2019, 3:51 p.m. UTC | #1
On 7/9/2019 1:34 PM, Harry van Haaren wrote:
> This commit moves some data-structures to be available
> in the dpif-netdev.h header. This allows specific implementations
> of the subtable lookup function to include just that header
> file, and not require that the code exists in dpif-netdev.c
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> Tested-by: Malvika Gupta <malvika.gupta@arm.com>
> 
> ---
> 
> v10:
> - Rebase updates from previous patch in code that moved.
> - Move cmap.h include into alphabetical order (Ian)
> - Fix comment and typo (Ian)
> - Restructure function typedef to fit in 80 chars

Thanks for addressing the issues raised in v9,

Although previously I said this could be combined with patch 1 and 3,now 
I'm thinking it might be better to leave them split as is. Logically 
they accomplish separate goals in each patch.

Seeing the division of the patches in this series was definitely helpful 
to myself during reviews so I'm thinking it may be helpful to others in 
the future if they are moving from the current dpcls approach to this 
generic approach. Unless there are objections I suggest we leave as is.

Ian

Patch
diff mbox series

diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index f02bcd552..2414103d8 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -129,15 +129,6 @@  static struct odp_support dp_netdev_support = {
     .ct_orig_tuple6 = true,
 };
 
-/* Stores a miniflow with inline values */
-
-struct netdev_flow_key {
-    uint32_t hash;       /* Hash function differs for different users. */
-    uint32_t len;        /* Length of the following miniflow (incl. map). */
-    struct miniflow mf;
-    uint64_t buf[FLOW_MAX_PACKET_U64S];
-};
-
 /* EMC cache and SMC cache compose the datapath flow cache (DFC)
  *
  * Exact match cache for frequently used flows
@@ -7603,48 +7594,6 @@  dpif_dummy_register(enum dummy_level level)
 
 /* Datapath Classifier. */
 
-/* Forward declaration for lookup_func typedef. */
-struct dpcls_subtable;
-
-/* Lookup function for a subtable in the dpcls. This function is called
- * by each subtable with an array of packets, and a bitmask of packets to
- * perform the lookup on. Using a function pointer gives flexibility to
- * optimize the lookup function based on subtable properties and the
- * CPU instruction set available at runtime.
- */
-typedef
-uint32_t (*dpcls_subtable_lookup_func)(struct dpcls_subtable *subtable,
-                                       uint32_t keys_map,
-                                       const struct netdev_flow_key *keys[],
-                                       struct dpcls_rule **rules);
-
-/* Prototype for generic lookup func, using same code path as before. */
-uint32_t
-dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
-                              uint32_t keys_map,
-                              const struct netdev_flow_key *keys[],
-                              struct dpcls_rule **rules);
-
-/* A set of rules that all have the same fields wildcarded. */
-struct dpcls_subtable {
-    /* The fields are only used by writers. */
-    struct cmap_node cmap_node OVS_GUARDED; /* Within dpcls 'subtables_map'. */
-
-    /* These fields are accessed by readers. */
-    struct cmap rules;           /* Contains "struct dpcls_rule"s. */
-    uint32_t hit_cnt;            /* Number of match hits in subtable in current
-                                    optimization interval. */
-
-    /* The lookup function to use for this subtable. If there is a known
-     * property of the subtable (eg: only 3 bits of miniflow metadata is
-     * used for the lookup) then this can point at an optimized version of
-     * the lookup function for this particular subtable. */
-    dpcls_subtable_lookup_func lookup_func;
-
-    struct netdev_flow_key mask; /* Wildcards for fields (const). */
-    /* 'mask' must be the last field, additional space is allocated here. */
-};
-
 static void
 dpcls_subtable_destroy_cb(struct dpcls_subtable *subtable)
 {
diff --git a/lib/dpif-netdev.h b/lib/dpif-netdev.h
index 6db6ed2e2..3d23743bd 100644
--- a/lib/dpif-netdev.h
+++ b/lib/dpif-netdev.h
@@ -20,6 +20,8 @@ 
 #include <stdbool.h>
 #include <stddef.h>
 #include <stdint.h>
+
+#include "cmap.h"
 #include "dpif.h"
 #include "openvswitch/types.h"
 #include "dp-packet.h"
@@ -38,6 +40,59 @@  bool dpif_is_netdev(const struct dpif *);
 #define NR_QUEUE   1
 #define NR_PMD_THREADS 1
 
+/* Forward declaration for lookup_func typedef. */
+struct dpcls_subtable;
+struct dpcls_rule;
+
+/* Must be public as it is instantiated in subtable struct below. */
+struct netdev_flow_key {
+    uint32_t hash;       /* Hash function differs for different users. */
+    uint32_t len;        /* Length of the following miniflow (incl. map). */
+    struct miniflow mf;
+    uint64_t buf[FLOW_MAX_PACKET_U64S];
+};
+
+/* Lookup function for a subtable in the dpcls. This function is called
+ * by each subtable with an array of packets, and a bitmask of packets to
+ * perform the lookup on. Using a function pointer gives flexibility to
+ * optimize the lookup function based on subtable properties and the
+ * CPU instruction set available at runtime.
+ */
+typedef
+uint32_t (*dpcls_subtable_lookup_func)(struct dpcls_subtable *subtable,
+                                       uint32_t keys_map,
+                                       const struct netdev_flow_key *keys[],
+                                       struct dpcls_rule **rules);
+
+/* Prototype for generic lookup func, using same code path as before. */
+uint32_t
+dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
+                              uint32_t keys_map,
+                              const struct netdev_flow_key *keys[],
+                              struct dpcls_rule **rules);
+
+/* A set of rules that all have the same fields wildcarded. */
+struct dpcls_subtable {
+    /* The fields are only used by writers. */
+    struct cmap_node cmap_node OVS_GUARDED; /* Within dpcls 'subtables_map'. */
+
+    /* These fields are accessed by readers. */
+    struct cmap rules;           /* Contains "struct dpcls_rule"s. */
+    uint32_t hit_cnt;            /* Number of match hits in subtable in current
+                                    optimization interval. */
+
+    /* The lookup function to use for this subtable. From a technical point of
+     * view, this is just an implementation of mutliple dispatch. The attribute
+     * used to identify the ideal function is the miniflow fingerprint that the
+     * subtable matches on. The miniflow "bits" are used to select the actual
+     * dpcls lookup implementation at subtable creation time.
+     */
+    dpcls_subtable_lookup_func lookup_func;
+
+    struct netdev_flow_key mask; /* Wildcards for fields (const). */
+    /* 'mask' must be the last field, additional space is allocated here. */
+};
+
 #ifdef  __cplusplus
 }
 #endif