[PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence

[Leo Xu (Networking SW)]

Hi Thomas,

Thanks for those comments.

PSB

> -----Original Message-----
> From: Thomas Monjalon <thomas at monjalon.net>
> Sent: Wednesday, January 18, 2023 5:31 PM
> To: Leo Xu (Networking SW) <yongquanx at nvidia.com>
> Cc: dev at dpdk.org; Bing Zhao <bingz at nvidia.com>; Ori Kam
> <orika at nvidia.com>; Aman Singh <aman.deep.singh at intel.com>; Yuying
> Zhang <yuying.zhang at intel.com>; Ferruh Yigit <ferruh.yigit at amd.com>;
> Andrew Rybchenko <andrew.rybchenko at oktetlabs.ru>; Olivier Matz
> <olivier.matz at 6wind.com>; david.marchand at redhat.com
> Subject: Re: [PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence
> 
> External email: Use caution opening links or attachments
> 
> 
> 20/12/2022 08:44, Leo Xu:
> > +* **Added rte_flow support for matching ICMPv6 ID and sequence
> > +fields.**
> > +
> > +  Added ``icmp6_echo`` item in rte_flow to support ID and sequence
> > + matching in ICMPv6 echo request/reply packets.
> 
> Easier to read:
> "
> Added flow items to match ICMPv6 echo request and reply packets.
> Matching patterns can include ICMP identifier and sequence numbers.
> "

Thanks for that good sentence, will update accordingly.

> 
> > +     /**
> > +      * Matches an ICMPv6 echo request.
> > +      *
> > +      * See struct rte_flow_item_icmp6_echo.
> > +      */
> > +     RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST,
> > +
> > +     /**
> > +      * Matches an ICMPv6 echo reply.
> > +      *
> > +      * See struct rte_flow_item_icmp6_echo.
> > +      */
> > +     RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY,
> 
> It is better to use @see doxygen syntax.

Ok, thanks.

> 
> > +/**
> > + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REQUEST
> > + * RTE_FLOW_ITEM_TYPE_ICMP6_ECHO_REPLY
> > + *
> > + * Matches an ICMPv6 echo request or reply.
> > + */
> > +struct rte_flow_item_icmp6_echo {
> > +     struct rte_icmp6_echo echo;
> > +};
> 
> Other items are defined with "hdr" as first field (instead of the name "echo"
> here).
> 
> > --- a/lib/net/meson.build
> > +++ b/lib/net/meson.build
> > @@ -22,6 +22,7 @@ headers = files(
> >          'rte_geneve.h',
> >          'rte_l2tpv2.h',
> >          'rte_ppp.h',
> > +        'rte_icmp6.h',
> >  )
> 
> Please insert it after rte_icmp.h.

Will merge the rte_icmp6.h into rte_icmp.h, then no rte_icmp6.h any more.

> 
> > +#ifndef _RTE_ICMP6_H_
> > +#define _RTE_ICMP6_H_
> 
> No need of underscores at the beginning and end, it is not reserved.
> 

Will merge the rte_icmp6.h into rte_icmp.h, then no rte_icmp6.h any more.

> [...]
> > +/**
> > + * ICMP6 header
> > + */
> > +struct rte_icmp6_hdr {
> > +     uint8_t type;
> > +     uint8_t code;
> > +     rte_be16_t checksum;
> > +} __rte_packed;
> > +
> > +/**
> > + * ICMP6 echo
> > + */
> > +struct rte_icmp6_echo {
> > +     struct rte_icmp6_hdr hdr;
> > +     rte_be16_t identifier;
> > +     rte_be16_t sequence;
> > +} __rte_packed;
> 
> It is exactly the same as struct rte_icmp_hdr.
> Why not reuse it?
> Maybe introduce struct rte_icmp_base_hdr and define rte_icmp_echo_hdr as
> rte_icmp_hdr?

Hi Thomas,
Looks like, using rte_icmp_hdr as base header for both icmp and icmpv6 is not that good. 
since, rte_icmp_hdr default their headers always having id and sequence fields, which is not applicable for most other icmp6/icmp types packets.

I may suggest to keep icmp and icmp6 structures independent against each other, because, looks like these two protocols definitions do not share common base.

> 
> > +/* ICMP6 packet types */
> > +#define RTE_ICMP6_ECHO_REQUEST 128
> > +#define RTE_ICMP6_ECHO_REPLY   129
> 
> Can we avoid adding this file and add only these defines to rte_icmp.h?
> 
Yes, good idea. rte_ip.h does not have independent v6 header file either.
We should not create v6 header file for icmp.