On 06/21, Roland McGrath wrote: > > > I am wondering if there is another way to handle such a race... > > Suppose we > > > > - add UTRACE_STOP_XXX > > > > - modify utrace_control(UTRACE_STOP_XXX) to do > > mark_engine_wants_stop() and nothing more. > > utrace_control(current,,UTRACE_STOP) doesn't really do anything more.
It does utrace_do_stop() which sets UTRACE_REPORT/TIF_NOTIFY_RESUME. Harmless, but makes sense to avoid. > > Then, probably producer could do > > > > utrace_control(current, UTRACE_STOP_XXX); > > > > slot = book_message_in_buffer(buf); > > if (slot < 0) > > return UTRACE_STOP_XXX; > > > > ... > > return UTRACE_RESUME; > > > > Even better, UTRACE_STOP_XXX can live outside of UTRACE_RESUME_MASK. > > Right, it doesn't belong in enum utrace_resume_action--it doesn't make > sense as an argument for utrace_control, for example. So it could be > something like a return-value flag bit saying "stop if marked for stop", > e.g. UTRACE_SINGLESTEP|UTRACE_STICKY_STOP for single-step, unless marked > to stop, in which case stay stopped. Yes, this is what I meant. And yes, if UTRACE_STICKY_STOP doesn't fall into UTRACE_RESUME_MASK utrace_control(enum utrace_resume_action) shouldn't be used. OTOH, I am wondering if we can give more power to utrace_control(STOP). Currently debugger can't stop the tracee unless it cooperates. However, if the tracee doesn't have UTRACE_EVENT(QUIESCE), then utrace_control(STOP) works. This is strange. Stupid question: what if we just remove the clear_engine_wants_stop() code in finish_callback_report() ? > So this would fix that race. > ... > (But note if there is > another engine that decides to stop anyway, then we'll have used a > second CPU and/or ping-pong scheduled to run the tracer thread in > parallel with a very short amount of work in the tracee before it just > blocks anyway.) Hmm. Not sure I understand... Yes, the tracee still can stop if another tracer demands, but this is fine? Btw, before I forgot about this. shouldn't utrace_control(UTRACE_RESUME) remove ENGINE_STOP from ->utrace_flags like DETACH does? > Here > what the callback does is wake up the tracer, and return UTRACE_STOP. > > For this sort of scenario, it is clearly better by far to have the > wake_up_tracer() done as late as possible in the sequence. > > In the past, Oleg has suggested something very general, like a > utrace_engine_ops hook that gets called right before schedule(). > I shy away from something like that, since I think it degrades the > intended property of the API that it make it harder to write truly > arcane bugs with terrible effects on the system, not easier. It's > just too big a sledgehammer to wield above the thinly-protected toes > of the utrace engine writer. > > To avoid something dangerously general, the only real option is to give > something safely constrained and specific. > It's straightforward enough to > come up with something for any single synchronization method. For > example, if every need for this kind of synchronization with tracees is > based on <linux/wait.h> facilities, then we can have a utrace API feature > whereby a callback can queue up a wait_queue_head_t pointer to be passed > to wake_up_interruptible_sync() after all the callbacks are done and > UTRACE_STOP is being performed. It's probably sufficient to allow one > pending wake-up per engine, which makes it pretty cheap and simple to > implement. Heh. it turns out that I misunderstood you in the past, I thought you dislike the whole idea of wake_up from utrace_stop(). I agree, the utrace_engine_ops->notify() hook is dangerous. But, it is simple and allows us to kill utrace_stop()->ptrace_notify_stop(). (yes I understand, this is not only about utrace_stop(), utrace_barrier wants notification too). > It's straightforward enough to > come up with something for any single synchronization method. Hmm. Not sure... at least I don't immediately see something simple. Except, just add wait_queue_head into struct utrace. This way utrace_barrier() can wait longer than needed if it notices utrace->reporting == engine. Or we can add it into utrace_engine, in this case utrace_stop() needs list_for_each(engine, utrace->attached). Or I misunderstood you again? Oleg.
