From: David Marchand <david.marchand@redhat.com>
To: Thomas Monjalon <thomas@monjalon.net>
Cc: "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
"Morten Brørup" <mb@smartsharesystems.com>,
bruce.richardson@intel.com, "Ferruh Yigit" <ferruh.yigit@amd.com>,
dev@dpdk.org, rmody@marvell.com, timothy.mcdaniel@intel.com,
matan@nvidia.com, viacheslavo@nvidia.com, ruifeng.wang@arm.com,
zhoumin@loongson.cn, drc@linux.vnet.ibm.com, kda@semihalf.com,
konstantin.v.ananyev@yandex.ru, stephen@networkplumber.org,
jerinj@marvell.com, honnappa.nagarahalli@arm.com
Subject: Re: [PATCH v7 4/4] eal: add nonnull and access function attributes
Date: Mon, 6 Feb 2023 17:11:19 +0100 [thread overview]
Message-ID: <CAJFAV8yZ3LmvMKfB9SLHZVUQCFVFJkX=UOaCMKgUW1rMtRKueg@mail.gmail.com> (raw)
In-Reply-To: <5353813.29KlJPOoH8@thomas>
On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon <thomas@monjalon.net> wrote:
> > > I tend to prefer this kind of namespace as well.
> > > Let's compare different naming proposals,
> > > taking into account what we already have for some annotations,
> > > and what is proposed to be added in this patch and David's patch
> > > for lock annotations.
> >
> > David's lock annotations don't use a dedicated naming convention, but simply replaces __attribute__(name(params)) with __rte_name(params), e.g.:
> >
> > #define __rte_guarded_by(...) \
> > __attribute__((guarded_by(__VA_ARGS__)))
> > #define __rte_exclusive_locks_required(...) \
> > __attribute__((exclusive_locks_required(__VA_ARGS__)))
> > #define __rte_assert_exclusive_lock(...) \
> > __attribute__((assert_exclusive_lock(__VA_ARGS__)))
> >
> > This follows the existing convention in rte_common.h, which is easily readable, because they directly translate to GCC attribute names, e.g.:
> >
> > #define __rte_warn_unused_result __attribute__((warn_unused_result))
> > #define __rte_always_inline inline __attribute__((always_inline))
> > #define __rte_noinline __attribute__((noinline))
> > #define __rte_hot __attribute__((hot))
> > #define __rte_cold __attribute__((cold))
> >
> > I could follow this convention too, e.g.:
> >
> > #define __rte_nonnull(...) __attribute__((nonnull(__VA_ARGS__)))
> >
> > #define __rte_access_read_only(...) \
> > __attribute__((access(read_only, __VA_ARGS__)))
> > #define __rte_access_write_only(...) \
> > __attribute__((access(write_only, __VA_ARGS__)))
> >
> [...]
> > > Longer macro names may be better for code readers.
> >
> > You have a good point here, Thomas. It will also prevent using the access mode attribute macros incorrectly.
> >
> > Let's proceed with two macros (without and with size parameter) instead of the combined macros (with optional size parameter).
> >
> > Referring to the existing convention in rte_common.h, what do you think about naming the new macros as follows?
> >
> > __rte_access_read_only(ptr_index)
> > __rte_access_read_only_size(ptr_index, size_index)
> >
> > Or just:
> >
> > __rte_read_only(ptr_index)
> > __rte_read_only_size(ptr_index, size_index)
>
> I think we don't need the word "access", so I prefer the second form.
>
> What about the proposal of having "param" in the name?
> We could also have "function" is some macro names.
> Does it bring a benefit?
>
> Please let's have a naming discussion with many opinions.
Thomas, I think naming convention/discussion is the most likely to
never conclude.
Just copying my previous suggestion.
__rte_read_only_params(indexes...)
__rte_write_only_params(indexes...)
__rte_no_access_params(indexes...)
So if we are not going with the existing (kind of) convention to just
prefix with __rte_, I prefer Morten second form too and I have no
better idea.
As for the lock annotations series, if you are not confident with the
form I went with, I don't mind deferring to a later release.
Though it adds more work on my pile like rebasing the vhost library.
Additionnally, we lose the opportunity to catch introduction of new
lock issues in the dpdk tree.
--
David Marchand
next prev parent reply other threads:[~2023-02-06 16:11 UTC|newest]
Thread overview: 77+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-12-02 15:34 [PATCH] " Morten Brørup
2022-12-02 20:02 ` Tyler Retzlaff
2022-12-03 14:22 ` [PATCH v2] " Morten Brørup
2022-12-05 10:17 ` Ruifeng Wang
2022-12-12 7:40 ` Morten Brørup
2022-12-28 10:27 ` [PATCH v3 1/2] " Morten Brørup
2022-12-28 10:27 ` [PATCH v3 2/2] net/bnx2x: fix warnings about rte_memcopy lengths Morten Brørup
2022-12-28 11:40 ` [PATCH v4 1/2] eal: add nonnull and access function attributes Morten Brørup
2022-12-28 11:40 ` [PATCH v4 2/2] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2022-12-28 15:10 ` [PATCH v5 1/4] eal: add nonnull and access function attributes Morten Brørup
2022-12-28 15:10 ` [PATCH v5 2/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2022-12-28 16:13 ` Stanisław Kardach
2022-12-28 16:38 ` Morten Brørup
2022-12-28 17:03 ` Stephen Hemminger
2022-12-28 17:37 ` Morten Brørup
2023-01-09 10:36 ` David Marchand
2022-12-28 15:10 ` [PATCH v5 3/4] event/dlb2: remove superfluous rte_memcpy Morten Brørup
2022-12-28 17:01 ` Stephen Hemminger
2022-12-28 15:10 ` [PATCH v5 4/4] net/mlx5: fix warning about rte_memcpy length Morten Brørup
2023-01-09 11:08 ` [PATCH v5 1/4] eal: add nonnull and access function attributes Thomas Monjalon
2023-01-09 12:16 ` Morten Brørup
2023-01-09 11:22 ` David Marchand
2023-01-09 12:28 ` Morten Brørup
2023-01-16 12:49 ` Morten Brørup
2023-01-16 12:44 ` [PATCH v6 1/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2023-01-16 12:44 ` [PATCH v6 2/4] event/dlb2: remove superfluous rte_memcpy Morten Brørup
2023-01-16 12:44 ` [PATCH v6 3/4] net/mlx5: fix warning about rte_memcpy length Morten Brørup
2023-01-16 12:44 ` [PATCH v6 4/4] eal: add nonnull and access function attributes Morten Brørup
2023-01-16 13:07 ` [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2023-01-16 13:07 ` [PATCH v7 2/4] event/dlb2: remove superfluous rte_memcpy Morten Brørup
2023-02-09 16:51 ` Morten Brørup
2023-02-09 18:50 ` Sevincer, Abdullah
2023-02-10 7:43 ` Morten Brørup
2024-02-23 13:19 ` Jerin Jacob
2024-02-23 13:49 ` [PATCH v8] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2024-02-23 14:00 ` [PATCH v9] " Morten Brørup
2024-02-26 8:34 ` Jerin Jacob
2024-02-26 14:47 ` Morten Brørup
2024-02-27 11:00 ` Jerin Jacob
2024-02-27 11:27 ` Morten Brørup
2024-02-27 19:06 ` Stephen Hemminger
2024-02-28 9:02 ` Bruce Richardson
2024-02-28 8:27 ` Raslan Darawsheh
2024-02-26 21:45 ` Stephen Hemminger
2023-01-16 13:07 ` [PATCH v7 3/4] net/mlx5: fix warning about rte_memcpy length Morten Brørup
2023-02-09 16:54 ` Morten Brørup
2023-02-10 9:13 ` Slava Ovsiienko
2024-03-13 10:00 ` Raslan Darawsheh
2023-01-16 13:07 ` [PATCH v7 4/4] eal: add nonnull and access function attributes Morten Brørup
2023-01-16 17:02 ` Ferruh Yigit
2023-01-17 8:19 ` Morten Brørup
2023-01-17 21:16 ` Tyler Retzlaff
2023-01-18 8:31 ` Morten Brørup
2023-01-18 17:23 ` Stephen Hemminger
2023-01-31 11:14 ` David Marchand
2023-01-31 12:23 ` Morten Brørup
2023-01-31 18:26 ` Tyler Retzlaff
2023-01-31 22:52 ` Thomas Monjalon
2023-02-01 12:50 ` Morten Brørup
2023-02-01 13:15 ` Thomas Monjalon
2023-02-06 16:11 ` David Marchand [this message]
2023-02-06 16:49 ` Morten Brørup
2023-02-06 17:28 ` Tyler Retzlaff
2023-05-08 12:32 ` Morten Brørup
2023-10-13 23:31 ` Morten Brørup
2023-04-04 13:41 ` Morten Brørup
2023-04-04 13:51 ` David Marchand
2023-04-04 14:01 ` Morten Brørup
2023-04-04 14:19 ` David Marchand
2023-02-09 16:49 ` [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths Morten Brørup
2023-02-26 9:21 ` Marvell QLogic bnx2x PMD support status Morten Brørup
2023-03-09 10:25 ` [PATCH v7 1/4] net/bnx2x: fix warnings about rte_memcpy lengths David Marchand
2023-03-09 10:33 ` Thomas Monjalon
2023-03-09 16:11 ` [EXT] " Akhil Goyal
[not found] ` <SJ0PR18MB390039A8C34E485F8D2D518EA1B59@SJ0PR18MB3900.namprd18.prod.outlook.com>
2023-03-09 16:19 ` Devendra Singh Rawat
2023-03-09 16:23 ` Stephen Hemminger
2024-02-23 12:39 ` Jerin Jacob
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='CAJFAV8yZ3LmvMKfB9SLHZVUQCFVFJkX=UOaCMKgUW1rMtRKueg@mail.gmail.com' \
--to=david.marchand@redhat.com \
--cc=bruce.richardson@intel.com \
--cc=dev@dpdk.org \
--cc=drc@linux.vnet.ibm.com \
--cc=ferruh.yigit@amd.com \
--cc=honnappa.nagarahalli@arm.com \
--cc=jerinj@marvell.com \
--cc=kda@semihalf.com \
--cc=konstantin.v.ananyev@yandex.ru \
--cc=matan@nvidia.com \
--cc=mb@smartsharesystems.com \
--cc=rmody@marvell.com \
--cc=roretzla@linux.microsoft.com \
--cc=ruifeng.wang@arm.com \
--cc=stephen@networkplumber.org \
--cc=thomas@monjalon.net \
--cc=timothy.mcdaniel@intel.com \
--cc=viacheslavo@nvidia.com \
--cc=zhoumin@loongson.cn \
/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).