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

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


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

>>> 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


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

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

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

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


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

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

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

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


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

>>> 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


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

>>> 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


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

>>> 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


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

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


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

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


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