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 4FB51A034F; Wed, 28 Jul 2021 11:56:49 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 30A1640E64; Wed, 28 Jul 2021 11:56:49 +0200 (CEST) Received: from mail-wm1-f50.google.com (mail-wm1-f50.google.com [209.85.128.50]) by mails.dpdk.org (Postfix) with ESMTP id DD70740142 for ; Wed, 28 Jul 2021 11:56:47 +0200 (CEST) Received: by mail-wm1-f50.google.com with SMTP id l11-20020a7bc34b0000b029021f84fcaf75so3859093wmj.1 for ; Wed, 28 Jul 2021 02:56:47 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=6wind.com; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=8+nA2L3cFoY0vtTu4uMngIcM1yB4nx3xaOOTZ9QvJtE=; b=Ui+/Exbejjg5TOirJ+ikwLsftZqeNS4K1NtIFF6waiLc5PcN7iGOP0qZ+wZYoDb2vf z60Mu5GpWNxO8nKGDIDxa8/Nayq6vhLFXsmA4d32tpmDqP14Rr3Qqe0JH25pmQqEdLg5 1Il7jmGBzzCwX/PQ3pc0lye4RxxA02Df7PL2SjEPhYEPb/5JD7hRq5dEAseduQcT2JE1 FV2CE0LVljhwjewkpzQtWEClk0tu+w1oJ31JWppmT7Lf0u0qLKSRQPnsAJQle5NT1A1Q eleY2Hs7AuTL2JbL7LH46d5MhYszq2GgZflD4pG3GeTWmYCRWJUO8oyYTp9lH0gQc9wm KmIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=8+nA2L3cFoY0vtTu4uMngIcM1yB4nx3xaOOTZ9QvJtE=; b=cJ6IDSeyJoslaG6fhhdJCRfjRIOm/rainNB5VDZpApUpBnt/pP1tMUwbz7Ur+ikZ+/ +2u6zV1nCnqD689Y82A/En3/Npwd1TmHncDozlUPEaPApA2bJR8hY8DArce0yApkXrve hH5XiNOyPxuom/RKPomgoTZxQiRcXFK6qGGT2zi07t1ROso1h0uhGT0BWqqhXpJqhkQY Fn/xghjp2TBHdzu8XBU+hVOgamCGY+LSfgT+GuG0E0aAvAkG50bj/XdD3w8OSTVi8+1v 7W8IW9jhBGfjTqW0TQeb4Voifw+ScGsBv3pICipCx91scgehb1q3RoYVG8vT2icVAUBx Xk8w== X-Gm-Message-State: AOAM532WLZf9a6g/vzriwBimV4cTOHYPBM6AVdgd7zVpuyfO/2wN/SoF ln7mgGockHQtRDiKDHjXMTRtfQ== X-Google-Smtp-Source: ABdhPJzSnjegl4TnNuX440bujivpMERB2zlpb3DmOv4BJ6YHZZnG/8heLps05PfyFxNuVRFcdYql2g== X-Received: by 2002:a05:600c:3510:: with SMTP id h16mr27068069wmq.65.1627466207613; Wed, 28 Jul 2021 02:56:47 -0700 (PDT) Received: from 6wind.com ([2a01:e0a:5ac:6460:c065:401d:87eb:9b25]) by smtp.gmail.com with ESMTPSA id o18sm6317082wrx.21.2021.07.28.02.56.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 28 Jul 2021 02:56:47 -0700 (PDT) Date: Wed, 28 Jul 2021 11:56:46 +0200 From: Olivier Matz To: Joyce Kong Cc: thomas@monjalon.net, david.marchand@redhat.com, roretzla@linux.microsoft.com, stephen@networkplumber.org, andrew.rybchenko@oktetlabs.ru, harry.van.haaren@intel.com, honnappa.nagarahalli@arm.com, ruifeng.wang@arm.com, dev@dpdk.org, nd@arm.com Message-ID: References: <20210616025459.22717-1-joyce.kong@arm.com> <20210720035125.14214-1-joyce.kong@arm.com> <20210720035125.14214-5-joyce.kong@arm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20210720035125.14214-5-joyce.kong@arm.com> Subject: Re: [dpdk-dev] [PATCH v3 4/8] test/mcslock: use compiler atomics for lcores sync 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" Hi Joyce, On Mon, Jul 19, 2021 at 10:51:21PM -0500, Joyce Kong wrote: > Convert rte_atomic usages to compiler atomic built-ins for lcores > sync in mcslock testcases. > > Signed-off-by: Joyce Kong > Reviewed-by: Ruifeng Wang > Acked-by: Stephen Hemminger > --- > app/test/test_mcslock.c | 14 ++++++-------- > 1 file changed, 6 insertions(+), 8 deletions(-) > > diff --git a/app/test/test_mcslock.c b/app/test/test_mcslock.c > index 80eaecc90a..52e45e7e2a 100644 > --- a/app/test/test_mcslock.c > +++ b/app/test/test_mcslock.c > @@ -17,7 +17,6 @@ > #include > #include > #include > -#include > > #include "test.h" > > @@ -43,7 +42,7 @@ rte_mcslock_t *p_ml_perf; > > static unsigned int count; > > -static rte_atomic32_t synchro; > +static uint32_t synchro; > > static int > test_mcslock_per_core(__rte_unused void *arg) > @@ -76,8 +75,7 @@ load_loop_fn(void *func_param) > rte_mcslock_t ml_perf_me; > > /* wait synchro */ > - while (rte_atomic32_read(&synchro) == 0) > - ; > + rte_wait_until_equal_32(&synchro, 1, __ATOMIC_RELAXED); > > begin = rte_get_timer_cycles(); > while (lcount < MAX_LOOP) { > @@ -102,15 +100,15 @@ test_mcslock_perf(void) > const unsigned int lcore = rte_lcore_id(); > > printf("\nTest with no lock on single core...\n"); > - rte_atomic32_set(&synchro, 1); > + __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED); > load_loop_fn(&lock); > printf("Core [%u] Cost Time = %"PRIu64" us\n", > lcore, time_count[lcore]); > memset(time_count, 0, sizeof(time_count)); > > printf("\nTest with lock on single core...\n"); > + __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED); > lock = 1; > - rte_atomic32_set(&synchro, 1); nit: is there a reason for moving this line? > load_loop_fn(&lock); > printf("Core [%u] Cost Time = %"PRIu64" us\n", > lcore, time_count[lcore]); > @@ -118,11 +116,11 @@ test_mcslock_perf(void) > > printf("\nTest with lock on %u cores...\n", (rte_lcore_count())); > > - rte_atomic32_set(&synchro, 0); > + __atomic_store_n(&synchro, 0, __ATOMIC_RELAXED); > rte_eal_mp_remote_launch(load_loop_fn, &lock, SKIP_MAIN); > > /* start synchro and launch test on main */ > - rte_atomic32_set(&synchro, 1); > + __atomic_store_n(&synchro, 1, __ATOMIC_RELAXED); > load_loop_fn(&lock); I have a more general question. Please forgive my ignorance about the C++11 atomic builtins and memory model. Both gcc manual and C11 standard are not that easy to understand :) In all the patches of this patchset, __ATOMIC_RELAXED is used. My understanding is that it does not add any inter-thread ordering constraint. I suppose that in this particular case, we rely on the call to rte_eal_mp_remote_launch() being a compiler barrier, and the function itself to be a memory barrier. This ensures that worker threads sees synchro=0 until it is set to 1 by the master. Is it correct? What is the reason for using the atomic API here? Wouldn't a standard affectation work too? (I mean "synchro = 1;") > > rte_eal_mp_wait_lcore(); > -- > 2.17.1 >