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 046164280A; Wed, 22 Mar 2023 15:21:37 +0100 (CET) Received: from mails.dpdk.org (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id D401F40E09; Wed, 22 Mar 2023 15:21:36 +0100 (CET) Received: from linux.microsoft.com (linux.microsoft.com [13.77.154.182]) by mails.dpdk.org (Postfix) with ESMTP id F3DC640A84 for ; Wed, 22 Mar 2023 15:21:34 +0100 (CET) Received: by linux.microsoft.com (Postfix, from userid 1086) id 3E66B20FBA69; Wed, 22 Mar 2023 07:21:34 -0700 (PDT) DKIM-Filter: OpenDKIM Filter v2.11.0 linux.microsoft.com 3E66B20FBA69 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linux.microsoft.com; s=default; t=1679494894; bh=8Od0fV2H79kcpO5LntapEQlsIowOd0T2WU76Q8Hi2k8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=GqGWrU2lLjgnn6iFgXeP+tSiWLp3xxw7yanV79mGDJhdwI9XJisajpE+D0CJI6O2f WPzG98UwQxVERTAaqQKzZw+qP269dqmTDBRJZIC+/YuZP3LLwtxb2niyAIQRoYnZmx UC6lfFMDo9u/bKGOxmq2PmClx3T04HHYtDQsD4Nw= Date: Wed, 22 Mar 2023 07:21:34 -0700 From: Tyler Retzlaff To: Morten =?iso-8859-1?Q?Br=F8rup?= Cc: Stephen Hemminger , dev@dpdk.org, Honnappa.Nagarahalli@arm.com, Ruifeng.Wang@arm.com, thomas@monjalon.net Subject: Re: [PATCH 0/7] replace rte atomics with GCC builtin atomics Message-ID: <20230322142134.GA29057@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> References: <1679084388-19267-1-git-send-email-roretzla@linux.microsoft.com> <20230317144226.2f26bad1@hermes.local> <20230317214910.GA31884@linuxonhyperv3.guj3yctzbm1etfxqx2vob5hsef.xx.internal.cloudapp.net> <98CBD80474FA8B44BF855DF32C47DC35D877E1@smartserver.smartshare.dk> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <98CBD80474FA8B44BF855DF32C47DC35D877E1@smartserver.smartshare.dk> User-Agent: Mutt/1.5.21 (2010-09-15) 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 Wed, Mar 22, 2023 at 12:28:44PM +0100, Morten Brørup wrote: > > From: Tyler Retzlaff [mailto:roretzla@linux.microsoft.com] > > Sent: Friday, 17 March 2023 22.49 > > > > On Fri, Mar 17, 2023 at 02:42:26PM -0700, Stephen Hemminger wrote: > > > On Fri, 17 Mar 2023 13:19:41 -0700 > > > Tyler Retzlaff wrote: > > > > > > > Replace the use of rte_atomic.h types and functions, instead use GCC > > > > supplied C++11 memory model builtins. > > > > > > > > This series covers the libraries and drivers that are built on Windows. > > > > > > > > The code has be converted to use the __atomic builtins but there are > > > > additional during conversion i notice that there may be some issues > > > > that need to be addressed. > > > > > > I don't think all these cmpset need to use SEQ_CST. > > > Especially for the places where it is used a loop, might > > > be more efficient with some of the other memory models. > > > > i agree. > > > > however, i'm not trying to improve the code with this change, just > > decouple it from rte_atomics.h so trying my best to avoid any > > unnecessary semantic change. > > > > certainly if the maintainers of this code wish to weaken the ordering > > where appropriate after the change is merged they can do so and handily > > this change has enabled them to do so easily allowing them to test just > > their change in isolation. > > I agree with the two-step approach, where this first step is a simple search-and-replacement; but I insist that you add a FIXME or similar note where you have blindly used SEQ_CST, indicating that the memory order needs to be reviewed and potentially corrected. i think the maintainers need to take some responsibility, if they see optimizations they missed when previously writing the code they need to follow up with a patch themselves. i can't do everything for them and marking things i'm not sure about will only lead to me having to churn patch series to remove the unwanted comments later. keep in mind i have to touch each of these again when converting to standard so that's a better time to review ~everything in more detail because when converting to standard that's when suddenly you get a bunch of code generation that is "fallback" to seq_cst that isn't happening now. the series that converts to standard needs to be up for review as soon as possible to maximize available time for feedback before 23.11 so it would be better to get the simpler cut & paste normalizing the code out of the way to unblock that series submission. > > Also, in a couple of the drivers, you are using int64_t for packet counters. These cannot be negative and should be uint64_t. And AFAIK, such counters can use RELAXED memory order. i know you don't mean to say i selected the types and rather that the types that were selected are not quite correct for their usage. again on the review that actually adopts std atomics is a better place to make any potential type changes since we are "breaking" the API for 23.11 anyway. further, the std atomics series technically changes all the types so it's probably better to make one type change then rather than one now and one later. i think it would be best to get these validated and merged asap so we can get to the std atomics review. when that series is up let's discuss further how i can mark areas of concern, with that series i expect there will have to be some changes in order to avoid minor regressions. thanks!