Re: [PATCH?] avoid the unnecessary utrace_resume()-utrace_reset()

[Oleg Nesterov]

On 08/18, Oleg Nesterov wrote:

 utrace_resume(UTRACE_REPORT) always calls utrace_reset() because
 start_callback() obviously can't clear report-spurious when
 event == 0.

 Change start_callback() to correctly clear -spurious in this case.

Correctly? I am stupid, and this patch is wrong (47c593ee in
your tree).

 --- kstub/kernel/utrace.c~10_utrace_resume_and_spurious   2010-08-18 
 19:00:50.0 +0200
 +++ kstub/kernel/utrace.c 2010-08-18 19:41:05.0 +0200
 @@ -1540,7 +1540,7 @@ static const struct utrace_engine_ops *s
   if (want  ENGINE_STOP)
   report-action = UTRACE_STOP;

 - if (want  event) {
 + if (want  (event ?: UTRACE_EVENT(QUIESCE))) {
   report-spurious = false;
   return ops;
   }

with this change utrace_resume()-start_callback() returns with
utrace-reporting != NULL !!! This obviously breaks utrace_barrier(),
it can hang forever.

I noticed this by accident, when I was trying to understand the
problems with vCont changes.

I'll send the fix tomorrow. Damn, the fix is trivial but I'd like
to avoid another ugly check in start_callback(), and I don't think
utrace_resume() should clear -reporting.

Oleg.

Re: [PATCH?] avoid the unnecessary utrace_resume()-utrace_reset()

[Roland McGrath]

 utrace_resume(UTRACE_REPORT) always calls utrace_reset() because
 start_callback() obviously can't clear report-spurious when
 event == 0.
 
 Change start_callback() to correctly clear -spurious in this case.

Ok.

 Note: utrace_control(DETACH) does utrace_do_stop() and sets UTRACE_REPORT
 if the tracee is not stopped. It also does mark_engine_detached() which
 does not set QUIESCE in target-utrace_flags. This means we rely on
 report.spurious which should provoke utrace_reset() from utrace_resume()
 if target-utrace_flags doesn't have QUIESCE. A bit too subtle, imho.

Agreed.  There is no reason utrace_control can't set it in utrace_flags
in its !reset case.

 Also, UTRACE_REPORT can be lost because of UTRACE_INTERRUPT or normal
 signal: utrace_get_signal() checks utrace_flags  UTRACE_EVENT(QUIESCE)
 and returns otherwise. This should be fixed somehow. This check is wrong
 anyway, but it is not clear how we can fix the race with DETACH.

I see.  That would be fixed by utrace_control setting it.


Thanks,
Roland

[PATCH?] avoid the unnecessary utrace_resume()-utrace_reset()

[Oleg Nesterov]

utrace_resume(UTRACE_REPORT) always calls utrace_reset() because
start_callback() obviously can't clear report-spurious when
event == 0.

Change start_callback() to correctly clear -spurious in this case.
We could probably clear it in utrace_resume() unconditionally after
reporting loop, but this is not exactly right if there are no engines
which have QUIESCE in engine-flags.

utrace_get_signal() probably has the same problem.

Note: utrace_control(DETACH) does utrace_do_stop() and sets UTRACE_REPORT
if the tracee is not stopped. It also does mark_engine_detached() which
does not set QUIESCE in target-utrace_flags. This means we rely on
report.spurious which should provoke utrace_reset() from utrace_resume()
if target-utrace_flags doesn't have QUIESCE. A bit too subtle, imho.
Also, UTRACE_REPORT can be lost because of UTRACE_INTERRUPT or normal
signal: utrace_get_signal() checks utrace_flags  UTRACE_EVENT(QUIESCE)
and returns otherwise. This should be fixed somehow. This check is wrong
anyway, but it is not clear how we can fix the race with DETACH.

Signed-off-by: Oleg Nesterov o...@redhat.com
---

 kernel/utrace.c |2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- kstub/kernel/utrace.c~10_utrace_resume_and_spurious 2010-08-18 
19:00:50.0 +0200
+++ kstub/kernel/utrace.c   2010-08-18 19:41:05.0 +0200
@@ -1540,7 +1540,7 @@ static const struct utrace_engine_ops *s
if (want  ENGINE_STOP)
report-action = UTRACE_STOP;
 
-   if (want  event) {
+   if (want  (event ?: UTRACE_EVENT(QUIESCE))) {
report-spurious = false;
return ops;
}