On 10/19, Roland McGrath wrote:
>
> > > But that's how it used to be, and then we changed it.
> > > So it merits digging up our old discussion threads that led to doing that
> > > and see what all we had in mind then.
> >
> > I tried to grep my mail archives and google, but failed to find anything
> > related.
>
> Based on the date of the commit you just mentioned, I browsed in:
> https://www.redhat.com/archives/utrace-devel/2009-October/thread.html
> and saw:
> https://www.redhat.com/archives/utrace-devel/2009-October/msg00287.html
> which is the day before that commit. I didn't read through all that to
> refamiliarize myself with all the details.
Yes, I saw this thread, but didn't read it carefully. When I read it again,
I started to believe that yes, it can explain the current ->resume logic.
However, I think this is false alarm. With a lot of efforts, I seem to
fully understand our discussion.
In short: I believe we never discussed why utrace->resume should be
*STEP, especially if task is stopped or it is going to stop. Although
I recall I thought this is "obviously nice" when I tried to review
"utrace: sticky resume action".
As for this thread. Yes, I tried to suggest the (ugly)
utrace->set_singlestep which should be checked after wakeup. But my
motivation was quite different, what I was trying to preserve was
TIF_SINGLESTEP, not resume_action. This is why the top email says
"I no longer think utrace_control() should just turn SINGLESTEP into
REPORT silently".
Please recall that, by that time
1. utrace_control(SINGLESTEP) called user_enable_single_step()
2. To handle the stepping over syscall correctly, both ptrace
and ptrace-utrace relied on syscall_trace_leave() paths
which checked TIF_SINGLESTEP to send SIGTRAP if needed.
1 was wrong, user_enable_single_step() was called under utrace->lock.
It was decided that the tracee itself should call this helper before
it returns to user-mode. To implement this, we could change
utrace_control(SINGLESTEP) to set TIF_NOTIFY_RESUME, so that
->report_quiesce() could return UTRACE_SINGLESTEP.
However, this breaks 2, the stepping over syscall. By the time
->report_quiesce() is called we already passed syscall_trace_leave()
without TIF_SINGLESTEP, and ->report_quiesce() can't just return
UTRACE_SINGLESTEP but otoh it can't know that we should synthesize
a trap before return to user-mode.
So. I do not think there is any reason to ever keep UTRACE_*STEP in
utrace->resume. Except, if utrace_control(STEP) sets ->resume = STEP
before wakeup, we avoid the "unnecessary" reporting loop in utrace_resume.
But at the same time, this leads to the problems we are discussing.
I even tested the kernel with the patch below, everything _seems_
to work (not that I think this can proves something).
What do you think?
Oleg.
--- kstub/kernel/utrace.c~XXX_RESUME 2010-10-20 18:18:40.000000000 +0200
+++ kstub/kernel/utrace.c 2010-10-21 18:48:52.000000000 +0200
@@ -768,11 +768,13 @@ relock:
/*
* Ensure a reporting pass when we're resumed.
*/
- utrace->resume = action;
if (action == UTRACE_INTERRUPT)
set_thread_flag(TIF_SIGPENDING);
- else
+ else {
+ action = UTRACE_REPORT;
set_thread_flag(TIF_NOTIFY_RESUME);
+ }
+ utrace->resume = action;
}
/*
@@ -1195,6 +1197,7 @@ int utrace_control(struct task_struct *t
* In that case, utrace_get_signal() will be reporting soon.
*/
clear_engine_wants_stop(engine);
+ action = UTRACE_REPORT;
if (action < utrace->resume) {
utrace->resume = action;
set_notify_resume(target);
@@ -1381,11 +1384,13 @@ static void finish_report(struct task_st
if (resume < utrace->resume) {
spin_lock(&utrace->lock);
- utrace->resume = resume;
if (resume == UTRACE_INTERRUPT)
set_tsk_thread_flag(task, TIF_SIGPENDING);
- else
+ else {
+ resume = UTRACE_REPORT;
set_tsk_thread_flag(task, TIF_NOTIFY_RESUME);
+ }
+ utrace->resume = resume;
spin_unlock(&utrace->lock);
}
