* [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store()
@ 2019-08-31 12:49 YueHaibing
2019-09-02 7:50 ` Christoph Hellwig
2019-09-03 8:22 ` [PATCH v2 -next] PCI: Don't use GFP_KERNEL for kstrbdup in resource_alignment_store YueHaibing
0 siblings, 2 replies; 5+ messages in thread
From: YueHaibing @ 2019-08-31 12:49 UTC (permalink / raw)
To: Bjorn Helgaas, Logan Gunthorpe
Cc: YueHaibing, linux-pci, linux-kernel, kernel-janitors
When allocating memory, the GFP_KERNEL cannot be used during the
spin_lock period. It may cause scheduling when holding spin_lock.
Fixes: f13755318675 ("PCI: Move pci_[get|set]_resource_alignment_param() into their callers")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/pci/pci.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 484e35349565..0b5fc6736f3f 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6148,7 +6148,7 @@ static ssize_t resource_alignment_store(struct bus_type *bus,
spin_lock(&resource_alignment_lock);
kfree(resource_alignment_param);
- resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
+ resource_alignment_param = kstrndup(buf, count, GFP_ATOMIC);
spin_unlock(&resource_alignment_lock);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store()
2019-08-31 12:49 [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store() YueHaibing
@ 2019-09-02 7:50 ` Christoph Hellwig
2019-09-03 15:51 ` Logan Gunthorpe
2019-09-03 8:22 ` [PATCH v2 -next] PCI: Don't use GFP_KERNEL for kstrbdup in resource_alignment_store YueHaibing
1 sibling, 1 reply; 5+ messages in thread
From: Christoph Hellwig @ 2019-09-02 7:50 UTC (permalink / raw)
To: YueHaibing
Cc: Bjorn Helgaas, Logan Gunthorpe, linux-pci, linux-kernel, kernel-janitors
On Sat, Aug 31, 2019 at 12:49:32PM +0000, YueHaibing wrote:
> When allocating memory, the GFP_KERNEL cannot be used during the
> spin_lock period. It may cause scheduling when holding spin_lock.
>
> Fixes: f13755318675 ("PCI: Move pci_[get|set]_resource_alignment_param() into their callers")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> ---
> drivers/pci/pci.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 484e35349565..0b5fc6736f3f 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6148,7 +6148,7 @@ static ssize_t resource_alignment_store(struct bus_type *bus,
> spin_lock(&resource_alignment_lock);
>
> kfree(resource_alignment_param);
> - resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
> + resource_alignment_param = kstrndup(buf, count, GFP_ATOMIC);
>
> spin_unlock(&resource_alignment_lock);
Why not move the allocation outside the lock? Something like this
seems much more sensible:
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 484e35349565..fe205829f676 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6145,14 +6145,16 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
static ssize_t resource_alignment_store(struct bus_type *bus,
const char *buf, size_t count)
{
- spin_lock(&resource_alignment_lock);
+ char *param = kstrndup(buf, count, GFP_KERNEL);
- kfree(resource_alignment_param);
- resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
+ if (!param)
+ return -ENOMEM;
+ spin_lock(&resource_alignment_lock);
+ kfree(resource_alignment_param);
+ resource_alignment_param = param;
spin_unlock(&resource_alignment_lock);
-
- return resource_alignment_param ? count : -ENOMEM;
+ return count;
}
static BUS_ATTR_RW(resource_alignment);
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 -next] PCI: Don't use GFP_KERNEL for kstrbdup in resource_alignment_store
2019-08-31 12:49 [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store() YueHaibing
2019-09-02 7:50 ` Christoph Hellwig
@ 2019-09-03 8:22 ` YueHaibing
1 sibling, 0 replies; 5+ messages in thread
From: YueHaibing @ 2019-09-03 8:22 UTC (permalink / raw)
To: bhelgaas, hch; +Cc: linux-pci, linux-kernel, YueHaibing
When allocating memory, the GFP_KERNEL cannot be used during the
spin_lock period. It may cause scheduling when holding spin_lock.
Suggested-by: Christoph Hellwig <hch@infradead.org>
Fixes: f13755318675 ("PCI: Move pci_[get|set]_resource_alignment_param() into their callers")
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
v2: move alloc out of spinlock
---
drivers/pci/pci.c | 11 +++++++----
1 file changed, 7 insertions(+), 4 deletions(-)
diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index 484e353..a3d5920 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -6145,14 +6145,17 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
static ssize_t resource_alignment_store(struct bus_type *bus,
const char *buf, size_t count)
{
- spin_lock(&resource_alignment_lock);
+ char *param = kstrndup(buf, count, GFP_KERNEL);
- kfree(resource_alignment_param);
- resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
+ if (!param)
+ return -ENOMEM;
+ spin_lock(&resource_alignment_lock);
+ kfree(resource_alignment_param);
+ resource_alignment_param = param;
spin_unlock(&resource_alignment_lock);
- return resource_alignment_param ? count : -ENOMEM;
+ return count;
}
static BUS_ATTR_RW(resource_alignment);
--
2.7.4
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store()
2019-09-02 7:50 ` Christoph Hellwig
@ 2019-09-03 15:51 ` Logan Gunthorpe
2019-09-05 21:47 ` Bjorn Helgaas
0 siblings, 1 reply; 5+ messages in thread
From: Logan Gunthorpe @ 2019-09-03 15:51 UTC (permalink / raw)
To: Christoph Hellwig, YueHaibing
Cc: Bjorn Helgaas, linux-pci, linux-kernel, kernel-janitors
On 2019-09-02 1:50 a.m., Christoph Hellwig wrote:
> On Sat, Aug 31, 2019 at 12:49:32PM +0000, YueHaibing wrote:
>> When allocating memory, the GFP_KERNEL cannot be used during the
>> spin_lock period. It may cause scheduling when holding spin_lock.
>>
>> Fixes: f13755318675 ("PCI: Move pci_[get|set]_resource_alignment_param() into their callers")
>> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
>> ---
>> drivers/pci/pci.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
>> index 484e35349565..0b5fc6736f3f 100644
>> --- a/drivers/pci/pci.c
>> +++ b/drivers/pci/pci.c
>> @@ -6148,7 +6148,7 @@ static ssize_t resource_alignment_store(struct bus_type *bus,
>> spin_lock(&resource_alignment_lock);
>>
>> kfree(resource_alignment_param);
>> - resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
>> + resource_alignment_param = kstrndup(buf, count, GFP_ATOMIC);
>>
>> spin_unlock(&resource_alignment_lock);
>
> Why not move the allocation outside the lock? Something like this
> seems much more sensible:
Yes, that seems like a good way to do it. Bjorn, can you squash
Christoph's patch or do you want me to resend a new one?
Thanks,
Logan
>
> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> index 484e35349565..fe205829f676 100644
> --- a/drivers/pci/pci.c
> +++ b/drivers/pci/pci.c
> @@ -6145,14 +6145,16 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
> static ssize_t resource_alignment_store(struct bus_type *bus,
> const char *buf, size_t count)
> {
> - spin_lock(&resource_alignment_lock);
> + char *param = kstrndup(buf, count, GFP_KERNEL);
>
> - kfree(resource_alignment_param);
> - resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
> + if (!param)
> + return -ENOMEM;
>
> + spin_lock(&resource_alignment_lock);
> + kfree(resource_alignment_param);
> + resource_alignment_param = param;
> spin_unlock(&resource_alignment_lock);
> -
> - return resource_alignment_param ? count : -ENOMEM;
> + return count;
> }
>
> static BUS_ATTR_RW(resource_alignment);
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store()
2019-09-03 15:51 ` Logan Gunthorpe
@ 2019-09-05 21:47 ` Bjorn Helgaas
0 siblings, 0 replies; 5+ messages in thread
From: Bjorn Helgaas @ 2019-09-05 21:47 UTC (permalink / raw)
To: Logan Gunthorpe
Cc: Christoph Hellwig, YueHaibing, linux-pci, linux-kernel, kernel-janitors
On Tue, Sep 03, 2019 at 09:51:05AM -0600, Logan Gunthorpe wrote:
>
>
> On 2019-09-02 1:50 a.m., Christoph Hellwig wrote:
> > On Sat, Aug 31, 2019 at 12:49:32PM +0000, YueHaibing wrote:
> >> When allocating memory, the GFP_KERNEL cannot be used during the
> >> spin_lock period. It may cause scheduling when holding spin_lock.
> >>
> >> Fixes: f13755318675 ("PCI: Move pci_[get|set]_resource_alignment_param() into their callers")
> >> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> >> ---
> >> drivers/pci/pci.c | 2 +-
> >> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> >> index 484e35349565..0b5fc6736f3f 100644
> >> --- a/drivers/pci/pci.c
> >> +++ b/drivers/pci/pci.c
> >> @@ -6148,7 +6148,7 @@ static ssize_t resource_alignment_store(struct bus_type *bus,
> >> spin_lock(&resource_alignment_lock);
> >>
> >> kfree(resource_alignment_param);
> >> - resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
> >> + resource_alignment_param = kstrndup(buf, count, GFP_ATOMIC);
> >>
> >> spin_unlock(&resource_alignment_lock);
> >
> > Why not move the allocation outside the lock? Something like this
> > seems much more sensible:
>
> Yes, that seems like a good way to do it. Bjorn, can you squash
> Christoph's patch or do you want me to resend a new one?
I folded Christoph's fix into it, thanks!
> > diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
> > index 484e35349565..fe205829f676 100644
> > --- a/drivers/pci/pci.c
> > +++ b/drivers/pci/pci.c
> > @@ -6145,14 +6145,16 @@ static ssize_t resource_alignment_show(struct bus_type *bus, char *buf)
> > static ssize_t resource_alignment_store(struct bus_type *bus,
> > const char *buf, size_t count)
> > {
> > - spin_lock(&resource_alignment_lock);
> > + char *param = kstrndup(buf, count, GFP_KERNEL);
> >
> > - kfree(resource_alignment_param);
> > - resource_alignment_param = kstrndup(buf, count, GFP_KERNEL);
> > + if (!param)
> > + return -ENOMEM;
> >
> > + spin_lock(&resource_alignment_lock);
> > + kfree(resource_alignment_param);
> > + resource_alignment_param = param;
> > spin_unlock(&resource_alignment_lock);
> > -
> > - return resource_alignment_param ? count : -ENOMEM;
> > + return count;
> > }
> >
> > static BUS_ATTR_RW(resource_alignment);
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-09-05 21:47 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-31 12:49 [PATCH -next] PCI: Use GFP_ATOMIC in resource_alignment_store() YueHaibing
2019-09-02 7:50 ` Christoph Hellwig
2019-09-03 15:51 ` Logan Gunthorpe
2019-09-05 21:47 ` Bjorn Helgaas
2019-09-03 8:22 ` [PATCH v2 -next] PCI: Don't use GFP_KERNEL for kstrbdup in resource_alignment_store YueHaibing
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).