Messages in this thread | | | Date | Wed, 4 Jun 2008 22:01:01 +0400 | From | Oleg Nesterov <> | Subject | Re: [PATCH 1/2] schedule: fix TASK_WAKEKILL vs SIGKILL race |
| |
On 06/04, Matthew Wilcox wrote: > > On Wed, Jun 04, 2008 at 09:09:05PM +0400, Oleg Nesterov wrote: > > Note this "__TASK_STOPPED | __TASK_TRACED" check in signal_pending_state(). > > Probably it would be better to remove it, but this will change the current > > behaviour and thus needs a separate discussion. > > We're changing the behaviour anyway. Let's have the discussion and get > it right. > > In my opinion, not checking for TASK_STOPPED or TASK_TRACED previously was > an oversight. This should be fixed.
Perhaps, and the changelog has a special note. But imho we need another patch for that, this is a user-visible change.
> > +int signal_pending_state(long state, struct task_struct *p) > > +{ > > + if (!(state & (TASK_INTERRUPTIBLE | TASK_WAKEKILL))) > > + return 0; > > + if (!signal_pending(p)) > > + return 0; > > + > > + if (state & TASK_INTERRUPTIBLE) > > + return 1; > > + if (state & (__TASK_STOPPED | __TASK_TRACED)) > > + return 0; > > Just deleting the above two lines should do it?
Yes.
> > if (prev->state && !(preempt_count() & PREEMPT_ACTIVE)) { > > - if (unlikely((prev->state & TASK_INTERRUPTIBLE) && > > - signal_pending(prev))) { > > + if (unlikely(signal_pending_state(prev->state, prev))) > > prev->state = TASK_RUNNING; > > - } else { > > + else > > deactivate_task(rq, prev, 1); > > - } > > Getting rid of the extra braces is against CodingStyle: > > Do not unnecessarily use braces where a single statement will do. > > if (condition) > action(); > > This does not apply if one branch of a conditional statement is a single > statement. Use braces in both branches. > > if (condition) { > do_this(); > do_that(); > } else { > otherwise(); > }
With this patch the code is
if (unlikely(signal_pending_state(prev->state, prev))) prev->state = TASK_RUNNING; else deactivate_task(rq, prev, 1);
> This patch is going to add quite a few cycles to schedule(). Has anyone > done any benchmarks with a schedule-heavy workload?
No, I didn't. This patch is bugfix.
> I don't think signal_pending_state() should be in signal.c, just put it > in sched.c along with its only caller. That way, gcc can choose to > inline it if that's more efficient.
Perhaps you are right. In that case it doesn't need the "long state" argument.
However, I think the new helper can have other users. Not that I have a strong opinion.
Oleg.
| |