On 9/12/2016 1:41 PM, Andy Lutomirski wrote:
On Sep 9, 2016 1:40 PM, "Chris Metcalf" <cmetcalf@xxxxxxxxxxxx> wrote:
On 9/2/2016 1:28 PM, Andy Lutomirski wrote:
On Sep 2, 2016 7:04 AM, "Chris Metcalf" <cmetcalf@xxxxxxxxxxxx> wrote:
On 8/30/2016 3:50 PM, Andy Lutomirski wrote:
On Tue, Aug 30, 2016 at 12:37 PM, Chris Metcalf <cmetcalf@xxxxxxxxxxxx> wrote:
So to pop up a level, what is your actual concern about the existing
"do it in a loop" model? The macrology currently in use means there
is zero cost if you don't configure TASK_ISOLATION, and the software
maintenance cost seems low since the idioms used for task isolation
in the loop are generally familiar to people reading that code.
My concern is that it's not obvious to readers of the code that the
loop ever terminates. It really ought to, but it's doing something
very odd. Normally we can loop because we get scheduled out, but
actually blocking in the return-to-userspace path, especially blocking
on a condition that doesn't have a wakeup associated with it, is odd.
True, although, comments :-)
Regardless, though, this doesn't seem at all weird to me in the
context of the vmstat and lru stuff, though. It's exactly parallel to
the fact that we loop around on checking need_resched and signal, and
in some cases you could imagine multiple loops around when we schedule
out and get a signal, so loop around again, and then another
reschedule event happens during signal processing so we go around
again, etc. Eventually it settles down. It's the same with the
vmstat/lru stuff.
Only kind of.
When we say, effectively, while (need_resched()) schedule();, we're
not waiting for an event or condition per se. We're runnable (in the
sense that userspace wants to run and we're not blocked on anything)
the entire time -- we're simply yielding to some other thread that is
also runnable. So if that loop runs forever, it either means that
we're at low priority and we genuinely shouldn't be running or that
there's a scheduler bug.
If, on the other hand, we say while (not quiesced) schedule(); (or
equivalent), we're saying that we're *not* really ready to run and
that we're waiting for some condition to change. The condition in
question is fairly complicated and won't wake us when we are ready. I
can also imagine the scheduler getting rather confused, since, as far
as the scheduler knows, we are runnable and we are supposed to be
running.
So, how about a code structure like this?
In the main return-to-userspace loop where we check TIF flags,
we keep the notion of our TIF_TASK_ISOLATION flag that causes
us to invoke a task_isolation_prepare() routine. This routine
does the following things:
1. As you suggested, set a new TIF bit (or equivalent) that says the
system should no longer create deferred work on this core, and then
flush any necessary already-deferred work (currently, the LRU cache
and the vmstat stuff). We never have to go flush the deferred work
again during this task's return to userspace. Note that this bit can
only be set on a core marked for task isolation, so it can't be used
for denial of service type attacks on normal cores that are trying to
multitask normal Linux processes.
I think it can't be a TIF flag unless you can do the whole mess with
preemption off because, if you get preempted, other tasks on the CPU
won't see the flag. You could do it with a percpu flag, I think.
Yes, a percpu flag - you're right. I think it will make sense for this to
be a flag declared in linux/isolation.h which can be read by vmstat, LRU, etc.
2. Check if the dyntick is stopped, and if not, wait on a completion
that will be set when it does stop. This means we may schedule out at
this point, but when we return, the deferred work stuff is still safe
since your bit is still set, and in principle the dyn tick is
stopped.
Then, after we disable interrupts and re-read the thread-info flags,
we check to see if the TIF_TASK_ISOLATION flag is the ONLY flag still
set that would keep us in the loop. This will always end up happening
on each return to userspace, since the only thing that actually clears
the bit is a prctl() call. When that happens we know we are about to
return to userspace, so we call task_isolation_ready(), which now has
two things to do:
Would it perhaps be more straightforward to do the stuff before the
loop and not check TIF_TASK_ISOLATION in the loop?
We can certainly play around with just not looping in this case, but
in particular I can imagine an isolated task entering the kernel and
then doing something that requires scheduling a kernel task. We'd
clearly like that other task to run before the isolated task returns to
userspace. But then, that other task might do something to re-enable
the dyntick. That's why we'd like to recheck that dyntick is off in
the loop after each potential call to schedule().
1. We check that the dyntick is in fact stopped, since it's possible
that a race condition led to it being somehow restarted by an interrupt.
If it is not stopped, we go around the loop again so we can go back in
to the completion discussed above and wait some more. This may merit
a WARN_ON or other notice since it seems like people aren't convinced
there are things that could restart it, but honestly the dyntick stuff
is complex enough that I think a belt-and-suspenders kind of test here
at the last minute is just defensive programming.
Seems reasonable. But maybe this could go after the loop and, if the
dyntick is back, it could be treated like any other kernel bug that
interrupts an isolated task? That would preserve more of the existing
code structure.
Well, we can certainly try it that way. If I move it out and my testing
doesn't trigger the bug, that's at least an initial sign that it might be
OK. But I worry/suspect that it will trip up at some point in some use
case and we'll have to fix it at that point.
If that works, it could go in user_enter().
Presumably with trace_user_enter() and vtime_user_enter()
in __context_tracking_enter()?
--
Chris Metcalf, Mellanox Technologies
http://www.mellanox.com
--
To unsubscribe from this list: send the line "unsubscribe linux-api" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
