LKML Archive on
help / color / mirror / Atom feed
From: Jerome Glisse <>
To: John Hubbard <>
Cc:, Andrew Morton <>,,
	Evgeny Baskakov <>,
	Ralph Campbell <>,
	Mark Hairgrove <>
Subject: Re: [PATCH 4/4] mm/hmm: change CPU page table snapshot functions to simplify drivers
Date: Fri, 16 Mar 2018 15:19:09 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On Thu, Mar 15, 2018 at 10:08:21PM -0700, John Hubbard wrote:
> On 03/15/2018 11:37 AM, wrote:
> > From: Jérôme Glisse <>
> > 
> > This change hmm_vma_fault() and hmm_vma_get_pfns() API to allow HMM
> > to directly write entry that can match any device page table entry
> > format. Device driver now provide an array of flags value and we use
> > enum to index this array for each flag.
> > 
> > This also allow the device driver to ask for write fault on a per page
> > basis making API more flexible to service multiple device page faults
> > in one go.
> > 
> Hi Jerome,
> This is a large patch, so I'm going to review it in two passes. The first 
> pass is just an overview plus the hmm.h changes (now), and tomorrow I will
> review the hmm.c, which is where the real changes are.
> Overview: the hmm.c changes are doing several things, and it is difficult to
> review, because refactoring, plus new behavior, makes diffs less useful here.
> It would probably be good to split the hmm.c changes into a few patches, such
> as:
> 	-- HMM_PFN_FLAG_* changes, plus function signature changes (mm_range* 
>            being passed to functions), and
>         -- New behavior in the page handling loops, and 
> 	-- Refactoring into new routines (hmm_vma_handle_pte, and others)
> That way, reviewers can see more easily that things are correct. 

So i resent patchset and i splitted this patch in many (11 patches now).
I included your comments so far in the new version so probably better if
you look at new one.


> > - * HMM_PFN_READ:  CPU page table has read permission set
> So why is it that we don't need the _READ flag anymore? I looked at the corresponding
> hmm.c but still don't quite get it. Is it that we just expect that _READ is
> always set if there is an entry at all? Or something else?

Explained why in the commit message, !READ when WRITE make no sense so
now VALID imply READ as does WRITE (write by itself is meaningless
without valid).


      reply	other threads:[~2018-03-16 19:19 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-15 18:36 [PATCH 0/4] hmm: fixes and documentations jglisse
2018-03-15 18:36 ` [PATCH 1/4] mm/hmm: documentation editorial update to HMM documentation jglisse
2018-03-15 18:36 ` [PATCH 2/4] mm/hmm: fix header file if/else/endif maze jglisse
2018-03-16  0:11   ` Balbir Singh
2018-03-17  0:53   ` kbuild test robot
2018-03-15 18:36 ` [PATCH 3/4] mm/hmm: HMM should have a callback before MM is destroyed jglisse
2018-03-15 22:48   ` Andrew Morton
2018-03-16  0:54     ` Jerome Glisse
2018-03-16  1:17       ` John Hubbard
2018-03-20 11:33     ` Michal Hocko
2018-03-20 14:45       ` Jerome Glisse
2018-03-15 18:37 ` [PATCH 4/4] mm/hmm: change CPU page table snapshot functions to simplify drivers jglisse
2018-03-16  5:08   ` John Hubbard
2018-03-16 19:19     ` Jerome Glisse [this message]

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:

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

  git send-email \ \ \ \ \ \ \ \ \ \

* 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).