Re: [PATCH 08/10] posix-cpu-timers: Migrate to use new tick dependency mask model

[Frederic Weisbecker]

On Fri, Jul 24, 2015 at 12:57:24PM -0400, Chris Metcalf wrote:
> On 07/23/2015 12:42 PM, Frederic Weisbecker wrote:
> >+static void cpu_timer_list_dequeue(struct cpu_timer_list *t)
> >+{
> >+	if (!list_empty(&t->entry))
> >+		cpu_timer_dec_tick_dependency();
> >+	list_del_init(&t->entry);
> >+}
> 
> Is the list_empty() test necessary? It wasn't in the original posix-timers
> code, and it feels like a pretty serious bug if you're doing a list_del on
> an empty list.

No multiple calls to list_del_init() is fine on a list_head as long as it
has been correctly initialized with INIT_LIST_HEAD() and list_del() hasn't
been called at some point before.

It's necessary because we do that dequeue also when we change the timer,
we disarm it in case it was added somewhere before.

> At a higher level, is the posix-cpu-timers code here really providing the
> right semantics? It seems like before, the code was checking a struct
> task-specific state, and now you are setting a global state such that if ANY
> task anywhere in the system (even on housekeeping cores) has a pending posix
> cpu timer, then nothing can go into nohz_full mode.
> 
> Perhaps what is needed is a task_struct->tick_dependency to go along with
> the system-wide and per-cpu flag words?

That's an excellent point! Indeed the tick dependency check on posix-cpu-timers
was made on task granularity before and now it's a global dependency.

Which means that if any task in the system has a posix-cpu-timer enqueued, it
prevents all CPUs from shutting down the tick. I need to mention that in the
changelog.

Now here is the rationale: I expect that nohz full users are not interested in
posix cpu timers at all. The only chance for one to run without breaking the
isolation is on housekeeping CPUs. So perhaps there is a corner case somewhere
but I assume there isn't until somebody reports an issue.

Keeping a task level dependency check means that we need to update it on context
switch. Plus it's not only about task but also process. So that means two
states to update on context switch and to check from interrupts. I don't think
it's worth the effort if there is no user at all.
--
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/