diff mbox

[ovs-dev,patch_v7,2/9] dpdk: Remove batch sorting in userspace conntrack.

Message ID 1490346920-104476-3-git-send-email-dlu998@gmail.com
State Changes Requested
Delegated to: Daniele Di Proietto
Headers show

Commit Message

Darrell Ball March 24, 2017, 9:15 a.m. UTC 
Signed-off-by: Darrell Ball <dlu998@gmail.com>
Acked-by: Flavio Leitner <fbl@sysclose.org>
---
 lib/conntrack.c | 58 +++++++++++----------------------------------------------
 1 file changed, 11 insertions(+), 47 deletions(-)

Comments

Daniele Di Proietto April 30, 2017, 2 a.m. UTC | #1 
Thanks for doing this

Acked-by: Daniele Di Proietto <diproiettod@ovn.org>

2017-03-24 2:15 GMT-07:00 Darrell Ball <dlu998@gmail.com>:
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> Acked-by: Flavio Leitner <fbl@sysclose.org>
> ---
>  lib/conntrack.c | 58 +++++++++++----------------------------------------------
>  1 file changed, 11 insertions(+), 47 deletions(-)
>
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 4f490fb..9a0763e 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -318,22 +318,9 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
>  {
>      struct dp_packet **pkts = pkt_batch->packets;
>      size_t cnt = pkt_batch->count;
> -#if !defined(__CHECKER__) && !defined(_WIN32)
> -    const size_t KEY_ARRAY_SIZE = cnt;
> -#else
> -    enum { KEY_ARRAY_SIZE = NETDEV_MAX_BURST };
> -#endif
> -    struct conn_lookup_ctx ctxs[KEY_ARRAY_SIZE];
> -    int8_t bucket_list[CONNTRACK_BUCKETS];
> -    struct {
> -        unsigned bucket;
> -        unsigned long maps;
> -    } arr[KEY_ARRAY_SIZE];
> +    struct conn_lookup_ctx ctx;
>      long long now = time_msec();
>      size_t i = 0;
> -    uint8_t arrcnt = 0;
> -
> -    BUILD_ASSERT_DECL(sizeof arr[0].maps * CHAR_BIT >= NETDEV_MAX_BURST);
>
>      if (helper) {
>          static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> @@ -342,48 +329,25 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
>          /* Continue without the helper */
>      }
>
> -    memset(bucket_list, INT8_C(-1), sizeof bucket_list);
>      for (i = 0; i < cnt; i++) {
> -        unsigned bucket;
>
> -        if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
> +        if (!conn_key_extract(ct, pkts[i], dl_type, &ctx, zone)) {
>              write_ct_md(pkts[i], CS_INVALID, zone, NULL, NULL);
>              continue;
>          }
>
> -        bucket = hash_to_bucket(ctxs[i].hash);
> -        if (bucket_list[bucket] == INT8_C(-1)) {
> -            bucket_list[bucket] = arrcnt;
> -
> -            arr[arrcnt].maps = 0;
> -            ULLONG_SET1(arr[arrcnt].maps, i);
> -            arr[arrcnt++].bucket = bucket;
> -        } else {
> -            ULLONG_SET1(arr[bucket_list[bucket]].maps, i);
> -        }
> -    }
> -
> -    for (i = 0; i < arrcnt; i++) {
> -        struct conntrack_bucket *ctb = &ct->buckets[arr[i].bucket];
> -        size_t j;
> -
> +        struct conntrack_bucket *ctb = &ct->buckets[i];
>          ct_lock_lock(&ctb->lock);
> +        conn_key_lookup(ctb, &ctx, now);
> +        struct conn *conn = process_one(ct, pkts[i], &ctx, zone,
> +                                        force, commit, now);
>
> -        ULLONG_FOR_EACH_1(j, arr[i].maps) {
> -            struct conn *conn;
> -
> -            conn_key_lookup(ctb, &ctxs[j], now);
> -
> -            conn = process_one(ct, pkts[j], &ctxs[j], zone, force, commit,
> -                               now);
> -
> -            if (conn && setmark) {
> -                set_mark(pkts[j], conn, setmark[0], setmark[1]);
> -            }
> +        if (conn && setmark) {
> +            set_mark(pkts[i], conn, setmark[0], setmark[1]);
> +        }
>
> -            if (conn && setlabel) {
> -                set_label(pkts[j], conn, &setlabel[0], &setlabel[1]);
> -            }
> +        if (conn && setlabel) {
> +            set_label(pkts[i], conn, &setlabel[0], &setlabel[1]);
>          }
>          ct_lock_unlock(&ctb->lock);
>      }
> --
> 1.9.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Jarno Rajahalme May 1, 2017, 5:54 p.m. UTC | #2 
Would be nice to have a commit message, with the motivation for the change.

  Jarno

> On Mar 24, 2017, at 2:15 AM, Darrell Ball <dlu998@gmail.com> wrote:
> 
> Signed-off-by: Darrell Ball <dlu998@gmail.com>
> Acked-by: Flavio Leitner <fbl@sysclose.org>
> ---
> lib/conntrack.c | 58 +++++++++++----------------------------------------------
> 1 file changed, 11 insertions(+), 47 deletions(-)
> 
> diff --git a/lib/conntrack.c b/lib/conntrack.c
> index 4f490fb..9a0763e 100644
> --- a/lib/conntrack.c
> +++ b/lib/conntrack.c
> @@ -318,22 +318,9 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
> {
>     struct dp_packet **pkts = pkt_batch->packets;
>     size_t cnt = pkt_batch->count;
> -#if !defined(__CHECKER__) && !defined(_WIN32)
> -    const size_t KEY_ARRAY_SIZE = cnt;
> -#else
> -    enum { KEY_ARRAY_SIZE = NETDEV_MAX_BURST };
> -#endif
> -    struct conn_lookup_ctx ctxs[KEY_ARRAY_SIZE];
> -    int8_t bucket_list[CONNTRACK_BUCKETS];
> -    struct {
> -        unsigned bucket;
> -        unsigned long maps;
> -    } arr[KEY_ARRAY_SIZE];
> +    struct conn_lookup_ctx ctx;
>     long long now = time_msec();
>     size_t i = 0;
> -    uint8_t arrcnt = 0;
> -
> -    BUILD_ASSERT_DECL(sizeof arr[0].maps * CHAR_BIT >= NETDEV_MAX_BURST);
> 
>     if (helper) {
>         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
> @@ -342,48 +329,25 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
>         /* Continue without the helper */
>     }
> 
> -    memset(bucket_list, INT8_C(-1), sizeof bucket_list);
>     for (i = 0; i < cnt; i++) {
> -        unsigned bucket;
> 
> -        if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
> +        if (!conn_key_extract(ct, pkts[i], dl_type, &ctx, zone)) {
>             write_ct_md(pkts[i], CS_INVALID, zone, NULL, NULL);
>             continue;
>         }
> 
> -        bucket = hash_to_bucket(ctxs[i].hash);
> -        if (bucket_list[bucket] == INT8_C(-1)) {
> -            bucket_list[bucket] = arrcnt;
> -
> -            arr[arrcnt].maps = 0;
> -            ULLONG_SET1(arr[arrcnt].maps, i);
> -            arr[arrcnt++].bucket = bucket;
> -        } else {
> -            ULLONG_SET1(arr[bucket_list[bucket]].maps, i);
> -        }
> -    }
> -
> -    for (i = 0; i < arrcnt; i++) {
> -        struct conntrack_bucket *ctb = &ct->buckets[arr[i].bucket];
> -        size_t j;
> -
> +        struct conntrack_bucket *ctb = &ct->buckets[i];
>         ct_lock_lock(&ctb->lock);
> +        conn_key_lookup(ctb, &ctx, now);
> +        struct conn *conn = process_one(ct, pkts[i], &ctx, zone,
> +                                        force, commit, now);
> 
> -        ULLONG_FOR_EACH_1(j, arr[i].maps) {
> -            struct conn *conn;
> -
> -            conn_key_lookup(ctb, &ctxs[j], now);
> -
> -            conn = process_one(ct, pkts[j], &ctxs[j], zone, force, commit,
> -                               now);
> -
> -            if (conn && setmark) {
> -                set_mark(pkts[j], conn, setmark[0], setmark[1]);
> -            }
> +        if (conn && setmark) {
> +            set_mark(pkts[i], conn, setmark[0], setmark[1]);
> +        }
> 
> -            if (conn && setlabel) {
> -                set_label(pkts[j], conn, &setlabel[0], &setlabel[1]);
> -            }
> +        if (conn && setlabel) {
> +            set_label(pkts[i], conn, &setlabel[0], &setlabel[1]);
>         }
>         ct_lock_unlock(&ctb->lock);
>     }
> -- 
> 1.9.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Darrell Ball May 1, 2017, 6:28 p.m. UTC | #3 
Thanks for pointing that out; it was on my todo list from before and I
forgot about it.

On 5/1/17, 10:54 AM, "ovs-dev-bounces@openvswitch.org on behalf of Jarno Rajahalme" <ovs-dev-bounces@openvswitch.org on behalf of jarno@ovn.org> wrote:

    Would be nice to have a commit message, with the motivation for the change.
    
      Jarno
    
    > On Mar 24, 2017, at 2:15 AM, Darrell Ball <dlu998@gmail.com> wrote:
    > 
    > Signed-off-by: Darrell Ball <dlu998@gmail.com>
    > Acked-by: Flavio Leitner <fbl@sysclose.org>
    > ---
    > lib/conntrack.c | 58 +++++++++++----------------------------------------------
    > 1 file changed, 11 insertions(+), 47 deletions(-)
    > 
    > diff --git a/lib/conntrack.c b/lib/conntrack.c
    > index 4f490fb..9a0763e 100644
    > --- a/lib/conntrack.c
    > +++ b/lib/conntrack.c
    > @@ -318,22 +318,9 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
    > {
    >     struct dp_packet **pkts = pkt_batch->packets;
    >     size_t cnt = pkt_batch->count;
    > -#if !defined(__CHECKER__) && !defined(_WIN32)
    > -    const size_t KEY_ARRAY_SIZE = cnt;
    > -#else
    > -    enum { KEY_ARRAY_SIZE = NETDEV_MAX_BURST };
    > -#endif
    > -    struct conn_lookup_ctx ctxs[KEY_ARRAY_SIZE];
    > -    int8_t bucket_list[CONNTRACK_BUCKETS];
    > -    struct {
    > -        unsigned bucket;
    > -        unsigned long maps;
    > -    } arr[KEY_ARRAY_SIZE];
    > +    struct conn_lookup_ctx ctx;
    >     long long now = time_msec();
    >     size_t i = 0;
    > -    uint8_t arrcnt = 0;
    > -
    > -    BUILD_ASSERT_DECL(sizeof arr[0].maps * CHAR_BIT >= NETDEV_MAX_BURST);
    > 
    >     if (helper) {
    >         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
    > @@ -342,48 +329,25 @@ conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
    >         /* Continue without the helper */
    >     }
    > 
    > -    memset(bucket_list, INT8_C(-1), sizeof bucket_list);
    >     for (i = 0; i < cnt; i++) {
    > -        unsigned bucket;
    > 
    > -        if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
    > +        if (!conn_key_extract(ct, pkts[i], dl_type, &ctx, zone)) {
    >             write_ct_md(pkts[i], CS_INVALID, zone, NULL, NULL);
    >             continue;
    >         }
    > 
    > -        bucket = hash_to_bucket(ctxs[i].hash);
    > -        if (bucket_list[bucket] == INT8_C(-1)) {
    > -            bucket_list[bucket] = arrcnt;
    > -
    > -            arr[arrcnt].maps = 0;
    > -            ULLONG_SET1(arr[arrcnt].maps, i);
    > -            arr[arrcnt++].bucket = bucket;
    > -        } else {
    > -            ULLONG_SET1(arr[bucket_list[bucket]].maps, i);
    > -        }
    > -    }
    > -
    > -    for (i = 0; i < arrcnt; i++) {
    > -        struct conntrack_bucket *ctb = &ct->buckets[arr[i].bucket];
    > -        size_t j;
    > -
    > +        struct conntrack_bucket *ctb = &ct->buckets[i];
    >         ct_lock_lock(&ctb->lock);
    > +        conn_key_lookup(ctb, &ctx, now);
    > +        struct conn *conn = process_one(ct, pkts[i], &ctx, zone,
    > +                                        force, commit, now);
    > 
    > -        ULLONG_FOR_EACH_1(j, arr[i].maps) {
    > -            struct conn *conn;
    > -
    > -            conn_key_lookup(ctb, &ctxs[j], now);
    > -
    > -            conn = process_one(ct, pkts[j], &ctxs[j], zone, force, commit,
    > -                               now);
    > -
    > -            if (conn && setmark) {
    > -                set_mark(pkts[j], conn, setmark[0], setmark[1]);
    > -            }
    > +        if (conn && setmark) {
    > +            set_mark(pkts[i], conn, setmark[0], setmark[1]);
    > +        }
    > 
    > -            if (conn && setlabel) {
    > -                set_label(pkts[j], conn, &setlabel[0], &setlabel[1]);
    > -            }
    > +        if (conn && setlabel) {
    > +            set_label(pkts[i], conn, &setlabel[0], &setlabel[1]);
    >         }
    >         ct_lock_unlock(&ctb->lock);
    >     }
    > -- 
    > 1.9.1
    > 
    > _______________________________________________
    > dev mailing list
    > dev@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=6g0A5S-JcYSJgz5d0NvkCE57V_TZhpdjImW7b6GhnIY&s=mtFK9-RPvtX_F1wWJM0SgoFhEWQ2c7v3iiUpUIQiA_A&e= 
    
    _______________________________________________
    dev mailing list
    dev@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=6g0A5S-JcYSJgz5d0NvkCE57V_TZhpdjImW7b6GhnIY&s=mtFK9-RPvtX_F1wWJM0SgoFhEWQ2c7v3iiUpUIQiA_A&e=
diff mbox

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 4f490fb..9a0763e 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -318,22 +318,9 @@  conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
 {
     struct dp_packet **pkts = pkt_batch->packets;
     size_t cnt = pkt_batch->count;
-#if !defined(__CHECKER__) && !defined(_WIN32)
-    const size_t KEY_ARRAY_SIZE = cnt;
-#else
-    enum { KEY_ARRAY_SIZE = NETDEV_MAX_BURST };
-#endif
-    struct conn_lookup_ctx ctxs[KEY_ARRAY_SIZE];
-    int8_t bucket_list[CONNTRACK_BUCKETS];
-    struct {
-        unsigned bucket;
-        unsigned long maps;
-    } arr[KEY_ARRAY_SIZE];
+    struct conn_lookup_ctx ctx;
     long long now = time_msec();
     size_t i = 0;
-    uint8_t arrcnt = 0;
-
-    BUILD_ASSERT_DECL(sizeof arr[0].maps * CHAR_BIT >= NETDEV_MAX_BURST);
 
     if (helper) {
         static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 5);
@@ -342,48 +329,25 @@  conntrack_execute(struct conntrack *ct, struct dp_packet_batch *pkt_batch,
         /* Continue without the helper */
     }
 
-    memset(bucket_list, INT8_C(-1), sizeof bucket_list);
     for (i = 0; i < cnt; i++) {
-        unsigned bucket;
 
-        if (!conn_key_extract(ct, pkts[i], dl_type, &ctxs[i], zone)) {
+        if (!conn_key_extract(ct, pkts[i], dl_type, &ctx, zone)) {
             write_ct_md(pkts[i], CS_INVALID, zone, NULL, NULL);
             continue;
         }
 
-        bucket = hash_to_bucket(ctxs[i].hash);
-        if (bucket_list[bucket] == INT8_C(-1)) {
-            bucket_list[bucket] = arrcnt;
-
-            arr[arrcnt].maps = 0;
-            ULLONG_SET1(arr[arrcnt].maps, i);
-            arr[arrcnt++].bucket = bucket;
-        } else {
-            ULLONG_SET1(arr[bucket_list[bucket]].maps, i);
-        }
-    }
-
-    for (i = 0; i < arrcnt; i++) {
-        struct conntrack_bucket *ctb = &ct->buckets[arr[i].bucket];
-        size_t j;
-
+        struct conntrack_bucket *ctb = &ct->buckets[i];
         ct_lock_lock(&ctb->lock);
+        conn_key_lookup(ctb, &ctx, now);
+        struct conn *conn = process_one(ct, pkts[i], &ctx, zone,
+                                        force, commit, now);
 
-        ULLONG_FOR_EACH_1(j, arr[i].maps) {
-            struct conn *conn;
-
-            conn_key_lookup(ctb, &ctxs[j], now);
-
-            conn = process_one(ct, pkts[j], &ctxs[j], zone, force, commit,
-                               now);
-
-            if (conn && setmark) {
-                set_mark(pkts[j], conn, setmark[0], setmark[1]);
-            }
+        if (conn && setmark) {
+            set_mark(pkts[i], conn, setmark[0], setmark[1]);
+        }
 
-            if (conn && setlabel) {
-                set_label(pkts[j], conn, &setlabel[0], &setlabel[1]);
-            }
+        if (conn && setlabel) {
+            set_label(pkts[i], conn, &setlabel[0], &setlabel[1]);
         }
         ct_lock_unlock(&ctb->lock);
     }