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 80035C433FE for ; Thu, 17 Feb 2022 04:47:01 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id D99C86B0074; Wed, 16 Feb 2022 23:47:00 -0500 (EST) Received: by kanga.kvack.org (Postfix, from userid 40) id D24546B0075; Wed, 16 Feb 2022 23:47:00 -0500 (EST) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id B74976B0078; Wed, 16 Feb 2022 23:47:00 -0500 (EST) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0098.hostedemail.com [216.40.44.98]) by kanga.kvack.org (Postfix) with ESMTP id A2C3E6B0074 for ; Wed, 16 Feb 2022 23:47:00 -0500 (EST) Received: from smtpin10.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay03.hostedemail.com (Postfix) with ESMTP id 39A9E8248D52 for ; Thu, 17 Feb 2022 04:47:00 +0000 (UTC) X-FDA: 79151037000.10.5F69CEF Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by imf10.hostedemail.com (Postfix) with ESMTP id 74200C0002 for ; Thu, 17 Feb 2022 04:46:59 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1645073218; 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: in-reply-to:in-reply-to:references:references; bh=w67bokXv1wnn2sDvhXuLtvR4gKRz+41c8sPAHA3v48M=; b=ILFeTokgeeAob+Thjo5yRALBIldHEjhZg9HQ/yRta5OxJg0896OtQhfyQwTLyXLAS5Wvi6 Y4IlnQxdW5735js1+UZhV+AflPwBKp05aRsjWTuIu2vmT7juBxD7CCgi1N04Kd9C+Skt4z ZYhiV78dAniPdnbHlCpKwTmzWuomPAg= Received: from mail-pf1-f198.google.com (mail-pf1-f198.google.com [209.85.210.198]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-412-a1Ep9eE7OwmRF7Sy6XAZgQ-1; Wed, 16 Feb 2022 23:46:52 -0500 X-MC-Unique: a1Ep9eE7OwmRF7Sy6XAZgQ-1 Received: by mail-pf1-f198.google.com with SMTP id j204-20020a6280d5000000b004e107ad3488so2559884pfd.15 for ; Wed, 16 Feb 2022 20:46:52 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=w67bokXv1wnn2sDvhXuLtvR4gKRz+41c8sPAHA3v48M=; b=pZKCu9eAqXXJBwxzEGLOtsD4Bd7SxqYY4m6d0B+u563uarmWAHcaBkKEs3Xs/rssg6 D2XCawE7tJ8TWVJA0Ez9/WkAddIxyTNvgzNUqdX9FcR1n20Kz/M+ibTwWvqLkIXfd+bx YQiKemBYSITzzj54MmCwDgXbulcXKqm6dF+SqIEfh36sVjzywyUnWPXP6cq0Ug+WQeez MlG0HDZc3pNl8UOQVk1+6ValZ9JSI0mgDNvPD4X4SeQr+OnYu9be9ygI+LEkVFWT8mD5 jlkOvo3fmhSf6MtOnKg3hZ5qV3itaEWSRA4GXvfssWSje2z2qoYD556Zzw7G5M7c9JRD YUlg== X-Gm-Message-State: AOAM530OT2eQfrcG58PvXIBG4dv02R4wH1oMUuOo6Z2m2TGj23Q4jNqo XYevfZv3CU67t6XMQVVRHe7NrBh0evkppSqDDqtu4acNaGTKB34A+gE+CtqrZNyIv9C2bV6O2Dd fwzYE8URzk4s= X-Received: by 2002:a17:902:ea09:b0:14d:9644:6f03 with SMTP id s9-20020a170902ea0900b0014d96446f03mr1270241plg.110.1645073211123; Wed, 16 Feb 2022 20:46:51 -0800 (PST) X-Google-Smtp-Source: ABdhPJypK27oNtj2dgWzTF2J96OC/kIj9OSuLw/VdHj5CfJxFDd6QPZLvNSUGDUBjHfWS1Rb5182HQ== X-Received: by 2002:a17:902:ea09:b0:14d:9644:6f03 with SMTP id s9-20020a170902ea0900b0014d96446f03mr1270216plg.110.1645073210743; Wed, 16 Feb 2022 20:46:50 -0800 (PST) Received: from xz-m1.local ([94.177.118.126]) by smtp.gmail.com with ESMTPSA id k9sm47270513pfc.157.2022.02.16.20.46.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 16 Feb 2022 20:46:44 -0800 (PST) Date: Thu, 17 Feb 2022 12:46:32 +0800 From: Peter Xu To: John Hubbard Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , "Kirill A . Shutemov" , Matthew Wilcox , Yang Shi , Andrea Arcangeli , Alistair Popple , David Hildenbrand , Vlastimil Babka , Hugh Dickins Subject: Re: [PATCH v4 1/4] mm: Don't skip swap entry even if zap_details specified Message-ID: References: <20220216094810.60572-1-peterx@redhat.com> <20220216094810.60572-2-peterx@redhat.com> MIME-Version: 1.0 In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Authentication-Results: imf10.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=ILFeTokg; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf10.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 170.10.133.124) smtp.mailfrom=peterx@redhat.com X-Rspam-User: X-Rspamd-Server: rspam10 X-Rspamd-Queue-Id: 74200C0002 X-Stat-Signature: k6ra3nuiqdzdkkswm89nhbhq4teo1zxj X-HE-Tag: 1645073219-95332 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, Feb 16, 2022 at 07:15:30PM -0800, John Hubbard wrote: > On 2/16/22 1:48 AM, Peter Xu wrote: > > The "details" pointer shouldn't be the token to decide whether we should skip > > swap entries. For example, when the user specified details->zap_mapping==NULL, > > it means the user wants to zap all the pages (including COWed pages), then we > > need to look into swap entries because there can be private COWed pages that > > was swapped out. > > Hi Peter, > > The changes look good, just some minor readability suggestions below: > > (btw, hch is going to ask you to reflow all of the commit descriptions > to 72 cols, so you might as well do it in advance. :) Thanks for the heads-up. :) I personally used 78/79 col width for a long time for different projects, but sure I can adjust my config. I found that the "official guide" points us to 75 instead: https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html The body of the explanation, line wrapped at 75 columns, which will be copied to the permanent changelog to describe this patch. I'll follow that. [...] > > @@ -1320,11 +1331,15 @@ struct zap_details { > > static inline bool > > zap_skip_check_mapping(struct zap_details *details, struct page *page) > > { > > - if (!details || !page) > > + /* If we can make a decision without *page.. */ > > + if (should_zap_cows(details)) > > return false; > > > > - return details->zap_mapping && > > - (details->zap_mapping != page_rmapping(page)); > > + /* E.g. zero page */ > > It's a bit confusing to see a comment that "this could be the zero page", if > the value is NULL. Maybe, "the caller passes NULL for the case of a zero > page", or something along those lines? It didn't show much difference here.. but for sure I can coordinate. > > > > + if (!page) > > + return false; > > + > > + return details->zap_mapping != page_rmapping(page); > > } > > > > static unsigned long zap_pte_range(struct mmu_gather *tlb, > > @@ -1405,17 +1420,29 @@ static unsigned long zap_pte_range(struct mmu_gather *tlb, > > continue; > > } > > > > - /* If details->check_mapping, we leave swap entries. */ > > - if (unlikely(details)) > > - continue; > > - > > - if (!non_swap_entry(entry)) > > + if (!non_swap_entry(entry)) { > > + /* > > + * If this is a genuine swap entry, then it must be an > > + * private anon page. If the caller wants to skip > > + * COWed pages, ignore it. > > + */ > > How about this instead: > > /* Genuine swap entry, and therefore a private anon page. */ Yes the last sentence is kind of redundant. > > > + if (!should_zap_cows(details)) > > + continue; > > rss[MM_SWAPENTS]--; > > - else if (is_migration_entry(entry)) { > > Can we put a newline here, and before each "else" block? Because now it > is getting very dense, and the visual separation really helps. The thing is we don't have a rule to add empty lines here, or do we? Changing it could make it less like what we have had. Personally it looks fine, because separations are done with either new lines or indents. Here it's done via indents, IMHO. > > > + } else if (is_migration_entry(entry)) { > > struct page *page; > > > > page = pfn_swap_entry_to_page(entry); > > + if (zap_skip_check_mapping(details, page)) > > + continue; > > rss[mm_counter(page)]--; > > Newline here. > > > + } else if (is_hwpoison_entry(entry)) { > > + /* If the caller wants to skip COWed pages, ignore it */ > > Likewise, I'd prefer we delete that comment, because it exactly matches > what the following line of code says. Will do. > > > + if (!should_zap_cows(details)) > > + continue; > > And newline here too. > > > + } else { > > + /* We should have covered all the swap entry types */ > > + WARN_ON_ONCE(1); > > } > > if (unlikely(!free_swap_and_cache(entry))) > > print_bad_pte(vma, addr, ptent, NULL); > > Those are all just nits, and as I mentioned, the actual changes look good > to me, so: > > Reviewed-by: John Hubbard Thanks, -- Peter Xu