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=-16.1 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, INCLUDES_CR_TRAILER,INCLUDES_PATCH,MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=unavailable 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 30A3EC4320A for ; Thu, 2 Sep 2021 14:48:43 +0000 (UTC) Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by mail.kernel.org (Postfix) with ESMTP id A4E376108B for ; Thu, 2 Sep 2021 14:48:42 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.4.1 mail.kernel.org A4E376108B Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=redhat.com Authentication-Results: mail.kernel.org; spf=pass smtp.mailfrom=kvack.org Received: by kanga.kvack.org (Postfix) id 946AF6B006C; Thu, 2 Sep 2021 10:48:41 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id 8F55E6B0071; Thu, 2 Sep 2021 10:48:41 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 7BD418D0001; Thu, 2 Sep 2021 10:48:41 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from forelay.hostedemail.com (smtprelay0084.hostedemail.com [216.40.44.84]) by kanga.kvack.org (Postfix) with ESMTP id 6F9A16B006C for ; Thu, 2 Sep 2021 10:48:41 -0400 (EDT) Received: from smtpin29.hostedemail.com (10.5.19.251.rfc1918.com [10.5.19.251]) by forelay05.hostedemail.com (Postfix) with ESMTP id 1DCC7182CEC8D for ; Thu, 2 Sep 2021 14:48:41 +0000 (UTC) X-FDA: 78542914842.29.804A015 Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [216.205.24.124]) by imf25.hostedemail.com (Postfix) with ESMTP id B3EFCB000183 for ; Thu, 2 Sep 2021 14:48:40 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1630594120; 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=0wVKNF0XQtjZ9RbZFV3p67mUIOM94HpjWZkNzTpPK1k=; b=WrHnBJw2Ss5DDvz7lZBoXiSzTpzqRRAPFkh6VtZGTTmA6Xz/6CuTLSdwgACsy6N1X1saa8 792tZupLQnTrqPLysyP5KegeyP1/sZMgNbk5+PnSkOtKPNDWDf0elgGyUfDpuaBUMDLwP9 denGrBY0lyVSPDPxXwwCVyzFjFCc340= Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (Using TLS) by relay.mimecast.com with ESMTP id us-mta-537-0haGCinPPgWuttk53oTi1Q-1; Thu, 02 Sep 2021 10:48:37 -0400 X-MC-Unique: 0haGCinPPgWuttk53oTi1Q-1 Received: by mail-qt1-f200.google.com with SMTP id b15-20020a05622a020f00b0029e28300d94so1403655qtx.16 for ; Thu, 02 Sep 2021 07:48:37 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=0wVKNF0XQtjZ9RbZFV3p67mUIOM94HpjWZkNzTpPK1k=; b=XfbOjh59em2LwS43IdnrKH0umyHzMK6LF0Pqu6jsY83m82pgB0Fp6EDid+ReOzXfBV 9uhcMy52nRNOCgWM/asyeZL7bWLXuv2xjPtDNNHAiqmMnut8ofwWkO493jIUVEpMMt9V FlxLa9mZ+XaI17nG5IvQuPPSMnp+DA6lYhBvN1sPnERxgUqwAtljc2f+6IeMO6krAxlX EGffvQgTtSXQPUu4uOEDIBPR3ObdBbJe+QqF+u5ELGfzRWUWDHoP5jQao/Uk7bSqdupZ KjHV8HaYzkuei3kj74ODKaTE2XZKhstckTPOXAxTrMuhX7QJmr/kglD1E/rzCo9AuvN2 eHIw== X-Gm-Message-State: AOAM531PQwLKhbhtqVBG8Z9DTjCUEmiwIx78bp4YUiyvQjZN3cVwGx9j f63n+8FKvhovbkn42sXBT8jwEBs24TUfKogiyCAZGcDYUJl3ougOhL/oy08K0NwEsxGKbQ1Dbgy V0n5AIZqXOeQ= X-Received: by 2002:a05:620a:4404:: with SMTP id v4mr3538354qkp.344.1630594115310; Thu, 02 Sep 2021 07:48:35 -0700 (PDT) X-Google-Smtp-Source: ABdhPJwxaosPn9uHdUrs1IrRUUqvUMsgR8b60qaNexWZOR8FDZJ8HYo9cMS23TOMY78uAbxFZwANGg== X-Received: by 2002:a05:620a:4404:: with SMTP id v4mr3538331qkp.344.1630594115067; Thu, 02 Sep 2021 07:48:35 -0700 (PDT) Received: from t490s ([2607:fea8:56a3:500::ad7f]) by smtp.gmail.com with ESMTPSA id q7sm1565488qkm.68.2021.09.02.07.48.33 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 02 Sep 2021 07:48:34 -0700 (PDT) Date: Thu, 2 Sep 2021 10:48:32 -0400 From: Peter Xu To: David Hildenbrand Cc: linux-kernel@vger.kernel.org, linux-mm@kvack.org, Andrew Morton , Matthew Wilcox , Yang Shi , Miaohe Lin , Hugh Dickins , Mike Rapoport , Andrea Arcangeli , "Kirill A . Shutemov" , Jerome Glisse , Alistair Popple Subject: Re: [PATCH 4/5] mm: Introduce zap_details.zap_flags Message-ID: References: <20210901205622.6935-1-peterx@redhat.com> <20210901205722.7328-1-peterx@redhat.com> <5f3aa3fa-e877-02d1-8721-debbe688e7af@redhat.com> MIME-Version: 1.0 In-Reply-To: <5f3aa3fa-e877-02d1-8721-debbe688e7af@redhat.com> X-Mimecast-Spam-Score: 0 X-Mimecast-Originator: redhat.com Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Authentication-Results: imf25.hostedemail.com; dkim=pass header.d=redhat.com header.s=mimecast20190719 header.b=WrHnBJw2; dmarc=pass (policy=none) header.from=redhat.com; spf=none (imf25.hostedemail.com: domain of peterx@redhat.com has no SPF policy when checking 216.205.24.124) smtp.mailfrom=peterx@redhat.com X-Rspamd-Server: rspam06 X-Rspamd-Queue-Id: B3EFCB000183 X-Stat-Signature: uxeghwpp31rtimfe9tu7ymht39n3if3y X-HE-Tag: 1630594120-524078 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 Thu, Sep 02, 2021 at 09:28:42AM +0200, David Hildenbrand wrote: > On 01.09.21 22:57, Peter Xu wrote: > > Instead of trying to introduce one variable for every new zap_details fields, > > let's introduce a flag so that it can start to encode true/false informations. > > > > Let's start to use this flag first to clean up the only check_mapping variable. > > Firstly, the name "check_mapping" implies this is a "boolean", but actually it > > stores the mapping inside, just in a way that it won't be set if we don't want > > to check the mapping. > > > > To make things clearer, introduce the 1st zap flag ZAP_FLAG_CHECK_MAPPING, so > > that we only check against the mapping if this bit set. At the same time, we > > can rename check_mapping into zap_mapping and set it always. > > > > Since at it, introduce another helper zap_check_mapping_skip() and use it in > > zap_pte_range() properly. > > > > Some old comments have been removed in zap_pte_range() because they're > > duplicated, and since now we're with ZAP_FLAG_CHECK_MAPPING flag, it'll be very > > easy to grep this information by simply grepping the flag. > > > > It'll also make life easier when we want to e.g. pass in zap_flags into the > > callers like unmap_mapping_pages() (instead of adding new booleans besides the > > even_cows parameter). > > > > Signed-off-by: Peter Xu > > --- > > include/linux/mm.h | 19 ++++++++++++++++++- > > mm/memory.c | 34 ++++++++++------------------------ > > 2 files changed, 28 insertions(+), 25 deletions(-) > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h > > index 69259229f090..fcbc1c4f8e8e 100644 > > --- a/include/linux/mm.h > > +++ b/include/linux/mm.h > > @@ -1716,14 +1716,31 @@ static inline bool can_do_mlock(void) { return false; } > > extern int user_shm_lock(size_t, struct ucounts *); > > extern void user_shm_unlock(size_t, struct ucounts *); > > +/* Whether to check page->mapping when zapping */ > > +#define ZAP_FLAG_CHECK_MAPPING BIT(0) > > So we want to go full way, like: > > typedef int __bitwise zap_flags_t; > > #define ZAP_FLAG_CHECK_MAPPING ((__force zap_flags_t)BIT(0)) Sure. > > > + > > /* > > * Parameter block passed down to zap_pte_range in exceptional cases. > > */ > > struct zap_details { > > - struct address_space *check_mapping; /* Check page->mapping if set */ > > + struct address_space *zap_mapping; > > struct page *single_page; /* Locked page to be unmapped */ > > + unsigned long zap_flags; > > Why call it "zap_*" if everything in the structure is related to zapping? > IOW, simply "mapping", "flags" would be good enough. Not sure if it's a good habit or bad - it's just for tagging system to be able to identify other "mapping" variables, or a simple grep with the name. So I normally prefix fields with some special wording to avoid collisions. > > > }; > > +/* Return true if skip zapping this page, false otherwise */ > > +static inline bool > > +zap_skip_check_mapping(struct zap_details *details, struct page *page) > > +{ > > + if (!details || !page) > > + return false; > > + > > + if (!(details->zap_flags & ZAP_FLAG_CHECK_MAPPING)) > > + return false; > > + > > + return details->zap_mapping != page_rmapping(page); > > +} > > I'm confused, why isn't "!details->zap_mapping" vs. "details->zap_mapping" > sufficient? I can see that you may need flags for other purposes (next > patch), but why do we need it here? > > Factoring it out into this helper is a nice cleanup, though. But I'd just > not introduce ZAP_FLAG_CHECK_MAPPING. Yes I think it's okay. I wanted to separate them as they're fundamentall two things to me. Example: what if the mapping we want to check is NULL itself (remove private pages only; though it may not have a real user at least so far)? In that case one variable won't be able to cover it. But indeed Matthew raised similar comment, so it seems to be a common preference. No strong opinion on my side, let me coordinate with it. Thanks for looking, -- Peter Xu