diff mbox series

[ovs-dev,v5,3/3] dpif-netdev: split out generic lookup function

Message ID 20190225161815.59733-4-harry.van.haaren@intel.com
State Superseded
Headers show
Series dpcls subtable func pointers | expand

Commit Message

Van Haaren, Harry Feb. 25, 2019, 4:18 p.m. UTC 
This commit splits the generic hash-lookup-verify
function to its own file. In doing so, we must move
some MACRO definitions to dpif-netdev.h

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/automake.mk                  |  1 +
 lib/dpif-netdev-lookup-generic.c | 95 ++++++++++++++++++++++++++++++++
 lib/dpif-netdev.c                | 81 +--------------------------
 lib/dpif-netdev.h                | 16 ++++++
 4 files changed, 113 insertions(+), 80 deletions(-)
 create mode 100644 lib/dpif-netdev-lookup-generic.c

Comments

0-day Robot Feb. 25, 2019, 5:06 p.m. UTC | #1 
Bleep bloop.  Greetings Harry van Haaren, 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: Improper whitespace around control block
#77 FILE: lib/dpif-netdev-lookup-generic.c:41:
    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {

ERROR: C99 style comment
#119 FILE: lib/dpif-netdev-lookup-generic.c:83:
                    //lookups_match += subtable_pos;

ERROR: Improper whitespace around control block
#276 FILE: lib/dpif-netdev.h:100:
#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \

ERROR: Improper whitespace around control block
#277 FILE: lib/dpif-netdev.h:101:
    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)

Lines checked: 289, Warnings: 0, Errors: 4


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

Thanks,
0-day Robot
Eelco Chaudron March 22, 2019, 12:28 p.m. UTC | #2 
On 25 Feb 2019, at 17:18, Harry van Haaren wrote:

> This commit splits the generic hash-lookup-verify
> function to its own file. In doing so, we must move
> some MACRO definitions to dpif-netdev.h

Changes look good, but want to wait for some performance numbers before 
ack’ing the series.

Did you do any performance tests on the change? It does not seem to 
impact much, but I’ve been surprised before.

> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/automake.mk                  |  1 +
>  lib/dpif-netdev-lookup-generic.c | 95 
> ++++++++++++++++++++++++++++++++
>  lib/dpif-netdev.c                | 81 +--------------------------
>  lib/dpif-netdev.h                | 16 ++++++
>  4 files changed, 113 insertions(+), 80 deletions(-)
>  create mode 100644 lib/dpif-netdev-lookup-generic.c
>
> diff --git a/lib/automake.mk b/lib/automake.mk
> index bae032bd8..3a5baf2b8 100644
> --- a/lib/automake.mk
> +++ b/lib/automake.mk
> @@ -78,6 +78,7 @@ lib_libopenvswitch_la_SOURCES = \
>  	lib/dp-packet.h \
>  	lib/dp-packet.c \
>  	lib/dpdk.h \
> +	lib/dpif-netdev-lookup-generic.c \
>  	lib/dpif-netdev.c \
>  	lib/dpif-netdev.h \
>  	lib/dpif-netdev-perf.c \
> diff --git a/lib/dpif-netdev-lookup-generic.c 
> b/lib/dpif-netdev-lookup-generic.c
> new file mode 100644
> index 000000000..8ae8ff59d
> --- /dev/null
> +++ b/lib/dpif-netdev-lookup-generic.c
> @@ -0,0 +1,95 @@
> +/*
> + * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016, 2017 
> Nicira, Inc.
> + *
> + * Licensed under the Apache License, Version 2.0 (the "License");
> + * you may not use this file except in compliance with the License.
> + * You may obtain a copy of the License at:
> + *
> + *     http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, 
> software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or 
> implied.
> + * See the License for the specific language governing permissions 
> and
> + * limitations under the License.
> + */
> +
> +#include <config.h>
> +#include "dpif-netdev.h"
> +
> +#include "bitmap.h"
> +#include "cmap.h"
> +
> +#include "dp-packet.h"
> +#include "dpif.h"
> +#include "dpif-netdev-perf.h"
> +#include "dpif-provider.h"
> +#include "flow.h"
> +#include "packets.h"
> +#include "pvector.h"
> +
> +/* Returns a hash value for the bits of 'key' where there are 1-bits 
> in
> + * 'mask'. */
> +static inline uint32_t
> +netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
> +                             const struct netdev_flow_key *mask)
> +{
> +    const uint64_t *p = miniflow_get_values(&mask->mf);
> +    uint32_t hash = 0;
> +    uint64_t value;
> +
> +    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
> +        hash = hash_add64(hash, value & *p);
> +        p++;
> +    }
> +
> +    return hash_finish(hash, (p - miniflow_get_values(&mask->mf)) * 
> 8);
> +}
> +
> +uint32_t
> +dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
> +                              uint32_t keys_map,
> +                              const struct netdev_flow_key *keys[],
> +                              struct dpcls_rule **rules)
> +{
> +        int i;
> +        /* Compute hashes for the remaining keys.  Each search-key is
> +         * masked with the subtable's mask to avoid hashing the 
> wildcarded
> +         * bits. */
> +        uint32_t hashes[NETDEV_MAX_BURST];
> +        ULLONG_FOR_EACH_1(i, keys_map) {
> +            hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
> +                                                     
> &subtable->mask);
> +        }
> +
> +        /* Lookup. */
> +        const struct cmap_node *nodes[NETDEV_MAX_BURST];
> +        uint32_t found_map =
> +                cmap_find_batch(&subtable->rules, keys_map, hashes, 
> nodes);
> +        /* Check results.  When the i-th bit of found_map is set, it 
> means
> +         * that a set of nodes with a matching hash value was found 
> for the
> +         * i-th search-key.  Due to possible hash collisions we need 
> to check
> +         * which of the found rules, if any, really matches our 
> masked
> +         * search-key. */
> +        ULLONG_FOR_EACH_1(i, found_map) {
> +            struct dpcls_rule *rule;
> +
> +            CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
> +                if (OVS_LIKELY(dpcls_rule_matches_key(rule, 
> keys[i]))) {
> +                    rules[i] = rule;
> +                    /* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
> +                     * within one second optimization interval. */
> +                    subtable->hit_cnt++;
> +                    //lookups_match += subtable_pos;
> +                    goto next;
> +                }
> +            }
> +            /* None of the found rules was a match.  Reset the i-th 
> bit to
> +             * keep searching this key in the next subtable. */
> +            ULLONG_SET0(found_map, i);  /* Did not match. */
> +        next:
> +            ;                     /* Keep Sparse happy. */
> +        }
> +
> +        return found_map;
> +}
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 143f1c288..bc0339a25 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -234,14 +234,6 @@ struct dpcls {
>      struct pvector subtables;
>  };
>
> -/* A rule to be inserted to the classifier. */
> -struct dpcls_rule {
> -    struct cmap_node cmap_node;   /* Within struct dpcls_subtable 
> 'rules'. */
> -    struct netdev_flow_key *mask; /* Subtable's mask. */
> -    struct netdev_flow_key flow;  /* Matching key. */
> -    /* 'flow' must be the last field, additional space is allocated 
> here. */
> -};
> -
>  /* Data structure to keep packet order till fastpath processing. */
>  struct dp_packet_flow_map {
>      struct dp_packet *packet;
> @@ -259,8 +251,6 @@ static bool dpcls_lookup(struct dpcls *cls,
>                           const struct netdev_flow_key *keys[],
>                           struct dpcls_rule **rules, size_t cnt,
>                           int *num_lookups_p);
> -static bool dpcls_rule_matches_key(const struct dpcls_rule *rule,
> -                            const struct netdev_flow_key *target);
>  /* Set of supported meter flags */
>  #define DP_SUPPORTED_METER_FLAGS_MASK \
>      (OFPMF13_STATS | OFPMF13_PKTPS | OFPMF13_KBPS | OFPMF13_BURST)
> @@ -2734,27 +2724,6 @@ netdev_flow_key_init_masked(struct 
> netdev_flow_key *dst,
>                              (dst_u64 - miniflow_get_values(&dst->mf)) 
> * 8);
>  }
>
> -/* Iterate through netdev_flow_key TNL u64 values specified by 
> 'FLOWMAP'. */
> -#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \
> -    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)
> -
> -/* Returns a hash value for the bits of 'key' where there are 1-bits 
> in
> - * 'mask'. */
> -static inline uint32_t
> -netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
> -                             const struct netdev_flow_key *mask)
> -{
> -    const uint64_t *p = miniflow_get_values(&mask->mf);
> -    uint32_t hash = 0;
> -    uint64_t value;
> -
> -    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
> -        hash = hash_add64(hash, value & *p++);
> -    }
> -
> -    return hash_finish(hash, (p - miniflow_get_values(&mask->mf)) * 
> 8);
> -}
> -
>  static inline bool
>  emc_entry_alive(struct emc_entry *ce)
>  {
> @@ -7767,7 +7736,7 @@ dpcls_remove(struct dpcls *cls, struct 
> dpcls_rule *rule)
>
>  /* Returns true if 'target' satisfies 'key' in 'mask', that is, if 
> each 1-bit
>   * in 'mask' the values in 'key' and 'target' are the same. */
> -static bool
> +bool
>  dpcls_rule_matches_key(const struct dpcls_rule *rule,
>                         const struct netdev_flow_key *target)
>  {
> @@ -7783,54 +7752,6 @@ dpcls_rule_matches_key(const struct dpcls_rule 
> *rule,
>      return true;
>  }
>
> -uint32_t
> -dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
> -                              uint32_t keys_map,
> -                              const struct netdev_flow_key *keys[],
> -                              struct dpcls_rule **rules)
> -{
> -        int i;
> -        /* Compute hashes for the remaining keys.  Each search-key is
> -         * masked with the subtable's mask to avoid hashing the 
> wildcarded
> -         * bits. */
> -        uint32_t hashes[NETDEV_MAX_BURST];
> -        ULLONG_FOR_EACH_1(i, keys_map) {
> -            hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
> -                                                     
> &subtable->mask);
> -        }
> -
> -        /* Lookup. */
> -        const struct cmap_node *nodes[NETDEV_MAX_BURST];
> -        uint32_t found_map =
> -                cmap_find_batch(&subtable->rules, keys_map, hashes, 
> nodes);
> -        /* Check results.  When the i-th bit of found_map is set, it 
> means
> -         * that a set of nodes with a matching hash value was found 
> for the
> -         * i-th search-key.  Due to possible hash collisions we need 
> to check
> -         * which of the found rules, if any, really matches our 
> masked
> -         * search-key. */
> -        ULLONG_FOR_EACH_1(i, found_map) {
> -            struct dpcls_rule *rule;
> -
> -            CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
> -                if (OVS_LIKELY(dpcls_rule_matches_key(rule, 
> keys[i]))) {
> -                    rules[i] = rule;
> -                    /* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
> -                     * within one second optimization interval. */
> -                    subtable->hit_cnt++;
> -                    //lookups_match += subtable_pos;
> -                    goto next;
> -                }
> -            }
> -            /* None of the found rules was a match.  Reset the i-th 
> bit to
> -             * keep searching this key in the next subtable. */
> -            ULLONG_SET0(found_map, i);  /* Did not match. */
> -        next:
> -            ;                     /* Keep Sparse happy. */
> -        }
> -
> -        return found_map;
> -}
> -
>  /* For each miniflow in 'keys' performs a classifier lookup writing 
> the result
>   * into the corresponding slot in 'rules'.  If a particular entry in 
> 'keys' is
>   * NULL it is skipped.
> diff --git a/lib/dpif-netdev.h b/lib/dpif-netdev.h
> index 720425fb3..7195b9791 100644
> --- a/lib/dpif-netdev.h
> +++ b/lib/dpif-netdev.h
> @@ -51,6 +51,14 @@ struct netdev_flow_key {
>      uint64_t buf[FLOW_MAX_PACKET_U64S];
>  };
>
> +/* A rule to be inserted to the classifier. */
> +struct dpcls_rule {
> +    struct cmap_node cmap_node;   /* Within struct dpcls_subtable 
> 'rules'. */
> +    struct netdev_flow_key *mask; /* Subtable's mask. */
> +    struct netdev_flow_key flow;  /* Matching key. */
> +    /* 'flow' must be the last field, additional space is allocated 
> here. */
> +};
> +
>  /** 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
> @@ -88,6 +96,14 @@ struct dpcls_subtable {
>      /* 'mask' must be the last field, additional space is allocated 
> here. */
>  };
>
> +/* Iterate through netdev_flow_key TNL u64 values specified by 
> 'FLOWMAP'. */
> +#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \
> +    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)
> +
> +bool dpcls_rule_matches_key(const struct dpcls_rule *rule,
> +                            const struct netdev_flow_key *target);
> +
> +
>  #ifdef  __cplusplus
>  }
>  #endif
> -- 
> 2.17.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Van Haaren, Harry March 22, 2019, 1:08 p.m. UTC | #3 
Hey Eelco,

> -----Original Message-----
> From: Eelco Chaudron [mailto:echaudro@redhat.com]
> Sent: Friday, March 22, 2019 12:29 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: ovs-dev@openvswitch.org; i.maximets@samsung.com
> Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup
> function
> 
> On 25 Feb 2019, at 17:18, Harry van Haaren wrote:
> 
> > This commit splits the generic hash-lookup-verify
> > function to its own file. In doing so, we must move
> > some MACRO definitions to dpif-netdev.h
> 
> Changes look good, but want to wait for some performance numbers before
> ack’ing the series.
> 
> Did you do any performance tests on the change? It does not seem to
> impact much, but I’ve been surprised before.

Thanks for the review - I'll take the comments into account for the V+1.

Regarding performance, I expect there to be no performance delta with
this patchset - it is just enabling flexibility in the dpcls infrastructure.

As presented at OvS conf last November, with the flexibility mentioned above
we can select optimized implementations at runtime, and that is where the
performance gains will be seen.


I'll work on addressing your comments early next week - thanks again! -H
Ilya Maximets March 22, 2019, 1:18 p.m. UTC | #4 
On 22.03.2019 16:08, Van Haaren, Harry wrote:
> Hey Eelco,
> 
>> -----Original Message-----
>> From: Eelco Chaudron [mailto:echaudro@redhat.com]
>> Sent: Friday, March 22, 2019 12:29 PM
>> To: Van Haaren, Harry <harry.van.haaren@intel.com>
>> Cc: ovs-dev@openvswitch.org; i.maximets@samsung.com
>> Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup
>> function
>>
>> On 25 Feb 2019, at 17:18, Harry van Haaren wrote:
>>
>>> This commit splits the generic hash-lookup-verify
>>> function to its own file. In doing so, we must move
>>> some MACRO definitions to dpif-netdev.h
>>
>> Changes look good, but want to wait for some performance numbers before
>> ack’ing the series.
>>
>> Did you do any performance tests on the change? It does not seem to
>> impact much, but I’ve been surprised before.
> 
> Thanks for the review - I'll take the comments into account for the V+1.
> 
> Regarding performance, I expect there to be no performance delta with
> this patchset - it is just enabling flexibility in the dpcls infrastructure.
> 
> As presented at OvS conf last November, with the flexibility mentioned above
> we can select optimized implementations at runtime, and that is where the
> performance gains will be seen.
> 
> 
> I'll work on addressing your comments early next week - thanks again! -H

Hi Harry,

I didn't look at this series closely yet. But if you're going to prepare new
version, it'll be good to fix checkpatch issues reported by 0-day robot too:

  https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356615.html
  https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356616.html

BTW, commented out counters looks suspicious.

Best regards, Ilya Maximets.
Van Haaren, Harry March 25, 2019, 4:30 p.m. UTC | #5 
> -----Original Message-----
> From: Ilya Maximets [mailto:i.maximets@samsung.com]
> Sent: Friday, March 22, 2019 1:19 PM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eelco Chaudron
> <echaudro@redhat.com>
> Cc: ovs-dev@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic lookup
> function
> 
> On 22.03.2019 16:08, Van Haaren, Harry wrote:
> > Hey Eelco,
> >
> >> -----Original Message-----

<snip>

> > I'll work on addressing your comments early next week - thanks again! -H
> 
> Hi Harry,
> 
> I didn't look at this series closely yet. But if you're going to prepare new
> version, it'll be good to fix checkpatch issues reported by 0-day robot too:
> 
>   https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356615.html
>   https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356616.html
> 
> BTW, commented out counters looks suspicious.
> 
> Best regards, Ilya Maximets.

Thanks Ilya, fixing checkpatch issues now, no problems.

Correct the lookups_match counter isn't implemented yet - will add it in v6 implementation.

The name of the variable is a bit misleading - it really counts the number of subtables searched for a match, per packet that it found a hit for.

Eg: a packet that matched the 3rd subtable searched, will += counter by 3.

So really, it counts "subtable-effort per packet match". Does this align with you understanding of it?

Thanks, -Harry
Eelco Chaudron March 26, 2019, 10:38 a.m. UTC | #6 
On 25 Mar 2019, at 17:30, Van Haaren, Harry wrote:

>> -----Original Message-----
>> From: Ilya Maximets [mailto:i.maximets@samsung.com]
>> Sent: Friday, March 22, 2019 1:19 PM
>> To: Van Haaren, Harry <harry.van.haaren@intel.com>; Eelco Chaudron
>> <echaudro@redhat.com>
>> Cc: ovs-dev@openvswitch.org
>> Subject: Re: [ovs-dev] [PATCH v5 3/3] dpif-netdev: split out generic 
>> lookup
>> function
>>
>> On 22.03.2019 16:08, Van Haaren, Harry wrote:
>>> Hey Eelco,
>>>
>>>> -----Original Message-----
>
> <snip>
>
>>> I'll work on addressing your comments early next week - thanks 
>>> again! -H
>>
>> Hi Harry,
>>
>> I didn't look at this series closely yet. But if you're going to 
>> prepare new
>> version, it'll be good to fix checkpatch issues reported by 0-day 
>> robot too:
>>
>>   https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356615.html
>>   https://mail.openvswitch.org/pipermail/ovs-dev/2019-February/356616.html
>>
>> BTW, commented out counters looks suspicious.
>>
>> Best regards, Ilya Maximets.
>
> Thanks Ilya, fixing checkpatch issues now, no problems.
>
> Correct the lookups_match counter isn't implemented yet - will add it 
> in v6 implementation.
>
> The name of the variable is a bit misleading - it really counts the 
> number of subtables searched for a match, per packet that it found a 
> hit for.
>
> Eg: a packet that matched the 3rd subtable searched, will += counter 
> by 3.
>
> So really, it counts "subtable-effort per packet match". Does this 
> align with you understanding of it?
>
> Thanks, -Harry

Ran the PVP tests over the weekend and the results look good. The Zero 
loss ones have quite some deviation, so I would ignore them.
Here is a link to the results:

https://docs.google.com/spreadsheets/u/0/d/e/2PACX-1vSrHgy18vTWFWk_SCF4vggtr1qpimMUkhZuHCSQtmEQi77yCjPrxlAr4hvNvtuIO3Vzzsy31bls2_wP/pubhtml?skip_itp2_check=true#

Will continue the review on v6.
diff mbox series

Patch

diff --git a/lib/automake.mk b/lib/automake.mk
index bae032bd8..3a5baf2b8 100644
--- a/lib/automake.mk
+++ b/lib/automake.mk
@@ -78,6 +78,7 @@  lib_libopenvswitch_la_SOURCES = \
 	lib/dp-packet.h \
 	lib/dp-packet.c \
 	lib/dpdk.h \
+	lib/dpif-netdev-lookup-generic.c \
 	lib/dpif-netdev.c \
 	lib/dpif-netdev.h \
 	lib/dpif-netdev-perf.c \
diff --git a/lib/dpif-netdev-lookup-generic.c b/lib/dpif-netdev-lookup-generic.c
new file mode 100644
index 000000000..8ae8ff59d
--- /dev/null
+++ b/lib/dpif-netdev-lookup-generic.c
@@ -0,0 +1,95 @@ 
+/*
+ * Copyright (c) 2009, 2010, 2011, 2012, 2013, 2014, 2016, 2017 Nicira, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at:
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <config.h>
+#include "dpif-netdev.h"
+
+#include "bitmap.h"
+#include "cmap.h"
+
+#include "dp-packet.h"
+#include "dpif.h"
+#include "dpif-netdev-perf.h"
+#include "dpif-provider.h"
+#include "flow.h"
+#include "packets.h"
+#include "pvector.h"
+
+/* Returns a hash value for the bits of 'key' where there are 1-bits in
+ * 'mask'. */
+static inline uint32_t
+netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
+                             const struct netdev_flow_key *mask)
+{
+    const uint64_t *p = miniflow_get_values(&mask->mf);
+    uint32_t hash = 0;
+    uint64_t value;
+
+    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
+        hash = hash_add64(hash, value & *p);
+        p++;
+    }
+
+    return hash_finish(hash, (p - miniflow_get_values(&mask->mf)) * 8);
+}
+
+uint32_t
+dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
+                              uint32_t keys_map,
+                              const struct netdev_flow_key *keys[],
+                              struct dpcls_rule **rules)
+{
+        int i;
+        /* Compute hashes for the remaining keys.  Each search-key is
+         * masked with the subtable's mask to avoid hashing the wildcarded
+         * bits. */
+        uint32_t hashes[NETDEV_MAX_BURST];
+        ULLONG_FOR_EACH_1(i, keys_map) {
+            hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
+                                                     &subtable->mask);
+        }
+
+        /* Lookup. */
+        const struct cmap_node *nodes[NETDEV_MAX_BURST];
+        uint32_t found_map =
+                cmap_find_batch(&subtable->rules, keys_map, hashes, nodes);
+        /* Check results.  When the i-th bit of found_map is set, it means
+         * that a set of nodes with a matching hash value was found for the
+         * i-th search-key.  Due to possible hash collisions we need to check
+         * which of the found rules, if any, really matches our masked
+         * search-key. */
+        ULLONG_FOR_EACH_1(i, found_map) {
+            struct dpcls_rule *rule;
+
+            CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
+                if (OVS_LIKELY(dpcls_rule_matches_key(rule, keys[i]))) {
+                    rules[i] = rule;
+                    /* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
+                     * within one second optimization interval. */
+                    subtable->hit_cnt++;
+                    //lookups_match += subtable_pos;
+                    goto next;
+                }
+            }
+            /* None of the found rules was a match.  Reset the i-th bit to
+             * keep searching this key in the next subtable. */
+            ULLONG_SET0(found_map, i);  /* Did not match. */
+        next:
+            ;                     /* Keep Sparse happy. */
+        }
+
+        return found_map;
+}
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 143f1c288..bc0339a25 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -234,14 +234,6 @@  struct dpcls {
     struct pvector subtables;
 };
 
-/* A rule to be inserted to the classifier. */
-struct dpcls_rule {
-    struct cmap_node cmap_node;   /* Within struct dpcls_subtable 'rules'. */
-    struct netdev_flow_key *mask; /* Subtable's mask. */
-    struct netdev_flow_key flow;  /* Matching key. */
-    /* 'flow' must be the last field, additional space is allocated here. */
-};
-
 /* Data structure to keep packet order till fastpath processing. */
 struct dp_packet_flow_map {
     struct dp_packet *packet;
@@ -259,8 +251,6 @@  static bool dpcls_lookup(struct dpcls *cls,
                          const struct netdev_flow_key *keys[],
                          struct dpcls_rule **rules, size_t cnt,
                          int *num_lookups_p);
-static bool dpcls_rule_matches_key(const struct dpcls_rule *rule,
-                            const struct netdev_flow_key *target);
 /* Set of supported meter flags */
 #define DP_SUPPORTED_METER_FLAGS_MASK \
     (OFPMF13_STATS | OFPMF13_PKTPS | OFPMF13_KBPS | OFPMF13_BURST)
@@ -2734,27 +2724,6 @@  netdev_flow_key_init_masked(struct netdev_flow_key *dst,
                             (dst_u64 - miniflow_get_values(&dst->mf)) * 8);
 }
 
-/* Iterate through netdev_flow_key TNL u64 values specified by 'FLOWMAP'. */
-#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \
-    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)
-
-/* Returns a hash value for the bits of 'key' where there are 1-bits in
- * 'mask'. */
-static inline uint32_t
-netdev_flow_key_hash_in_mask(const struct netdev_flow_key *key,
-                             const struct netdev_flow_key *mask)
-{
-    const uint64_t *p = miniflow_get_values(&mask->mf);
-    uint32_t hash = 0;
-    uint64_t value;
-
-    NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(value, key, mask->mf.map) {
-        hash = hash_add64(hash, value & *p++);
-    }
-
-    return hash_finish(hash, (p - miniflow_get_values(&mask->mf)) * 8);
-}
-
 static inline bool
 emc_entry_alive(struct emc_entry *ce)
 {
@@ -7767,7 +7736,7 @@  dpcls_remove(struct dpcls *cls, struct dpcls_rule *rule)
 
 /* Returns true if 'target' satisfies 'key' in 'mask', that is, if each 1-bit
  * in 'mask' the values in 'key' and 'target' are the same. */
-static bool
+bool
 dpcls_rule_matches_key(const struct dpcls_rule *rule,
                        const struct netdev_flow_key *target)
 {
@@ -7783,54 +7752,6 @@  dpcls_rule_matches_key(const struct dpcls_rule *rule,
     return true;
 }
 
-uint32_t
-dpcls_subtable_lookup_generic(struct dpcls_subtable *subtable,
-                              uint32_t keys_map,
-                              const struct netdev_flow_key *keys[],
-                              struct dpcls_rule **rules)
-{
-        int i;
-        /* Compute hashes for the remaining keys.  Each search-key is
-         * masked with the subtable's mask to avoid hashing the wildcarded
-         * bits. */
-        uint32_t hashes[NETDEV_MAX_BURST];
-        ULLONG_FOR_EACH_1(i, keys_map) {
-            hashes[i] = netdev_flow_key_hash_in_mask(keys[i],
-                                                     &subtable->mask);
-        }
-
-        /* Lookup. */
-        const struct cmap_node *nodes[NETDEV_MAX_BURST];
-        uint32_t found_map =
-                cmap_find_batch(&subtable->rules, keys_map, hashes, nodes);
-        /* Check results.  When the i-th bit of found_map is set, it means
-         * that a set of nodes with a matching hash value was found for the
-         * i-th search-key.  Due to possible hash collisions we need to check
-         * which of the found rules, if any, really matches our masked
-         * search-key. */
-        ULLONG_FOR_EACH_1(i, found_map) {
-            struct dpcls_rule *rule;
-
-            CMAP_NODE_FOR_EACH (rule, cmap_node, nodes[i]) {
-                if (OVS_LIKELY(dpcls_rule_matches_key(rule, keys[i]))) {
-                    rules[i] = rule;
-                    /* Even at 20 Mpps the 32-bit hit_cnt cannot wrap
-                     * within one second optimization interval. */
-                    subtable->hit_cnt++;
-                    //lookups_match += subtable_pos;
-                    goto next;
-                }
-            }
-            /* None of the found rules was a match.  Reset the i-th bit to
-             * keep searching this key in the next subtable. */
-            ULLONG_SET0(found_map, i);  /* Did not match. */
-        next:
-            ;                     /* Keep Sparse happy. */
-        }
-
-        return found_map;
-}
-
 /* For each miniflow in 'keys' performs a classifier lookup writing the result
  * into the corresponding slot in 'rules'.  If a particular entry in 'keys' is
  * NULL it is skipped.
diff --git a/lib/dpif-netdev.h b/lib/dpif-netdev.h
index 720425fb3..7195b9791 100644
--- a/lib/dpif-netdev.h
+++ b/lib/dpif-netdev.h
@@ -51,6 +51,14 @@  struct netdev_flow_key {
     uint64_t buf[FLOW_MAX_PACKET_U64S];
 };
 
+/* A rule to be inserted to the classifier. */
+struct dpcls_rule {
+    struct cmap_node cmap_node;   /* Within struct dpcls_subtable 'rules'. */
+    struct netdev_flow_key *mask; /* Subtable's mask. */
+    struct netdev_flow_key flow;  /* Matching key. */
+    /* 'flow' must be the last field, additional space is allocated here. */
+};
+
 /** 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
@@ -88,6 +96,14 @@  struct dpcls_subtable {
     /* 'mask' must be the last field, additional space is allocated here. */
 };
 
+/* Iterate through netdev_flow_key TNL u64 values specified by 'FLOWMAP'. */
+#define NETDEV_FLOW_KEY_FOR_EACH_IN_FLOWMAP(VALUE, KEY, FLOWMAP)   \
+    MINIFLOW_FOR_EACH_IN_FLOWMAP(VALUE, &(KEY)->mf, FLOWMAP)
+
+bool dpcls_rule_matches_key(const struct dpcls_rule *rule,
+                            const struct netdev_flow_key *target);
+
+
 #ifdef  __cplusplus
 }
 #endif