linux-mm.kvack.org archive mirror
 help / color / mirror / Atom feed
From: Johannes Weiner <hannes@cmpxchg.org>
To: Matthew Wilcox <willy@infradead.org>
Cc: Jan Kara <jack@suse.cz>,
	linux-mm@kvack.org, Andrew Morton <akpm@linux-foundation.org>,
	Hugh Dickins <hughd@google.com>,
	William Kucharski <william.kucharski@oracle.com>,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH 6/7] mm: Pass pvec directly to find_get_entries
Date: Tue, 25 Aug 2020 12:23:42 -0400	[thread overview]
Message-ID: <20200825162342.GC932571@cmpxchg.org> (raw)
In-Reply-To: <20200825132814.GO17456@casper.infradead.org>

On Tue, Aug 25, 2020 at 02:28:14PM +0100, Matthew Wilcox wrote:
> On Tue, Aug 25, 2020 at 02:33:24PM +0200, Jan Kara wrote:
> > On Mon 24-08-20 18:36:39, Matthew Wilcox wrote:
> > > We already have functions in filemap which take a pagevec, eg
> > > page_cache_delete_batch() and delete_from_page_cache_batch().
> > 
> > Right but those are really pretty internal helper functions so I don't
> > think they form or strong precedence.
> 
> To be honest, I saw that as being the way forward for the page cache APIs.
> If we're going to use a batching mechanism, it should be pagevecs, and
> it should be built into the page cache interfaces rather than hanging
> out off on the side.

Agreed.

> > > So if we're going to merge the two functions, it seems more natural to
> > > have it in filemap.c and called find_get_entries(), but I'm definitely
> > > open to persuasion on this!
> > 
> > I agree that having non-trivial xarray code in mm/swap.c isn't attractive
> > either. Dunno, I dislike the inconsistency between find_get_pages() and
> > find_get_entries() you create but they aren't completely consistent anyway
> > so I can live with that. Or we can just leave the pagevec_lookup_entries()
> > wrapper and the API will stay consistent...
> 
> I was thinking about this some more [1] [2].  I think we can get to the
> point where find_get_pages(), find_get_entries() and find_get_pages_tag()
> (and all their variants) end up taking a pagevec as their last argument.

Agreed.

> Also, I was thinking that all these names are wrong.  Really, they're
> mapping_get_pages(), mapping_get_entries() and mapping_get_marked_pages().
> So maybe I should move in that direction.

That sounds like a lateral move in naming to me. The mapping prefix is
a slight improvement, but without the "find" it sounds like a refcount
operation and hides the fact that this is doing some sort of lookup
and has higher complexity.

It also makes working on this code easier on people not yet familiar
with it at the cost of people familiar with it. Remembering new names
for known concepts is a ton of mental churn.

So IMO the new names should be unambigously and significantly better
than the old ones to justify this.

Signed: somebody who is still struggling with the change from
exceptional entries in the radix tree to value entries in the xarray
(Are pointers or integers the values? Aren't they both "values"?)


  parent reply	other threads:[~2020-08-25 16:24 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-08-19 15:05 [PATCH 0/7] Overhaul find_get_entries and pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-08-19 15:05 ` [PATCH 1/7] mm: Use pagevec_lookup in shmem_unlock_mapping Matthew Wilcox (Oracle)
2020-08-21 15:53   ` Jan Kara
2020-08-19 15:05 ` [PATCH 2/7] mm: Rewrite shmem_seek_hole_data Matthew Wilcox (Oracle)
2020-08-20 16:45   ` Mike Rapoport
2020-08-20 19:04     ` Matthew Wilcox
2020-08-19 15:05 ` [PATCH 3/7] mm: Add an 'end' parameter to find_get_entries Matthew Wilcox (Oracle)
2020-08-20 16:47   ` Mike Rapoport
2020-08-21 16:07   ` Jan Kara
2020-08-21 16:33     ` Matthew Wilcox
2020-08-21 18:06       ` Jan Kara
2020-08-19 15:05 ` [PATCH 4/7] mm: Add an 'end' parameter to pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-08-24 16:09   ` Jan Kara
2020-08-19 15:05 ` [PATCH 5/7] mm: Remove nr_entries parameter from pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-08-24 16:10   ` Jan Kara
2020-08-19 15:05 ` [PATCH 6/7] mm: Pass pvec directly to find_get_entries Matthew Wilcox (Oracle)
2020-08-24 16:16   ` Jan Kara
2020-08-24 17:36     ` Matthew Wilcox
2020-08-25 12:33       ` Jan Kara
2020-08-25 13:28         ` Matthew Wilcox
2020-08-25 15:24           ` Jan Kara
2020-08-25 16:23           ` Johannes Weiner [this message]
2020-08-19 15:05 ` [PATCH 7/7] mm: Remove pagevec_lookup_entries Matthew Wilcox (Oracle)
2020-08-22  2:34 ` [PATCH 0/7] Overhaul find_get_entries and pagevec_lookup_entries William Kucharski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200825162342.GC932571@cmpxchg.org \
    --to=hannes@cmpxchg.org \
    --cc=akpm@linux-foundation.org \
    --cc=hughd@google.com \
    --cc=jack@suse.cz \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=william.kucharski@oracle.com \
    --cc=willy@infradead.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).