* [PATCH] megaraid_sas: silence a warning
@ 2020-01-31 13:23 Tomas Henzl
2020-02-01 17:26 ` Lee Duncan
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Henzl @ 2020-01-31 13:23 UTC (permalink / raw)
To: linux-scsi; +Cc: sumit.saxena, shivasharan.srikanteshwara
Add a flag to dma mem allocation to silence a warning.
Signed-off-by: Tomas Henzl <thenzl@redhat.com>
---
drivers/scsi/megaraid/megaraid_sas_fusion.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 0f5399b3e..1fa2d1449 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -606,7 +606,8 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
fusion->io_request_frames =
dma_pool_alloc(fusion->io_request_frames_pool,
- GFP_KERNEL, &fusion->io_request_frames_phys);
+ GFP_KERNEL | __GFP_NOWARN,
+ &fusion->io_request_frames_phys);
if (!fusion->io_request_frames) {
if (instance->max_fw_cmds >= (MEGASAS_REDUCE_QD_COUNT * 2)) {
instance->max_fw_cmds -= MEGASAS_REDUCE_QD_COUNT;
@@ -644,7 +645,7 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
fusion->io_request_frames =
dma_pool_alloc(fusion->io_request_frames_pool,
- GFP_KERNEL,
+ GFP_KERNEL | __GFP_NOWARN,
&fusion->io_request_frames_phys);
if (!fusion->io_request_frames) {
--
2.21.1
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH] megaraid_sas: silence a warning
2020-01-31 13:23 [PATCH] megaraid_sas: silence a warning Tomas Henzl
@ 2020-02-01 17:26 ` Lee Duncan
2020-02-03 9:16 ` Sumit Saxena
0 siblings, 1 reply; 5+ messages in thread
From: Lee Duncan @ 2020-02-01 17:26 UTC (permalink / raw)
To: Tomas Henzl, linux-scsi; +Cc: sumit.saxena, shivasharan.srikanteshwara
On 1/31/20 5:23 AM, Tomas Henzl wrote:
> Add a flag to dma mem allocation to silence a warning.
>
> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> ---
> drivers/scsi/megaraid/megaraid_sas_fusion.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 0f5399b3e..1fa2d1449 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -606,7 +606,8 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
>
> fusion->io_request_frames =
> dma_pool_alloc(fusion->io_request_frames_pool,
> - GFP_KERNEL, &fusion->io_request_frames_phys);
> + GFP_KERNEL | __GFP_NOWARN,
> + &fusion->io_request_frames_phys);
> if (!fusion->io_request_frames) {
> if (instance->max_fw_cmds >= (MEGASAS_REDUCE_QD_COUNT * 2)) {
> instance->max_fw_cmds -= MEGASAS_REDUCE_QD_COUNT;
> @@ -644,7 +645,7 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
> open-isns-updates.diff.bz2
> fusion->io_request_frames =
> dma_pool_alloc(fusion->io_request_frames_pool,
> - GFP_KERNEL,
> + GFP_KERNEL | __GFP_NOWARN,
> &fusion->io_request_frames_phys);
>
> if (!fusion->io_request_frames) {
>
I'm fairly sure this is a good fix, but I'd appreciate more information
in the comment, such as what warning was silenced, and why it's okay to
silence it rather than "fix" it. I know from experience that, when
choosing which commits to backport, more information is better than less.
--
Lee Duncan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] megaraid_sas: silence a warning
2020-02-01 17:26 ` Lee Duncan
@ 2020-02-03 9:16 ` Sumit Saxena
2020-02-03 12:20 ` Tomas Henzl
0 siblings, 1 reply; 5+ messages in thread
From: Sumit Saxena @ 2020-02-03 9:16 UTC (permalink / raw)
To: Lee Duncan; +Cc: Tomas Henzl, Linux SCSI List, Shivasharan Srikanteshwara
On Sat, Feb 1, 2020 at 10:57 PM Lee Duncan <lduncan@suse.com> wrote:
>
> On 1/31/20 5:23 AM, Tomas Henzl wrote:
> > Add a flag to dma mem allocation to silence a warning.
> >
> > Signed-off-by: Tomas Henzl <thenzl@redhat.com>
> > ---
> > drivers/scsi/megaraid/megaraid_sas_fusion.c | 5 +++--
> > 1 file changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > index 0f5399b3e..1fa2d1449 100644
> > --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> > @@ -606,7 +606,8 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
> >
> > fusion->io_request_frames =
> > dma_pool_alloc(fusion->io_request_frames_pool,
> > - GFP_KERNEL, &fusion->io_request_frames_phys);
> > + GFP_KERNEL | __GFP_NOWARN,
> > + &fusion->io_request_frames_phys);
> > if (!fusion->io_request_frames) {
> > if (instance->max_fw_cmds >= (MEGASAS_REDUCE_QD_COUNT * 2)) {
> > instance->max_fw_cmds -= MEGASAS_REDUCE_QD_COUNT;
> > @@ -644,7 +645,7 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
> > open-isns-updates.diff.bz2
> > fusion->io_request_frames =
> > dma_pool_alloc(fusion->io_request_frames_pool,
> > - GFP_KERNEL,
> > + GFP_KERNEL | __GFP_NOWARN,
> > &fusion->io_request_frames_phys);
> >
> > if (!fusion->io_request_frames) {
> >
>
> I'm fairly sure this is a good fix, but I'd appreciate more information
> in the comment, such as what warning was silenced, and why it's okay to
> silence it rather than "fix" it. I know from experience that, when
> choosing which commits to backport, more information is better than less.
This code allocates DMA memory for driver's IO frames which may exceed
MAX_ORDER pages for few
megaraid_sas controllers(controllers with High Queue Depth). So there
is logic to keep on reducing controller
Queue Depth until DMA memory required for IO frames fits within
MAX_ORDER. So or impacted megaraid_sas controllers,
there would be multiple DMA allocation failure until driver settles
down to Controller Queue Depth which has memory requirement
within MAX_ORDER. These failed DMA allocation requests causes stack
traces in system logs which is not harmful and this patch
would silence those warnings/stack traces.
With CMA (Contiguous Memory Allocator) enabled, it's possible to
allocate DMA memory exceeding MAX_ORDER.
And that is the reason of keeping this retry logic with less
controller Queue Depth instead of calculating controller Queue depth
at first hand which has memory requirement less than MAX_ORDER.
Thanks,
Sumit
>
> --
> Lee Duncan
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] megaraid_sas: silence a warning
2020-02-03 9:16 ` Sumit Saxena
@ 2020-02-03 12:20 ` Tomas Henzl
2020-02-03 16:29 ` Lee Duncan
0 siblings, 1 reply; 5+ messages in thread
From: Tomas Henzl @ 2020-02-03 12:20 UTC (permalink / raw)
To: Sumit Saxena, Lee Duncan; +Cc: Linux SCSI List, Shivasharan Srikanteshwara
On 2/3/20 10:16 AM, Sumit Saxena wrote:
> On Sat, Feb 1, 2020 at 10:57 PM Lee Duncan <lduncan@suse.com> wrote:
>>
>> On 1/31/20 5:23 AM, Tomas Henzl wrote:
>>> Add a flag to dma mem allocation to silence a warning.
>>>
>>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>>> ---
>>> drivers/scsi/megaraid/megaraid_sas_fusion.c | 5 +++--
>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> index 0f5399b3e..1fa2d1449 100644
>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>> @@ -606,7 +606,8 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
>>>
>>> fusion->io_request_frames =
>>> dma_pool_alloc(fusion->io_request_frames_pool,
>>> - GFP_KERNEL, &fusion->io_request_frames_phys);
>>> + GFP_KERNEL | __GFP_NOWARN,
>>> + &fusion->io_request_frames_phys);
>>> if (!fusion->io_request_frames) {
>>> if (instance->max_fw_cmds >= (MEGASAS_REDUCE_QD_COUNT * 2)) {
>>> instance->max_fw_cmds -= MEGASAS_REDUCE_QD_COUNT;
>>> @@ -644,7 +645,7 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
>>> open-isns-updates.diff.bz2
>>> fusion->io_request_frames =
>>> dma_pool_alloc(fusion->io_request_frames_pool,
>>> - GFP_KERNEL,
>>> + GFP_KERNEL | __GFP_NOWARN,
>>> &fusion->io_request_frames_phys);
>>>
>>> if (!fusion->io_request_frames) {
>>>
>>
>> I'm fairly sure this is a good fix, but I'd appreciate more information
>> in the comment, such as what warning was silenced, and why it's okay to
>> silence it rather than "fix" it. I know from experience that, when
>> choosing which commits to backport, more information is better than less.
> This code allocates DMA memory for driver's IO frames which may exceed
> MAX_ORDER pages for few
> megaraid_sas controllers(controllers with High Queue Depth). So there
> is logic to keep on reducing controller
> Queue Depth until DMA memory required for IO frames fits within
> MAX_ORDER. So or impacted megaraid_sas controllers,
> there would be multiple DMA allocation failure until driver settles
> down to Controller Queue Depth which has memory requirement
> within MAX_ORDER. These failed DMA allocation requests causes stack
> traces in system logs which is not harmful and this patch
> would silence those warnings/stack traces.
>
> With CMA (Contiguous Memory Allocator) enabled, it's possible to
> allocate DMA memory exceeding MAX_ORDER.
> And that is the reason of keeping this retry logic with less
> controller Queue Depth instead of calculating controller Queue depth
> at first hand which has memory requirement less than MAX_ORDER.
Thank you Sumit for writing it down.
An over-sized allocation failure is sanitized in a proper way. The
warning may hide other allocation warnings in other parts of kernel as
it is printed only once.
I could have written more vecasue I've underestimated it and I'm sorry
for that.
Tomas
> Thanks,
> Sumit
>>
>> --
>> Lee Duncan
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] megaraid_sas: silence a warning
2020-02-03 12:20 ` Tomas Henzl
@ 2020-02-03 16:29 ` Lee Duncan
0 siblings, 0 replies; 5+ messages in thread
From: Lee Duncan @ 2020-02-03 16:29 UTC (permalink / raw)
To: Tomas Henzl, Sumit Saxena; +Cc: Linux SCSI List, Shivasharan Srikanteshwara
On 2/3/20 4:20 AM, Tomas Henzl wrote:
> On 2/3/20 10:16 AM, Sumit Saxena wrote:
>> On Sat, Feb 1, 2020 at 10:57 PM Lee Duncan <lduncan@suse.com> wrote:
>>>
>>> On 1/31/20 5:23 AM, Tomas Henzl wrote:
>>>> Add a flag to dma mem allocation to silence a warning.
>>>>
>>>> Signed-off-by: Tomas Henzl <thenzl@redhat.com>
>>>> ---
>>>> drivers/scsi/megaraid/megaraid_sas_fusion.c | 5 +++--
>>>> 1 file changed, 3 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>> index 0f5399b3e..1fa2d1449 100644
>>>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>>>> @@ -606,7 +606,8 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
>>>>
>>>> fusion->io_request_frames =
>>>> dma_pool_alloc(fusion->io_request_frames_pool,
>>>> - GFP_KERNEL, &fusion->io_request_frames_phys);
>>>> + GFP_KERNEL | __GFP_NOWARN,
>>>> + &fusion->io_request_frames_phys);
>>>> if (!fusion->io_request_frames) {
>>>> if (instance->max_fw_cmds >= (MEGASAS_REDUCE_QD_COUNT * 2)) {
>>>> instance->max_fw_cmds -= MEGASAS_REDUCE_QD_COUNT;
>>>> @@ -644,7 +645,7 @@ megasas_alloc_request_fusion(struct megasas_instance *instance)
>>>> open-isns-updates.diff.bz2
>>>> fusion->io_request_frames =
>>>> dma_pool_alloc(fusion->io_request_frames_pool,
>>>> - GFP_KERNEL,
>>>> + GFP_KERNEL | __GFP_NOWARN,
>>>> &fusion->io_request_frames_phys);
>>>>
>>>> if (!fusion->io_request_frames) {
>>>>
>>>
>>> I'm fairly sure this is a good fix, but I'd appreciate more information
>>> in the comment, such as what warning was silenced, and why it's okay to
>>> silence it rather than "fix" it. I know from experience that, when
>>> choosing which commits to backport, more information is better than less.
>> This code allocates DMA memory for driver's IO frames which may exceed
>> MAX_ORDER pages for few
>> megaraid_sas controllers(controllers with High Queue Depth). So there
>> is logic to keep on reducing controller
>> Queue Depth until DMA memory required for IO frames fits within
>> MAX_ORDER. So or impacted megaraid_sas controllers,
>> there would be multiple DMA allocation failure until driver settles
>> down to Controller Queue Depth which has memory requirement
>> within MAX_ORDER. These failed DMA allocation requests causes stack
>> traces in system logs which is not harmful and this patch
>> would silence those warnings/stack traces.
>>
>> With CMA (Contiguous Memory Allocator) enabled, it's possible to
>> allocate DMA memory exceeding MAX_ORDER.
>> And that is the reason of keeping this retry logic with less
>> controller Queue Depth instead of calculating controller Queue depth
>> at first hand which has memory requirement less than MAX_ORDER.
>
> Thank you Sumit for writing it down.
> An over-sized allocation failure is sanitized in a proper way. The
> warning may hide other allocation warnings in other parts of kernel as
> it is printed only once.
>
> I could have written more vecasue I've underestimated it and I'm sorry
> for that.
>
> Tomas
No problem! Thank you for adding this. Can you resubmit with this info?
If you're not familiar with submitting a 2nd version (V2), please look
at the mailing list archives (or email me directly for advice).
I'd be glad to add my reviewed tag once the patch is updated.
>
>
>> Thanks,
>> Sumit
>>>
>>> --
>>> Lee Duncan
>>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-02-03 16:30 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-31 13:23 [PATCH] megaraid_sas: silence a warning Tomas Henzl
2020-02-01 17:26 ` Lee Duncan
2020-02-03 9:16 ` Sumit Saxena
2020-02-03 12:20 ` Tomas Henzl
2020-02-03 16:29 ` Lee Duncan
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.