Skip to contentSkip to conversation

smp_processor_id warning in megasas driver on 3.19.3

PATCH In review Andy Lutomirski 6 participants 11 messages Apr 8, 2015 – Apr 21, 2015

Subsystems (2)
SCSI Megaraid SCSI/SAS drivers
lore
Share & tools
lore

Andy Lutomirski

author
#0 lore raw

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

maintainer
#1 lore raw
Quoted 2 lines · Andy Lutomirski

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

author
#2 lore raw
Quoted 9 lines · James Bottomley
Quoted 2 lines · Andy Lutomirski

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 <[email protected]>
Date:   Tue Jun 21 17:14:34 2005 -0700

[PATCH] smp_processor_id() cleanup

Quoted 5 lines

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

maintainer
#3 lore raw
Quoted 23 lines · Andy Lutomirski
Quoted 9 lines · James Bottomley
Quoted 2 lines · Andy Lutomirski

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 <[email protected]>
Date:   Tue Jun 21 17:14:34 2005 -0700

[PATCH] smp_processor_id() cleanup

Quoted 1 line
Quoted 7 lines
Quoted 4 lines

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

#4 lore raw
In reply to Andy Lutomirski
Quoted 9 lines

-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Andy Lutomirski
Sent: Wednesday, April 8, 2015 1:27 PM
To: James Bottomley
Cc: Kashyap Desai; Sumit Saxena; Uday Lingala; [email protected]; Linux SCSI List
Subject: Re: smp_processor_id warning in megasas driver on 3.19.3

...

Quoted 4 lines

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

#5 lore raw
In reply to Andy Lutomirski
Quoted 2 lines · Andy Lutomirski

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

Try this patch:

drivers/scsi/megaraid/megaraid_sas_fusion.c +6 -3
index 675b5e7..5a0800d 100644
@@ -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 &&
author Tested-by
#6 lore raw
Quoted 26 lines · Christoph Hellwig
Quoted 2 lines · Andy Lutomirski

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

Try this patch:

drivers/scsi/megaraid/megaraid_sas_fusion.c +2 -2
index 675b5e7..5a0800d 100644
@@ -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;

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_build_dcdb_fusion(struct megasas_instance *instance, · line 1702

Quoted 10 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +4 -1
                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 <[email protected]>

--Andy

Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
Acked-by
#7 lore raw
Quoted 53 lines

-----Original Message-----
From: Andy Lutomirski [mailto:[email protected]]
Sent: Thursday, April 09, 2015 12:45 AM
To: Christoph Hellwig
Cc: Kashyap Desai; Sumit Saxena; Uday Lingala; [email protected]; Linux SCSI List
Subject: Re: smp_processor_id warning in megasas driver on 3.19.3

drivers/scsi/megaraid/megaraid_sas_fusion.c megasas_build_ldio_fusion(struct

Quoted 12 lines · Christoph Hellwig
Quoted 2 lines · Andy Lutomirski

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

Try this patch:

drivers/scsi/megaraid/megaraid_sas_fusion.c +0 -0
index 675b5e7..5a0800d 100644
@@ -1584,11 +1584,11 @@ megasas_build_ldio_fusion(struct

megasas_instance *instance,

Quoted 15 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +2 -1
                        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 ?

drivers/scsi/megaraid/megaraid_sas_fusion.c +1 -1
-               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,
Quoted 1 line

scp, @@ -1693,7 +1693,10 @@ megasas_build_dcdb_fusion(struct

megasas_instance *instance,

Quoted 13 lines
                        << 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;

drivers/scsi/megaraid/megaraid_sas_fusion.c +4 -0
+                       instance->msix_vectors ?
+                               raw_smp_processor_id() %
+                                       instance->msix_vectors :
+                               0;
                os_timeout_value = scmd->request->timeout / HZ;
Quoted 5 lines
Quoted 1 line
                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 <[email protected]>
Quoted 8 lines
Tested-by Andy Lutomirski <[email protected]>

--Andy

--
Andy Lutomirski
AMA Capital Management, LLC

#8 lore raw

PATCH 1/1 megaraid_sas: use raw_smp_processor_id()

In reply to Andy Lutomirski

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 <[email protected]>
Reported-by Andy Lutomirski <[email protected]>
Tested-by Andy Lutomirski <[email protected]>
Acked-by Sumit Saxena <[email protected]>
drivers/scsi/megaraid/megaraid_sas_fusion.c +6 -3
index 675b5e7..5a0800d 100644
@@ -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 &&
author
#9 lore raw

Re: [PATCH] megaraid_sas: use raw_smp_processor_id()

Quoted 8 lines · 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 <[email protected]>
Reported-by Andy Lutomirski <[email protected]>
Tested-by Andy Lutomirski <[email protected]>
Acked-by Sumit Saxena <[email protected]>
Cc stable?
Quoted 31 lines
drivers/scsi/megaraid/megaraid_sas_fusion.c +6 -3
index 675b5e7..5a0800d 100644
@@ -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 &&
Signature · 2 lines
Andy Lutomirski
AMA Capital Management, LLC
Reviewed-by on v1 1/1
#10 lore raw

Re: [PATCH] megaraid_sas: use raw_smp_processor_id()

In reply to Christoph Hellwig
Quoted 9 lines · 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 <[email protected]>
Reported-by Andy Lutomirski <[email protected]>
Tested-by Andy Lutomirski <[email protected]>
Acked-by Sumit Saxena <[email protected]>
Reviewed-by Hannes Reinecke <[email protected]>

Cheers,

Hannes

Signature · 8 lines
Dr. Hannes Reinecke		      zSeries & Storage
[email protected]			      +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 [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html