Re: [PATCH v2 07/11] mm/hmm: add default fault flags to avoid the need to pre-fill pfns arrays.

[Jerome Glisse]

On Thu, Mar 28, 2019 at 03:40:42PM -0700, John Hubbard wrote:
> On 3/28/19 3:31 PM, Jerome Glisse wrote:
> > On Thu, Mar 28, 2019 at 03:19:06PM -0700, John Hubbard wrote:
> >> On 3/28/19 3:12 PM, Jerome Glisse wrote:
> >>> On Thu, Mar 28, 2019 at 02:59:50PM -0700, John Hubbard wrote:
> >>>> On 3/25/19 7:40 AM, jglisse@xxxxxxxxxx wrote:
> >>>>> From: Jérôme Glisse <jglisse@xxxxxxxxxx>
> >>>>>
> >>>>> The HMM mirror API can be use in two fashions. The first one where the HMM
> >>>>> user coalesce multiple page faults into one request and set flags per pfns
> >>>>> for of those faults. The second one where the HMM user want to pre-fault a
> >>>>> range with specific flags. For the latter one it is a waste to have the user
> >>>>> pre-fill the pfn arrays with a default flags value.
> >>>>>
> >>>>> This patch adds a default flags value allowing user to set them for a range
> >>>>> without having to pre-fill the pfn array.
> >>>>>
> >>>>> Signed-off-by: Jérôme Glisse <jglisse@xxxxxxxxxx>
> >>>>> Reviewed-by: Ralph Campbell <rcampbell@xxxxxxxxxx>
> >>>>> Cc: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> >>>>> Cc: John Hubbard <jhubbard@xxxxxxxxxx>
> >>>>> Cc: Dan Williams <dan.j.williams@xxxxxxxxx>
> >>>>> ---
> >>>>>  include/linux/hmm.h |  7 +++++++
> >>>>>  mm/hmm.c            | 12 ++++++++++++
> >>>>>  2 files changed, 19 insertions(+)
> >>>>>
> >>>>> diff --git a/include/linux/hmm.h b/include/linux/hmm.h
> >>>>> index 79671036cb5f..13bc2c72f791 100644
> >>>>> --- a/include/linux/hmm.h
> >>>>> +++ b/include/linux/hmm.h
> >>>>> @@ -165,6 +165,8 @@ enum hmm_pfn_value_e {
> >>>>>   * @pfns: array of pfns (big enough for the range)
> >>>>>   * @flags: pfn flags to match device driver page table
> >>>>>   * @values: pfn value for some special case (none, special, error, ...)
> >>>>> + * @default_flags: default flags for the range (write, read, ...)
> >>>>> + * @pfn_flags_mask: allows to mask pfn flags so that only default_flags matter
> >>>>>   * @pfn_shifts: pfn shift value (should be <= PAGE_SHIFT)
> >>>>>   * @valid: pfns array did not change since it has been fill by an HMM function
> >>>>>   */
> >>>>> @@ -177,6 +179,8 @@ struct hmm_range {
> >>>>>  	uint64_t		*pfns;
> >>>>>  	const uint64_t		*flags;
> >>>>>  	const uint64_t		*values;
> >>>>> +	uint64_t		default_flags;
> >>>>> +	uint64_t		pfn_flags_mask;
> >>>>>  	uint8_t			pfn_shift;
> >>>>>  	bool			valid;
> >>>>>  };
> >>>>> @@ -521,6 +525,9 @@ static inline int hmm_vma_fault(struct hmm_range *range, bool block)
> >>>>>  {
> >>>>>  	long ret;
> >>>>>  
> >>>>> +	range->default_flags = 0;
> >>>>> +	range->pfn_flags_mask = -1UL;
> >>>>
> >>>> Hi Jerome,
> >>>>
> >>>> This is nice to have. Let's constrain it a little bit more, though: the pfn_flags_mask
> >>>> definitely does not need to be a run time value. And we want some assurance that
> >>>> the mask is 
> >>>> 	a) large enough for the flags, and
> >>>> 	b) small enough to avoid overrunning the pfns field.
> >>>>
> >>>> Those are less certain with a run-time struct field, and more obviously correct with
> >>>> something like, approximately:
> >>>>
> >>>>  	#define PFN_FLAGS_MASK 0xFFFF
> >>>>
> >>>> or something.
> >>>>
> >>>> In other words, this is more flexibility than we need--just a touch too much,
> >>>> IMHO.
> >>>
> >>> This mirror the fact that flags are provided as an array and some devices use
> >>> the top bits for flags (read, write, ...). So here it is the safe default to
> >>> set it to -1. If the caller want to leverage this optimization it can override
> >>> the default_flags value.
> >>>
> >>
> >> Optimization? OK, now I'm a bit lost. Maybe this is another place where I could
> >> use a peek at the calling code. The only flags I've seen so far use the bottom
> >> 3 bits and that's it. 
> >>
> >> Maybe comments here?
> >>
> >>>>
> >>>>> +
> >>>>>  	ret = hmm_range_register(range, range->vma->vm_mm,
> >>>>>  				 range->start, range->end);
> >>>>>  	if (ret)
> >>>>> diff --git a/mm/hmm.c b/mm/hmm.c
> >>>>> index fa9498eeb9b6..4fe88a196d17 100644
> >>>>> --- a/mm/hmm.c
> >>>>> +++ b/mm/hmm.c
> >>>>> @@ -415,6 +415,18 @@ static inline void hmm_pte_need_fault(const struct hmm_vma_walk *hmm_vma_walk,
> >>>>>  	if (!hmm_vma_walk->fault)
> >>>>>  		return;
> >>>>>  
> >>>>> +	/*
> >>>>> +	 * So we not only consider the individual per page request we also
> >>>>> +	 * consider the default flags requested for the range. The API can
> >>>>> +	 * be use in 2 fashions. The first one where the HMM user coalesce
> >>>>> +	 * multiple page fault into one request and set flags per pfns for
> >>>>> +	 * of those faults. The second one where the HMM user want to pre-
> >>>>> +	 * fault a range with specific flags. For the latter one it is a
> >>>>> +	 * waste to have the user pre-fill the pfn arrays with a default
> >>>>> +	 * flags value.
> >>>>> +	 */
> >>>>> +	pfns = (pfns & range->pfn_flags_mask) | range->default_flags;
> >>>>
> >>>> Need to verify that the mask isn't too large or too small.
> >>>
> >>> I need to check agin but default flag is anded somewhere to limit
> >>> the bit to the one we expect.
> >>
> >> Right, but in general, the *mask* could be wrong. It would be nice to have
> >> an assert, and/or a comment, or something to verify the mask is proper.
> >>
> >> Really, a hardcoded mask is simple and correct--unless it *definitely* must
> >> vary for devices of course.
> > 
> > Ok so re-read the code and it is correct. The helper for compatibility with
> > old API (so that i do not break nouveau upstream code) initialize those to
> > the safe default ie:
> > 
> > range->default_flags = 0;
> > range->pfn_flags_mask = -1;
> > 
> > Which means that in the above comment we are in the case where it is the
> > individual entry within the pfn array that will determine if we fault or
> > not.
> > 
> > Driver using the new API can either use this safe default or use the
> > second case in the above comment and set default_flags to something
> > else than 0.
> > 
> > Note that those default_flags are not set in the final result they are
> > use to determine if we need to do a page fault. For instance if you set
> > the write bit in the default flags then the pfns computed above will
> > have the write bit set and when we compare with the CPU pte if the CPU
> > pte do not have the write bit set then we will fault. What matter is
> > that in this case the value within the pfns array is totaly pointless
> > ie we do not care what it is, it will not affect the decission ie the
> > decision is made by looking at the default flags.
> > 
> > Hope this clarify thing. You can look at the ODP patch to see how it
> > is use:
> > 
> > https://cgit.freedesktop.org/~glisse/linux/commit/?h=hmm-odp-v2&id=eebd4f3095290a16ebc03182e2d3ab5dfa7b05ec
> > 
> 
> Hi Jerome,
> 
> I think you're talking about flags, but I'm talking about the mask. The 
> above link doesn't appear to use the pfn_flags_mask, and the default_flags 
> that it uses are still in the same lower 3 bits:
> 
> +static uint64_t odp_hmm_flags[HMM_PFN_FLAG_MAX] = {
> +	ODP_READ_BIT,	/* HMM_PFN_VALID */
> +	ODP_WRITE_BIT,	/* HMM_PFN_WRITE */
> +	ODP_DEVICE_BIT,	/* HMM_PFN_DEVICE_PRIVATE */
> +};
> 
> So I still don't see why we need the flexibility of a full 0xFFFFFFFFFFFFFFFF
> mask, that is *also* runtime changeable. 

So the pfn array is using a device driver specific format and we have
no idea nor do we need to know where the valid, write, ... bit are in
that format. Those bits can be in the top 60 bits like 63, 62, 61, ...
we do not care. They are device with bit at the top and for those you
need a mask that allows you to mask out those bits or not depending on
what the user want to do.

The mask here is against an _unknown_ (from HMM POV) format. So we can
not presume where the bits will be and thus we can not presume what a
proper mask is.

So that's why a full unsigned long mask is use here.

Maybe an example will help let say the device flag are:
    VALID (1 << 63)
    WRITE (1 << 62)

Now let say that device wants to fault with at least read a range
it does set:
    range->default_flags = (1 << 63)
    range->pfn_flags_mask = 0;

This will fill fault all page in the range with at least read
permission.

Now let say it wants to do the same except for one page in the range
for which its want to have write. Now driver set:
    range->default_flags = (1 << 63);
    range->pfn_flags_mask = (1 << 62);
    range->pfns[index_of_write] = (1 << 62);

With this HMM will fault in all page with at least read (ie valid)
and for the address: range->start + index_of_write << PAGE_SHIFT it
will fault with write permission ie if the CPU pte does not have
write permission set then handle_mm_fault() will be call asking for
write permission.


Note that in the above HMM will populate the pfns array with write
permission for any entry that have write permission within the CPU
pte ie the default_flags and pfn_flags_mask is only the minimun
requirement but HMM always returns all the flag that are set in the
CPU pte.


Now let say you are an "old" driver like nouveau upstream, then it
means that you are setting each individual entry within range->pfns
with the exact flags you want for each address hence here what you
want is:
    range->default_flags = 0;
    range->pfn_flags_mask = -1UL;

So that what we do is (for each entry):
    (range->pfns[index] & range->pfn_flags_mask) | range->default_flags
and we end up with the flags that were set by the driver for each of
the individual range->pfns entries.


Does this help ?

Cheers,
Jérôme