From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DA05BC76196 for ; Tue, 28 Mar 2023 19:43:11 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229648AbjC1TnK (ORCPT ); Tue, 28 Mar 2023 15:43:10 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58554 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229436AbjC1TnJ (ORCPT ); Tue, 28 Mar 2023 15:43:09 -0400 Received: from mail-ed1-x52a.google.com (mail-ed1-x52a.google.com [IPv6:2a00:1450:4864:20::52a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 141DA18B for ; Tue, 28 Mar 2023 12:43:08 -0700 (PDT) Received: by mail-ed1-x52a.google.com with SMTP id ek18so54246271edb.6 for ; Tue, 28 Mar 2023 12:43:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20210112; t=1680032586; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=nqIPDr+ZqfoCi8eQc3DTNr+ThF98a/RBvzrvNFqWYDM=; b=TIEXaRQDBAZ6mBWE2R4TELhNaijNMDedZH3kg6n0ljtndAxJLXXqyFQwcErH6B46Lz U9BN+2qoq3bDs+Ypbo1k5NzI/RQIygK7ia03C7cfrRO0ByWfBXsdRCT0alHvfQTfmvRe qv9IocyC7Cjb3idOoIKKeNX5YVur0irro0khGXutbTnhV67ua5rmPZ8USHumeZuOnxd8 bw+LUtnZMfNIRTh+++D2tITNO4f5XHgKLCv401nXpXS49KzLZKP/0ycEI4wvThEYgq34 SdVY6fUmkfV2iCF/Uacl+jKfcVLSr1puNKsWanMsSlWfcXyo+ZpeNqbzs0La62nvhfVM xa2w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; t=1680032586; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=nqIPDr+ZqfoCi8eQc3DTNr+ThF98a/RBvzrvNFqWYDM=; b=omXyb4Za99RUvpZpLxzRE4xIwde8HFlku0l8wRmU0Exjo9eJhoqLBzYIaO/q+EUFGv SNfipqabW3JcivD1tUnIXdAzRpczOKAFv4MGqLP3cbWt9hnOgeWtRe7f2CXmPIMXcCGI 3/y4Jl0iNJQkut9+mrkchfSqxArXys8oeB6AEcYxnyY9T+LaSNzwkV3shC11UqCZ1kJ1 dlxU46ALSCiCqAZDh5hjYNN+zKHQeFpmZfAkc3yzGmg79O4iX7lNO8AIBAIQfRtfieJc LUXb6tKnTkha8GKx5qcZMFdrlWdz+wIcEIFPLVhsIhNPbJ9GegvhwhahHNYau66IuEt7 HOIQ== X-Gm-Message-State: AAQBX9eGtNEnSXrBxGuZPe+H0j55BkkMWi5OLlZ3UzZX9mhDuIYhun4P ZWv/kBdaY479KgBzj27ZhnUObFkddPnE3IIdrqtZSQ== X-Google-Smtp-Source: AKy350Y+r8YQubYTs8R7PJEitaPs1q0OlTz1iemnZQ7KI+3/VnM/MXrQyknZBxEGhPVmnt+/5hUtEtoV/kv4VWDou8E= X-Received: by 2002:a50:a444:0:b0:500:5463:35de with SMTP id v4-20020a50a444000000b00500546335demr8511392edb.8.1680032586465; Tue, 28 Mar 2023 12:43:06 -0700 (PDT) MIME-Version: 1.0 References: <20230328061638.203420-1-yosryahmed@google.com> <20230328061638.203420-6-yosryahmed@google.com> <20230328141523.txyhl7wt7wtvssea@google.com> In-Reply-To: From: Yosry Ahmed Date: Tue, 28 Mar 2023 12:42:30 -0700 Message-ID: Subject: Re: [PATCH v1 5/9] memcg: replace stats_flush_lock with an atomic To: Shakeel Butt Cc: Tejun Heo , Josef Bacik , Jens Axboe , Zefan Li , Johannes Weiner , Michal Hocko , Roman Gushchin , Muchun Song , Andrew Morton , =?UTF-8?Q?Michal_Koutn=C3=BD?= , Vasily Averin , cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, bpf@vger.kernel.org Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org On Tue, Mar 28, 2023 at 12:34=E2=80=AFPM Yosry Ahmed wrote: > > On Tue, Mar 28, 2023 at 12:28=E2=80=AFPM Shakeel Butt wrote: > > > > On Tue, Mar 28, 2023 at 11:53=E2=80=AFAM Yosry Ahmed wrote: > > > > > [...] > > > > > + if (atomic_xchg(&stats_flush_ongoing, 1)) > > > > > > > > Have you profiled this? I wonder if we should replace the above wit= h > > > > > > > > if (atomic_read(&stats_flush_ongoing) || atomic_xchg(&stats= _flush_ongoing, 1)) > > > > > > I profiled the entire series with perf and I haven't noticed a notabl= e > > > difference between before and after the patch series -- but maybe som= e > > > specific access patterns cause a regression, not sure. > > > > > > Does an atomic_cmpxchg() satisfy the same purpose? it's easier to rea= d > > > / more concise I guess. > > > > > > Something like > > > > > > if (atomic_cmpxchg(&stats_flush_ongoing, 0, 1)) > > > > > > WDYT? > > > > > > > No, I don't think cmpxchg will be any different from xchg(). On x86, > > the cmpxchg will always write to stats_flush_ongoing and depending on > > the comparison result, it will either be 0 or 1 here. > > > > If you see the implementation of queued_spin_trylock(), it does the > > same as well. > > Interesting. I thought cmpxchg by definition will compare first and > only do the write if stats_flush_ongoing =3D=3D 0 in this case. > > I thought queued_spin_trylock() was doing an atomic_read() first to > avoid the LOCK instruction unnecessarily the lock is held by someone > else. Anyway, perhaps it's better to follow what queued_spin_trylock() is doing, even if only to avoid locking the cache line unnecessarily. (Although now that I think about it, I wonder why atomic_cmpxchg doesn't do this by default, food for thought)