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 E2ED9A00C5; Fri, 8 Jul 2022 18:21:14 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id C2342406B4; Fri, 8 Jul 2022 18:21:14 +0200 (CEST) Received: from mga12.intel.com (mga12.intel.com [192.55.52.136]) by mails.dpdk.org (Postfix) with ESMTP id 04D054021E for ; Fri, 8 Jul 2022 18:21:12 +0200 (CEST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1657297273; x=1688833273; h=date:from:to:cc:subject:message-id:references: mime-version:content-transfer-encoding:in-reply-to; bh=4+PZKSyhDwUqAkt9qtpFVpol1IC18fleLZ9XuVFCxjw=; b=WoTPyQWDxK9TZRdfFcoJT9KucLklOtEBJnM5vZpVwT/7Hddm4dbOtjys T9jdBCKt87Ei/RVy70bMZsmnmDqCZRqvPiLebUM4FqTa9aMrmxaMYReVy qdnI89ze1bnPGHxX/Ge9my2w94VvVj9UUQSeC9CFZbzYTM/IdqLI/2832 6W0TyziotnMm2ejJyjtYXfwu7EbfB0bkVzUujQMjbGXnBZ2dbga2Qv9N3 MQA18T8jESN5Rl0kkiIH309uQQ6vwPMcpNSWluq4+xgb4lSEve9GC9fo6 wMwyRgoijvAEgqw+5GaagVUM6MHSwrZyHvbAdQjbMqo/H6nkBv6BuZQO8 A==; X-IronPort-AV: E=McAfee;i="6400,9594,10402"; a="264095740" X-IronPort-AV: E=Sophos;i="5.92,256,1650956400"; d="scan'208";a="264095740" Received: from orsmga006.jf.intel.com ([10.7.209.51]) by fmsmga106.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 08 Jul 2022 09:21:11 -0700 X-IronPort-AV: E=Sophos;i="5.92,256,1650956400"; d="scan'208";a="568995046" Received: from bricha3-mobl.ger.corp.intel.com ([10.55.133.37]) by orsmga006-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 08 Jul 2022 09:21:09 -0700 Date: Fri, 8 Jul 2022 17:21:06 +0100 From: Bruce Richardson To: "Van Haaren, Harry" Cc: Honnappa Nagarahalli , "dev@dpdk.org" , "mattias.ronnblom" , Morten =?iso-8859-1?Q?Br=F8rup?= , nd Subject: Re: [PATCH 2/2] service: fix potential stats race-condition on MT services Message-ID: References: <20220708125645.3141464-1-harry.van.haaren@intel.com> <20220708125645.3141464-2-harry.van.haaren@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: 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 Fri, Jul 08, 2022 at 03:31:01PM +0000, Van Haaren, Harry wrote: > > -----Original Message----- > > From: Honnappa Nagarahalli > > Sent: Friday, July 8, 2022 4:16 PM > > To: Van Haaren, Harry ; dev@dpdk.org > > Cc: mattias.ronnblom ; Morten Brørup > > ; nd ; nd > > Subject: RE: [PATCH 2/2] service: fix potential stats race-condition on MT services > > > > > > diff --git a/lib/eal/common/rte_service.c b/lib/eal/common/rte_service.c > > > index ef31b1f63c..f045e74ef3 100644 > > > --- a/lib/eal/common/rte_service.c > > > +++ b/lib/eal/common/rte_service.c > > > @@ -363,9 +363,15 @@ service_runner_do_callback(struct > > > rte_service_spec_impl *s, > > > uint64_t start = rte_rdtsc(); > > > s->spec.callback(userdata); > > > uint64_t end = rte_rdtsc(); > > > - s->cycles_spent += end - start; > > > + uint64_t cycles = end - start; > > > cs->calls_per_service[service_idx]++; > > > - s->calls++; > > > + if (service_mt_safe(s)) { > > > + __atomic_fetch_add(&s->cycles_spent, cycles, > > > __ATOMIC_RELAXED); > > > + __atomic_fetch_add(&s->calls, 1, > > > __ATOMIC_RELAXED); > > > + } else { > > > + s->cycles_spent += cycles; > > > + s->calls++; > > This is still a problem from a reader perspective. It is possible that the writes could be > > split while a reader is reading the stats. These need to be atomic adds. > > Thanks for pointing out; I do "think" in x86 in terms of load/store tearing; and on x86 > naturally aligned load/stores will not tear. Apologies for missing the ARM angle here. > > I'm not sure how to best encode the difference between tearing & "locked instructions" > to make things multi-writer safe. But they're not the same thing, and I'd prefer not pay > the penalty for LOCK instructions (multi-writer) only to satisfy the non-tearing requirements. > > Is there an rte_atomic-* type that is guaranteed as non-tearing? > > In that case, changing the type of the calls/cycles_spent variables to such a type to ensure "non-tearing" > single-reader, single-writer behaviour is enough, instead of forcing __atomic_fetch_add() everywhere? Regular read, increment and then atomic store should work without locks where alignment is correct on most architectures, and doing the store atomically should prevent any tearing.