Re: [PATCH] sched: fix task and run queue run_delay inconsistencies

[Peter Zijlstra <peterz@infradead.org>]

On Wed, Sep 30, 2015 at 08:28:41PM +0000, Meyer, Mike wrote:
> > From: Peter Zijlstra [mailto:peterz@infradead.org]
> >
> > On Wed, Sep 23, 2015 at 12:37:18AM +0000, Meyer, Mike wrote:
> > >
> > > The proposed patch addresses the issue by calling
> > > sched_info_reset_dequeued(thread) following the call to
> > > enqueue_task(rq, thread) for running threads in situations in which
> > > thread->sched_info.last_queued should remain 0.
> > 
> > Would something like the below; which avoids calling
> > sched_info_{de,}queued() for these sites also work?
> > 
> > It even shrinks the code (due to inlining {en,de}queue_task()):
> > 
> > $ size defconfig-build/kernel/sched/core.o defconfig-
> > build/kernel/sched/core.o.orig
> >    text    data     bss     dec     hex filename
> >   64019   23378    2344   89741   15e8d defconfig-build/kernel/sched/core.o
> >   64149   23378    2344   89871   15f0f defconfig-build/kernel/sched/core.o.orig
> > 
> Yes that will also address the issue.
> 
> The reason I approached the way I did was to avoid adding code path to
> the far more common uses of {en,de}queue_task() but I doubt anyone is
> going to notice a difference with the addition of some register
> save/restores and a compare in that path.  Overall the code does
> shrink with the alternative which is good.

In most cases the flags should be compile time constants, and with the
inline we can determine the branch at compile time, avoiding emitting
that branch instruction entirely.

But let me double check the asm for a few important sites.

> My only comment is I am not sure about the naming of the flag
> ENQUEUE_TEMP which implies (to me) the enqueue is temporary which
> clearly it isn't.    Maybe something like DEQUEUE_MOVE/ENQUEUE_MOVE
> would be a bit more descriptive of the use case.

Yes, I ran out of creative juices, let me attempt a better name once
I've woken up a bit.