DPDK patches and discussions
 help / color / mirror / Atom feed
From: Morten Brørup <mb@smartsharesystems.com>
To: "Ananyev, Konstantin" <konstantin.ananyev@intel.com>,
	"Gregory Etelson" <getelson@nvidia.com>, <dev@dpdk.org>
Cc: <matan@nvidia.com>, <orika@nvidia.com>, <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: Fri, 28 May 2021 12:52:52 +0200
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk> (raw)
In-Reply-To: <BY5PR11MB4482957E35C0B23F493020159A229@BY5PR11MB4482.namprd11.prod.outlook.com>

> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Ananyev,
> Konstantin
> Sent: Friday, 28 May 2021 12.21
> 
> > > From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Gregory
> Etelson
> > > Sent: Thursday, 27 May 2021 17.29
> > and version fields
> > >
> > > 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 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-28 10:52 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 [this message]
2021-05-28 14:18       ` Gregory Etelson
2021-05-31  9:58         ` Ananyev, Konstantin
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=98CBD80474FA8B44BF855DF32C47DC35C617E6@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=bernard.iremonger@intel.com \
    --cc=dev@dpdk.org \
    --cc=getelson@nvidia.com \
    --cc=konstantin.ananyev@intel.com \
    --cc=matan@nvidia.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