| Message ID | 20181106171718.89594-3-bvanassche@acm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make ABORT and LUN RESET handling synchronous | expand |
Looks reasonable. Any reason for dropping the extra __GFP flags?
Either way...
Reviewed-by: David Disseldorp <ddiss@suse.de>
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.
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
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.
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
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,
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(-)