Re: [Intel-gfx] [PATCH 3/4] lib/scatterlist: Introduce and export __sg_alloc_table_from_pages

[Tvrtko Ursulin]

On 03/08/2017 00:01, Andrew Morton wrote:
> On Wed, 2 Aug 2017 14:06:39 +0100 Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx> wrote:
>
> > Hi Andrew,
> >
> > We have a couple of small lib/scatterlist.c tidies here, plus exporting
> > the new API which allows drivers to control the maximum coalesced entry
> > as created by __sg_alloc_table_from_pages.
> >
> > I am looking for an ack to merge these three patches via the drm-intel tree.
> >
> >
> > ...
> >
> > On 27/07/2017 10:05, Tvrtko Ursulin wrote:
> > > From: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxx>
> > >
> > > Drivers like i915 benefit from being able to control the maxium
> > > size of the sg coallesced segment while building the scatter-
>
> "coalesced"

Oops, I've had this in other patches in the series. Fixed now.

> > > gather list.
> > >
> > > Introduce and export the __sg_alloc_table_from_pages function
> > > which will allow it that control.
> >
> > ...
> >
> > > --- a/lib/scatterlist.c
> > > +++ b/lib/scatterlist.c
> > > @@ -370,14 +370,15 @@ int sg_alloc_table(struct sg_table *table, unsigned int nents, gfp_t gfp_mask)
> > >    EXPORT_SYMBOL(sg_alloc_table);
> > >    
> > >    /**
> > > - * sg_alloc_table_from_pages - Allocate and initialize an sg table from
> > > - *			       an array of pages
> > > - * @sgt:	The sg table header to use
> > > - * @pages:	Pointer to an array of page pointers
> > > - * @n_pages:	Number of pages in the pages array
> > > - * @offset:     Offset from start of the first page to the start of a buffer
> > > - * @size:       Number of valid bytes in the buffer (after offset)
> > > - * @gfp_mask:	GFP allocation mask
> > > + * __sg_alloc_table_from_pages - Allocate and initialize an sg table from
> > > + *			         an array of pages
> > > + * @sgt:	 The sg table header to use
> > > + * @pages:	 Pointer to an array of page pointers
> > > + * @n_pages:	 Number of pages in the pages array
> > > + * @offset:      Offset from start of the first page to the start of a buffer
> > > + * @size:        Number of valid bytes in the buffer (after offset)
> > > + * @max_segment: Maximum size of a scatterlist node in bytes (page aligned)
> > > + * @gfp_mask:	 GFP allocation mask
> > >     *
> > >     *  Description:
> > >     *    Allocate and initialize an sg table from a list of pages. Contiguous
>
> The Description doesn't describe how this differs from
> sg_alloc_table_from_pages(), although it doesn't seem terribly
> important.

Well spotted, I've fixed this as well.

> > > +
> > > +/**
> > > + * sg_alloc_table_from_pages - Allocate and initialize an sg table from
> > > + *			       an array of pages
> > > + * @sgt:	 The sg table header to use
> > > + * @pages:	 Pointer to an array of page pointers
> > > + * @n_pages:	 Number of pages in the pages array
> > > + * @offset:      Offset from start of the first page to the start of a buffer
> > > + * @size:        Number of valid bytes in the buffer (after offset)
> > > + * @gfp_mask:	 GFP allocation mask
> > > + *
> > > + *  Description:
> > > + *    Allocate and initialize an sg table from a list of pages. Contiguous
> > > + *    ranges of the pages are squashed into a single scatterlist node. A user
> > > + *    may provide an offset at a start and a size of valid data in a buffer
> > > + *    specified by the page array. The returned sg table is released by
> > > + *    sg_free_table.
> > > + *
> > > + * Returns:
> > > + *   0 on success, negative error on failure
> > > + */
> > > +int sg_alloc_table_from_pages(struct sg_table *sgt, struct page **pages,
> > > +			      unsigned int n_pages, unsigned int offset,
> > > +			      unsigned long size, gfp_t gfp_mask)
> > > +{
> > > +	return __sg_alloc_table_from_pages(sgt, pages, n_pages, offset, size,
> > > +					   SCATTERLIST_MAX_SEGMENT, gfp_mask);
> > > +}
>
> Making this an inline would save a bunch or stack space in the callers?
>
> One could just add the new arg then run around and update the 10ish
> callers but I guess the new interface is OK.

On the first suggestion - there are other trivial wrappers in 
lib/scatterlist.c which could benefit from the same treatment (move to 
inline) so I opted not to do this in this patch but will send a follow up.

> Otherwise it's OK by me, please go ahead as proposed.

Thanks a lot!

Regards,

Tvrtko