lkml.org 
[lkml]   [2010]   [Jun]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 1/3] block: Implement a blk_yield function to voluntarily give up the I/O scheduler.
    Date
    Vivek Goyal <vgoyal@redhat.com> 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


    \
     
     \ /
      Last update: 2010-06-25 18:55    [W:0.033 / U:122.376 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site