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 485EBA0543; Thu, 22 Sep 2022 15:00:34 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 257B740156; Thu, 22 Sep 2022 15:00:34 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 837A2400D7 for ; Thu, 22 Sep 2022 15:00:32 +0200 (CEST) X-MimeOLE: Produced By Microsoft Exchange V6.5 Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH v2] eal: Pointer alignment check improvements Date: Thu, 22 Sep 2022 15:00:31 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D8734D@smartserver.smartshare.dk> In-Reply-To: X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH v2] eal: Pointer alignment check improvements Thread-Index: AdjOetDCzL7b5BIQT4aLWhlZTP+n7AABI/7A References: <20220921142830.71272-1-mb@smartsharesystems.com> <20220922114413.106291-1-mb@smartsharesystems.com> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" Cc: , , 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 > From: Bruce Richardson [mailto:bruce.richardson@intel.com] > Sent: Thursday, 22 September 2022 13.59 >=20 > On Thu, Sep 22, 2022 at 12:52:42PM +0100, Bruce Richardson wrote: > > On Thu, Sep 22, 2022 at 01:44:13PM +0200, Morten Br=F8rup wrote: > > > Checking a const pointer for alignment would emit a warning about > the > > > const qualifier being discarded. > > > > > > No need to calculate the aligned pointer; just check the last bits > of the > > > pointer. > > > > > > v2: > > > - Remove compiler attribute ((const)) from function; > > > it was a coding style issue. > > > > > > Signed-off-by: Morten Br=F8rup > > > --- > > > lib/eal/include/rte_common.h | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/lib/eal/include/rte_common.h > b/lib/eal/include/rte_common.h > > > index 2e22c1b955..ed81e0db0a 100644 > > > --- a/lib/eal/include/rte_common.h > > > +++ b/lib/eal/include/rte_common.h > > > @@ -404,9 +404,9 @@ static void > __attribute__((destructor(RTE_PRIO(prio)), used)) func(void) > > > * True(1) where the pointer is correctly aligned, false(0) > otherwise > > > */ > > > static inline int > > > -rte_is_aligned(void *ptr, unsigned align) > > > +rte_is_aligned(const void * const __rte_restrict ptr, const > unsigned int align) > > > { > > > - return RTE_PTR_ALIGN(ptr, align) =3D=3D ptr; > > > + return ((uintptr_t)ptr & (align - 1)) =3D=3D 0; > > > > Are we confident that in future, or using come compiler settings, we > won't > > get an error due to using "uintptr_t" rather than "const uintptr_t" > in the > > cast? I would put a const in there myself, just to be safe. Good idea. > > > > A further point, only-semi-related to this patch, which is fine as- > is: > > looking at the code for the various macros in rte_common.h: > > * The various macros for working on pointers can can probably be > converted > > to functions, since they don't need to work with variable-sized > types. > > * We can then see about properly ensuring those inline functions are > > const-correct. The problem with const in a function parameter is the ripple effect: all = the underlying functions must also use const. I generally prefer using const where possible, but the ripple effect = often makes it difficult. > > > Actually, on further investigation in trying this, it appears that the > macros are used in a number of places with integer data too, despite > the > "PTR" in the name, so things are best alone for now, I think. Even the macros that also exist without "PTR" in the name? (Example, = please.) Instead of providing multiple macros for essentially doing the same = thing to different types, we could use __builtin_choose_expr [1] to = support a variety of types in the macros. This built-in can be used as a = workaround for not being able to use C++, where the same function name = can be used by multiple functions with different parameter types. [1]: https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html I was also wondering why the rte_is_aligned function doesn't have "ptr" = in its name, because it cannot be used for integer types. Changing = rte_is_aligned to a macro using __builtin_choose_expr could solve this. = But I don't think such a patch will be popular, so I chose to stick with = the simple fix. -Morten