* [bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL
@ 2021-08-13 12:38 Dan Carpenter
2021-08-14 19:50 ` Ofir Bitton
0 siblings, 1 reply; 2+ messages in thread
From: Dan Carpenter @ 2021-08-13 12:38 UTC (permalink / raw)
To: obitton; +Cc: Oded Gabbay, kernel-janitors
Hello Ofir Bitton,
The patch d5eb8373b2ce: "habanalabs: replace GFP_ATOMIC with
GFP_KERNEL" from Feb 14, 2021, leads to the following
Smatch static checker warning:
drivers/misc/habanalabs/common/command_buffer.c:318 hl_cb_create()
warn: sleeping in atomic context
drivers/misc/habanalabs/common/command_buffer.c
311 if (rc)
312 goto release_cb;
313 }
314
315 spin_lock(&mgr->cb_lock);
^^^^^^^^^^^^^
316 rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_ATOMIC);
317 if (rc < 0)
--> 318 rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_KERNEL);
^^^^^^^^^^
This falls back to GFP_KERNEL if GFP_ATOMIC fails. I don't understand
that. If it's at all a performance improvement to call idr_alloc() with
ATOMIC first, then the correct thing to do is to fix idr_alloc() instead
of working around it in the caller.
319 spin_unlock(&mgr->cb_lock);
320
The other warning is also idr_alloc() but holding a different spin lock.
drivers/misc/habanalabs/common/memory.c:126 alloc_device_memory()
warn: sleeping in atomic context
124
125 spin_lock(&vm->idr_lock);
^^^^^^^^^^^^^^^^^^^^^^^^
126 handle = idr_alloc(&vm->phys_pg_pack_handles, phys_pg_pack, 1, 0,
127 GFP_KERNEL);
^^^^^^^^^^
128 spin_unlock(&vm->idr_lock);
129
130 if (handle < 0) {
131 dev_err(hdev->dev, "Failed to get handle for page\n");
132 rc = -EFAULT;
133 goto idr_err;
134 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 2+ messages in thread
* RE: [bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL
2021-08-13 12:38 [bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL Dan Carpenter
@ 2021-08-14 19:50 ` Ofir Bitton
0 siblings, 0 replies; 2+ messages in thread
From: Ofir Bitton @ 2021-08-14 19:50 UTC (permalink / raw)
To: Dan Carpenter; +Cc: Oded Gabbay, kernel-janitors
On Fri, Aug 13, 2021 at 3:38 PM Dan Carpenter <dan.carpenter@oracle.com> wrote:
>
> [???? ???? ???? ???????? ????? ?????? ?- dan.carpenter@oracle.com. ??? ???? ???? ???? ?- http://aka.ms/LearnAboutSenderIdentification.]
>
> Hello Ofir Bitton,
>
> The patch d5eb8373b2ce: "habanalabs: replace GFP_ATOMIC with GFP_KERNEL" from Feb 14, 2021, leads to the following Smatch static checker warning:
>
> drivers/misc/habanalabs/common/command_buffer.c:318 hl_cb_create()
> warn: sleeping in atomic context
>
> drivers/misc/habanalabs/common/command_buffer.c
> 311 if (rc)
> 312 goto release_cb;
> 313 }
> 314
> 315 spin_lock(&mgr->cb_lock);
> ^^^^^^^^^^^^^
> 316 rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_ATOMIC);
> 317 if (rc < 0)
> --> 318 rc = idr_alloc(&mgr->cb_handles, cb, 1, 0, GFP_KERNEL);
> ^^^^^^^^^^ This falls back to GFP_KERNEL if GFP_ATOMIC fails. I don't understand that. If it's at all a performance improvement to call idr_alloc() with ATOMIC first, then the correct thing to do is to fix idr_alloc() instead of working around it in the caller.
>
> 319 spin_unlock(&mgr->cb_lock);
> 320
>
> The other warning is also idr_alloc() but holding a different spin lock.
>
> drivers/misc/habanalabs/common/memory.c:126 alloc_device_memory()
> warn: sleeping in atomic context
>
> 124
> 125 spin_lock(&vm->idr_lock);
> ^^^^^^^^^^^^^^^^^^^^^^^^
> 126 handle = idr_alloc(&vm->phys_pg_pack_handles, phys_pg_pack, 1, 0,
> 127 GFP_KERNEL);
> ^^^^^^^^^^
> 128 spin_unlock(&vm->idr_lock);
> 129
> 130 if (handle < 0) {
> 131 dev_err(hdev->dev, "Failed to get handle for page\n");
> 132 rc = -EFAULT;
> 133 goto idr_err;
> 134 }
>
> regards,
> dan carpenter
Thanks for noticing I will handle it.
Ofir
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2021-08-14 19:50 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-13 12:38 [bug report] habanalabs: replace GFP_ATOMIC with GFP_KERNEL Dan Carpenter
2021-08-14 19:50 ` Ofir Bitton
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.