Re: [PATCH] Add unaccounted time to timeslice_used.

[Vivek Goyal <vgoyal@redhat.com>]

On Tue, Mar 22, 2011 at 10:36:25AM -0700, Justin TerAvest wrote:
> On Tue, Mar 22, 2011 at 10:33 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> 
> > On Mon, Mar 14, 2011 at 02:55:27PM -0700, Justin TerAvest wrote:
> > > On Mon, Mar 14, 2011 at 6:55 AM, Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > On Fri, Mar 11, 2011 at 01:06:12PM -0800, Justin TerAvest wrote:
> > > >> There are two kind of times that tasks are not charged for: the first
> > > >> seek and the extra time slice used over the allocated timeslice. Both
> > > >> of these exported as a new unaccounted_time stat.
> > > >>
> > > >> I think it would be good to have this reported in 'time' as well, but
> > > >> that is probably a separate discussion.
> > > >>
> > > >
> > > > Justin,
> > > >
> > > > I would say that for such optimization do make sure that you mention
> > that
> > > > these are useful only if one is driving a queue depth of 1.
> > >
> > > Hi Vivek,
> > >
> > > That's a good point. I should have mentioned that.
> > >
> > > >
> > > > Otherwise previous queue might have dumped bunch of requests in device
> > > > and expired. Now new queue's first request completion time is also
> > > > impacted by the requests dumped by other queues.
> > > >
> > > > There are already so many stats which I have never used so far and I
> > have
> > > > not encountered anybody else using these either. I think primary reason
> > > > being that in general nobody forced the queue depth of 1 hence most of
> > the
> > > > timing stats are of no use.
> > >
> > > We could probably put the data collected here back into "time"
> > > eventually, but having it separate right now helps build confidence in
> > > the accuracy of the stats.
> > >
> > > >
> > > > So personally I am not very keen on keep on increasing number of stats
> > in
> > > > CFQ which are useful only when using queue depth 1 as that might not be
> > > > the common case. But Jens likes it, so....
> > > >
> > > > Also a comment inline.
> > > >
> > > > [..]
> > > >> @@ -3314,9 +3321,7 @@ static void cfq_preempt_queue(struct cfq_data
> > *cfqd, struct cfq_queue *cfqq)
> > > >>       BUG_ON(!cfq_cfqq_on_rr(cfqq));
> > > >>
> > > >>       cfq_service_tree_add(cfqd, cfqq, 1);
> > > >> -
> > > >> -     cfqq->slice_end = 0;
> > > >> -     cfq_mark_cfqq_slice_new(cfqq);
> > > >> +     __cfq_set_active_queue(cfqd, cfqq);
> > > >
> > > > So far a new queue selection was always in select_queue(). Now this
> > will
> > > > change it and new queue selection will also take place in
> > > > cfq_preempt_queue().
> > > >
> > > > Also I think this is not right. It is not necessary that we select the
> > > > preempting queue. For example a sync queue in one group can preempt the
> > > > async in root group but it might happen that we still select again
> > > > the root group's sync queue for dispatch.
> > > >
> > > > So queue selection logic should be driven by select_queue() which
> > selects
> > > > group first then workload with-in group and then queue with-in workload
> > > > and we shoud not be setting active queue here.
> > >
> > > That sounds reasonable. I will send out another version of the patch
> > > that will only clear the stats for the cfqq.
> >
> > Hi Justin,
> >
> > Are you planning to send a fix?
> >
> > - do not set active queue in preempt_queue()
> >
> 
> Yes. The reason I did not do this immediately was because I started to
> wonder if now we'll have a stale value of jiffies. :( If you want, I can
> still make that change immediately so that the active queue isn't set, at
> least for now.
> 

I think fixing this is important otherwise it gets serving_group also
out of sync and that can lead to further bad effects.

unaccounted time is a debug feature which works only with queue depth
1, so even if there are little issues with jiffies, you can sort that
out in a follow up patch.

Thanks
Vivek

> 
> > - move unaccounted time under debug?
> >
> 
> Yes.
> 
> I should be able to do this by the end of the day today.
> 
> 
> >
> > Thanks
> > Vivek
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >