| Message ID | 1573204092-10124-1-git-send-email-wangzk320@163.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [ovs-dev] conntrack: Fix tcp payload length in case multi-segments. | expand |
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 >
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 */
> 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.
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 >
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 */
Signed-off-by: Zhike Wang <wangzk320@163.com> --- lib/conntrack-private.h | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-)