[PATCH 1/4] set_restore_sigmask

[PATCH 1/4] set_restore_sigmask

[Roland McGrath]

This adds the set_restore_sigmask() inline in <linux/thread_info.h> and
replaces every set_thread_flag(TIF_RESTORE_SIGMASK) with a call to it.
No change, but abstracts the flag protocol details from all the calls.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 fs/compat.c                 |    6 +++---
 fs/eventpoll.c              |    3 +--
 fs/select.c                 |    4 ++--
 include/linux/thread_info.h |   15 ++++++++++++++-
 kernel/compat.c             |    3 +--
 kernel/signal.c             |    2 +-
 6 files changed, 22 insertions(+), 11 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 2ce4456..9964d54 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1720,7 +1720,7 @@ sticky:
 		if (sigmask) {
 			memcpy(&current->saved_sigmask, &sigsaved,
 					sizeof(sigsaved));
-			set_thread_flag(TIF_RESTORE_SIGMASK);
+			set_restore_sigmask();
 		}
 	} else if (sigmask)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
@@ -1791,7 +1791,7 @@ asmlinkage long compat_sys_ppoll(struct pollfd __user *ufds,
 		if (sigmask) {
 			memcpy(&current->saved_sigmask, &sigsaved,
 				sizeof(sigsaved));
-			set_thread_flag(TIF_RESTORE_SIGMASK);
+			set_restore_sigmask();
 		}
 		ret = -ERESTARTNOHAND;
 	} else if (sigmask)
@@ -2117,7 +2117,7 @@ asmlinkage long compat_sys_epoll_pwait(int epfd,
 		if (err == -EINTR) {
 			memcpy(&current->saved_sigmask, &sigsaved,
 			       sizeof(sigsaved));
-			set_thread_flag(TIF_RESTORE_SIGMASK);
+			set_restore_sigmask();
 		} else
 			sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 	}
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index a415f42..503ffa4 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1300,7 +1300,7 @@ asmlinkage long sys_epoll_pwait(int epfd, struct epoll_event __user *events,
 		if (error == -EINTR) {
 			memcpy(&current->saved_sigmask, &sigsaved,
 			       sizeof(sigsaved));
-			set_thread_flag(TIF_RESTORE_SIGMASK);
+			set_restore_sigmask();
 		} else
 			sigprocmask(SIG_SETMASK, &sigsaved, NULL);
 	}
@@ -1330,4 +1330,3 @@ static int __init eventpoll_init(void)
 	return 0;
 }
 fs_initcall(eventpoll_init);
-
diff --git a/fs/select.c b/fs/select.c
index 5633fe9..bbd351c 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -498,7 +498,7 @@ sticky:
 		if (sigmask) {
 			memcpy(&current->saved_sigmask, &sigsaved,
 					sizeof(sigsaved));
-			set_thread_flag(TIF_RESTORE_SIGMASK);
+			set_restore_sigmask();
 		}
 	} else if (sigmask)
 		sigprocmask(SIG_SETMASK, &sigsaved, NULL);
@@ -805,7 +805,7 @@ asmlinkage long sys_ppoll(struct pollfd __user *ufds, unsigned int nfds,
 		if (sigmask) {
 			memcpy(&current->saved_sigmask, &sigsaved,
 					sizeof(sigsaved));
-			set_thread_flag(TIF_RESTORE_SIGMASK);
+			set_restore_sigmask();
 		}
 		ret = -ERESTARTNOHAND;
 	} else if (sigmask)
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 421323e..d82c073 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -80,6 +80,19 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 #define set_need_resched()	set_thread_flag(TIF_NEED_RESCHED)
 #define clear_need_resched()	clear_thread_flag(TIF_NEED_RESCHED)
 
-#endif
+#ifdef TIF_RESTORE_SIGMASK
+/**
+ * set_restore_sigmask() - make sure saved_sigmask processing gets done
+ *
+ * This sets TIF_RESTORE_SIGMASK and ensures that the arch signal code
+ * will run before returning to user mode, to process the flag.
+ */
+static inline void set_restore_sigmask(void)
+{
+	set_thread_flag(TIF_RESTORE_SIGMASK);
+}
+#endif	/* TIF_RESTORE_SIGMASK */
+
+#endif	/* __KERNEL__ */
 
 #endif /* _LINUX_THREAD_INFO_H */
diff --git a/kernel/compat.c b/kernel/compat.c
index 5f0e201..a830c84 100644
--- a/kernel/compat.c
+++ b/kernel/compat.c
@@ -899,7 +899,7 @@ asmlinkage long compat_sys_rt_sigsuspend(compat_sigset_t __user *unewset, compat
 
 	current->state = TASK_INTERRUPTIBLE;
 	schedule();
-	set_thread_flag(TIF_RESTORE_SIGMASK);
+	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
 #endif /* __ARCH_WANT_COMPAT_SYS_RT_SIGSUSPEND */
@@ -1081,4 +1081,3 @@ compat_sys_sysinfo(struct compat_sysinfo __user *info)
 
 	return 0;
 }
-
diff --git a/kernel/signal.c b/kernel/signal.c
index 6af1210..977b3cd 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2610,7 +2610,7 @@ asmlinkage long sys_rt_sigsuspend(sigset_t __user *unewset, size_t sigsetsize)
 
 	current->state = TASK_INTERRUPTIBLE;
 	schedule();
-	set_thread_flag(TIF_RESTORE_SIGMASK);
+	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
 #endif /* __ARCH_WANT_SYS_RT_SIGSUSPEND */

[PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Roland McGrath]

Set TIF_SIGPENDING in set_restore_sigmask.  This lets arch code take
TIF_RESTORE_SIGMASK out of the set of bits that will be noticed on
return to user mode.  On some machines those bits are scarce, and we
can free this unneeded one up for other uses.

It is probably the case that TIF_SIGPENDING is always set anyway
everywhere set_restore_sigmask() is used.  But this is some cheap
paranoia in case there is an arcane case where it might not be.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 include/linux/thread_info.h |    8 +++++++-
 1 files changed, 7 insertions(+), 1 deletions(-)

diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index d82c073..4a89477 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -85,11 +85,17 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
  * set_restore_sigmask() - make sure saved_sigmask processing gets done
  *
  * This sets TIF_RESTORE_SIGMASK and ensures that the arch signal code
- * will run before returning to user mode, to process the flag.
+ * will run before returning to user mode, to process the flag.  For
+ * all callers, TIF_SIGPENDING is already set or it's no harm to set
+ * it.  TIF_RESTORE_SIGMASK need not be in the set of bits that the
+ * arch code will notice on return to user mode, in case those bits
+ * are scarce.  We set TIF_SIGPENDING here to ensure that the arch
+ * signal code always gets run when TIF_RESTORE_SIGMASK is set.
  */
 static inline void set_restore_sigmask(void)
 {
 	set_thread_flag(TIF_RESTORE_SIGMASK);
+	set_thread_flag(TIF_SIGPENDING);
 }
 #endif	/* TIF_RESTORE_SIGMASK */
 

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Linus Torvalds]

On Fri, 28 Mar 2008, Roland McGrath wrote:
>
> Set TIF_SIGPENDING in set_restore_sigmask.  This lets arch code take
> TIF_RESTORE_SIGMASK out of the set of bits that will be noticed on
> return to user mode.  On some machines those bits are scarce, and we
> can free this unneeded one up for other uses.

Hmm. That probably means that TIF_RESTORE_SIGMASK shouldn't be a "TIF" 
flag at all, but a "TS" ("thread status") flag.

The TS flags are faster, because they are thread-synchronous and do not 
need atomic accesses (ie they are purely thread-local in setting, testing 
and clearing).

Of course, it may well not be worth it. Unlike the TIF flags, the TS flags 
have been architecture-specific and I don't think all architectures even 
do them (x86 uses them for FP state bits and stuff like that).

I guess TIF_RESTORE_SIGMASK is never *so* performance-critical that we'd 
care about the difference between a single cycle (approx) for a non-atomic 
"or" into memory and an atomic bitop (~50 cycles or so). 

			Linus

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Roland McGrath]

> Hmm. That probably means that TIF_RESTORE_SIGMASK shouldn't be a "TIF" 
> flag at all, but a "TS" ("thread status") flag.

Yes.  It only ever needs to be set or tested by the thread itself.
It's used in an entirely synchronous fashion and never from
interrupts or anything like that.  

I didn't tackle changing this at the same time because of the can of
worms you cited (and because of doing one thing at a time).

It could be a PF_* too, I suppose.  There aren't too many of those
bits free, but it would have the advantage of being a place for an
arch that doesn't store any TS_* bits anywhere.

Since acting on the flag is in arch signal code anyway, it makes some
sense to let the arch define how it gets that to happen.  I'll send
some follow-on patches that change the conditionals to use #ifdef
HAVE_SET_RESTORE_SIGMASK.  Then an arch can define that to provide
its own set_restore_sigmask() instead of TIF_RESTORE_SIGMASK, using a
TS_* flag or whatever it likes.  It will make for an easy slow
transition by each arch whenever it decides it cares about the cycles.

> I guess TIF_RESTORE_SIGMASK is never *so* performance-critical that we'd 
> care about the difference between a single cycle (approx) for a non-atomic 
> "or" into memory and an atomic bitop (~50 cycles or so). 

I doubt it is.  There is always about to be a whole lot more overhead
for taking siglock and all manner of synchronizing hooey, anyway.

OTOH, my patch here just added a second set_thread_flag there purely
for paranoia (it's probably never needed, but I'd have to be Oleg to
be sure ;-).  So even if 50 doesn't much matter, 51 sounds better
than 100.


Thanks,
Roland

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Linus Torvalds]

On Fri, 28 Mar 2008, Roland McGrath wrote:
> 
> It could be a PF_* too, I suppose.  There aren't too many of those
> bits free, but it would have the advantage of being a place for an
> arch that doesn't store any TS_* bits anywhere.

Yeah, I guess PF_ would be a bit more regular. Maybe we should even try to 
avoid the use of TS_ in x86, and turn it into PF_. There are probably bad 
historical reasons for the duplication of capabilities.

> Since acting on the flag is in arch signal code anyway, it makes some
> sense to let the arch define how it gets that to happen.  I'll send
> some follow-on patches that change the conditionals to use #ifdef
> HAVE_SET_RESTORE_SIGMASK.

Let's see if it matters first. No reason to add another arch-specific 
thing if nobody can even measure this thing, and from a quick look it 
seems like every RESTORE_SIGMASK user is basically an error path for a 
system call. Those few extra cycles really won't be noticeable, we almost 
certainly have better things we could use our energy on.

So never mind. I think your series is fine, and my TS_ idea doesn't really 
look like it's worth it (and using PF_ sounds a bit more palatable since 
we could do it with existing infrastructure, but a quick grep shows that 
there's more users of test_thread_flag(TIF_RESTORE_SIGMASK) than I would 
have expected (and the *testing* is equally cheap for atomic and thread- 
synchronous fields, so that's not a performance issue).

			Linus

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Roland McGrath]

> So never mind. [...]

Well fooey on you, 'cause I wrote it already.  So I'm a'postin'!

[PATCH 1/2] HAVE_SET_RESTORE_SIGMASK

[Roland McGrath]

Change all the #ifdef TIF_RESTORE_SIGMASK conditionals in non-arch
code to #ifdef HAVE_SET_RESTORE_SIGMASK.  If arch code defines it
first, the generic set_restore_sigmask() using TIF_RESTORE_SIGMASK
is not defined.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 fs/compat.c                 |    8 ++++----
 fs/eventpoll.c              |    4 ++--
 fs/select.c                 |    8 ++++----
 include/linux/sched.h       |    2 +-
 include/linux/thread_info.h |   10 ++++++++--
 5 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 9964d54..139dc93 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1634,7 +1634,7 @@ sticky:
 	return ret;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long compat_sys_pselect7(int n, compat_ulong_t __user *inp,
 	compat_ulong_t __user *outp, compat_ulong_t __user *exp,
 	struct compat_timespec __user *tsp, compat_sigset_t __user *sigmask,
@@ -1825,7 +1825,7 @@ sticky:
 
 	return ret;
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 #if defined(CONFIG_NFSD) || defined(CONFIG_NFSD_MODULE)
 /* Stuff for NFS server syscalls... */
@@ -2080,7 +2080,7 @@ long asmlinkage compat_sys_nfsservctl(int cmd, void *notused, void *notused2)
 
 #ifdef CONFIG_EPOLL
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long compat_sys_epoll_pwait(int epfd,
 			struct compat_epoll_event __user *events,
 			int maxevents, int timeout,
@@ -2124,7 +2124,7 @@ asmlinkage long compat_sys_epoll_pwait(int epfd,
 
 	return err;
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 #endif /* CONFIG_EPOLL */
 
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 503ffa4..e55451e 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1262,7 +1262,7 @@ error_return:
 	return error;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 
 /*
  * Implement the event wait interface for the eventpoll file. It is the kernel
@@ -1308,7 +1308,7 @@ asmlinkage long sys_epoll_pwait(int epfd, struct epoll_event __user *events,
 	return error;
 }
 
-#endif /* #ifdef TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 static int __init eventpoll_init(void)
 {
diff --git a/fs/select.c b/fs/select.c
index bbd351c..c06fe0b 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -425,7 +425,7 @@ sticky:
 	return ret;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long sys_pselect7(int n, fd_set __user *inp, fd_set __user *outp,
 		fd_set __user *exp, struct timespec __user *tsp,
 		const sigset_t __user *sigmask, size_t sigsetsize)
@@ -528,7 +528,7 @@ asmlinkage long sys_pselect6(int n, fd_set __user *inp, fd_set __user *outp,
 
 	return sys_pselect7(n, inp, outp, exp, tsp, up, sigsetsize);
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 struct poll_list {
 	struct poll_list *next;
@@ -759,7 +759,7 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 	return ret;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long sys_ppoll(struct pollfd __user *ufds, unsigned int nfds,
 	struct timespec __user *tsp, const sigset_t __user *sigmask,
 	size_t sigsetsize)
@@ -839,4 +839,4 @@ asmlinkage long sys_ppoll(struct pollfd __user *ufds, unsigned int nfds,
 
 	return ret;
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6a1e7af..6c275e8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1144,7 +1144,7 @@ struct task_struct {
 	struct sighand_struct *sighand;
 
 	sigset_t blocked, real_blocked;
-	sigset_t saved_sigmask;		/* To be restored with TIF_RESTORE_SIGMASK */
+	sigset_t saved_sigmask;	/* restored if set_restore_sigmask() was used */
 	struct sigpending pending;
 
 	unsigned long sas_ss_sp;
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 4a89477..dbf5daa 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -80,7 +80,13 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 #define set_need_resched()	set_thread_flag(TIF_NEED_RESCHED)
 #define clear_need_resched()	clear_thread_flag(TIF_NEED_RESCHED)
 
-#ifdef TIF_RESTORE_SIGMASK
+#if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
+/*
+ * An arch can define its own version of set_restore_sigmask() to get the
+ * job done however works, with or without TIF_RESTORE_SIGMASK.
+ */
+#define HAVE_SET_RESTORE_SIGMASK	1
+
 /**
  * set_restore_sigmask() - make sure saved_sigmask processing gets done
  *
@@ -97,7 +103,7 @@ static inline void set_restore_sigmask(void)
 	set_thread_flag(TIF_RESTORE_SIGMASK);
 	set_thread_flag(TIF_SIGPENDING);
 }
-#endif	/* TIF_RESTORE_SIGMASK */
+#endif	/* TIF_RESTORE_SIGMASK && !HAVE_SET_RESTORE_SIGMASK */
 
 #endif	/* __KERNEL__ */
 

Re: [PATCH 1/2] HAVE_SET_RESTORE_SIGMASK

[David Woodhouse]

On Fri, 2008-03-28 at 20:11 -0700, Roland McGrath wrote:
> Change all the #ifdef TIF_RESTORE_SIGMASK conditionals in non-arch
> code to #ifdef HAVE_SET_RESTORE_SIGMASK.

That ifdef was only supposed to be a temporary thing until all
architectures had implemented TIF_RESTORE_SIGMASK anyway.

It looks like only ARM, v850 and m68k which are still missing it; if
those three architectures can catch up, then hopefully it can die off
completely quite soon.

-- 
dwmw2

Re: [PATCH 1/2] HAVE_SET_RESTORE_SIGMASK

[Russell King]

On Wed, Apr 09, 2008 at 12:45:32PM +0100, David Woodhouse wrote:
> On Fri, 2008-03-28 at 20:11 -0700, Roland McGrath wrote:
> > Change all the #ifdef TIF_RESTORE_SIGMASK conditionals in non-arch
> > code to #ifdef HAVE_SET_RESTORE_SIGMASK.
> 
> That ifdef was only supposed to be a temporary thing until all
> architectures had implemented TIF_RESTORE_SIGMASK anyway.
> 
> It looks like only ARM, v850 and m68k which are still missing it; if
> those three architectures can catch up, then hopefully it can die off
> completely quite soon.

Well, we don't have the pselect/ppoll/epoll_wait syscalls and there's
been no demand for them, so I don't particularly see the point of
going to the trouble of adding support for something no one's
interested in using.

I was going to suggest defining TIF_RESTORE_SIGMASK to zero to eliminate
some of the code (and the ifdefs), but unfortunately its a bit position
not a bitmask so that won't work.

-- 
Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

[PATCH 1/2] HAVE_SET_RESTORE_SIGMASK

[Roland McGrath]

Change all the #ifdef TIF_RESTORE_SIGMASK conditionals in non-arch
code to #ifdef HAVE_SET_RESTORE_SIGMASK.  If arch code defines it
first, the generic set_restore_sigmask() using TIF_RESTORE_SIGMASK
is not defined.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 fs/compat.c                 |    8 ++++----
 fs/eventpoll.c              |    4 ++--
 fs/select.c                 |    8 ++++----
 include/linux/sched.h       |    2 +-
 include/linux/thread_info.h |   10 ++++++++--
 5 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/fs/compat.c b/fs/compat.c
index 9964d54..139dc93 100644
--- a/fs/compat.c
+++ b/fs/compat.c
@@ -1634,7 +1634,7 @@ sticky:
 	return ret;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long compat_sys_pselect7(int n, compat_ulong_t __user *inp,
 	compat_ulong_t __user *outp, compat_ulong_t __user *exp,
 	struct compat_timespec __user *tsp, compat_sigset_t __user *sigmask,
@@ -1825,7 +1825,7 @@ sticky:
 
 	return ret;
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 #if defined(CONFIG_NFSD) || defined(CONFIG_NFSD_MODULE)
 /* Stuff for NFS server syscalls... */
@@ -2080,7 +2080,7 @@ long asmlinkage compat_sys_nfsservctl(int cmd, void *notused, void *notused2)
 
 #ifdef CONFIG_EPOLL
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long compat_sys_epoll_pwait(int epfd,
 			struct compat_epoll_event __user *events,
 			int maxevents, int timeout,
@@ -2124,7 +2124,7 @@ asmlinkage long compat_sys_epoll_pwait(int epfd,
 
 	return err;
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 #endif /* CONFIG_EPOLL */
 
diff --git a/fs/eventpoll.c b/fs/eventpoll.c
index 503ffa4..e55451e 100644
--- a/fs/eventpoll.c
+++ b/fs/eventpoll.c
@@ -1262,7 +1262,7 @@ error_return:
 	return error;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 
 /*
  * Implement the event wait interface for the eventpoll file. It is the kernel
@@ -1308,7 +1308,7 @@ asmlinkage long sys_epoll_pwait(int epfd, struct epoll_event __user *events,
 	return error;
 }
 
-#endif /* #ifdef TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 static int __init eventpoll_init(void)
 {
diff --git a/fs/select.c b/fs/select.c
index bbd351c..c06fe0b 100644
--- a/fs/select.c
+++ b/fs/select.c
@@ -425,7 +425,7 @@ sticky:
 	return ret;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long sys_pselect7(int n, fd_set __user *inp, fd_set __user *outp,
 		fd_set __user *exp, struct timespec __user *tsp,
 		const sigset_t __user *sigmask, size_t sigsetsize)
@@ -528,7 +528,7 @@ asmlinkage long sys_pselect6(int n, fd_set __user *inp, fd_set __user *outp,
 
 	return sys_pselect7(n, inp, outp, exp, tsp, up, sigsetsize);
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
 
 struct poll_list {
 	struct poll_list *next;
@@ -759,7 +759,7 @@ asmlinkage long sys_poll(struct pollfd __user *ufds, unsigned int nfds,
 	return ret;
 }
 
-#ifdef TIF_RESTORE_SIGMASK
+#ifdef HAVE_SET_RESTORE_SIGMASK
 asmlinkage long sys_ppoll(struct pollfd __user *ufds, unsigned int nfds,
 	struct timespec __user *tsp, const sigset_t __user *sigmask,
 	size_t sigsetsize)
@@ -839,4 +839,4 @@ asmlinkage long sys_ppoll(struct pollfd __user *ufds, unsigned int nfds,
 
 	return ret;
 }
-#endif /* TIF_RESTORE_SIGMASK */
+#endif /* HAVE_SET_RESTORE_SIGMASK */
diff --git a/include/linux/sched.h b/include/linux/sched.h
index 6a1e7af..6c275e8 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -1144,7 +1144,7 @@ struct task_struct {
 	struct sighand_struct *sighand;
 
 	sigset_t blocked, real_blocked;
-	sigset_t saved_sigmask;		/* To be restored with TIF_RESTORE_SIGMASK */
+	sigset_t saved_sigmask;	/* restored if set_restore_sigmask() was used */
 	struct sigpending pending;
 
 	unsigned long sas_ss_sp;
diff --git a/include/linux/thread_info.h b/include/linux/thread_info.h
index 4a89477..dbf5daa 100644
--- a/include/linux/thread_info.h
+++ b/include/linux/thread_info.h
@@ -80,7 +80,13 @@ static inline int test_ti_thread_flag(struct thread_info *ti, int flag)
 #define set_need_resched()	set_thread_flag(TIF_NEED_RESCHED)
 #define clear_need_resched()	clear_thread_flag(TIF_NEED_RESCHED)
 
-#ifdef TIF_RESTORE_SIGMASK
+#if defined TIF_RESTORE_SIGMASK && !defined HAVE_SET_RESTORE_SIGMASK
+/*
+ * An arch can define its own version of set_restore_sigmask() to get the
+ * job done however works, with or without TIF_RESTORE_SIGMASK.
+ */
+#define HAVE_SET_RESTORE_SIGMASK	1
+
 /**
  * set_restore_sigmask() - make sure saved_sigmask processing gets done
  *
@@ -97,7 +103,7 @@ static inline void set_restore_sigmask(void)
 	set_thread_flag(TIF_RESTORE_SIGMASK);
 	set_thread_flag(TIF_SIGPENDING);
 }
-#endif	/* TIF_RESTORE_SIGMASK */
+#endif	/* TIF_RESTORE_SIGMASK && !HAVE_SET_RESTORE_SIGMASK */
 
 #endif	/* __KERNEL__ */
 

[PATCH 2/2] x86 TS_RESTORE_SIGMASK

[Roland McGrath]

Replace TIF_RESTORE_SIGMASK with TS_RESTORE_SIGMASK and define
our own set_restore_sigmask() function.  This saves the costly
SMP-safe set_bit operation, which we do not need for the sigmask
flag since TIF_SIGPENDING always has to be set too.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/x86/ia32/ia32_signal.c      |    2 +-
 arch/x86/kernel/signal_32.c      |   19 ++++++++++---------
 arch/x86/kernel/signal_64.c      |   16 +++++++++-------
 include/asm-x86/thread_info_32.h |   13 +++++++++++--
 include/asm-x86/thread_info_64.h |   13 +++++++++++--
 5 files changed, 42 insertions(+), 21 deletions(-)

diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 101b4b8..b7a531f 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -128,7 +128,7 @@ asmlinkage long sys32_sigsuspend(int history0, int history1, old_sigset_t mask)
 
 	current->state = TASK_INTERRUPTIBLE;
 	schedule();
-	set_thread_flag(TIF_RESTORE_SIGMASK);
+	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
 
diff --git a/arch/x86/kernel/signal_32.c b/arch/x86/kernel/signal_32.c
index a06fa55..f8fbc20 100644
--- a/arch/x86/kernel/signal_32.c
+++ b/arch/x86/kernel/signal_32.c
@@ -45,7 +45,7 @@ sys_sigsuspend(int history0, int history1, old_sigset_t mask)
 
 	current->state = TASK_INTERRUPTIBLE;
 	schedule();
-	set_thread_flag(TIF_RESTORE_SIGMASK);
+	set_restore_sigmask();
 	return -ERESTARTNOHAND;
 }
 
@@ -591,7 +591,7 @@ static void do_signal(struct pt_regs *regs)
 	if (!user_mode(regs))
 		return;
 
-	if (test_thread_flag(TIF_RESTORE_SIGMASK))
+	if (current_thread_info()->status & TS_RESTORE_SIGMASK)
 		oldset = &current->saved_sigmask;
 	else
 		oldset = &current->blocked;
@@ -608,12 +608,13 @@ static void do_signal(struct pt_regs *regs)
 
 		/* Whee!  Actually deliver the signal.  */
 		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
-			/* a signal was successfully delivered; the saved
+			/*
+			 * A signal was successfully delivered; the saved
 			 * sigmask will have been stored in the signal frame,
 			 * and will be restored by sigreturn, so we can simply
-			 * clear the TIF_RESTORE_SIGMASK flag */
-			if (test_thread_flag(TIF_RESTORE_SIGMASK))
-				clear_thread_flag(TIF_RESTORE_SIGMASK);
+			 * clear the TS_RESTORE_SIGMASK flag.
+			 */
+			current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		}
 
 		return;
@@ -639,8 +640,8 @@ static void do_signal(struct pt_regs *regs)
 
 	/* if there's no signal to deliver, we just put the saved sigmask
 	 * back */
-	if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
-		clear_thread_flag(TIF_RESTORE_SIGMASK);
+	if (current_thread_info()->status & TS_RESTORE_SIGMASK) {
+		current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
 	}
 }
@@ -660,7 +661,7 @@ void do_notify_resume(struct pt_regs *regs, void *_unused,
 	}
 
 	/* deal with pending signal delivery */
-	if (thread_info_flags & (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK))
+	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs);
 
 	if (thread_info_flags & _TIF_HRTICK_RESCHED)
diff --git a/arch/x86/kernel/signal_64.c b/arch/x86/kernel/signal_64.c
index 016413b..fc97dec 100644
--- a/arch/x86/kernel/signal_64.c
+++ b/arch/x86/kernel/signal_64.c
@@ -443,7 +443,7 @@ static void do_signal(struct pt_regs *regs)
 	if (!user_mode(regs))
 		return;
 
-	if (test_thread_flag(TIF_RESTORE_SIGMASK))
+	if (current_thread_info()->status & TS_RESTORE_SIGMASK)
 		oldset = &current->saved_sigmask;
 	else
 		oldset = &current->blocked;
@@ -460,11 +460,13 @@ static void do_signal(struct pt_regs *regs)
 
 		/* Whee!  Actually deliver the signal.  */
 		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
-			/* a signal was successfully delivered; the saved
+			/*
+			 * A signal was successfully delivered; the saved
 			 * sigmask will have been stored in the signal frame,
 			 * and will be restored by sigreturn, so we can simply
-			 * clear the TIF_RESTORE_SIGMASK flag */
-			clear_thread_flag(TIF_RESTORE_SIGMASK);
+			 * clear the TS_RESTORE_SIGMASK flag.
+			 */
+			current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		}
 		return;
 	}
@@ -490,8 +492,8 @@ static void do_signal(struct pt_regs *regs)
 
 	/* if there's no signal to deliver, we just put the saved sigmask
 	   back. */
-	if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
-		clear_thread_flag(TIF_RESTORE_SIGMASK);
+	if (current_thread_info()->status & TS_RESTORE_SIGMASK) {
+		current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
 	}
 }
@@ -517,7 +519,7 @@ do_notify_resume(struct pt_regs *regs, void *unused, __u32 thread_info_flags)
 #endif /* CONFIG_X86_MCE */
 
 	/* deal with pending signal delivery */
-	if (thread_info_flags & (_TIF_SIGPENDING|_TIF_RESTORE_SIGMASK))
+	if (thread_info_flags & _TIF_SIGPENDING)
 		do_signal(regs);
 
 	if (thread_info_flags & _TIF_HRTICK_RESCHED)
diff --git a/include/asm-x86/thread_info_32.h b/include/asm-x86/thread_info_32.h
index 5bd5082..4c4fd4e 100644
--- a/include/asm-x86/thread_info_32.h
+++ b/include/asm-x86/thread_info_32.h
@@ -131,7 +131,6 @@ static inline struct thread_info *current_thread_info(void)
 #define TIF_SYSCALL_EMU		5	/* syscall emulation active */
 #define TIF_SYSCALL_AUDIT	6	/* syscall auditing active */
 #define TIF_SECCOMP		7	/* secure computing */
-#define TIF_RESTORE_SIGMASK	8	/* restore signal mask in do_signal() */
 #define TIF_HRTICK_RESCHED	9	/* reprogram hrtick timer */
 #define TIF_MEMDIE		16
 #define TIF_DEBUG		17	/* uses debug registers */
@@ -151,7 +150,6 @@ static inline struct thread_info *current_thread_info(void)
 #define _TIF_SYSCALL_EMU	(1<<TIF_SYSCALL_EMU)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1<<TIF_SECCOMP)
-#define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_HRTICK_RESCHED	(1<<TIF_HRTICK_RESCHED)
 #define _TIF_DEBUG		(1<<TIF_DEBUG)
 #define _TIF_IO_BITMAP		(1<<TIF_IO_BITMAP)
@@ -186,9 +184,20 @@ static inline struct thread_info *current_thread_info(void)
  */
 #define TS_USEDFPU		0x0001	/* FPU was used by this task this quantum (SMP) */
 #define TS_POLLING		0x0002	/* True if in idle loop and not sleeping */
+#define TS_RESTORE_SIGMASK	0x0004	/* restore signal mask in do_signal() */
 
 #define tsk_is_polling(t) (task_thread_info(t)->status & TS_POLLING)
 
+#ifndef __ASSEMBLY__
+#define HAVE_SET_RESTORE_SIGMASK	1
+static inline void set_restore_sigmask(void)
+{
+	struct thread_info *ti = current_thread_info();
+	ti->status |= TS_RESTORE_SIGMASK;
+	set_bit(TIF_SIGPENDING, &ti->flags);
+}
+#endif	/* !__ASSEMBLY__ */
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_THREAD_INFO_H */
diff --git a/include/asm-x86/thread_info_64.h b/include/asm-x86/thread_info_64.h
index 6c9b214..ead94d5 100644
--- a/include/asm-x86/thread_info_64.h
+++ b/include/asm-x86/thread_info_64.h
@@ -110,7 +110,6 @@ static inline struct thread_info *stack_thread_info(void)
 #define TIF_IRET		5	/* force IRET */
 #define TIF_SYSCALL_AUDIT	7	/* syscall auditing active */
 #define TIF_SECCOMP		8	/* secure computing */
-#define TIF_RESTORE_SIGMASK	9	/* restore signal mask in do_signal */
 #define TIF_MCE_NOTIFY		10	/* notify userspace of an MCE */
 #define TIF_HRTICK_RESCHED	11	/* reprogram hrtick timer */
 /* 16 free */
@@ -133,7 +132,6 @@ static inline struct thread_info *stack_thread_info(void)
 #define _TIF_IRET		(1<<TIF_IRET)
 #define _TIF_SYSCALL_AUDIT	(1<<TIF_SYSCALL_AUDIT)
 #define _TIF_SECCOMP		(1<<TIF_SECCOMP)
-#define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)
 #define _TIF_MCE_NOTIFY		(1<<TIF_MCE_NOTIFY)
 #define _TIF_HRTICK_RESCHED	(1<<TIF_HRTICK_RESCHED)
 #define _TIF_IA32		(1<<TIF_IA32)
@@ -174,9 +172,20 @@ static inline struct thread_info *stack_thread_info(void)
 #define TS_USEDFPU		0x0001	/* FPU was used by this task this quantum (SMP) */
 #define TS_COMPAT		0x0002	/* 32bit syscall active */
 #define TS_POLLING		0x0004	/* true if in idle loop and not sleeping */
+#define TS_RESTORE_SIGMASK	0x0008	/* restore signal mask in do_signal() */
 
 #define tsk_is_polling(t) (task_thread_info(t)->status & TS_POLLING)
 
+#ifndef __ASSEMBLY__
+#define HAVE_SET_RESTORE_SIGMASK	1
+static inline void set_restore_sigmask(void)
+{
+	struct thread_info *ti = current_thread_info();
+	ti->status |= TS_RESTORE_SIGMASK;
+	set_bit(TIF_SIGPENDING, &ti->flags);
+}
+#endif	/* !__ASSEMBLY__ */
+
 #endif /* __KERNEL__ */
 
 #endif /* _ASM_THREAD_INFO_H */

Re: [PATCH 2/2] x86 TS_RESTORE_SIGMASK

[Ingo Molnar]

* Roland McGrath <roland@redhat.com> wrote:

> Replace TIF_RESTORE_SIGMASK with TS_RESTORE_SIGMASK and define our own 
> set_restore_sigmask() function.  This saves the costly SMP-safe 
> set_bit operation, which we do not need for the sigmask flag since 
> TIF_SIGPENDING always has to be set too.
> 
> Signed-off-by: Roland McGrath <roland@redhat.com>
> ---
>  arch/x86/ia32/ia32_signal.c      |    2 +-
>  arch/x86/kernel/signal_32.c      |   19 ++++++++++---------
>  arch/x86/kernel/signal_64.c      |   16 +++++++++-------
>  include/asm-x86/thread_info_32.h |   13 +++++++++++--
>  include/asm-x86/thread_info_64.h |   13 +++++++++++--
>  5 files changed, 42 insertions(+), 21 deletions(-)

the x86 bits look nice - but i guess this all wants to go into 2.6.26 
via -mm (or linux-next), due to its cross-arch nature?

	Ingo

Re: [PATCH 2/2] x86 TS_RESTORE_SIGMASK

[Roland McGrath]

> the x86 bits look nice - but i guess this all wants to go into 2.6.26 
> via -mm (or linux-next), due to its cross-arch nature?

Yes, I would not suggest this change for 2.6.25 at this late date.


Thanks,
Roland

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Paul Mackerras]

Linus Torvalds writes:

> Of course, it may well not be worth it. Unlike the TIF flags, the TS flags 
> have been architecture-specific and I don't think all architectures even 
> do them (x86 uses them for FP state bits and stuff like that).

Powerpc has a "local_flags" field.  I don't mind changing it if we are
going to have a consistent name across architectures, but "status" (as
x86 uses) seems a bit nondescript to me.

Paul.

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Oleg Nesterov]

Roland, Linus, I'm sorry for being offtopic, but there is something
I can't understand from the very beginning, when TIF_RESTORE_SIGMASK
was introduced.

On 03/28, Linus Torvalds wrote:
> 
> On Fri, 28 Mar 2008, Roland McGrath wrote:
> >
> > Set TIF_SIGPENDING in set_restore_sigmask.  This lets arch code take
> > TIF_RESTORE_SIGMASK out of the set of bits that will be noticed on
> > return to user mode.  On some machines those bits are scarce, and we
> > can free this unneeded one up for other uses.
> 
> Hmm. That probably means that TIF_RESTORE_SIGMASK shouldn't be a "TIF" 
> flag at all,

Yes!

> but a "TS" ("thread status") flag.

Why do we need any flag? It looks a bit ugly. Isn't it better to introduce
the new magic ERESTART_XXX which means ERESTARTNOHAND + restore-sigmask ?

We only need this flag as an implicit parameter to the arch dependent do_signal()
which we can't call directly, and thus it must imply TIF_SIGPENDING, and it
is not valid after do_signal() (should be cleared). This all looks like
ERESTART_ magic, why should we add something else ?

See also http://marc.info/?l=linux-kernel&m=113734458516136

Of course, probably it is too late to change the implementation even if
I am right, the question is: what I am missed?

Oleg.

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Linus Torvalds]

On Tue, 8 Apr 2008, Oleg Nesterov wrote:
> 
> We only need this flag as an implicit parameter to the arch dependent do_signal()
> which we can't call directly, and thus it must imply TIF_SIGPENDING, and it
> is not valid after do_signal() (should be cleared). This all looks like
> ERESTART_ magic, why should we add something else ?

I think you're right. I didn't look at the actual code-paths, but my gut 
feel says "yes, TIF_RESTORE_SIGMASK should actually have been 
-ERESTARTSIGRESTORE". That sounds like the right thing to do.

		Linus

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Roland McGrath]

Off hand, I concur with you and Linus.  I wasn't involved in the
introduction of TIF_RESTORE_SIGMASK and never thought too much about
it before.  In my recent changes I only considered the issue of
getting it out of _TIF_WORK_MASK et al to free up the low bit, and
didn't contemplate the structure of the implementation beyond that.

I don't think it's "too late" to change anything.  (It's never too
late!  It just might take a while to make a change in a safe and
orderly fashion for all the arch's.)  After my patch series, the
details are fully in the arch's corner.  It should be straightforward
to convert one at a time to use regs->return_register = -ERESTARTRESTOREMASK
to implement set_restore_sigmask() if you want to tackle it.


Thanks,
Roland

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[David Woodhouse]

On Tue, 2008-04-08 at 15:35 +0400, Oleg Nesterov wrote:
> Why do we need any flag? It looks a bit ugly. Isn't it better to introduce
> the new magic ERESTART_XXX which means ERESTARTNOHAND + restore-sigmask ?
> 
> We only need this flag as an implicit parameter to the arch dependent do_signal()
> which we can't call directly, and thus it must imply TIF_SIGPENDING, and it
> is not valid after do_signal() (should be cleared). This all looks like
> ERESTART_ magic, why should we add something else ?
> 
> See also http://marc.info/?l=linux-kernel&m=113734458516136
> 
> Of course, probably it is too late to change the implementation even if
> I am right, the question is: what I am missed?

Q: When ppoll() is interrupted by a signal, what signal mask should be
active when the signal handler is active?

I believe that the signal handler should run with the temporary sigmask
which was set by ppoll(), and the original sigmask should be restored
only when the handler completes -- and that's what we achieve with
TIF_RESTORE_SIGMASK.

So a signal which was originally enabled but is temporarily disabled by
the mask passed to ppoll() will not be able to interrupt the handler for
the signal which interrupted ppoll().

Your version will restore the original signal mask _before_ invoking the
signal handler which interrupted ppoll() -- which I believe is not the
intended semantics. And IIRC that was the whole point in implementing
ppoll() in kernel rather than trying to emulate it in userspace in the
first place.

-- 
dwmw2

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Oleg Nesterov]

On 04/09, David Woodhouse wrote:
>
> On Tue, 2008-04-08 at 15:35 +0400, Oleg Nesterov wrote:
> > Why do we need any flag? It looks a bit ugly. Isn't it better to introduce
> > the new magic ERESTART_XXX which means ERESTARTNOHAND + restore-sigmask ?
> > 
> > We only need this flag as an implicit parameter to the arch dependent do_signal()
> > which we can't call directly, and thus it must imply TIF_SIGPENDING, and it
> > is not valid after do_signal() (should be cleared). This all looks like
> > ERESTART_ magic, why should we add something else ?
> > 
> > See also http://marc.info/?l=linux-kernel&m=113734458516136
> > 
> > Of course, probably it is too late to change the implementation even if
> > I am right, the question is: what I am missed?
> 
> Q: When ppoll() is interrupted by a signal, what signal mask should be
> active when the signal handler is active?
> 
> I believe that the signal handler should run with the temporary sigmask
> which was set by ppoll(), and the original sigmask should be restored
> only when the handler completes -- and that's what we achieve with
> TIF_RESTORE_SIGMASK.

Yes sure.

> So a signal which was originally enabled but is temporarily disabled by
> the mask passed to ppoll() will not be able to interrupt the handler for
> the signal which interrupted ppoll().
> 
> Your version will restore the original signal mask _before_ invoking the
> signal handler which interrupted ppoll()

Why do you think so?

Please look at the "patch" below,

	--- arch/x86/kernel/signal_32.c	2008-02-15 16:58:38.000000000 +0300
	+++ -	2008-04-09 15:16:05.393510662 +0400
	@@ -526,10 +526,14 @@ handle_signal(unsigned long sig, siginfo
	 {
		int ret;
	 
	+	oldset = &current->blocked;
	+
		/* Are we from a system call? */
		if (regs->orig_ax >= 0) {
			/* If so, check system call restarting.. */
			switch (regs->ax) {
	+			case -ERESTART_XXX:
	+				oldset = &current->saved_sigmask;
				case -ERESTART_RESTARTBLOCK:
				case -ERESTARTNOHAND:
					regs->ax = -EINTR;

We also need a similar change in do_signal(). Now,

	--- fs/select.c	2008-02-15 16:59:15.000000000 +0300
	+++ -	2008-04-09 15:19:29.015991911 +0400
	@@ -805,9 +805,8 @@ asmlinkage long sys_ppoll(struct pollfd 
			if (sigmask) {
				memcpy(&current->saved_sigmask, &sigsaved,
						sizeof(sigsaved));
	-			set_thread_flag(TIF_RESTORE_SIGMASK);
			}
	-		ret = -ERESTARTNOHAND;
	+		ret = -ERESTART_XXX;
		} else if (sigmask)
			sigprocmask(SIG_SETMASK, &sigsaved, NULL);

Perhaps I missed something else, though. Not that I really think it worth
changing, but I'll try to make a proof of concept patch on Weekend, on top
of Roland's cleanups.

As I see it, the main disadvantage of ERESTART_ approach is that we need 2
new ERESTART_ codes, one for ERESTARTNOHAND, another for ERESTART_RESTARTBLOCK.
And yes, while I personally think this is "more clean", it is very subjective.

Oleg.

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Petr Tesarik]

On Wed, 2008-04-09 at 15:39 +0400, Oleg Nesterov wrote:
> On 04/09, David Woodhouse wrote:
> >
> > On Tue, 2008-04-08 at 15:35 +0400, Oleg Nesterov wrote:
> > > Why do we need any flag? It looks a bit ugly. Isn't it better to introduce
> > > the new magic ERESTART_XXX which means ERESTARTNOHAND + restore-sigmask ?
> > > 
> > > We only need this flag as an implicit parameter to the arch dependent do_signal()
> > > which we can't call directly, and thus it must imply TIF_SIGPENDING, and it
> > > is not valid after do_signal() (should be cleared). This all looks like
> > > ERESTART_ magic, why should we add something else ?
> > > 
> > > See also http://marc.info/?l=linux-kernel&m=113734458516136
> > > 
> > > Of course, probably it is too late to change the implementation even if
> > > I am right, the question is: what I am missed?
> > 
> > Q: When ppoll() is interrupted by a signal, what signal mask should be
> > active when the signal handler is active?
> > 
> > I believe that the signal handler should run with the temporary sigmask
> > which was set by ppoll(), and the original sigmask should be restored
> > only when the handler completes -- and that's what we achieve with
> > TIF_RESTORE_SIGMASK.
> 
> Yes sure.
> 
> > So a signal which was originally enabled but is temporarily disabled by
> > the mask passed to ppoll() will not be able to interrupt the handler for
> > the signal which interrupted ppoll().
> > 
> > Your version will restore the original signal mask _before_ invoking the
> > signal handler which interrupted ppoll()
> 
> Why do you think so?
> 
> Please look at the "patch" below,
> 
> 	--- arch/x86/kernel/signal_32.c	2008-02-15 16:58:38.000000000 +0300
> 	+++ -	2008-04-09 15:16:05.393510662 +0400
> 	@@ -526,10 +526,14 @@ handle_signal(unsigned long sig, siginfo
> 	 {
> 		int ret;
> 	 
> 	+	oldset = &current->blocked;
> 	+
> 		/* Are we from a system call? */
> 		if (regs->orig_ax >= 0) {
> 			/* If so, check system call restarting.. */
> 			switch (regs->ax) {
> 	+			case -ERESTART_XXX:
> 	+				oldset = &current->saved_sigmask;
> 				case -ERESTART_RESTARTBLOCK:
> 				case -ERESTARTNOHAND:
> 					regs->ax = -EINTR;
> 
> We also need a similar change in do_signal(). Now,
> 
> 	--- fs/select.c	2008-02-15 16:59:15.000000000 +0300
> 	+++ -	2008-04-09 15:19:29.015991911 +0400
> 	@@ -805,9 +805,8 @@ asmlinkage long sys_ppoll(struct pollfd 
> 			if (sigmask) {
> 				memcpy(&current->saved_sigmask, &sigsaved,
> 						sizeof(sigsaved));
> 	-			set_thread_flag(TIF_RESTORE_SIGMASK);
> 			}
> 	-		ret = -ERESTARTNOHAND;
> 	+		ret = -ERESTART_XXX;
> 		} else if (sigmask)
> 			sigprocmask(SIG_SETMASK, &sigsaved, NULL);
> 
> Perhaps I missed something else, though. Not that I really think it worth
> changing, but I'll try to make a proof of concept patch on Weekend, on top
> of Roland's cleanups.
> 
> As I see it, the main disadvantage of ERESTART_ approach is that we need 2
> new ERESTART_ codes, one for ERESTARTNOHAND, another for ERESTART_RESTARTBLOCK.
> And yes, while I personally think this is "more clean", it is very subjective.

One error code more or less, that's cheap. Thread flags are a much more
limited resource.

Just my two cents,
Petr Tesarik

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[David Woodhouse]

On Wed, 2008-04-09 at 15:39 +0400, Oleg Nesterov wrote:
> As I see it, the main disadvantage of ERESTART_ approach is that we need 2
> new ERESTART_ codes, one for ERESTARTNOHAND, another for ERESTART_RESTARTBLOCK.
> And yes, while I personally think this is "more clean", it is very subjective.

Subjective, yeah.... personally, I don't like using ERESTART_xxx much,
because you're _not_ necessarily restarting the system call. The
separate flag for TIF_RESTORE_SIGMASK (or TLF_RESTORE_SIGMASK) seems
cleaner to me -- especially once you observe that you need new codes for
ERESTART_xxx_AND_RESTORE_SIGMASK for each ERESTART_xxx that you might
want to use in conjunction with the flags.

But I don't really care much either, if you want to change it and get
the details right.

One of the supposed advantages of TIF_RESTORE_SIGMASK in the first
place, iirc, was that it allowed us to return a result code other than
-EINTR as _well_ as restoring the signal mask. But we don't actually
make use of that possibility now anyway.

-- 
dwmw2

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[Oleg Nesterov]

On 04/09, David Woodhouse wrote:
>
> One of the supposed advantages of TIF_RESTORE_SIGMASK in the first
> place, iirc, was that it allowed us to return a result code other than
> -EINTR as _well_ as restoring the signal mask.

Agreed, good point. ERESTART_ is not that flexible.

Somehow I assumed we will never need something "special" here, this is
not very clever.

Thanks to all!

Oleg.

Re: [PATCH 2/4] set_restore_sigmask TIF_SIGPENDING

[David Woodhouse]

On Wed, 2008-04-09 at 20:22 +0400, Oleg Nesterov wrote:
> On 04/09, David Woodhouse wrote:
> >
> > One of the supposed advantages of TIF_RESTORE_SIGMASK in the first
> > place, iirc, was that it allowed us to return a result code other than
> > -EINTR as _well_ as restoring the signal mask.
> 
> Agreed, good point. ERESTART_ is not that flexible.
> 
> Somehow I assumed we will never need something "special" here, this is
> not very clever.

Well, it's not clear that we _will_ need it to be so special. You could
perhaps argue that it's overengineering. It's just that at the time I
did it, I _thought_ I'd need it for ppoll().

It's only in later optimisations that I realised we only ever really
needed to use TIF_RESTORE_SIGMASK in the case where ppoll() or pselect()
was interrupted.

-- 
dwmw2

[PATCH 3/4] s390 renumber TIF_RESTORE_SIGMASK

[Roland McGrath]

TIF_RESTORE_SIGMASK no longer needs to be in the _TIF_WORK_* masks.
Those low bits are scarce, and are all used up now.
Renumber TIF_RESTORE_SIGMASK to free one up.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/s390/kernel/entry.S       |   14 +++++++-------
 arch/s390/kernel/entry64.S     |   12 ++++++------
 include/asm-s390/thread_info.h |    2 +-
 3 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/arch/s390/kernel/entry.S b/arch/s390/kernel/entry.S
index 6766e37..bdbb3bc 100644
--- a/arch/s390/kernel/entry.S
+++ b/arch/s390/kernel/entry.S
@@ -49,9 +49,9 @@ SP_ILC	     =	STACK_FRAME_OVERHEAD + __PT_ILC
 SP_TRAP      =	STACK_FRAME_OVERHEAD + __PT_TRAP
 SP_SIZE      =	STACK_FRAME_OVERHEAD + __PT_SIZE
 
-_TIF_WORK_SVC = (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK | _TIF_NEED_RESCHED | \
+_TIF_WORK_SVC = (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
 		 _TIF_MCCK_PENDING | _TIF_RESTART_SVC | _TIF_SINGLE_STEP )
-_TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK | _TIF_NEED_RESCHED | \
+_TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
 		 _TIF_MCCK_PENDING)
 
 STACK_SHIFT = PAGE_SHIFT + THREAD_ORDER
@@ -316,7 +316,7 @@ sysc_work:
 	bo	BASED(sysc_mcck_pending)
 	tm	__TI_flags+3(%r9),_TIF_NEED_RESCHED
 	bo	BASED(sysc_reschedule)
-	tm	__TI_flags+3(%r9),(_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)
+	tm	__TI_flags+3(%r9),_TIF_SIGPENDING
 	bnz	BASED(sysc_sigpending)
 	tm	__TI_flags+3(%r9),_TIF_RESTART_SVC
 	bo	BASED(sysc_restart)
@@ -342,7 +342,7 @@ sysc_mcck_pending:
 	br	%r1			# TIF bit will be cleared by handler
 
 #
-# _TIF_SIGPENDING or _TIF_RESTORE_SIGMASK is set, call do_signal
+# _TIF_SIGPENDING is set, call do_signal
 #
 sysc_sigpending:
 	ni	__TI_flags+3(%r9),255-_TIF_SINGLE_STEP # clear TIF_SINGLE_STEP
@@ -657,7 +657,7 @@ io_work:
 	lr	%r15,%r1
 #
 # One of the work bits is on. Find out which one.
-# Checked are: _TIF_SIGPENDING, _TIF_RESTORE_SIGMASK, _TIF_NEED_RESCHED
+# Checked are: _TIF_SIGPENDING, _TIF_NEED_RESCHED
 #		and _TIF_MCCK_PENDING
 #
 io_work_loop:
@@ -665,7 +665,7 @@ io_work_loop:
 	bo	BASED(io_mcck_pending)
 	tm	__TI_flags+3(%r9),_TIF_NEED_RESCHED
 	bo	BASED(io_reschedule)
-	tm	__TI_flags+3(%r9),(_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)
+	tm	__TI_flags+3(%r9),_TIF_SIGPENDING
 	bnz	BASED(io_sigpending)
 	b	BASED(io_restore)
 io_work_done:
@@ -693,7 +693,7 @@ io_reschedule:
 	b	BASED(io_work_loop)
 
 #
-# _TIF_SIGPENDING or _TIF_RESTORE_SIGMASK is set, call do_signal
+# _TIF_SIGPENDING is set, call do_signal
 #
 io_sigpending:
 	TRACE_IRQS_ON
diff --git a/arch/s390/kernel/entry64.S b/arch/s390/kernel/entry64.S
index efde6e1..0338579 100644
--- a/arch/s390/kernel/entry64.S
+++ b/arch/s390/kernel/entry64.S
@@ -52,9 +52,9 @@ SP_SIZE      =	STACK_FRAME_OVERHEAD + __PT_SIZE
 STACK_SHIFT = PAGE_SHIFT + THREAD_ORDER
 STACK_SIZE  = 1 << STACK_SHIFT
 
-_TIF_WORK_SVC = (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK | _TIF_NEED_RESCHED | \
+_TIF_WORK_SVC = (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
 		 _TIF_MCCK_PENDING | _TIF_RESTART_SVC | _TIF_SINGLE_STEP )
-_TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK | _TIF_NEED_RESCHED | \
+_TIF_WORK_INT = (_TIF_SIGPENDING | _TIF_NEED_RESCHED | \
 		 _TIF_MCCK_PENDING)
 
 #define BASED(name) name-system_call(%r13)
@@ -308,7 +308,7 @@ sysc_work:
 	jo	sysc_mcck_pending
 	tm	__TI_flags+7(%r9),_TIF_NEED_RESCHED
 	jo	sysc_reschedule
-	tm	__TI_flags+7(%r9),(_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)
+	tm	__TI_flags+7(%r9),_TIF_SIGPENDING
 	jnz	sysc_sigpending
 	tm	__TI_flags+7(%r9),_TIF_RESTART_SVC
 	jo	sysc_restart
@@ -332,7 +332,7 @@ sysc_mcck_pending:
 	jg	s390_handle_mcck	# TIF bit will be cleared by handler
 
 #
-# _TIF_SIGPENDING or _TIF_RESTORE_SIGMASK is set, call do_signal
+# _TIF_SIGPENDING is set, call do_signal
 #
 sysc_sigpending:
 	ni	__TI_flags+7(%r9),255-_TIF_SINGLE_STEP # clear TIF_SINGLE_STEP
@@ -647,7 +647,7 @@ io_work_loop:
 	jo	io_mcck_pending
 	tm	__TI_flags+7(%r9),_TIF_NEED_RESCHED
 	jo	io_reschedule
-	tm	__TI_flags+7(%r9),(_TIF_SIGPENDING | _TIF_RESTORE_SIGMASK)
+	tm	__TI_flags+7(%r9),_TIF_SIGPENDING
 	jnz	io_sigpending
 	j	io_restore
 io_work_done:
@@ -673,7 +673,7 @@ io_reschedule:
 	j	io_work_loop
 
 #
-# _TIF_SIGPENDING or _TIF_RESTORE_SIGMASK is set, call do_signal
+# _TIF_SIGPENDING or is set, call do_signal
 #
 io_sigpending:
 	TRACE_IRQS_ON
diff --git a/include/asm-s390/thread_info.h b/include/asm-s390/thread_info.h
index 0a51891..99bbed9 100644
--- a/include/asm-s390/thread_info.h
+++ b/include/asm-s390/thread_info.h
@@ -89,7 +89,6 @@ static inline struct thread_info *current_thread_info(void)
  * thread information flags bit numbers
  */
 #define TIF_SYSCALL_TRACE	0	/* syscall trace active */
-#define TIF_RESTORE_SIGMASK	1	/* restore signal mask in do_signal() */
 #define TIF_SIGPENDING		2	/* signal pending */
 #define TIF_NEED_RESCHED	3	/* rescheduling necessary */
 #define TIF_RESTART_SVC		4	/* restart svc with new svc number */
@@ -101,6 +100,7 @@ static inline struct thread_info *current_thread_info(void)
 					   TIF_NEED_RESCHED */
 #define TIF_31BIT		18	/* 32bit process */ 
 #define TIF_MEMDIE		19
+#define TIF_RESTORE_SIGMASK	20	/* restore signal mask in do_signal() */
 
 #define _TIF_SYSCALL_TRACE	(1<<TIF_SYSCALL_TRACE)
 #define _TIF_RESTORE_SIGMASK	(1<<TIF_RESTORE_SIGMASK)

Re: [PATCH 3/4] s390 renumber TIF_RESTORE_SIGMASK

[Martin Schwidefsky]

On Fri, 2008-03-28 at 17:14 -0700, Roland McGrath wrote:
> TIF_RESTORE_SIGMASK no longer needs to be in the _TIF_WORK_* masks.
> Those low bits are scarce, and are all used up now.
> Renumber TIF_RESTORE_SIGMASK to free one up.
> 
> Signed-off-by: Roland McGrath <roland@redhat.com>

Acked-by: Martin Schwidefsky <schwidefsky@de.ibm.com>

-- 
blue skies,
  Martin.

"Reality continues to ruin my life." - Calvin.

[PATCH 4/4] ia64 renumber TIF_RESTORE_SIGMASK

[Roland McGrath]

TIF_RESTORE_SIGMASK no longer needs to be in the _TIF_WORK_* masks.
Those low bits are scarce.  Renumber TIF_RESTORE_SIGMASK to free one up.

Signed-off-by: Roland McGrath <roland@redhat.com>
---
 arch/ia64/kernel/process.c     |    2 +-
 include/asm-ia64/thread_info.h |    5 ++---
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/arch/ia64/kernel/process.c b/arch/ia64/kernel/process.c
index 49937a3..2ef2794 100644
--- a/arch/ia64/kernel/process.c
+++ b/arch/ia64/kernel/process.c
@@ -183,7 +183,7 @@ do_notify_resume_user (sigset_t *unused, struct sigscratch *scr, long in_syscall
 #endif
 
 	/* deal with pending signal delivery */
-	if (test_thread_flag(TIF_SIGPENDING)||test_thread_flag(TIF_RESTORE_SIGMASK))
+	if (test_thread_flag(TIF_SIGPENDING))
 		ia64_do_signal(scr, in_syscall);
 
 	/* copy user rbs to kernel rbs */
diff --git a/include/asm-ia64/thread_info.h b/include/asm-ia64/thread_info.h
index 93d83cb..366e517 100644
--- a/include/asm-ia64/thread_info.h
+++ b/include/asm-ia64/thread_info.h
@@ -87,7 +87,6 @@ extern void tsk_clear_notify_resume(struct task_struct *tsk);
 #define TIF_SYSCALL_TRACE	2	/* syscall trace active */
 #define TIF_SYSCALL_AUDIT	3	/* syscall auditing active */
 #define TIF_SINGLESTEP		4	/* restore singlestep on return to user mode */
-#define TIF_RESTORE_SIGMASK	5	/* restore signal mask in do_signal() */
 #define TIF_NOTIFY_RESUME	6	/* resumption notification requested */
 #define TIF_POLLING_NRFLAG	16	/* true if poll_idle() is polling TIF_NEED_RESCHED */
 #define TIF_MEMDIE		17
@@ -95,6 +94,7 @@ extern void tsk_clear_notify_resume(struct task_struct *tsk);
 #define TIF_DB_DISABLED		19	/* debug trap disabled for fsyscall */
 #define TIF_FREEZE		20	/* is freezing for suspend */
 #define TIF_RESTORE_RSE		21	/* user RBS is newer than kernel RBS */
+#define TIF_RESTORE_SIGMASK	22	/* restore signal mask in do_signal() */
 
 #define _TIF_SYSCALL_TRACE	(1 << TIF_SYSCALL_TRACE)
 #define _TIF_SYSCALL_AUDIT	(1 << TIF_SYSCALL_AUDIT)
@@ -112,8 +112,7 @@ extern void tsk_clear_notify_resume(struct task_struct *tsk);
 
 /* "work to do on user-return" bits */
 #define TIF_ALLWORK_MASK	(_TIF_SIGPENDING|_TIF_NOTIFY_RESUME|_TIF_SYSCALL_AUDIT|\
-				 _TIF_NEED_RESCHED| _TIF_SYSCALL_TRACE|\
-				 _TIF_RESTORE_SIGMASK)
+				 _TIF_NEED_RESCHED|_TIF_SYSCALL_TRACE)
 /* like TIF_ALLWORK_BITS but sans TIF_SYSCALL_TRACE or TIF_SYSCALL_AUDIT */
 #define TIF_WORK_MASK		(TIF_ALLWORK_MASK&~(_TIF_SYSCALL_TRACE|_TIF_SYSCALL_AUDIT))