Hi Thomas,
Thanks for those comments.
PSB
> -----Original Message-----
> From: Thomas Monjalon <tho...@monjalon.net>
> Sent: Wednesday, January 18, 2023 5:31 PM
> To: Leo Xu (Networking SW) <yongqu...@nvidia.com>
> Cc: dev@dpdk.org; Bing Zhao <bi...@nvidia.com>; Ori Kam
> <or...@nvidia.com>; Aman Singh <aman.deep.si...@intel.com>; Yuying
> Zhang <yuying.zh...@intel.com>; Ferruh Yigit <ferruh.yi...@amd.com>;
> Andrew Rybchenko <andrew.rybche...@oktetlabs.ru>; Olivier Matz
> <olivier.m...@6wind.com>; david.march...@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.
