Mailing List Archive

2012/1/21 Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>:
> On Sat, Jan 21, 2012 at 12:15:26AM +0900, Akinobu Mita wrote:
>> Use bitmap_set and bitmap_clear rather than modifying individual bits
>> in a memory region.
>>
>> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
>> Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
>> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
>> Cc: xen-devel@lists.xensource.com
>> Cc: virtualization@lists.linux-foundation.org
>> ---
>>  drivers/block/xen-blkfront.c |    7 +++----
>>  1 files changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
>> index 2f22874..619868d 100644
>> --- a/drivers/block/xen-blkfront.c
>> +++ b/drivers/block/xen-blkfront.c
>> @@ -43,6 +43,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/mutex.h>
>>  #include <linux/scatterlist.h>
>> +#include <linux/bitmap.h>
>>
>>  #include <xen/xen.h>
>>  #include <xen/xenbus.h>
>> @@ -177,8 +178,7 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
>>
>>       spin_lock(&minor_lock);
>>       if (find_next_bit(minors, end, minor) >= end) {
>> -             for (; minor < end; ++minor)
>> -                     __set_bit(minor, minors);
>> +             bitmap_set(minors, minor, nr);
>
> Hm, I would have thought the last argument should have been 'end'?

'end' is the index of the last bit to clear. But the last argument of
bitmap_clear() is the number of bits to clear. So I think 'nr' is correct.

> Did you test this patch with a large amount of minors?

Sorry I didn't do runtime test.

>>               rc = 0;
>>       } else
>>               rc = -EBUSY;
>> @@ -193,8 +193,7 @@ static void xlbd_release_minors(unsigned int minor, unsigned int nr)
>>
>>       BUG_ON(end > nr_minors);
>>       spin_lock(&minor_lock);
>> -     for (; minor < end; ++minor)
>> -             __clear_bit(minor, minors);
>> +     bitmap_clear(minors,  minor, nr);
>
> Ditto.
>>       spin_unlock(&minor_lock);
>>  }
>>
>> --
>> 1.7.4.4
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/

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

On Sat, Jan 21, 2012 at 12:41:56AM +0900, Akinobu Mita wrote:
> 2012/1/21 Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>:
> > On Sat, Jan 21, 2012 at 12:15:26AM +0900, Akinobu Mita wrote:
> >> Use bitmap_set and bitmap_clear rather than modifying individual bits
> >> in a memory region.
> >>
> >> Signed-off-by: Akinobu Mita <akinobu.mita@gmail.com>
> >> Cc: Jeremy Fitzhardinge <jeremy.fitzhardinge@citrix.com>
> >> Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
> >> Cc: xen-devel@lists.xensource.com
> >> Cc: virtualization@lists.linux-foundation.org
> >> ---
> >>  drivers/block/xen-blkfront.c |    7 +++----
> >>  1 files changed, 3 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/drivers/block/xen-blkfront.c b/drivers/block/xen-blkfront.c
> >> index 2f22874..619868d 100644
> >> --- a/drivers/block/xen-blkfront.c
> >> +++ b/drivers/block/xen-blkfront.c
> >> @@ -43,6 +43,7 @@
> >>  #include <linux/slab.h>
> >>  #include <linux/mutex.h>
> >>  #include <linux/scatterlist.h>
> >> +#include <linux/bitmap.h>
> >>
> >>  #include <xen/xen.h>
> >>  #include <xen/xenbus.h>
> >> @@ -177,8 +178,7 @@ static int xlbd_reserve_minors(unsigned int minor, unsigned int nr)
> >>
> >>       spin_lock(&minor_lock);
> >>       if (find_next_bit(minors, end, minor) >= end) {
> >> -             for (; minor < end; ++minor)
> >> -                     __set_bit(minor, minors);
> >> +             bitmap_set(minors, minor, nr);
> >
> > Hm, I would have thought the last argument should have been 'end'?
>
> 'end' is the index of the last bit to clear. But the last argument of
> bitmap_clear() is the number of bits to clear. So I think 'nr' is correct.

Ah, I see it.
>
> > Did you test this patch with a large amount of minors?
>
> Sorry I didn't do runtime test.

Please do.

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

On Fri, 20 Jan 2012 11:09:38 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> >
> > > Did you test this patch with a large amount of minors?
> >
> > Sorry I didn't do runtime test.
>
> Please do.

The poor guy probably doesn't know how to test it and surely it would
be quite a lot of work for him to do so.

Overall, it would be much more efficient if the tester of this code is
someone who is set up to easily apply the patch and test it. ie: the
code maintainer(s).


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

On Fri, Jan 20, 2012 at 03:11:49PM -0800, Andrew Morton wrote:
> On Fri, 20 Jan 2012 11:09:38 -0500
> Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
>
> > >
> > > > Did you test this patch with a large amount of minors?
> > >
> > > Sorry I didn't do runtime test.
> >
> > Please do.
>
> The poor guy probably doesn't know how to test it and surely it would
> be quite a lot of work for him to do so.
>
> Overall, it would be much more efficient if the tester of this code is
> someone who is set up to easily apply the patch and test it. ie: the
> code maintainer(s).

<grins>

This patch aside - Andrew how do you deal with a large amount of patches
and make sure they are tested? Is there a weekly Test Tuesday where you
kick off your automated tests and dilligently look for any variations?

Or is it more of compile the kernel with X features and run it for a week
doing normal (and abnormal!) things to see if it falls over?

Thanks!

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

On Fri, 20 Jan 2012 18:55:08 -0500
Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:

> On Fri, Jan 20, 2012 at 03:11:49PM -0800, Andrew Morton wrote:
> > On Fri, 20 Jan 2012 11:09:38 -0500
> > Konrad Rzeszutek Wilk <konrad.wilk@oracle.com> wrote:
> >
> > > >
> > > > > Did you test this patch with a large amount of minors?
> > > >
> > > > Sorry I didn't do runtime test.
> > >
> > > Please do.
> >
> > The poor guy probably doesn't know how to test it and surely it would
> > be quite a lot of work for him to do so.
> >
> > Overall, it would be much more efficient if the tester of this code is
> > someone who is set up to easily apply the patch and test it. ie: the
> > code maintainer(s).
>
> <grins>
>
> This patch aside - Andrew how do you deal with a large amount of patches
> and make sure they are tested?

Not very well :(

> Is there a weekly Test Tuesday where you
> kick off your automated tests and dilligently look for any variations?
> Or is it more of compile the kernel with X features and run it for a week
> doing normal (and abnormal!) things to see if it falls over?

I build all the patches I have along with all of linux-next and boot
the thing, then use the resulting kernel for a few hours compilation
testing, then shove it all into linux-next where I naively hope that
someone else is testing things.

The coverage which is obtained this way is pretty poor.

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