Skip to contentSkip to conversation

goldfish_pipe: use GFP_ATOMIC under spin lock

PATCH In review Wei Yongjun 5 participants 7 messages May 21, 2017 – Jun 13, 2017

lore
Share & tools
lore

Wei Yongjun

author
#0 lore raw

PATCH 1/1 goldfish_pipe: use GFP_ATOMIC under spin lock

From: Wei Yongjun <[email protected]>

The function get_free_pipe_id_locked() is called from goldfish_pipe_open() with a lock is held, so we should use GFP_ATOMIC instead of GFP_KERNEL.

Signed-off-by Wei Yongjun <[email protected]>
---
 drivers/platform/goldfish/goldfish_pipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/goldfish/goldfish_pipe.c +1 -1
index 2de1e60..5f36721 100644
@@ -704,7 +704,7 @@ static int get_free_pipe_id_locked(struct goldfish_pipe_dev *dev)
 		/* Reallocate the array */
 		u32 new_capacity = 2 * dev->pipes_capacity;
 		struct goldfish_pipe **pipes =
-			kcalloc(new_capacity, sizeof(*pipes), GFP_KERNEL);
+			kcalloc(new_capacity, sizeof(*pipes), GFP_ATOMIC);
 		if (!pipes)
 			return -ENOMEM;
 		memcpy(pipes, dev->pipes, sizeof(*pipes) * dev->pipes_capacity);
#1 lore raw
Quoted 5 lines · Wei Yongjun

From: Wei Yongjun <[email protected]>

The function get_free_pipe_id_locked() is called from goldfish_pipe_open() with a lock is held, so we should use GFP_ATOMIC instead of GFP_KERNEL.

Why is GFP_NOWAIT insufficient? Does this path needs an access to memory reserves?

Quoted 19 lines
Signed-off-by Wei Yongjun <[email protected]>
---
 drivers/platform/goldfish/goldfish_pipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/goldfish/goldfish_pipe.c +1 -1
index 2de1e60..5f36721 100644
@@ -704,7 +704,7 @@ static int get_free_pipe_id_locked(struct goldfish_pipe_dev *dev)
 		/* Reallocate the array */
 		u32 new_capacity = 2 * dev->pipes_capacity;
 		struct goldfish_pipe **pipes =
-			kcalloc(new_capacity, sizeof(*pipes), GFP_KERNEL);
+			kcalloc(new_capacity, sizeof(*pipes), GFP_ATOMIC);
 		if (!pipes)
 			return -ENOMEM;
 		memcpy(pipes, dev->pipes, sizeof(*pipes) * dev->pipes_capacity);
Signature · 2 lines
Michal Hocko
SUSE Labs
#2 lore raw
Quoted 9 lines · Michal Hocko
Quoted 5 lines · Wei Yongjun

From: Wei Yongjun <[email protected]>

The function get_free_pipe_id_locked() is called from goldfish_pipe_open() with a lock is held, so we should use GFP_ATOMIC instead of GFP_KERNEL.

Why is GFP_NOWAIT insufficient? Does this path needs an access to memory reserves?

And now when looking at the code more deeply, wouldn't it be much better to simply do the allocation outside of the spin lock and do assignments with the lock held?

Quoted 24 lines
Quoted 20 lines
Signed-off-by Wei Yongjun <[email protected]>
---
 drivers/platform/goldfish/goldfish_pipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/platform/goldfish/goldfish_pipe.c +1 -1
index 2de1e60..5f36721 100644
@@ -704,7 +704,7 @@ static int get_free_pipe_id_locked(struct goldfish_pipe_dev *dev)
 		/* Reallocate the array */
 		u32 new_capacity = 2 * dev->pipes_capacity;
 		struct goldfish_pipe **pipes =
-			kcalloc(new_capacity, sizeof(*pipes), GFP_KERNEL);
+			kcalloc(new_capacity, sizeof(*pipes), GFP_ATOMIC);
 		if (!pipes)
 			return -ENOMEM;
 		memcpy(pipes, dev->pipes, sizeof(*pipes) * dev->pipes_capacity);
Signature · 2 lines
Michal Hocko
SUSE Labs
Signature · 2 lines
Michal Hocko
SUSE Labs
#3 lore raw
Quoted 8 lines · On Mon, 22 May 2017 13:51:52 +0200 Michal Hocko <[email protected]> wrote:
Quoted 7 lines · Michal Hocko
Quoted 5 lines · Wei Yongjun

From: Wei Yongjun <[email protected]>

The function get_free_pipe_id_locked() is called from goldfish_pipe_open() with a lock is held, so we should use GFP_ATOMIC instead of GFP_KERNEL.

Quoted 6 lines
Quoted 2 lines

Why is GFP_NOWAIT insufficient? Does this path needs an access to memory reserves?

And now when looking at the code more deeply, wouldn't it be much better to simply do the allocation outside of the spin lock and do assignments with the lock held?

That's far from trivial and certainly for backporting and an immediate fix this seems better. The allocations are not that large and any fail would be in open() not anywhere weird.

Alan

Reviewed-by
#4 lore raw
Reviewed-by Jin Qian <[email protected]>

Can we merge this to stable?

Thanks!
jin

Quoted 23 lines · Alan Cox
Quoted 8 lines · On Mon, 22 May 2017 13:51:52 +0200 Michal Hocko <[email protected]> wrote:
Quoted 7 lines · Michal Hocko
Quoted 5 lines · Wei Yongjun

From: Wei Yongjun <[email protected]>

The function get_free_pipe_id_locked() is called from goldfish_pipe_open() with a lock is held, so we should use GFP_ATOMIC instead of GFP_KERNEL.

Quoted 6 lines
Quoted 2 lines

Why is GFP_NOWAIT insufficient? Does this path needs an access to memory reserves?

And now when looking at the code more deeply, wouldn't it be much better to simply do the allocation outside of the spin lock and do assignments with the lock held?

That's far from trivial and certainly for backporting and an immediate fix this seems better. The allocations are not that large and any fail would be in open() not anywhere weird.

Alan

#6 lore raw

I meant that the patch needs Cc: [email protected].

Thanks,
jin

Quoted 11 lines · Greg Kroah-Hartman
Quoted 3 lines · Jin Qian
Reviewed-by Jin Qian <[email protected]>

Can we merge this to stable?

I don't understand the question, what kernel(s) exactly do you want this commit to be added to?

thanks,

greg k-h