Messages in this thread |  | | | Date | Mon, 25 Apr 2011 13:52:53 +0200 | | From | Tejun Heo <> | | Subject | Re: [PATCH 3/3] signal: do_sigtimedwait() needs retarget_shared_pending() |
| |
Hello,
On Sat, Apr 23, 2011 at 08:00:00PM +0200, Oleg Nesterov wrote: > do_sigtimedwait() changes current->blocked and thus it needs > set_current_bloked()->retarget_shared_pending(). > > We could use set_current_bloked() directly. It is fine to change > ->real_blocked from all-zeroes to ->blocked and vice versa lockless, > but this is not immediately clear, looks racy, and needs a huge > comment to explain why this is correct. > > To keep the things simple this patch adds the new static helper, > __set_task_blocked() which should be called with ->siglock held. This > way we can change both ->real_blocked and ->blocked atomically under > ->siglock as the current code does. This is more understandable. > > Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Acked-by: Tejun Heo <tj@kernel.org>
> @@ -2322,7 +2327,8 @@ int do_sigtimedwait(sigset_t *these, sig > /* > * None ready -- temporarily unblock those we're > * interested while we are sleeping in so that we'll > - * be awakened when they arrive. > + * be awakened when they arrive. Unblocking is always > + * fine, we can avoid set_current_blocked(). > */ > tsk->real_blocked = tsk->blocked; > sigandsets(&tsk->blocked, &tsk->blocked, these);
Maybe it would be a good idea to introduce a new helper which checks / enforces that the operation indeed is only unblocking? Also, it can be a pure preference but I think _locked suffix is better / more common for APIs which expect the caller to be responsible for locking.
Thanks.
-- tejun
|  |