lkml.org 
[lkml]   [2008]   [Jul]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH] x86_64: fix delayed signals
Date
> But if you really want that behaviour, then re-introducing the loop would 
> likely be the better approach (or should be combined), since I think you
> effectively just re-introduced it (at a much bigger granularity).

I don't think so. Firstly, TIF_SIGPENDING is not the only flag in
question. There are other reasons to re-enter do_notify_resume().
If those are set during signal processing et al, they should take
effect before going back to user mode.

Second, there is always a race. Anywhere after the last time the
siglock was held inside do_signal(), there can be an interrupt that
sets TIF_SIGPENDING (or other _TIF_DO_NOTIFY_MASK flags). If you go
on to return to user mode, then it can be a long time before the new
signal is actually delivered (til the next tick).

It really is necessary to check all the _TIF_WORK_MASK flags with
interrupts disabled, last thing. I just don't see how any short
cuts here can be robust. It's simple, it's right, and it's what all
the other paths (and all other arch code I've ever noticed) do.

Since it's necessary to have robust checks in the final part of the
assembly code path anyway, and stacked signals are rare, there is
just no special reason to have a loop in do_signal(). In the common
case it every time retakes the siglock again when unnecessary, with
bad SMP performance effects; optimizing that with a signal_pending()
check just shows why it's simpler not to have a loop. Frankly, I'm
glad we don't have one because it would fix only the scenario that
has a test case that's real easy to write, and leave lying all the
much more hairy ones that will cause someone to spend days and days
some day later tearing his hair out.


Thanks,
Roland


\
 
 \ /
  Last update: 2008-07-11 00:47    [W:0.111 / U:0.104 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site