DPDK patches and discussions
 help / color / mirror / Atom feed
From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Thomas Monjalon" <thomas@monjalon.net>,
	"Jerin Jacob" <jerinjacobk@gmail.com>,
	"Van Haaren, Harry" <harry.van.haaren@intel.com>
Cc: "Jerin Jacob" <jerinj@marvell.com>, <dev@dpdk.org>,
	"Li, WeiyuanX" <weiyuanx.li@intel.com>,
	"Ferruh Yigit" <ferruh.yigit@amd.com>,
	"Andrew Rybchenko" <andrew.rybchenko@oktetlabs.ru>,
	<david.marchand@redhat.com>
Subject: RE: rte_event_dev_xstats_reset id type
Date: Wed, 12 Oct 2022 17:35:45 +0200	[thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D873D8@smartserver.smartshare.dk> (raw)
In-Reply-To: <8864071.MhkbZ0Pkbq@thomas>

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, 12 October 2022 17.13
> 
> 12/10/2022 14:14, Van Haaren, Harry:
> > From: Morten Brørup <mb@smartsharesystems.com>
> > > From: Van Haaren, Harry [mailto:harry.van.haaren@intel.com]
> > > > From: Jerin Jacob <jerinjacobk@gmail.com>
> > > > > On Wed, Oct 12, 2022 at 1:40 PM Morten Brørup wrote:
> > > > > >
> > > > > > Hi Jerin (eventdev maintainer),
> > > > >
> > > > > + harry.van.haaren@intel.com as the changes in
> drivers/event/sw.
> > > >
> > > > Thanks Jerin.
> > > >
> > > >
> > > > > > While looking into bug #1101 [1], I noticed a mix of unsigned
> int
> > > > and uint32_t in
> > > > > the test code, which will fail on 64-bit big endian CPUs.
> > > >
> > > > Aha; that we can fix. I am curious why this isn't found in
> CI/reported
> > > > before.
> > >
> > > We probably don't test any 64-bit *big endian* architectures. Just
> a guess.
> >
> > Seems so yes.
> >
> > > > > > Specifically, rte_event_dev_xstats_reset() is called with the
> "ids"
> > > > parameter
> > > > > pointing to an unsigned int [2], but that parameter is a
> pointer to
> > > > an uint32_t.
> > > > > >
> > > > > > I think the type of the ids array parameter to
> > > > rte_event_dev_xstats_reset() should
> > > > > be changed to unsigned int array, like in the other
> > > > rte_event_dev_xxx() functions.
> > > >
> > > > In this case, we have the option to change the type of a variable
> in a
> > > > test-case, or change API and cause API/ABI breakage.
> > >
> > > Well.. yes, but I would phrase that last option: Change the
> API/ABI, so related
> > > functions consistently use the same type for the same variable,
> instead of randomly
> > > mixing uint64_t, uint32_t and unsigned int, depending on function.
> >
> > Aah ok; I see your point now; there is inconsistent usage of
> uint32_t/unsigned int
> > between the Eventdev APIs itself. Agree this is sub-optimal, and
> would have been
> > nice to have spotted before the Eventdev API was stabilized.
> >
> >
> > > Unfortunately, these functions are not marked experimental, so
> breaking API/ABI is
> > > hard to do. :-(
> >
> > Agreed again.
> 
> 22.11 is a breaking release,
> and changing type in the API is not much impactful,
> so that's something you can change now,
> or be quiet forever :)

Question:
1. Only change the "xstats id" type in the one eventdev function, which deviates from other eventdev functions, or
2. Change the "xstats id" type for all xstats functions across all device types, for consistency across device types?

If 2, then what would be a good type?

Ethdev uses uint64_t for xstats id, and (speaking without knowledge about its internals) that seems like overkill to me. Arrays of these are being used, so size does matter.


  reply	other threads:[~2022-10-12 15:35 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-10-12  8:10 Morten Brørup
2022-10-12  9:45 ` Jerin Jacob
2022-10-12 10:29   ` Van Haaren, Harry
2022-10-12 10:41     ` Morten Brørup
2022-10-12 12:14       ` Van Haaren, Harry
2022-10-12 15:13         ` Thomas Monjalon
2022-10-12 15:35           ` Morten Brørup [this message]
2022-10-12 16:16             ` Jerin Jacob
2022-10-12 16:28               ` Thomas Monjalon
2022-10-12 16:47                 ` Jerin Jacob
2022-10-12 20:44                   ` Thomas Monjalon
2022-10-13  6:51                     ` xstats " Morten Brørup
2022-10-13  7:12                       ` Pavan Nikhilesh Bhagavatula
2022-10-13  8:26                         ` Thomas Monjalon
2022-10-13  8:33                           ` [EXT] " Pavan Nikhilesh Bhagavatula
2022-10-13  8:59                         ` Van Haaren, Harry

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=98CBD80474FA8B44BF855DF32C47DC35D873D8@smartserver.smartshare.dk \
    --to=mb@smartsharesystems.com \
    --cc=andrew.rybchenko@oktetlabs.ru \
    --cc=david.marchand@redhat.com \
    --cc=dev@dpdk.org \
    --cc=ferruh.yigit@amd.com \
    --cc=harry.van.haaren@intel.com \
    --cc=jerinj@marvell.com \
    --cc=jerinjacobk@gmail.com \
    --cc=thomas@monjalon.net \
    --cc=weiyuanx.li@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).