On 9/29/2020 11:13 PM, Jason Gunthorpe wrote:
+ WARN_ON(!(range.hmm_pfns[pfn_index] & HMM_PFN_VALID));
+ hmm_order = hmm_pfn_to_map_order(range.hmm_pfns[pfn_index]);
+ /* If a hugepage was detected and ODP wasn't set for, the umem
+ * page_shift will be used, the opposite case is an error.
+ */
+ if (hmm_order + PAGE_SHIFT < page_shift) {
+ ret = -EINVAL;
+ pr_debug("%s: un-expected hmm_order %d, page_shift %d\n",
+ __func__, hmm_order, page_shift);
break;
}
I think this break should be a continue here. There is no reason not
to go to the next aligned PFN and try to sync as much as possible.
This might happen if the application didn't honor the contract to use
hugepages for the full range despite that it sets IB_ACCESS_HUGETLB, right ?
Yes
Do we still need to sync as much as possible in that case ? I
believe that we may consider return an error in this case to let
application be aware of as was before this series.
We might be prefetching or something weird where it could make sense.
In addition to my previous note here as of below [1], ignoring the clear
error case might break some testing that expects to get an error in this
case when the contract was not honored.
Also not sure how the HW will behave, won't that cause an extra /
infinite call to the driver to page fault for the missing data as the
result will be success but no dma will be provided ?
As of that I believe that better leave the code as is, what do you think ?
[1] "Not sure about the exact scenario rather than an application issue,
this follows the original code in this area, maybe better sets an error
in the clear application error."
Yishai
