DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>
To: Gregory Etelson <getelson@nvidia.com>,
	Morten Brørup <mb@smartsharesystems.com>,
	"dev@dpdk.org" <dev@dpdk.org>
Cc: Matan Azrad <matan@nvidia.com>, Ori Kam <orika@nvidia.com>,
	"Raslan Darawsheh" <rasland@nvidia.com>,
	"Iremonger, Bernard" <bernard.iremonger@intel.com>,
	Olivier Matz <olivier.matz@6wind.com>
Subject: Re: [dpdk-dev] [PATCH] net: introduce IPv4 ihl and version fields
Date: Mon, 31 May 2021 09:58:39 +0000
Message-ID: <DM6PR11MB4491682F88519F3DA89F48169A3F9@DM6PR11MB4491.namprd11.prod.outlook.com> (raw)
In-Reply-To: <BY5PR12MB48340A3CD9D5133CE2D48535A5229@BY5PR12MB4834.namprd12.prod.outlook.com>



> > > > > RTE IPv4 header definition combines the `version' and `ihl'
> > > > > fields into a single structure member.
> > > > > This patch introduces dedicated structure members for both
> > > `version'
> > > > > and `ihl' IPv4 fields. Separated header fields definitions allow
> > > > > to create simplified code to match on the IHL value in a flow rule.
> > > > > The original `version_ihl' structure member is kept for backward
> > > > > compatibility.
> > > > >
> > > > > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> > > > > ---
> > > > >  app/test/test_flow_classify.c |  8 ++++----
> > > > >  lib/net/rte_ip.h              | 16 +++++++++++++++-
> > > > >  2 files changed, 19 insertions(+), 5 deletions(-)
> > > > >
> > > > > diff --git a/app/test/test_flow_classify.c
> > > > > b/app/test/test_flow_classify.c index 951606f248..4f64be5357
> > > > > 100644
> > > > > --- a/app/test/test_flow_classify.c
> > > > > +++ b/app/test/test_flow_classify.c
> > > > > @@ -95,7 +95,7 @@ static struct rte_acl_field_def
> > > > > ipv4_defs[NUM_FIELDS_IPV4] = {
> > > > >   *  dst mask 255.255.255.00 / udp src is 32 dst is 33 / end"
> > > > >   */
> > > > >  static struct rte_flow_item_ipv4 ipv4_udp_spec_1 = {
> > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_UDP, 0,
> > > > >     RTE_IPV4(2, 2, 2, 3), RTE_IPV4(2, 2, 2, 7)}  };  static const
> > > > > struct rte_flow_item_ipv4 ipv4_mask_24 = { @@ -131,7 +131,7 @@
> > > > > static struct rte_flow_item  end_item = {
> > RTE_FLOW_ITEM_TYPE_END,
> > > > >   *  dst mask 255.255.255.00 / tcp src is 16 dst is 17 / end"
> > > > >   */
> > > > >  static struct rte_flow_item_ipv4 ipv4_tcp_spec_1 = {
> > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_TCP, 0,
> > > > >     RTE_IPV4(1, 2, 3, 4), RTE_IPV4(5, 6, 7, 8)}  };
> > > > >
> > > > > @@ -150,8 +150,8 @@ static struct rte_flow_item  tcp_item_1 = {
> > > > > RTE_FLOW_ITEM_TYPE_TCP,
> > > > >   *  dst mask 255.255.255.00 / sctp src is 16 dst is 17/ end"
> > > > >   */
> > > > >  static struct rte_flow_item_ipv4 ipv4_sctp_spec_1 = {
> > > > > - { 0, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0, RTE_IPV4(11, 12, 13, 14),
> > > > > - RTE_IPV4(15, 16, 17, 18)}
> > > > > + { { .version_ihl = 0}, 0, 0, 0, 0, 0, IPPROTO_SCTP, 0,
> > > > > + RTE_IPV4(11, 12, 13, 14), RTE_IPV4(15, 16, 17, 18)}
> > > > >  };
> > > > >
> > > > >  static struct rte_flow_item_sctp sctp_spec_1 = { diff --git
> > > > > a/lib/net/rte_ip.h b/lib/net/rte_ip.h index 4b728969c1..684bb028b2
> > > > > 100644
> > > > > --- a/lib/net/rte_ip.h
> > > > > +++ b/lib/net/rte_ip.h
> > > > > @@ -38,7 +38,21 @@ extern "C" {
> > > > >   * IPv4 Header
> > > > >   */
> > > > >  struct rte_ipv4_hdr {
> > > > > - uint8_t  version_ihl;           /**< version and header length */
> > > > > + __extension__
> > > > > + union {
> > > > > +         uint8_t version_ihl;    /**< version and header length */
> > > > > +         struct {
> > > > > +#if RTE_BYTE_ORDER == RTE_LITTLE_ENDIAN
> > > > > +                 uint8_t ihl:4;
> > > > > +                 uint8_t version:4; #elif RTE_BYTE_ORDER ==
> > > > > +RTE_BIG_ENDIAN
> > > > > +                 uint8_t version:4;
> > > > > +                 uint8_t ihl:4;
> > > > > +#else
> > > > > +#error "setup endian definition"
> > > > > +#endif
> > > > > +         };
> > > > > + };
> > > > >   uint8_t  type_of_service;       /**< type of service */
> > > > >   rte_be16_t total_length;        /**< length of packet */
> > > > >   rte_be16_t packet_id;           /**< packet ID */
> > > > > --
> > > > > 2.31.1
> > > > >
> > > >
> > > > This does not break the ABI, but it could be discussed if it breaks
> > > the API due to the required structure initialization changes shown in
> > > > test_flow_classify.c.
> > >
> > > Yep, I guess it might be classified as API change.
> > > Another thing that concerns me - it is not the only place in IPv4
> > > header when we unite multiple bit-fields into one field:
> > > type_of_service, fragment_offset.
> > > If we start splitting ipv4 fields into actual bitfields, I suppose
> > > we'll end-up splitting these ones too.
> > > But I am not sure it will pay off - as compiler not always generates
> > > optimal code for reading/updating bitfields.
> > > Did you consider just adding extra macros to simplify access to these
> > > fields (like RTE_IPV4_HDR_(GET_SET)_*), instead?
> > >
> >
> > Let's please not introduce accessor macros for bitfields. If we don't
> > introduce bitfields like these, I would rather stick with the current _MASK,
> > _SHIFT and _FLAG defines.
> >
> > Yes, this change will lead to the introduction of more bitfields, both here
> > and in other places. We already accepted it in the eCPRI structure
> > (/lib/net/rte_ecpri.h), so why not just generally accept it.
> >
> > Are modern compilers really worse at handling a bitfield defined like this,
> > compared to handling a single uint8_t with hand coding? I consider your
> > concern very important, so I'm only asking if it is still relevant, to avoid
> > making decisions based on past experience that might be outdated. (I admit
> > to falling into that trap myself, once in a while.)
> >
> 
> I compared x86 code generated with gcc-9, gcc-10 and clang-10 for these 2 functions:
> void test_ipv4_hdr_byte(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
> {
> 	h->version_ihl = ((version & 0x0f) << 4) | (ihl & 0x0f);
> }
> void test_ipv4_hdr_bits(struct rte_ipv4_hdr *h, uint8_t version, uint8_t ihl)
> {
> 	h->version = version & 0x0f;
> 	h->ihl = ihl & 0x0f;
> }
> meson configuration flags: --default-library=static --buildtype=release
> Each compiler produced identical code for both functions.

For that particular case (2 bit-fields packed tightly into one byte)
compilers usually perform quite well. At least I never saw issues for such case.
Bit-fields that do cross byte boundaries - that might be a trouble.  

> 
> 
> > > > I think this patch is an improvement, and that such structure
> > > modifications should be generally accepted, so:
> > > >
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > >


  reply	other threads:[~2021-05-31  9:58 UTC|newest]

Thread overview: 52+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-05-27 15:28 Gregory Etelson
2021-05-27 15:56 ` Morten Brørup
2021-05-28 10:20   ` Ananyev, Konstantin
2021-05-28 10:52     ` Morten Brørup
2021-05-28 14:18       ` Gregory Etelson
2021-05-31  9:58         ` Ananyev, Konstantin [this message]
2021-05-31 11:10           ` Gregory Etelson
2021-06-02  9:51             ` Gregory Etelson
2021-06-10  4:10               ` Gregory Etelson
2021-06-10  9:22                 ` Olivier Matz
2021-06-14 16:36                   ` Andrew Rybchenko
2021-06-17 16:29                     ` Ferruh Yigit
2021-06-03  0:58   ` Min Hu (Connor)
2021-06-03  2:03     ` Stephen Hemminger
2021-06-03  4:59       ` Gregory Etelson
2021-06-17 15:02 ` Tyler Retzlaff
2021-09-03  7:30 ` [dpdk-dev] [PATCH v2] " getelson
2021-10-04  7:49   ` Olivier Matz
2021-10-04  8:59     ` Gregory Etelson
2021-10-04  8:46 ` [dpdk-dev] [PATCH v3] " Gregory Etelson
2021-10-04 12:13 ` [dpdk-dev] [PATCH v4] " Gregory Etelson
2021-10-04 15:59   ` Stephen Hemminger
2021-10-04 16:48     ` Olivier Matz
2021-10-12 12:36       ` Gregory Etelson
2021-10-12 12:29 ` [dpdk-dev] [PATCH v5] " Gregory Etelson
2021-10-13  8:58   ` Kinsella, Ray
2021-10-13 12:14   ` Ferruh Yigit
2021-10-13 17:12     ` Gregory Etelson
2021-10-13 17:13 ` [dpdk-dev] [PATCH v6 0/2] " Gregory Etelson
2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 1/2] net: fix IPv4 change announce Gregory Etelson
2021-10-14  8:37     ` Thomas Monjalon
2021-10-14 11:03       ` Ferruh Yigit
2021-10-14 12:21         ` Gregory Etelson
2021-10-14 12:32           ` Ferruh Yigit
2021-10-14 13:34             ` Ori Kam
2021-10-14 12:53       ` [dpdk-dev] [EXT] " Akhil Goyal
2021-10-13 17:13   ` [dpdk-dev] [PATCH v6 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
2021-10-14 15:11     ` Ferruh Yigit
2021-10-14 16:04       ` Thomas Monjalon
2021-10-14 17:42         ` Gregory Etelson
2021-10-14  8:21   ` [dpdk-dev] [PATCH v6 0/2] " Ferruh Yigit
2021-10-14  8:30     ` Thomas Monjalon
2021-10-14 11:01       ` Ferruh Yigit
2021-10-14  9:29     ` Gregory Etelson
2021-10-14 11:10       ` Ferruh Yigit
2021-10-14 17:41 ` [dpdk-dev] [PATCH v7 " Gregory Etelson
2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 1/2] net: fix IPv4 change announce Gregory Etelson
2021-10-14 20:00     ` Slava Ovsiienko
2021-10-21 22:00       ` Ajit Khaparde
2021-10-14 17:41   ` [dpdk-dev] [PATCH v7 2/2] net: introduce IPv4 ihl and version fields Gregory Etelson
2021-10-14 18:16     ` Ajit Khaparde
2021-10-14 19:52   ` [dpdk-dev] [PATCH v7 0/2] " 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=DM6PR11MB4491682F88519F3DA89F48169A3F9@DM6PR11MB4491.namprd11.prod.outlook.com \
    --to=konstantin.ananyev@intel.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=getelson@nvidia.com \
    --cc=matan@nvidia.com \
    --cc=mb@smartsharesystems.com \
    --cc=olivier.matz@6wind.com \
    --cc=orika@nvidia.com \
    --cc=rasland@nvidia.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

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror http://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ http://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git