Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]


Groups > linux.kernel > #1646229 > unrolled thread

[PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock

Started byWei Yongjun <weiyj.lk@gmail.com>
First post2017-05-21 02:50 +0200
Last post2017-05-22 20:50 +0200
Articles 4 — 3 participants

Back to article view | Back to linux.kernel


Contents

  [PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock Wei Yongjun <weiyj.lk@gmail.com> - 2017-05-21 02:50 +0200
    Re: [PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock Michal Hocko <mhocko@kernel.org> - 2017-05-21 09:50 +0200
      Re: [PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock Michal Hocko <mhocko@kernel.org> - 2017-05-22 14:00 +0200
        Re: [PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock Alan Cox <gnomes@lxorguk.ukuu.org.uk> - 2017-05-22 20:50 +0200

#1646229 — [PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock

FromWei Yongjun <weiyj.lk@gmail.com>
Date2017-05-21 02:50 +0200
Subject[PATCH] goldfish_pipe: use GFP_ATOMIC under spin lock
Message-ID<tJneh-kw-3@gated-at.bofh.it>
From: Wei Yongjun <weiyongjun1@huawei.com>

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 <weiyongjun1@huawei.com>
---
 drivers/platform/goldfish/goldfish_pipe.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
index 2de1e60..5f36721 100644
--- a/drivers/platform/goldfish/goldfish_pipe.c
+++ b/drivers/platform/goldfish/goldfish_pipe.c
@@ -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);

[toc] | [next] | [standalone]


#1646280

FromMichal Hocko <mhocko@kernel.org>
Date2017-05-21 09:50 +0200
Message-ID<tJtMJ-4Tk-9@gated-at.bofh.it>
In reply to#1646229
On Sun 21-05-17 00:45:46, Wei Yongjun wrote:
> From: Wei Yongjun <weiyongjun1@huawei.com>
> 
> 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?

> Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> ---
>  drivers/platform/goldfish/goldfish_pipe.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
> index 2de1e60..5f36721 100644
> --- a/drivers/platform/goldfish/goldfish_pipe.c
> +++ b/drivers/platform/goldfish/goldfish_pipe.c
> @@ -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);
> 

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]


#1646782

FromMichal Hocko <mhocko@kernel.org>
Date2017-05-22 14:00 +0200
Message-ID<tJUad-5dJ-13@gated-at.bofh.it>
In reply to#1646280
On Sun 21-05-17 09:48:36, Michal Hocko wrote:
> On Sun 21-05-17 00:45:46, Wei Yongjun wrote:
> > From: Wei Yongjun <weiyongjun1@huawei.com>
> > 
> > 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?

> 
> > Signed-off-by: Wei Yongjun <weiyongjun1@huawei.com>
> > ---
> >  drivers/platform/goldfish/goldfish_pipe.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/platform/goldfish/goldfish_pipe.c b/drivers/platform/goldfish/goldfish_pipe.c
> > index 2de1e60..5f36721 100644
> > --- a/drivers/platform/goldfish/goldfish_pipe.c
> > +++ b/drivers/platform/goldfish/goldfish_pipe.c
> > @@ -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);
> > 
> 
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]


#1647264

FromAlan Cox <gnomes@lxorguk.ukuu.org.uk>
Date2017-05-22 20:50 +0200
Message-ID<tK0z0-NW-29@gated-at.bofh.it>
In reply to#1646782
On Mon, 22 May 2017 13:51:52 +0200
Michal Hocko <mhocko@kernel.org> wrote:

> On Sun 21-05-17 09:48:36, Michal Hocko wrote:
> > On Sun 21-05-17 00:45:46, Wei Yongjun wrote:  
> > > From: Wei Yongjun <weiyongjun1@huawei.com>
> > > 
> > > 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?

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

[toc] | [prev] | [standalone]


Back to top | Article view | linux.kernel


csiph-web