On Mon, Dec 24, 2018 at 04:32:22PM -0600, Shiraz Saleem wrote:
> Combine contiguous regions of PAGE_SIZE pages
> into single scatter list entries while adding
> to the scatter table. This minimizes the number
> of the entries in the scatter list and reduces
> the DMA mapping overhead, particularly with the
> IOMMU.
>
> Additionally, update driver call sites to use the
> for_each_sg_page variant to iterate through the
> pages in the SGE. In certain call sites, there is
> the assumption of single SGE to page mapping and
> hence the iterator variant is required. In others,
> it is simpler to use the iterator variant.
>
> Suggested-by: Jason Gunthorpe <jgg@xxxxxxxx>
> Reviewed-by: Michael J. Ruhl <michael.j.ruhl@xxxxxxxxx>
> Signed-off-by: Shiraz Saleem <shiraz.saleem@xxxxxxxxx>
> drivers/infiniband/core/umem.c | 87 ++++++++---
> drivers/infiniband/hw/bnxt_re/ib_verbs.c | 21 +--
> drivers/infiniband/hw/bnxt_re/qplib_res.c | 9 +-
> drivers/infiniband/hw/cxgb3/iwch_provider.c | 27 ++--
> drivers/infiniband/hw/cxgb4/mem.c | 31 ++--
> drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 7 +-
> drivers/infiniband/hw/hns/hns_roce_hw_v2.c | 25 ++-
> drivers/infiniband/hw/hns/hns_roce_mr.c | 88 +++++------
> drivers/infiniband/hw/i40iw/i40iw_verbs.c | 35 ++---
> drivers/infiniband/hw/mthca/mthca_provider.c | 33 ++--
> drivers/infiniband/hw/nes/nes_verbs.c | 203 +++++++++++--------------
> drivers/infiniband/hw/ocrdma/ocrdma_verbs.c | 54 +++----
> drivers/infiniband/hw/qedr/verbs.c | 56 +++----
> drivers/infiniband/hw/vmw_pvrdma/pvrdma_misc.c | 21 +--
> drivers/infiniband/sw/rdmavt/mr.c | 8 +-
> drivers/infiniband/sw/rxe/rxe_mr.c | 7 +-
> 16 files changed, 342 insertions(+), 370 deletions(-)
>
> diff --git a/drivers/infiniband/core/umem.c b/drivers/infiniband/core/umem.c
> index c6144df..64bacc5 100644
> +++ b/drivers/infiniband/core/umem.c
> @@ -39,25 +39,23 @@
> #include <linux/export.h>
> #include <linux/hugetlb.h>
> #include <linux/slab.h>
> +#include <linux/pagemap.h>
> #include <rdma/ib_umem_odp.h>
>
> #include "uverbs.h"
>
> -
> static void __ib_umem_release(struct ib_device *dev, struct ib_umem *umem, int dirty)
> {
> - struct scatterlist *sg;
> + struct sg_page_iter sg_iter;
> struct page *page;
> - int i;
> + int nents = sg_nents(umem->sg_head.sgl);
I wonder if we should cache sg_nents in the umem? We compute it twice
and since this now directly adds each sgl we know the nents
naturally..
> +/* ib_umem_add_sg_table - Add N contiguous pages to scatter table
> + *
> + * cur: current scatterlist entry
> + * nxt: next scatterlist entry
> + * page_list: array of npage struct page pointers
> + * npages: number of pages in page_list
> + */
> +static void ib_umem_add_sg_table(struct scatterlist **cur,
> + struct scatterlist **nxt,
> + struct page **page_list,
> + unsigned long npages)
I think this is more understandable as:
struct scatterlist *ib_umem_add_sg_table(struct scatterlist *sgl,
struct page **page_list,
unsigned long npages,
bool first)
Where first is set if sgl does not already point to a valid entry, and
the return result is new end of the list.
> + /* Check if new page_list is contiguous with end of previous page_list*/
> + if (*cur != *nxt) {
> + cur_sglen = (*cur)->length >> PAGE_SHIFT;
> + first_pfn = page_to_pfn(sg_page(*cur));
> + if (first_pfn + cur_sglen == page_to_pfn(page_list[0]))
> + update_cur_sg = true;
> + }
> +
> + while (i != npages) {
> + unsigned long len;
> + struct page *first_page = page_list[i];
> +
> + first_pfn = page_to_pfn(first_page);
> +
> + /* Compute the number of contiguous pages we have starting
> + * at i
> + */
> + for (len = 0; i != npages &&
> + first_pfn + len == page_to_pfn(page_list[i]);
> + i++, len++);
The semicolon should be on a blank line alone for this construction
> +
> + /* Squash first N contiguous pages from page_list into current
> + * SGE
> + */
> + if (update_cur_sg) {
> + sg_set_page(*cur, sg_page(*cur),
> + (len + cur_sglen) * PAGE_SIZE, 0);
This would be nicer as (*cur)->length instead of cur_sglen
And if we are touching cur->length, it begs the question why not just
write cur->length += len << PAGE_SHIFT ?
> + sg_mark_end(sg_cur);
This seems like a good approach for now. We can look at trimming the
unused memory later..
Jason
