On Wed, Feb 20, 2019 at 04:37:46PM +0200, Leon Romanovsky wrote:
> On Tue, Feb 19, 2019 at 08:57:40AM -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.
> >
[....]
> > +struct scatterlist *ib_umem_add_sg_table(struct scatterlist *sg,
> > + struct page **page_list,
> > + unsigned long npages,
> > + int *nents)
> > +{
> > + unsigned long first_pfn;
> > + unsigned long i = 0;
> > + bool update_cur_sg = false;
> > + bool first = !sg_page(sg) ? true : false;
>
> For "bool" declarations you don't need to set explicitly.
> "bool first = !sg_page(sg)" is the same as you wrote.
Yes. Thanks :)
>
> > +
> > + /* Check if new page_list is contiguous with end of previous page_list
> > + */
> > + if (!first &&
> > + (page_to_pfn(sg_page(sg)) + (sg->length >> PAGE_SHIFT) ==
> > + 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++)
> > + ;
> > +
> > + /* Squash N contiguous pages from page_list into current sge */
> > + if (update_cur_sg) {
> > + sg->length += len << PAGE_SHIFT;
> > + sg_set_page(sg, sg_page(sg), sg->length, 0);
> > + update_cur_sg = false;
> > + continue;
> > + }
> > +
> > + /* Squash N contiguous pages into next sge or first sge */
> > + if (!first)
> > + sg = sg_next(sg);
>
> How will it work with last element? For such case, sg will be NULL.
We have allocated a scatterlist array sized equal to npages and wont iterate past
end of the list. In worst case where the entire page list is discontiguous, the last
page would be assigned to last sg.
> > +
> > + (*nents)++;
> > + sg->length = len << PAGE_SHIFT;
>
> And you will derefence it here.
>
> > + sg_set_page(sg, first_page, sg->length, 0);
> > + first = false;
> > + }
> > +
> > + return sg;
> > +}
> > +
>
> Thanks
