On 9/29/2020 11:13 PM, Jason Gunthorpe wrote:
On Tue, Sep 29, 2020 at 11:09:43PM +0300, Yishai Hadas wrote:
On 9/29/2020 10:27 PM, Jason Gunthorpe wrote:
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;
Did you mean
*dma_addr = (*dma_addr & (ODP_DMA_ADDR_MASK)) | access_mask;
Probably
flags. (see ODP_DMA_ADDR_MASK). Also, if we went through a
read->write access without invalidation why do we need to mask at
all ? the new access_mask should have the write access.
Feels like a good idea to be safe here
It followed your note from V1 that the extra mask was really redundant,
the original code also didn't have it, but up-to-you.
+ 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 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.
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.
This should also
WARN_ON(umem_odp->dma_list[dma_index]);
Can you add it locally if you prefer the above approach ?
And all the pr_debugs around this code being touched should become
mlx5_ib_dbg
We are in IB core, why mlx5_ib_debug ?
oops, dev_dbg
Can it can done locally ?
Jason
