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: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.

thanks,
-- 
John Hubbard
NVIDIA