[PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Hua Zhong]

With likely/unlikely profiling (see the recent patch dwalker@mvista.com sent), on my not-so-busy-typical-development system it shows more than 80K misses and no hits. So I guess it makes sense to revert.

I don't know BIO code very well, but I hope this data is useful for the experts.

Signed-off-by: Hua Zhong <hzhong@gmail.com>

diff --git a/block/ll_rw_blk.c b/block/ll_rw_blk.c
index 1755c05..499da1c 100644
--- a/block/ll_rw_blk.c
+++ b/block/ll_rw_blk.c
@@ -1432,7 +1432,7 @@ static int ll_back_merge_fn(request_queu
  	}
  	if (unlikely(!bio_flagged(req->biotail, BIO_SEG_VALID)))
  		blk_recount_segments(q, req->biotail);
-	if (unlikely(!bio_flagged(bio, BIO_SEG_VALID)))
+	if (likely(!bio_flagged(bio, BIO_SEG_VALID)))
  		blk_recount_segments(q, bio);
  	len = req->biotail->bi_hw_back_size + bio->bi_hw_front_size;
  	if (BIOVEC_VIRT_MERGEABLE(__BVEC_END(req->biotail), __BVEC_START(bio)) &&

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Jens Axboe]

On Tue, Apr 25 2006, Hua Zhong wrote:
> With likely/unlikely profiling (see the recent patch dwalker@mvista.com 
> sent), on my not-so-busy-typical-development system it shows more than 
> 80K misses and no hits. So I guess it makes sense to revert.
> 
> I don't know BIO code very well, but I hope this data is useful for the 
> experts.

Well you'd want to optimize for the busy case, right, no point in
optimizing for a more idle system.

I'm not at all uninterested in this, I'd just like to see a more
intelligent/controlled work load that actually stresses the io subsystem
being profiled. If you have a not-so-busy system, you like don't do
enough IO to trigger a lot of merges. Or maybe you do, and we just have
a bug somewhere so that we unfortunately repeatedly recount segments.

Care to run a simple io benchmark and profile that?

-- 
Jens Axboe

RE: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Hua Zhong]

It seems that new BIOs do not have BIO_SEG_VALID set. So when you do sequential IO, the IO being back-merged should always have not
had valid segments.

I ran bonnie++ and it shows the same thing.

> Well you'd want to optimize for the busy case, right, no 
> point in optimizing for a more idle system.
> 
> I'm not at all uninterested in this, I'd just like to see a 
> more intelligent/controlled work load that actually stresses 
> the io subsystem being profiled. If you have a not-so-busy 
> system, you like don't do enough IO to trigger a lot of 
> merges. Or maybe you do, and we just have a bug somewhere so 
> that we unfortunately repeatedly recount segments.
> 
> Care to run a simple io benchmark and profile that?
> 
> --
> Jens Axboe
> 

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Jens Axboe]

On Tue, Apr 25 2006, Hua Zhong wrote:
> It seems that new BIOs do not have BIO_SEG_VALID set. So when you do
> sequential IO, the IO being back-merged should always have not had
> valid segments.
> 
> I ran bonnie++ and it shows the same thing.

But blk_recount_segments() sets the BIO_SEG_VALID flag. Ugh ok
__bio_add_page() basically kills the flag. James, I think you are the
author of that addition, does it really need to be so restrictive?

        /* If we may be able to merge these biovecs, force a recount */
        if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec) ||
            BIOVEC_VIRT_MERGEABLE(bvec-1, bvec)))
                bio->bi_flags &= ~(1 << BIO_SEG_VALID);

with that in place, we may as well just remove ->bi_phys_segments
and ->bi_hw_segments since we'll be calculating the values over and over
again while building up a bio.

-- 
Jens Axboe

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[James Bottomley]

On Wed, 2006-04-26 at 07:20 +0200, Jens Axboe wrote:
> But blk_recount_segments() sets the BIO_SEG_VALID flag. Ugh ok
> __bio_add_page() basically kills the flag. James, I think you are the
> author of that addition, does it really need to be so restrictive?
> 
>         /* If we may be able to merge these biovecs, force a recount */
>         if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec) ||
>             BIOVEC_VIRT_MERGEABLE(bvec-1, bvec)))
>                 bio->bi_flags &= ~(1 << BIO_SEG_VALID);

Help me out here ... I can't find this chunk of code in the current
tree.  Where is it?

James

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Jens Axboe]

On Wed, Apr 26 2006, James Bottomley wrote:
> On Wed, 2006-04-26 at 07:20 +0200, Jens Axboe wrote:
> > But blk_recount_segments() sets the BIO_SEG_VALID flag. Ugh ok
> > __bio_add_page() basically kills the flag. James, I think you are the
> > author of that addition, does it really need to be so restrictive?
> > 
> >         /* If we may be able to merge these biovecs, force a recount */
> >         if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec) ||
> >             BIOVEC_VIRT_MERGEABLE(bvec-1, bvec)))
> >                 bio->bi_flags &= ~(1 << BIO_SEG_VALID);
> 
> Help me out here ... I can't find this chunk of code in the current
> tree.  Where is it?

Sorry, should have mentioned that. Current git tree (or 2.6.16 should be
the same), fs/bio.c:__bio_add_page():401.

-- 
Jens Axboe

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[James Bottomley]

On Wed, 2006-04-26 at 15:55 +0200, Jens Axboe wrote:
> On Wed, Apr 26 2006, James Bottomley wrote:
> > On Wed, 2006-04-26 at 07:20 +0200, Jens Axboe wrote:
> > > But blk_recount_segments() sets the BIO_SEG_VALID flag. Ugh ok
> > > __bio_add_page() basically kills the flag. James, I think you are the
> > > author of that addition, does it really need to be so restrictive?
> > > 
> > >         /* If we may be able to merge these biovecs, force a recount */
> > >         if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec) ||
> > >             BIOVEC_VIRT_MERGEABLE(bvec-1, bvec)))
> > >                 bio->bi_flags &= ~(1 << BIO_SEG_VALID);
> > 
> > Help me out here ... I can't find this chunk of code in the current
> > tree.  Where is it?
> 
> Sorry, should have mentioned that. Current git tree (or 2.6.16 should be
> the same), fs/bio.c:__bio_add_page():401.

OK, that's this change.

http://www.kernel.org/git/?p=linux/kernel/git/tglx/history.git;a=commit;h=2fed84384a0b084d78252aa14d6bfae03deb268f

I think the reason for this is that the bi_hw_back_size and bi_hw_front
size aren't updated without a segment recount, so you could get rid of
the valid flag clearing if you introduce a heuristic to update them.

James

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Jens Axboe]

On Wed, Apr 26 2006, James Bottomley wrote:
> On Wed, 2006-04-26 at 15:55 +0200, Jens Axboe wrote:
> > On Wed, Apr 26 2006, James Bottomley wrote:
> > > On Wed, 2006-04-26 at 07:20 +0200, Jens Axboe wrote:
> > > > But blk_recount_segments() sets the BIO_SEG_VALID flag. Ugh ok
> > > > __bio_add_page() basically kills the flag. James, I think you are the
> > > > author of that addition, does it really need to be so restrictive?
> > > > 
> > > >         /* If we may be able to merge these biovecs, force a recount */
> > > >         if (bio->bi_vcnt && (BIOVEC_PHYS_MERGEABLE(bvec-1, bvec) ||
> > > >             BIOVEC_VIRT_MERGEABLE(bvec-1, bvec)))
> > > >                 bio->bi_flags &= ~(1 << BIO_SEG_VALID);
> > > 
> > > Help me out here ... I can't find this chunk of code in the current
> > > tree.  Where is it?
> > 
> > Sorry, should have mentioned that. Current git tree (or 2.6.16 should be
> > the same), fs/bio.c:__bio_add_page():401.
> 
> OK, that's this change.
> 
> http://www.kernel.org/git/?p=linux/kernel/git/tglx/history.git;a=commit;h=2fed84384a0b084d78252aa14d6bfae03deb268f
> 
> I think the reason for this is that the bi_hw_back_size and bi_hw_front
> size aren't updated without a segment recount, so you could get rid of
> the valid flag clearing if you introduce a heuristic to update them.

Hmm yes, I'll ponder that a bit. Do you have any ideas on how to best
handle that? I guess it's not a huge deal, would be nice to avoid the
repeat segment calculations though.

-- 
Jens Axboe

RE: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Hua Zhong]

> I'm not at all uninterested in this, I'd just like to see a 
> more intelligent/controlled work load that actually stresses 
> the io subsystem being profiled. If you have a not-so-busy 
> system, you like don't do enough IO to trigger a lot of 
> merges. Or maybe you do, and we just have a bug somewhere so 
> that we unfortunately repeatedly recount segments.

I'm glad we actually found something interesting (bug or not). :-)

Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn

[Pavel Machek]

On Tue 25-04-06 20:30:26, Jens Axboe wrote:
> On Tue, Apr 25 2006, Hua Zhong wrote:
> > With likely/unlikely profiling (see the recent patch dwalker@mvista.com 
> > sent), on my not-so-busy-typical-development system it shows more than 
> > 80K misses and no hits. So I guess it makes sense to revert.
> > 
> > I don't know BIO code very well, but I hope this data is useful for the 
> > experts.
> 
> Well you'd want to optimize for the busy case, right, no point in
> optimizing for a more idle system.

Careful here... for battery-powered systems you may want to optimize
for idle system...
							Pavel
-- 
Thanks, Sharp!