Re: [ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.

[Daniele Di Proietto]

On 27/07/2016 21:13, "Joe Stringer"  wrote:

>On 27 July 2016 at 19:01, Daniele Di Proietto  wrote:
>>
>>
>>
>>
>>
>> On 27/07/2016 17:14, "Joe Stringer"  wrote:
>>
>>>On 26 July 2016 at 17:58, Daniele Di Proietto  wrote:
 This commit adds a thread that periodically removes expired connections.

 The expiration time of a connection can be expressed by:

 expiration = now + timeout

 For each possible 'timeout' value (there aren't many) we keep a list.
 When the expiration is updated, we move the connection to the back of the
 corresponding 'timeout' list. This ways, the list is always ordered by
 'expiration'.

 When the cleanup thread iterates through the lists for expired
 connections, it can stop at the first non expired connection.

 Suggested-by: Joe Stringer 
 Signed-off-by: Daniele Di Proietto 
>>>
>>>Acked-by: Joe Stringer 
>>
>> Thanks for the review!
>>
>>>
>>>Minor comments on comments below. Thanks!
>>>
>>>
>>>
 +/* Cleanup:
 + *
>>>
>>>Extra line.
>>
>> Fixed
>>
>>>
 + * We must call conntrack_clean() periodically.  conntrack_clean() return
 + * value gives an hint on when the next cleanup must be done (either 
 because
 + * there is an actual connection that expires, or because a new connection
 + * might be created with the minimum timeout).
 + *
 + * The logic below has two goals:
 + *
 + * - Avoid calling conntrack_clean() too often.  If we call 
 conntrack_clean()
 + *   each time a connection expires, the thread will consume 100% CPU, so 
 we
 + *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to 
 batch
 + *   removal.
>>>
>>>Isn't it CT_CLEAN_MIN_INTERVAL that prevents calls happening too
>>>often? I would imagine that under high cps conditions where you're
>>>likely to peg 100% on cleanup, cleanup is behind and CT_CLEAN_INTERVAL
>>>logic won't come into the picture.
>>>
 + * - On the other hand, it's not a good idea to keep the buckets locked 
 for
 + *   too long, as we might prevent traffic from flowing.  If 
 conntrack_clean()
 + *   returns a value which is in the past, it means that the internal 
 limit
 + *   has been reached and more cleanup is required.  In this case, just 
 wait
 + *   CT_CLEAN_MIN_INTERVAL before the next call.
 + */
>>>
>>>Keeping the buckets locked too long also happens if you constantly
>>>call conntrack_clean(), so I think these two paragraphs are arguing
>>>slightly different angles for the same parameter.
>>>
>>>CT_CLEAN_MIN_INTERVAL ensures that if cleanup is behind, there is
>>>atleast some 200ms blocks of time when buckets will be left alone so
>>>the datapath can operate unhindered.
>>>
>>>CT_CLEAN_INTERVAL ensures that if we are coping with the current
>>>cleanup tasks, then we wait at least 5 seconds to do further cleanup.
>>>This seems like it's more targeted towards reducing wakeups when there
>>>is a wide distribution of timeouts but relatively small number of
>>>connections, that could be handled by less frequent cleanups.
>>>
>>>I like the "the logic has two goals" presentation of this, but maybe
>>>there is a better way we can frame the comment above?
>>
>> I couldn't have said it better, I almost stole your wording entirely:
>>
>> + * The logic below has two goals:
>> + *
>> + * - Avoid calling conntrack_clean() too often.  If we call 
>> conntrack_clean()
>> + *   each time a connection expires, the thread will consume 100% CPU, so we
>> + *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to 
>> batch
>> + *   removal.
>> + *
>> + * - On the other hand, it's not a good idea to keep the buckets locked for
>> + *   too long, as we might prevent traffic from flowing.  If 
>> conntrack_clean()
>> + *   returns a value which is in the past, it means that the internal limit
>> + *   has been reached and more cleanup is required.  In this case, just wait
>> + *   CT_CLEAN_MIN_INTERVAL before the next call.
>
>Is the new wording missing? This looks the same as the old wording.

Oops, you're right, copy paste mistake, this is what I changed it to:

* The logic below has two goals:
 *
 * - We want to reduce the number of wakeups and batch connection cleanup
 *   when the load is not very high.  CT_CLEAN_INTERVAL ensures that if we
 *   are coping with the current cleanup tasks, then we wait at least
 *   5 seconds to do further cleanup.
 *
 * - We don't want to keep the buckets locked too long, as we might prevent
 *   traffic from flowing.  CT_CLEAN_MIN_INTERVAL ensures that if cleanup is
 *   behind, there is at least some 200ms blocks of time when buckets will be
 *   left alone, so the datapath can operate unhindered.

___
dev mailing list
dev@openvswitch.org

Re: [ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.

[Joe Stringer]

On 27 July 2016 at 19:01, Daniele Di Proietto  wrote:
>
>
>
>
>
> On 27/07/2016 17:14, "Joe Stringer"  wrote:
>
>>On 26 July 2016 at 17:58, Daniele Di Proietto  wrote:
>>> This commit adds a thread that periodically removes expired connections.
>>>
>>> The expiration time of a connection can be expressed by:
>>>
>>> expiration = now + timeout
>>>
>>> For each possible 'timeout' value (there aren't many) we keep a list.
>>> When the expiration is updated, we move the connection to the back of the
>>> corresponding 'timeout' list. This ways, the list is always ordered by
>>> 'expiration'.
>>>
>>> When the cleanup thread iterates through the lists for expired
>>> connections, it can stop at the first non expired connection.
>>>
>>> Suggested-by: Joe Stringer 
>>> Signed-off-by: Daniele Di Proietto 
>>
>>Acked-by: Joe Stringer 
>
> Thanks for the review!
>
>>
>>Minor comments on comments below. Thanks!
>>
>>
>>
>>> +/* Cleanup:
>>> + *
>>
>>Extra line.
>
> Fixed
>
>>
>>> + * We must call conntrack_clean() periodically.  conntrack_clean() return
>>> + * value gives an hint on when the next cleanup must be done (either 
>>> because
>>> + * there is an actual connection that expires, or because a new connection
>>> + * might be created with the minimum timeout).
>>> + *
>>> + * The logic below has two goals:
>>> + *
>>> + * - Avoid calling conntrack_clean() too often.  If we call 
>>> conntrack_clean()
>>> + *   each time a connection expires, the thread will consume 100% CPU, so 
>>> we
>>> + *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to 
>>> batch
>>> + *   removal.
>>
>>Isn't it CT_CLEAN_MIN_INTERVAL that prevents calls happening too
>>often? I would imagine that under high cps conditions where you're
>>likely to peg 100% on cleanup, cleanup is behind and CT_CLEAN_INTERVAL
>>logic won't come into the picture.
>>
>>> + * - On the other hand, it's not a good idea to keep the buckets locked for
>>> + *   too long, as we might prevent traffic from flowing.  If 
>>> conntrack_clean()
>>> + *   returns a value which is in the past, it means that the internal limit
>>> + *   has been reached and more cleanup is required.  In this case, just 
>>> wait
>>> + *   CT_CLEAN_MIN_INTERVAL before the next call.
>>> + */
>>
>>Keeping the buckets locked too long also happens if you constantly
>>call conntrack_clean(), so I think these two paragraphs are arguing
>>slightly different angles for the same parameter.
>>
>>CT_CLEAN_MIN_INTERVAL ensures that if cleanup is behind, there is
>>atleast some 200ms blocks of time when buckets will be left alone so
>>the datapath can operate unhindered.
>>
>>CT_CLEAN_INTERVAL ensures that if we are coping with the current
>>cleanup tasks, then we wait at least 5 seconds to do further cleanup.
>>This seems like it's more targeted towards reducing wakeups when there
>>is a wide distribution of timeouts but relatively small number of
>>connections, that could be handled by less frequent cleanups.
>>
>>I like the "the logic has two goals" presentation of this, but maybe
>>there is a better way we can frame the comment above?
>
> I couldn't have said it better, I almost stole your wording entirely:
>
> + * The logic below has two goals:
> + *
> + * - Avoid calling conntrack_clean() too often.  If we call conntrack_clean()
> + *   each time a connection expires, the thread will consume 100% CPU, so we
> + *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to 
> batch
> + *   removal.
> + *
> + * - On the other hand, it's not a good idea to keep the buckets locked for
> + *   too long, as we might prevent traffic from flowing.  If 
> conntrack_clean()
> + *   returns a value which is in the past, it means that the internal limit
> + *   has been reached and more cleanup is required.  In this case, just wait
> + *   CT_CLEAN_MIN_INTERVAL before the next call.

Is the new wording missing? This looks the same as the old wording.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.

[Daniele Di Proietto]

On 27/07/2016 17:14, "Joe Stringer"  wrote:

>On 26 July 2016 at 17:58, Daniele Di Proietto  wrote:
>> This commit adds a thread that periodically removes expired connections.
>>
>> The expiration time of a connection can be expressed by:
>>
>> expiration = now + timeout
>>
>> For each possible 'timeout' value (there aren't many) we keep a list.
>> When the expiration is updated, we move the connection to the back of the
>> corresponding 'timeout' list. This ways, the list is always ordered by
>> 'expiration'.
>>
>> When the cleanup thread iterates through the lists for expired
>> connections, it can stop at the first non expired connection.
>>
>> Suggested-by: Joe Stringer 
>> Signed-off-by: Daniele Di Proietto 
>
>Acked-by: Joe Stringer 

Thanks for the review!

>
>Minor comments on comments below. Thanks!
>
>
>
>> +/* Cleanup:
>> + *
>
>Extra line.

Fixed

>
>> + * We must call conntrack_clean() periodically.  conntrack_clean() return
>> + * value gives an hint on when the next cleanup must be done (either because
>> + * there is an actual connection that expires, or because a new connection
>> + * might be created with the minimum timeout).
>> + *
>> + * The logic below has two goals:
>> + *
>> + * - Avoid calling conntrack_clean() too often.  If we call 
>> conntrack_clean()
>> + *   each time a connection expires, the thread will consume 100% CPU, so we
>> + *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to 
>> batch
>> + *   removal.
>
>Isn't it CT_CLEAN_MIN_INTERVAL that prevents calls happening too
>often? I would imagine that under high cps conditions where you're
>likely to peg 100% on cleanup, cleanup is behind and CT_CLEAN_INTERVAL
>logic won't come into the picture.
>
>> + * - On the other hand, it's not a good idea to keep the buckets locked for
>> + *   too long, as we might prevent traffic from flowing.  If 
>> conntrack_clean()
>> + *   returns a value which is in the past, it means that the internal limit
>> + *   has been reached and more cleanup is required.  In this case, just wait
>> + *   CT_CLEAN_MIN_INTERVAL before the next call.
>> + */
>
>Keeping the buckets locked too long also happens if you constantly
>call conntrack_clean(), so I think these two paragraphs are arguing
>slightly different angles for the same parameter.
>
>CT_CLEAN_MIN_INTERVAL ensures that if cleanup is behind, there is
>atleast some 200ms blocks of time when buckets will be left alone so
>the datapath can operate unhindered.
>
>CT_CLEAN_INTERVAL ensures that if we are coping with the current
>cleanup tasks, then we wait at least 5 seconds to do further cleanup.
>This seems like it's more targeted towards reducing wakeups when there
>is a wide distribution of timeouts but relatively small number of
>connections, that could be handled by less frequent cleanups.
>
>I like the "the logic has two goals" presentation of this, but maybe
>there is a better way we can frame the comment above?

I couldn't have said it better, I almost stole your wording entirely:

+ * The logic below has two goals:
+ *
+ * - Avoid calling conntrack_clean() too often.  If we call conntrack_clean()
+ *   each time a connection expires, the thread will consume 100% CPU, so we
+ *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to batch
+ *   removal.
+ *
+ * - On the other hand, it's not a good idea to keep the buckets locked for
+ *   too long, as we might prevent traffic from flowing.  If conntrack_clean()
+ *   returns a value which is in the past, it means that the internal limit
+ *   has been reached and more cleanup is required.  In this case, just wait
+ *   CT_CLEAN_MIN_INTERVAL before the next call.

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.

[Joe Stringer]

On 26 July 2016 at 17:58, Daniele Di Proietto  wrote:
> This commit adds a thread that periodically removes expired connections.
>
> The expiration time of a connection can be expressed by:
>
> expiration = now + timeout
>
> For each possible 'timeout' value (there aren't many) we keep a list.
> When the expiration is updated, we move the connection to the back of the
> corresponding 'timeout' list. This ways, the list is always ordered by
> 'expiration'.
>
> When the cleanup thread iterates through the lists for expired
> connections, it can stop at the first non expired connection.
>
> Suggested-by: Joe Stringer 
> Signed-off-by: Daniele Di Proietto 

Acked-by: Joe Stringer 

Minor comments on comments below. Thanks!



> +/* Cleanup:
> + *

Extra line.

> + * We must call conntrack_clean() periodically.  conntrack_clean() return
> + * value gives an hint on when the next cleanup must be done (either because
> + * there is an actual connection that expires, or because a new connection
> + * might be created with the minimum timeout).
> + *
> + * The logic below has two goals:
> + *
> + * - Avoid calling conntrack_clean() too often.  If we call conntrack_clean()
> + *   each time a connection expires, the thread will consume 100% CPU, so we
> + *   try to call the function _at most_ once every CT_CLEAN_INTERVAL, to 
> batch
> + *   removal.

Isn't it CT_CLEAN_MIN_INTERVAL that prevents calls happening too
often? I would imagine that under high cps conditions where you're
likely to peg 100% on cleanup, cleanup is behind and CT_CLEAN_INTERVAL
logic won't come into the picture.

> + * - On the other hand, it's not a good idea to keep the buckets locked for
> + *   too long, as we might prevent traffic from flowing.  If 
> conntrack_clean()
> + *   returns a value which is in the past, it means that the internal limit
> + *   has been reached and more cleanup is required.  In this case, just wait
> + *   CT_CLEAN_MIN_INTERVAL before the next call.
> + */

Keeping the buckets locked too long also happens if you constantly
call conntrack_clean(), so I think these two paragraphs are arguing
slightly different angles for the same parameter.

CT_CLEAN_MIN_INTERVAL ensures that if cleanup is behind, there is
atleast some 200ms blocks of time when buckets will be left alone so
the datapath can operate unhindered.

CT_CLEAN_INTERVAL ensures that if we are coping with the current
cleanup tasks, then we wait at least 5 seconds to do further cleanup.
This seems like it's more targeted towards reducing wakeups when there
is a wide distribution of timeouts but relatively small number of
connections, that could be handled by less frequent cleanups.

I like the "the logic has two goals" presentation of this, but maybe
there is a better way we can frame the comment above?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.

[Daniele Di Proietto]

This commit adds a thread that periodically removes expired connections.

The expiration time of a connection can be expressed by:

expiration = now + timeout

For each possible 'timeout' value (there aren't many) we keep a list.
When the expiration is updated, we move the connection to the back of the
corresponding 'timeout' list. This ways, the list is always ordered by
'expiration'.

When the cleanup thread iterates through the lists for expired
connections, it can stop at the first non expired connection.

Suggested-by: Joe Stringer 
Signed-off-by: Daniele Di Proietto 
---
 lib/conntrack-other.c   |  11 +--
 lib/conntrack-private.h |  21 --
 lib/conntrack-tcp.c |  20 +++---
 lib/conntrack.c | 186 
 lib/conntrack.h |  36 +-
 5 files changed, 243 insertions(+), 31 deletions(-)

diff --git a/lib/conntrack-other.c b/lib/conntrack-other.c
index 295cb2c..2920889 100644
--- a/lib/conntrack-other.c
+++ b/lib/conntrack-other.c
@@ -43,8 +43,8 @@ conn_other_cast(const struct conn *conn)
 }
 
 static enum ct_update_res
-other_conn_update(struct conn *conn_, struct dp_packet *pkt OVS_UNUSED,
-  bool reply, long long now)
+other_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
+  struct dp_packet *pkt OVS_UNUSED, bool reply, long long now)
 {
 struct conn_other *conn = conn_other_cast(conn_);
 
@@ -54,7 +54,7 @@ other_conn_update(struct conn *conn_, struct dp_packet *pkt 
OVS_UNUSED,
 conn->state = OTHERS_MULTIPLE;
 }
 
-update_expiration(conn_, other_timeouts[conn->state], now);
+conn_update_expiration(ctb, >up, other_timeouts[conn->state], now);
 
 return CT_UPDATE_VALID;
 }
@@ -66,14 +66,15 @@ other_valid_new(struct dp_packet *pkt OVS_UNUSED)
 }
 
 static struct conn *
-other_new_conn(struct dp_packet *pkt OVS_UNUSED, long long now)
+other_new_conn(struct conntrack_bucket *ctb, struct dp_packet *pkt OVS_UNUSED,
+   long long now)
 {
 struct conn_other *conn;
 
 conn = xzalloc(sizeof *conn);
 conn->state = OTHERS_FIRST;
 
-update_expiration(>up, other_timeouts[conn->state], now);
+conn_init_expiration(ctb, >up, other_timeouts[conn->state], now);
 
 return >up;
 }
diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index bc32448..5aac938 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -69,10 +69,13 @@ enum ct_update_res {
 };
 
 struct ct_l4_proto {
-struct conn *(*new_conn)(struct dp_packet *pkt, long long now);
+struct conn *(*new_conn)(struct conntrack_bucket *, struct dp_packet *pkt,
+ long long now);
 bool (*valid_new)(struct dp_packet *pkt);
-enum ct_update_res (*conn_update)(struct conn *conn, struct dp_packet *pkt,
-  bool reply, long long now);
+enum ct_update_res (*conn_update)(struct conn *conn,
+  struct conntrack_bucket *,
+  struct dp_packet *pkt, bool reply,
+  long long now);
 };
 
 extern struct ct_l4_proto ct_proto_tcp;
@@ -81,9 +84,19 @@ extern struct ct_l4_proto ct_proto_other;
 extern long long ct_timeout_val[];
 
 static inline void
-update_expiration(struct conn *conn, enum ct_timeout tm, long long now)
+conn_init_expiration(struct conntrack_bucket *ctb, struct conn *conn,
+enum ct_timeout tm, long long now)
 {
 conn->expiration = now + ct_timeout_val[tm];
+ovs_list_push_back(>exp_lists[tm], >exp_node);
+}
+
+static inline void
+conn_update_expiration(struct conntrack_bucket *ctb, struct conn *conn,
+   enum ct_timeout tm, long long now)
+{
+ovs_list_remove(>exp_node);
+conn_init_expiration(ctb, conn, tm, now);
 }
 
 #endif /* conntrack-private.h */
diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
index 6da798d..7edcce3 100644
--- a/lib/conntrack-tcp.c
+++ b/lib/conntrack-tcp.c
@@ -152,8 +152,8 @@ tcp_payload_length(struct dp_packet *pkt)
 }
 
 static enum ct_update_res
-tcp_conn_update(struct conn* conn_, struct dp_packet *pkt, bool reply,
-long long now)
+tcp_conn_update(struct conn *conn_, struct conntrack_bucket *ctb,
+struct dp_packet *pkt, bool reply, long long now)
 {
 struct conn_tcp *conn = conn_tcp_cast(conn_);
 struct tcp_header *tcp = dp_packet_l4(pkt);
@@ -319,18 +319,18 @@ tcp_conn_update(struct conn* conn_, struct dp_packet 
*pkt, bool reply,
 
 if (src->state >= CT_DPIF_TCPS_FIN_WAIT_2
 && dst->state >= CT_DPIF_TCPS_FIN_WAIT_2) {
-update_expiration(conn_, CT_TM_TCP_CLOSED, now);
+conn_update_expiration(ctb, >up, CT_TM_TCP_CLOSED, now);
 } else if (src->state >= CT_DPIF_TCPS_CLOSING
&& dst->state >= CT_DPIF_TCPS_CLOSING) {
-