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

Message ID 20181106171718.89594-3-bvanassche@acm.org
State New, archived
Headers show
Series
  • Make ABORT and LUN RESET handling synchronous
Related show

Commit Message

Bart Van Assche Nov. 6, 2018, 5:17 p.m. UTC
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(-)

Comments

David Disseldorp Nov. 8, 2018, 12:54 p.m. UTC | #1
Looks reasonable. Any reason for dropping the extra __GFP flags?

Either way...
Reviewed-by: David Disseldorp <ddiss@suse.de>
Bart Van Assche Nov. 8, 2018, 11:17 p.m. UTC | #2
On Thu, 2018-11-08 at 13:54 +0100, David Disseldorp wrote:
> Looks reasonable. Any reason for dropping the extra __GFP flags?

Hi David,

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. 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.

Bart.
David Disseldorp Nov. 16, 2018, 1:10 p.m. UTC | #3
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
Bart Van Assche Nov. 16, 2018, 3:58 p.m. UTC | #4
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. From should_continue_reclaim():

	/*
	 * For __GFP_RETRY_MAYFAIL allocations, stop reclaiming if the
	 * full LRU list has been scanned and we are still failing
	 * to reclaim pages. This full LRU scan is potentially
	 * expensive but a __GFP_RETRY_MAYFAIL caller really wants to succeed
	 */

> > 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.

Bart.
David Disseldorp Nov. 16, 2018, 5:02 p.m. UTC | #5
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

Patch
diff mbox series

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,