From: Thomas Monjalon <thomas@monjalon.net>
To: "Tyler Retzlaff" <roretzla@linux.microsoft.com>,
"David Marchand" <david.marchand@redhat.com>,
"Morten Brørup" <mb@smartsharesystems.com>
Cc: 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: Wed, 01 Feb 2023 14:15:54 +0100 [thread overview]
Message-ID: <5353813.29KlJPOoH8@thomas> (raw)
In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D876E6@smartserver.smartshare.dk>
01/02/2023 13:50, Morten Brørup:
> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> > 31/01/2023 19:26, Tyler Retzlaff:
> > > On Tue, Jan 31, 2023 at 01:23:34PM +0100, Morten Brørup wrote:
> > > > > From: David Marchand [mailto:david.marchand@redhat.com]
> > > > > On Wed, Jan 18, 2023 at 9:31 AM Morten Brørup
> > > > > <mb@smartsharesystems.com> wrote:
> > > > > > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com]
> > > > > > > On Tue, Jan 17, 2023 at 09:19:22AM +0100, Morten Brørup
> > > > > > > > > > + */
> > > > > > > > > > +#if defined(RTE_CC_GCC) || defined(RTE_CC_CLANG)
> > > > > > > > > > +#define __rte_nonnull_params(...) \
> > > > > > > > > > + __attribute__((nonnull(__VA_ARGS__)))
> > > > > > > > > > +#else
> > > > > > > > > > +#define __rte_nonnull_params(...)
> > > > > > > > > > +#endif
> > > > > > > > > > +
> > > > > > > > >
> > > > > > > > > What do you think to have a namespace for macros like
> > > > > > > > > '__rte_param_xxx',
> > > > > > > > > so name macros as:
> > > > > > > > > __rte_param_nonull
> > > > > > > > > __rte_param_read_only
> > > > > > > > > __rte_param_write_only
> > > > > > > > >
> > > > > > > > > No strong opinion, it just feels tidier this way
> >
> > 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.
> > > > > > > microsoft also has a tool & annotation vehicle for this type
> > of
> > > > > stuff.
> > > > > > > this discussion has caused me to wonder what happens if we
> > would
> > > > > like
> > > > > > > to
> > > > > > > add additional annotations for other tools. just load on the
> > > > > > > annotations
> > > > > > > and expand them empty conditionally?
> > > > > > >
> > > > > > > https://learn.microsoft.com/en-us/cpp/code-quality/using-sal-
> > > > > > > annotations-to-reduce-c-cpp-code-defects?view=msvc-170
> > > > > > >
> > > > > > > anyway, just a thought. no serious response required here.
> > > > > >
> > > > > > Excellent input, Tyler!
> > > > > >
> > > > > > If we want DPDK to be considered truly cross-platform, and not
> > treat
> > > > > non-Linux/non-GCC as second class citizens, we need to discuss
> > this.
> > > > > >
> > > > > > Microsoft's Source Code Annotation Language (SAL) seems very
> > good,
> > > > > based on its finer granularity than GCC's attributes (which in
> > > > > comparison seem added as an afterthought, not cleanly structured
> > like
> > > > > SAL). I have only skimmed the documentation, but that is my
> > immediate
> > > > > impression of it.
> > > > > >
> > > > > > SAL uses a completely different syntax than GCC attributes, and
> > > > > Microsoft happens to also use memcpy() as an example in the
> > > > > documentation referred to:
> > > > > >
> > > > > > void * memcpy(
> > > > > > _Out_writes_bytes_all_(count) void *dest,
> > > > > > _In_reads_bytes_(count) const void *src,
> > > > > > size_t count
> > > > > > );
> > > >
> > > > Stephen had bad experiences with SAL, so let's just consider the
> > SAL memcpy() example a reference only, showing how the syntax of
> > annotations can differ very much between build systems.
> > >
> > > yes, if we are in a position to use annotations today that work with
> > > clang/gcc then let's do that even if they aren't compatible with SAL.
> > > so long as they expand empty when not using clang/gcc we can defer
> > discussion
> > > about other tools like SAL.
> >
> > Yes of course it must expand empty for compilers not supporting the
> > annotation.
> > Anyway I don't think we need to support all compilers with annotation.
> > The main goal of annotation is to do more checks in the CI,
> > so supporting one compiler should be enough.
>
> There is also a secondary goal of increased performance. When the compiler knows more about a function's behavior, it can optimize more around it. E.g. knowing that a function doesn't modify something already held in a register (or other local memory) allows the compiler to reuse the local copy (in the register) without fetching the original again.
If we want to enable such performance optimizations with all compilers,
we may have issues to find a common syntax.
In general I am OK to try (best effort)
but we should be reasonnable in case things are getting complex.
[...]
> > > > > 3d is the best option as it is not changing anything to what we
> > were
> > > > > doing so far: we evaluate the pros and cons of each
> > annotations/tools,
> > > > > case per case.
> > > >
> > > > Agree!
> >
> > I am for 3d as well.
> > We should focus on clang for using compilation checks.
>
> Clang doesn't support the access mode attribute (yet?). It is only supported by GCC.
OK
> The CI also uses GCC, so let's not limit ourselves to what clang can do. Using the access mode attribute with GCC is still useful for the CI to reveal bugs.
>
> I agree with you on the principle: We don't need to add the same bug-finding attributes to all compiler environments; adding them to one CI supported compiler environment should suffice.
>
> However, performance-enhancing attributes would be useful to support in multiple compiler environments.
>
> > Another question about annotations is how much we want to use them.
> > I think we should use such check for critical code only.
>
> Agree. And this includes some fast path code, where they might benefit performance.
>
> > Adding annotations everywhere is not always worth making the code
> > uglier.
>
> I agree. I suppose Stephen stressed the same point when referring his experience working with SAL.
>
> It will be impossible to define a criteria for when to use or not use such attributes, but we can probably agree on this: When adding new functions to DPDK, adding such attributes is not a requirement. This should make it up to the contributor to decide if she wants to add them or not, which can be discussed on a case by case basis if the maintainer disagrees.
Yes
> This principle should limit the big discussions to which attributes we want to allow into rte_common.h; like these and the ones in David's lock annotation patch.
OK
next prev parent reply other threads:[~2023-02-01 13:16 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 [this message]
2023-02-06 16:11 ` David Marchand
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=5353813.29KlJPOoH8@thomas \
--to=thomas@monjalon.net \
--cc=bruce.richardson@intel.com \
--cc=david.marchand@redhat.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=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).