[2.6.22 PATCH 22/26] dm: bio list helpers

[2.6.22 PATCH 22/26] dm: bio list helpers

[Alasdair G Kergon]

From: Heinz Mauelshagen <hjm@redhat.com>

More bio_list helper functions for new targets (including dm-delay and
dm-loop) to manipulate lists of bios.

Signed-off-by: Heinz Mauelshagen <hjm@redhat.com>
Signed-off-by: Bryn Reeves <breeves@redhat.com>
Signed-off-by: Milan Broz <mbroz@redhat.com>
Signed-off-by: Alasdair G Kergon <agk@redhat.com>
---

 drivers/md/dm-bio-list.h |   26 ++++++++++++++++++++++++++
 1 files changed, 26 insertions(+)

Index: linux-2.6.21/drivers/md/dm-bio-list.h
===================================================================
--- linux-2.6.21.orig/drivers/md/dm-bio-list.h	2007-05-01 17:38:31.000000000 +0100
+++ linux-2.6.21/drivers/md/dm-bio-list.h	2007-05-01 17:40:55.000000000 +0100
@@ -8,17 +8,43 @@
 #define DM_BIO_LIST_H
 
 #include <linux/bio.h>
+#include <linux/prefetch.h>
 
 struct bio_list {
 	struct bio *head;
 	struct bio *tail;
 };
 
+static inline int bio_list_empty(const struct bio_list *bl)
+{
+	return bl->head == NULL;
+}
+
+#define BIO_LIST_INIT { .head = NULL, .tail = NULL }
+
+#define BIO_LIST(bl) \
+	struct bio_list bl = BIO_LIST_INIT
+
 static inline void bio_list_init(struct bio_list *bl)
 {
 	bl->head = bl->tail = NULL;
 }
 
+#define bio_list_for_each(bio, bl) \
+	for (bio = (bl)->head; bio && ({ prefetch(bio->bi_next); 1; }); \
+	     bio = bio->bi_next)
+
+static inline unsigned bio_list_size(const struct bio_list *bl)
+{
+	unsigned sz = 0;
+	struct bio *bio;
+
+	bio_list_for_each(bio, bl)
+		sz++;
+
+	return sz;
+}
+
 static inline void bio_list_add(struct bio_list *bl, struct bio *bio)
 {
 	bio->bi_next = NULL;

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Andrew Morton]

On Tue, 8 May 2007 20:48:45 +0100
Alasdair G Kergon <agk@redhat.com> wrote:

> +#define BIO_LIST_INIT { .head = NULL, .tail = NULL }
> +
> +#define BIO_LIST(bl) \
> +	struct bio_list bl = BIO_LIST_INIT

BIO_LIST is a strange name for something which initialises storage.

>  static inline void bio_list_init(struct bio_list *bl)
>  {
>  	bl->head = bl->tail = NULL;
>  }
>  
> +#define bio_list_for_each(bio, bl) \
> +	for (bio = (bl)->head; bio && ({ prefetch(bio->bi_next); 1; }); \
> +	     bio = bio->bi_next)
> +

I have this vague memory from a long time ago that one particular CPU type
wants to go oops when prefetching from an invalid address.

Maybe that went away for some reason - we'd have hit it again if it was
real.

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Jens Axboe]

On Tue, May 08 2007, Andrew Morton wrote:
> >  static inline void bio_list_init(struct bio_list *bl)
> >  {
> >  	bl->head = bl->tail = NULL;
> >  }
> >  
> > +#define bio_list_for_each(bio, bl) \
> > +	for (bio = (bl)->head; bio && ({ prefetch(bio->bi_next); 1; }); \
> > +	     bio = bio->bi_next)
> > +
> 
> I have this vague memory from a long time ago that one particular CPU type
> wants to go oops when prefetching from an invalid address.
> 
> Maybe that went away for some reason - we'd have hit it again if it was
> real.

Besides, manual prefetching is very rarely a win. I dabbled with some
benchmarks a few weeks back (with the doubly linked lists), and in most
cases it was actually a loss. So I'd vote for just removing the
prefetch() above.

-- 
Jens Axboe

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Alasdair G Kergon]

On Tue, May 08, 2007 at 05:41:59PM -0700, Andrew Morton wrote:
> On Tue, 8 May 2007 20:48:45 +0100
> Alasdair G Kergon <agk@redhat.com> wrote:
> > +#define BIO_LIST(bl) \
> > +	struct bio_list bl = BIO_LIST_INIT
 
> BIO_LIST is a strange name for something which initialises storage.
 
Those came by analogy with list.h, with list_head -> bio_list:

  #define LIST_HEAD(name) \
        struct list_head name = LIST_HEAD_INIT(name)

We could use something like INIT_BIO_LIST(bl) for that, but perhaps it would be
more readable always written out in full?
   struct bio_list bl = BIO_LIST_INIT;

Alasdair
-- 
agk@redhat.com

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Alasdair G Kergon]

On Wed, May 09, 2007 at 08:49:23AM +0200, Jens Axboe wrote:
> Besides, manual prefetching is very rarely a win. I dabbled with some
> benchmarks a few weeks back (with the doubly linked lists), and in most
> cases it was actually a loss. So I'd vote for just removing the
> prefetch() above.

OK.
 
Alasdair
-- 
agk@redhat.com

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Andi Kleen]

Andrew Morton <akpm@linux-foundation.org> writes:
> 
> I have this vague memory from a long time ago that one particular CPU type
> wants to go oops when prefetching from an invalid address.

AMD K7/K8, but we got a generic workaround for a long time.

-Andi

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Jan Engelhardt]

On May 8 2007 17:41, Andrew Morton wrote:
>On Tue, 8 May 2007 20:48:45 +0100
>Alasdair G Kergon <agk@redhat.com> wrote:
>
>> +#define BIO_LIST_INIT { .head = NULL, .tail = NULL }
>> +
>> +#define BIO_LIST(bl) \
>> +	struct bio_list bl = BIO_LIST_INIT
>
>BIO_LIST is a strange name for something which initialises storage.

So try DEFINE_BIO_LIST, it goes in line with DEFINE_SPINLOCK for example.


	Jan
-- 

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Jan Engelhardt]

On May 9 2007 08:49, Jens Axboe wrote:
>On Tue, May 08 2007, Andrew Morton wrote:
>> > +#define bio_list_for_each(bio, bl) \
>> > +	for (bio = (bl)->head; bio && ({ prefetch(bio->bi_next); 1; }); \
>> > +	     bio = bio->bi_next)
>> > +
>
>Besides, manual prefetching is very rarely a win. I dabbled with some
>benchmarks a few weeks back (with the doubly linked lists), and in most
>cases it was actually a loss. So I'd vote for just removing the
>prefetch() above.

So is the prefetching in the basic ADTs (e.g. linux/list.h) a loss too?



	Jan
-- 

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Alan Cox]

On Thu, 10 May 2007 16:17:57 +0200 (MEST)
Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:

> 
> On May 9 2007 08:49, Jens Axboe wrote:
> >On Tue, May 08 2007, Andrew Morton wrote:
> >> > +#define bio_list_for_each(bio, bl) \
> >> > +	for (bio = (bl)->head; bio && ({ prefetch(bio->bi_next); 1; }); \
> >> > +	     bio = bio->bi_next)
> >> > +
> >
> >Besides, manual prefetching is very rarely a win. I dabbled with some
> >benchmarks a few weeks back (with the doubly linked lists), and in most
> >cases it was actually a loss. So I'd vote for just removing the
> >prefetch() above.
> 
> So is the prefetching in the basic ADTs (e.g. linux/list.h) a loss too?

Depends on the box it seems. On the newest systems the processor
prefetching seems to be very much smarter. On a "classic" AMD Athlon the
prefetching made the scheduler about 1.5% faster...

Alan

Re: [2.6.22 PATCH 22/26] dm: bio list helpers

[Jens Axboe]

On Thu, May 10 2007, Alan Cox wrote:
> On Thu, 10 May 2007 16:17:57 +0200 (MEST)
> Jan Engelhardt <jengelh@linux01.gwdg.de> wrote:
> 
> > 
> > On May 9 2007 08:49, Jens Axboe wrote:
> > >On Tue, May 08 2007, Andrew Morton wrote:
> > >> > +#define bio_list_for_each(bio, bl) \
> > >> > +	for (bio = (bl)->head; bio && ({ prefetch(bio->bi_next); 1; }); \
> > >> > +	     bio = bio->bi_next)
> > >> > +
> > >
> > >Besides, manual prefetching is very rarely a win. I dabbled with some
> > >benchmarks a few weeks back (with the doubly linked lists), and in most
> > >cases it was actually a loss. So I'd vote for just removing the
> > >prefetch() above.
> > 
> > So is the prefetching in the basic ADTs (e.g. linux/list.h) a loss too?
> 
> Depends on the box it seems. On the newest systems the processor
> prefetching seems to be very much smarter. On a "classic" AMD Athlon the
> prefetching made the scheduler about 1.5% faster...

It very much depends on the box, indeed. The ones I tested on were
_slower_ with the prefetching, perhaps the dumber CPU's will benefit. In
the long run, I don't think the manual prefetching is a good idea.

-- 
Jens Axboe