SCSI host num allocation improvement

PATCH In review Ben Collins 4 participants 11 messages Feb 26, 2004 – Feb 27, 2004 SCSI

Share & tools

Ben Collins

author

2004-02-26 23:54 UTC #0 lore raw

PATCH 1/1 SCSI host num allocation improvement

After doing a large round of debugging and fixups for ieee1394, I started getting really aggrivated with the way that the scsi layer allocates host numbers by forever incrementing from the last one.

Before too long, I was getting scsi137, etc. Not only did it look bad, it was bad. Cdrecord only scans bus 0-15, which means after a few hotplugs with your USB or Firewire CD/DVD recorder, you wont be able to burn to it. Yes, I know cdrecord needs to be fixed aswell, but I've had my fill of trying to convince the author of anything, so I wont be sending him a patch.

Anyway, this patch enables a race free (I hope) allocation of host numbers. And it will pick the lowest available number first. I've only tested it with ieee1394 sbp2, so real scsi users should give it a go.

You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================

[email protected], 2004-02-26 18:40:58-05:00, [email protected]
[SCSI]: Add a scsi_alloc_host_num function for race free, and non-incremental host ids

 hosts.c |   44 ++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 40 insertions(+), 4 deletions(-)

drivers/scsi/hosts.c +40 -4

diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
@@ -38,9 +38,6 @@
 #include "scsi_logging.h"
 
 
-static int scsi_host_next_hn;		/* host_no for next new host */
-
-
 static void scsi_host_cls_release(struct class_device *class_dev)
 {
 	put_device(&class_to_shost(class_dev)->shost_gendev);

@@ -166,6 +163,38 @@
 	kfree(shost);
 }
 
+static DECLARE_MUTEX(host_num_lock);
+/**
+ * scsi_host_num_alloc - allocate a unique host number for a scsi host.
+ *
+ * Note:
+ *      Must hold host_num_lock when calling this, and continue holding it
+ *      till after the host is added to the shost_class.
+ *
+ * Return value:
+ *      A unique host number.
+ **/
+static int scsi_host_num_alloc(void)
+{
+	int host_num = 0;
+	struct class *class = &shost_class;
+	struct class_device *cdev;
+	struct Scsi_Host *shost;
+
+	down_read(&class->subsys.rwsem);
+next_host_num_try:
+	list_for_each_entry(cdev, &class->children, node) {
+		shost = class_to_shost(cdev);
+		if (shost->host_no == host_num) {
+			host_num++;
+			goto next_host_num_try;
+		}
+	}
+	up_read(&class->subsys.rwsem);
+
+	return host_num;
+}
+
 /**
  * scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template

@@ -214,7 +243,6 @@
 
 	init_MUTEX(&shost->scan_mutex);
 
-	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */

@@ -263,6 +291,12 @@
 	if (rval)
 		goto fail_kfree;
 
+	/* Hold this lock until after we've added this to the scsi_host
+	 * class, to avoid race condititons with the host number
+	 * allocation scheme. */
+	down(&host_num_lock);
+	shost->host_no = scsi_host_num_alloc();
+
 	device_initialize(&shost->shost_gendev);
 	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);

@@ -273,6 +307,8 @@
 	shost->shost_classdev.class = &shost_class;
 	snprintf(shost->shost_classdev.class_id, BUS_ID_SIZE, "host%d",
 		  shost->host_no);
+
+	up(&host_num_lock);
 
 	shost->eh_notify = &complete;
 	rval = kernel_thread(scsi_error_handler, shost, 0);

===================================================================

This BitKeeper patch contains the following changesets:
1.1589
## Wrapped with gzip_uu ##

M'XL( *"&/D [56:V_;-A3]+/Z*"Q3('">6J*<=!0[2)<$6;-V"9 $&=(5!
MDTQ$1*8RDK(;S/WO(ZG*>=1ML:*3!4BZO(]S[SF4_ JN-5=E,*=-70NIT2OX MN=&F#!B?"R+#1MU:TV736%/4:A5I1:,[KB2O(^O?OA\E88&LRP4QM((E5[H, MXC#=6,S#/2^#R[.?KG]]?8G0= HG%9&W_(H;F$Z1:=22U$P?$U/5C0R-(E(O MN"$A;1;KC>LZP3BQOSP>IS@OUG&!L_&:QBR.219SAI-L4F2(D25?''?PPOG= M7!@=2AO]+$^&DR3&*4[B\3K/\GB"3B$.XWQR #B+<!(E!<23,L-E/AGAO,08 M^ND</TX%]F(88?0C?-\.3A"%MU<G5^?O2GC-&!#05(L9J>N&SBK+S$RV"[AI M)36BD7#3*%"$<KA1G.\#D0QD(T="4L477!I2@PL"P33Z!?(,CS&Z>&0 C?[C M@1 F&!U]I6FFA!-"Y*!'KKX.Z9/^,XSMY).DR-9S/)GS<4YRRT:6L*V3_FPZ M1V21I.E!DJ]3'"=C+Z]MWE]7VK=#1N3N?G'<:%8[L&_[,N\^FS*-DQ3C/,^L M_I(TCC.OOX/Q,_6E!V62?TE]&891]K_([[O+KN/F=QBIE3^MC"ZVTO0-<CS- M8DC1>5Q,($V0-L0("J=G)_9E<S9[<_W'V9^#'O[,]G*W>XBBX1#!L&MPL^8[ MA1'X*S'<3J"5XN^6=XU8ESE7ON]N--X<VCPNU6^-X:6[\<>;U@943<W@6658 M55P")8[*6S"5T-W@:".-?9%R'^*6A-FD,J*N@=P86]I4'Z$(#80QSBSSWJA] M%5H3K7L\E]RT2H+51?L$U^LM';F(8=3/34BS;2R#92/8+OH'!<YAHX8IX$,4 M:*-::L#7AV%WF<+.$U0OG&:,+X65SI#:F\>U*U?7?7I@Z&,/T5\H8,U*SA0G M;+#C8T='NIWK!QVJE>8+RZ7D[\TC6J,>2A34PCY:IF:<T&IFU:@>!J[6/O1) M:"5JIKC<MZIE?!=L9X$O:I%W&$TS\P8?:.L$@;B!@3>-CKIZC7VI;(;1Y0CZ MQ[T]%Q+<-I:C3R"ZI0_(G>W]%YNS U =DWWX(?I@K:?VRP4Q.D^*' H41$/[ MR;9J<Y("K[36*JK7S8K_L.2]8IQ'+YN>9Q18@7@ ^VZ-.+*[S6V5R801II$:
M5L)4CQKLQ.,C/VX8]U;0M+*[/P0K*,_<8.?EU@M>3G"KW'SKY\DXA\3-H+W_ <--'F[X4M2>]TNYBFN#@@\W&!_@4R$Z&RT@@

Signature · 4 lines

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Andrew Morton

2004-02-27 01:19 UTC #1 lore raw

drivers/scsi/hosts.c line 190

Quoted 7 lines · Ben Collins

drivers/scsi/hosts.c +6 -0

+next_host_num_try:
+	list_for_each_entry(cdev, &class->children, node) {
+		shost = class_to_shost(cdev);
+		if (shost->host_no == host_num) {
+			host_num++;
+			goto next_host_num_try;

yitch. I realise this is called rarely, for realtively small numbers of N (which gets squared), but.

This allocate-me-the-lowest-available-number is a common idiom in the kernel and we really should do it better. Seems we need to convert the dynamic pty allocation to do it as well - it has yet another open-coded ad-hoc allocator.

The lib/idr.c code is a bit clumsy but it does do the job relatively efficiently. If it needs some helper wrappers or API simplification then let's add those.

Andrew Morton

2004-02-27 01:36 UTC #2 lore raw

Quoted 3 lines · Andrew Morton

The lib/idr.c code is a bit clumsy but it does do the job relatively efficiently.

hmm, not too bad actually. It compiles, but I didn't test it.

drivers/scsi/hosts.c renamed from drivers/scsi/hosts.c~scsi-host-allocation-fix +31 -4

@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/completion.h>
 #include <linux/unistd.h>
+#include <linux/idr.h>
 
 #include <scsi/scsi_host.h>
 #include "scsi.h"

@@ -38,9 +39,6 @@
 #include "scsi_logging.h"
 
 
-static int scsi_host_next_hn;		/* host_no for next new host */
-
-
 static void scsi_host_cls_release(struct class_device *class_dev)
 {
 	put_device(&class_to_shost(class_dev)->shost_gendev);

@@ -166,6 +164,30 @@ static void scsi_host_dev_release(struct
 	kfree(shost);
 }
 
+static DECLARE_MUTEX(host_num_lock);
+static struct idr allocated_hosts;
+
+/**
+ * scsi_host_num_alloc - allocate a unique host number for a scsi host.
+ *
+ * Note:
+ *      Must hold host_num_lock when calling this, and continue holding it
+ *      till after the host is added to the shost_class.
+ *
+ * Return value:
+ *      A unique host number.
+ **/
+static int scsi_host_num_alloc(void)
+{
+	int ret;
+
+	down(&host_num_lock);
+	idr_pre_get(&allocated_hosts, GFP_KERNEL);
+	ret = idr_get_new(&allocated_hosts, NULL);
+	up(&host_num_lock);
+	return ret;
+}
+
 /**
  * scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template

@@ -214,7 +236,6 @@ struct Scsi_Host *scsi_host_alloc(struct
 
 	init_MUTEX(&shost->scan_mutex);
 
-	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */

@@ -263,6 +284,8 @@ struct Scsi_Host *scsi_host_alloc(struct
 	if (rval)
 		goto fail_kfree;
 
+	shost->host_no = scsi_host_num_alloc();
+
 	device_initialize(&shost->shost_gendev);
 	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);

@@ -308,6 +331,9 @@ struct Scsi_Host *scsi_register(struct s
 void scsi_unregister(struct Scsi_Host *shost)
 {
 	list_del(&shost->sht_legacy_list);
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, shost->host_no);
+	up(&host_num_lock);
 	scsi_host_put(shost);
 }

@@ -361,6 +387,7 @@ void scsi_host_put(struct Scsi_Host *sho
 
 int scsi_init_hosts(void)
 {
+	idr_init(&allocated_hosts);
 	return class_register(&shost_class);
 }

Ben Collins

author

2004-02-27 02:10 UTC #3 lore raw

Quoted 6 lines · Andrew Morton

Quoted 3 lines · Andrew Morton
The lib/idr.c code is a bit clumsy but it does do the job relatively efficiently.
hmm, not too bad actually. It compiles, but I didn't test it.

Ah, that looks a lot cleaner. Definitely better than what I had. I need to use idr in firewire then.

Signature · 4 lines

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Ben Collins

author

2004-02-27 02:32 UTC #4 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Quoted 3 lines · Andrew Morton
The lib/idr.c code is a bit clumsy but it does do the job relatively efficiently.
hmm, not too bad actually. It compiles, but I didn't test it.

Oh, this isn't any good. It does the same thing as the old way. Steadily incrementing numbers.

Signature · 4 lines

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Andrew Morton

2004-02-27 02:51 UTC #5 lore raw

Quoted 11 lines · Ben Collins

Quoted 6 lines · Andrew Morton
Quoted 3 lines · Andrew Morton
The lib/idr.c code is a bit clumsy but it does do the job relatively efficiently.
hmm, not too bad actually. It compiles, but I didn't test it.
Oh, this isn't any good. It does the same thing as the old way. Steadily incrementing numbers.

In that case the idr_remove() was in the wrong place. It was a wild guess.

Ben Collins

author

2004-02-27 02:50 UTC #6 lore raw

In reply to Andrew Morton

Quoted 6 lines · Andrew Morton

Quoted 3 lines · Andrew Morton
The lib/idr.c code is a bit clumsy but it does do the job relatively efficiently.
hmm, not too bad actually. It compiles, but I didn't test it.

Actually there was a bug. You weren't really releasing the idr's. This patch should work (linux-scsi added to Cc). Not sure how the legacy style case actually gets handled, but I figure this should take care of it.

===== drivers/scsi/hosts.c 1.96 vs edited =====

drivers/scsi/hosts.c +16 -3

@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/completion.h>
 #include <linux/unistd.h>
+#include <linux/idr.h>
 
 #include <scsi/scsi_host.h>
 #include "scsi.h"

@@ -37,12 +38,15 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
-
-static int scsi_host_next_hn;		/* host_no for next new host */
+static DECLARE_MUTEX(host_num_lock);
+static struct idr allocated_hosts;
 
 
 static void scsi_host_cls_release(struct class_device *class_dev)
 {
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, class_to_shost(class_dev)->host_no);
+	up(&host_num_lock);
 	put_device(&class_to_shost(class_dev)->shost_gendev);
 }

@@ -166,6 +170,7 @@
 	kfree(shost);
 }
 
+
 /**
  * scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template

@@ -214,7 +219,6 @@
 
 	init_MUTEX(&shost->scan_mutex);
 
-	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */

@@ -263,6 +267,11 @@
 	if (rval)
 		goto fail_kfree;
 
+	down(&host_num_lock);
+	idr_pre_get(&allocated_hosts, GFP_KERNEL);
+	shost->host_no = idr_get_new(&allocated_hosts, NULL);
+	up(&host_num_lock);
+
 	device_initialize(&shost->shost_gendev);
 	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);

@@ -307,6 +316,9 @@
 
 void scsi_unregister(struct Scsi_Host *shost)
 {
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, class_to_shost(class_dev)->host_no);
+	up(&host_num_lock);
 	list_del(&shost->sht_legacy_list);
 	scsi_host_put(shost);
 }

@@ -361,6 +373,7 @@
 
 int scsi_init_hosts(void)
 {
+	idr_init(&allocated_hosts);
 	return class_register(&shost_class);
 }

Signature · 4 lines

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Ben Collins

author

2004-02-27 03:05 UTC #7 lore raw

In reply to Andrew Morton

Here's one that actually compiles.

===== drivers/scsi/hosts.c 1.96 vs edited =====

drivers/scsi/hosts.c +16 -3

@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/completion.h>
 #include <linux/unistd.h>
+#include <linux/idr.h>
 
 #include <scsi/scsi_host.h>
 #include "scsi.h"

@@ -37,12 +38,15 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
-
-static int scsi_host_next_hn;		/* host_no for next new host */
+static DECLARE_MUTEX(host_num_lock);
+static struct idr allocated_hosts;
 
 
 static void scsi_host_cls_release(struct class_device *class_dev)
 {
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, class_to_shost(class_dev)->host_no);
+	up(&host_num_lock);
 	put_device(&class_to_shost(class_dev)->shost_gendev);
 }

@@ -166,6 +170,7 @@
 	kfree(shost);
 }
 
+
 /**
  * scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template

@@ -214,7 +219,6 @@
 
 	init_MUTEX(&shost->scan_mutex);
 
-	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */

@@ -263,6 +267,11 @@
 	if (rval)
 		goto fail_kfree;
 
+	down(&host_num_lock);
+	idr_pre_get(&allocated_hosts, GFP_KERNEL);
+	shost->host_no = idr_get_new(&allocated_hosts, NULL);
+	up(&host_num_lock);
+
 	device_initialize(&shost->shost_gendev);
 	snprintf(shost->shost_gendev.bus_id, BUS_ID_SIZE, "host%d",
 		shost->host_no);

@@ -307,6 +316,9 @@
 
 void scsi_unregister(struct Scsi_Host *shost)
 {
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, shost->host_no);
+	up(&host_num_lock);
 	list_del(&shost->sht_legacy_list);
 	scsi_host_put(shost);
 }

@@ -361,6 +373,7 @@
 
 int scsi_init_hosts(void)
 {
+	idr_init(&allocated_hosts);
 	return class_register(&shost_class);
 }

Signature · 4 lines

Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

Mike Anderson

2004-02-27 16:34 UTC #8 lore raw

drivers/scsi/hosts.c line 51

Quoted 8 lines · Ben Collins [[email protected]] wrote:

static void scsi_host_cls_release(struct class_device *class_dev) {
drivers/scsi/hosts.c +3 -0
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, class_to_shost(class_dev)->host_no);
+	up(&host_num_lock);
 	put_device(&class_to_shost(class_dev)->shost_gendev);
 }

Should the idr_remove be done in scsi_host_dev_release instead? We really should not make this number available until everyone is done with this host.

-andmike
--
Michael Anderson
[email protected]

Andrew Morton

2004-02-27 22:28 UTC #9 lore raw

Quoted 14 lines · Mike Anderson

Review
Quoted 8 lines · Ben Collins [[email protected]] wrote:
static void scsi_host_cls_release(struct class_device *class_dev) {
(quoted diff) +3 -0
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, class_to_shost(class_dev)->host_no);
+	up(&host_num_lock);
 	put_device(&class_to_shost(class_dev)->shost_gendev);
 }
Should the idr_remove be done in scsi_host_dev_release instead? We really should not make this number available until everyone is done with this host.

Beats me. If you're right, this is the patch.

drivers/scsi/hosts.c renamed from drivers/scsi/hosts.c~scsi-host-allocation-fix +28 -2

diff -puN drivers/scsi/hosts.c~scsi-host-allocation-fix drivers/scsi/hosts.c
@@ -30,6 +30,7 @@
 #include <linux/list.h>
 #include <linux/completion.h>
 #include <linux/unistd.h>
+#include <linux/idr.h>
 
 #include <scsi/scsi_host.h>
 #include "scsi.h"

@@ -37,9 +38,26 @@
 #include "scsi_priv.h"
 #include "scsi_logging.h"
 
+static DECLARE_MUTEX(host_num_lock);
+static struct idr allocated_hosts;
 
-static int scsi_host_next_hn;		/* host_no for next new host */
+static int alloc_host_no(void)
+{
+	int ret = -ENOMEM;
+
+	down(&host_num_lock);
+	if (idr_pre_get(&allocated_hosts, GFP_KERNEL))
+		ret = idr_get_new(&allocated_hosts, NULL);
+	up(&host_num_lock);
+	return ret;
+}
 
+static void free_host_no(int host_no)
+{
+	down(&host_num_lock);
+	idr_remove(&allocated_hosts, host_no);
+	up(&host_num_lock);
+}
 
 static void scsi_host_cls_release(struct class_device *class_dev)
 {

@@ -163,9 +181,11 @@ static void scsi_host_dev_release(struct
 	 */
 	if (parent)
 		put_device(parent);
+	free_host_no(shost->host_no);
 	kfree(shost);
 }
 
+
 /**
  * scsi_host_alloc - register a scsi host adapter instance.
  * @sht:	pointer to scsi host template

@@ -184,6 +204,7 @@ struct Scsi_Host *scsi_host_alloc(struct
 	struct Scsi_Host *shost;
 	int gfp_mask = GFP_KERNEL, rval;
 	DECLARE_COMPLETION(complete);
+	int host_no;
 
 	if (sht->unchecked_isa_dma && privsize)
 		gfp_mask |= __GFP_DMA;

@@ -214,7 +235,6 @@ struct Scsi_Host *scsi_host_alloc(struct
 
 	init_MUTEX(&shost->scan_mutex);
 
-	shost->host_no = scsi_host_next_hn++; /* XXX(hch): still racy */
 	shost->dma_channel = 0xff;
 
 	/* These three are default values which can be overridden */

@@ -259,6 +279,11 @@ struct Scsi_Host *scsi_host_alloc(struct
 	else
 		shost->dma_boundary = 0xffffffff;
 
+	host_no = alloc_host_no();
+	if (host_no < 0)
+		goto fail_kfree;
+	shost->host_no = host_no;
+
 	rval = scsi_setup_command_freelist(shost);
 	if (rval)
 		goto fail_kfree;

@@ -361,6 +386,7 @@ void scsi_host_put(struct Scsi_Host *sho
 
 int scsi_init_hosts(void)
 {
+	idr_init(&allocated_hosts);
 	return class_register(&shost_class);
 }

H. Peter Anvin

2004-02-27 18:51 UTC #10 lore raw

In reply to Andrew Morton

Followup to:  <[email protected]>
By author:    Andrew Morton <[email protected]>
In newsgroup: linux.dev.kernel

Quoted 6 lines

This allocate-me-the-lowest-available-number is a common idiom in the kernel and we really should do it better. Seems we need to convert the dynamic pty allocation to do it as well - it has yet another open-coded ad-hoc allocator.

Well, I actually *didn't* want it to be allocate-the-lowest-available number. I deliberately went with the same allocation scheme used for PIDs (continual advance with wraparound and duplication avoidance); this is a cheap approximation of NRU.

Immediate re-use is *BAD* (in the pty example, it means you're liable to have "write" write to an unrelated session by mistake, for example) at least if there is no penalty for expanding into the full allocated number space. Lowest available number is architecturally mandated for file descriptors, but it doesn't mean it's a preferred allocation scheme by any means.

-hpa