Re: [PATCH 0/3] UTRACE_DETACH fixes

2010-08-16 Thread Roland McGrath
> Whatever we do, start_callback() can see the old engine->flags but > the new ->ops = &utrace_detached_ops. Just suppose that the caller > of UTRACE_DETACH is interrupted right after setting engine->ops. * it can check the old flags before using the old ops, or check the old * flags before usin

Re: [PATCH 2/3] fix utrace_control(DETACH) && utrace->death interaction

2010-08-16 Thread Roland McGrath
> The problem is, utrace_control(DETACH) does nothing and returns > -EALREADY if utrace->death is set, this is not right. We can and > should detach in this case, we only should skip utrace_reset() to > avoid the race with utrace_report_death()->REPORT_CALLBACKS(). This behavior is the original (m

Re: [PATCH 1/3] get_utrace_lock() must not succeed if utrace->reap == T

2010-08-16 Thread Roland McGrath
> get_utrace_lock() must threat utrace->reap == T as engine->ops == NULL. Yes, I think you're right. This requires some changes to the kerneldoc and utrace.tmpl, because it now says that you get EALREADY if report_reap is already running. Now it will be consistent with utrace_control, where you

Re: [PATCH 3/4] utrace_set_events: fix UTRACE_EVENT(REAP) case

2010-08-16 Thread Roland McGrath
> - It is possible that both ->death and ->reap are true. In this > case it is OK to clear UTRACE_EVENT(REAP), but set_events fails. No, it's not OK to clear it. Once ->reap is set, then the engine's ops->report_reap might or might not have been called already. If utrace_set_events

Re: [PATCH 2/4] utrace_set_events: consolidate "setting _UTRACE_DEATH_EVENTS" checks

2010-08-16 Thread Roland McGrath
> utrace_set_events() checks the "setting _UTRACE_DEATH_EVENTS" case twice, > and it is not immediately obvious why the first check is needed, and why > it is not racy (we are checking exit_state without tasklist). The code is > correct, but looks confusing. More comments are always good. > In sh

Re: [PATCH 2/4] utrace_set_events: consolidate "setting _UTRACE_DEATH_EVENTS" checks

2010-08-16 Thread Roland McGrath
> Hmm. For unknown reason I do not see this 2/4 patch on utrace-devel, > strange. > > So I am attaching it in case my email was really lost and you didn't > get it. Indeed, it did not come through to me or the list. Thanks, Roland

Re: [PATCH 2/4] utrace_set_events: consolidate "setting _UTRACE_DEATH_EVENTS" checks

2010-08-16 Thread Oleg Nesterov
Hmm. For unknown reason I do not see this 2/4 patch on utrace-devel, strange. So I am attaching it in case my email was really lost and you didn't get it. Oleg. [PATCH 2/4] utrace_set_events: consolidate "setting _UTRACE_DEATH_EVENTS" checks utrace_set_events() checks the "setting _UTRACE_DEATH_

Re: [PATCH 0/3] UTRACE_DETACH fixes

2010-08-16 Thread Oleg Nesterov
On 08/16, Oleg Nesterov wrote: > > Well, I tried to test these changes, seems to work. > > But... From 2/3 changelog: > > This means that > mark_engine_detached() can race with REPORT_CALLBACKS() but there > is nothing new. > > Yes, there is nothing new. But, Roland, this is WRONG

Re: [PATCH 0/3] UTRACE_DETACH fixes

2010-08-16 Thread Oleg Nesterov
On 08/16, Oleg Nesterov wrote: > > On top of utrace_set_events() 1-4 fixes I sent. > > utrace_barrier() needs more fixes, I'll send them separately. Later. > Untested, but still I am asking for your review. Of course I am > going to test these changes later, but I'm afraid the thorough > review i

[PATCH 3/3] utrace_set_events: kill now unnecessary exit_state/reap checks

2010-08-16 Thread Oleg Nesterov
Now that get_utrace_lock() can never succeed if utrace->reap is true, we can kill this check in utrace_set_events(). This also means we can kill the target->exit_state check, it buys nothing now. Signed-off-by: Oleg Nesterov --- kernel/utrace.c | 13 - 1 file changed, 4 insertions

[PATCH 2/3] fix utrace_control(DETACH) && utrace->death interaction

2010-08-16 Thread Oleg Nesterov
Suppose that we want to detach the engine and free engine->data. To avoid the races with our calbacks which can use ->data we should do either err = utrace_set_events(0); if (err == ...) utrace_barrier(); utrace_control(DETACH); or err = utrace_con

[PATCH 1/3] get_utrace_lock() must not succeed if utrace->reap == T

2010-08-16 Thread Oleg Nesterov
get_utrace_lock() must threat utrace->reap == T as engine->ops == NULL. The logic in get_utrace_lock() is correct. If we see engine->ops != NULL under rcu_read_lock(), the target has not passed release_task() yet, so it is safe to dereference target/utrace and take utrace->lock. However. If utrac

[PATCH 0/3] UTRACE_DETACH fixes

2010-08-16 Thread Oleg Nesterov
On top of utrace_set_events() 1-4 fixes I sent. utrace_barrier() needs more fixes, I'll send them separately. Untested, but still I am asking for your review. Of course I am going to test these changes later, but I'm afraid the thorough review is much more important, it is not clear to me how it