diff mbox

[v2,5/8] target: Use sgl_alloc_order() and sgl_free()

Message ID 20171016224940.1332-6-bart.vanassche@wdc.com (mailing list archive)
State New, archived
Headers show

Commit Message

Bart Van Assche Oct. 16, 2017, 10:49 p.m. UTC
Use the sgl_alloc_order() and sgl_free() functions instead of open
coding these functions.

Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.com>
Cc: Sagi Grimberg <sagi@grimberg.me>
---
 drivers/target/Kconfig                 |  1 +
 drivers/target/target_core_transport.c | 46 +++-------------------------------
 2 files changed, 5 insertions(+), 42 deletions(-)

Comments

Hannes Reinecke Oct. 17, 2017, 6:14 a.m. UTC | #1
On 10/17/2017 12:49 AM, Bart Van Assche wrote:
> Use the sgl_alloc_order() and sgl_free() functions instead of open
> coding these functions.
> 
> Signed-off-by: Bart Van Assche <bart.vanassche@wdc.com>
> Cc: Nicholas A. Bellinger <nab@linux-iscsi.org>
> Cc: Christoph Hellwig <hch@lst.de>
> Cc: Hannes Reinecke <hare@suse.com>
> Cc: Sagi Grimberg <sagi@grimberg.me>
> ---
>  drivers/target/Kconfig                 |  1 +
>  drivers/target/target_core_transport.c | 46 +++-------------------------------
>  2 files changed, 5 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
> index e2bc99980f75..4c44d7bed01a 100644
> --- a/drivers/target/Kconfig
> +++ b/drivers/target/Kconfig
> @@ -5,6 +5,7 @@ menuconfig TARGET_CORE
>  	select CONFIGFS_FS
>  	select CRC_T10DIF
>  	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
> +	select SGL_ALLOC
>  	default n
>  	help
>  	Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 836d552b0385..9bbd08be9d60 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2293,13 +2293,7 @@ static void target_complete_ok_work(struct work_struct *work)
>  
>  void target_free_sgl(struct scatterlist *sgl, int nents)
>  {
> -	struct scatterlist *sg;
> -	int count;
> -
> -	for_each_sg(sgl, sg, nents, count)
> -		__free_page(sg_page(sg));
> -
> -	kfree(sgl);
> +	sgl_free(sgl);
>  }
>  EXPORT_SYMBOL(target_free_sgl);
>  
> @@ -2423,42 +2417,10 @@ int
>  target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
>  		 bool zero_page, bool chainable)
>  {
> -	struct scatterlist *sg;
> -	struct page *page;
> -	gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
> -	unsigned int nalloc, nent;
> -	int i = 0;
> -
> -	nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
> -	if (chainable)
> -		nalloc++;
> -	sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
> -	if (!sg)
> -		return -ENOMEM;
> +	gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
>  
> -	sg_init_table(sg, nalloc);
> -
> -	while (length) {
> -		u32 page_len = min_t(u32, length, PAGE_SIZE);
> -		page = alloc_page(GFP_KERNEL | zero_flag);
> -		if (!page)
> -			goto out;
> -
> -		sg_set_page(&sg[i], page, page_len, 0);
> -		length -= page_len;
> -		i++;
> -	}
> -	*sgl = sg;
> -	*nents = nent;
> -	return 0;
> -
> -out:
> -	while (i > 0) {
> -		i--;
> -		__free_page(sg_page(&sg[i]));
> -	}
> -	kfree(sg);
> -	return -ENOMEM;
> +	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
> +	return *sgl ? 0 : -ENOMEM;
>  }
>  EXPORT_SYMBOL(target_alloc_sgl);
>  
> The calling convention from target_alloc_sgl() is decidedly dodgy.
Can't we convert it into returning the sgl, and remove the first parameter?

Cheers,

Hannes
Bart Van Assche Oct. 17, 2017, 2:31 p.m. UTC | #2
On Tue, 2017-10-17 at 08:14 +0200, Hannes Reinecke wrote:
> On 10/17/2017 12:49 AM, Bart Van Assche wrote:

> > [ ... ]

> >  void target_free_sgl(struct scatterlist *sgl, int nents)

> >  {

> > -	struct scatterlist *sg;

> > -	int count;

> > -

> > -	for_each_sg(sgl, sg, nents, count)

> > -		__free_page(sg_page(sg));

> > -

> > -	kfree(sgl);

> > +	sgl_free(sgl);

> >  }

> >  EXPORT_SYMBOL(target_free_sgl);

> >  

> > @@ -2423,42 +2417,10 @@ int

> >  target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,

> >  		 bool zero_page, bool chainable)

> >  {

> > -	[ ... ]

> > +	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);

> > +	return *sgl ? 0 : -ENOMEM;

> >  }

> >  EXPORT_SYMBOL(target_alloc_sgl);

> >  

> > The calling convention from target_alloc_sgl() is decidedly dodgy.

> 

> Can't we convert it into returning the sgl, and remove the first parameter?


Hello Hannes,

Another option is to remove the target_alloc_sgl() and target_free_sgl() functions
and to make LIO target drivers call sgl_alloc_order() and sgl_free_order() directly.
Do you perhaps have a preference for one of these two approaches?

Thanks,

Bart.
diff mbox

Patch

diff --git a/drivers/target/Kconfig b/drivers/target/Kconfig
index e2bc99980f75..4c44d7bed01a 100644
--- a/drivers/target/Kconfig
+++ b/drivers/target/Kconfig
@@ -5,6 +5,7 @@  menuconfig TARGET_CORE
 	select CONFIGFS_FS
 	select CRC_T10DIF
 	select BLK_SCSI_REQUEST # only for scsi_command_size_tbl..
+	select SGL_ALLOC
 	default n
 	help
 	Say Y or M here to enable the TCM Storage Engine and ConfigFS enabled
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 836d552b0385..9bbd08be9d60 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -2293,13 +2293,7 @@  static void target_complete_ok_work(struct work_struct *work)
 
 void target_free_sgl(struct scatterlist *sgl, int nents)
 {
-	struct scatterlist *sg;
-	int count;
-
-	for_each_sg(sgl, sg, nents, count)
-		__free_page(sg_page(sg));
-
-	kfree(sgl);
+	sgl_free(sgl);
 }
 EXPORT_SYMBOL(target_free_sgl);
 
@@ -2423,42 +2417,10 @@  int
 target_alloc_sgl(struct scatterlist **sgl, unsigned int *nents, u32 length,
 		 bool zero_page, bool chainable)
 {
-	struct scatterlist *sg;
-	struct page *page;
-	gfp_t zero_flag = (zero_page) ? __GFP_ZERO : 0;
-	unsigned int nalloc, nent;
-	int i = 0;
-
-	nalloc = nent = DIV_ROUND_UP(length, PAGE_SIZE);
-	if (chainable)
-		nalloc++;
-	sg = kmalloc_array(nalloc, sizeof(struct scatterlist), GFP_KERNEL);
-	if (!sg)
-		return -ENOMEM;
+	gfp_t gfp = GFP_KERNEL | (zero_page ? __GFP_ZERO : 0);
 
-	sg_init_table(sg, nalloc);
-
-	while (length) {
-		u32 page_len = min_t(u32, length, PAGE_SIZE);
-		page = alloc_page(GFP_KERNEL | zero_flag);
-		if (!page)
-			goto out;
-
-		sg_set_page(&sg[i], page, page_len, 0);
-		length -= page_len;
-		i++;
-	}
-	*sgl = sg;
-	*nents = nent;
-	return 0;
-
-out:
-	while (i > 0) {
-		i--;
-		__free_page(sg_page(&sg[i]));
-	}
-	kfree(sg);
-	return -ENOMEM;
+	*sgl = sgl_alloc_order(length, 0, chainable, gfp, nents);
+	return *sgl ? 0 : -ENOMEM;
 }
 EXPORT_SYMBOL(target_alloc_sgl);