RE: [PATCH v7 4/4] eal: add nonnull and access function attributes

["Morten Brørup" <mb@smartsharesystems.com>]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Monday, 6 February 2023 17.11
> 
> 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?

The existing __rte_format_printf macro also follows the simple convention, and does not add "param" or "function" in the name:

#define __rte_format_printf(format_index, first_arg) \
	__attribute__((format(printf, format_index, first_arg)))

> >
> > Please let's have a naming discussion with many opinions.
> 
> Thomas, I think naming convention/discussion is the most likely to
> never conclude.

No one else seem to have strong enough opinions to join the discussion. So let's try to conclude the discussion.

> 
> 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.

I'm leaning towards following the existing convention in rte_common.h, and embrace Thomas' argument to make them more verbose in order to reduce the risk of wrong use. In other words, define these:

__rte_nonnull(...)
__rte_read_only(ptr_index)
__rte_read_only_size(ptr_index, size_index)
__rte_write_only(ptr_index)
__rte_write_only_size(ptr_index, size_index)
__rte_read_write(ptr_index)
__rte_read_write_size(ptr_index, size_index)
__rte_no_access(ptr_index)
__rte_no_access_size(ptr_index, size_index)

> 
> 
> 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.

The form follows the existing convention in rte_common.h, and I think we should stick with it.

> 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.

Conclusion:

The names I listed in this email, and what David already has in his lock annotation patch, are both in line with an existing convention already established in rte_common.h. So unless someone objects very soon, let's go for that.