diff mbox series

[ovs-dev,3/5] conntrack: r/w clean-up interval.

Message ID 1505730091-19042-3-git-send-email-antonio.fischetti@intel.com
State Deferred
Delegated to: Darrell Ball
Headers show
Series [ovs-dev,1/5] conntrack: add commands to r/w conntrack parameters. | expand

Commit Message

Fischetti, Antonio Sept. 18, 2017, 10:21 a.m. UTC
Read/Write conntrack clean-up interval used by
the clean_thread_main() thread.

Example:
   ovs-appctl dpctl/ct-set cleanup=4000  # Set a new value
   ovs-appctl dpctl/ct-get cleanup       # Read

Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>
---
 lib/conntrack.c | 27 ++++++++++++++++++++++++---
 lib/conntrack.h |  2 ++
 2 files changed, 26 insertions(+), 3 deletions(-)

Comments

Darrell Ball Sept. 19, 2017, 8:27 p.m. UTC | #1
Hi Antonio

What is the motivation for this ?
I don’t think this is a good idea, as it should not be needed under normal
usage and has the potential to create unnecessary issues for the user and
also maintenance issues.

Thanks Darrell

On 9/18/17, 3:23 AM, "ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of antonio.fischetti@intel.com> wrote:

    Read/Write conntrack clean-up interval used by
    the clean_thread_main() thread.
    
    Example:
       ovs-appctl dpctl/ct-set cleanup=4000  # Set a new value
       ovs-appctl dpctl/ct-get cleanup       # Read
    
    Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

    ---
     lib/conntrack.c | 27 ++++++++++++++++++++++++---
     lib/conntrack.h |  2 ++
     2 files changed, 26 insertions(+), 3 deletions(-)
    
    diff --git a/lib/conntrack.c b/lib/conntrack.c
    index 6d86625..60eb376 100644
    --- a/lib/conntrack.c
    +++ b/lib/conntrack.c
    @@ -225,6 +225,9 @@ conn_key_cmp(const struct conn_key *key1, const struct conn_key *key2)
         return 1;
     }
     
    +#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */
    +#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */
    +
     /* Initializes the connection tracker 'ct'.  The caller is responsible for
      * calling 'conntrack_destroy()', when the instance is not needed anymore */
     void
    @@ -258,6 +261,7 @@ conntrack_init(struct conntrack *ct)
         ct->hash_basis = random_uint32();
         atomic_count_init(&ct->n_conn, 0);
         atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);
    +    ct->clean_interval = CT_CLEAN_INTERVAL;
         latch_init(&ct->clean_thread_exit);
         ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main, ct);
     }
    @@ -1327,8 +1331,6 @@ next_bucket:
      *   behind, there is at least some 200ms blocks of time when buckets will be
      *   left alone, so the datapath can operate unhindered.
      */
    -#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */
    -#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */
     
     static void *
     clean_thread_main(void *f_)
    @@ -1344,7 +1346,7 @@ clean_thread_main(void *f_)
             if (next_wake < now) {
                 poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL);
             } else {
    -            poll_timer_wait_until(MAX(next_wake, now + CT_CLEAN_INTERVAL));
    +            poll_timer_wait_until(MAX(next_wake, now + ct->clean_interval));
             }
             latch_wait(&ct->clean_thread_exit);
             poll_block();
    @@ -2398,6 +2400,21 @@ conntrack_flush(struct conntrack *ct, const uint16_t *zone)
         return 0;
     }
     
    +/* Set an interval value to be used by clean_thread_main. */
    +static int
    +wr_clean_int(struct conntrack *ct, uint32_t new_val) {
    +    ct->clean_interval = new_val;
    +    VLOG_DBG("Set clean interval to %d", new_val);
    +    return 0;
    +}
    +
    +/* Read current clean-up interval used by clean_thread_main. */
    +static int
    +rd_clean_int(struct conntrack *ct, uint32_t *cur_val) {
    +    *cur_val = ct->clean_interval;
    +    return 0;
    +}
    +
     /* Set a new value for the upper limit of connections. */
     static int
     wr_max_conn(struct conntrack *ct, uint32_t new_val) {
    @@ -2414,11 +2431,15 @@ rd_max_conn(struct conntrack *ct, uint32_t *cur_val) {
     }
     
     /* List of managed parameters. */
    +/* Max nr of connections managed by CT module. */
     #define CT_RW_MAX_CONN "maxconn"
    +/* Clean-up interval used by clean_thread_main() thread. */
    +#define CT_RW_CLEAN_INTERVAL "cleanup"
     
     /* List of parameters that can be read/written at run-time. */
     struct ct_wk_params wk_params[] = {
         {CT_RW_MAX_CONN, wr_max_conn, rd_max_conn},
    +    {CT_RW_CLEAN_INTERVAL, wr_clean_int, rd_clean_int},
     };
     
     int
    diff --git a/lib/conntrack.h b/lib/conntrack.h
    index 4eb9a9a..ba9d3f1 100644
    --- a/lib/conntrack.h
    +++ b/lib/conntrack.h
    @@ -261,6 +261,8 @@ struct conntrack {
         pthread_t clean_thread;
         /* Latch to destroy the 'clean_thread' */
         struct latch clean_thread_exit;
    +    /* Clean interval. */
    +    uint32_t clean_interval;
     
         /* Number of connections currently in the connection tracker. */
         atomic_count n_conn;
    -- 
    2.4.11
    
    _______________________________________________
    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=KufLQwhfzaOAv5qQm_yEbJGussIlaYUvadeEqpKvDwQ&s=q6sQTxV7hxR1iqdLjnyaom-K3njxys4KHag_KG8uoHM&e=
Fischetti, Antonio Sept. 20, 2017, 6:33 p.m. UTC | #2
> -----Original Message-----

> From: Darrell Ball [mailto:dball@vmware.com]

> Sent: Tuesday, September 19, 2017 9:28 PM

> To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org

> Subject: Re: [ovs-dev] [PATCH 3/5] conntrack: r/w clean-up interval.

> 

> Hi Antonio

> 

> What is the motivation for this ?

> I don’t think this is a good idea, as it should not be needed under normal

> usage and has the potential to create unnecessary issues for the user and

> also maintenance issues.


[Antonio]
Agree, this would be more for debugging/experimenting purposes.
I will remove this patch from the series.

BTW let me know if you think some other CT cfg parameters could be 
read/written - or just read - by this ct-set-glbl-cfg command. 
For example, the Alg expectation timeout?

Thanks,
Antonio

> 

> Thanks Darrell

> 

> On 9/18/17, 3:23 AM, "ovs-dev-bounces@openvswitch.org on behalf of

> antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of

> antonio.fischetti@intel.com> wrote:

> 

>     Read/Write conntrack clean-up interval used by

>     the clean_thread_main() thread.

> 

>     Example:

>        ovs-appctl dpctl/ct-set cleanup=4000  # Set a new value

>        ovs-appctl dpctl/ct-get cleanup       # Read

> 

>     Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

>     ---

>      lib/conntrack.c | 27 ++++++++++++++++++++++++---

>      lib/conntrack.h |  2 ++

>      2 files changed, 26 insertions(+), 3 deletions(-)

> 

>     diff --git a/lib/conntrack.c b/lib/conntrack.c

>     index 6d86625..60eb376 100644

>     --- a/lib/conntrack.c

>     +++ b/lib/conntrack.c

>     @@ -225,6 +225,9 @@ conn_key_cmp(const struct conn_key *key1, const struct

> conn_key *key2)

>          return 1;

>      }

> 

>     +#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */

>     +#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */

>     +

>      /* Initializes the connection tracker 'ct'.  The caller is responsible for

>       * calling 'conntrack_destroy()', when the instance is not needed anymore

> */

>      void

>     @@ -258,6 +261,7 @@ conntrack_init(struct conntrack *ct)

>          ct->hash_basis = random_uint32();

>          atomic_count_init(&ct->n_conn, 0);

>          atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);

>     +    ct->clean_interval = CT_CLEAN_INTERVAL;

>          latch_init(&ct->clean_thread_exit);

>          ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main,

> ct);

>      }

>     @@ -1327,8 +1331,6 @@ next_bucket:

>       *   behind, there is at least some 200ms blocks of time when buckets will

> be

>       *   left alone, so the datapath can operate unhindered.

>       */

>     -#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */

>     -#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */

> 

>      static void *

>      clean_thread_main(void *f_)

>     @@ -1344,7 +1346,7 @@ clean_thread_main(void *f_)

>              if (next_wake < now) {

>                  poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL);

>              } else {

>     -            poll_timer_wait_until(MAX(next_wake, now +

> CT_CLEAN_INTERVAL));

>     +            poll_timer_wait_until(MAX(next_wake, now + ct-

> >clean_interval));

>              }

>              latch_wait(&ct->clean_thread_exit);

>              poll_block();

>     @@ -2398,6 +2400,21 @@ conntrack_flush(struct conntrack *ct, const uint16_t

> *zone)

>          return 0;

>      }

> 

>     +/* Set an interval value to be used by clean_thread_main. */

>     +static int

>     +wr_clean_int(struct conntrack *ct, uint32_t new_val) {

>     +    ct->clean_interval = new_val;

>     +    VLOG_DBG("Set clean interval to %d", new_val);

>     +    return 0;

>     +}

>     +

>     +/* Read current clean-up interval used by clean_thread_main. */

>     +static int

>     +rd_clean_int(struct conntrack *ct, uint32_t *cur_val) {

>     +    *cur_val = ct->clean_interval;

>     +    return 0;

>     +}

>     +

>      /* Set a new value for the upper limit of connections. */

>      static int

>      wr_max_conn(struct conntrack *ct, uint32_t new_val) {

>     @@ -2414,11 +2431,15 @@ rd_max_conn(struct conntrack *ct, uint32_t

> *cur_val) {

>      }

> 

>      /* List of managed parameters. */

>     +/* Max nr of connections managed by CT module. */

>      #define CT_RW_MAX_CONN "maxconn"

>     +/* Clean-up interval used by clean_thread_main() thread. */

>     +#define CT_RW_CLEAN_INTERVAL "cleanup"

> 

>      /* List of parameters that can be read/written at run-time. */

>      struct ct_wk_params wk_params[] = {

>          {CT_RW_MAX_CONN, wr_max_conn, rd_max_conn},

>     +    {CT_RW_CLEAN_INTERVAL, wr_clean_int, rd_clean_int},

>      };

> 

>      int

>     diff --git a/lib/conntrack.h b/lib/conntrack.h

>     index 4eb9a9a..ba9d3f1 100644

>     --- a/lib/conntrack.h

>     +++ b/lib/conntrack.h

>     @@ -261,6 +261,8 @@ struct conntrack {

>          pthread_t clean_thread;

>          /* Latch to destroy the 'clean_thread' */

>          struct latch clean_thread_exit;

>     +    /* Clean interval. */

>     +    uint32_t clean_interval;

> 

>          /* Number of connections currently in the connection tracker. */

>          atomic_count n_conn;

>     --

>     2.4.11

> 

>     _______________________________________________

>     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=KufLQwhfzaOAv5qQm_yEbJGussIlaYUvadeEqpKvDwQ&s=q6sQTxV7hxR1iqdLjnyaom-

> K3njxys4KHag_KG8uoHM&e=

>
Darrell Ball Sept. 21, 2017, 7:12 a.m. UTC | #3
On 9/20/17, 11:33 AM, "Fischetti, Antonio" <antonio.fischetti@intel.com> wrote:

    
    > -----Original Message-----

    > From: Darrell Ball [mailto:dball@vmware.com]

    > Sent: Tuesday, September 19, 2017 9:28 PM

    > To: Fischetti, Antonio <antonio.fischetti@intel.com>; dev@openvswitch.org

    > Subject: Re: [ovs-dev] [PATCH 3/5] conntrack: r/w clean-up interval.

    > 

    > Hi Antonio

    > 

    > What is the motivation for this ?

    > I don’t think this is a good idea, as it should not be needed under normal

    > usage and has the potential to create unnecessary issues for the user and

    > also maintenance issues.

    
    [Antonio]
    Agree, this would be more for debugging/experimenting purposes.
    I will remove this patch from the series.
    
    BTW let me know if you think some other CT cfg parameters could be 
    read/written - or just read - by this ct-set-glbl-cfg command. 
    For example, the Alg expectation timeout?

[Darrell] I left the expectation timeout simple for now as the area can be dicey;
timeouts are probably best left as read-only for now.


    
    Thanks,
    Antonio
    
    > 

    > Thanks Darrell

    > 

    > On 9/18/17, 3:23 AM, "ovs-dev-bounces@openvswitch.org on behalf of

    > antonio.fischetti@intel.com" <ovs-dev-bounces@openvswitch.org on behalf of

    > antonio.fischetti@intel.com> wrote:

    > 

    >     Read/Write conntrack clean-up interval used by

    >     the clean_thread_main() thread.

    > 

    >     Example:

    >        ovs-appctl dpctl/ct-set cleanup=4000  # Set a new value

    >        ovs-appctl dpctl/ct-get cleanup       # Read

    > 

    >     Signed-off-by: Antonio Fischetti <antonio.fischetti@intel.com>

    >     ---

    >      lib/conntrack.c | 27 ++++++++++++++++++++++++---

    >      lib/conntrack.h |  2 ++

    >      2 files changed, 26 insertions(+), 3 deletions(-)

    > 

    >     diff --git a/lib/conntrack.c b/lib/conntrack.c

    >     index 6d86625..60eb376 100644

    >     --- a/lib/conntrack.c

    >     +++ b/lib/conntrack.c

    >     @@ -225,6 +225,9 @@ conn_key_cmp(const struct conn_key *key1, const struct

    > conn_key *key2)

    >          return 1;

    >      }

    > 

    >     +#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */

    >     +#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */

    >     +

    >      /* Initializes the connection tracker 'ct'.  The caller is responsible for

    >       * calling 'conntrack_destroy()', when the instance is not needed anymore

    > */

    >      void

    >     @@ -258,6 +261,7 @@ conntrack_init(struct conntrack *ct)

    >          ct->hash_basis = random_uint32();

    >          atomic_count_init(&ct->n_conn, 0);

    >          atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);

    >     +    ct->clean_interval = CT_CLEAN_INTERVAL;

    >          latch_init(&ct->clean_thread_exit);

    >          ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main,

    > ct);

    >      }

    >     @@ -1327,8 +1331,6 @@ next_bucket:

    >       *   behind, there is at least some 200ms blocks of time when buckets will

    > be

    >       *   left alone, so the datapath can operate unhindered.

    >       */

    >     -#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */

    >     -#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */

    > 

    >      static void *

    >      clean_thread_main(void *f_)

    >     @@ -1344,7 +1346,7 @@ clean_thread_main(void *f_)

    >              if (next_wake < now) {

    >                  poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL);

    >              } else {

    >     -            poll_timer_wait_until(MAX(next_wake, now +

    > CT_CLEAN_INTERVAL));

    >     +            poll_timer_wait_until(MAX(next_wake, now + ct-

    > >clean_interval));

    >              }

    >              latch_wait(&ct->clean_thread_exit);

    >              poll_block();

    >     @@ -2398,6 +2400,21 @@ conntrack_flush(struct conntrack *ct, const uint16_t

    > *zone)

    >          return 0;

    >      }

    > 

    >     +/* Set an interval value to be used by clean_thread_main. */

    >     +static int

    >     +wr_clean_int(struct conntrack *ct, uint32_t new_val) {

    >     +    ct->clean_interval = new_val;

    >     +    VLOG_DBG("Set clean interval to %d", new_val);

    >     +    return 0;

    >     +}

    >     +

    >     +/* Read current clean-up interval used by clean_thread_main. */

    >     +static int

    >     +rd_clean_int(struct conntrack *ct, uint32_t *cur_val) {

    >     +    *cur_val = ct->clean_interval;

    >     +    return 0;

    >     +}

    >     +

    >      /* Set a new value for the upper limit of connections. */

    >      static int

    >      wr_max_conn(struct conntrack *ct, uint32_t new_val) {

    >     @@ -2414,11 +2431,15 @@ rd_max_conn(struct conntrack *ct, uint32_t

    > *cur_val) {

    >      }

    > 

    >      /* List of managed parameters. */

    >     +/* Max nr of connections managed by CT module. */

    >      #define CT_RW_MAX_CONN "maxconn"

    >     +/* Clean-up interval used by clean_thread_main() thread. */

    >     +#define CT_RW_CLEAN_INTERVAL "cleanup"

    > 

    >      /* List of parameters that can be read/written at run-time. */

    >      struct ct_wk_params wk_params[] = {

    >          {CT_RW_MAX_CONN, wr_max_conn, rd_max_conn},

    >     +    {CT_RW_CLEAN_INTERVAL, wr_clean_int, rd_clean_int},

    >      };

    > 

    >      int

    >     diff --git a/lib/conntrack.h b/lib/conntrack.h

    >     index 4eb9a9a..ba9d3f1 100644

    >     --- a/lib/conntrack.h

    >     +++ b/lib/conntrack.h

    >     @@ -261,6 +261,8 @@ struct conntrack {

    >          pthread_t clean_thread;

    >          /* Latch to destroy the 'clean_thread' */

    >          struct latch clean_thread_exit;

    >     +    /* Clean interval. */

    >     +    uint32_t clean_interval;

    > 

    >          /* Number of connections currently in the connection tracker. */

    >          atomic_count n_conn;

    >     --

    >     2.4.11

    > 

    >     _______________________________________________

    >     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=KufLQwhfzaOAv5qQm_yEbJGussIlaYUvadeEqpKvDwQ&s=q6sQTxV7hxR1iqdLjnyaom-

    > K3njxys4KHag_KG8uoHM&e=

    >
diff mbox series

Patch

diff --git a/lib/conntrack.c b/lib/conntrack.c
index 6d86625..60eb376 100644
--- a/lib/conntrack.c
+++ b/lib/conntrack.c
@@ -225,6 +225,9 @@  conn_key_cmp(const struct conn_key *key1, const struct conn_key *key2)
     return 1;
 }
 
+#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */
+#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */
+
 /* Initializes the connection tracker 'ct'.  The caller is responsible for
  * calling 'conntrack_destroy()', when the instance is not needed anymore */
 void
@@ -258,6 +261,7 @@  conntrack_init(struct conntrack *ct)
     ct->hash_basis = random_uint32();
     atomic_count_init(&ct->n_conn, 0);
     atomic_init(&ct->n_conn_limit, DEFAULT_N_CONN_LIMIT);
+    ct->clean_interval = CT_CLEAN_INTERVAL;
     latch_init(&ct->clean_thread_exit);
     ct->clean_thread = ovs_thread_create("ct_clean", clean_thread_main, ct);
 }
@@ -1327,8 +1331,6 @@  next_bucket:
  *   behind, there is at least some 200ms blocks of time when buckets will be
  *   left alone, so the datapath can operate unhindered.
  */
-#define CT_CLEAN_INTERVAL 5000 /* 5 seconds */
-#define CT_CLEAN_MIN_INTERVAL 200  /* 0.2 seconds */
 
 static void *
 clean_thread_main(void *f_)
@@ -1344,7 +1346,7 @@  clean_thread_main(void *f_)
         if (next_wake < now) {
             poll_timer_wait_until(now + CT_CLEAN_MIN_INTERVAL);
         } else {
-            poll_timer_wait_until(MAX(next_wake, now + CT_CLEAN_INTERVAL));
+            poll_timer_wait_until(MAX(next_wake, now + ct->clean_interval));
         }
         latch_wait(&ct->clean_thread_exit);
         poll_block();
@@ -2398,6 +2400,21 @@  conntrack_flush(struct conntrack *ct, const uint16_t *zone)
     return 0;
 }
 
+/* Set an interval value to be used by clean_thread_main. */
+static int
+wr_clean_int(struct conntrack *ct, uint32_t new_val) {
+    ct->clean_interval = new_val;
+    VLOG_DBG("Set clean interval to %d", new_val);
+    return 0;
+}
+
+/* Read current clean-up interval used by clean_thread_main. */
+static int
+rd_clean_int(struct conntrack *ct, uint32_t *cur_val) {
+    *cur_val = ct->clean_interval;
+    return 0;
+}
+
 /* Set a new value for the upper limit of connections. */
 static int
 wr_max_conn(struct conntrack *ct, uint32_t new_val) {
@@ -2414,11 +2431,15 @@  rd_max_conn(struct conntrack *ct, uint32_t *cur_val) {
 }
 
 /* List of managed parameters. */
+/* Max nr of connections managed by CT module. */
 #define CT_RW_MAX_CONN "maxconn"
+/* Clean-up interval used by clean_thread_main() thread. */
+#define CT_RW_CLEAN_INTERVAL "cleanup"
 
 /* List of parameters that can be read/written at run-time. */
 struct ct_wk_params wk_params[] = {
     {CT_RW_MAX_CONN, wr_max_conn, rd_max_conn},
+    {CT_RW_CLEAN_INTERVAL, wr_clean_int, rd_clean_int},
 };
 
 int
diff --git a/lib/conntrack.h b/lib/conntrack.h
index 4eb9a9a..ba9d3f1 100644
--- a/lib/conntrack.h
+++ b/lib/conntrack.h
@@ -261,6 +261,8 @@  struct conntrack {
     pthread_t clean_thread;
     /* Latch to destroy the 'clean_thread' */
     struct latch clean_thread_exit;
+    /* Clean interval. */
+    uint32_t clean_interval;
 
     /* Number of connections currently in the connection tracker. */
     atomic_count n_conn;