From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 5BEA841C24; Mon, 6 Feb 2023 18:28:45 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 2B78C427E9; Mon, 6 Feb 2023 18:28:45 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id C5E3640ED7 for ; Mon, 6 Feb 2023 18:28:42 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 1F37F20C7E04; Mon, 6 Feb 2023 09:28:42 -0800 (PST) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 1F37F20C7E04 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1675704522; bh=jWQ/6SB/y4uNePkMpxfZRNRTF9pQ4KS3gKCVZH+IxfQ=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=k4P8tfJBF7QeTDAnD4Ae+5gVGArOYzZvCSkwE3oMEkOdi8T1cF1bf9BXxdzNJSy6d eJJ/G8kVM3t/o2rFt3Gb0HhtcWU380Qu3yPk7nXc7Y91fh92gbxpEMsGu2K2jmYOUx QV9xhulGP+x8B5S0L3ijNbZTT1xSC09UWW05wYL0= Date: Mon, 6 Feb 2023 09:28:42 -0800 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: David Marchand , Thomas Monjalon , bruce.richardson@intel.com, Ferruh Yigit , 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 Message-ID: <20230206172842.GA16086@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <20221202153432.131023-1-mb@smartsharesystems.com> <3659208.MHq7AAxBmi@thomas> <98CBD80474FA8B44BF855DF32C47DC35D876E6@smartserver.smartshare.dk> <5353813.29KlJPOoH8@thomas> <98CBD80474FA8B44BF855DF32C47DC35D87704@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D87704@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org On Mon, Feb 06, 2023 at 05:49:18PM +0100, Morten Brørup wrote: > > 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 > > 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. +1 the value of getting this in with ~whatever names outweighs a prolonged discussion about naming. my view is this isn't part of the dpdk stable public api and it is incidental that the headers introduce the names. if applications choose to consume them, we document that they change from time to time without notice. i also think if naming is of high importance it can be done post-merge. slightly related it would be nice to establish a more streamlined contribution process for non-semantic changes. it's painful to review changes filled with gratuitous style refactors mixed amongst functional change or code move / rename etc.. it's much easier to spot errors when such changes are in separate series, so let's make integrating them a fast path. > > > > > 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. >