Hi Jens,
Jeff did some testing of this patchset on his NCQ-enabled SSD (the
30GB OCZ Vertex).
The test suite contained various multiple competing workloads
scenarios, and was run on for-2.6.33 and cfq-2.6.33 branches.
Max latencies were reduced in most cases, and we had also improvements
on bandwidth side in some scenarios, especially
for multiple random readers, either alone or competing with writes.
2 random readers aggregate bw increased from 48356 to 74205
and 4 random readers vs 1 seq writer:
* aggregate reader bw increased from 35242 to 56400
* writer bandwidth increased from 33269 to 55127
* maximum latency on read decreased from 535 to 324
* maximum latency on writes decreased from 22243 to 1153
It's a win on all measures.
The effect increasing the number of readers to 32 (latency_test_2.fio)
is even more visible (max read latency reduced from 3305 to 268,
aggregated read BW increased from 32894 to 164571).
The only case where I see an increased max latency is for 2 random
readers vs 1 seq reader:
for-2.6.33:
randomread.0: read_bw = 15,418K
randomread.1: read_bw = 15,399K
seqread: read_bw = 409K
0: read_bw = 31226
0: read_lat_max = 11.589
0: read_lat_avg = 3.22366666666667
cfq-2.6.33:
randomread.0: read_bw = 10,065K
randomread.1: read_bw = 10,067K
seqread: read_bw = 101M
0: read_bw = 121132
0: read_lat_max = 303
0: read_lat_avg = 0.282333333333333
but here the increased latency is paid back by a large increase in
sequential read BW (the max latency is, btw, experienced by the seq
reader, so I think it is a fair behaviour).
Jeff observed that the for-2.6.33 numbers were worse than his baseline
runs, probably due to changed hw_tag detection.
My patchset is much less sensible to hw_tag on SSDs (since there are
much less situations in which it would idle), so my numbers are
unaffected.
Corrado
On Tue, Nov 03 2009, Corrado Zoccolo wrote:
> Hi Jens,
> Jeff did some testing of this patchset on his NCQ-enabled SSD (the
> 30GB OCZ Vertex).
> The test suite contained various multiple competing workloads
> scenarios, and was run on for-2.6.33 and cfq-2.6.33 branches.
>
> Max latencies were reduced in most cases, and we had also improvements
> on bandwidth side in some scenarios, especially
> for multiple random readers, either alone or competing with writes.
> 2 random readers aggregate bw increased from 48356 to 74205
> and 4 random readers vs 1 seq writer:
> * aggregate reader bw increased from 35242 to 56400
> * writer bandwidth increased from 33269 to 55127
> * maximum latency on read decreased from 535 to 324
> * maximum latency on writes decreased from 22243 to 1153
> It's a win on all measures.
> The effect increasing the number of readers to 32 (latency_test_2.fio)
> is even more visible (max read latency reduced from 3305 to 268,
> aggregated read BW increased from 32894 to 164571).
>
> The only case where I see an increased max latency is for 2 random
> readers vs 1 seq reader:
>
> for-2.6.33:
> randomread.0: read_bw = 15,418K
> randomread.1: read_bw = 15,399K
> seqread: read_bw = 409K
> 0: read_bw = 31226
> 0: read_lat_max = 11.589
> 0: read_lat_avg = 3.22366666666667
>
> cfq-2.6.33:
> randomread.0: read_bw = 10,065K
> randomread.1: read_bw = 10,067K
> seqread: read_bw = 101M
> 0: read_bw = 121132
> 0: read_lat_max = 303
> 0: read_lat_avg = 0.282333333333333
>
> but here the increased latency is paid back by a large increase in
> sequential read BW (the max latency is, btw, experienced by the seq
> reader, so I think it is a fair behaviour).
>
> Jeff observed that the for-2.6.33 numbers were worse than his baseline
> runs, probably due to changed hw_tag detection.
> My patchset is much less sensible to hw_tag on SSDs (since there are
> much less situations in which it would idle), so my numbers are
> unaffected.
Thanks a lot for your testing. My testing on cfq-2.6.33 looks good too,
so I pulled it into for-2.6.33 today.
Since for-linus contains conflicting changes, can you and Jeff please
double check that everything is still in order? The interesting bit here
is the merge with for-2.6.33 and the coop limit from Shaohua Li. I did
the straight forward merge, but we likely just need to drop that logic
since the coop concept is radically different given that we merge and
break queues in for-2.6.33.
--
Jens Axboe
Jens Axboe <[email protected]> writes:
> Since for-linus contains conflicting changes, can you and Jeff please
> double check that everything is still in order? The interesting bit here
> is the merge with for-2.6.33 and the coop limit from Shaohua Li. I did
> the straight forward merge, but we likely just need to drop that logic
> since the coop concept is radically different given that we merge and
> break queues in for-2.6.33.
Yeah, since I changed the meaning of the cfqq_coop flag, a lot of those
tests are just plain wrong. Let me play with it and I'll send you an
incremental patch in a bit.
Cheers,
Jeff
On Tue, Nov 03 2009, Jeff Moyer wrote:
> Jens Axboe <[email protected]> writes:
>
> > Since for-linus contains conflicting changes, can you and Jeff please
> > double check that everything is still in order? The interesting bit here
> > is the merge with for-2.6.33 and the coop limit from Shaohua Li. I did
> > the straight forward merge, but we likely just need to drop that logic
> > since the coop concept is radically different given that we merge and
> > break queues in for-2.6.33.
>
> Yeah, since I changed the meaning of the cfqq_coop flag, a lot of those
> tests are just plain wrong. Let me play with it and I'll send you an
> incremental patch in a bit.
Thanks, here's what I have. It's basically a revert of the commit in
question.
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index b700f41..4ab240c 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -253,7 +253,6 @@ enum cfqq_state_flags {
CFQ_CFQQ_FLAG_slice_new, /* no requests dispatched in slice */
CFQ_CFQQ_FLAG_sync, /* synchronous queue */
CFQ_CFQQ_FLAG_coop, /* cfqq is shared */
- CFQ_CFQQ_FLAG_coop_preempt, /* coop preempt */
};
#define CFQ_CFQQ_FNS(name) \
@@ -280,7 +279,6 @@ CFQ_CFQQ_FNS(prio_changed);
CFQ_CFQQ_FNS(slice_new);
CFQ_CFQQ_FNS(sync);
CFQ_CFQQ_FNS(coop);
-CFQ_CFQQ_FNS(coop_preempt);
#undef CFQ_CFQQ_FNS
#define cfq_log_cfqq(cfqd, cfqq, fmt, args...) \
@@ -1070,16 +1068,9 @@ static struct cfq_queue *cfq_get_next_queue(struct cfq_data *cfqd)
static struct cfq_queue *cfq_set_active_queue(struct cfq_data *cfqd,
struct cfq_queue *cfqq)
{
- if (!cfqq) {
+ if (!cfqq)
cfqq = cfq_get_next_queue(cfqd);
- if (cfqq && !cfq_cfqq_coop_preempt(cfqq))
- cfq_clear_cfqq_coop(cfqq);
- }
-
- if (cfqq)
- cfq_clear_cfqq_coop_preempt(cfqq);
-
__cfq_set_active_queue(cfqd, cfqq);
return cfqq;
}
@@ -2433,16 +2424,8 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
* if this request is as-good as one we would expect from the
* current cfqq, let it preempt
*/
- if (cfq_rq_close(cfqd, cfqq, rq) && (!cfq_cfqq_coop(new_cfqq) ||
- cfqd->busy_queues == 1)) {
- /*
- * Mark new queue coop_preempt, so its coop flag will not be
- * cleared when new queue gets scheduled at the very first time
- */
- cfq_mark_cfqq_coop_preempt(new_cfqq);
- cfq_mark_cfqq_coop(new_cfqq);
+ if (cfq_rq_close(cfqd, cfqq, rq))
return true;
- }
return false;
}
--
Jens Axboe
Jens Axboe <[email protected]> writes:
> On Tue, Nov 03 2009, Jeff Moyer wrote:
>> Jens Axboe <[email protected]> writes:
>>
>> > Since for-linus contains conflicting changes, can you and Jeff please
>> > double check that everything is still in order? The interesting bit here
>> > is the merge with for-2.6.33 and the coop limit from Shaohua Li. I did
>> > the straight forward merge, but we likely just need to drop that logic
>> > since the coop concept is radically different given that we merge and
>> > break queues in for-2.6.33.
>>
>> Yeah, since I changed the meaning of the cfqq_coop flag, a lot of those
>> tests are just plain wrong. Let me play with it and I'll send you an
>> incremental patch in a bit.
>
> Thanks, here's what I have. It's basically a revert of the commit in
> question.
Your patch looks like a straight-forward revert. I still think we need
some guards in place, though. For now, I think we can go with what you
have, and I'll come up with some other mechanism to deal with this case.
Cheers,
Jeff
On Tue, Nov 03 2009, Jeff Moyer wrote:
> Jens Axboe <[email protected]> writes:
>
> > On Tue, Nov 03 2009, Jeff Moyer wrote:
> >> Jens Axboe <[email protected]> writes:
> >>
> >> > Since for-linus contains conflicting changes, can you and Jeff please
> >> > double check that everything is still in order? The interesting bit here
> >> > is the merge with for-2.6.33 and the coop limit from Shaohua Li. I did
> >> > the straight forward merge, but we likely just need to drop that logic
> >> > since the coop concept is radically different given that we merge and
> >> > break queues in for-2.6.33.
> >>
> >> Yeah, since I changed the meaning of the cfqq_coop flag, a lot of those
> >> tests are just plain wrong. Let me play with it and I'll send you an
> >> incremental patch in a bit.
> >
> > Thanks, here's what I have. It's basically a revert of the commit in
> > question.
>
> Your patch looks like a straight-forward revert. I still think we need
> some guards in place, though. For now, I think we can go with what you
> have, and I'll come up with some other mechanism to deal with this case.
Thanks Jeff, I'll merge it up and we can get things straightened out in
due time.
--
Jens Axboe