Re: [PATCH] ip_frag: support IPv6 reassembly with extensions

[Vignesh Purushotham Srinivas <vignesh.purushotham.srinivas@ericsson.com>]

-----Original Message-----
From: Konstantin Ananyev <konstantin.ananyev@huawei.com>
To: Stephen Hemminger <stephen@networkplumber.org>,
vignesh.purushotham.srinivas@ericsson.com
<vignesh.purushotham.srinivas@ericsson.com>
Cc: konstantin.v.ananyev@yandex.ru <konstantin.v.ananyev@yandex.ru>,
dev@dpdk.org <dev@dpdk.org>
Subject: RE: [PATCH] ip_frag: support IPv6 reassembly with extensions
Date: Tue, 17 Sep 2024 17:57:59 +0000

[You don't often get email from konstantin.ananyev@huawei.com. Learn
why this is important at
https://aka.ms/LearnAboutSenderIdentification ]

> 
> On Mon, 26 Aug 2024 13:23:28 +0200
> <vignesh.purushotham.srinivas@ericsson.com> wrote:
> 
> > diff --git a/lib/ip_frag/ip_reassembly.h
> > b/lib/ip_frag/ip_reassembly.h
> > index 54afed5417..429e74f1b3 100644
> > --- a/lib/ip_frag/ip_reassembly.h
> > +++ b/lib/ip_frag/ip_reassembly.h
> > @@ -54,6 +54,8 @@ struct __rte_cache_aligned ip_frag_pkt {
> >     uint32_t total_size;                   /* expected reassembled
> > size */
> >     uint32_t frag_size;                    /* size of fragments
> > received */
> >     uint32_t last_idx;                     /* index of next entry
> > to fill */
> > +   uint32_t exts_len;                     /* length of extension
> > hdrs for first fragment */
> > +   uint8_t *next_proto;                   /* pointer of the
> > next_proto field */
> >     struct ip_frag frags[IP_MAX_FRAG_NUM]; /* fragments */
> >  };
> 
> This creates a 32 bit hole in the structure.
> Better to put next_proto after the start field.

Another alternative - use offset within the mbuf instead of pointer.

ACK

> 
> > +
> > +   while (next_proto != IPPROTO_FRAGMENT &&
> > +           num_exts < MAX_NUM_IPV6_EXTS &&
> > +           (next_proto = rte_ipv6_get_next_ext(
> > +           *last_ext, next_proto, &ext_len)) >= 0) {
> 
> I would break up this loop condition for clarity.

+ 1

ACK

> Something like:
> 
>       while (next_proto != IPPROTO_FRAGMENT && num_exts <
> MAX_NUM_IPV6_EXTS) {
>               next_proto = rte_ipv6_get_next_ext(*last_ext,
> next_proto, &ext_len);
>               if (next_proto < 0)
>                       break
> 
> Also, need a new test cases for this.

Agree, that would be good thing to add.

ACK