public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed
* [BK PATCH] SCSI host num allocation improvement
@ 2004-02-26 23:54 Ben Collins
  2004-02-27  1:19 ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Ben Collins @ 2004-02-26 23:54 UTC (permalink / raw)
  To: James Bottomley; +Cc: Linux Kernel

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.

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


ChangeSet@1.1589, 2004-02-26 18:40:58-05:00, bcollins@debian.org
  [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(-)


diff -Nru a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
--- a/drivers/scsi/hosts.c	Thu Feb 26 18:52:00 2004
+++ b/drivers/scsi/hosts.c	Thu Feb 26 18:52:00 2004
@@ -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@@     
 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-26 23:54 [BK PATCH] SCSI host num allocation improvement Ben Collins
@ 2004-02-27  1:19 ` Andrew Morton
  2004-02-27  1:37   ` Andrew Morton
  2004-02-27 18:51   ` H. Peter Anvin
  0 siblings, 2 replies; 11+ messages in thread
From: Andrew Morton @ 2004-02-27  1:19 UTC (permalink / raw)
  To: Ben Collins; +Cc: James.Bottomley, linux-kernel

Ben Collins <bcollins@debian.org> wrote:
>
> +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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  1:19 ` Andrew Morton
@ 2004-02-27  1:37   ` Andrew Morton
  2004-02-27  2:09     ` Ben Collins
                       ` (3 more replies)
  2004-02-27 18:51   ` H. Peter Anvin
  1 sibling, 4 replies; 11+ messages in thread
From: Andrew Morton @ 2004-02-27  1:37 UTC (permalink / raw)
  To: bcollins, James.Bottomley, linux-kernel

Andrew Morton <akpm@osdl.org> wrote:
>
> 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.



--- 25/drivers/scsi/hosts.c~scsi-host-allocation-fix	Thu Feb 26 17:30:40 2004
+++ 25-akpm/drivers/scsi/hosts.c	Thu Feb 26 17:36:47 2004
@@ -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);
 }
 

_


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  1:37   ` Andrew Morton
@ 2004-02-27  2:09     ` Ben Collins
  2004-02-27  2:32     ` Ben Collins
                       ` (2 subsequent siblings)
  3 siblings, 0 replies; 11+ messages in thread
From: Ben Collins @ 2004-02-27  2:09 UTC (permalink / raw)
  To: Andrew Morton; +Cc: James.Bottomley, linux-kernel

On Thu, Feb 26, 2004 at 05:37:43PM -0800, Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> >
> > 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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  1:37   ` Andrew Morton
  2004-02-27  2:09     ` Ben Collins
@ 2004-02-27  2:32     ` Ben Collins
  2004-02-27  2:51       ` Andrew Morton
  2004-02-27  2:50     ` Ben Collins
  2004-02-27  3:04     ` Ben Collins
  3 siblings, 1 reply; 11+ messages in thread
From: Ben Collins @ 2004-02-27  2:32 UTC (permalink / raw)
  To: Andrew Morton; +Cc: James.Bottomley, linux-kernel

On Thu, Feb 26, 2004 at 05:37:43PM -0800, Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> >
> > 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.

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  1:37   ` Andrew Morton
  2004-02-27  2:09     ` Ben Collins
  2004-02-27  2:32     ` Ben Collins
@ 2004-02-27  2:50     ` Ben Collins
  2004-02-27  3:04     ` Ben Collins
  3 siblings, 0 replies; 11+ messages in thread
From: Ben Collins @ 2004-02-27  2:50 UTC (permalink / raw)
  To: Andrew Morton; +Cc: James.Bottomley, linux-kernel, linux-scsi

On Thu, Feb 26, 2004 at 05:37:43PM -0800, Andrew Morton wrote:
> Andrew Morton <akpm@osdl.org> wrote:
> >
> > 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 =====
--- 1.96/drivers/scsi/hosts.c	Mon Dec 29 16:38:10 2003
+++ edited/drivers/scsi/hosts.c	Thu Feb 26 21:49:30 2004
@@ -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);
 }
 

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

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  2:32     ` Ben Collins
@ 2004-02-27  2:51       ` Andrew Morton
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2004-02-27  2:51 UTC (permalink / raw)
  To: Ben Collins; +Cc: James.Bottomley, linux-kernel

Ben Collins <bcollins@debian.org> wrote:
>
> On Thu, Feb 26, 2004 at 05:37:43PM -0800, Andrew Morton wrote:
> > Andrew Morton <akpm@osdl.org> wrote:
> > >
> > > 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.

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  1:37   ` Andrew Morton
                       ` (2 preceding siblings ...)
  2004-02-27  2:50     ` Ben Collins
@ 2004-02-27  3:04     ` Ben Collins
  2004-02-27 16:33       ` Mike Anderson
  3 siblings, 1 reply; 11+ messages in thread
From: Ben Collins @ 2004-02-27  3:04 UTC (permalink / raw)
  To: Andrew Morton; +Cc: James.Bottomley, linux-kernel, linux-scsi

Here's one that actually compiles.

===== drivers/scsi/hosts.c 1.96 vs edited =====
--- 1.96/drivers/scsi/hosts.c	Mon Dec 29 16:38:10 2003
+++ edited/drivers/scsi/hosts.c	Thu Feb 26 22:00:33 2004
@@ -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);
 }
 
-- 
Debian     - http://www.debian.org/
Linux 1394 - http://www.linux1394.org/
Subversion - http://subversion.tigris.org/
WatchGuard - http://www.watchguard.com/

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  3:04     ` Ben Collins
@ 2004-02-27 16:33       ` Mike Anderson
  2004-02-27 22:30         ` Andrew Morton
  0 siblings, 1 reply; 11+ messages in thread
From: Mike Anderson @ 2004-02-27 16:33 UTC (permalink / raw)
  To: Ben Collins; +Cc: Andrew Morton, James.Bottomley, linux-kernel, linux-scsi

Ben Collins [bcollins@debian.org] wrote:
>  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);
>  }
>  

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
andmike@us.ibm.com


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27  1:19 ` Andrew Morton
  2004-02-27  1:37   ` Andrew Morton
@ 2004-02-27 18:51   ` H. Peter Anvin
  1 sibling, 0 replies; 11+ messages in thread
From: H. Peter Anvin @ 2004-02-27 18:51 UTC (permalink / raw)
  To: linux-kernel

Followup to:  <20040226171928.750f5f6f.akpm@osdl.org>
By author:    Andrew Morton <akpm@osdl.org>
In newsgroup: linux.dev.kernel
> 
> 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


^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [BK PATCH] SCSI host num allocation improvement
  2004-02-27 16:33       ` Mike Anderson
@ 2004-02-27 22:30         ` Andrew Morton
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Morton @ 2004-02-27 22:30 UTC (permalink / raw)
  To: Mike Anderson; +Cc: bcollins, James.Bottomley, linux-kernel, linux-scsi

Mike Anderson <andmike@us.ibm.com> wrote:
>
> Ben Collins [bcollins@debian.org] wrote:
> >  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);
> >  }
> >  
> 
> 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.



diff -puN drivers/scsi/hosts.c~scsi-host-allocation-fix drivers/scsi/hosts.c
--- 25/drivers/scsi/hosts.c~scsi-host-allocation-fix	Fri Feb 27 14:11:47 2004
+++ 25-akpm/drivers/scsi/hosts.c	Fri Feb 27 14:29:01 2004
@@ -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);
 }
 

_


^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2004-02-27 22:28 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2004-02-26 23:54 [BK PATCH] SCSI host num allocation improvement Ben Collins
2004-02-27  1:19 ` Andrew Morton
2004-02-27  1:37   ` Andrew Morton
2004-02-27  2:09     ` Ben Collins
2004-02-27  2:32     ` Ben Collins
2004-02-27  2:51       ` Andrew Morton
2004-02-27  2:50     ` Ben Collins
2004-02-27  3:04     ` Ben Collins
2004-02-27 16:33       ` Mike Anderson
2004-02-27 22:30         ` Andrew Morton
2004-02-27 18:51   ` H. Peter Anvin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox