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 kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id F2DFAC433EF for ; Wed, 8 Dec 2021 00:27:02 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 8DBCB6B0073; Tue, 7 Dec 2021 19:26:52 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id 88B036B0074; Tue, 7 Dec 2021 19:26:52 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7531B6B0075; Tue, 7 Dec 2021 19:26:52 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0193.hostedemail.com [216.40.44.193]) by kanga.kvack.org (Postfix) with ESMTP id 67E9B6B0073 for ; Tue, 7 Dec 2021 19:26:52 -0500 (EST) Received: from smtpin12.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay04.hostedemail.com (Postfix) with ESMTP id 22F4789B2A for ; Wed, 8 Dec 2021 00:26:42 +0000 (UTC) X-FDA: 78892736244.12.6568DF0 Received: from mail-ed1-f50.google.com (mail-ed1-f50.google.com [209.85.208.50]) by imf28.hostedemail.com (Postfix) with ESMTP id C239D90000AB for ; Wed, 8 Dec 2021 00:26:41 +0000 (UTC) Received: by mail-ed1-f50.google.com with SMTP id g14so2461713edb.8 for ; Tue, 07 Dec 2021 16:26:41 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc; bh=pnwaxXWui3gt2JZ+0qiBlm1abRwyk5iLPJHl06L2hAo=; b=fs+OLMIuqVipcPWZE3/Kan9ba+o1UYCcof5P++Q0lNetI26biUrgWAU1u4wQOTBmH2 Qs9ii3iucf1XPjOax/7DnGSGcAr7bOlBWD00sc8HIc4awsYrGsqMbJgBpJwr7RVL/04I 0nvJBTcYWUFDTs06xIv79MCRDkD3q4BvFyiGs9MAh2jCZxfH0hR/hn8lDtHyT7zAvATV ckrL54WHUAunyZ+8HqWq0Le4QYA9n87vKLBTxB9XexhMGtuY4dUP0ySZsCUUzyaxTbze 1y8Bi83RW+9nXEuVwphHk5l7EwgQOvf+F7Kj9ajx8OMXQWtnd7KbUM9YGhpOOh8iUGz5 NV2g== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc; bh=pnwaxXWui3gt2JZ+0qiBlm1abRwyk5iLPJHl06L2hAo=; b=guBU93A3IgYmHI5iipfwXIoiPrefw2WBs/8tnxt98DAMY0054rNXTFFT4+prj8nEDi P5CP+PD30RiANDBMbXxrzcFdv+BebldxlC3Xi4cV57k4cQenoUzJ5Czpz1kloOP0L/DP ub1xCxKc/LYkkf2WCRxUv1shkEbHAKVKWr4rw3Y3HPKV1JJw0pRc2C+ki458GSvWcfZ8 7RM5o5Kpt758g0lnAPdgG8bxjcW/4CATetjbsw33PZEdlYAfuWSQk7A/dpdVIw2IbFCs oAzf4IRv5czYfxpQ+hex09A7WtOKIxXp2xeEnsG8hrNhPilCIe2IVZALM+Qe4zb+oTw+ GFRg== X-Gm-Message-State: AOAM531NS1DmsqMHfvmu4oC7wA4CYpPQZF75rX5eBBtZaiuyctV2gzM+ WS0dKMctS/WApRrv0Cu0no/Fv1p/4EOa3YWC+4A= X-Google-Smtp-Source: ABdhPJz+jtrB85elwFzrf6zDVdu64UBvHZuqqVPF14xMHHQth1rGv39AldN5fr0hYfrTAKz9KGYdVAfgFIBWk8TAS/k= X-Received: by 2002:a17:906:538d:: with SMTP id g13mr3410503ejo.62.1638923200363; Tue, 07 Dec 2021 16:26:40 -0800 (PST) MIME-Version: 1.0 References: <20211207224013.880775-1-npache@redhat.com> <20211207224013.880775-2-npache@redhat.com> <20211207154438.c1e49a3f0b5ebc9245aac61b@linux-foundation.org> In-Reply-To: <20211207154438.c1e49a3f0b5ebc9245aac61b@linux-foundation.org> From: Yang Shi Date: Tue, 7 Dec 2021 16:26:28 -0800 Message-ID: Subject: Re: [PATCH v2 1/1] mm/vmscan.c: Prevent allocating shrinker_info on offlined nodes To: Andrew Morton Cc: Nico Pache , Linux Kernel Mailing List , Linux MM , Shakeel Butt , Kirill Tkhai , Roman Gushchin , Vlastimil Babka , Vladimir Davydov , raquini@redhat.com, Michal Hocko , David Hildenbrand Content-Type: text/plain; charset="UTF-8" X-Stat-Signature: or1uq7b8omzfxnww9tx4545brnnntftp Authentication-Results: imf28.hostedemail.com; dkim=pass header.d=gmail.com header.s=20210112 header.b=fs+OLMIu; dmarc=pass (policy=none) header.from=gmail.com; spf=pass (imf28.hostedemail.com: domain of shy828301@gmail.com designates 209.85.208.50 as permitted sender) smtp.mailfrom=shy828301@gmail.com X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: C239D90000AB X-HE-Tag: 1638923201-10990 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 Tue, Dec 7, 2021 at 3:44 PM Andrew Morton wrote: > > On Tue, 7 Dec 2021 17:40:13 -0500 Nico Pache wrote: > > > We have run into a panic caused by a shrinker allocation being attempted > > on an offlined node. > > > > Our crash analysis has determined that the issue originates from trying > > to allocate pages on an offlined node in expand_one_shrinker_info. This > > function makes the incorrect assumption that we can allocate on any node. > > To correct this we make sure the node is online before tempting an > > allocation. If it is not online choose the closest node. > > This isn't fully accurate, is it? We could allocate on a node which is > presently offline but which was previously onlined, by testing > NODE_DATA(nid). > > It isn't entirely clear to me from the v1 discussion why this approach > isn't being taken? > > AFAICT the proposed patch is *already* taking this approach, by having > no protection against a concurrent or subsequent node offlining? AFAICT, we have not reached agreement on how to fix it yet. I saw 3 proposals at least: 1. From Michal, allocate node data for all possible nodes. https://lore.kernel.org/all/Ya89aqij6nMwJrIZ@dhcp22.suse.cz/T/#u 2. What this patch does. Proposed originally from https://lore.kernel.org/all/20211108202325.20304-1-amakhalov@vmware.com/T/#u 3. From David, fix in node_zonelist(). https://lore.kernel.org/all/51c65635-1dae-6ba4-daf9-db9df0ec35d8@redhat.com/T/#u > > > --- a/mm/vmscan.c > > +++ b/mm/vmscan.c > > @@ -222,13 +222,16 @@ static int expand_one_shrinker_info(struct mem_cgroup *memcg, > > int size = map_size + defer_size; > > > > for_each_node(nid) { > > + int tmp = nid; > > Not `tmp', please. Better to use an identifier which explains the > variable's use. target_nid? > > And a newline after defining locals, please. > > > pn = memcg->nodeinfo[nid]; > > old = shrinker_info_protected(memcg, nid); > > /* Not yet online memcg */ > > if (!old) > > return 0; > > > > - new = kvmalloc_node(sizeof(*new) + size, GFP_KERNEL, nid); > > + if(!node_online(nid)) > > s/if(/if (/ > > > + tmp = numa_mem_id(); > > + new = kvmalloc_node(sizeof(*new) + size, GFP_KERNEL, tmp); > > if (!new) > > return -ENOMEM; > > > > And a code comment fully explaining what's going on here?