[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[Jens Axboe]

On 10/12/2017 04:45 PM, Bart Van Assche wrote:
> Many kernel drivers contain code that allocate and free both a
> scatterlist and the pages that populate that scatterlist.
> Introduce functions under lib/ that perform these tasks instead
> of duplicating this functionality in multiple drivers.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche at wdc.com>
> ---
>  include/linux/sgl_alloc.h |  16 ++++++++
>  lib/Kconfig               |   4 ++
>  lib/Makefile              |   1 +
>  lib/sgl_alloc.c           | 102 ++++++++++++++++++++++++++++++++++++++++++++++
>  4 files changed, 123 insertions(+)
>  create mode 100644 include/linux/sgl_alloc.h
>  create mode 100644 lib/sgl_alloc.c
> 
> diff --git a/include/linux/sgl_alloc.h b/include/linux/sgl_alloc.h
> new file mode 100644
> index 000000000000..a0f719690c9e
> --- /dev/null
> +++ b/include/linux/sgl_alloc.h
> @@ -0,0 +1,16 @@
> +#ifndef _SGL_ALLOC_H_
> +#define _SGL_ALLOC_H_
> +
> +#include <linux/types.h> /* bool, gfp_t */
> +
> +struct scatterlist;
> +
> +struct scatterlist *sgl_alloc_order(unsigned long long length,
> +				    unsigned int order, unsigned int *nent_p,
> +				    gfp_t gfp, bool chainable);
> +struct scatterlist *sgl_alloc(unsigned long long length, unsigned int *nent_p,
> +			      gfp_t gfp);
> +void sgl_free_order(struct scatterlist *sgl, int order);
> +void sgl_free(struct scatterlist *sgl);
> +
> +#endif /* _SGL_ALLOC_H_ */

Should this just go into linux/scatterlist.h instead of creating a new header
file?

-- 
Jens Axboe

[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[Bart Van Assche]

On Thu, 2017-10-12@16:52 -0600, Jens Axboe wrote:
> On 10/12/2017 04:45 PM, Bart Van Assche wrote:
> > +++ b/include/linux/sgl_alloc.h
> > @@ -0,0 +1,16 @@
> > +#ifndef _SGL_ALLOC_H_
> > +#define _SGL_ALLOC_H_
> > +
> > +#include <linux/types.h> /* bool, gfp_t */
> > +
> > +struct scatterlist;
> > +
> > +struct scatterlist *sgl_alloc_order(unsigned long long length,
> > +				    unsigned int order, unsigned int *nent_p,
> > +				    gfp_t gfp, bool chainable);
> > +struct scatterlist *sgl_alloc(unsigned long long length, unsigned int *nent_p,
> > +			      gfp_t gfp);
> > +void sgl_free_order(struct scatterlist *sgl, int order);
> > +void sgl_free(struct scatterlist *sgl);
> > +
> > +#endif /* _SGL_ALLOC_H_ */
> 
> Should this just go into linux/scatterlist.h instead of creating a new header
> file?

That's something I can change. I don't have a strong opinion about whether
these declarations should go into a new header file or into linux/scatterlist.h.

Bart.

[PATCH 3/8] nvmet/fc: Use sgl_alloc() and sgl_free()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 4/8] nvmet/rdma: Use sgl_alloc() and sgl_free()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[Johannes Thumshirn]

On Thu, Oct 12, 2017@11:00:10PM +0000, Bart Van Assche wrote:
> On Thu, 2017-10-12@16:52 -0600, Jens Axboe wrote:
> > On 10/12/2017 04:45 PM, Bart Van Assche wrote:
> > > +++ b/include/linux/sgl_alloc.h
> > > @@ -0,0 +1,16 @@
> > > +#ifndef _SGL_ALLOC_H_
> > > +#define _SGL_ALLOC_H_
> > > +
> > > +#include <linux/types.h> /* bool, gfp_t */
> > > +
> > > +struct scatterlist;
> > > +
> > > +struct scatterlist *sgl_alloc_order(unsigned long long length,
> > > +				    unsigned int order, unsigned int *nent_p,
> > > +				    gfp_t gfp, bool chainable);
> > > +struct scatterlist *sgl_alloc(unsigned long long length, unsigned int *nent_p,
> > > +			      gfp_t gfp);
> > > +void sgl_free_order(struct scatterlist *sgl, int order);
> > > +void sgl_free(struct scatterlist *sgl);
> > > +
> > > +#endif /* _SGL_ALLOC_H_ */
> > 
> > Should this just go into linux/scatterlist.h instead of creating a new header
> > file?
> 
> That's something I can change. I don't have a strong opinion about whether
> these declarations should go into a new header file or into linux/scatterlist.h.

Yes please do (and the implementaion should go into lib/scatterlist.c).
Additionally please get rid of the new CONFIG_SGL_ALLOC as well.

Thanks,
	Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 6/8] scsi/ipr: Use sgl_alloc_order() and sgl_free_order()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 7/8] scsi/pmcraid: Remove an unused structure member

[Johannes Thumshirn]

Fair enough,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 8/8] scsi/pmcraid: Use sgl_alloc_order() and sgl_free_order()

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[Jens Axboe]

On 10/12/2017 05:00 PM, Bart Van Assche wrote:
> On Thu, 2017-10-12@16:52 -0600, Jens Axboe wrote:
>> On 10/12/2017 04:45 PM, Bart Van Assche wrote:
>>> +++ b/include/linux/sgl_alloc.h
>>> @@ -0,0 +1,16 @@
>>> +#ifndef _SGL_ALLOC_H_
>>> +#define _SGL_ALLOC_H_
>>> +
>>> +#include <linux/types.h> /* bool, gfp_t */
>>> +
>>> +struct scatterlist;
>>> +
>>> +struct scatterlist *sgl_alloc_order(unsigned long long length,
>>> +				    unsigned int order, unsigned int *nent_p,
>>> +				    gfp_t gfp, bool chainable);
>>> +struct scatterlist *sgl_alloc(unsigned long long length, unsigned int *nent_p,
>>> +			      gfp_t gfp);
>>> +void sgl_free_order(struct scatterlist *sgl, int order);
>>> +void sgl_free(struct scatterlist *sgl);
>>> +
>>> +#endif /* _SGL_ALLOC_H_ */
>>
>> Should this just go into linux/scatterlist.h instead of creating a new header
>> file?
> 
> That's something I can change. I don't have a strong opinion about whether
> these declarations should go into a new header file or into linux/scatterlist.h.

I think you should make that change, keep things simpler. As Johannes
suggested, you could move the code to lib/scatterlist.c as well. I would
recommend keeping the kconfig option to only build it when needed, though.

-- 
Jens Axboe

[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[Randy Dunlap]

On 10/12/17 15:45, Bart Van Assche wrote:

> diff --git a/lib/sgl_alloc.c b/lib/sgl_alloc.c
> new file mode 100644
> index 000000000000..d96b395dd5c8
> --- /dev/null
> +++ b/lib/sgl_alloc.c
> @@ -0,0 +1,102 @@

> +/**
> + * sgl_free_order - free a scatterlist and its pages
> + * @sg: Scatterlist with one or more elements

      @sgl:

> + * @order: Second argument for __free_pages()
> + */
> +void sgl_free_order(struct scatterlist *sgl, int order)
> +{
> +	struct scatterlist *sg;
> +	struct page *page;
> +
> +	for (sg = sgl; sg; sg = sg_next(sg)) {
> +		page = sg_page(sg);
> +		if (page)
> +			__free_pages(page, order);
> +	}
> +	kfree(sgl);
> +}
> +EXPORT_SYMBOL(sgl_free_order);
> +
> +/**
> + * sgl_free - free a scatterlist and its pages
> + * @sg: Scatterlist with one or more elements

      @sgl:

> + */
> +void sgl_free(struct scatterlist *sgl)
> +{
> +	sgl_free_order(sgl, 0);
> +}
> +EXPORT_SYMBOL(sgl_free);
> 

ta.
-- 
~Randy

[PATCH 1/8] lib: Introduce sgl_alloc() and sgl_free()

[Bart Van Assche]

On Fri, 2017-10-13@10:43 -0700, Randy Dunlap wrote:
> On 10/12/17 15:45, Bart Van Assche wrote:
> > + * @sg: Scatterlist with one or more elements
> 
>       @sgl:

Thanks Randy. I will fix this before I repost this series.

Bart.