Re: [PATCH] x86/sched/dynamic-ticks: Call new schedule_irq_disable() for scheduling in entry_64.S

[Frederic Weisbecker <fweisbec@gmail.com>]

On Thu, May 09, 2013 at 08:47:09PM -0400, Steven Rostedt wrote:
> On Fri, 2013-05-10 at 02:22 +0200, Frederic Weisbecker wrote:
> >  
> > > @@ -654,6 +653,7 @@ sysret_check:
> > >  	LOCKDEP_SYS_EXIT
> > >  	DISABLE_INTERRUPTS(CLBR_NONE)
> > >  	TRACE_IRQS_OFF
> > > +sysret_check_irqsoff:
> > >  	movl TI_flags+THREAD_INFO(%rsp,RIP-ARGOFFSET),%edx
> > >  	andl %edi,%edx
> > >  	jnz  sysret_careful
> > > @@ -675,12 +675,10 @@ sysret_check:
> > >  sysret_careful:
> > >  	bt $TIF_NEED_RESCHED,%edx
> > >  	jnc sysret_signal
> > > -	TRACE_IRQS_ON
> > > -	ENABLE_INTERRUPTS(CLBR_NONE)
> > >  	pushq_cfi %rdi
> > > -	SCHEDULE_USER
> > > +	call schedule_irq_disabled
> > >  	popq_cfi %rdi
> > > -	jmp sysret_check
> > > +	jmp sysret_check_irqsoff
> > 
> > Don't we still want to call LOCKDEP_SYS_EXIT?, the previous
> > task scheduled might have taken some locks.
> 
> Sure, but would that still catch this? I thought LOCKDEP_SYS_EXIT just
> cared about going into user land if the current task had locks. Where it
> shouldn't here. And you are concerned about the previous task. Well, the
> scheduler should have caught that, as you shouldn't have any spin locks
> there. And it doesn't matter if the previous task had a mutex. The
> previous task is still in kernel space.

Ah you're right. But still the current task had several opportunities to take locks
since the context switch. It's still relevant to check that.

> > 
> > > +
> > > +	local_irq_enable();
> > > +	__schedule();
> > 
> > Are you sure it's fine to call __schedule() instead of schedule()?
> > I don't know much about the blk work to handle there but it may be
> > worth considering.
> 
> Note, this isn't a call to schedule that was made by normal kernel
> space. This is very much like a preemption. In fact, it is a preemption
> and my first patch used preempt_schedule_irq() instead, but as that's
> made for CONFIG_PREEMPT enabled, it seemed a bit hacky to take that out
> of that context.
> 
> This is only called when the task is being preempted. But because its
> going to user land, its OK. The blk code there is more worried about
> something that did some work, set itself to TASK_UNINTERRUPTIBLE and
> then called schedule() (think completions). That is, the blk code that
> needs to be submitted will be the code that wakes up this task.
> 
> We should never be in anything but TASK_RUNNING when going into user
> space.

Ok, I haven't checked the details of what that blk_ thing is doing so,
if it really doesn't care about any kind of preemption, that's ok.