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); }