Vivek Goyal <vgoyal@xxxxxxxxxx> writes:
> On Tue, Jun 22, 2010 at 05:35:00PM -0400, Jeff Moyer wrote:
>
> [..]
>> @@ -1614,6 +1620,15 @@ __cfq_slice_expired(struct cfq_data *cfqd, struct cfq_queue *cfqq,
>> cfq_clear_cfqq_wait_request(cfqq);
>> cfq_clear_cfqq_wait_busy(cfqq);
>>
>> + if (!cfq_cfqq_yield(cfqq)) {
>> + struct cfq_rb_root *st;
>> + st = service_tree_for(cfqq->cfqg,
>> + cfqq_prio(cfqq), cfqq_type(cfqq));
>> + st->last_expiry = jiffies;
>> + st->last_pid = cfqq->pid;
>> + }
>> + cfq_clear_cfqq_yield(cfqq);
>
> Jeff, I think cfqq is still on service tree at this point of time. If yes,
> then we can simply use cfqq->service_tree, instead of calling
> service_tree_for().
Yup.
> No clearing of cfqq->yield_to field?
Nope. Again, it's not required, but if you really want me to, I'll add
it.
> [..]
>> /*
>> * Select a queue for service. If we have a current active queue,
>> * check whether to continue servicing it, or retrieve and set a new one.
>> @@ -2187,6 +2232,10 @@ static struct cfq_queue *cfq_select_queue(struct cfq_data *cfqd)
>> * have been idling all along on this queue and it should be
>> * ok to wait for this request to complete.
>> */
>> + if (cfq_cfqq_yield(cfqq) &&
>> + cfq_should_yield_now(cfqq, &new_cfqq))
>> + goto expire;
>> +
>
> I think we can get rid of this condition here and move the yield check
> above outside above if condition. This if condition waits for request to
> complete from this queue and waits for queue to get busy before slice
> expiry. If we have decided to yield the queue, there is no point in
> waiting for next request for queue to get busy.
Yeah, this is a vestige of the older code layout. Thanks, this cleans
things up nicely.
>> + cfq_log_cfqq(cfqd, cfqq, "yielding queue to %d", tsk->pid);
>> + cfqq->yield_to = new_cic;
>
> We are stashing away a pointer to cic without taking reference?
There is no reference counting on the cic.
>> @@ -3123,6 +3234,13 @@ cfq_should_preempt(struct cfq_data *cfqd, struct cfq_queue *new_cfqq,
>> if (!cfqq)
>> return false;
>>
>> + /*
>> + * If the active queue yielded its timeslice to this queue, let
>> + * it preempt.
>> + */
>> + if (cfq_cfqq_yield(cfqq) && RQ_CIC(rq) == cfqq->yield_to)
>> + return true;
>> +
>
> I think we need to again if if we are sync-noidle workload then allow
> preemption only if no dependent read is currently on, otherwise
> sync-noidle service tree loses share.
I think you mean don't yield if there is a dependent reader. Yeah,
makes sense.
> This version looks much simpler than previous one and is much easier
> to understand. I will do some testing on friday and provide you feedback.
Great, thanks again for the review!
Cheers,
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
