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
