Investigate avoiding lifecycle_lock traffic in task::unkillable()

[bblum]

task::unkillable is used a bunch in task, some in sync, and I suspect it really should be used in other places where we have unsafe code, like pipes.

Currently it hammers on lifecycle_lock every time:

void rust_task::inhibit_kill() {
    scoped_lock with(lifecycle_lock);
    disallow_kill++;
}
void rust_task::allow_kill() {
    scoped_lock with(lifecycle_lock);
    disallow_kill--;
}

The reason is to protect it from racing a killer (this code is approximate):

void rust_task::kill() {
    scoped_lock with(lifecycle_lock);
    killed = true;
    if (blocked && disallow_kill == 0) {
        // Point A
        punt_awake();
    }
}

If we just removed the lock in inhibit_kill, the race could be that at point A, the unkillable task increments its flag and promptly goes to sleep, only to get punted awake from what it thought was an unkillable sleep.

If we removed the lock in allow_kill, the race would be that the killer sees the task is not killable, and at point A it then becomes killable and goes to sleep, but the killer doesn't punt it awake, and it blocks forever.

I believe the following version should be safe (though not helgrind-clean), and reduce traffic on lifecycle_lock:

void rust_task::inhibit_kill() {
    disallow_kill++;
    if (killed) {
        scoped_lock with(lifecycle_lock);
    }
}
void rust_task::allow_kill() {
    disallow_kill--;
    if (killed) {
        scoped_lock with(lifecycle_lock);
    }
}

Try this and see what it does to task and sync performance. This is pretty fragile code (i.e. might start racing if something changes elsewhere), so only actually use it if it's a clear win.

[bblum]

Actually, I don't believe we should have to bounce off the lock at all in the proposed fix. The reason is because rust_task::block() itself takes the lifecycle lock and checks the killed flag.

[bblum]

The other things to consider would be yield() / wait_event(), which are where else the disallow_kill flag gets accessed. But all those functions are always run by the same task, so there would be no race among them.

[bblum]

I ran the tests. On task-perf-jargon-metal-smoke there was a 5.5% average speedup and on task-perf-linked-failure there was a 3% average speedup. Here are some graphs:

http://bblum.net/images/perf-removing-lifecycle-lock/100-generations.svg
http://bblum.net/images/perf-removing-lifecycle-lock/100-generations-load.svg
http://bblum.net/images/perf-removing-lifecycle-lock/linked-failure.svg
http://bblum.net/images/perf-removing-lifecycle-lock/linked-failure-load.svg

Electing to go ahead with the change.

[bblum]

In honesty, the dubious change (i.e. removing it around inhibit_kill/allow_kill) is probably responsible for half the speedup. The rest is from inhibit_yield/allow_yield, which never even looked like they needed the locks in the first place.

[bblum]

Also see 5ba7434. Removing the locks in yield() may be possible.