Re: [PATCH v3] gro : ipv6 changes to support GRO for TCP/ipv6

[kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 13020 bytes --]

On Tue, Jun 6, 2023 at 10:05 AM Hu, Jiayu <jiayu.hu@intel.com> wrote:

> Hi Kumara,
>
> The v3 patch is not complete and it seems to be a patch based on v2.
> In addition, did you test the code for tcp4 and tcp6 after your change?
>
>> Will make sure that next patch contains the entire diff.
>>
> For others, please see replies inline.
>
> Thanks,
> Jiayu
>
> > -----Original Message-----
> > From: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > Sent: Friday, June 2, 2023 2:34 PM
> > To: Hu, Jiayu <jiayu.hu@intel.com>
> > Cc: dev@dpdk.org; Kumara Parameshwaran
> > <kumaraparamesh92@gmail.com>
> > Subject: [PATCH v3] gro : ipv6 changes to support GRO for TCP/ipv6
> >
> > The patch adds GRO support for TCP/ipv6 packets. This does not include
> the
> > support for vxlan, udp ipv6 packets.
> >
> > Signed-off-by: Kumara Parameshwaran <kumaraparamesh92@gmail.com>
> > ---
> > v1:
> >       * Changes to support GRO for TCP/ipv6 packets. This does not
> > include
> >         vxlan changes.
> >       * The GRO is performed only for ipv6 packets that does not contain
> >        extension headers.
> >       * The logic for the TCP coalescing remains the same, in ipv6 header
> >         the source address, destination address, flow label, version
> fields
> >         are expected to be the same.
> >       * Re-organised the code to reuse certain tcp functions for both
> ipv4
> > and
> >         ipv6 flows.
> > v2:
> >       * Fix comments in gro_tcp6.h header file.
> >
> > v3:
> >       * Adderess review comments to fix code duplication for v4 and v6
> >
> >  lib/gro/gro_tcp.c        | 160 ++++++++++++++++++++++++
> >  lib/gro/gro_tcp.h        |  63 ++++++++++
> >  lib/gro/gro_tcp4.c       | 255 ++++++++++++---------------------------
> >  lib/gro/gro_tcp4.h       |  18 +--
> >  lib/gro/gro_tcp6.c       | 243 ++++++++++---------------------------
> >  lib/gro/gro_tcp6.h       |  31 +++--
> >  lib/gro/gro_vxlan_tcp4.c |  18 +--
> >  lib/gro/meson.build      |   1 +
> >  8 files changed, 396 insertions(+), 393 deletions(-)  create mode 100644
> > lib/gro/gro_tcp.c
> >
> > diff --git a/lib/gro/gro_tcp.c b/lib/gro/gro_tcp.c new file mode 100644
> index
> > 0000000000..6a5aaada58
> > --- /dev/null
> > +++ b/lib/gro/gro_tcp.c
> > @@ -0,0 +1,160 @@
> > +/* SPDX-License-Identifier: BSD-3-Clause
> > + * Copyright(c) 2017 Intel Corporation
> > + */
> > +#include <rte_malloc.h>
> > +#include <rte_mbuf.h>
> > +#include <rte_ethdev.h>
> > +
> > +#include "gro_tcp.h"
> > +
> > +static inline uint32_t
> > +find_an_empty_item(struct gro_tcp_item *items,
> > +     uint32_t table_size)
> > +{
> > +     uint32_t i;
> > +
> > +     for (i = 0; i < table_size; i++)
> > +             if (items[i].firstseg == NULL)
> > +                     return i;
> > +     return INVALID_ARRAY_INDEX;
> > +}
> > +
> > +uint32_t
> > +insert_new_tcp_item(struct rte_mbuf *pkt,
> > +             struct gro_tcp_item *items,
> > +             uint32_t *item_num,
> > +             uint32_t table_size,
> > +             uint64_t start_time,
> > +             uint32_t prev_idx,
> > +             uint32_t sent_seq,
> > +             uint16_t ip_id,
> > +             uint8_t is_atomic)
> > +{
> > +     uint32_t item_idx;
> > +
> > +     item_idx = find_an_empty_item(items, table_size);
> > +     if (item_idx == INVALID_ARRAY_INDEX)
> > +             return INVALID_ARRAY_INDEX;
> > +
> > +     items[item_idx].firstseg = pkt;
> > +     items[item_idx].lastseg = rte_pktmbuf_lastseg(pkt);
> > +     items[item_idx].start_time = start_time;
> > +     items[item_idx].next_pkt_idx = INVALID_ARRAY_INDEX;
> > +     items[item_idx].sent_seq = sent_seq;
> > +     items[item_idx].ip_id = ip_id;
> > +     items[item_idx].nb_merged = 1;
> > +     items[item_idx].is_atomic = is_atomic;
> > +     (*item_num) += 1;
> > +
> > +     /* if the previous packet exists, chain them together. */
> > +     if (prev_idx != INVALID_ARRAY_INDEX) {
> > +             items[item_idx].next_pkt_idx =
> > +                     items[prev_idx].next_pkt_idx;
> > +             items[prev_idx].next_pkt_idx = item_idx;
> > +     }
> > +
> > +     return item_idx;
> > +}
> > +
> > +uint32_t
> > +delete_tcp_item(struct gro_tcp_item *items, uint32_t item_idx,
> > +             uint32_t *item_num,
> > +             uint32_t prev_item_idx)
> > +{
> > +     uint32_t next_idx = items[item_idx].next_pkt_idx;
> > +
> > +     /* NULL indicates an empty item */
> > +     items[item_idx].firstseg = NULL;
> > +     (*item_num) -= 1;
> > +     if (prev_item_idx != INVALID_ARRAY_INDEX)
> > +             items[prev_item_idx].next_pkt_idx = next_idx;
> > +
> > +     return next_idx;
> > +}
> > +
> > +int32_t
> > +gro_tcp_reassemble(struct rte_mbuf *pkt,
> > +     void *tbl,
> > +     void *key,
> > +     int32_t tcp_dl,
> > +     struct gro_tcp_flow_ops *ops,
> > +     struct gro_tcp_item *items,
> > +     uint32_t *item_num,
> > +     uint32_t table_size,
> > +     uint16_t ip_id,
> > +     uint8_t is_atomic,
> > +     uint64_t start_time)
>
> In general, TCP4 and TCP6 share struct gro_tcp_item and have private flow
> structures,
> i.e., struct gro_tcp4/6_flow, and I like this abstraction. IMO, the code
> processing
> struct gro_tcp_item should be implemented as common functions shared by
> gro_tcp4.c and gro_tcp6.c. The code processing struct gro_tcp4/6_flow is
> tcp4 and
> tcp6 dependent and no need to abstract and share.
>
> In gro_tcp_reassemble(), it uses callback functions defined in struct
> gro_tcp_flow_ops
> to provide the different operations on struct gro_tcp4/6_flow. I don't
> think it's necessary
> for abstraction purpose as gro_tcp4/6_flows_ops implementations are alike
> and it also
> introduces extra cost on function calls.
>
>> Sure, would make the changes accordingly.
>>
>

> The gro_tcp_reassemble() has two parts: the common part to process struct
> gro_tcp_item
> and the private part to process struct gro_tcp4/6_flow. I think a better
> way is to remove
> gro_tcp_reassemble() and struct gro_tcp_flow_ops, and implement the common
> part as
> an internal function in gro_tcp.c/gro_tcp.h, and make both
> gro_tcp4/6_reassemble()
> implement own private part and invoke the common function to process
> struct gro_tcp_item.
> With this change, there is no callback cost anymore and tcp4/6 can share
> the common function.
>
>> Sure, got it.
>>
>> > +{
> > +     uint32_t item_idx;
> > +     uint32_t cur_idx;
> > +     uint32_t prev_idx;
> > +     struct rte_tcp_hdr *tcp_hdr;
> > +     int cmp;
> > +     uint32_t sent_seq;
> > +
> > +     tcp_hdr = rte_pktmbuf_mtod_offset(pkt, struct rte_tcp_hdr *, pkt-
> > >l2_len + pkt->l3_len);
> > +     /*
> > +      * Don't process the packet which has FIN, SYN, RST, PSH, URG, ECE
> > +      * or CWR set.
> > +      */
> > +     if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
> > +             return -1;
> > +     sent_seq = rte_be_to_cpu_32(tcp_hdr->sent_seq);
> > +
> > +     ops->tcp_flow_key_init(key, tcp_hdr);
> > +
> > +     item_idx = ops->tcp_flow_lookup(tbl, key);
> > +     if (item_idx == INVALID_ARRAY_INDEX) {
> > +             item_idx = insert_new_tcp_item(pkt, items, item_num,
> > table_size, start_time,
> > +
> >       INVALID_ARRAY_INDEX, sent_seq, ip_id,
> > +                                                     is_atomic);
> > +             if (item_idx == INVALID_ARRAY_INDEX)
> > +                     return -1;
> > +             if (ops->tcp_flow_insert(tbl, key, item_idx) ==
> > +                     INVALID_ARRAY_INDEX) {
> > +                     /*
> > +                      * Fail to insert a new flow, so delete the
> > +                      * stored packet.
> > +                      */
> > +                     delete_tcp_item(items, item_idx, item_num,
> > INVALID_ARRAY_INDEX);
> > +                     return -1;
> > +             }
> > +             return 0;
> > +     }
> > +     /*
> > +      * Check all packets in the flow and try to find a neighbor for
> > +      * the input packet.
> > +      */
> > +     cur_idx = item_idx;
> > +     prev_idx = cur_idx;
> > +     do {
> > +             cmp = check_seq_option(&items[cur_idx], tcp_hdr,
> > +                             sent_seq, ip_id, pkt->l4_len, tcp_dl, 0,
> > +                             is_atomic);
> > +             if (cmp) {
> > +                     if (merge_two_tcp_packets(&items[cur_idx],
> > +                                             pkt, cmp, sent_seq, ip_id,
> 0))
> > +                             return 1;
> > +                     /*
> > +                      * Fail to merge the two packets, as the packet
> > +                      * length is greater than the max value. Store
> > +                      * the packet into the flow.
> > +                      */
> > +                     if (insert_new_tcp_item(pkt, items, item_num,
> > table_size, start_time, cur_idx,
> > +                                             sent_seq, ip_id,
> is_atomic) ==
> > +                                     INVALID_ARRAY_INDEX)
> > +                             return -1;
> > +                     return 0;
> > +             }
> > +             prev_idx = cur_idx;
> > +             cur_idx = items[cur_idx].next_pkt_idx;
> > +     } while (cur_idx != INVALID_ARRAY_INDEX);
> > +
> > +     /* Fail to find a neighbor, so store the packet into the flow. */
> > +     if (insert_new_tcp_item(pkt, items, item_num, table_size,
> start_time,
> > prev_idx, sent_seq,
> > +                             ip_id, is_atomic) == INVALID_ARRAY_INDEX)
> > +             return -1;
> > +
> > +     return 0;
> > +
> > +}
> > diff --git a/lib/gro/gro_tcp.h b/lib/gro/gro_tcp.h index
> > c5d248a022..202f485c18 100644
> > --- a/lib/gro/gro_tcp.h
> > +++ b/lib/gro/gro_tcp.h
> > @@ -1,6 +1,8 @@
> >  #ifndef _GRO_TCP_H_
> >  #define _GRO_TCP_H_
> >
> > +#define INVALID_ARRAY_INDEX 0xffffffffUL
> > +
> >  #include <rte_tcp.h>
> >
> >  /*
> > @@ -14,6 +16,31 @@
> >  #define INVALID_TCP_HDRLEN(len) \
> >       (((len) < sizeof(struct rte_tcp_hdr)) || ((len) > MAX_TCP_HLEN))
> >
> > +struct gro_tcp_flow {
> > +     struct rte_ether_addr eth_saddr;
> > +     struct rte_ether_addr eth_daddr;
> > +     uint32_t recv_ack;
> > +     uint16_t src_port;
> > +     uint16_t dst_port;
> > +};
> > +
> > +#define ASSIGN_TCP_FLOW_KEY(k1, k2) \
> > +     rte_ether_addr_copy(&(k1->eth_saddr), &(k2->eth_saddr)); \
> > +     rte_ether_addr_copy(&(k1->eth_daddr), &(k2->eth_daddr)); \
> > +     k2->recv_ack = k1->recv_ack; \
> > +     k2->src_port = k1->src_port; \
> > +     k2->dst_port = k1->dst_port;
>
> For multiline macro, it's better to use do{...}while(0) to avoid
> unexpected errors
> in the future.
>
> > +
> > +typedef uint32_t (*gro_tcp_flow_lookup)(void *table, void *key);
> > +typedef uint32_t (*gro_tcp_flow_insert)(void *table, void *key,
> > +uint32_t item_idx); typedef void (*gro_tcp_flow_key_init)(void *key,
> > +struct rte_tcp_hdr *tcp_hdr);
> > +
> > +struct gro_tcp_flow_ops {
> > +     gro_tcp_flow_lookup tcp_flow_lookup;
> > +     gro_tcp_flow_insert tcp_flow_insert;
> > +     gro_tcp_flow_key_init tcp_flow_key_init; };
> > +
> >  struct gro_tcp_item {
> >       /*
> >        * The first MBUF segment of the packet. If the value @@ -44,6
> > +71,36 @@ struct gro_tcp_item {
> >       uint8_t is_atomic;
> >  };
> >
> > +uint32_t
> > +insert_new_tcp_item(struct rte_mbuf *pkt,
> > +             struct gro_tcp_item *items,
> > +             uint32_t *item_num,
> > +             uint32_t table_size,
> > +             uint64_t start_time,
> > +             uint32_t prev_idx,
> > +             uint32_t sent_seq,
> > +             uint16_t ip_id,
> > +             uint8_t is_atomic);
> > +
> > +uint32_t
> > +delete_tcp_item(struct gro_tcp_item *items,
> > +             uint32_t item_idx,
> > +             uint32_t *item_num,
> > +             uint32_t prev_item_idx);
> > +
> > +int32_t
> > +gro_tcp_reassemble(struct rte_mbuf *pkt,
> > +     void *tbl,
> > +     void *key,
> > +     int32_t tcp_dl,
> > +     struct gro_tcp_flow_ops *ops,
> > +     struct gro_tcp_item *items,
> > +     uint32_t *item_num,
> > +     uint32_t table_size,
> > +     uint16_t ip_id,
> > +     uint8_t is_atomic,
> > +     uint64_t start_time);
> > +
> >  /*
> >   * Merge two TCP packets without updating checksums.
> >   * If cmp is larger than 0, append the new packet to the @@ -152,4
> +209,10
> > @@ check_seq_option(struct gro_tcp_item *item,
> >       return 0;
> >  }
> >
> > +static inline int
> > +is_same_tcp_flow(struct gro_tcp_flow *k1, struct gro_tcp_flow *k2) {
> > +     return (!memcmp(k1, k2, sizeof(struct gro_tcp_flow))); }
> > +
> >  #endif
>
>

[-- Attachment #2: Type: text/html, Size: 17276 bytes --]