From: Thomas Monjalon <thomas@monjalon.net>
To: "Leo Xu (Networking SW)" <yongquanx@nvidia.com>
Cc: "dev@dpdk.org" <dev@dpdk.org>, Bing Zhao <bingz@nvidia.com>,
Ori Kam <orika@nvidia.com>,
Aman Singh <aman.deep.singh@intel.com>,
Yuying Zhang <yuying.zhang@intel.com>,
Ferruh Yigit <ferruh.yigit@amd.com>,
Andrew Rybchenko <andrew.rybchenko@oktetlabs.ru>,
Olivier Matz <olivier.matz@6wind.com>,
"david.marchand@redhat.com" <david.marchand@redhat.com>
Subject: Re: [PATCH v2 1/3] ethdev: add ICMPv6 ID and sequence
Date: Thu, 02 Feb 2023 22:23:25 +0100 [thread overview]
Message-ID: <1991609.QkHrqEjB74@thomas> (raw)
In-Reply-To: <CH2PR12MB42481A77C463BBA7B724CB97C4D69@CH2PR12MB4248.namprd12.prod.outlook.com>
02/02/2023 19:33, Leo Xu (Networking SW):
> > 31/01/2023 07:53, Leo Xu (Networking SW):
> > > From: Thomas Monjalon <thomas@monjalon.net>
> > > > 20/12/2022 08:44, Leo Xu:
> > > > > +/**
> > > > > + * 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.
> >
> > What about introducing rte_icmp_base_hdr?
> > We should try to avoid duplicating things.
> >
>
> You mean introduce rte_icmp_base_hdr like following?
> struct rte_icmp_base_hdr {
> uint8_t icmp_type;
> uint8_t icmp_code;
> rte_be16_t icmp_cksum;
> } __rte_packed;
>
> And change the existing rte_icmp_hdr to be:
> struct rte_icmp_hdr {
> rte_icmp_base_hdr bash_hdr;
> rte_be16_t icmp_ident;
> rte_be16_t icmp_seq_nb;
> } __rte_packed;
> #define rte_icmp6_echo struct rte_icmp_hdr;
>
> If it is, there will be some compatibilities issues, since we changed existing structure.
>
> Or, maybe I'm missing something.
> Would you help to give more details about above comment?
Currently we have this:
struct rte_icmp_hdr {
uint8_t icmp_type; /* ICMP packet type. */
uint8_t icmp_code; /* ICMP packet code. */
rte_be16_t icmp_cksum; /* ICMP packet checksum. */
rte_be16_t icmp_ident; /* ICMP packet identifier. */
rte_be16_t icmp_seq_nb; /* ICMP packet sequence number. */
} __rte_packed;
I agree we can move some fields in a base struct,
it would change the API.
We could manage with a union, but we would lose the benefit.
It looks like we need to keep rte_icmp_hdr as is.
So we need to duplicate and define new structs.
What about removing the "6" from the new structs,
so it would apply both to IPv4 and IPv6?
struct rte_icmp_base_hdr {
uint8_t type;
uint8_t code;
rte_be16_t checksum;
} __rte_packed;
struct rte_icmp_echo_hdr {
struct rte_icmp_base_hdr base;
rte_be16_t identifier;
rte_be16_t sequence;
} __rte_packed;
next prev parent reply other threads:[~2023-02-02 21:23 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-12 8:59 [PATCH 0/3] support match icmpv6 id " Leo Xu
2022-12-12 8:59 ` [PATCH 1/3] ethdev: add ICMPv6 " Leo Xu
2022-12-12 8:59 ` [PATCH 2/3] net/mlx5: add ICMPv6 id and sequence match support Leo Xu
2022-12-12 8:59 ` [PATCH 3/3] net/mlx5/hws: " Leo Xu
2022-12-20 7:44 ` [PATCH v2 0/3] support match icmpv6 ID and sequence Leo Xu
2022-12-20 7:44 ` [PATCH v2 1/3] ethdev: add ICMPv6 " Leo Xu
2023-01-03 8:17 ` Ori Kam
2023-01-18 9:30 ` Thomas Monjalon
2023-01-31 6:53 ` Leo Xu (Networking SW)
2023-02-01 9:56 ` Thomas Monjalon
2023-02-02 18:33 ` Leo Xu (Networking SW)
2023-02-02 21:23 ` Thomas Monjalon [this message]
2023-02-03 2:56 ` Leo Xu (Networking SW)
2023-01-26 10:45 ` Ferruh Yigit
2023-01-31 3:58 ` Leo Xu (Networking SW)
2022-12-20 7:44 ` [PATCH v2 2/3] net/mlx5: add ICMPv6 ID and sequence match support Leo Xu
2023-01-18 8:55 ` Thomas Monjalon
2023-01-31 6:57 ` Leo Xu (Networking SW)
2022-12-20 7:44 ` [PATCH v2 3/3] net/mlx5/hws: " Leo Xu
2023-01-18 8:58 ` Thomas Monjalon
2023-01-31 6:56 ` Leo Xu (Networking SW)
2023-01-26 10:47 ` [PATCH v2 0/3] support match icmpv6 ID and sequence Ferruh Yigit
2023-01-31 3:54 ` Leo Xu (Networking SW)
2023-02-05 13:41 ` [PATCH v3 " Leo Xu
2023-02-05 13:41 ` [PATCH v3 1/3] ethdev: add ICMPv6 " Leo Xu
2023-02-05 13:41 ` [PATCH v3 2/3] net/mlx5: add ICMPv6 ID and sequence match support Leo Xu
2023-02-07 13:48 ` Slava Ovsiienko
2023-02-05 13:41 ` [PATCH v3 3/3] net/mlx5/hws: " Leo Xu
2023-02-07 13:05 ` Alex Vesker
2023-02-07 13:49 ` Slava Ovsiienko
2023-02-09 13:04 ` [PATCH v3 0/3] support match icmpv6 ID and sequence Ferruh Yigit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1991609.QkHrqEjB74@thomas \
--to=thomas@monjalon.net \
--cc=aman.deep.singh@intel.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bingz@nvidia.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=ferruh.yigit@amd.com \
--cc=olivier.matz@6wind.com \
--cc=orika@nvidia.com \
--cc=yongquanx@nvidia.com \
--cc=yuying.zhang@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).