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_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.
The first check covers the case when target->utrace_flags already has
_UTRACE_DEATH_EVENTS and thus tracehook_report_death() can't bypass
utrace_report_death(), but we should ensure that REPORT_CALLBACKS() was
not called yet. We can check ->exit_state lockless because it must be
visible when utrace_report_death() unlocks utrace->lock.
The second check ensures we can't race with tracehook_report_death()
which checks task_utrace_flags(), so if we add _UTRACE_DEATH_EVENTS to
->utrace_flags we have to take tasklist.
In short: this double check allows to avoid tasklist when utrace_flags
already has these bits while engine->flags doesn't.
But multitracing is unlikely case, in the likely case if we add
_UTRACE_DEATH_EVENTS to engine->flags we set these bits in utrace_flags
too. I think it makes sense to consolidate these checks to make the
code a bit more understandable.
Also, add the small comment to explain the lockless exit_state check.
Signed-off-by: Oleg Nesterov <o...@redhat.com>
---
kernel/utrace.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
--- kstub/kernel/utrace.c~2_consolidate_death_events_checks 2010-08-14
04:30:28.000000000 +0200
+++ kstub/kernel/utrace.c 2010-08-14 04:30:28.000000000 +0200
@@ -539,11 +539,11 @@ int utrace_set_events(struct task_struct
old_utrace_flags = target->utrace_flags;
old_flags = engine->flags & ~ENGINE_STOP;
+ /* If ->death or ->reap is true we must see exit_state != 0. */
if (target->exit_state) {
unsigned long cleared = (old_flags & ~events);
- if (((events & ~old_flags) & _UTRACE_DEATH_EVENTS) ||
- (utrace->death && (cleared & _UTRACE_DEATH_EVENTS)) ||
+ if ((utrace->death && (cleared & _UTRACE_DEATH_EVENTS)) ||
(utrace->reap && (cleared & UTRACE_EVENT(REAP)))) {
spin_unlock(&utrace->lock);
return -EALREADY;
@@ -560,7 +560,7 @@ int utrace_set_events(struct task_struct
* knows positively that utrace_report_death() will be called or
* that it won't.
*/
- if ((events & ~old_utrace_flags) & _UTRACE_DEATH_EVENTS) {
+ if ((events & ~old_flags) & _UTRACE_DEATH_EVENTS) {
read_lock(&tasklist_lock);
if (unlikely(target->exit_state)) {
read_unlock(&tasklist_lock);
