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 45DE6A0C4B; Sun, 31 Oct 2021 09:38:48 +0100 (CET) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3226E40DF8; Sun, 31 Oct 2021 09:38:48 +0100 (CET) Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by mails.dpdk.org (Postfix) with ESMTP id ACE2D40689 for ; Sun, 31 Oct 2021 09:38:46 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1635669526; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=DiQ6r32lvzB/KecQu+4DzhltjbUvOIpGP1womGPkmCo=; b=H4ew9g5attsXmRI9IWiqLnGXabV7Cdi0GL8BmgXMFwKu7cbG6bzC6qbi8Ys8bQPg5blAhF Hq9kTBpbvrpNR+2ZmTUSonaN68MjnpWscrNcC6Oh62C401G5ZQl7p2gJQbg7uO5uFJ3v+G tYRSFVF8q8nK0GhcMoNj6Ohmnoek/WE= Received: from mail-lj1-f197.google.com (mail-lj1-f197.google.com [209.85.208.197]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-64-yvWPE0QbPLqszyTC2-ZzSg-1; Sun, 31 Oct 2021 04:38:44 -0400 X-MC-Unique: yvWPE0QbPLqszyTC2-ZzSg-1 Received: by mail-lj1-f197.google.com with SMTP id 133-20020a2e098b000000b00212bd56bdccso2570229ljj.0 for ; Sun, 31 Oct 2021 01:38:44 -0700 (PDT) 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=DiQ6r32lvzB/KecQu+4DzhltjbUvOIpGP1womGPkmCo=; b=aerKeNX01LvSJwDYCHWuMrjdPoX0BgU51/AF7fdixJfG0AvyiMaX/WTGgX2oaQMr4O fX+E1OGa+DeoY9yIL+InQ/H+th5/HWO2e5bEbJb7JDb4tQvye+Z65bEZCgs82Dk6KKTa j4bJet0Hi9pyMkGgeI43bc7wMo6hciyV4CCjrCBk7Shk8aoIrwpS7myZQoenRZxJN9fR uX8ilg138Lm0CQfO8WbMtgJzTNJGh4zdJ7SnsfjI4DZuwSdg/pUodwgJpdzNwKoXx6p6 44Y+v9vnwq6jtVOKmqJpVxku2FOYnct0sIeAh8OKDmK9YWaP57taOhIvbyigB0kDknk3 8J4Q== X-Gm-Message-State: AOAM532BJp9JcGQdp/z3wP7Rd4MLaaxr/3BDif+/vsD2gsWkW7eYg1Yh OlS5RjgZbVKCBpY0IFAFl8DMEKk8jPSMP5UeJLfbOPBO8taCZy5k5DQFYMPW/29ee9Cp2efeYlJ wnUJBasepfku0IeoHKcs= X-Received: by 2002:a05:6512:6ce:: with SMTP id u14mr15886803lff.265.1635669523179; Sun, 31 Oct 2021 01:38:43 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwitOgIl+q0y/YaNu6vLlbeN1/vNllOQUjL1qZ7Q8cDQ4v+KImyVigo5sH42Y4D8Q+EDHr6dx488K946FARDkA= X-Received: by 2002:a05:6512:6ce:: with SMTP id u14mr15886788lff.265.1635669522905; Sun, 31 Oct 2021 01:38:42 -0700 (PDT) MIME-Version: 1.0 References: <20210902053253.3017858-1-feifei.wang2@arm.com> <20211029082021.945586-1-feifei.wang2@arm.com> <20211029082021.945586-2-feifei.wang2@arm.com> In-Reply-To: <20211029082021.945586-2-feifei.wang2@arm.com> From: David Marchand Date: Sun, 31 Oct 2021 09:38:31 +0100 Message-ID: To: Feifei Wang Cc: Ruifeng Wang , dev , nd , Jerin Jacob , Stephen Hemminger , Thomas Monjalon , =?UTF-8?Q?Mattias_R=C3=B6nnblom?= , "Ananyev, Konstantin" Authentication-Results: relay.mimecast.com; auth=pass smtp.auth=CUSA124A263 smtp.mailfrom=dmarchan@redhat.com X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Subject: Re: [dpdk-dev] [PATCH v8 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 Fri, Oct 29, 2021 at 10:20 AM Feifei Wang wrote: > > Introduce macros as generic interface for address monitoring. The main point of this patch is to add a new generic helper. > > Add '__LOAD_EXC_128' for size of 128. For different size, encapsulate > '__LOAD_EXC_16', '__LOAD_EXC_32', '__LOAD_EXC_64' and '__LOAD_EXC_128' > into a new macro '__LOAD_EXC'. ARM macros are just a result of introducing this new helper as a macro. I would not mention them. > > Furthermore, to prevent compilation warning in arm: > ---------------------------------------------- > 'warning: implicit declaration of function ...' > ---------------------------------------------- > Delete 'undef' constructions for '__LOAD_EXC_xx', '__SEVL' and '__WFE'. > And add =E2=80=98__RTE_ARM=E2=80=99 for these macros to fix the namespace= . > This is because original macros are undefine at the end of the file. > If new macro 'rte_wait_event' calls them in other files, they will be > seen as 'not defined'. About this new helper, it's rather confusing: - it is a macro, should be in capital letters, - "rte_wait_event(addr, mask, cond, expected)" waits until "*addr & mask cond expected" becomes false. I find this confusing. I would invert the condition. - so far, we had rte_wait_until_* helpers, rte_wait_event seems like a step backward as it seems to talk about the ARM stuff (wfe), - the masking part is artificial in some cases, at least let's avoid using a too generic name, we can decide to add a non-masked helper later. For those reasons, I'd prefer we have something like: /* * Wait until *addr & mask makes the condition true. With a relaxed memory * ordering model, the loads around this helper can be reordered. * * @param addr * A pointer to the memory location. * @param mask * A mask of *addr bits in interest. * @param cond * A symbol representing the condition. * @param expected * An expected value to be in the memory location. * @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. */ #define RTE_WAIT_UNTIL_MASKED(addr, mask, cond, expected, memorder) \ do { \ RTE_BUILD_BUG_ON(memorder !=3D __ATOMIC_ACQUIRE && \ memorder !=3D __ATOMIC_RELAXED); \ typeof(*(addr)) expected_value =3D expected; \ while (!((__atomic_load_n(addr, memorder) & (mask)) cond expected_value)) \ rte_pause(); \ } while (0) Comments below. > > Signed-off-by: Feifei Wang > Reviewed-by: Ruifeng Wang > Acked-by: Konstantin Ananyev > --- > lib/eal/arm/include/rte_pause_64.h | 202 +++++++++++++++++----------- > lib/eal/include/generic/rte_pause.h | 28 ++++ > 2 files changed, 154 insertions(+), 76 deletions(-) > > diff --git a/lib/eal/arm/include/rte_pause_64.h b/lib/eal/arm/include/rte= _pause_64.h > index e87d10b8cc..783c6aae87 100644 > --- a/lib/eal/arm/include/rte_pause_64.h > +++ b/lib/eal/arm/include/rte_pause_64.h [snip] > +/* > + * 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 an event is generated > + * implicitly to exit WFE. > + */ > +#define __RTE_ARM_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"); \ > + } } > + > +/* > + * Atomic exclusive load from addr, it returns the 128-bit content of > + * *addr while making it 'monitored', when it is written by someone > + * else, the 'monitored' state is cleared and an event is generated > + * implicitly to exit WFE. > + */ > +#define __RTE_ARM_LOAD_EXC_128(src, dst, memorder) { = \ > + volatile rte_int128_t *dst_128 =3D (volatile rte_int128_t *)&dst;= \ dst needs some () protection =3D> &(dst) Is volatile necessary? > + if (memorder =3D=3D __ATOMIC_RELAXED) { = \ > + asm volatile("ldxp %x[tmp0], %x[tmp1], [%x[addr]]" \ > + : [tmp0] "=3D&r" (dst_128->val[0]), = \ > + [tmp1] "=3D&r" (dst_128->val[1]) = \ > + : [addr] "r" (src) \ > + : "memory"); \ > + } else { \ > + asm volatile("ldaxp %x[tmp0], %x[tmp1], [%x[addr]]" \ > + : [tmp0] "=3D&r" (dst_128->val[0]), = \ > + [tmp1] "=3D&r" (dst_128->val[1]) = \ > + : [addr] "r" (src) \ > + : "memory"); \ > + } } \ > + > +#define __RTE_ARM_LOAD_EXC(src, dst, memorder, size) { \ > + RTE_BUILD_BUG_ON(size !=3D 16 && size !=3D 32 && size !=3D 64 \ > + && size !=3D 128); \ Indent should be one tab (idem in other places of this patch). Double tab is when we have line continuation in tests. > + if (size =3D=3D 16) \ > + __RTE_ARM_LOAD_EXC_16(src, dst, memorder) \ > + else if (size =3D=3D 32) \ > + __RTE_ARM_LOAD_EXC_32(src, dst, memorder) \ > + else if (size =3D=3D 64) \ > + __RTE_ARM_LOAD_EXC_64(src, dst, memorder) \ > + else if (size =3D=3D 128) \ > + __RTE_ARM_LOAD_EXC_128(src, dst, memorder) \ > +} > + [snip] > -#undef __LOAD_EXC_64 > > -#undef __SEVL > -#undef __WFE > +#define rte_wait_event(addr, mask, cond, expected, memorder) = \ > +do { = \ > + RTE_BUILD_BUG_ON(!__builtin_constant_p(memorder)); = \ Is this check on memorder being constant necessary? We have a build bug on, right after, would it not catch non constant cases? > + RTE_BUILD_BUG_ON(memorder !=3D __ATOMIC_ACQUIRE && = \ > + memorder !=3D __ATOMIC_RELAXED); = \ > + const uint32_t size =3D sizeof(*(addr)) << 3; = \ > + typeof(*(addr)) expected_value =3D (expected); = \ No need for () around expected. > + typeof(*(addr)) value; = \ > + __RTE_ARM_LOAD_EXC((addr), value, memorder, size) = \ No need for () around addr. > + if ((value & (mask)) cond expected_value) { = \ > + __RTE_ARM_SEVL() = \ > + do { = \ > + __RTE_ARM_WFE() = \ > + __RTE_ARM_LOAD_EXC((addr), value, memorder, size)= \ Idem. > + } while ((value & (mask)) cond expected_value); = \ > + } = \ > +} while (0) > > #endif > > diff --git a/lib/eal/include/generic/rte_pause.h b/lib/eal/include/generi= c/rte_pause.h > index 668ee4a184..d0c5b5a415 100644 > --- a/lib/eal/include/generic/rte_pause.h > +++ b/lib/eal/include/generic/rte_pause.h > @@ -111,6 +111,34 @@ rte_wait_until_equal_64(volatile uint64_t *addr, uin= t64_t expected, > while (__atomic_load_n(addr, memorder) !=3D expected) > rte_pause(); > } With this patch, ARM header goes though a conversion of assert() to compilation checks (build bug on). I don't see a reason not to do the same in generic header. As a result of this conversion, #include then can be removed. Though it triggers build failure on following files (afaics) who were implictly relying on this inclusion: drivers/net/ark/ark_ddm.c drivers/net/ark/ark_udm.c drivers/net/ice/ice_fdir_filter.c drivers/net/ionic/ionic_rxtx.c drivers/net/mlx4/mlx4_txq.c --=20 David Marchand