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

[Daniel Stodden <daniel.stodden@xxxxxxxxxx>]

On Thu, 2011-02-24 at 11:40 -0500, Jan Beulich wrote:
> >>> On 23.02.11 at 11:38, "Jan Beulich" <JBeulich@xxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxx
http://lists.xensource.com/xen-devel