On 10/22, Oleg Nesterov wrote: > > On 10/19, Roland McGrath wrote: > > > > > Yes. But, with this patch, in this case > > > utrace_barrier()->get_utrace_lock(attached => false) returns success > > > and then we check utrace->reporting == engine. > > > > I guess that's OK if ->reporting == engine is always set when any kind of > > callback might be in progress. > > I think utrace_maybe_reap() is the only exception, > > > > (Hmm. Probably utrace->reap = T case needs more attention, > > > utrace_maybe_reap() doesn't set utrace->reporting). > > > > That would be a problem. > > Yes, but the current code has the same problem? Hmm, I need to > recheck this all once again tomorrow. Suddenly I started to suspect > that even ->ops == NULL case is not right. And the more I read this > code now, the more I confused.
Yes. And "get_utrace_lock() must not succeed if utrace->reap == T" c93fecc9 makes things worse. With this patch utrace_barrier() can return -ESRCH while ->report_death() or report_reap() is in progress. But even before that comment, I do not think that !engine->ops check can prevent the race with ->report_reap(), we need at least mb() in utrace_maybe_reap() before "engine->ops = NULL". > But in any case. If engine->ops == &utrace_detached_ops and > utrace->reporting != engine, I do not see why utrace_barrier() can't > return success, even if utrace->reap is set. No! it must not return success! I still think it should not spin if ->reporting != engine, but it should not return zero. Somehow I forgot that utrace_barrier() != "reporting != engine". This means that my patch is very wrong. Oleg.
