Re: [BK PATCH] SCSI host num allocation improvement

Re: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

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/

Re: [BK PATCH] SCSI host num allocation improvement

[Ben Collins]

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/

Re: [BK PATCH] SCSI host num allocation improvement

[Mike Anderson]

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

Re: [BK PATCH] SCSI host num allocation improvement

[Andrew Morton]

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);
 }
 

_