[PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Laszlo Ersek]

[-- Attachment #1: Type: text/plain, Size: 355 bytes --]

Hi,

should anybody still use the blktap(1) driver in linux-2.6.18-xen, the 
following patch intends to make the maximum number of tapdevs 
configurable at module insertion time. The number is clamped to [256 .. 
NR_EVENT_CHANNELS]. I removed the definition of MAX_DEV_NAME because it 
didn't seem to be used at all.

Thanks for considering,
Laszlo Ersek

[-- Attachment #2: upstream.patch --]
[-- Type: text/plain, Size: 4386 bytes --]

diff -r 2994d2997f9d drivers/xen/blktap/blktap.c
--- a/drivers/xen/blktap/blktap.c	Thu Feb 10 13:59:02 2011 +0000
+++ b/drivers/xen/blktap/blktap.c	Tue Feb 22 14:23:22 2011 +0100
@@ -55,8 +55,7 @@
 #include <linux/delay.h>
 #include <asm/tlbflush.h>
 
-#define MAX_TAP_DEV 256     /*the maximum number of tapdisk ring devices    */
-#define MAX_DEV_NAME 100    /*the max tapdisk ring device name e.g. blktap0 */
+#define DEF_MAX_TAP_DEV 256 /* default maximum of tapdisk ring devices */
 
 /*
  * The maximum number of requests that can be outstanding at any time
@@ -120,7 +119,8 @@
 	struct vm_foreign_map foreign_map;    /*Mapping page */
 } tap_blkif_t;
 
-static struct tap_blkif *tapfds[MAX_TAP_DEV];
+/* Points to the first of "max_tap_dev" objects. */
+static struct tap_blkif **tapfds;
 static int blktap_next_minor;
 
 /* Run-time switchable: /sys/module/blktap/parameters/ */
@@ -129,6 +129,12 @@
 module_param(log_stats, int, 0644);
 module_param(debug_lvl, int, 0644);
 
+/* Settable only at initialization time. */
+static int max_tap_dev = DEF_MAX_TAP_DEV;
+module_param(max_tap_dev, int, 0444);
+MODULE_PARM_DESC(max_tap_dev, "maximum number of tapdisk devices, defaults to "
+		 __stringify(DEF_MAX_TAP_DEV));
+
 /*
  * Each outstanding request that we've passed to the lower device layers has a 
  * 'pending_req' allocated to it. Each buffer_head that completes decrements 
@@ -470,7 +476,7 @@
 	 * available.  This is done while we are still under
 	 * the protection of the pending_free_lock.
 	 */
-	if (blktap_next_minor < MAX_TAP_DEV)
+	if (blktap_next_minor < max_tap_dev)
 		minor = blktap_next_minor++;
 found:
 	spin_unlock_irq(&pending_free_lock);
@@ -538,7 +544,7 @@
 	 * if the userland tools set things up wrong, this could be negative;
 	 * just don't try to signal in this case
 	 */
-	if (idx < 0 || idx >= MAX_TAP_DEV)
+	if (idx < 0 || idx >= max_tap_dev)
 		return;
 
 	info = tapfds[idx];
@@ -567,7 +573,7 @@
 	/* ctrl device, treat differently */
 	if (!idx)
 		return 0;
-	if (idx < 0 || idx >= MAX_TAP_DEV) {
+	if (idx < 0 || idx >= max_tap_dev) {
 		WPRINTK("No device /dev/xen/blktap%d\n", idx);
 		return -ENODEV;
 	}
@@ -836,7 +842,7 @@
 		unsigned long dev = arg;
 		unsigned long flags;
 
-		if (info || dev >= MAX_TAP_DEV)
+		if (info || dev >= max_tap_dev)
 			return -EINVAL;
 
 		info = tapfds[dev];
@@ -854,7 +860,7 @@
 		if (!info) {
 			unsigned long dev = arg;
 
-			if (dev >= MAX_TAP_DEV)
+			if (dev >= max_tap_dev)
 				return -EINVAL;
 
 			info = tapfds[dev];
@@ -895,7 +901,7 @@
 {
 	tap_blkif_t *info;
 
-	if (idx < 0 || idx >= MAX_TAP_DEV)
+	if (idx < 0 || idx >= max_tap_dev)
 		return;
 
 	info = tapfds[idx];
@@ -1045,7 +1051,7 @@
 	struct mm_struct *mm;
 	
 
-	if ((tapidx < 0) || (tapidx >= MAX_TAP_DEV)
+	if ((tapidx < 0) || (tapidx >= max_tap_dev)
 	    || !(info = tapfds[tapidx])) {
 		WPRINTK("fast_flush: Couldn't get info!\n");
 		return;
@@ -1298,7 +1304,7 @@
 	rmb(); /* Ensure we see queued requests up to 'rp'. */
 
 	/*Check blkif has corresponding UE ring*/
-	if (blkif->dev_num < 0 || blkif->dev_num >= MAX_TAP_DEV) {
+	if (blkif->dev_num < 0 || blkif->dev_num >= max_tap_dev) {
 		/*oops*/
 		if (print_dbug) {
 			WPRINTK("Corresponding UE " 
@@ -1413,7 +1419,7 @@
 	struct mm_struct *mm;
 	struct vm_area_struct *vma = NULL;
 
-	if (blkif->dev_num < 0 || blkif->dev_num >= MAX_TAP_DEV)
+	if (blkif->dev_num < 0 || blkif->dev_num >= max_tap_dev)
 		goto fail_response;
 
 	info = tapfds[blkif->dev_num];
@@ -1696,6 +1702,21 @@
 		notify_remote_via_irq(blkif->irq);
 }
 
+static int alloc_tapfds(void)
+{
+	int new = clamp_t(int, max_tap_dev, DEF_MAX_TAP_DEV,
+			  NR_EVENT_CHANNELS);
+
+	if (new != max_tap_dev) {
+		WPRINTK("clamping max_tap_dev from %d to %d\n", max_tap_dev,
+			new);
+		max_tap_dev = new;
+	}
+
+	tapfds = kmalloc(max_tap_dev * sizeof(*tapfds), GFP_KERNEL);
+	return tapfds ? 0 : -ENOMEM;
+}
+
 static int __init blkif_init(void)
 {
 	int i, ret;
@@ -1704,6 +1725,16 @@
 	if (!is_running_on_xen())
 		return -ENODEV;
 
+	ret = alloc_tapfds();
+	if (ret != 0) {
+		WPRINTK("failed to allocate tapfds\n");
+		return ret;
+	}
+	/* Not released on error paths below, because the failure of
+	 * register_chrdev() leaks whatever was allocated by the req_increase()
+	 * calls anyway.
+	 */
+
 	INIT_LIST_HEAD(&pending_free);
         for(i = 0; i < 2; i++) {
 		ret = req_increase();

[-- Attachment #3: Type: text/plain, Size: 138 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Jan Beulich]

>>> On 22.02.11 at 15:20, Laszlo Ersek <lersek@redhat.com> wrote:
> Hi,
> 
> should anybody still use the blktap(1) driver in linux-2.6.18-xen, the 
> following patch intends to make the maximum number of tapdevs 
> configurable at module insertion time. The number is clamped to [256 .. 
> NR_EVENT_CHANNELS]. I removed the definition of MAX_DEV_NAME because it 
> didn't seem to be used at all.
> 
> Thanks for considering,
> Laszlo Ersek

Without replacing the call to register_chrdev() with one to
__register_chrdev() (available only with 2.6.32 and newer) I
can't see how you would get beyond 256 devices with the
changes you propose.

Jan

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Laszlo Ersek]

On 02/22/11 16:49, Jan Beulich wrote:
>>>> On 22.02.11 at 15:20, Laszlo Ersek<lersek@redhat.com>  wrote:
>> Hi,
>>
>> should anybody still use the blktap(1) driver in linux-2.6.18-xen, the
>> following patch intends to make the maximum number of tapdevs
>> configurable at module insertion time. The number is clamped to [256 ..
>> NR_EVENT_CHANNELS]. I removed the definition of MAX_DEV_NAME because it
>> didn't seem to be used at all.
>>
>> Thanks for considering,
>> Laszlo Ersek
>
> Without replacing the call to register_chrdev() with one to
> __register_chrdev() (available only with 2.6.32 and newer) I
> can't see how you would get beyond 256 devices with the
> changes you propose.

Oops, sorry; I naively assumed that minor device numbers were already 
covered by an earlier change.

I figure register_chrdev() could be reimplemented in blktap, based on 
lower-level char_dev.c (and kobject) primitives, but I'm not sure if the 
original goal is worth that ugliness. In any case, should I bother 
posting a version like that eventually, or would it have no chance of 
being accepted?

Thanks & sorry for the noise.
lacos

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Daniel Stodden]

On Tue, 2011-02-22 at 12:34 -0500, Laszlo Ersek wrote:
> On 02/22/11 16:49, Jan Beulich wrote:
> >>>> On 22.02.11 at 15:20, Laszlo Ersek<lersek@redhat.com>  wrote:
> >> Hi,
> >>
> >> should anybody still use the blktap(1) driver in linux-2.6.18-xen, the
> >> following patch intends to make the maximum number of tapdevs
> >> configurable at module insertion time. The number is clamped to [256 ..
> >> NR_EVENT_CHANNELS]. I removed the definition of MAX_DEV_NAME because it
> >> didn't seem to be used at all.
> >>
> >> Thanks for considering,
> >> Laszlo Ersek
> >
> > Without replacing the call to register_chrdev() with one to
> > __register_chrdev() (available only with 2.6.32 and newer) I
> > can't see how you would get beyond 256 devices with the
> > changes you propose.
> 
> Oops, sorry; I naively assumed that minor device numbers were already 
> covered by an earlier change.
> 
> I figure register_chrdev() could be reimplemented in blktap, based on 
> lower-level char_dev.c (and kobject) primitives, but I'm not sure if the 
> original goal is worth that ugliness. In any case, should I bother 
> posting a version like that eventually, or would it have no chance of 
> being accepted?

I'm pretty sure minors > 256 date way before 2.6.32. Here's the module
init fragment from blktap2, replacing the register_chrdev() call:

int __init
blktap_ring_init(void)
{
	dev_t dev = 0;
	int err;

	cdev_init(&blktap_ring_cdev, &blktap_ring_file_operations);
	blktap_ring_cdev.owner = THIS_MODULE;

	err = alloc_chrdev_region(&dev, 0, MAX_BLKTAP_DEVICE, "blktap2");
	if (err < 0) {
		BTERR("error registering ring devices: %d\n", err);
		return err;
	}

	err = cdev_add(&blktap_ring_cdev, dev, MAX_BLKTAP_DEVICE);
	if (err) {
		BTERR("error adding ring device: %d\n", err);
		unregister_chrdev_region(dev, MAX_BLKTAP_DEVICE);
		return err;
	}

	blktap_ring_major = MAJOR(dev);
	BTINFO("blktap ring major: %d\n", blktap_ring_major);

	return 0;
}

void
blktap_ring_exit(void)
{
	if (!blktap_ring_major)
		return;

	cdev_del(&blktap_ring_cdev);
	unregister_chrdev_region(MKDEV(blktap_ring_major, 0),
				 MAX_BLKTAP_DEVICE);

	blktap_ring_major = 0;
}

Happy hacking.

Daniel

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Laszlo Ersek]

On 02/22/11 18:44, Daniel Stodden wrote:

> I'm pretty sure minors>  256 date way before 2.6.32. Here's the module
> init fragment from blktap2, replacing the register_chrdev() call:
> 
> [...]

That's about what I was thinking of in [0]. It also sets the cdev's owner manually. However, register_chrdev() also does this:

   225		kobject_set_name(&cdev->kobj, "%s", name);
   226		for (s = strchr(kobject_name(&cdev->kobj),'/'); s; s = strchr(s, '/'))
   227			*s = '!';

I reckon we can ignore the 's,/,!,g' replacement, since the name is fixed "blktap" (or "blktap2"). But the kobject name doesn't appear to be set in the first place. Is that no problem? If not, I'd just omit it from blktap as well.

Thank you!
lacos

[0] https://bugzilla.redhat.com/show_bug.cgi?id=452650#c21

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Daniel Stodden]

On Tue, 2011-02-22 at 13:08 -0500, Laszlo Ersek wrote:
> On 02/22/11 18:44, Daniel Stodden wrote:
> 
> > I'm pretty sure minors>  256 date way before 2.6.32. Here's the module
> > init fragment from blktap2, replacing the register_chrdev() call:
> > 
> > [...]
> 
> That's about what I was thinking of in [0]. It also sets the cdev's owner manually. However, register_chrdev() also does this:
> 
>    225		kobject_set_name(&cdev->kobj, "%s", name);
>    226		for (s = strchr(kobject_name(&cdev->kobj),'/'); s; s = strchr(s, '/'))
>    227			*s = '!';
> 
> I reckon we can ignore the 's,/,!,g' replacement, since the name is fixed "blktap" (or "blktap2"). But the kobject name doesn't appear to be set in the first place. 

> Is that no problem? If not, I'd just omit it from blktap as well.

I remember I browsed a whole lot of code back then when I wrote that.
All gone now :)

After looking again, I think the way this works was that the kobj map
holds the blktap2 class device, nowhere a cdev->kobj, after
device_register (blktap2/sysfs.c) actually placed a node for that minor.
Before then, only the range is allocated, but won't show up in sysfs. So
I guess cdev->kobj is a somewhat anonymous refcount bag only.

# l /sys/dev/char/* | grep blktap
lrwxrwxrwx 1 root root 0 Feb 22 10:24 /sys/dev/char/10:61 -> ../../class/misc/blktap-control
lrwxrwxrwx 1 root root 0 Feb 22 10:24 /sys/dev/char/254:0 -> ../../class/blktap2/blktap0
lrwxrwxrwx 1 root root 0 Feb 22 10:24 /sys/dev/char/254:1 -> ../../class/blktap2/blktap1
lrwxrwxrwx 1 root root 0 Feb 22 10:24 /sys/dev/char/254:2 -> ../../class/blktap2/blktap2

Compare this to the number of tty mappings you'll probably find on your
box in the same directory.

Daniel

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Jan Beulich]

>>> On 22.02.11 at 18:44, Daniel Stodden <daniel.stodden@citrix.com> wrote:
> On Tue, 2011-02-22 at 12:34 -0500, Laszlo Ersek wrote:
>> On 02/22/11 16:49, Jan Beulich wrote:
>> >>>> On 22.02.11 at 15:20, Laszlo Ersek<lersek@redhat.com>  wrote:
>> >> Hi,
>> >>
>> >> should anybody still use the blktap(1) driver in linux-2.6.18-xen, the
>> >> following patch intends to make the maximum number of tapdevs
>> >> configurable at module insertion time. The number is clamped to [256 ..
>> >> NR_EVENT_CHANNELS]. I removed the definition of MAX_DEV_NAME because it
>> >> didn't seem to be used at all.
>> >>
>> >> Thanks for considering,
>> >> Laszlo Ersek
>> >
>> > Without replacing the call to register_chrdev() with one to
>> > __register_chrdev() (available only with 2.6.32 and newer) I
>> > can't see how you would get beyond 256 devices with the
>> > changes you propose.
>> 
>> Oops, sorry; I naively assumed that minor device numbers were already 
>> covered by an earlier change.
>> 
>> I figure register_chrdev() could be reimplemented in blktap, based on 
>> lower-level char_dev.c (and kobject) primitives, but I'm not sure if the 
>> original goal is worth that ugliness. In any case, should I bother 
>> posting a version like that eventually, or would it have no chance of 
>> being accepted?
> 
> I'm pretty sure minors > 256 date way before 2.6.32. Here's the module
> init fragment from blktap2, replacing the register_chrdev() call:

Sure, just that you have do more things "manually".

> int __init
> blktap_ring_init(void)
> {
> 	dev_t dev = 0;
> 	int err;
> 
> 	cdev_init(&blktap_ring_cdev, &blktap_ring_file_operations);
> 	blktap_ring_cdev.owner = THIS_MODULE;
> 
> 	err = alloc_chrdev_region(&dev, 0, MAX_BLKTAP_DEVICE, "blktap2");
> 	if (err < 0) {
> 		BTERR("error registering ring devices: %d\n", err);
> 		return err;
> 	}
> 
> 	err = cdev_add(&blktap_ring_cdev, dev, MAX_BLKTAP_DEVICE);
> 	if (err) {
> 		BTERR("error adding ring device: %d\n", err);
> 		unregister_chrdev_region(dev, MAX_BLKTAP_DEVICE);
> 		return err;
> 	}
> 
> 	blktap_ring_major = MAJOR(dev);
> 	BTINFO("blktap ring major: %d\n", blktap_ring_major);
> 
> 	return 0;
> }

Any reason why in .32 and newer you still don't use
__register_chrdev() (and __unregister_chrdev())? And even
if this still needs to be this way, I note that unregister_chrdev()
calls __unregister_chrdev_region() before cdev_del(), while
blktap_ring_exit() does it the other way around?

Jan

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Jan Beulich]

>>> On 23.02.11 at 11:38, "Jan Beulich" <JBeulich@novell.com> wrote:
> Any reason why in .32 and newer you still don't use
> __register_chrdev() (and __unregister_chrdev())? And even
> if this still needs to be this way, I note that unregister_chrdev()
> calls __unregister_chrdev_region() before cdev_del(), while
> blktap_ring_exit() does it the other way around?

I think this could be cleaned up like this:

--- a/drivers/xen/blktap/ring.c
+++ b/drivers/xen/blktap/ring.c
@@ -8,7 +8,6 @@
 #include "blktap.h"
 
 int blktap_ring_major;
-static struct cdev blktap_ring_cdev;
 
  /* 
   * BLKTAP - immediately before the mmap area,
@@ -511,26 +510,16 @@ blktap_ring_debug(struct blktap *tap, ch
 int __init
 blktap_ring_init(void)
 {
-	dev_t dev = 0;
 	int err;
 
-	cdev_init(&blktap_ring_cdev, &blktap_ring_file_operations);
-	blktap_ring_cdev.owner = THIS_MODULE;
-
-	err = alloc_chrdev_region(&dev, 0, MAX_BLKTAP_DEVICE, "blktap2");
+	err = __register_chrdev(0, 0, MAX_BLKTAP_DEVICE, "blktap2",
+				&blktap_ring_file_operations);
 	if (err < 0) {
 		BTERR("error registering ring devices: %d\n", err);
 		return err;
 	}
 
-	err = cdev_add(&blktap_ring_cdev, dev, MAX_BLKTAP_DEVICE);
-	if (err) {
-		BTERR("error adding ring device: %d\n", err);
-		unregister_chrdev_region(dev, MAX_BLKTAP_DEVICE);
-		return err;
-	}
-
-	blktap_ring_major = MAJOR(dev);
+	blktap_ring_major = err;
 	BTINFO("blktap ring major: %d\n", blktap_ring_major);
 
 	return 0;
@@ -542,9 +531,8 @@ blktap_ring_exit(void)
 	if (!blktap_ring_major)
 		return;
 
-	cdev_del(&blktap_ring_cdev);
-	unregister_chrdev_region(MKDEV(blktap_ring_major, 0),
-				 MAX_BLKTAP_DEVICE);
+	__unregister_chrdev(blktap_ring_major, 0, MAX_BLKTAP_DEVICE,
+			    "blktap2");
 
 	blktap_ring_major = 0;
 }

And probably converting MAX_BLKTAP_DEVICE into a configure
option would also be reasonable.

In any case, if I wanted to formally submit patches to clean up
this and some other things in the pv-ops variant, what (preferably
non-topic) branch should those be against? If I'm not mistaken,
xen/next-2.6.38 for example doesn't even have a blktap - or did
it get moved out of drivers/xen/? And what would be the most
up-to-date non-experimental branch to pull blktap bits from?

Jan

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Daniel Stodden]

On Thu, 2011-02-24 at 11:40 -0500, Jan Beulich wrote:
> >>> On 23.02.11 at 11:38, "Jan Beulich" <JBeulich@novell.com> wrote:
> > Any reason why in .32 and newer you still don't use
> > __register_chrdev() (and __unregister_chrdev())? And even
> > if this still needs to be this way, I note that unregister_chrdev()
> > calls __unregister_chrdev_region() before cdev_del(), while
> > blktap_ring_exit() does it the other way around?

I wrote this for .27 iirc and haven't looked into char_dev since then.
I'm always for prettification.

> I think this could be cleaned up like this:
> 
> --- a/drivers/xen/blktap/ring.c
> +++ b/drivers/xen/blktap/ring.c
> @@ -8,7 +8,6 @@
>  #include "blktap.h"
>  
>  int blktap_ring_major;
> -static struct cdev blktap_ring_cdev;
>  
>   /* 
>    * BLKTAP - immediately before the mmap area,
> @@ -511,26 +510,16 @@ blktap_ring_debug(struct blktap *tap, ch
>  int __init
>  blktap_ring_init(void)
>  {
> -	dev_t dev = 0;
>  	int err;
>  
> -	cdev_init(&blktap_ring_cdev, &blktap_ring_file_operations);
> -	blktap_ring_cdev.owner = THIS_MODULE;
> -
> -	err = alloc_chrdev_region(&dev, 0, MAX_BLKTAP_DEVICE, "blktap2");
> +	err = __register_chrdev(0, 0, MAX_BLKTAP_DEVICE, "blktap2",
> +				&blktap_ring_file_operations);
>  	if (err < 0) {
>  		BTERR("error registering ring devices: %d\n", err);
>  		return err;
>  	}
>  
> -	err = cdev_add(&blktap_ring_cdev, dev, MAX_BLKTAP_DEVICE);
> -	if (err) {
> -		BTERR("error adding ring device: %d\n", err);
> -		unregister_chrdev_region(dev, MAX_BLKTAP_DEVICE);
> -		return err;
> -	}
> -
> -	blktap_ring_major = MAJOR(dev);
> +	blktap_ring_major = err;
>  	BTINFO("blktap ring major: %d\n", blktap_ring_major);
>  
>  	return 0;
> @@ -542,9 +531,8 @@ blktap_ring_exit(void)
>  	if (!blktap_ring_major)
>  		return;
>  
> -	cdev_del(&blktap_ring_cdev);
> -	unregister_chrdev_region(MKDEV(blktap_ring_major, 0),
> -				 MAX_BLKTAP_DEVICE);
> +	__unregister_chrdev(blktap_ring_major, 0, MAX_BLKTAP_DEVICE,
> +			    "blktap2");
>  
>  	blktap_ring_major = 0;
>  }
> 
> And probably converting MAX_BLKTAP_DEVICE into a configure
> option would also be reasonable.

If you could confirm that __register_chrdev doesn't grow O(n) in space
anywhere (I guess it doesn't), then I don't think that people should be
required to spend much thought on it.

It just to matter because the the *blktaps[minor] vector was statically
allocated. Nowadays it grows during ALLOC_TAP, base-2.

We can set it to either something insanely large, provided toolstacks
don't shoot themselves by running tap-ctl allocate in a loop.

Or keep it large enough so few would ever have to care (I thought 1024
would be just that), and add a sysfs node to override that limit.

> In any case, if I wanted to formally submit patches to clean up
> this and some other things in the pv-ops variant, what (preferably
> non-topic) branch should those be against? If I'm not mistaken,
> xen/next-2.6.38 for example doesn't even have a blktap - or did
> it get moved out of drivers/xen/? 

I've got a patch for .38 mostly ready. It's going to move into
drivers/block/blktap.

Plus and some thoughts on logical block size, barrier support and some
early trim stuff.

> And what would be the most
> up-to-date non-experimental branch to pull blktap bits from?

That'd be my tree on xenbits after I pushed some more stuff up there.
Which apparently is gone. Anyone knowing what's going on there? Did I
miss some organizational change or is it just broken?

Used to be
http://xenbits.xensource.com/gitweb/?p=people/dstodden/linux.git/.git;a=summary

Daniel

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Daniel Stodden]

On Thu, 2011-02-24 at 11:40 -0500, Jan Beulich wrote:

>   /* 
>    * BLKTAP - immediately before the mmap area,
> @@ -511,26 +510,16 @@ blktap_ring_debug(struct blktap *tap, ch
>  int __init
>  blktap_ring_init(void)
>  {
> -	dev_t dev = 0;
>  	int err;
>  
> -	cdev_init(&blktap_ring_cdev, &blktap_ring_file_operations);
> -	blktap_ring_cdev.owner = THIS_MODULE;
> -
> -	err = alloc_chrdev_region(&dev, 0, MAX_BLKTAP_DEVICE, "blktap2");
> +	err = __register_chrdev(0, 0, MAX_BLKTAP_DEVICE, "blktap2",
> +				&blktap_ring_file_operations);
>  	if (err < 0) {
>  		BTERR("error registering ring devices: %d\n", err);
>  		return err;
>  	}
>  
> -	err = cdev_add(&blktap_ring_cdev, dev, MAX_BLKTAP_DEVICE);
> -	if (err) {
> -		BTERR("error adding ring device: %d\n", err);
> -		unregister_chrdev_region(dev, MAX_BLKTAP_DEVICE);
> -		return err;
> -	}
> -
> -	blktap_ring_major = MAJOR(dev);
> +	blktap_ring_major = err;
>  	BTINFO("blktap ring major: %d\n", blktap_ring_major);

While you are at it: Feel free to drop init message(s?), too. Blktap is
not boot critical, and there are plenty alternative places to check
presence.

Cheers,
Daniel

Re: [PATCH linux-2.6.18-xen] blktap: make max # of tap devices a module parameter

[Jan Beulich]

>>> On 22.02.11 at 18:34, Laszlo Ersek <lersek@redhat.com> wrote:
> On 02/22/11 16:49, Jan Beulich wrote:
>>>>> On 22.02.11 at 15:20, Laszlo Ersek<lersek@redhat.com>  wrote:
>>> Hi,
>>>
>>> should anybody still use the blktap(1) driver in linux-2.6.18-xen, the
>>> following patch intends to make the maximum number of tapdevs
>>> configurable at module insertion time. The number is clamped to [256 ..
>>> NR_EVENT_CHANNELS]. I removed the definition of MAX_DEV_NAME because it
>>> didn't seem to be used at all.
>>>
>>> Thanks for considering,
>>> Laszlo Ersek
>>
>> Without replacing the call to register_chrdev() with one to
>> __register_chrdev() (available only with 2.6.32 and newer) I
>> can't see how you would get beyond 256 devices with the
>> changes you propose.
> 
> Oops, sorry; I naively assumed that minor device numbers were already 
> covered by an earlier change.
> 
> I figure register_chrdev() could be reimplemented in blktap, based on 
> lower-level char_dev.c (and kobject) primitives, but I'm not sure if the 
> original goal is worth that ugliness. In any case, should I bother 
> posting a version like that eventually, or would it have no chance of 
> being accepted?

I too had thought about doing this in the past, but decided there#s
not much point in it.

Jan