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

From: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Tvrtko Ursulin <tursulin@ursulin.net>,
	Intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org,
	Masahiro Yamada <yamada.masahiro@socionext.com>,
	Ben Widawsky <benjamin.widawsky@intel.com>,
	"Vetter, Daniel" <daniel.vetter@intel.com>
Subject: Re: [Intel-gfx] [PATCH 3/4] lib/scatterlist: Introduce and export __sg_alloc_table_from_pages
Date: Thu, 3 Aug 2017 10:21:19 +0100	[thread overview]
Message-ID: <4b50eddb-b8ce-7e8e-a3cc-71bec7252b32@linux.intel.com> (raw)
In-Reply-To: <20170802160153.dee32799b03f81b925cf5289@linux-foundation.org>

On 03/08/2017 00:01, Andrew Morton wrote:
> On Wed, 2 Aug 2017 14:06:39 +0100 Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com> 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@intel.com>
>>>
>>> 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