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

[John Hubbard]

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. 

thanks,
-- 
John Hubbard
NVIDIAr