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 DE335A00C5; Fri, 8 Jul 2022 22:02:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 7AE79406B4; Fri, 8 Jul 2022 22:02:30 +0200 (CEST) Received: from mail.lysator.liu.se (mail.lysator.liu.se [130.236.254.3]) by mails.dpdk.org (Postfix) with ESMTP id B27614021E for ; Fri, 8 Jul 2022 22:02:29 +0200 (CEST) Received: from mail.lysator.liu.se (localhost [127.0.0.1]) by mail.lysator.liu.se (Postfix) with ESMTP id 033B012D30 for ; Fri, 8 Jul 2022 22:02:28 +0200 (CEST) Received: by mail.lysator.liu.se (Postfix, from userid 1004) id 01C441303E; Fri, 8 Jul 2022 22:02:27 +0200 (CEST) X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on hermod.lysator.liu.se X-Spam-Level: X-Spam-Status: No, score=-1.8 required=5.0 tests=ALL_TRUSTED, AWL, NICE_REPLY_A, T_SCC_BODY_TEXT_LINE autolearn=disabled version=3.4.6 X-Spam-Score: -1.8 Received: from [192.168.1.59] (unknown [62.63.215.114]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange ECDHE (P-256) server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by mail.lysator.liu.se (Postfix) with ESMTPSA id 9110B12E43; Fri, 8 Jul 2022 22:02:25 +0200 (CEST) Message-ID: Date: Fri, 8 Jul 2022 22:02:24 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.9.1 Subject: Re: [PATCH 2/2] service: fix potential stats race-condition on MT services Content-Language: en-US To: Bruce Richardson , "Van Haaren, Harry" Cc: Honnappa Nagarahalli , "dev@dpdk.org" , "mattias.ronnblom" , =?UTF-8?Q?Morten_Br=c3=b8rup?= , nd References: <20220708125645.3141464-1-harry.van.haaren@intel.com> <20220708125645.3141464-2-harry.van.haaren@intel.com> From: =?UTF-8?Q?Mattias_R=c3=b6nnblom?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Virus-Scanned: ClamAV using ClamSMTP 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 2022-07-08 18:21, Bruce Richardson wrote: > 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. This is a good pattern, and provides a noticeable performance benefit compared to using an atomic add (which is an overkill in single-writer scenarios), in my experience. The DPDK seqcount implementation increments the count in this manner.