From: Thomas Monjalon <thomas@monjalon.net>
To: "David Marchand" <david.marchand@redhat.com>,
"Morten Brørup" <mb@smartsharesystems.com>,
"Andre Muezerie" <andremue@linux.microsoft.com>
Cc: dev@dpdk.org, Stephen Hemminger <stephen@networkplumber.org>,
Bruce Richardson <bruce.richardson@intel.com>
Subject: Re: [PATCH v5 0/3] add portable version of __builtin_add_overflow
Date: Wed, 04 Jun 2025 13:39:19 +0200 [thread overview]
Message-ID: <3992237.PYKUYFuaPT@thomas> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35E9FCC0@smartserver.smartshare.dk>
04/06/2025 13:04, Morten Brørup:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Wednesday, 4 June 2025 12.41
> >
> > On Wed, Jun 4, 2025 at 12:29 PM Morten Brørup
> > <mb@smartsharesystems.com> wrote:
> > > > I am not a fan of adding such public API, an internal API would be
> > > > enough.
> > > > Do you plan to add more helpers for math operations?
> > > >
> > > > For the current helper, the only user is a driver (base code).
> > > > Can't we just wrap a __builtin_add_overflow (under #ifdef msvc) in
> > the
> > > > osdep.h header?
> > >
> > > We already have public APIs for bit operations in rte_bitops.h.
> > > This math API follows the same principle; and math operations - just
> > like bit operations - might be useful for DPDK applications, so let's
> > keep it public.
> >
> > This comparison is poor.
> >
> > There are many users of bitops in dpdk, and *public* headers needed it.
>
> I don't think the number of uses of a generic function should determine if it should be public or private.
> The important thing is avoiding copy-pasting.
>
> > Here, we have one single function in a driver implementation.
> > And this code is unused (__builtin_add_overflow -> check_add_overflow
> > -> ice_get_pfa_module_tlv -> ice_get_link_default_override ->
> > ice_cfg_phy_fec, with no intree user).
> >
>
> I'm mainly saying that Andre is doing nothing wrong here;
> it's a matter of setting the bar for making generic functions part of DPDK's public API.
>
> In this particular case, I don't have a strong opinion on how public the new function is.
> Putting it in some generic private header is also perfectly acceptable for me.
> Just don't put it directly in the driver; that would lead to copy-paste into other drivers.
We can move it as a public API later if there is a real need.
I don't think it is good to rush on adding new API in general.
> > > The only issue I have with these (incl. the bit operations) are that
> > they are in the EAL library, although they have absolutely nothing to
> > do with hardware or O/S abstraction, so they really should be in a
> > "utils" library.
> > > But that's another story, so let's not burden Andre with that.
> >
> > Orthogonal to the question.
>
> Partly, yes.
EAL is also good to abstract compiler differences.
I agree such basic stuff should be in EAL
if we would decide to add it.
> But if we had a generic "utils" library, there would be less resistance
> to adding the new function there than there is to adding it to the EAL API.
It would be the same.
An API is supposed to be maintained forever
and give guidance on what to use.
As a conclusion, I agree with David, it is safe to keep it private
for the unused base function for now.
next prev parent reply other threads:[~2025-06-04 11:39 UTC|newest]
Thread overview: 52+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-01-02 22:32 [PATCH 0/5] " Andre Muezerie
2025-01-02 22:32 ` [PATCH 1/5] maintainers: " Andre Muezerie
2025-01-02 22:32 ` [PATCH 2/5] lib/eal: " Andre Muezerie
2025-01-03 8:19 ` Morten Brørup
2025-01-03 20:38 ` Andre Muezerie
2025-01-02 22:32 ` [PATCH 3/5] doc/api: " Andre Muezerie
2025-01-02 22:32 ` [PATCH 4/5] drivers/net: use " Andre Muezerie
2025-01-02 22:32 ` [PATCH 5/5] app/test: add tests for portable versions " Andre Muezerie
2025-01-02 23:51 ` [PATCH 0/5] add portable version " Stephen Hemminger
2025-01-03 0:15 ` Andre Muezerie
2025-01-03 0:41 ` Andre Muezerie
2025-01-03 20:39 ` [PATCH v2 " Andre Muezerie
2025-01-03 20:39 ` [PATCH v2 1/5] maintainers: " Andre Muezerie
2025-01-03 20:39 ` [PATCH v2 2/5] lib/eal: " Andre Muezerie
2025-01-06 11:07 ` Bruce Richardson
2025-01-06 11:21 ` Morten Brørup
2025-01-06 11:34 ` Bruce Richardson
2025-01-06 11:58 ` Morten Brørup
2025-03-05 15:46 ` Andre Muezerie
2025-01-03 20:39 ` [PATCH v2 3/5] doc/api: " Andre Muezerie
2025-01-03 20:39 ` [PATCH v2 4/5] drivers/net: use " Andre Muezerie
2025-01-03 20:39 ` [PATCH v2 5/5] app/test: add tests for " Andre Muezerie
2025-03-05 16:38 ` [PATCH v3 0/5] add " Andre Muezerie
2025-03-05 16:38 ` [PATCH v3 1/5] maintainers: " Andre Muezerie
2025-03-05 16:38 ` [PATCH v3 2/5] eal: " Andre Muezerie
2025-03-05 16:38 ` [PATCH v3 3/5] doc/api: " Andre Muezerie
2025-03-05 16:38 ` [PATCH v3 4/5] net/intel: use " Andre Muezerie
2025-03-05 16:52 ` Bruce Richardson
2025-03-11 18:39 ` Andre Muezerie
2025-03-05 16:38 ` [PATCH v3 5/5] app/test: add tests for " Andre Muezerie
2025-03-06 3:00 ` Patrick Robb
2025-03-13 20:00 ` [PATCH v4 0/5] add " Andre Muezerie
2025-03-13 20:00 ` [PATCH v4 1/5] maintainers: " Andre Muezerie
2025-03-14 8:39 ` Bruce Richardson
2025-03-13 20:00 ` [PATCH v4 2/5] eal: " Andre Muezerie
2025-03-13 20:00 ` [PATCH v4 3/5] doc/api: " Andre Muezerie
2025-03-14 8:40 ` Bruce Richardson
2025-03-13 20:00 ` [PATCH v4 4/5] net/intel: use " Andre Muezerie
2025-03-13 20:00 ` [PATCH v4 5/5] app/test: add tests for " Andre Muezerie
2025-03-14 14:33 ` [PATCH v5 0/3] add " Andre Muezerie
2025-03-14 14:33 ` [PATCH v5 1/3] eal: " Andre Muezerie
2025-03-14 14:33 ` [PATCH v5 2/3] net/intel: use " Andre Muezerie
2025-03-14 14:46 ` Bruce Richardson
2025-03-14 14:33 ` [PATCH v5 3/3] app/test: add tests for " Andre Muezerie
2025-06-04 10:08 ` [PATCH v5 0/3] add " David Marchand
2025-06-04 10:29 ` Morten Brørup
2025-06-04 10:41 ` David Marchand
2025-06-04 11:04 ` Morten Brørup
2025-06-04 11:39 ` Thomas Monjalon [this message]
2025-06-05 14:47 ` [PATCH v6 0/1] define __builtin_add_overflow for MSVC Andre Muezerie
2025-06-05 14:47 ` [PATCH v6 1/1] net/intel: " Andre Muezerie
2025-06-05 14:57 ` Morten Brørup
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=3992237.PYKUYFuaPT@thomas \
--to=thomas@monjalon.net \
--cc=andremue@linux.microsoft.com \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=mb@smartsharesystems.com \
--cc=stephen@networkplumber.org \
/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).