Re: [PATCH v5 2/6] cpu_isolated: add initial support

Frederic Weisbecker
2015-08-26T15:26:58Z

On Wed, Aug 12, 2015 at 02:22:09PM -0400, Chris Metcalf wrote:
> On 08/12/2015 12:00 PM, Frederic Weisbecker wrote:
> >>+#ifdef CONFIG_CPU_ISOLATED
> >>+void cpu_isolated_wait(void)
> >>+{
> >>+ set_current_state(TASK_INTERRUPTIBLE);
> >>+ _cpu_idle();
> >>+ set_current_state(TASK_RUNNING);
> >>+}
> >I'm still uncomfortable with that. A wake up model could work?
>
> I don't know exactly what you have in mind. The theory is that
> at this point we're ready to return to user space and we're just
> waiting for a timer tick that is guaranteed to arrive, since there
> is something pending for the timer.

Hmm, ok I'm going to discuss that in the new version. One worry is that
it gets racy and we sleep there for ever.

>
> And, this is an arch-specific method anyway; the generic method
> is actually checking to see if a signal has been delivered,
> scheduling is needed, etc., each time around the loop, so if
> you're not sure your architecture will do the right thing, just
> don't provide a method that idles while waiting. For tilegx I'm
> sure it works correctly, so I'm OK providing that method.

Yes but we do busy waiting on all other archs then. And since we can wait
for a while there, it doesn't look sane.

> >>diff --git a/include/linux/sched.h b/include/linux/sched.h
> >>index 04b5ada460b4..0bb248385d88 100644
> >>--- a/include/linux/sched.h
> >>+++ b/include/linux/sched.h
> >>@@ -1776,6 +1776,9 @@ struct task_struct {
> >> unsigned long task_state_change;
> >> #endif
> >> int pagefault_disabled;
> >>+#ifdef CONFIG_CPU_ISOLATED
> >>+ unsigned int cpu_isolated_flags;
> >>+#endif
> >Can't we add a new flag to tsk->flags? There seem to be some values remaining.
>
> Yeah, I thought of that, but it seems like a pretty scarce resource,
> and I wasn't sure it was the right thing to do. Also, I'm not actually
> sure why the lowest two bits aren't apparently being used

Probably they were used but got removed.

> looks
> like PF_EXITING (0x4) is the first bit used. And there are only three
> more bits higher up in the word that are not assigned.

Which makes room for 5 :)

>
> Also, right now we are allowing users to customize the signal delivered
> for STRICT violation, and that signal value is stored in the
> cpu_isolated_flags word as well, so we really don't have room in
> tsk->flags for all of that anyway.

Yeah indeed, ok lets keep it that way for now.

Thanks.