Re: [ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.
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 http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v5 05/16] conntrack: Periodically delete expired connections.
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.
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.
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.
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, &conn->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(&conn->up, other_timeouts[conn->state], now); +conn_init_expiration(ctb, &conn->up, other_timeouts[conn->state], now); return &conn->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(&ctb->exp_lists[tm], &conn->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(&conn->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, &conn->up, CT_TM_TCP_CLOSED, now); } else if (src->state >= CT_DPIF_TCPS_CLOSING && dst->state >= CT_DPIF_TCPS_CLOSING) { -update_expirati