> From: David Marchand [mailto:david.march...@redhat.com]
> Sent: Monday, 6 February 2023 17.11
>
> On Wed, Feb 1, 2023 at 2:16 PM Thomas Monjalon <tho...@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.
