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 6CDBBA0C4B; Mon, 25 Oct 2021 11:44:05 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 53B53407FF; Mon, 25 Oct 2021 11:44:05 +0200 (CEST) Received: from mail-il1-f176.google.com (mail-il1-f176.google.com [209.85.166.176]) by mails.dpdk.org (Postfix) with ESMTP id 1A9004003E for ; Mon, 25 Oct 2021 11:44:03 +0200 (CEST) Received: by mail-il1-f176.google.com with SMTP id l7so12141794iln.8 for ; Mon, 25 Oct 2021 02:44:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=iXOlbx38Y2q3OqEV1X8TI1ofGZ7AB3QTf0udX1e5zlI=; b=O1/Rygo+iC5IBYO6nSh8Ei6cxzN58n7Cd7vMWNu7l1TYl8Fat7oEcWkDNElB8osqBH DunMzcJm4cGAi7KMUbkv2IfOT2NFgORyPnwh2EOaal7u6U9l14EtWjXqbFT7UxN4u+XV 8rqziDjjZspkIpzATbUXVJdhJn/SXPxCXJjs1Y5hRRTZ5wZuu5p25Dfi4nYnWyVBScFs a4l3/Ym7+BY252PV3o5XB8K0aWneAkknMNS5hMlk+UWE+d3Q1rjZkHeV1017kJ6exLVl d+AB9VQkmsSbzsHKY7JpZAuNt9AR9un7+FsYupxtSEF+X9omMl0MIl+aRv8z3rKEs0ly /xUg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=iXOlbx38Y2q3OqEV1X8TI1ofGZ7AB3QTf0udX1e5zlI=; b=Q2s8G0VpUj8IPTE7/r85qkI8zLQ8rYFZx4nAhOhB3EYDCkgu1WGmj2+Jix2jqYJ299 PcXKypEMs/zc7ER7Zatc/zeiqUnlsZztySE9CE2PLjz0Mzs4Zr1ZrhwiguiPcOS6EjwW wX1lw2GoWRXA3yR5RIz2loxxBkeg6JzWJUrg/XulLaoSc3gYp0vXmZNqFzcm+xeF0Hdi wml86MuZe+uSBTp92Wn1xvpGIGeDEu3AlaoYOXyTiX1PV0nJM8i19riWf6+yZuzvD4IM 7g4mTqSuZQaXNq2HRcMpMBB5BvWI/O0eXjD9t5TBQtpYU6Iz/29UHSFS9wZQsJix0wyP GpaQ== X-Gm-Message-State: AOAM532CBE6bquZCBW0A1oDLK3Muje2+XgUvZuMbsVOMvwgypMOzqCag nNxQa/AVBqF4RAQRbUUawRFMOnu5DQhUpnr6cBBhBRs/LD8= X-Google-Smtp-Source: ABdhPJwK4gXr5J54x0BaMMo5PE+bLfCxCQBO1o98pkn2VyoFu541x9zeJzNMIfaaV6fQk+8K/MSi1F7XEgaik4p+AZI= X-Received: by 2002:a05:6e02:1d98:: with SMTP id h24mr5383494ila.94.1635155042535; Mon, 25 Oct 2021 02:44:02 -0700 (PDT) MIME-Version: 1.0 References: <20210926063302.1541193-1-feifei.wang2@arm.com> <20211020084523.1309177-1-feifei.wang2@arm.com> <20211020084523.1309177-2-feifei.wang2@arm.com> In-Reply-To: From: Jerin Jacob Date: Mon, 25 Oct 2021 15:13:36 +0530 Message-ID: To: Feifei Wang Cc: Ruifeng Wang , "Ananyev, Konstantin" , dpdk-dev , nd Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v4 1/5] eal: add new definitions for wait scheme 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 Sender: "dev" On Mon, Oct 25, 2021 at 3:01 PM Feifei Wang wrote: > > > -----=E9=82=AE=E4=BB=B6=E5=8E=9F=E4=BB=B6----- > > =E5=8F=91=E4=BB=B6=E4=BA=BA: Jerin Jacob > > =E5=8F=91=E9=80=81=E6=97=B6=E9=97=B4: Friday, October 22, 2021 8:10 AM > > =E6=94=B6=E4=BB=B6=E4=BA=BA: Feifei Wang > > =E6=8A=84=E9=80=81: Ruifeng Wang ; Ananyev, Konst= antin > > ; dpdk-dev ; nd > > > > =E4=B8=BB=E9=A2=98: Re: [dpdk-dev] [PATCH v4 1/5] eal: add new definiti= ons for wait scheme > > > > On Wed, Oct 20, 2021 at 2:16 PM Feifei Wang > > wrote: > > > > > > Introduce macros as generic interface for address monitoring. > > > > > > Signed-off-by: Feifei Wang > > > Reviewed-by: Ruifeng Wang > > > --- > > > lib/eal/arm/include/rte_pause_64.h | 126 > > > ++++++++++++++++------------ lib/eal/include/generic/rte_pause.h | > > > 32 +++++++ > > > 2 files changed, 104 insertions(+), 54 deletions(-) > > > > > > diff --git a/lib/eal/arm/include/rte_pause_64.h > > > b/lib/eal/arm/include/rte_pause_64.h > > > index e87d10b8cc..23954c2de2 100644 > > > --- a/lib/eal/arm/include/rte_pause_64.h > > > +++ b/lib/eal/arm/include/rte_pause_64.h > > > @@ -31,20 +31,12 @@ static inline void rte_pause(void) > > > /* Put processor into low power WFE(Wait For Event) state. */ > > > #define __WFE() { asm volatile("wfe" : : : "memory"); } > > > > > > -static __rte_always_inline void > > > -rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected, > > > - int memorder) > > > -{ > > > - uint16_t value; > > > - > > > - assert(memorder =3D=3D __ATOMIC_ACQUIRE || memorder =3D=3D > > __ATOMIC_RELAXED); > > > - > > > - /* > > > - * Atomic exclusive load from addr, it returns the 16-bit con= tent of > > > - * *addr while making it 'monitored',when it is written by so= meone > > > - * else, the 'monitored' state is cleared and a event is gene= rated > > > > a event -> an event in all the occurrence. > > > > > - * implicitly to exit WFE. > > > - */ > > > +/* > > > + * Atomic exclusive load from addr, it returns the 16-bit content of > > > + * *addr while making it 'monitored', when it is written by someone > > > + * else, the 'monitored' state is cleared and a event is generated > > > + * implicitly to exit WFE. > > > + */ > > > #define __LOAD_EXC_16(src, dst, memorder) { \ > > > if (memorder =3D=3D __ATOMIC_RELAXED) { \ > > > asm volatile("ldxrh %w[tmp], [%x[addr]]" \ @@ -58,6 > > > +50,52 @@ rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t > > expected, > > > : "memory"); \ > > > } } > > > > > > +/* > > > + * Atomic exclusive load from addr, it returns the 32-bit content of > > > + * *addr while making it 'monitored', when it is written by someone > > > + * else, the 'monitored' state is cleared and a event is generated > > > + * implicitly to exit WFE. > > > + */ > > > +#define __LOAD_EXC_32(src, dst, memorder) { \ > > > + if (memorder =3D=3D __ATOMIC_RELAXED) { \ > > > + asm volatile("ldxr %w[tmp], [%x[addr]]" \ > > > + : [tmp] "=3D&r" (dst) \ > > > + : [addr] "r"(src) \ > > > + : "memory"); \ > > > + } else { \ > > > + asm volatile("ldaxr %w[tmp], [%x[addr]]" \ > > > + : [tmp] "=3D&r" (dst) \ > > > + : [addr] "r"(src) \ > > > + : "memory"); \ > > > + } } > > > + > > > +/* > > > + * Atomic exclusive load from addr, it returns the 64-bit content of > > > + * *addr while making it 'monitored', when it is written by someone > > > + * else, the 'monitored' state is cleared and a event is generated > > > + * implicitly to exit WFE. > > > + */ > > > +#define __LOAD_EXC_64(src, dst, memorder) { \ > > > + if (memorder =3D=3D __ATOMIC_RELAXED) { \ > > > + asm volatile("ldxr %x[tmp], [%x[addr]]" \ > > > + : [tmp] "=3D&r" (dst) \ > > > + : [addr] "r"(src) \ > > > + : "memory"); \ > > > + } else { \ > > > + asm volatile("ldaxr %x[tmp], [%x[addr]]" \ > > > + : [tmp] "=3D&r" (dst) \ > > > + : [addr] "r"(src) \ > > > + : "memory"); \ > > > + } } > > > + > > > +static __rte_always_inline void > > > +rte_wait_until_equal_16(volatile uint16_t *addr, uint16_t expected, > > > + int memorder) > > > +{ > > > + uint16_t value; > > > + > > > + assert(memorder =3D=3D __ATOMIC_ACQUIRE || memorder =3D=3D > > > + __ATOMIC_RELAXED); > > > + > > > __LOAD_EXC_16(addr, value, memorder) > > > if (value !=3D expected) { > > > __SEVL() > > > @@ -66,7 +104,6 @@ rte_wait_until_equal_16(volatile uint16_t *addr, > > uint16_t expected, > > > __LOAD_EXC_16(addr, value, memorder) > > > } while (value !=3D expected); > > > } > > > -#undef __LOAD_EXC_16 > > > } > > > > > > static __rte_always_inline void > > > @@ -77,25 +114,6 @@ rte_wait_until_equal_32(volatile uint32_t *addr, > > > uint32_t expected, > > > > > > assert(memorder =3D=3D __ATOMIC_ACQUIRE || memorder =3D=3D > > > __ATOMIC_RELAXED); > > > > > > - /* > > > - * Atomic exclusive load from addr, it returns the 32-bit con= tent of > > > - * *addr while making it 'monitored',when it is written by so= meone > > > - * else, the 'monitored' state is cleared and a event is gene= rated > > > - * implicitly to exit WFE. > > > - */ > > > -#define __LOAD_EXC_32(src, dst, memorder) { \ > > > - if (memorder =3D=3D __ATOMIC_RELAXED) { \ > > > - asm volatile("ldxr %w[tmp], [%x[addr]]" \ > > > - : [tmp] "=3D&r" (dst) \ > > > - : [addr] "r"(src) \ > > > - : "memory"); \ > > > - } else { \ > > > - asm volatile("ldaxr %w[tmp], [%x[addr]]" \ > > > - : [tmp] "=3D&r" (dst) \ > > > - : [addr] "r"(src) \ > > > - : "memory"); \ > > > - } } > > > - > > > __LOAD_EXC_32(addr, value, memorder) > > > if (value !=3D expected) { > > > __SEVL() > > > @@ -104,7 +122,6 @@ rte_wait_until_equal_32(volatile uint32_t *addr, > > uint32_t expected, > > > __LOAD_EXC_32(addr, value, memorder) > > > } while (value !=3D expected); > > > } > > > -#undef __LOAD_EXC_32 > > > } > > > > > > static __rte_always_inline void > > > @@ -115,25 +132,6 @@ rte_wait_until_equal_64(volatile uint64_t *addr, > > > uint64_t expected, > > > > > > assert(memorder =3D=3D __ATOMIC_ACQUIRE || memorder =3D=3D > > > __ATOMIC_RELAXED); > > > > > > - /* > > > - * Atomic exclusive load from addr, it returns the 64-bit con= tent of > > > - * *addr while making it 'monitored',when it is written by so= meone > > > - * else, the 'monitored' state is cleared and a event is gene= rated > > > - * implicitly to exit WFE. > > > - */ > > > -#define __LOAD_EXC_64(src, dst, memorder) { \ > > > - if (memorder =3D=3D __ATOMIC_RELAXED) { \ > > > - asm volatile("ldxr %x[tmp], [%x[addr]]" \ > > > - : [tmp] "=3D&r" (dst) \ > > > - : [addr] "r"(src) \ > > > - : "memory"); \ > > > - } else { \ > > > - asm volatile("ldaxr %x[tmp], [%x[addr]]" \ > > > - : [tmp] "=3D&r" (dst) \ > > > - : [addr] "r"(src) \ > > > - : "memory"); \ > > > - } } > > > - > > > __LOAD_EXC_64(addr, value, memorder) > > > if (value !=3D expected) { > > > __SEVL() > > > @@ -143,6 +141,26 @@ rte_wait_until_equal_64(volatile uint64_t *addr, > > uint64_t expected, > > > } while (value !=3D expected); > > > } > > > } > > > + > > > +#define rte_wait_event(addr, mask, expected, cond, memorder, size) \ > > > > I think it is better to swap "cond" and "expected" positions to get bet= ter > > readability. > Thanks for the comments, it is better than before and I will update in th= e next version. > > > > rte_wait_event(&buf->bufptr64, RTE_DISTRIB_FLAGS_MASK, 0, !=3D, > > __ATOMIC_RELAXED, 64); > > > > Vs > > > > rte_wait_event(&buf->bufptr64, RTE_DISTRIB_FLAGS_MASK, !=3D, 0, > > __ATOMIC_RELAXED, 64); > > > > > +do { \ > > > > Any reason to not make an inline function instead of macro? > Because there were many new APIs for different cases. And then we refer t= o > Linux 'wait_event' code for an example. Please see the first version and = its discussion: > http://patches.dpdk.org/project/dpdk/cover/20210902053253.3017858-1-feife= i.wang2@arm.com/ OK. > > > > > + RTE_BUILD_BUG_ON(!__builtin_constant_p(memorder)); \ > > > > Should n't we add __builtin_constant_p(size) of check? > > Please see the discussion with Konstantin. > 'size' will not be as a parameter and then it is unnecessary to check it = with build_bug. Make sense to remove the 'size'. My comment was more in the direction of, if the 'size' is required to pass. > > > > > + RTE_BUILD_BUG_ON(memorder !=3D __ATOMIC_ACQUIRE && = \ > > > + memorder !=3D __ATOMIC_RELAXED); = \ > > > + RTE_BUILD_BUG_ON(size !=3D 16 && size !=3D 32 && size !=3D 64= ); \ > > > + uint##size_t value; > > > > > > \ > > > + __LOAD_EXC_##size(addr, value, memorder) \ > > > + if ((value & mask) cond expected) { \ > > > + __SEVL() \ > > > + do { \ > > > + __WFE() \ > > > + __LOAD_EXC_##size(addr, value, memorder) \ > > > + } while ((value & mask) cond expected); \ > > > + } \ > > > +} while (0) > > > + > > > +#undef __LOAD_EXC_16 > > > +#undef __LOAD_EXC_32 > > > #undef __LOAD_EXC_64 > > > > > > #undef __SEVL > > > diff --git a/lib/eal/include/generic/rte_pause.h > > > b/lib/eal/include/generic/rte_pause.h > > > index 668ee4a184..20a5d2a9fd 100644 > > > --- a/lib/eal/include/generic/rte_pause.h > > > +++ b/lib/eal/include/generic/rte_pause.h > > > @@ -111,6 +111,38 @@ rte_wait_until_equal_64(volatile uint64_t *addr, > > uint64_t expected, > > > while (__atomic_load_n(addr, memorder) !=3D expected) > > > rte_pause(); > > > } > > > + > > > +/* > > > + * Wait until *addr breaks the condition, with a relaxed memory > > > + * ordering model meaning the loads around this API can be reordered= . > > > + * > > > + * @param addr > > > + * A pointer to the memory location. > > > + * @param mask > > > + * A mask of value bits in interest. > > > + * @param expected > > > + * A 16-bit expected value to be in the memory location. > > > + * @param cond > > > + * A symbol representing the condition (=3D=3D, !=3D). > > > + * @param memorder > > > + * Two different memory orders that can be specified: > > > + * __ATOMIC_ACQUIRE and __ATOMIC_RELAXED. These map to > > > + * C++11 memory orders with the same names, see the C++11 standard > > > +or > > > + * the GCC wiki on atomic synchronization for detailed definition. > > > + * @param size > > > + * The bit size of *addr: > > > + * It is used for arm architecture to choose load instructions, > > > + * and the optional value is 16, 32 and 64. > > > + */ > > > +#define rte_wait_event(addr, mask, expected, cond, memorder, size) = \ > > > +do { = \ > > > + RTE_BUILD_BUG_ON(!__builtin_constant_p(memorder)); = \ > > > + RTE_BUILD_BUG_ON(memorder !=3D __ATOMIC_ACQUIRE && = \ > > > + memorder !=3D __ATOMIC_RELAXED); = \ > > > + RTE_BUILD_BUG_ON(size !=3D 16 && size !=3D 32 && size !=3D 64= ); \ > > > + while ((__atomic_load_n(addr, memorder) & mask) cond expected= ) \ > > > + rte_pause(); = \ > > > +} while (0) > > > #endif > > > > > > #endif /* _RTE_PAUSE_H_ */ > > > -- > > > 2.25.1 > > >