Re: TIF_NOHZ can escape nonhz mask? (Was: [PATCH v3 6/8] x86: Split syscall_trace_enter into two phases)

[Frederic Weisbecker]

On Tue, Jul 29, 2014 at 07:54:14PM +0200, Oleg Nesterov wrote:
> Thanks Frederic for your explanations. Yes, I was confused. But cough, now I am
> even more confused.
> 
> I didn't even try to read this code, perhaps I'll try later, but let me ask
> another question while you are here ;)
> 
> The comment above __context_tracking_task_switch() says:
> 
> 	 * The context tracking uses the syscall slow path to implement its user-kernel
> 	 * boundaries probes on syscalls. This way it doesn't impact the syscall fast
> 	 * path on CPUs that don't do context tracking.
> 	        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Indeed, in fact the comment is confusing the way it explain things. It suggests that
some CPUs maybe doing context tracking while some other can choose not to context track.

It's rather all or nothing. Actually TIF_NOHZ optimize systems that have CONFIG_CONTEXT_TRACKING=y
and that don't need context tracking. In this case TIF_NOHZ is cleared and thus the syscall
fastpath has no overhead.

So I should rephrase it that way:

        * The context tracking uses the syscall slow path to implement its user-kernel
        * boundaries probes on syscalls. This way it doesn't impact the syscall fast
        * path when context tracking is globally disabled.

> 
> How? Every running task should have TIF_NOHZ set if context_tracking_is_enabled() ?
> 
> 	 * But we need to clear the flag on the previous task because it may later
> 	 * migrate to some CPU that doesn't do the context tracking. As such the TIF
> 	 * flag may not be desired there.
> 
> For what? How this can help? This flag will be set again when we switch to this
> task again?

That is indeed a stale comment from aborted early design.

> 
> Looks like, we can kill context_tracking_task_switch() and simply change the
> "__init" callers of context_tracking_cpu_set() to do set_thread_flag(TIF_NOHZ) ?
> Then this flag will be propagated by copy_process().

Right, that would be much better. Good catch! context tracking is enabled from
tick_nohz_init(). This is the init 0 task so the flag should be propagated from there.

I still think we need a for_each_process_thread() set as well though because some
kernel threads may well have been created at this stage already.

> 
> Or I am totally confused? (quite possible).
> 
> > So here is a scenario where this is a problem: a task runs on CPU 0, passes the context
> > tracking call before returning from a syscall to userspace, and gets an interrupt. The
> > interrupt preempts the task and it moves to CPU 1. So it returns from preempt_schedule_irq()
> > after which it is going to resume to userspace.
> >
> > In this scenario, if context tracking is only enabled on CPU 1, we have no way to know that
> > the task is resuming to userspace, because we passed through the context tracking probe
> > already and it was ignored on CPU 0.
> 
> Thanks. But I still can't understand... So if we only track CPU 1, then in this
> case context_tracking.state == IN_USER on CPU 0, but it can be IN_USER or IN_KERNEL
> on CPU 1.

I'm not sure I understand your question. Context tracking is either enabled everywhere or
nowhere.

I need to say though that there is a per CPU context tracking state named context_tracking.active.
It's confusing because it suggests that context tracking is active per CPU. Actually it's tracked
everywhere when globally enabled, but active determines if we call the RCU and vtime callbacks or
not.

So only nohz full CPUs have context_tracking.active set because only these need to call the RCU
and vtime callbacks. Other CPUs still do the context tracking but they won't call rcu and vtime
functions.

> 
> Oleg.
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/