smp_processor_id warning in megasas driver on 3.19.3

smp_processor_id warning in megasas driver on 3.19.3

[Andy Lutomirski]

This is a regression somewhere between 3.15 and 3.19.3.  Let me know
if more diagnostics would be helpful.

[58635.050733] BUG: using smp_processor_id() in preemptible [00000000]
code: apt-check/13293
[58635.059861] caller is debug_smp_processor_id+0x17/0x20
[58635.059863] CPU: 0 PID: 13293 Comm: apt-check Not tainted 3.19.3-ama+ #44
[58635.059864] Hardware name: Dell Inc. PowerEdge R620/0KCKR5, BIOS
1.2.6 05/10/2012
[58635.059865]  ffffffff81810777 ffff88041bbbb898 ffffffff815d6ae5
0000000000000002
[58635.059867]  0000000000000000 ffff88041bbbb8c8 ffffffff81307945
0000000000005000
[58635.059869]  ffff880035745e00 0000000000000000 ffff88040dc67780
ffff88041bbbb8d8
[58635.059871] Call Trace:
[58635.059874]  [<ffffffff815d6ae5>] dump_stack+0x4f/0x7b
[58635.059877]  [<ffffffff81307945>] check_preemption_disabled+0xe5/0x100
[58635.059879]  [<ffffffff81307977>] debug_smp_processor_id+0x17/0x20
[58635.059884]  [<ffffffffa003d079>]
megasas_build_ldio_fusion+0x259/0x450 [megaraid_sas]
[58635.059887]  [<ffffffffa003d312>]
megasas_build_io_fusion+0xa2/0x590 [megaraid_sas]
[58635.059890]  [<ffffffff815de2e2>] ? _raw_spin_unlock_irqrestore+0x22/0x40
[58635.059893]  [<ffffffffa003d8b6>]
megasas_build_and_issue_cmd_fusion+0x66/0x$00 [megaraid_sas]
[58635.059896]  [<ffffffffa00310dc>] megasas_queue_command+0x12c/0x170
[megarai$_sas]
[58635.059898]  [<ffffffff8141d45b>] ? scsi_prep_fn+0xdb/0x180
[58635.059901]  [<ffffffff8141c2ae>] scsi_dispatch_cmd+0xee/0x340
[58635.059902]  [<ffffffff8141f019>] scsi_request_fn+0x4b9/0x6c0
[58635.059904]  [<ffffffff815de215>] ? _raw_spin_lock_irqsave+0x25/0x30
[58635.059906]  [<ffffffff812c9077>] __blk_run_queue+0x37/0x50
[58635.059908]  [<ffffffff812ec639>] cfq_insert_request+0x2a9/0x600
[58635.059910]  [<ffffffff812c618c>] __elv_add_request+0x1bc/0x300
[58635.059912]  [<ffffffff812ccd28>] blk_flush_plug_list+0x1b8/0x210
[58635.059914]  [<ffffffff812cd118>] blk_finish_plug+0x18/0x50
[58635.059916]  [<ffffffff81154543>] __do_page_cache_readahead+0x1d3/0x250
[58635.059919]  [<ffffffff81154708>] ondemand_readahead+0x148/0x2a0
[58635.059921]  [<ffffffff8114880c>] ? pagecache_get_page+0x2c/0x1d0
[58635.059923]  [<ffffffff811549e1>] page_cache_sync_readahead+0x31/0x50
[58635.059924]  [<ffffffff81149bf9>] generic_file_read_iter+0x449/0x630
[58635.059926]  [<ffffffff811acede>] new_sync_read+0x7e/0xb0
[58635.059928]  [<ffffffff811ae0c8>] __vfs_read+0x18/0x50
[58635.059930]  [<ffffffff811ae186>] vfs_read+0x86/0x140
[58635.059932]  [<ffffffff811ae289>] SyS_read+0x49/0xb0
[58635.059933]  [<ffffffff81045dac>] ? do_page_fault+0xc/0x10
[58635.059935]  [<ffffffff815dea72>] system_call_fastpath+0x12/0x17

Thanks,
Andy

Re: smp_processor_id warning in megasas driver on 3.19.3

[James Bottomley]

On Wed, 2015-04-08 at 10:59 -0700, Andy Lutomirski wrote:
> This is a regression somewhere between 3.15 and 3.19.3.  Let me know
> if more diagnostics would be helpful.

It's not a regression.  Likely someone turned on additional warnings.
So the problem is that the warning is incorrect: the use of
smp_processor_id() isn't pre-empt unsafe.  The driver is using it as a
hint as to which queue it should be using, so it doesn't matter if
pre-empt schedules the driver thread away from that CPU.

I presume the warning is because whoever added it thinks that you should
be using the get/put cpu API, which would be wholly inappropriate here
because we don't want to bind the thread, we just want a hint about the
queue.

James

Re: smp_processor_id warning in megasas driver on 3.19.3

[Andy Lutomirski]

On Wed, Apr 8, 2015 at 11:17 AM, James Bottomley
<James.Bottomley@hansenpartnership.com> wrote:
> On Wed, 2015-04-08 at 10:59 -0700, Andy Lutomirski wrote:
>> This is a regression somewhere between 3.15 and 3.19.3.  Let me know
>> if more diagnostics would be helpful.
>
> It's not a regression.  Likely someone turned on additional warnings.
> So the problem is that the warning is incorrect: the use of
> smp_processor_id() isn't pre-empt unsafe.  The driver is using it as a
> hint as to which queue it should be using, so it doesn't matter if
> pre-empt schedules the driver thread away from that CPU.

The warning goes a long way back.  For example, this change from 2005
seems to have refactored it:

commit 39c715b71740c4a78ba4769fb54826929bac03cb
Author: Ingo Molnar <mingo@elte.hu>
Date:   Tue Jun 21 17:14:34 2005 -0700

    [PATCH] smp_processor_id() cleanup


>
> I presume the warning is because whoever added it thinks that you should
> be using the get/put cpu API, which would be wholly inappropriate here
> because we don't want to bind the thread, we just want a hint about the
> queue.

Would raw_smp_processor_id be a good compromise?  I'm testing a patch
right now and, if it works, I can send it and cc stable.

--Andy

Re: smp_processor_id warning in megasas driver on 3.19.3

[James Bottomley]

On Wed, 2015-04-08 at 11:26 -0700, Andy Lutomirski wrote:
> On Wed, Apr 8, 2015 at 11:17 AM, James Bottomley
> <James.Bottomley@hansenpartnership.com> wrote:
> > On Wed, 2015-04-08 at 10:59 -0700, Andy Lutomirski wrote:
> >> This is a regression somewhere between 3.15 and 3.19.3.  Let me know
> >> if more diagnostics would be helpful.
> >
> > It's not a regression.  Likely someone turned on additional warnings.
> > So the problem is that the warning is incorrect: the use of
> > smp_processor_id() isn't pre-empt unsafe.  The driver is using it as a
> > hint as to which queue it should be using, so it doesn't matter if
> > pre-empt schedules the driver thread away from that CPU.
> 
> The warning goes a long way back.  For example, this change from 2005
> seems to have refactored it:
> 
> commit 39c715b71740c4a78ba4769fb54826929bac03cb
> Author: Ingo Molnar <mingo@elte.hu>
> Date:   Tue Jun 21 17:14:34 2005 -0700
> 
>     [PATCH] smp_processor_id() cleanup
> 
> 
> >
> > I presume the warning is because whoever added it thinks that you should
> > be using the get/put cpu API, which would be wholly inappropriate here
> > because we don't want to bind the thread, we just want a hint about the
> > queue.
> 
> Would raw_smp_processor_id be a good compromise?  I'm testing a patch
> right now and, if it works, I can send it and cc stable.

Anything that doesn't dump the warning would be fine.  Of course, the
current queue selection

smp_processor_id() % instance->msix_vectors

Is a bit suboptimal anyway, so perhaps avago would like to fix it more
elegantly.

James

Re: smp_processor_id warning in megasas driver on 3.19.3

[Christoph Hellwig]

On Wed, Apr 08, 2015 at 10:59:36AM -0700, Andy Lutomirski wrote:
> This is a regression somewhere between 3.15 and 3.19.3.  Let me know
> if more diagnostics would be helpful.

Try this patch:

diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 675b5e7..5a0800d 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct megasas_instance *instance,
 			fp_possible = io_info.fpOkForIo;
 	}
 
-	/* Use smp_processor_id() for now until cmd->request->cpu is CPU
+	/* Use raw_smp_processor_id() for now until cmd->request->cpu is CPU
 	   id by default, not CPU group id, otherwise all MSI-X queues won't
 	   be utilized */
 	cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
-		smp_processor_id() % instance->msix_vectors : 0;
+		raw_smp_processor_id() % instance->msix_vectors : 0;
 
 	if (fp_possible) {
 		megasas_set_pd_lba(io_request, scp->cmd_len, &io_info, scp,
@@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct megasas_instance *instance,
 			<< MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
 		cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
 		cmd->request_desc->SCSIIO.MSIxIndex =
-			instance->msix_vectors ? smp_processor_id() % instance->msix_vectors : 0;
+			instance->msix_vectors ?
+				raw_smp_processor_id() %
+					instance->msix_vectors :
+				0;
 		os_timeout_value = scmd->request->timeout / HZ;
 
 		if (instance->secure_jbod_support &&

RE: smp_processor_id warning in megasas driver on 3.19.3

[Elliott, Robert (Server Storage)]

> -----Original Message-----
> From: linux-scsi-owner@vger.kernel.org [mailto:linux-scsi-
> owner@vger.kernel.org] On Behalf Of Andy Lutomirski
> Sent: Wednesday, April 8, 2015 1:27 PM
> To: James Bottomley
> Cc: Kashyap Desai; Sumit Saxena; Uday Lingala;
> megaraidlinux.pdl@avagotech.com; Linux SCSI List
> Subject: Re: smp_processor_id warning in megasas driver on 3.19.3
> 
...
> Would raw_smp_processor_id be a good compromise?  I'm testing a patch
> right now and, if it works, I can send it and cc stable.
> 
> --Andy

That's what hpsa uses.

---
Robert Elliott, HP Server Storage

Re: smp_processor_id warning in megasas driver on 3.19.3

[Andy Lutomirski]

On Wed, Apr 8, 2015 at 11:51 AM, Christoph Hellwig <hch@infradead.org> wrote:
> On Wed, Apr 08, 2015 at 10:59:36AM -0700, Andy Lutomirski wrote:
>> This is a regression somewhere between 3.15 and 3.19.3.  Let me know
>> if more diagnostics would be helpful.
>
> Try this patch:
>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 675b5e7..5a0800d 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct megasas_instance *instance,
>                         fp_possible = io_info.fpOkForIo;
>         }
>
> -       /* Use smp_processor_id() for now until cmd->request->cpu is CPU
> +       /* Use raw_smp_processor_id() for now until cmd->request->cpu is CPU
>            id by default, not CPU group id, otherwise all MSI-X queues won't
>            be utilized */
>         cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
> -               smp_processor_id() % instance->msix_vectors : 0;
> +               raw_smp_processor_id() % instance->msix_vectors : 0;
>
>         if (fp_possible) {
>                 megasas_set_pd_lba(io_request, scp->cmd_len, &io_info, scp,
> @@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct megasas_instance *instance,
>                         << MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
>                 cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
>                 cmd->request_desc->SCSIIO.MSIxIndex =
> -                       instance->msix_vectors ? smp_processor_id() % instance->msix_vectors : 0;
> +                       instance->msix_vectors ?
> +                               raw_smp_processor_id() %
> +                                       instance->msix_vectors :
> +                               0;
>                 os_timeout_value = scmd->request->timeout / HZ;
>
>                 if (instance->secure_jbod_support &&

I wrote an effectively identical patch (differed in whitespace and
formatting), and it seems to work.

Tested-by: Andy Lutomirski <luto@kernel.org>

--Andy

-- 
Andy Lutomirski
AMA Capital Management, LLC

RE: smp_processor_id warning in megasas driver on 3.19.3

[Sumit Saxena]

>-----Original Message-----
>From: Andy Lutomirski [mailto:luto@amacapital.net]
>Sent: Thursday, April 09, 2015 12:45 AM
>To: Christoph Hellwig
>Cc: Kashyap Desai; Sumit Saxena; Uday Lingala;
>megaraidlinux.pdl@avagotech.com; Linux SCSI List
>Subject: Re: smp_processor_id warning in megasas driver on 3.19.3
>
>On Wed, Apr 8, 2015 at 11:51 AM, Christoph Hellwig <hch@infradead.org>
>wrote:
>> On Wed, Apr 08, 2015 at 10:59:36AM -0700, Andy Lutomirski wrote:
>>> This is a regression somewhere between 3.15 and 3.19.3.  Let me know
>>> if more diagnostics would be helpful.
>>
>> Try this patch:
>>
>> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> index 675b5e7..5a0800d 100644
>> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
>> @@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct
>megasas_instance *instance,
>>                         fp_possible = io_info.fpOkForIo;
>>         }
>>
>> -       /* Use smp_processor_id() for now until cmd->request->cpu is CPU
>> +       /* Use raw_smp_processor_id() for now until cmd->request->cpu
>> + is CPU
>>            id by default, not CPU group id, otherwise all MSI-X queues
>> won't
>>            be utilized */
>>         cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
>> -               smp_processor_id() % instance->msix_vectors : 0;
>> +               raw_smp_processor_id() % instance->msix_vectors : 0;
>>
>>         if (fp_possible) {
>>                 megasas_set_pd_lba(io_request, scp->cmd_len, &io_info,
>> scp, @@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct
>megasas_instance *instance,
>>                         << MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
>>                 cmd->request_desc->SCSIIO.DevHandle =
>> io_request->DevHandle;
>>                 cmd->request_desc->SCSIIO.MSIxIndex =
>> -                       instance->msix_vectors ? smp_processor_id() %
>> instance-
>>msix_vectors : 0;
>> +                       instance->msix_vectors ?
>> +                               raw_smp_processor_id() %
>> +                                       instance->msix_vectors :
>> +                               0;
>>                 os_timeout_value = scmd->request->timeout / HZ;
>>
>>                 if (instance->secure_jbod_support &&
>
>I wrote an effectively identical patch (differed in whitespace and
>formatting),
>and it seems to work.


Acked-by: Sumit Saxena <sumit.saxena@avagotech.com>

>
>Tested-by: Andy Lutomirski <luto@kernel.org>
>
>--Andy
>
>--
>Andy Lutomirski
>AMA Capital Management, LLC

[PATCH] megaraid_sas: use raw_smp_processor_id()

[Christoph Hellwig]

We only want to steer the I/O completion towards a queue, but don't
actually access any per-CPU data, so the raw_ version is fine to use
and avoids the warnings when using smp_processor_id().

Signed-off-by: Christoph Hellwig <hch@lst.de>
Reported-by: Andy Lutomirski <luto@kernel.org>
Tested-by: Andy Lutomirski <luto@kernel.org>
Acked-by: Sumit Saxena <sumit.saxena@avagotech.com>

diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
index 675b5e7..5a0800d 100644
--- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
+++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
@@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct megasas_instance *instance,
 			fp_possible = io_info.fpOkForIo;
 	}
 
-	/* Use smp_processor_id() for now until cmd->request->cpu is CPU
+	/* Use raw_smp_processor_id() for now until cmd->request->cpu is CPU
 	   id by default, not CPU group id, otherwise all MSI-X queues won't
 	   be utilized */
 	cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
-		smp_processor_id() % instance->msix_vectors : 0;
+		raw_smp_processor_id() % instance->msix_vectors : 0;
 
 	if (fp_possible) {
 		megasas_set_pd_lba(io_request, scp->cmd_len, &io_info, scp,
@@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct megasas_instance *instance,
 			<< MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
 		cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
 		cmd->request_desc->SCSIIO.MSIxIndex =
-			instance->msix_vectors ? smp_processor_id() % instance->msix_vectors : 0;
+			instance->msix_vectors ?
+				raw_smp_processor_id() %
+					instance->msix_vectors :
+				0;
 		os_timeout_value = scmd->request->timeout / HZ;
 
 		if (instance->secure_jbod_support &&

Re: [PATCH] megaraid_sas: use raw_smp_processor_id()

[Andy Lutomirski]

On Wed, Apr 15, 2015 at 9:44 AM, Christoph Hellwig <hch@infradead.org> wrote:
> We only want to steer the I/O completion towards a queue, but don't
> actually access any per-CPU data, so the raw_ version is fine to use
> and avoids the warnings when using smp_processor_id().
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Andy Lutomirski <luto@kernel.org>
> Tested-by: Andy Lutomirski <luto@kernel.org>
> Acked-by: Sumit Saxena <sumit.saxena@avagotech.com>

Cc: stable?

>
> diff --git a/drivers/scsi/megaraid/megaraid_sas_fusion.c b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> index 675b5e7..5a0800d 100644
> --- a/drivers/scsi/megaraid/megaraid_sas_fusion.c
> +++ b/drivers/scsi/megaraid/megaraid_sas_fusion.c
> @@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct megasas_instance *instance,
>                         fp_possible = io_info.fpOkForIo;
>         }
>
> -       /* Use smp_processor_id() for now until cmd->request->cpu is CPU
> +       /* Use raw_smp_processor_id() for now until cmd->request->cpu is CPU
>            id by default, not CPU group id, otherwise all MSI-X queues won't
>            be utilized */
>         cmd->request_desc->SCSIIO.MSIxIndex = instance->msix_vectors ?
> -               smp_processor_id() % instance->msix_vectors : 0;
> +               raw_smp_processor_id() % instance->msix_vectors : 0;
>
>         if (fp_possible) {
>                 megasas_set_pd_lba(io_request, scp->cmd_len, &io_info, scp,
> @@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct megasas_instance *instance,
>                         << MR_RAID_CTX_RAID_FLAGS_IO_SUB_TYPE_SHIFT;
>                 cmd->request_desc->SCSIIO.DevHandle = io_request->DevHandle;
>                 cmd->request_desc->SCSIIO.MSIxIndex =
> -                       instance->msix_vectors ? smp_processor_id() % instance->msix_vectors : 0;
> +                       instance->msix_vectors ?
> +                               raw_smp_processor_id() %
> +                                       instance->msix_vectors :
> +                               0;
>                 os_timeout_value = scmd->request->timeout / HZ;
>
>                 if (instance->secure_jbod_support &&



-- 
Andy Lutomirski
AMA Capital Management, LLC

Re: [PATCH] megaraid_sas: use raw_smp_processor_id()

[Hannes Reinecke]

On 04/15/2015 06:44 PM, Christoph Hellwig wrote:
> We only want to steer the I/O completion towards a queue, but don't
> actually access any per-CPU data, so the raw_ version is fine to use
> and avoids the warnings when using smp_processor_id().
> 
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> Reported-by: Andy Lutomirski <luto@kernel.org>
> Tested-by: Andy Lutomirski <luto@kernel.org>
> Acked-by: Sumit Saxena <sumit.saxena@avagotech.com>
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      zSeries & Storage
hare@suse.de			      +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html