Hi!
It seems the preemption "bonus" logic in CFQ is reversed, a preempted
process is given an additional delay in start time instead of a bonus.
This seems unfair. I'm not sure if it's a good idea to let
slice_resid grow without limit as shown below, but isn't this more
like the way it was intended to work? Or did I misunderstand
something?
PS! The comment above cfq_preempt_queue seems outdated too.
Code not tested, just showing what I mean:
diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
index 664ebfd..ea18d45 100644
--- a/block/cfq-iosched.c
+++ b/block/cfq-iosched.c
@@ -292,7 +292,8 @@ cfq_prio_to_slice(struct cfq_data *cfqd, struct
cfq_queue *cfqq)
static inline void
cfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
{
- cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) + jiffies;
+ cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) +
cfqq->slice_resid + jiffies;
+ cfqq->slice_resid = 0;
cfq_log_cfqq(cfqd, cfqq, "set_slice=%lu", cfqq->slice_end - jiffies);
}
@@ -489,8 +490,6 @@ static void cfq_service_tree_add(struct cfq_data *cfqd,
rb_key += jiffies;
} else if (!add_front) {
rb_key = cfq_slice_offset(cfqd, cfqq) + jiffies;
- rb_key += cfqq->slice_resid;
- cfqq->slice_resid = 0;
} else
rb_key = 0;
@@ -1851,8 +1850,7 @@ cfq_should_preempt(struct cfq_data *cfqd, struct
cfq_queue *new_cfqq,
}
/*
- * cfqq preempts the active queue. if we allowed preempt with no slice left,
- * let it have half of its nominal slice.
+ * cfqq preempts the active queue. start a new time slice.
*/
static void cfq_preempt_queue(struct cfq_data *cfqd, struct cfq_queue *cfqq)
{
--
mvh
Carl Henrik
On Mon, Apr 20 2009, Carl Henrik Lunde wrote:
> Hi!
>
> It seems the preemption "bonus" logic in CFQ is reversed, a preempted
> process is given an additional delay in start time instead of a bonus.
> This seems unfair. I'm not sure if it's a good idea to let
Hmm? ->slice_resid is a long, so if we preempt the process 10 jiffies
before it was supposed to end, the resid will be -10. So it'll not
increase the rb_key, it'll decrease it.
> slice_resid grow without limit as shown below, but isn't this more
> like the way it was intended to work? Or did I misunderstand
> something?
->slice_resid is reset when it gets repositioned in the rb tree. The
intent was not to increase the slice length, but instead allow it sooner
service again.
> PS! The comment above cfq_preempt_queue seems outdated too.
Yep, the slice length comment is out dated indeed.
> Code not tested, just showing what I mean:
>
> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
> index 664ebfd..ea18d45 100644
> --- a/block/cfq-iosched.c
> +++ b/block/cfq-iosched.c
> @@ -292,7 +292,8 @@ cfq_prio_to_slice(struct cfq_data *cfqd, struct
> cfq_queue *cfqq)
> static inline void
> cfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
> {
> - cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) + jiffies;
> + cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) +
> cfqq->slice_resid + jiffies;
So if ->slice_resid is negative because we preempted this queue, it'll
now get a shorter slice. That's not very nice :-)
--
Jens Axboe
On Mon, Apr 20, 2009 at 08:16, Jens Axboe <[email protected]> wrote:
> On Mon, Apr 20 2009, Carl Henrik Lunde wrote:
>> Hi!
>>
>> It seems the preemption "bonus" logic in CFQ is reversed, a preempted
>> process is given an additional delay in start time instead of a bonus.
>> ?This seems unfair. ?I'm not sure if it's a good idea to let
>
> Hmm? ->slice_resid is a long, so if we preempt the process 10 jiffies
> before it was supposed to end, the resid will be -10. So it'll not
> increase the rb_key, it'll decrease it.
OK, so maybe I'm tired (I am!), but I don't get it. :)
{
if (... busy_rt_queues)
cfq_slice_expire(timed_out=1)
}
cfq_slice_expire() {
{
if (timed_out)
slice_resid = slice_end - jiffies;
}
if preempted it would be 100 - 90 = +10?
>
>> slice_resid grow without limit as shown below, but isn't this more
>> like the way it was intended to work? ?Or did I misunderstand
>> something?
>
> ->slice_resid is reset when it gets repositioned in the rb tree. The
> intent was not to increase the slice length, but instead allow it sooner
> service again.
Fair enough
>> PS! The comment above cfq_preempt_queue seems outdated too.
>
> Yep, the slice length comment is out dated indeed.
>
>> Code not tested, just showing what I mean:
>>
>> diff --git a/block/cfq-iosched.c b/block/cfq-iosched.c
>> index 664ebfd..ea18d45 100644
>> --- a/block/cfq-iosched.c
>> +++ b/block/cfq-iosched.c
>> @@ -292,7 +292,8 @@ cfq_prio_to_slice(struct cfq_data *cfqd, struct
>> cfq_queue *cfqq)
>> ?static inline void
>> ?cfq_set_prio_slice(struct cfq_data *cfqd, struct cfq_queue *cfqq)
>> ?{
>> - ? ? ? cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) + jiffies;
>> + ? ? ? cfqq->slice_end = cfq_prio_to_slice(cfqd, cfqq) +
>> cfqq->slice_resid + jiffies;
>
> So if ->slice_resid is negative because we preempted this queue, it'll
> now get a shorter slice. That's not very nice :-)
>
> --
> Jens Axboe
>
>
--
mvh
Carl Henrik
On Mon, Apr 20 2009, Carl Henrik Lunde wrote:
> On Mon, Apr 20, 2009 at 08:16, Jens Axboe <[email protected]> wrote:
> > On Mon, Apr 20 2009, Carl Henrik Lunde wrote:
> >> Hi!
> >>
> >> It seems the preemption "bonus" logic in CFQ is reversed, a preempted
> >> process is given an additional delay in start time instead of a bonus.
> >> ?This seems unfair. ?I'm not sure if it's a good idea to let
> >
> > Hmm? ->slice_resid is a long, so if we preempt the process 10 jiffies
> > before it was supposed to end, the resid will be -10. So it'll not
> > increase the rb_key, it'll decrease it.
>
> OK, so maybe I'm tired (I am!), but I don't get it. :)
>
> {
> if (... busy_rt_queues)
> cfq_slice_expire(timed_out=1)
> }
>
> cfq_slice_expire() {
> {
> if (timed_out)
> slice_resid = slice_end - jiffies;
> }
>
> if preempted it would be 100 - 90 = +10?
Hrmpf, that is buggy, it should be jiffies - slice_end! Presumably that
has been buggy since the shift to rb service tree, since (IIRC) that was
when it was changed from "add to slice length" to "adjust in tree".
--
Jens Axboe