Re: Possible problem with utrace_control

2010-06-17 Thread Alpár Török
2010/6/13 Roland McGrath rol...@redhat.com:
 Thanks for your interest in utrace.

 It's correct that passing UTRACE_REPORT to utrace_control should
 always ensure you get some report_* callback before the tracee returns
 to user mode.

 However, I think your use may be susceptible to a race between
 resumption by utrace_control, and the UTRACE_STOP done by the
 callback.  If your callback looks something like:

        wake_up_consumer();
        return UTRACE_STOP;

 then there is a window when the consumer can wake up and call
 utrace_control before that callback has actually returned UTRACE_STOP
 and had it processed.  In that event, your UTRACE_REPORT comes before
 the UTRACE_STOP, and then you do nothing afterwards to resume it.  If
 it ever did resume, it would indeed report (the UTRACE_REPORT is still
 pending).  But that doesn't help you.

 Off hand, I think there is only one way to address this race.  When
 you get the notification to consume, before calling utrace_control,
 call utrace_barrier first.  This will block the consumer until the
 producer's callback has actually finished and had UTRACE_STOP recorded
 for your engine.  At that point, you can be sure that your
 utrace_control call will indeed wake up the stopped tracee.

 Oleg or others here can double-check my reasoning and help explain the
 situation if you share your code for them to see.

 Because of this possibility, it might make sense to have utrace_control
 return -EINPROGRESS for this situation.  That at least would alert you
 that something might be amiss.  Unlike other cases of -EINPROGRESS, this
 would not just mean that you need to call utrace_barrier to be sure the
 effect has completed.  Rather, it means there is a danger of the
 scenario I described above, where the effect you asked for is going to
 be trumped by the callback's UTRACE_STOP.  So it doesn't quite fit in
 with the other situations--it's not really an indication that you have
 to follow up with a synchronization, it's an indication that your
 synchronization logic is already wrong.  But at least it would relieve
 the mystery of such a situation, assuming you are checking return values
 and noticing in debugging.  I'm certainly open to opinions on this API
 change.

 The need to use utrace_barrier as a second synchronization after the
 main wake-up (done by whatever normal means you are using) is rather
 clunky, and when it matters (as we suspect it does in the occasional
 race in your case) it leads to ping-pong scheduling.  So we would like
 to improve the utrace API in this area at some point.  One idea we have
 discussed before is a utrace_wake_up call.  (This assumes that some
 variant of linux/wait.h wake_up* is what you are using in the guts of
 wake_up_consumer.)  This would replace wake_up* for use in a callback,
 and simply do the wake-up after all the callbacks are finished (or
 perhaps just yours, but probably all), so your return value action is
 already recorded.  In fact, it might do the wakeup only after the
 UTRACE_STOP is not only recorded but actually performed, so that by the
 time your consumer wakes up, the tracee is actually stopped and
 available for third-party examination.  (You don't actually care about
 that for your case--for your purposes, it would be optimal just to delay
 the wakeup until your own callback's return value is recorded.  That
 way, if your consumer acts quickly enoug on another CPU, it might even
 resume the tracee before it actually yields the processor.)

 As you might tell from the complexity of that paragraph, there are many
 fuzzy nuances of exactly what that better API might be.  (And I didn't
 even delve into the possibility that you're using some synchronization
 mechanism other than wake_up* calls on a wait_queue_head_t.)  Hence we
 haven't tried to work it out exactly yet.  We hope that seeing a variety
 of uses such as yours will help us figure out how best to improve the
 utrace API for tracee-callback-to-tracer synchronization handshakes.


 Thanks,
 Roland


Sorry for the long delay. Here's my scenario for better understanding.

The buffer that's used for communication is in essence a circular
buffer,  but instead of  the buffer wrapping, the consumer is
suspended.

 I have  *a single* consumer at any time.  The consumer waits for data
to be available. The code looks like this:

set_current_state(TASK_INTERRUPTIBLE);
while (atomic_read(buf-tail) == atomic_read(buf-head))
 {
     pc_debug(Reader task going to sleep!);
     schedule();
     if (signal_pending(current)) {
         return -ERESTARTSYS;
     }
 }
set_current_state(TASK_RUNNING);

once data is available it read some , freeing up some space. After
that it  wakes up all producers. Producers are utrace monitored tasks.
 I use a  standard linked list  protected by a spin lock to protect it
to hold references   to the  utrace engine and task structures, that i
need for utrace control.  Is it possible to use a wait_queue for this
? I know it can 

Re: Possible problem with utrace_control

2010-06-17 Thread Roland McGrath
You should use linux/wait.h or similar facilities rather than calling
schedule() and wake_up_process() directly.  This doesn't have anything to
do with utrace, it's just the clean practice for normal kinds of blocking
in the kernel, for a variety of reasons.  That has to do with how your
control thread blocks, which is just the normal set of issues for any
thread in kernel code.  You are using the lowest-level way of doing things,
which is not recommended--that's why various higher-level facilities exist.

The admonition to use utrace facilities for all blocks has to do with when
you make a traced thread block.  You are on the correct path in that regard.

The relevant parts of your scenario match what I described.  The race I
talked about between your control thread's utrace_control() and tracee
callbacks returning UTRACE_STOP explains what you are seeing.


Thanks,
Roland