From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752255AbdKHCHK (ORCPT ); Tue, 7 Nov 2017 21:07:10 -0500 Received: from mail-wm0-f53.google.com ([74.125.82.53]:54228 "EHLO mail-wm0-f53.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751081AbdKHCHI (ORCPT ); Tue, 7 Nov 2017 21:07:08 -0500 X-Google-Smtp-Source: ABhQp+Sp6+D4NDMrUysea7IN8cDCf+FHfkJvji1lFPBD5MY7/98qxyOpdHDl0tI0ne+RwOVivLiAKA== X-ME-Sender: Date: Wed, 8 Nov 2017 10:08:34 +0800 From: Boqun Feng To: Andreas Dilger Cc: Jan Kara , Davidlohr Bueso , Waiman Long , Alexander Viro , Jan Kara , Jeff Layton , "J. Bruce Fields" , Tejun Heo , Christoph Lameter , linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, Ingo Molnar , Peter Zijlstra , Andi Kleen , Dave Chinner Subject: Re: [PATCH v4] lib/dlock-list: Scale dlock_lists_empty() Message-ID: <20171108020834.GC6280@tardis> References: <1509475860-16139-1-git-send-email-longman@redhat.com> <1509475860-16139-2-git-send-email-longman@redhat.com> <20171102170431.oq3i5mxtjcg53uot@linux-n805> <81bb3365-63f3-fea8-d238-e3880a4c8033@redhat.com> <20171103133420.pngmrsfmtimataz4@linux-n805> <20171103142254.d55bu2n44xe4aruf@linux-n805> <20171106184708.kmwfcchjwjzucuja@linux-n805> <20171107115921.GC11391@quack2.suse.cz> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="96YOpH+ONegL0A3E" Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.9.1 (2017-09-22) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org --96YOpH+ONegL0A3E Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Tue, Nov 07, 2017 at 10:59:29AM -0700, Andreas Dilger wrote: > On Nov 7, 2017, at 4:59 AM, Jan Kara wrote: > > On Mon 06-11-17 10:47:08, Davidlohr Bueso wrote: > >> + /* > >> + * Serialize dlist->used_lists such that a 0->1 transition is not > >> + * missed by another thread checking if any of the dlock lists are > >> + * used. > >> + * > >> + * CPU0 CPU1 > >> + * dlock_list_add() dlock_lists_empty() > >> + * [S] atomic_inc(used_lists); > >> + * smp_mb__after_atomic(); > >> + * smp_mb__before_atomic(); > >> + * [L] atomic_read(used_lists) > >> + * list_add() > >> + */ > >> + smp_mb__before_atomic(); > >> + return !atomic_read(&dlist->used_lists); >=20 > Just a general kernel programming question here - I thought the whole poi= nt > of atomics is that they are, well, atomic across all CPUs so there is no > need for a memory barrier? If there is a need for a memory barrier for > each atomic access (assuming it isn't accessed under another lock, which = would > make the use of atomic types pointless, IMHO) then I'd think there is a l= ot > of code in the kernel that isn't doing this properly. >=20 > What am I missing here? >=20 > I don't see how this helps if the operations are executed like: >=20 > * CPU0 CPU1 > * dlock_list_add() dlock_lists_empty() > * [S] atomic_inc(used_lists); > * smp_mb__before_atomic(); > * smp_mb__after_atomic(); > * [L] atomic_read(used_lists) >=20 > or alternately like: >=20 > * CPU0 CPU1 > * dlock_list_add() dlock_lists_empty() > * smp_mb__before_atomic(); > * [S] atomic_inc(used_lists); > * smp_mb__after_atomic(); > * [L] atomic_read(used_lists) >=20 Or worse: * CPU0 CPU1 * dlock_list_add() dlock_lists_empty() * smp_mb__before_atomic(); * [L] atomic_read(used_lists) * [S] atomic_inc(used_lists); * smp_mb__after_atomic(); , in which case dlock_lists_empty() misses a increment of used_lists. That said, this may be fine for the usage of dlock_list. But I think the comments are confusing or wrong. The reason is you can not use barriers to order two memory ops on different CPUs, and usually we add comments like: [S] ... [S] ... [L] ... [L] ... Davidlohr, could you try to improve the comments by finding both memory ops preceding and following the barriers? Maybe you will find those barriers are not necessary in the end. Regards, Boqun > then the same problem would exist, unless those functions/macros are some= how > bound to the atomic operations themselves? In that case, what makes the = use > of atomic_{inc,dec,read}() in other parts of the code safe without them? >=20 > Cheers, Andreas >=20 >=20 >=20 >=20 >=20 --96YOpH+ONegL0A3E Content-Type: application/pgp-signature; name="signature.asc" -----BEGIN PGP SIGNATURE----- iQEzBAABCAAdFiEEj5IosQTPz8XU1wRHSXnow7UH+rgFAloCZxYACgkQSXnow7UH +risGQgArL4zJjo0x7/dYOr22cV9YhO9/6Vvvj/7lusHe1DiOgeNAIDZFRSulDCw 8p0WTD9snNSF2BJvW+VGTPEMNAM+k7TLi9gWSnLQi81Aheq/m/0HEnHYi6YQ8Xmn fQJFvTJlpPBDaUMiZbRrU4yFSGhPGymLxmxvp5O89K9rpyiC8b2gaqt0TwVmUVqN nU4z53fp2AUgpATzDG436khH1ZEbCoxuatuCLj6bS8KmW+P4MrP/JvcOz1FOFmsC EUt4qndSqGoBOIp4qdigfAfmaZ1vqrqOYfX2n/gbOkYqyk2paUSYg0yc7NN/RIo7 lHeHTsnOSyximRZKKkulbim5ulCoSA== =egwX -----END PGP SIGNATURE----- --96YOpH+ONegL0A3E--