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 AD3C4C32772 for ; Tue, 23 Aug 2022 10:34:14 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id 32A6C8D0001; Tue, 23 Aug 2022 06:34:14 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 2DA026B0074; Tue, 23 Aug 2022 06:34:14 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 17A0E8D0001; Tue, 23 Aug 2022 06:34:14 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0016.hostedemail.com [216.40.44.16]) by kanga.kvack.org (Postfix) with ESMTP id 08F036B0073 for ; Tue, 23 Aug 2022 06:34:14 -0400 (EDT) Received: from smtpin18.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay07.hostedemail.com (Postfix) with ESMTP id CAD79161470 for ; Tue, 23 Aug 2022 10:34:13 +0000 (UTC) X-FDA: 79830497586.18.A22DA8D Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf03.hostedemail.com (Postfix) with ESMTP id 2E75120045 for ; Tue, 23 Aug 2022 10:34:13 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1661250852; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=jDxom684Sx6IcSK9tbWna4U0yUZJ4yaPW6qKl9Evdak=; b=Nxmh0gS61BrD36Fd2wLPMTaYlXZ8RU2vF1g14hOboyLlLRhEd7N7f3u/maJ9tzLPLwSGTB fN/ks6zNg7v3YAjN3SZpfqYcjr1BzhfZOxk2A+eHGqr8eIhnX/G0IctCZiMTflTNL4bsbw R96z49s9pX0RlVffG3U1JGjg5K9Xth0= Received: from mail-wr1-f69.google.com (mail-wr1-f69.google.com [209.85.221.69]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_128_GCM_SHA256) id us-mta-86-egKUFGthPgCdLSoFoByjtQ-1; Tue, 23 Aug 2022 06:34:11 -0400 X-MC-Unique: egKUFGthPgCdLSoFoByjtQ-1 Received: by mail-wr1-f69.google.com with SMTP id t12-20020adfba4c000000b0021e7440666bso2160889wrg.22 for ; Tue, 23 Aug 2022 03:34:11 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=content-transfer-encoding:in-reply-to:subject:organization:from :references:cc:to:content-language:user-agent:mime-version:date :message-id:x-gm-message-state:from:to:cc; bh=jDxom684Sx6IcSK9tbWna4U0yUZJ4yaPW6qKl9Evdak=; b=TZ7XysJpODa9DHtaJwqsBBTRnLC4QBMsuU1DPSpjTww3hHPFUz3QFbRoy3/j2JZySV tuJeyJibMffc9SAKO4YEajmnlaxI2TQsHy8jsGCc/+Mg29N7GKhMjeLVeGKUdDMAhUsD 5+HB2Xx6E/BsRYg3agIPpK54kIPlVl/HQEQ9dvOwwgV/NgNO4pMOiG6Io7QwxwZ11k4O Am3NxReJxXrXWTp1JwxcCYtZtoDGFdhlQPt6POrgLDLriq4H8AHpW8UfS0D6WHYb9F3P 6Od2NmpO1YwJ1p6Wcuj3MlQuP5p4Ao6/S7Sab1IwE82Kr1Vo+46tCkbRxlagZ4zrCDcT mVHQ== X-Gm-Message-State: ACgBeo1GhYttFXKrNzx44IJ6gio44up9E4IryTZVRSr3Dw4J0DZAMP1s vfA4Qz8NtILUKAVrb3xwLV8DQoCxxAzf1siuSxjF5sOBEgvakR5Amm2yVd46wvteS6EzT1NvneA icmPWChYkL4E= X-Received: by 2002:a05:6000:1848:b0:225:5870:1d34 with SMTP id c8-20020a056000184800b0022558701d34mr5421305wri.44.1661250850598; Tue, 23 Aug 2022 03:34:10 -0700 (PDT) X-Google-Smtp-Source: AA6agR6K6ZWhmpxmKnYSEo2yPdQeBLoS3srxOcjOOT8dWdUuOkgIwGkogzATFFP8w//6Yk3s3ZbAZg== X-Received: by 2002:a05:6000:1848:b0:225:5870:1d34 with SMTP id c8-20020a056000184800b0022558701d34mr5421284wri.44.1661250850264; Tue, 23 Aug 2022 03:34:10 -0700 (PDT) Received: from ?IPV6:2003:cb:c70b:1600:c48b:1fab:a330:5182? (p200300cbc70b1600c48b1faba3305182.dip0.t-ipconnect.de. [2003:cb:c70b:1600:c48b:1fab:a330:5182]) by smtp.gmail.com with ESMTPSA id n39-20020a05600c502700b003a60bc8ae8fsm17329246wmr.21.2022.08.23.03.34.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Tue, 23 Aug 2022 03:34:09 -0700 (PDT) Message-ID: <0fc01e47-51f3-baf7-2d46-72291422f695@redhat.com> Date: Tue, 23 Aug 2022 12:34:09 +0200 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.11.0 To: Mel Gorman Cc: Michal Hocko , Patrick Daly , linux-mm@kvack.org, linux-arm-kernel@lists.infradead.org, Juergen Gross References: <20220817034250.GB2473@hu-pdaly-lv.qualcomm.com> <20220817104028.uin7cmkb4qlpgfbi@suse.de> <11f91089-1958-c7eb-126f-af32130d9f8a@redhat.com> <20220823083349.5c2aolc6xgfhp3k7@suse.de> <20220823094950.ocjyur2h3mqnqbeg@suse.de> From: David Hildenbrand Organization: Red Hat Subject: Re: Race condition in build_all_zonelists() when offlining movable zone In-Reply-To: <20220823094950.ocjyur2h3mqnqbeg@suse.de> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1661250853; a=rsa-sha256; cv=none; b=mjn0S2LU9pz2QKEgY5yBGYRzD+iK4KHQuhJdxozqIgRl+9xYbhqCxX9zVxicStcnrZv5o3 9/tKRbGKGkQZBKJ731kbcfK22OtK3NJPEsrzlFKHijughC9HSuR+BOfIJWTCHaUS9IMdcQ rkIQCg9Qf3dsCLNsRBDqTSbBsQuHA34= ARC-Authentication-Results: i=1; imf03.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Nxmh0gS6; spf=pass (imf03.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com; dmarc=pass (policy=none) header.from=redhat.com ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1661250853; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references:dkim-signature; bh=jDxom684Sx6IcSK9tbWna4U0yUZJ4yaPW6qKl9Evdak=; b=sfBqg4+5/23n1/Fc15g9lA56fNjaet2kGAaudWnJMeZPuoUbS/5dYdnh7R3y3zkEIA5U+1 wKhJqdNbrT1THrHNeUAICHUsLlqleGeGVsC2C7bDTNyvhAu+F17cfNTXwNR8jahNNl0obh HTX8cquXH7rPjI/38XWi+g5rGSjCetA= X-Rspam-User: X-Rspamd-Server: rspam08 X-Rspamd-Queue-Id: 2E75120045 Authentication-Results: imf03.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=Nxmh0gS6; spf=pass (imf03.hostedemail.com: domain of david@redhat.com designates 170.10.133.124 as permitted sender) smtp.mailfrom=david@redhat.com; dmarc=pass (policy=none) header.from=redhat.com X-Stat-Signature: 5gkhbqobobxrcq3hf34oatt4nkkyxcxx X-HE-Tag: 1661250853-11557 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 23.08.22 11:49, Mel Gorman wrote: > On Tue, Aug 23, 2022 at 10:52:34AM +0200, David Hildenbrand wrote: >> On 23.08.22 10:33, Mel Gorman wrote: >>> On Tue, Aug 23, 2022 at 08:36:34AM +0200, David Hildenbrand wrote: >>>>> @@ -6517,6 +6538,7 @@ static void __build_all_zonelists(void *data) >>>>> static DEFINE_SPINLOCK(lock); >>>>> >>>>> spin_lock(&lock); >>>>> + write_seqcount_begin(&zonelist_update_seq); >>>>> >>>>> #ifdef CONFIG_NUMA >>>>> memset(node_load, 0, sizeof(node_load)); >>>>> @@ -6553,6 +6575,7 @@ static void __build_all_zonelists(void *data) >>>>> #endif >>>>> } >>>>> >>>>> + write_seqcount_end(&zonelist_update_seq); >>>>> spin_unlock(&lock); >>>> >>>> Do we want to get rid of the static lock by using a seqlock_t instead of >>>> a seqcount_t? >>>> >>> >>> I do not think so because it's a relatively heavy lock compared to the >>> counter and the read side. >> >> I was primarily asking because seqlock.h states: "Sequential locks >> (seqlock_t): Sequence counters with an embedded spinlock for writer >> serialization and non-preemptibility." seems to be precisely what we are >> doing here. >> >>> >>> As the read-side can be called from hardirq or softirq context, the >>> write-side needs to disable irqs or bottom halves as well according to the >>> documentation. That is relatively minor as the write side is rare but it's >>> tricker because the calling context can be both IRQ or softirq so the IRQ >>> protection would have to be used. >> >> Naive me would just have used write_seqlock()/write_sequnlock() and >> read_seqbegin()/read_seqretry() to result in almost the same code as >> with your change -- but having both mechanisms encapsulated. >> >> >> Yeah, there are special write_seqlock_bh()/write_sequnlock_bh(), >> write_sequnlock_irq() ... but IIRC we don't have to care about that at >> all when just using the primitives as above. But most probably I am >> missing something important. >> > > You're not missing anything important, I'm just not a massive fan of the > API naming because it's unclear from the context if it's a plain counter > or a locked counter and felt it was better to keep the locking explicit. > > A seqlock version is below. I updated the comments and naming to make it > clear the read-side is for iteration, what the locking protocol is and > match the retry naming with the cpuset equivalent. It boots on KVM but > would need another test from Patrick to be certain it still works. Patrick, > would you mind testing this version please? > > ---8<--- > mm/page_alloc.c | 33 ++++++++++++++++++++++++++++----- > 1 file changed, 28 insertions(+), 5 deletions(-) > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c > index e5486d47406e..a644c7b638a3 100644 > --- a/mm/page_alloc.c > +++ b/mm/page_alloc.c > @@ -4708,6 +4708,24 @@ void fs_reclaim_release(gfp_t gfp_mask) > EXPORT_SYMBOL_GPL(fs_reclaim_release); > #endif > > +/* > + * Zonelists may change due to hotplug during allocation. Detect when zonelists > + * have been rebuilt so allocation retries. Reader side does not lock and > + * retries the allocation if zonelist changes. Writer side is protected by the > + * embedded spin_lock. > + */ > +DEFINE_SEQLOCK(zonelist_update_seq); > + > +static unsigned int zonelist_iter_begin(void) > +{ > + return read_seqbegin(&zonelist_update_seq); > +} > + > +static unsigned int check_retry_zonelist(unsigned int seq) > +{ > + return read_seqretry(&zonelist_update_seq, seq); > +} > + > /* Perform direct synchronous page reclaim */ > static unsigned long > __perform_reclaim(gfp_t gfp_mask, unsigned int order, > @@ -5001,6 +5019,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, > int compaction_retries; > int no_progress_loops; > unsigned int cpuset_mems_cookie; > + unsigned int zonelist_iter_cookie; > int reserve_flags; > > /* > @@ -5016,6 +5035,7 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, > no_progress_loops = 0; > compact_priority = DEF_COMPACT_PRIORITY; > cpuset_mems_cookie = read_mems_allowed_begin(); > + zonelist_iter_cookie = zonelist_iter_begin(); > > /* > * The fast path uses conservative alloc_flags to succeed only until > @@ -5187,8 +5207,12 @@ __alloc_pages_slowpath(gfp_t gfp_mask, unsigned int order, > goto retry; > > > - /* Deal with possible cpuset update races before we start OOM killing */ > - if (check_retry_cpuset(cpuset_mems_cookie, ac)) > + /* > + * Deal with possible cpuset update races or zonelist updates to avoid > + * a unnecessary OOM kill. > + */ > + if (check_retry_cpuset(cpuset_mems_cookie, ac) || > + check_retry_zonelist(zonelist_iter_cookie)) > goto retry_cpuset; > > /* Reclaim has failed us, start killing things */ > @@ -6514,9 +6538,8 @@ static void __build_all_zonelists(void *data) > int nid; > int __maybe_unused cpu; > pg_data_t *self = data; > - static DEFINE_SPINLOCK(lock); > > - spin_lock(&lock); > + write_seqlock(&zonelist_update_seq); > > #ifdef CONFIG_NUMA > memset(node_load, 0, sizeof(node_load)); > @@ -6553,7 +6576,7 @@ static void __build_all_zonelists(void *data) > #endif > } > > - spin_unlock(&lock); > + write_sequnlock(&zonelist_update_seq); > } > > static noinline void __init > LGTM. The "retry_cpuset" label might deserve a better name now. Would Fixes: 6aa303defb74 ("mm, vmscan: only allocate and reclaim from zones with pages managed by the buddy allocator") be correct? -- Thanks, David / dhildenb