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 X-Spam-Level: X-Spam-Status: No, score=-2.0 required=3.0 tests=DKIM_INVALID,DKIM_SIGNED, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS, URIBL_BLOCKED,USER_AGENT_SANE_2 autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 9A3DBC4360C for ; Wed, 25 Sep 2019 20:32:54 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id 4F51121D7F for ; Wed, 25 Sep 2019 20:32:54 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=fail reason="signature verification failed" (2048-bit key) header.d=lca.pw header.i=@lca.pw header.b="p2CU4vi2" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 4F51121D7F Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=lca.pw Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=owner-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix) id DE5966B0006; Wed, 25 Sep 2019 16:32:53 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id D96186B0007; Wed, 25 Sep 2019 16:32:53 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id C5EF66B0008; Wed, 25 Sep 2019 16:32:53 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0047.hostedemail.com [216.40.44.47]) by kanga.kvack.org (Postfix) with ESMTP id A41946B0006 for ; Wed, 25 Sep 2019 16:32:53 -0400 (EDT) Received: from smtpin24.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with SMTP id 42A498243762 for ; Wed, 25 Sep 2019 20:32:53 +0000 (UTC) X-FDA: 75974591826.24.feast64_535644a83c255 X-HE-Tag: feast64_535644a83c255 X-Filterd-Recvd-Size: 8949 Received: from mail-qt1-f193.google.com (mail-qt1-f193.google.com [209.85.160.193]) by imf46.hostedemail.com (Postfix) with ESMTP for ; Wed, 25 Sep 2019 20:32:52 +0000 (UTC) Received: by mail-qt1-f193.google.com with SMTP id w14so12449qto.9 for ; Wed, 25 Sep 2019 13:32:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=lca.pw; s=google; h=message-id:subject:from:to:cc:date:in-reply-to:references :mime-version:content-transfer-encoding; bh=J57wOkNytV0R3QXZeQjJRBC086OiS7uIcVQyHpK+i0g=; b=p2CU4vi2sWx+7UfJkiFNoo8vpM7H8Gf/T/0I97xj/7zbDLPQlSn9GV3RFA0Fj5cdQp 4uTlcQvd2MbdxdwH3RpzeDp/6IUcCxFZD62HYfxIqlXJmReN4Xuloi18elQuOwGPmnFu uCAhwdz3tjTJ5VUy4hxxkEq8f+dk5kHg8MuHYL4RCmn1eCuNw4BTaKaKrK8Tncu440ZI vG0nX5MyEYNFq1FFDrDi0OX7Mds39gJPHxH1U2eQjF1nxn4xVoIs6h9PY9oEiY7JUrmC iBEG4Uc9jUHgoFcmgTB8tqBTSwe5HTUsOjQz5ub2SgMpSVSKAs1WYVtVv0g8jbZW3TKC PzIg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:message-id:subject:from:to:cc:date:in-reply-to :references:mime-version:content-transfer-encoding; bh=J57wOkNytV0R3QXZeQjJRBC086OiS7uIcVQyHpK+i0g=; b=f7u356B+3mivkEsAnMcdszKbjstSTfgbRb27zVCWRFPCc1ppKXb9MrejNGgNygTZi4 BJobCRm0uPXxLR9shASylucHU0UKtCzhQJdykqT1sb93RZQMY8ZXx1IAtgX7A9TdeNC7 plrG6obJOe+RWRhdPgnVxCvhakVuUOT5/0vx/6/65kJr+3ajSvZKGj6OPkOe1WsbFWO4 pxzGvFMrOYDC0iOz2YOmLBMwA8husU6Rdik9KjYLPjCF6zWyvN8f0pgjSvcYo0Fh87Mv +u6SnDZwqNiU0UdSNdf5/I6eQn96nR6BM20ZPhYjeGxICnKExY6Y35uQKZlwb+dGpJZR 1TSQ== X-Gm-Message-State: APjAAAVUV63MSrBNQS+eStY7Slwge4urWzmSzpaptf43J7R56W3DXT3W hbeOusbAjEf+JsYr6ANPFlpP5A== X-Google-Smtp-Source: APXvYqx3EbclPFEDN8LDEQbR3sOz9fbH21aVNcvda7KZXbguZB+0NrEGz7nKgjTNZY2vgLlU0vnVaw== X-Received: by 2002:ac8:5147:: with SMTP id h7mr298272qtn.117.1569443571714; Wed, 25 Sep 2019 13:32:51 -0700 (PDT) Received: from dhcp-41-57.bos.redhat.com (nat-pool-bos-t.redhat.com. [66.187.233.206]) by smtp.gmail.com with ESMTPSA id 199sm3507177qkk.112.2019.09.25.13.32.49 (version=TLS1_2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Wed, 25 Sep 2019 13:32:50 -0700 (PDT) Message-ID: <1569443568.5576.231.camel@lca.pw> Subject: Re: [PATCH v1] mm/memory_hotplug: Don't take the cpu_hotplug_lock From: Qian Cai To: David Hildenbrand , Michal Hocko Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Oscar Salvador , Pavel Tatashin , Dan Williams , Thomas Gleixner Date: Wed, 25 Sep 2019 16:32:48 -0400 In-Reply-To: <92bce3d4-0a3e-e157-529d-35aafbc30f3b@redhat.com> References: <20190924143615.19628-1-david@redhat.com> <1569337401.5576.217.camel@lca.pw> <20190924151147.GB23050@dhcp22.suse.cz> <1569351244.5576.219.camel@lca.pw> <2f8c8099-8de0-eccc-2056-a79d2f97fbf7@redhat.com> <1569427262.5576.225.camel@lca.pw> <20190925174809.GM23050@dhcp22.suse.cz> <1569435659.5576.227.camel@lca.pw> <92bce3d4-0a3e-e157-529d-35aafbc30f3b@redhat.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.22.6 (3.22.6-10.el7) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Wed, 2019-09-25 at 21:48 +0200, David Hildenbrand wrote: > On 25.09.19 20:20, Qian Cai wrote: > > On Wed, 2019-09-25 at 19:48 +0200, Michal Hocko wrote: > > > On Wed 25-09-19 12:01:02, Qian Cai wrote: > > > > On Wed, 2019-09-25 at 09:02 +0200, David Hildenbrand wrote: > > > > > On 24.09.19 20:54, Qian Cai wrote: > > > > > > On Tue, 2019-09-24 at 17:11 +0200, Michal Hocko wrote: > > > > > > > On Tue 24-09-19 11:03:21, Qian Cai wrote: > > > > > > > [...] > > > > > > > > While at it, it might be a good time to rethink the whole= locking over there, as > > > > > > > > it right now read files under /sys/kernel/slab/ could tri= gger a possible > > > > > > > > deadlock anyway. > > > > > > > >=20 > > > > > > >=20 > > > > > > > [...] > > > > > > > > [=C2=A0=C2=A0442.452090][ T5224] -> #0 (mem_hotplug_lock.= rw_sem){++++}: > > > > > > > > [=C2=A0=C2=A0442.459748][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0validate_chain+0xd10/0x2bcc > > > > > > > > [=C2=A0=C2=A0442.464883][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0__lock_acquire+0x7f4/0xb8c > > > > > > > > [=C2=A0=C2=A0442.469930][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0lock_acquire+0x31c/0x360 > > > > > > > > [=C2=A0=C2=A0442.474803][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0get_online_mems+0x54/0x150 > > > > > > > > [=C2=A0=C2=A0442.479850][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0show_slab_objects+0x94/0x3a8 > > > > > > > > [=C2=A0=C2=A0442.485072][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0total_objects_show+0x28/0x34 > > > > > > > > [=C2=A0=C2=A0442.490292][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0slab_attr_show+0x38/0x54 > > > > > > > > [=C2=A0=C2=A0442.495166][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0sysfs_kf_seq_show+0x198/0x2d4 > > > > > > > > [=C2=A0=C2=A0442.500473][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0kernfs_seq_show+0xa4/0xcc > > > > > > > > [=C2=A0=C2=A0442.505433][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0seq_read+0x30c/0x8a8 > > > > > > > > [=C2=A0=C2=A0442.509958][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0kernfs_fop_read+0xa8/0x314 > > > > > > > > [=C2=A0=C2=A0442.515007][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0__vfs_read+0x88/0x20c > > > > > > > > [=C2=A0=C2=A0442.519620][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0vfs_read+0xd8/0x10c > > > > > > > > [=C2=A0=C2=A0442.524060][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0ksys_read+0xb0/0x120 > > > > > > > > [=C2=A0=C2=A0442.528586][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0__arm64_sys_read+0x54/0x88 > > > > > > > > [=C2=A0=C2=A0442.533634][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0el0_svc_handler+0x170/0x240 > > > > > > > > [=C2=A0=C2=A0442.538768][ T5224]=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0=C2=A0el0_svc+0x8/0xc > > > > > > >=20 > > > > > > > I believe the lock is not really needed here. We do not dea= llocated > > > > > > > pgdat of a hotremoved node nor destroy the slab state becau= se an > > > > > > > existing slabs would prevent hotremove to continue in the f= irst place. > > > > > > >=20 > > > > > > > There are likely details to be checked of course but the lo= ck just seems > > > > > > > bogus. > > > > > >=20 > > > > > > Check 03afc0e25f7f ("slab: get_online_mems for > > > > > > kmem_cache_{create,destroy,shrink}"). It actually talk about = the races during > > > > > > memory as well cpu hotplug, so it might even that cpu_hotplug= _lock removal is > > > > > > problematic? > > > > > >=20 > > > > >=20 > > > > > Which removal are you referring to? get_online_mems() does not = mess with > > > > > the cpu hotplug lock (and therefore this patch). > > > >=20 > > > > The one in your patch. I suspect there might be races among the w= hole NUMA node > > > > hotplug, kmem_cache_create, and show_slab_objects(). See bfc8c901= 39eb ("mem- > > > > hotplug: implement get/put_online_mems") > > > >=20 > > > > "kmem_cache_{create,destroy,shrink} need to get a stable value of= cpu/node > > > > online mask, because they init/destroy/access per-cpu/node kmem_c= ache parts, > > > > which can be allocated or destroyed on cpu/mem hotplug." > > >=20 > > > I still have to grasp that code but if the slub allocator really ne= eds > > > a stable cpu mask then it should be using the explicit cpu hotplug > > > locking rather than rely on side effect of memory hotplug locking. > > >=20 > > > > Both online_pages() and show_slab_objects() need to get a stable = value of > > > > cpu/node online mask. > > >=20 > > > Could tou be more specific why online_pages need a stable cpu onlin= e > > > mask? I do not think that show_slab_objects is a real problem becau= se a > > > potential race shouldn't be critical. > >=20 > > build_all_zonelists() > > __build_all_zonelists() > > for_each_online_cpu(cpu) > >=20 >=20 > Two things: >=20 > a) We currently always hold the device hotplug lock when onlining memor= y > and when onlining cpus (for CPUs at least via user space - we would hav= e > to double check other call paths). So theoretically, that should guard > us from something like that already. >=20 > b) >=20 > commit 11cd8638c37f6c400cc472cc52b6eccb505aba6e > Author: Michal Hocko > Date: Wed Sep 6 16:20:34 2017 -0700 >=20 > mm, page_alloc: remove stop_machine from build_all_zonelists >=20 > Tells me: >=20 > "Updates of the zonelists happen very seldom, basically only when a zon= e > becomes populated during memory online or when it loses all the memory > during offline. A racing iteration over zonelists could either miss a > zone or try to work on one zone twice. Both of these are something we > can live with occasionally because there will always be at least one > zone visible so we are not likely to fail allocation too easily for > example." >=20 > Sounds like if there would be a race, we could live with it if I am not > getting that totally wrong. >=20 What's the problem you are trying to solve? Why it is more important to l= ive with races than to keep the correct code?