[ovs-dev] conntrack: Fix tcp payload length in case multi-segments.
diff mbox series

Message ID 1573204092-10124-1-git-send-email-wangzk320@163.com
State New
Headers show
Series
  • [ovs-dev] conntrack: Fix tcp payload length in case multi-segments.
Related show

Commit Message

Zhike Wang Nov. 8, 2019, 9:08 a.m. UTC
Signed-off-by: Zhike Wang <wangzk320@163.com>
---
 lib/conntrack-private.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

Comments

Darrell Ball Nov. 9, 2019, 12:11 a.m. UTC | #1
Thanks for the patch

Would you mind describing the use case that this patch is aiming to support
?

On Fri, Nov 8, 2019 at 1:23 AM Zhike Wang <wangzk320@163.com> wrote:

> Signed-off-by: Zhike Wang <wangzk320@163.com>
> ---
>  lib/conntrack-private.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 590f139..1d21f6e 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -233,13 +233,17 @@ conn_update_expiration(struct conntrack *ct, struct
> conn *conn,
>  static inline uint32_t
>  tcp_payload_length(struct dp_packet *pkt)
>  {
> -    const char *tcp_payload = dp_packet_get_tcp_payload(pkt);
> -    if (tcp_payload) {
> -        return ((char *) dp_packet_tail(pkt) - dp_packet_l2_pad_size(pkt)
> -                - tcp_payload);
> -    } else {
> -        return 0;
> +    size_t l4_size = dp_packet_l4_size(pkt);
> +
> +    if (OVS_LIKELY(l4_size >= TCP_HEADER_LEN)) {
> +        struct tcp_header *tcp = dp_packet_l4(pkt);
> +        int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
> +
> +        if (OVS_LIKELY(tcp_len >= TCP_HEADER_LEN && tcp_len <= l4_size)) {
> +            return (l4_size - tcp_len);
> +        }
>

Maybe I missed something, but it looks like the same calculation is arrived
at.


>      }
> +    return 0;
>  }
>
>  #endif /* conntrack-private.h */
> --
> 1.8.3.1
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
Sathya Perla via dev Nov. 11, 2019, 5:50 a.m. UTC | #2
Hi Darrell,

In TSO case, the packet may use multi-segments mbuf, and I do not think we need to make it linearal. In this case, we can NOT use pointer to calculate the tcp length.

Br,

Zhike Wang 
JDCloud, Product Development, IaaS   
------------------------------------------------------------------------------------------------
Mobile/+86 13466719566
E- mail/wangzhike@jd.com
Address/5F Building A,North-Star Century Center,8 Beichen West Street,Chaoyang District Beijing
Https://JDCloud.com
------------------------------------------------------------------------------------------------


From: Darrell Ball [mailto:dlu998@gmail.com] 
Sent: Saturday, November 09, 2019 8:12 AM
To: Zhike Wang
Cc: ovs dev; 王志克
Subject: Re: [ovs-dev] [PATCH] conntrack: Fix tcp payload length in case multi-segments.

Thanks for the patch 

Would you mind describing the use case that this patch is aiming to support ?

On Fri, Nov 8, 2019 at 1:23 AM Zhike Wang <wangzk320@163.com> wrote:
Signed-off-by: Zhike Wang <wangzk320@163.com>
---
 lib/conntrack-private.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 590f139..1d21f6e 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -233,13 +233,17 @@ conn_update_expiration(struct conntrack *ct, struct conn *conn,
 static inline uint32_t
 tcp_payload_length(struct dp_packet *pkt)
 {
-    const char *tcp_payload = dp_packet_get_tcp_payload(pkt);
-    if (tcp_payload) {
-        return ((char *) dp_packet_tail(pkt) - dp_packet_l2_pad_size(pkt)
-                - tcp_payload);
-    } else {
-        return 0;
+    size_t l4_size = dp_packet_l4_size(pkt);
+
+    if (OVS_LIKELY(l4_size >= TCP_HEADER_LEN)) {
+        struct tcp_header *tcp = dp_packet_l4(pkt);
+        int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
+
+        if (OVS_LIKELY(tcp_len >= TCP_HEADER_LEN && tcp_len <= l4_size)) {
+            return (l4_size - tcp_len);
+        }

Maybe I missed something, but it looks like the same calculation is arrived at.
 
     }
+    return 0;
 }

 #endif /* conntrack-private.h */
Ilya Maximets Nov. 11, 2019, 1:01 p.m. UTC | #3
> Hi Darrell,
> 
> In TSO case, the packet may use multi-segments mbuf, and I do not> think we need to make it linearal. In this case, we can NOT use
> pointer to calculate the tcp length.

Hi.  Just wanted to mention that current main course for TSO enabling in
OVS seems to be using of extbuf memory in vhost.  And corresponding patches
are already accepted [1] to DPDK.  So, there will be no multi-segment mbufs.

[1] c3ff0ac70acb ("vhost: improve performance by supporting large buffer")

Best regards, Ilya Maximets.
Darrell Ball Nov. 11, 2019, 4:13 p.m. UTC | #4
Hi Zhike

Thanks for clarifying
There is presently no support for multi-segment mbufs in OVS, so the patch
would
not be needed and in general patches are only proposed at the time they are
relevant.

Also note that in the hypothetical case of multisegment mbufs, the packet
would be linearized at
this point.

Also note that this patch still relies on pointers..

As Ilya already mentioned there is no need to use multi-segment mbufs on
OVS as large buffers
can be used instead.

On Sun, Nov 10, 2019 at 9:50 PM 王志克 <wangzhike@jd.com> wrote:

> Hi Darrell,
>
> In TSO case, the packet may use multi-segments mbuf, and I do not think we
> need to make it linearal. In this case, we can NOT use pointer to calculate
> the tcp length.
>
> Br,
>
> Zhike Wang
> JDCloud, Product Development, IaaS
>
> ------------------------------------------------------------------------------------------------
> Mobile/+86 13466719566
> E- mail/wangzhike@jd.com
> Address/5F Building A,North-Star Century Center,8 Beichen West
> Street,Chaoyang District Beijing
> Https://JDCloud.com
>
> ------------------------------------------------------------------------------------------------
>
>
> From: Darrell Ball [mailto:dlu998@gmail.com]
> Sent: Saturday, November 09, 2019 8:12 AM
> To: Zhike Wang
> Cc: ovs dev; 王志克
> Subject: Re: [ovs-dev] [PATCH] conntrack: Fix tcp payload length in case
> multi-segments.
>
> Thanks for the patch
>
> Would you mind describing the use case that this patch is aiming to
> support ?
>
> On Fri, Nov 8, 2019 at 1:23 AM Zhike Wang <wangzk320@163.com> wrote:
> Signed-off-by: Zhike Wang <wangzk320@163.com>
> ---
>  lib/conntrack-private.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
>
> diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
> index 590f139..1d21f6e 100644
> --- a/lib/conntrack-private.h
> +++ b/lib/conntrack-private.h
> @@ -233,13 +233,17 @@ conn_update_expiration(struct conntrack *ct, struct
> conn *conn,
>  static inline uint32_t
>  tcp_payload_length(struct dp_packet *pkt)
>  {
> -    const char *tcp_payload = dp_packet_get_tcp_payload(pkt);
> -    if (tcp_payload) {
> -        return ((char *) dp_packet_tail(pkt) - dp_packet_l2_pad_size(pkt)
> -                - tcp_payload);
> -    } else {
> -        return 0;
> +    size_t l4_size = dp_packet_l4_size(pkt);
> +
> +    if (OVS_LIKELY(l4_size >= TCP_HEADER_LEN)) {
> +        struct tcp_header *tcp = dp_packet_l4(pkt);
> +        int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
> +
> +        if (OVS_LIKELY(tcp_len >= TCP_HEADER_LEN && tcp_len <= l4_size)) {
> +            return (l4_size - tcp_len);
> +        }
>
> Maybe I missed something, but it looks like the same calculation is
> arrived at.
>
>      }
> +    return 0;
>  }
>
>  #endif /* conntrack-private.h */
> --
> 1.8.3.1
>
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>

Patch
diff mbox series

diff --git a/lib/conntrack-private.h b/lib/conntrack-private.h
index 590f139..1d21f6e 100644
--- a/lib/conntrack-private.h
+++ b/lib/conntrack-private.h
@@ -233,13 +233,17 @@  conn_update_expiration(struct conntrack *ct, struct conn *conn,
 static inline uint32_t
 tcp_payload_length(struct dp_packet *pkt)
 {
-    const char *tcp_payload = dp_packet_get_tcp_payload(pkt);
-    if (tcp_payload) {
-        return ((char *) dp_packet_tail(pkt) - dp_packet_l2_pad_size(pkt)
-                - tcp_payload);
-    } else {
-        return 0;
+    size_t l4_size = dp_packet_l4_size(pkt);
+
+    if (OVS_LIKELY(l4_size >= TCP_HEADER_LEN)) {
+        struct tcp_header *tcp = dp_packet_l4(pkt);
+        int tcp_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
+
+        if (OVS_LIKELY(tcp_len >= TCP_HEADER_LEN && tcp_len <= l4_size)) {
+            return (l4_size - tcp_len);
+        }
     }
+    return 0;
 }
 
 #endif /* conntrack-private.h */