Re: [PATCH rdma-next v2 1/4] IB/core: Improve ODP to use hmm_range_fault()

[Jason Gunthorpe <jgg@nvidia.com>]

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