[ovs-dev] [PATCH] conntrack: add tcp_in_liberal option in userspace conntrack
Darrell Ball
dlu998 at gmail.com
Fri May 31 18:10:38 UTC 2019
On Fri, May 31, 2019 at 9:40 AM Darrell Ball <dlu998 at gmail.com> wrote:
>
>
> On Fri, May 31, 2019 at 2:42 AM 姜立东 <jianglidong3 at jd.com> wrote:
>
>> Hi Darrell,
>>
>>
>>
>> Thanks for prompt action.
>>
>>
>>
>> I think there are 2 main differences in your change,
>>
>> 1. Replace Open_vswitch.other_config by dpctl CLI command.
>>
>> 2. One more case to check tcp_liberal as below.
>>
>> @@ -333,10 +335,12 @@ tcp_conn_update(struct conntrack *ct, struct conn
>> *conn_,
>>
>> } else if ((dst->state < CT_DPIF_TCPS_SYN_SENT
>>
>> || dst->state >= CT_DPIF_TCPS_FIN_WAIT_2
>>
>> || src->state >= CT_DPIF_TCPS_FIN_WAIT_2)
>>
>> - && SEQ_GEQ(src->seqhi + MAXACKWINDOW, end)
>>
>> - /* Within a window forward of the originating packet */
>>
>> - && SEQ_GEQ(seq, src->seqlo - MAXACKWINDOW)) {
>>
>> - /* Within a window backward of the originating packet */
>>
>> + && ((SEQ_GEQ(src->seqhi + MAXACKWINDOW, end)
>>
>> + /* Within a window forward of the originating packet
>> */
>>
>> + && SEQ_GEQ(seq, src->seqlo - MAXACKWINDOW))
>>
>> + /* Within a window backward of the originating
>> packet */
>>
>> + || (conntrack_get_tcp_liberal(ct)
>>
>> + && (tcp_flags & TCP_RST) == 0)))
>>
>> For item 1, it is fine for us.
>>
>> For item 2, we didn’t actually hit into this conditional branch in our
>> application, but considering conntrack is in the middle of this path,
>>
>> it is reasonable to add this check and leave SEQ validation to
>> endpoints’ local TCP state machine. So it looks good to us.
>>
>
Lidong
I just noticed you were referring to the "else if" condition above
This change had been dropped in v2 since it is not needed, here:
https://patchwork.ozlabs.org/patch/1107761/
The change in the 'if' condition check is the main algorithm change; you
might want to
compare it to the proposed change that you had sent out.
Thanks Darrell
diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.cindex
397aca1..e95d2f3 100644--- a/lib/conntrack-tcp.c+++
b/lib/conntrack-tcp.c@@ -272,16 +272,18 @@ tcp_conn_update(struct
conntrack *ct, struct conn *conn_,
int ackskew = check_ackskew ? dst->seqlo - ack : 0;
#define MAXACKWINDOW (0xffff + 1500) /* 1500 is an arbitrary fudge
factor */- if (SEQ_GEQ(src->seqhi, end)- /* Last octet
inside other's window space */- && SEQ_GEQ(seq, src->seqlo -
(dst->max_win << dws))- /* Retrans: not more than one window
back */- && (ackskew >= -MAXACKWINDOW)- /* Acking not
more than one reassembled fragment backwards */- && (ackskew <=
(MAXACKWINDOW << sws))- /* Acking not more than one window
forward */+ if (((SEQ_GEQ(src->seqhi, end)+ /* Last octet
inside other's window space */+ && SEQ_GEQ(seq, src->seqlo -
(dst->max_win << dws))+ /* Retrans: not more than one window
back */+ && ackskew >= -MAXACKWINDOW+ /* Acking not
more than one reassembled fragment backwards */+ && ackskew
<= (MAXACKWINDOW << sws))+ || conntrack_get_tcp_liberal(ct))+
/* Acking not more than one window forward */
&& ((tcp_flags & TCP_RST) == 0 || orig_seq == src->seqlo-
|| (orig_seq == src->seqlo + 1) || (orig_seq + 1 ==
src->seqlo))) {+ || (orig_seq == src->seqlo + 1)+
|| (orig_seq + 1 == src->seqlo))) {
/* Require an exact/+1 sequence match on resets when possible */
/* update max window */
>
>>
>
> Thanks Lidong
>
> The algorithm I used adheres to the liberal mode definition.
>
> For future reference, another important aspect is test support.
>
> Darrell
>
>
>>
>>
>> BR,
>>
>> Lidong
>>
>>
>>
>> *发件人:* Darrell Ball <dlu998 at gmail.com>
>> *发送时间:* 2019年5月30日 14:51
>> *收件人:* 姜立东 <jianglidong3 at jd.com>
>> *抄送:* dev at openvswitch.org; 王志克 <wangzhike at jd.com>
>> *主题:* Re: [ovs-dev] [PATCH] conntrack: add tcp_in_liberal option in
>> userspace conntrack
>>
>>
>>
>> Hi Lidong/Zhike
>>
>>
>>
>> I sent an alternative implementation:
>> https://mail.openvswitch.org/pipermail/ovs-dev/2019-May/359364.html
>>
>>
>>
>> Pls take a look and check if it meets your requirements.
>>
>>
>>
>> Thanks Darrell
>>
>>
>>
>> On Fri, May 24, 2019 at 10:11 AM Darrell Ball <dlu998 at gmail.com> wrote:
>>
>> Thanks for the patch Lidong/Zhike
>>
>>
>>
>> I took an initial look and will send a response next week.
>>
>>
>>
>> Darrell
>>
>>
>>
>> On Tue, May 21, 2019 at 8:53 PM 姜立东 <jianglidong3 at jd.com> wrote:
>>
>> From 3ce112684921bca74839e109fda91848aa024a54 Mon Sep 17 00:00:00 2001
>> From: Jiang Lidong <jianglidong3 at jd.com>
>> Date: Wed, 22 May 2019 11:21:34 +0800
>> Subject: [PATCH] conntrack: add tcp_in_liberal option in userspace
>> conntrack
>>
>> Adding similar cp_in_liberal option in userspace conntrack as
>> kernel conntrack does to skip seq check on tcp connection.
>> It prevents packet is marked as INVALID by stable seq
>> info in conntrack connection.
>>
>> This option can help to make traffic survive in hardware
>> offloading cases, especially when traffic is being moved
>> back to software path from hardware forwarding engine.
>>
>> Signed-off-by: Lidong Jiang <jianglidong3 at jd.com>
>> Signed-off-by: Zhike Wang <wangzhike at jd.com>
>>
>> ---
>> lib/conntrack-private.h | 2 ++
>> lib/conntrack-tcp.c | 5 +++--
>> lib/conntrack.c | 6 ++++++
>> lib/conntrack.h | 4 +++-
>> lib/dpif-netdev.c | 6 ++++++
>> 5 files changed, 20 insertions(+), 3 deletions(-)
>>
>> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
>> index 51b7d7f..9bc99cd 100644
>> --- a/lib/conntrack-private.h
>> +++ b/lib/conntrack-private.h
>> @@ -172,6 +172,8 @@ struct conntrack {
>> /* Fragmentation handling context. */
>> struct ipf *ipf;
>> +
>> + bool tcp_be_liberal;
>> };
>> /* Lock acquisition order:
>> diff --git a/lib/conntrack-tcp.c b/lib/conntrack-tcp.c
>> index 397aca1..61abafb 100644
>> --- a/lib/conntrack-tcp.c
>> +++ b/lib/conntrack-tcp.c
>> @@ -272,7 +272,7 @@ tcp_conn_update(struct conntrack *ct, struct conn
>> *conn_,
>> int ackskew = check_ackskew ? dst->seqlo - ack : 0;
>> #define MAXACKWINDOW (0xffff + 1500) /* 1500 is an arbitrary fudge
>> factor */
>> - if (SEQ_GEQ(src->seqhi, end)
>> + if ((SEQ_GEQ(src->seqhi, end)
>> /* Last octet inside other's window space */
>> && SEQ_GEQ(seq, src->seqlo - (dst->max_win << dws))
>> /* Retrans: not more than one window back */
>> @@ -281,7 +281,8 @@ tcp_conn_update(struct conntrack *ct, struct conn
>> *conn_,
>> && (ackskew <= (MAXACKWINDOW << sws))
>> /* Acking not more than one window forward */
>> && ((tcp_flags & TCP_RST) == 0 || orig_seq == src->seqlo
>> - || (orig_seq == src->seqlo + 1) || (orig_seq + 1 ==
>> src->seqlo))) {
>> + || (orig_seq == src->seqlo + 1) || (orig_seq + 1 ==
>> src->seqlo)))
>> + || (ct->tcp_be_liberal)) {
>> /* Require an exact/+1 sequence match on resets when possible */
>> /* update max window */
>> diff --git a/lib/conntrack.c b/lib/conntrack.c
>> index 6711f5e..bd92710 100644
>> --- a/lib/conntrack.c
>> +++ b/lib/conntrack.c
>> @@ -2282,6 +2282,12 @@ conntrack_ipf_ctx(struct conntrack *ct)
>> return ct->ipf;
>> }
>> +void
>> +conntrack_set_tcp_be_liberal(struct conntrack *ct, bool enabled)
>> +{
>> + ct->tcp_be_liberal = enabled;
>> +}
>> +
>> int
>> conntrack_dump_start(struct conntrack *ct, struct conntrack_dump *dump,
>> const uint16_t *pzone, int *ptot_bkts)
>> diff --git a/lib/conntrack.h b/lib/conntrack.h
>> index 2012150..b8d799d 100644
>> --- a/lib/conntrack.h
>> +++ b/lib/conntrack.h
>> @@ -119,5 +119,7 @@ int conntrack_set_maxconns(struct conntrack *ct,
>> uint32_t maxconns);
>> int conntrack_get_maxconns(struct conntrack *ct, uint32_t *maxconns);
>> int conntrack_get_nconns(struct conntrack *ct, uint32_t *nconns);
>> struct ipf *conntrack_ipf_ctx(struct conntrack *ct);
>> -
>> +
>> +void conntrack_set_tcp_be_liberal(struct conntrack *ct, bool enabled);
>> +
>> #endif /* conntrack.h */
>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
>> index 5a6f2ab..ae6a18e 100644
>> --- a/lib/dpif-netdev.c
>> +++ b/lib/dpif-netdev.c
>> @@ -3823,6 +3823,12 @@ dpif_netdev_set_config(struct dpif *dpif, const
>> struct smap *other_config)
>> uint32_t tx_flush_interval, cur_tx_flush_interval;
>> uint64_t rebalance_intvl;
>> + bool tcp_be_liberal = smap_get_bool(other_config,
>> + "conntrack_tcp_be_liberal",
>> + false);
>> +
>> + conntrack_set_tcp_be_liberal(&dp->conntrack, tcp_be_liberal);
>> +
>> tx_flush_interval = smap_get_int(other_config, "tx-flush-interval",
>> DEFAULT_TX_FLUSH_INTERVAL);
>> atomic_read_relaxed(&dp->tx_flush_interval, &cur_tx_flush_interval);
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev at openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>
>>
More information about the dev
mailing list