Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 17 Feb 2022 12:46:32 +0800
From:   Peter Xu <peterx@redhat.com>
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     linux-kernel@vger.kernel.org, linux-mm@kvack.org,
        Andrew Morton <akpm@linux-foundation.org>,
        "Kirill A . Shutemov" <kirill@shutemov.name>,
        Matthew Wilcox <willy@infradead.org>,
        Yang Shi <shy828301@gmail.com>,
        Andrea Arcangeli <aarcange@redhat.com>,
        Alistair Popple <apopple@nvidia.com>,
        David Hildenbrand <david@redhat.com>,
        Vlastimil Babka <vbabka@suse.cz>,
        Hugh Dickins <hughd@google.com>
Subject: Re: [PATCH v4 1/4] mm: Don't skip swap entry even if zap_details
 specified
Message-ID: <Yg3TKD6i9JjoRe80@xz-m1.local>
References: <20220216094810.60572-1-peterx@redhat.com>
 <20220216094810.60572-2-peterx@redhat.com>
 <da98141f-c8d8-f796-d8ec-352d9b2abd6a@nvidia.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <da98141f-c8d8-f796-d8ec-352d9b2abd6a@nvidia.com>
Precedence: bulk

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 <jhubbard@nvidia.com>

Thanks,

-- 
Peter Xu