* [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
@ 2006-04-25 18:18 Hua Zhong
2006-04-25 18:30 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: Hua Zhong @ 2006-04-25 18:18 UTC (permalink / raw)
To: linux-kernel; +Cc: akpm, axboe
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)) &&
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-25 18:18 [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn Hua Zhong
@ 2006-04-25 18:30 ` Jens Axboe
2006-04-25 21:38 ` Hua Zhong
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Jens Axboe @ 2006-04-25 18:30 UTC (permalink / raw)
To: Hua Zhong; +Cc: linux-kernel, akpm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-25 18:30 ` Jens Axboe
@ 2006-04-25 21:38 ` Hua Zhong
2006-04-26 5:20 ` Jens Axboe
2006-04-26 17:06 ` Hua Zhong
2006-04-30 19:25 ` Pavel Machek
2 siblings, 1 reply; 10+ messages in thread
From: Hua Zhong @ 2006-04-25 21:38 UTC (permalink / raw)
To: 'Jens Axboe'; +Cc: linux-kernel, akpm
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
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-25 21:38 ` Hua Zhong
@ 2006-04-26 5:20 ` Jens Axboe
2006-04-26 13:50 ` James Bottomley
0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2006-04-26 5:20 UTC (permalink / raw)
To: Hua Zhong; +Cc: linux-kernel, akpm, James.Bottomley
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-26 5:20 ` Jens Axboe
@ 2006-04-26 13:50 ` James Bottomley
2006-04-26 13:55 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2006-04-26 13:50 UTC (permalink / raw)
To: Jens Axboe; +Cc: Hua Zhong, linux-kernel, akpm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-26 13:50 ` James Bottomley
@ 2006-04-26 13:55 ` Jens Axboe
2006-04-26 14:24 ` James Bottomley
0 siblings, 1 reply; 10+ messages in thread
From: Jens Axboe @ 2006-04-26 13:55 UTC (permalink / raw)
To: James Bottomley; +Cc: Hua Zhong, linux-kernel, akpm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-26 13:55 ` Jens Axboe
@ 2006-04-26 14:24 ` James Bottomley
2006-04-27 14:39 ` Jens Axboe
0 siblings, 1 reply; 10+ messages in thread
From: James Bottomley @ 2006-04-26 14:24 UTC (permalink / raw)
To: Jens Axboe; +Cc: Hua Zhong, linux-kernel, akpm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* RE: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-25 18:30 ` Jens Axboe
2006-04-25 21:38 ` Hua Zhong
@ 2006-04-26 17:06 ` Hua Zhong
2006-04-30 19:25 ` Pavel Machek
2 siblings, 0 replies; 10+ messages in thread
From: Hua Zhong @ 2006-04-26 17:06 UTC (permalink / raw)
To: 'Jens Axboe'; +Cc: linux-kernel, akpm
> 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). :-)
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-26 14:24 ` James Bottomley
@ 2006-04-27 14:39 ` Jens Axboe
0 siblings, 0 replies; 10+ messages in thread
From: Jens Axboe @ 2006-04-27 14:39 UTC (permalink / raw)
To: James Bottomley; +Cc: Hua Zhong, linux-kernel, akpm
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn
2006-04-25 18:30 ` Jens Axboe
2006-04-25 21:38 ` Hua Zhong
2006-04-26 17:06 ` Hua Zhong
@ 2006-04-30 19:25 ` Pavel Machek
2 siblings, 0 replies; 10+ messages in thread
From: Pavel Machek @ 2006-04-30 19:25 UTC (permalink / raw)
To: Jens Axboe; +Cc: Hua Zhong, linux-kernel, akpm
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!
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2006-05-01 20:30 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-04-25 18:18 [PATCH] likely cleanup: revert unlikely in ll_back_merge_fn Hua Zhong
2006-04-25 18:30 ` Jens Axboe
2006-04-25 21:38 ` Hua Zhong
2006-04-26 5:20 ` Jens Axboe
2006-04-26 13:50 ` James Bottomley
2006-04-26 13:55 ` Jens Axboe
2006-04-26 14:24 ` James Bottomley
2006-04-27 14:39 ` Jens Axboe
2006-04-26 17:06 ` Hua Zhong
2006-04-30 19:25 ` Pavel Machek
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.