lkml.org 
[lkml]   [2012]   [Apr]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [RFC] TIF_NOTIFY_RESUME, arch/*/*/*signal*.c and all such
On 04/24, Al Viro wrote:
>
> Untested variants pushed into signal.git#master; will test tomorrow. In
> the meanwhile, any code review (and testing of the entire thing on as many
> targets as possible) would be very welcome.

I started to read these patches today, will continue tomorrow. Somehow
I got stuck at f1fcb14721b4f1e65387d4563311f15f0bd33684, please see the
question below. And a couple of minor nits.




b4b620b87fd2f388cf4c13fea21f31bed7c9a1b0 new helper: sigsuspend()

Looks obviously correct but I do not understand this chunk in kernel.c,

+ #ifndef __ARCH_HAS_SYS_RT_SIGSUSPEND
+ /**
+ * sys_rt_sigsuspend - replace the signal mask for a value with the
+
#ifdef __ARCH_WANT_SYS_RT_SIGSUSPEND

So this checks the (never used/defined?) __ARCH_HAS_SYS_RT_SIGSUSPEND
but comments out __ARCH_WANT_SYS_RT_SIGSUSPEND. Looks like a typo.





6b78370886e4f61187404b7737a831281bde35e8 xtensa: switch to generic rt_sigsuspend(2)
and
d978bf9dd41728dd60fe2269493fe8f21d28eef3 h8300: switch to saved_sigmask-based sigsuspend/rt_sigsuspend

(off-topic, but do_signal()->try_to_freeze() looks unneeded and wrong)

+ /* If there's no signal to deliver, we just restore the saved mask. */
+ if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
+ clear_thread_flag(TIF_RESTORE_SIGMASK);
+ sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
^^^^^^^^^^^

set_current_blocked(&current->saved_sigmask) looks better.






f1fcb14721b4f1e65387d4563311f15f0bd33684 alpha: tidy signal delivery up

Everything looks fine, but I have the off-topic question. The changelog
says:

* checking for TIF_SIGPENDING is enough; set_restart_sigmask() sets this
one as well.

Agreed, but why set_restore_sigmask() sets TIF_SIGPENDING? It should be
never used without signal_pending() == T.

IOW, do you know a reason why this patch

--- x/arch/x86/include/asm/thread_info.h
+++ x/arch/x86/include/asm/thread_info.h
@@ -264,7 +264,7 @@ static inline void set_restore_sigmask(v
{
struct thread_info *ti = current_thread_info();
ti->status |= TS_RESTORE_SIGMASK;
- set_bit(TIF_SIGPENDING, (unsigned long *)&ti->flags);
+ WARN_ON(!test_bit(TIF_SIGPENDING, (unsigned long *)&ti->flags));
}

static inline bool is_ia32_task(void)

is not correct?

OK, say, sys_sigsuspend() does

current->state = TASK_INTERRUPTIBLE;
schedule();
set_restore_sigmask();
return -ERESTARTNOHAND;

so set_bit(TIF_SIGPENDING) saves us from the "spurious wakeup". But is
it really possible?

We had the bugs in ptrace some time ago (and iirc this is why sys_pause
checks signal_pending), but is there any reason today why the
TASK_INTERRUPTIBLE task can return from schedule() without SIGPENDING?
(of course, ignoring the case when this task was added to some
wait_queue_head_t).

I am just curious. Perhaps set_restore_sigmask() sets SIGPENDING just
to be safer, but otoh this can hide the problem.

Oleg.



\
 
 \ /
  Last update: 2012-04-26 20:41    [W:0.181 / U:1.416 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site