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

[Jason Gunthorpe <jgg@nvidia.com>]

On Tue, Sep 22, 2020 at 11:21:01AM +0300, Leon Romanovsky wrote:

> +	if (!*dma_addr) {
> +		*dma_addr = ib_dma_map_page(dev, page, 0,
> +				1 << umem_odp->page_shift,
> +				DMA_BIDIRECTIONAL);
> +		if (ib_dma_mapping_error(dev, *dma_addr)) {
> +			*dma_addr = 0;
> +			return -EFAULT;
> +		}
> +		umem_odp->npages++;
> +	}
> +
> +	*dma_addr |= access_mask;

This does need some masking, the purpose of this is to update the
access flags in the case we hit a fault on a dma mapped thing. Looks
like this can happen on a read-only page becoming writable again
(wp_page_reuse() doesn't trigger notifiers)

It should also have a comment to that effect.

something like:

if (*dma_addr) {
    /*
     * If the page is already dma mapped it means it went through a
     * non-invalidating trasition, like read-only to writable. Resync the
     * flags.
     */
    *dma_addr = (*dma_addr & (~ODP_DMA_ADDR_MASK)) | access_mask;
    return;
}

new_dma_addr = ib_dma_map_page()
[..]
*dma_addr = new_dma_addr | access_mask

> +		WARN_ON(range.hmm_pfns[pfn_index] & HMM_PFN_ERROR);
> +		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 should also

  WARN_ON(umem_odp->dma_list[dma_index]);

And all the pr_debugs around this code being touched should become
mlx5_ib_dbg

Jason