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, [email protected] wrote: >>> From: Jérôme Glisse <[email protected]> >>> >>> 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 <[email protected]> >>> Reviewed-by: Ralph Campbell <[email protected]> >>> Cc: Andrew Morton <[email protected]> >>> Cc: John Hubbard <[email protected]> >>> Cc: Dan Williams <[email protected]> >>> --- >>> 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
