[PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[Bart Van Assche]

This patch does not change any functionality. Note: the code that frees
sess_cmd_map already uses kvfree() so that code does not need to be
modified.

Cc: Nicholas Bellinger <nab@linux-iscsi.org>
Cc: Mike Christie <mchristi@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>
Cc: Hannes Reinecke <hare@suse.de>
Cc: David Disseldorp <ddiss@suse.de>
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/target/target_core_transport.c | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 2cfd61d62e97..086cac18ebca 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -273,14 +273,10 @@ int transport_alloc_session_tags(struct se_session *se_sess,
 {
 	int rc;
 
-	se_sess->sess_cmd_map = kcalloc(tag_size, tag_num,
-					GFP_KERNEL | __GFP_NOWARN | __GFP_RETRY_MAYFAIL);
+	se_sess->sess_cmd_map = kvcalloc(tag_size, tag_num, GFP_KERNEL);
 	if (!se_sess->sess_cmd_map) {
-		se_sess->sess_cmd_map = vzalloc(array_size(tag_size, tag_num));
-		if (!se_sess->sess_cmd_map) {
-			pr_err("Unable to allocate se_sess->sess_cmd_map\n");
-			return -ENOMEM;
-		}
+		pr_err("Unable to allocate se_sess->sess_cmd_map\n");
+		return -ENOMEM;
 	}
 
 	rc = sbitmap_queue_init_node(&se_sess->sess_tag_pool, tag_num, -1,
-- 
2.19.1.930.g4563a0d9d0-goog

Re: [PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[David Disseldorp]

Looks reasonable. Any reason for dropping the extra __GFP flags?

Either way...
Reviewed-by: David Disseldorp <ddiss@suse.de>

Re: [PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[Bart Van Assche]

On Thu, 2018-11-08 at 13:54 +-0100, David Disseldorp wrote:
+AD4 Looks reasonable. Any reason for dropping the extra +AF8AXw-GFP flags?

Hi David,

What makes you think that I dropped any of the +AF8AXw-GFP flags? The only purpose
of the +AF8AXw-GFP+AF8-NOWARN and +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL flags is to make kcalloc() fail
quickly and silently. I'm not dropping these flags: as one can see in the
kvmalloc+AF8-node() implementation that function passes these flags to
kmalloc+AF8-node(). The source code I'm referring to is available in mm/util.c.

Bart.

Re: [PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[David Disseldorp]

Hi Bart,

On Thu, 08 Nov 2018 15:17:45 -0800, Bart Van Assche wrote:

> What makes you think that I dropped any of the __GFP flags? The only purpose
> of the __GFP_NOWARN and __GFP_RETRY_MAYFAIL flags is to make kcalloc() fail
> quickly and silently.

Are you confusing __GFP_RETRY_MAYFAIL with __GFP_NORETRY here?

> I'm not dropping these flags: as one can see in the
> kvmalloc_node() implementation that function passes these flags to
> kmalloc_node(). The source code I'm referring to is available in mm/util.c.

I see that (given tag_num * tag_size > PAGE_SIZE) __GFP_NOWARN is added
by kvmalloc_node(), but __GFP_RETRY_MAYFAIL is dropped with your change,
which sees __GFP_NORETRY added in the >PAGE_SIZE path.

Cheers, David

Re: [PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[Bart Van Assche]

On Fri, 2018-11-16 at 14:10 +-0100, David Disseldorp wrote:
+AD4 On Thu, 08 Nov 2018 15:17:45 -0800, Bart Van Assche wrote:
+AD4 +AD4 What makes you think that I dropped any of the +AF8AXw-GFP flags? The only purpose
+AD4 +AD4 of the +AF8AXw-GFP+AF8-NOWARN and +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL flags is to make kcalloc() fail
+AD4 +AD4 quickly and silently.
+AD4 
+AD4 Are you confusing +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL with +AF8AXw-GFP+AF8-NORETRY here?

No. Passing +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL to a memory allocation function makes the memory
allocator try less hard than without that flag. From should+AF8-continue+AF8-reclaim():

	/+ACo
	 +ACo For +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL allocations, stop reclaiming if the
	 +ACo full LRU list has been scanned and we are still failing
	 +ACo to reclaim pages. This full LRU scan is potentially
	 +ACo expensive but a +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL caller really wants to succeed
	 +ACo-/

+AD4 +AD4 I'm not dropping these flags: as one can see in the
+AD4 +AD4 kvmalloc+AF8-node() implementation that function passes these flags to
+AD4 +AD4 kmalloc+AF8-node(). The source code I'm referring to is available in mm/util.c.
+AD4 
+AD4 I see that (given tag+AF8-num +ACo tag+AF8-size +AD4 PAGE+AF8-SIZE) +AF8AXw-GFP+AF8-NOWARN is added
+AD4 by kvmalloc+AF8-node(), but +AF8AXw-GFP+AF8-RETRY+AF8-MAYFAIL is dropped with your change,
+AD4 which sees +AF8AXw-GFP+AF8-NORETRY added in the +AD4-PAGE+AF8-SIZE path.

That's right. If you want I can mention this in the patch description.

Bart.

Re: [PATCH 02/10] target/core: Use kvcalloc() instead of open-coding it

[David Disseldorp]

On Fri, 16 Nov 2018 07:58:41 -0800, Bart Van Assche wrote:

> On Fri, 2018-11-16 at 14:10 +0100, David Disseldorp wrote:
> > On Thu, 08 Nov 2018 15:17:45 -0800, Bart Van Assche wrote:  
> > > What makes you think that I dropped any of the __GFP flags? The only purpose
> > > of the __GFP_NOWARN and __GFP_RETRY_MAYFAIL flags is to make kcalloc() fail
> > > quickly and silently.  
> > 
> > Are you confusing __GFP_RETRY_MAYFAIL with __GFP_NORETRY here?  
> 
> No. Passing __GFP_RETRY_MAYFAIL to a memory allocation function makes the memory
> allocator try less hard than without that flag.

In the context of this patch we're going from __GFP_RETRY_MAYFAIL to
__GFP_NORETRY (due to the  kvmalloc_node() >PAGE_SIZE logic), so "fail
quickly" makes more sense to me when referring to the latter.

> > > I'm not dropping these flags: as one can see in the
> > > kvmalloc_node() implementation that function passes these flags to
> > > kmalloc_node(). The source code I'm referring to is available in mm/util.c.  
> > 
> > I see that (given tag_num * tag_size > PAGE_SIZE) __GFP_NOWARN is added
> > by kvmalloc_node(), but __GFP_RETRY_MAYFAIL is dropped with your change,
> > which sees __GFP_NORETRY added in the >PAGE_SIZE path.  
> 
> That's right. If you want I can mention this in the patch description.

I don't think that's needed, I was just interested in the motivation for
the change in __GFP flags.

Cheers, David