Re: [PATCH v11] gro: fix reordering of packets in GRO layer

[kumaraparameshwaran rathinavel <kumaraparamesh92@gmail.com>]

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

On Sun, Jan 7, 2024 at 10:50 PM Stephen Hemminger <
stephen@networkplumber.org> wrote:

> On Sun,  7 Jan 2024 16:59:20 +0530
> Kumara Parameshwaran <kumaraparamesh92@gmail.com> wrote:
>
> > +     /* Return early if the TCP flags are not handled in GRO layer */
> > +     if (tcp_hdr->tcp_flags & (~(VALID_GRO_TCP_FLAGS)))
>
> Nit, lots of extra paren here. Could be:
>         if (tcp_hdr->tcp_flags & ~VALID_GRO_TCP_FLAGS)
>
>> Done.
>>
> > +     if (find == 1) {
> > +             /*
> > +              * Any packet with additional flags like PSH,FIN should be
> processed
> > +              * and flushed immediately.
> > +              * Hence marking the start time to 0, so that the packets
> will be flushed
> > +              * immediately in timer mode.
> > +              */
> > +             if (tcp_hdr->tcp_flags & (RTE_TCP_ACK_FLAG |
> RTE_TCP_PSH_FLAG | RTE_TCP_FIN_FLAG)) {
> > +                     if (tcp_hdr->tcp_flags != RTE_TCP_ACK_FLAG)
> > +                             tbl->items[item_start_idx].start_time = 0;
> > +                     return process_tcp_item(pkt, tcp_hdr, tcp_dl,
> tbl->items,
> > +                                             tbl->flows[i].start_index,
> > +                                             &tbl->item_num,
> tbl->max_item_num,
> > +                                             ip_id, is_atomic,
> start_time);
> > +             } else {
> > +                     return -1;
> > +             }
> > +     }
>
> Reordering this conditional would keep code from being so indented.
>
>> Doing this reordering as suggested by Jiyau since the find == 1 would be
>> likely in most cases.
>>
> > -                     delete_tcp_item(tbl->items, item_idx,
> &tbl->item_num, INVALID_ARRAY_INDEX);
> > +                     delete_tcp_item(tbl->items, item_idx,
> &tbl->item_num,
> > +                             INVALID_ARRAY_INDEX);
> >                       return -1;
>
> This change is unnecessary, max line length in DPDK is 100 characters for
> readability.
>
>> Done.
>>
> >               return 0;
> > +     } else {
> > +             return -1;
> >       }
> >
> > -     return process_tcp_item(pkt, tcp_hdr, tcp_dl, tbl->items,
> tbl->flows[i].start_index,
> > -                                             &tbl->item_num,
> tbl->max_item_num,
> > -                                             ip_id, is_atomic,
> start_time);
> > +     return -1;
> >  }
>
> Since end of else and end of function both return -1, the else clause is
> unnecessary.
>
>> Done.
>>
>

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