On Fri, May 31, 2019 at 2:42 AM 姜立东 <[email protected]> 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.
>
>
>
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 <[email protected]>
> *发送时间:* 2019年5月30日 14:51
> *收件人:* 姜立东 <[email protected]>
> *抄送:* [email protected]; 王志克 <[email protected]>
> *主题:* 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 <[email protected]> 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 姜立东 <[email protected]> wrote:
>
> From 3ce112684921bca74839e109fda91848aa024a54 Mon Sep 17 00:00:00 2001
> From: Jiang Lidong <[email protected]>
> 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 <[email protected]>
> Signed-off-by: Zhike Wang <[email protected]>
>
> ---
> 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
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
