lkml.org 
[lkml]   [2021]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    Date
    SubjectRe: [PATCH v3 4/20] PCI/P2PDMA: introduce helpers for dma_map_sg implementations



    On 2021-09-29 4:46 p.m., Jason Gunthorpe wrote:
    > On Wed, Sep 29, 2021 at 03:30:42PM -0600, Logan Gunthorpe wrote:
    >> On 2021-09-28 4:05 p.m., Jason Gunthorpe wrote:
    >> No, that's not a correct reading of the code. Every time there is a new
    >> pagemap, this code calculates the mapping type and bus offset. If a page
    >> comes along with a different page map,f it recalculates. This just
    >> reduces the overhead so that the calculation is done only every time a
    >> page with a different pgmap comes along and not doing it for every
    >> single page.
    >
    > Each 'struct scatterlist *sg' refers to a range of contiguous pfns
    > starting at page_to_pfn(sg_page()) and going for approx sg->length/PAGE_SIZE
    > pfns long.
    >

    Ugh, right. A bit contrived for consecutive pages to have different
    pgmaps and still be next to each other in a DMA transaction. But I guess
    it is technically possible and should be protected against.

    > sg_page() returns the first page, but nothing says that sg_page()+1
    > has the same pgmap.
    >
    > The code in this patch does check the first page of each sg in a
    > larger sgl.
    >
    >>> At least sg_alloc_append_table_from_pages() and probably something in
    >>> the block world should be updated to not combine struct pages with
    >>> different pgmaps, and this should be documented in scatterlist.*
    >>> someplace.
    >>
    >> There's no sane place to do this check. The code is designed to support
    >> mappings with different pgmaps.
    >
    > All places that generate compound sg's by aggregating multiple pages
    > need to include this check along side the check for physical
    > contiguity. There are not that many places but
    > sg_alloc_append_table_from_pages() is one of them:

    Yes. The block layer also does this. I believe a check in
    page_is_mergable() will be sufficient there.

    > @@ -470,7 +470,8 @@ int sg_alloc_append_table_from_pages(struct sg_append_table *sgt_append,
    >
    > /* Merge contiguous pages into the last SG */
    > prv_len = sgt_append->prv->length;
    > - while (n_pages && page_to_pfn(pages[0]) == paddr) {
    > + while (n_pages && page_to_pfn(pages[0]) == paddr &&
    > + sg_page(sgt_append->prv)->pgmap == pages[0]->pgmap) {

    I don't think it's correct to use pgmap without first checking if it is
    a zone device page. But your point is taken. I'll try to address this.

    Logan

    \
     
     \ /
      Last update: 2021-09-30 01:01    [W:2.644 / U:0.464 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site