On Wed, Sep 30, 2020 at 12:34:55AM +0300, Yishai Hadas wrote:
> 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.
The error code should be preserved, but not all callers care, like
prefetch for instance
> 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 ?
The HW must trigger fail if it reaches a pfn that isn't valid. The
return code is an indirect indication this happened.
Please send an update tomorre with these small changes since it is
late for me now
Jason
