linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH RFC 0/8] Signal: harmonize syscall restart logic
@ 2011-10-23 10:19 Jonas Bonn
  2011-10-23 10:19 ` [PATCH RFC 1/8] signal: introduce generic " Jonas Bonn
                   ` (7 more replies)
  0 siblings, 8 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:19 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn


I've been looking into trying to harmonize the architecture specific
signal handling code.  With a couple of helper functions to mask some
of the architecture specific bits, the code for do_signal and handle_signal
(implemented by essentially everybody) start to look identical across
all arch's and could likely be made generic.

The big helper is the one in this series.  This one moves the syscall
restart logic out to a common helper.  The arch specific bit here is
the register manipulations required to actually effect the restart;
this patch series moves those manipulations to the asm/syscall.h header
which already attempts to define a generic syscall interface for several
arch's today.

Where this series remains an RFC is due to a couple of open questions:

i)  ARM (and a few arch's copied from it) does its restart logic in two
    steps so that a debugger can intervene; is there still value in
    this approach or would the single step approach in this patch series
    be sufficient for ARM (and others)?

ii) The syscall.h interface requires that the information needed to do
    the register manipulations be in the pt_regs struct.  This isn't
    the case for all arch's today.  Are there any issues with putting that
    info into pt_regs for those arch's?

With the rest of the cleanups in this series it currently comes to 64
patches, so I'm just posting a couple of the syscall restart patches
now for comment in order not to flood the lists.  I'll post the remainder
of the series later based on the feedback from this series.

/Jonas

Jonas Bonn (8):
  signal: introduce generic syscall restart logic
  blackfin: implement syscall restart generically
  frv: implement syscall restart generically
  mips: implement syscall restart generically
  x86: implement syscall restart generically
  m68k: implement syscall restart generically
  ia64: implement syscall restart generically
  tile: implement syscall restart generically

 arch/blackfin/include/asm/syscall.h |   26 +++++++++-
 arch/blackfin/kernel/signal.c       |   47 +-----------------
 arch/frv/include/asm/syscall.h      |   23 ++++++++-
 arch/frv/kernel/signal.c            |   48 +-----------------
 arch/ia64/include/asm/syscall.h     |   21 ++++++++
 arch/ia64/kernel/signal.c           |   54 +--------------------
 arch/m68k/include/asm/syscall.h     |   87 +++++++++++++++++++++++++++++++++
 arch/m68k/kernel/signal_mm.c        |   45 +----------------
 arch/m68k/kernel/signal_no.c        |   46 +----------------
 arch/mips/include/asm/syscall.h     |   91 +++++++++++++++++++++++++++++++++++
 arch/mips/kernel/signal.c           |   40 +--------------
 arch/tile/include/asm/syscall.h     |   28 ++++++++++-
 arch/tile/kernel/signal.c           |   52 ++------------------
 arch/x86/include/asm/syscall.h      |   21 ++++++++
 arch/x86/kernel/signal.c            |   43 +---------------
 include/asm-generic/syscall.h       |   46 +++++++++++++++++-
 include/linux/signal.h              |    3 +
 kernel/signal.c                     |   72 +++++++++++++++++++++++++++-
 18 files changed, 435 insertions(+), 358 deletions(-)
 create mode 100644 arch/m68k/include/asm/syscall.h
 create mode 100644 arch/mips/include/asm/syscall.h

-- 
1.7.5.4


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH RFC 1/8] signal: introduce generic syscall restart logic
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
@ 2011-10-23 10:19 ` Jonas Bonn
  2011-10-23 10:19 ` [PATCH RFC 2/8] blackfin: implement syscall restart generically Jonas Bonn
                   ` (6 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:19 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn


Manipulating the task state so that an interrupted syscall will be
re-executed after signal handling has historically been purely architecture
specific logic.  Most architectures, however, are implementing essentially
the same thing (some incorrectly) with the actual details of the registers
to be manipulated being the only relevant difference.

This patch introduces generic code for handling the syscall restart logic
by:

i)  introducing methods for doing the actual register manipulations to
the generic syscall interface in asm/syscall.h

ii) introducing the function handle_syscall_restart to determine the
actual restart requirements and invoke the required register manipulations;
this function is designed to be called immediately after invoking the
function get_signal_to_deliver

This generic variant is applicable for any architecture that maintains the
information necessary to effect a syscall restart in its pt_regs struct.

Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 include/asm-generic/syscall.h |   46 +++++++++++++++++++++++++-
 include/linux/signal.h        |    3 ++
 kernel/signal.c               |   72 ++++++++++++++++++++++++++++++++++++++++-
 3 files changed, 119 insertions(+), 2 deletions(-)

diff --git a/include/asm-generic/syscall.h b/include/asm-generic/syscall.h
index 5c122ae..b7b489f 100644
--- a/include/asm-generic/syscall.h
+++ b/include/asm-generic/syscall.h
@@ -36,11 +36,55 @@ struct pt_regs;
  * system call number can be meaningful.  If the actual arch value
  * is 64 bits, this truncates to 32 bits so 0xffffffff means -1.
  *
- * It's only valid to call this when @task is known to be blocked.
+ * It's only valid to call this for the current task or when @task
+ * is known to be blocked.
  */
 int syscall_get_nr(struct task_struct *task, struct pt_regs *regs);
 
 /**
+ * syscall_clear - clear the syscall execution state
+ *
+ * Clear the syscall execution state of @task so that future invocations
+ * of syscall_get_nr for @task return -1.
+ *
+ * It's only valid to call this for the current task.  This function
+ * is primarily intended for use in the signal handling path; when
+ * signal handling takes over after a syscall then the syscall handling
+ * is effectively finished and the syscall state can be cleared.
+ */
+void syscall_clear(struct task_struct *task, struct pt_regs *regs);
+
+/**
+ * syscall_do_restart - roll back PC and argument regs for syscall restart
+ * @task:	must be current task
+ * @regs:	task_pt_regs() of @task
+ * 
+ * Rolls back the syscall number, arguments, and the instruction pointer
+ * so that the current syscall will be reexecuted upon return to userspace.
+ *
+ * This function is primarily intended for use in the signal handling
+ * code and shouldn't really be used elsewhere.
+ */
+void syscall_restart(struct task_struct *task, struct pt_regs *regs);
+
+/**
+ * syscall_do_restartblock - roll back PC and execute restart_syscall
+ * @task:	must be current task
+ * @regs:	task_pt_regs() of @task
+ * 
+ * Configure @task so that upon return to userspace the restart_syscall
+ * (or equivalent) function is executed to perform syscall restart via a
+ * restart_block.  Since restart_syscall normally doesn't take any arguments,
+ * restoring the syscall arguments can be skipped.  This normally boils
+ * down to rewinding the PC and setting the syscall register to 
+ * __NR_restart_syscall; however, some arch's may do this differently.
+ *
+ * This function is primarily intended for use in the signal handling
+ * code.
+ */
+void syscall_do_restartblock(struct task_struct * taks, struct pt_regs* regs);
+
+/**
  * syscall_rollback - roll back registers after an aborted system call
  * @task:	task of interest, must be in system call exit tracing
  * @regs:	task_pt_regs() of @task
diff --git a/include/linux/signal.h b/include/linux/signal.h
index a822300..7ede592 100644
--- a/include/linux/signal.h
+++ b/include/linux/signal.h
@@ -256,6 +256,9 @@ extern int show_unhandled_signals;
 extern int get_signal_to_deliver(siginfo_t *info, struct k_sigaction *return_ka, struct pt_regs *regs, void *cookie);
 extern void exit_signals(struct task_struct *tsk);
 
+void handle_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
+			    int has_handler);
+
 extern struct kmem_cache *sighand_cachep;
 
 int unhandled_signal(struct task_struct *tsk, int sig);
diff --git a/kernel/signal.c b/kernel/signal.c
index 291c970..7b3d9a4 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -33,8 +33,8 @@
 
 #include <asm/param.h>
 #include <asm/uaccess.h>
-#include <asm/unistd.h>
 #include <asm/siginfo.h>
+#include <asm/syscall.h>
 #include "audit.h"	/* audit_signal_info() */
 
 /*
@@ -2396,6 +2396,76 @@ EXPORT_SYMBOL(sigprocmask);
 EXPORT_SYMBOL(block_all_signals);
 EXPORT_SYMBOL(unblock_all_signals);
 
+/**
+ * handle_syscall_restart();
+ * @regs: pt_regs for current process
+ * @ka:   k_sigaction struct for current signal handler 
+ * @has_handler: boolean indicating whether there is a handler waiting
+ *               to be invoked for a pending signal
+ *
+ * Prepare the currently executing task to restart an interrupted
+ * syscall.  After completion of this function, the task is effectively
+ * switched from syscall-execution mode to signal-processing mode and
+ * further attempts to manipulate the syscall state will fail.
+ *
+ * If has_handler != 0, then ka must point to a valid sigaction struct.
+ */
+
+void handle_syscall_restart(struct pt_regs *regs, struct k_sigaction *ka,
+			    int has_handler)
+{
+	/* Are we from a system call? */
+	if (syscall_get_nr(current, regs) >= 0) {
+		int restart = 1;
+		long error;
+
+		error = syscall_get_error(current, regs);
+
+		/* Check if syscall was interrupted. */
+		switch (error) {
+		case -ERESTART_RESTARTBLOCK:
+		case -ERESTARTNOHAND:
+			/* Restart if there is no handler */
+			restart = !has_handler;
+			break;
+
+		case -ERESTARTSYS:
+			/*
+			 * Restart if there is no handler or if the handler
+			 * was registered with SA_RESTART
+			 */
+			restart = !has_handler
+				  || (ka->sa.sa_flags & SA_RESTART);
+			break;
+
+		case -ERESTARTNOINTR:
+			/* Restart after signal handler returns */ 
+			restart = 1;
+			break;
+
+		default:
+			/* Syscall wasn't interrupted so we're done with it */
+			syscall_clear(current, regs);
+			return;
+		}
+
+		if (restart) {
+			if (error == -ERESTART_RESTARTBLOCK)
+				syscall_do_restartblock(current, regs);
+			else
+				syscall_restart(current, regs);
+		} else {
+			syscall_set_return_value(current, regs, -EINTR, 0);
+		}
+
+		/*
+		 * The syscall processing is effectively complete here so
+		 * the syscall state can be cleared to prevent this function
+		 * from triggering again for nested signals.
+		 */
+		syscall_clear(current, regs);
+	}
+}
 
 /*
  * System call entry points.
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 2/8] blackfin: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
  2011-10-23 10:19 ` [PATCH RFC 1/8] signal: introduce generic " Jonas Bonn
@ 2011-10-23 10:19 ` Jonas Bonn
  2011-10-26  0:01   ` Mike Frysinger
  2011-10-23 10:19 ` [PATCH RFC 3/8] frv: " Jonas Bonn
                   ` (5 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:19 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, uclinux-dist-devel


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: uclinux-dist-devel@blackfin.uclinux.org
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/blackfin/include/asm/syscall.h |   26 ++++++++++++++++++-
 arch/blackfin/kernel/signal.c       |   47 ++--------------------------------
 2 files changed, 27 insertions(+), 46 deletions(-)

diff --git a/arch/blackfin/include/asm/syscall.h b/arch/blackfin/include/asm/syscall.h
index 4921a48..87005c4 100644
--- a/arch/blackfin/include/asm/syscall.h
+++ b/arch/blackfin/include/asm/syscall.h
@@ -21,17 +21,39 @@
 #include <linux/err.h>
 #include <linux/sched.h>
 #include <asm/ptrace.h>
+#include <asm/unistd.h>
 
 static inline long
 syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
 {
-	return regs->p0;
+	return regs->orig_p0;
 }
 
 static inline void
 syscall_rollback(struct task_struct *task, struct pt_regs *regs)
 {
 	regs->p0 = regs->orig_p0;
+	regs->r0 = regs->orig_r0;
+}
+
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->orig_p0 = -1;
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	regs->pc -= 2;
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->p0 = __NR_restart_syscall;
+	regs->pc -= 2;
 }
 
 static inline long
@@ -50,7 +72,7 @@ static inline void
 syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
                          int error, long val)
 {
-	regs->r0 = error ? -error : val;
+	regs->r0 = error ? error : val;
 }
 
 /**
diff --git a/arch/blackfin/kernel/signal.c b/arch/blackfin/kernel/signal.c
index d536f35..7fc176a 100644
--- a/arch/blackfin/kernel/signal.c
+++ b/arch/blackfin/kernel/signal.c
@@ -219,36 +219,6 @@ setup_rt_frame(int sig, struct k_sigaction *ka, siginfo_t * info,
 	return -EFAULT;
 }
 
-static inline void
-handle_restart(struct pt_regs *regs, struct k_sigaction *ka, int has_handler)
-{
-	switch (regs->r0) {
-	case -ERESTARTNOHAND:
-		if (!has_handler)
-			goto do_restart;
-		regs->r0 = -EINTR;
-		break;
-
-	case -ERESTARTSYS:
-		if (has_handler && !(ka->sa.sa_flags & SA_RESTART)) {
-			regs->r0 = -EINTR;
-			break;
-		}
-		/* fallthrough */
-	case -ERESTARTNOINTR:
- do_restart:
-		regs->p0 = regs->orig_p0;
-		regs->r0 = regs->orig_r0;
-		regs->pc -= 2;
-		break;
-
-	case -ERESTART_RESTARTBLOCK:
-		regs->p0 = __NR_restart_syscall;
-		regs->pc -= 2;
-		break;
-	}
-}
-
 /*
  * OK, we're invoking a handler
  */
@@ -258,11 +228,6 @@ handle_signal(int sig, siginfo_t *info, struct k_sigaction *ka,
 {
 	int ret;
 
-	/* are we from a system call? to see pt_regs->orig_p0 */
-	if (regs->orig_p0 >= 0)
-		/* If so, check system call restarting.. */
-		handle_restart(regs, ka, 1);
-
 	/* set up the stack frame */
 	ret = setup_rt_frame(sig, ka, info, oldset, regs);
 
@@ -296,15 +261,15 @@ asmlinkage void do_signal(struct pt_regs *regs)
 
 	current->thread.esp0 = (unsigned long)regs;
 
-	if (try_to_freeze())
-		goto no_signal;
-
 	if (test_thread_flag(TIF_RESTORE_SIGMASK))
 		oldset = &current->saved_sigmask;
 	else
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+	handle_syscall_restart(regs, &ka, (signr > 0));
+
 	if (signr > 0) {
 		/* Whee!  Actually deliver the signal.  */
 		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
@@ -322,12 +287,6 @@ asmlinkage void do_signal(struct pt_regs *regs)
 		return;
 	}
 
- no_signal:
-	/* Did we come from a system call? */
-	if (regs->orig_p0 >= 0)
-		/* Restart the system call - no handlers present */
-		handle_restart(regs, NULL, 0);
-
 	/* if there's no signal to deliver, we just put the saved sigmask
 	 * back */
 	if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 3/8] frv: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
  2011-10-23 10:19 ` [PATCH RFC 1/8] signal: introduce generic " Jonas Bonn
  2011-10-23 10:19 ` [PATCH RFC 2/8] blackfin: implement syscall restart generically Jonas Bonn
@ 2011-10-23 10:19 ` Jonas Bonn
  2011-10-23 10:19 ` [PATCH RFC 4/8] mips: " Jonas Bonn
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:19 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, David Howells


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: David Howells <dhowells@redhat.com>
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/frv/include/asm/syscall.h |   23 ++++++++++++++++++-
 arch/frv/kernel/signal.c       |   48 ++-------------------------------------
 2 files changed, 25 insertions(+), 46 deletions(-)

diff --git a/arch/frv/include/asm/syscall.h b/arch/frv/include/asm/syscall.h
index 70689eb..a0493da 100644
--- a/arch/frv/include/asm/syscall.h
+++ b/arch/frv/include/asm/syscall.h
@@ -14,6 +14,7 @@
 
 #include <linux/err.h>
 #include <asm/ptrace.h>
+#include <asm/unistd.h>
 
 /*
  * Get the system call number or -1
@@ -34,6 +35,26 @@ static inline void syscall_rollback(struct task_struct *task,
 	regs->gr8 = regs->orig_gr8;
 }
 
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->syscallno = -1;
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	regs->pc -= 4;
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->gr7 = __NR_restart_syscall;
+	regs->pc -= 4;
+}
+
 /*
  * See if the syscall return value is an error, returning it if it is and 0 if
  * not
@@ -61,7 +82,7 @@ static inline void syscall_set_return_value(struct task_struct *task,
 					    int error, long val)
 {
 	if (error)
-		regs->gr8 = -error;
+		regs->gr8 = error;
 	else
 		regs->gr8 = val;
 }
diff --git a/arch/frv/kernel/signal.c b/arch/frv/kernel/signal.c
index bab0129..a8bacdd 100644
--- a/arch/frv/kernel/signal.c
+++ b/arch/frv/kernel/signal.c
@@ -445,29 +445,6 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
 {
 	int ret;
 
-	/* Are we from a system call? */
-	if (__frame->syscallno != -1) {
-		/* If so, check system call restarting.. */
-		switch (__frame->gr8) {
-		case -ERESTART_RESTARTBLOCK:
-		case -ERESTARTNOHAND:
-			__frame->gr8 = -EINTR;
-			break;
-
-		case -ERESTARTSYS:
-			if (!(ka->sa.sa_flags & SA_RESTART)) {
-				__frame->gr8 = -EINTR;
-				break;
-			}
-
-			/* fallthrough */
-		case -ERESTARTNOINTR:
-			__frame->gr8 = __frame->orig_gr8;
-			__frame->pc -= 4;
-		}
-		__frame->syscallno = -1;
-	}
-
 	/* Set up the stack frame */
 	if (ka->sa.sa_flags & SA_SIGINFO)
 		ret = setup_rt_frame(sig, ka, info, oldset);
@@ -510,15 +487,15 @@ static void do_signal(void)
 	if (!user_mode(__frame))
 		return;
 
-	if (try_to_freeze())
-		goto no_signal;
-
 	if (test_thread_flag(TIF_RESTORE_SIGMASK))
 		oldset = &current->saved_sigmask;
 	else
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, __frame, NULL);
+
+	handle_syscall_restart(__frame, &ka, (signr > 0));
+
 	if (signr > 0) {
 		if (handle_signal(signr, &info, &ka, oldset) == 0) {
 			/* a signal was successfully delivered; the saved
@@ -536,25 +513,6 @@ static void do_signal(void)
 	}
 
 no_signal:
-	/* Did we come from a system call? */
-	if (__frame->syscallno != -1) {
-		/* Restart the system call - no handlers present */
-		switch (__frame->gr8) {
-		case -ERESTARTNOHAND:
-		case -ERESTARTSYS:
-		case -ERESTARTNOINTR:
-			__frame->gr8 = __frame->orig_gr8;
-			__frame->pc -= 4;
-			break;
-
-		case -ERESTART_RESTARTBLOCK:
-			__frame->gr7 = __NR_restart_syscall;
-			__frame->pc -= 4;
-			break;
-		}
-		__frame->syscallno = -1;
-	}
-
 	/* if there's no signal to deliver, we just put the saved sigmask
 	 * back */
 	if (test_thread_flag(TIF_RESTORE_SIGMASK)) {
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 4/8] mips: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
                   ` (2 preceding siblings ...)
  2011-10-23 10:19 ` [PATCH RFC 3/8] frv: " Jonas Bonn
@ 2011-10-23 10:19 ` Jonas Bonn
  2011-11-04  9:32   ` Ralf Baechle
  2011-10-23 10:19 ` [PATCH RFC 5/8] x86: " Jonas Bonn
                   ` (3 subsequent siblings)
  7 siblings, 1 reply; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:19 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, linux-mips


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: linux-mips@linux-mips.org
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/mips/include/asm/syscall.h |   91 +++++++++++++++++++++++++++++++++++++++
 arch/mips/kernel/signal.c       |   40 +----------------
 2 files changed, 94 insertions(+), 37 deletions(-)
 create mode 100644 arch/mips/include/asm/syscall.h

diff --git a/arch/mips/include/asm/syscall.h b/arch/mips/include/asm/syscall.h
new file mode 100644
index 0000000..7280d95
--- /dev/null
+++ b/arch/mips/include/asm/syscall.h
@@ -0,0 +1,91 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Copyright (C) 2011 Jonas Bonn <jonas@southpole.se>
+ */
+
+#ifndef _ASM_SYSCALL_H
+#define _ASM_SYSCALL_H
+
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <asm/unistd.h>
+
+static inline int
+syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
+{
+	return regs->regs[0] ?: -1;
+}
+
+static inline void
+syscall_rollback(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->regs[2] = regs->regs[0];
+	regs->regs[7] = regs->regs[26];
+}
+
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->regs[0] = 0;
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	regs->cp0_epc -= 4;
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->regs[2] = current->thread.abi->restart;
+	regs->regs[7] = regs->regs[26];
+	regs->cp0_epc -= 4;
+}
+
+static inline long
+syscall_get_error(struct task_struct *task, struct pt_regs *regs)
+{
+	/*
+	 * r7 == 1 if syscall returns error
+	 * r2 contains _positive_ result
+	 */
+	if (regs->regs[7])
+		return -regs->regs[2];
+	else
+		return 0;
+}
+
+static inline long
+syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
+{
+	return regs->gpr[2];
+}
+
+static inline void
+syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
+			 int error, long val)
+{
+	regs->gpr[2] = (long) error ? -error : val;
+}
+
+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+		      unsigned int i, unsigned int n, unsigned long *args)
+{
+	/* TODO */
+}
+
+static inline void
+syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
+		      unsigned int i, unsigned int n, const unsigned long *args)
+{
+	/* TODO */
+}
+
+#endif
diff --git a/arch/mips/kernel/signal.c b/arch/mips/kernel/signal.c
index dbbe0ce..13f2864 100644
--- a/arch/mips/kernel/signal.c
+++ b/arch/mips/kernel/signal.c
@@ -547,27 +547,6 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
 	struct mips_abi *abi = current->thread.abi;
 	void *vdso = current->mm->context.vdso;
 
-	if (regs->regs[0]) {
-		switch(regs->regs[2]) {
-		case ERESTART_RESTARTBLOCK:
-		case ERESTARTNOHAND:
-			regs->regs[2] = EINTR;
-			break;
-		case ERESTARTSYS:
-			if (!(ka->sa.sa_flags & SA_RESTART)) {
-				regs->regs[2] = EINTR;
-				break;
-			}
-		/* fallthrough */
-		case ERESTARTNOINTR:
-			regs->regs[7] = regs->regs[26];
-			regs->regs[2] = regs->regs[0];
-			regs->cp0_epc -= 4;
-		}
-
-		regs->regs[0] = 0;		/* Don't deal with this again.  */
-	}
-
 	if (sig_uses_siginfo(ka))
 		ret = abi->setup_rt_frame(vdso + abi->rt_signal_return_offset,
 					  ka, regs, sig, oldset, info);
@@ -609,6 +588,9 @@ static void do_signal(struct pt_regs *regs)
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+	handle_syscall_restart(regs, &ka, (signr > 0));
+
 	if (signr > 0) {
 		/* Whee!  Actually deliver the signal.  */
 		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
@@ -625,22 +607,6 @@ static void do_signal(struct pt_regs *regs)
 		return;
 	}
 
-	if (regs->regs[0]) {
-		if (regs->regs[2] == ERESTARTNOHAND ||
-		    regs->regs[2] == ERESTARTSYS ||
-		    regs->regs[2] == ERESTARTNOINTR) {
-			regs->regs[2] = regs->regs[0];
-			regs->regs[7] = regs->regs[26];
-			regs->cp0_epc -= 4;
-		}
-		if (regs->regs[2] == ERESTART_RESTARTBLOCK) {
-			regs->regs[2] = current->thread.abi->restart;
-			regs->regs[7] = regs->regs[26];
-			regs->cp0_epc -= 4;
-		}
-		regs->regs[0] = 0;	/* Don't deal with this again.  */
-	}
-
 	/*
 	 * If there's no signal to deliver, we just put the saved sigmask
 	 * back
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 5/8] x86: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
                   ` (3 preceding siblings ...)
  2011-10-23 10:19 ` [PATCH RFC 4/8] mips: " Jonas Bonn
@ 2011-10-23 10:19 ` Jonas Bonn
  2011-10-23 10:20 ` [PATCH RFC 6/8] m68k: " Jonas Bonn
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:19 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, x86


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: x86@kernel.org
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/x86/include/asm/syscall.h |   21 +++++++++++++++++++
 arch/x86/kernel/signal.c       |   43 ++-------------------------------------
 2 files changed, 24 insertions(+), 40 deletions(-)

diff --git a/arch/x86/include/asm/syscall.h b/arch/x86/include/asm/syscall.h
index c4a348f..bd7e0f6 100644
--- a/arch/x86/include/asm/syscall.h
+++ b/arch/x86/include/asm/syscall.h
@@ -15,6 +15,7 @@
 
 #include <linux/sched.h>
 #include <linux/err.h>
+#include <asm/unistd.h>
 
 extern const unsigned long sys_call_table[];
 
@@ -34,6 +35,26 @@ static inline void syscall_rollback(struct task_struct *task,
 	regs->ax = regs->orig_ax;
 }
 
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->orig_ax = -1;
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	regs->ip -= 2;
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->ax = __NR_restart_syscall;
+	regs->ip -= 2;
+}
+
 static inline long syscall_get_error(struct task_struct *task,
 				     struct pt_regs *regs)
 {
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index 54ddaeb2..366d5f7 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -685,28 +685,6 @@ handle_signal(unsigned long sig, siginfo_t *info, struct k_sigaction *ka,
 	sigset_t blocked;
 	int ret;
 
-	/* Are we from a system call? */
-	if (syscall_get_nr(current, regs) >= 0) {
-		/* If so, check system call restarting.. */
-		switch (syscall_get_error(current, regs)) {
-		case -ERESTART_RESTARTBLOCK:
-		case -ERESTARTNOHAND:
-			regs->ax = -EINTR;
-			break;
-
-		case -ERESTARTSYS:
-			if (!(ka->sa.sa_flags & SA_RESTART)) {
-				regs->ax = -EINTR;
-				break;
-			}
-		/* fallthrough */
-		case -ERESTARTNOINTR:
-			regs->ax = regs->orig_ax;
-			regs->ip -= 2;
-			break;
-		}
-	}
-
 	/*
 	 * If TF is set due to a debugger (TIF_FORCED_TF), clear the TF
 	 * flag so that register information in the sigcontext is correct.
@@ -773,30 +751,15 @@ static void do_signal(struct pt_regs *regs)
 		return;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+	handle_syscall_restart(regs, &ka, (signr > 0));
+
 	if (signr > 0) {
 		/* Whee! Actually deliver the signal.  */
 		handle_signal(signr, &info, &ka, regs);
 		return;
 	}
 
-	/* Did we come from a system call? */
-	if (syscall_get_nr(current, regs) >= 0) {
-		/* Restart the system call - no handlers present */
-		switch (syscall_get_error(current, regs)) {
-		case -ERESTARTNOHAND:
-		case -ERESTARTSYS:
-		case -ERESTARTNOINTR:
-			regs->ax = regs->orig_ax;
-			regs->ip -= 2;
-			break;
-
-		case -ERESTART_RESTARTBLOCK:
-			regs->ax = NR_restart_syscall;
-			regs->ip -= 2;
-			break;
-		}
-	}
-
 	/*
 	 * If there's no signal to deliver, we just put the saved sigmask
 	 * back.
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 6/8] m68k: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
                   ` (4 preceding siblings ...)
  2011-10-23 10:19 ` [PATCH RFC 5/8] x86: " Jonas Bonn
@ 2011-10-23 10:20 ` Jonas Bonn
  2011-10-23 10:20 ` [PATCH RFC 7/8] ia64: " Jonas Bonn
  2011-10-23 10:20 ` [PATCH RFC 8/8] tile: " Jonas Bonn
  7 siblings, 0 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:20 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, linux-m68k


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: linux-m68k@lists.linux-m68k.org
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/m68k/include/asm/syscall.h |   87 +++++++++++++++++++++++++++++++++++++++
 arch/m68k/kernel/signal_mm.c    |   45 +-------------------
 arch/m68k/kernel/signal_no.c    |   46 +-------------------
 3 files changed, 93 insertions(+), 85 deletions(-)
 create mode 100644 arch/m68k/include/asm/syscall.h

diff --git a/arch/m68k/include/asm/syscall.h b/arch/m68k/include/asm/syscall.h
new file mode 100644
index 0000000..a482b7b
--- /dev/null
+++ b/arch/m68k/include/asm/syscall.h
@@ -0,0 +1,87 @@
+/*
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ * Copyright (C) 2011 Jonas Bonn <jonas@southpole.se>
+ */
+
+#ifndef _M68K_SYSCALL_H
+#define _M68K_SYSCALL_H
+
+#include <linux/err.h>
+#include <linux/sched.h>
+#include <asm/unistd.h>
+
+static inline int
+syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
+{
+	if (regs->orig_d0 >= 0)
+		return regs->orig_d0;
+	else
+		return -1;
+}
+
+static inline void
+syscall_rollback(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->d0 = regs->orig_d0;
+}
+
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->orig_d0 = -1;
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	regs->pc -= 2;
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->d0 = __NR_restart_syscall;
+	regs->pc -= 2;
+}
+
+static inline long
+syscall_get_error(struct task_struct *task, struct pt_regs *regs)
+{
+	return IS_ERR_VALUE(regs->d0) ? regs->d0 : 0;
+}
+
+static inline long
+syscall_get_return_value(struct task_struct *task, struct pt_regs *regs)
+{
+	return regs->d0;
+}
+
+static inline void
+syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
+			 int error, long val)
+{
+	regs->d0 = (long) error ?: val;
+}
+
+#if 0
+static inline void
+syscall_get_arguments(struct task_struct *task, struct pt_regs *regs,
+		      unsigned int i, unsigned int n, unsigned long *args)
+{
+	/* TODO */
+}
+
+static inline void
+syscall_set_arguments(struct task_struct *task, struct pt_regs *regs,
+		      unsigned int i, unsigned int n, const unsigned long *args)
+{
+	/* TODO */
+}
+#endif
+
+#endif
diff --git a/arch/m68k/kernel/signal_mm.c b/arch/m68k/kernel/signal_mm.c
index a0afc23..a8fa041 100644
--- a/arch/m68k/kernel/signal_mm.c
+++ b/arch/m68k/kernel/signal_mm.c
@@ -895,39 +895,6 @@ give_sigsegv:
 	return err;
 }
 
-static inline void
-handle_restart(struct pt_regs *regs, struct k_sigaction *ka, int has_handler)
-{
-	switch (regs->d0) {
-	case -ERESTARTNOHAND:
-		if (!has_handler)
-			goto do_restart;
-		regs->d0 = -EINTR;
-		break;
-
-	case -ERESTART_RESTARTBLOCK:
-		if (!has_handler) {
-			regs->d0 = __NR_restart_syscall;
-			regs->pc -= 2;
-			break;
-		}
-		regs->d0 = -EINTR;
-		break;
-
-	case -ERESTARTSYS:
-		if (has_handler && !(ka->sa.sa_flags & SA_RESTART)) {
-			regs->d0 = -EINTR;
-			break;
-		}
-	/* fallthrough */
-	case -ERESTARTNOINTR:
-	do_restart:
-		regs->d0 = regs->orig_d0;
-		regs->pc -= 2;
-		break;
-	}
-}
-
 void ptrace_signal_deliver(struct pt_regs *regs, void *cookie)
 {
 	if (regs->orig_d0 < 0)
@@ -951,10 +918,6 @@ handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
 	      sigset_t *oldset, struct pt_regs *regs)
 {
 	int err;
-	/* are we from a system call? */
-	if (regs->orig_d0 >= 0)
-		/* If so, check system call restarting.. */
-		handle_restart(regs, ka, 1);
 
 	/* set up the stack frame */
 	if (ka->sa.sa_flags & SA_SIGINFO)
@@ -998,17 +961,15 @@ asmlinkage void do_signal(struct pt_regs *regs)
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+	handle_syscall_restart(regs, &ka, (signr > 0));
+
 	if (signr > 0) {
 		/* Whee!  Actually deliver the signal.  */
 		handle_signal(signr, &ka, &info, oldset, regs);
 		return;
 	}
 
-	/* Did we come from a system call? */
-	if (regs->orig_d0 >= 0)
-		/* Restart the system call - no handlers present */
-		handle_restart(regs, NULL, 0);
-
 	/* 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);
diff --git a/arch/m68k/kernel/signal_no.c b/arch/m68k/kernel/signal_no.c
index 36a81bb..2fad7c7 100644
--- a/arch/m68k/kernel/signal_no.c
+++ b/arch/m68k/kernel/signal_no.c
@@ -653,39 +653,6 @@ give_sigsegv:
 	goto adjust_stack;
 }
 
-static inline void
-handle_restart(struct pt_regs *regs, struct k_sigaction *ka, int has_handler)
-{
-	switch (regs->d0) {
-	case -ERESTARTNOHAND:
-		if (!has_handler)
-			goto do_restart;
-		regs->d0 = -EINTR;
-		break;
-
-	case -ERESTART_RESTARTBLOCK:
-		if (!has_handler) {
-			regs->d0 = __NR_restart_syscall;
-			regs->pc -= 2;
-			break;
-		}
-		regs->d0 = -EINTR;
-		break;
-
-	case -ERESTARTSYS:
-		if (has_handler && !(ka->sa.sa_flags & SA_RESTART)) {
-			regs->d0 = -EINTR;
-			break;
-		}
-	/* fallthrough */
-	case -ERESTARTNOINTR:
-	do_restart:
-		regs->d0 = regs->orig_d0;
-		regs->pc -= 2;
-		break;
-	}
-}
-
 /*
  * OK, we're invoking a handler
  */
@@ -694,10 +661,6 @@ handle_signal(int sig, struct k_sigaction *ka, siginfo_t *info,
 	      sigset_t *oldset, struct pt_regs *regs)
 {
 	int err;
-	/* are we from a system call? */
-	if (regs->orig_d0 >= 0)
-		/* If so, check system call restarting.. */
-		handle_restart(regs, ka, 1);
 
 	/* set up the stack frame */
 	if (ka->sa.sa_flags & SA_SIGINFO)
@@ -745,18 +708,15 @@ asmlinkage void do_signal(struct pt_regs *regs)
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+	handle_syscall_restart(regs, &ka, (signr > 0));
+
 	if (signr > 0) {
 		/* Whee!  Actually deliver the signal.  */
 		handle_signal(signr, &ka, &info, oldset, regs);
 		return;
 	}
 
-	/* Did we come from a system call? */
-	if (regs->orig_d0 >= 0) {
-		/* Restart the system call - no handlers present */
-		handle_restart(regs, NULL, 0);
-	}
-
 	/* 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);
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 7/8] ia64: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
                   ` (5 preceding siblings ...)
  2011-10-23 10:20 ` [PATCH RFC 6/8] m68k: " Jonas Bonn
@ 2011-10-23 10:20 ` Jonas Bonn
  2011-10-23 10:20 ` [PATCH RFC 8/8] tile: " Jonas Bonn
  7 siblings, 0 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:20 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, linux-ia64


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: linux-ia64@vger.kernel.org
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/ia64/include/asm/syscall.h |   21 +++++++++++++++
 arch/ia64/kernel/signal.c       |   54 +--------------------------------------
 2 files changed, 22 insertions(+), 53 deletions(-)

diff --git a/arch/ia64/include/asm/syscall.h b/arch/ia64/include/asm/syscall.h
index a7ff1c6..ee18042 100644
--- a/arch/ia64/include/asm/syscall.h
+++ b/arch/ia64/include/asm/syscall.h
@@ -15,6 +15,7 @@
 
 #include <linux/sched.h>
 #include <linux/err.h>
+#include <asm/unistd.h>
 
 static inline long syscall_get_nr(struct task_struct *task,
 				  struct pt_regs *regs)
@@ -31,6 +32,26 @@ static inline void syscall_rollback(struct task_struct *task,
 	/* do nothing */
 }
 
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->cr_ifs |= (1UL << 63);
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	ia64_decrement_ip(regs);
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->r15 = __NR_restart_syscall;
+	ia64_decrement_ip(regs);
+}
+
 static inline long syscall_get_error(struct task_struct *task,
 				     struct pt_regs *regs)
 {
diff --git a/arch/ia64/kernel/signal.c b/arch/ia64/kernel/signal.c
index 7bdafc8..0b8abbc 100644
--- a/arch/ia64/kernel/signal.c
+++ b/arch/ia64/kernel/signal.c
@@ -453,8 +453,6 @@ ia64_do_signal (struct sigscratch *scr, long in_syscall)
 	struct k_sigaction ka;
 	sigset_t *oldset;
 	siginfo_t info;
-	long restart = in_syscall;
-	long errno = scr->pt.r8;
 
 	/*
 	 * In the ia64_leave_kernel code path, we want the common case to go fast, which
@@ -476,44 +474,11 @@ ia64_do_signal (struct sigscratch *scr, long in_syscall)
 	while (1) {
 		int signr = get_signal_to_deliver(&info, &ka, &scr->pt, NULL);
 
-		/*
-		 * get_signal_to_deliver() may have run a debugger (via notify_parent())
-		 * and the debugger may have modified the state (e.g., to arrange for an
-		 * inferior call), thus it's important to check for restarting _after_
-		 * get_signal_to_deliver().
-		 */
-		if ((long) scr->pt.r10 != -1)
-			/*
-			 * A system calls has to be restarted only if one of the error codes
-			 * ERESTARTNOHAND, ERESTARTSYS, or ERESTARTNOINTR is returned.  If r10
-			 * isn't -1 then r8 doesn't hold an error code and we don't need to
-			 * restart the syscall, so we can clear the "restart" flag here.
-			 */
-			restart = 0;
+		handle_syscall_restart(&scr->pt, &ka, (signr > 0));
 
 		if (signr <= 0)
 			break;
 
-		if (unlikely(restart)) {
-			switch (errno) {
-			      case ERESTART_RESTARTBLOCK:
-			      case ERESTARTNOHAND:
-				scr->pt.r8 = EINTR;
-				/* note: scr->pt.r10 is already -1 */
-				break;
-
-			      case ERESTARTSYS:
-				if ((ka.sa.sa_flags & SA_RESTART) == 0) {
-					scr->pt.r8 = EINTR;
-					/* note: scr->pt.r10 is already -1 */
-					break;
-				}
-			      case ERESTARTNOINTR:
-				ia64_decrement_ip(&scr->pt);
-				restart = 0; /* don't restart twice if handle_signal() fails... */
-			}
-		}
-
 		/*
 		 * Whee!  Actually deliver the signal.  If the delivery failed, we need to
 		 * continue to iterate in this loop so we can deliver the SIGSEGV...
@@ -530,23 +495,6 @@ ia64_do_signal (struct sigscratch *scr, long in_syscall)
 		}
 	}
 
-	/* Did we come from a system call? */
-	if (restart) {
-		/* Restart the system call - no handlers present */
-		if (errno == ERESTARTNOHAND || errno == ERESTARTSYS || errno == ERESTARTNOINTR
-		    || errno == ERESTART_RESTARTBLOCK)
-		{
-			/*
-			 * Note: the syscall number is in r15 which is saved in
-			 * pt_regs so all we need to do here is adjust ip so that
-			 * the "break" instruction gets re-executed.
-			 */
-			ia64_decrement_ip(&scr->pt);
-			if (errno == ERESTART_RESTARTBLOCK)
-				scr->pt.r15 = __NR_restart_syscall;
-		}
-	}
-
 	/* if there's no signal to deliver, we just put the saved sigmask
 	 * back */
 	if (current_thread_info()->status & TS_RESTORE_SIGMASK) {
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH RFC 8/8] tile: implement syscall restart generically
  2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
                   ` (6 preceding siblings ...)
  2011-10-23 10:20 ` [PATCH RFC 7/8] ia64: " Jonas Bonn
@ 2011-10-23 10:20 ` Jonas Bonn
  2011-10-31 15:48   ` Chris Metcalf
  7 siblings, 1 reply; 15+ messages in thread
From: Jonas Bonn @ 2011-10-23 10:20 UTC (permalink / raw)
  To: linux-kernel, linux-arch; +Cc: Jonas Bonn, Chris Metcalf


Manipulating task state to effect re-execution of an interrupted syscall
used to be purely architecture specific code.  However, as most arch's
were essentially just making minor adjustments to almost identical logic,
this code could be moved to a common implementation.

The generic variant introduces the function handle_syscall_restart() to be
called after get_signal_to_deliver().  The architecture specific register
manipulations required to effect the actual restart are now implemented
in the generic syscall interface found in asm/syscall.h

This patch transitions this architecture's signal handling code over to
using the generic syscall restart code by:

i)  Implementing the register manipulations in asm/syscall.h
ii) Replacing the restart logic with a call to handle_syscall_restart

Cc: Chris Metcalf <cmetcalf@tilera.com>
Signed-off-by: Jonas Bonn <jonas@southpole.se>
---
 arch/tile/include/asm/syscall.h |   28 ++++++++++++++++++++-
 arch/tile/kernel/signal.c       |   52 +++------------------------------------
 2 files changed, 31 insertions(+), 49 deletions(-)

diff --git a/arch/tile/include/asm/syscall.h b/arch/tile/include/asm/syscall.h
index d35e0dc..428e28c 100644
--- a/arch/tile/include/asm/syscall.h
+++ b/arch/tile/include/asm/syscall.h
@@ -21,6 +21,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <arch/abi.h>
+#include <asm/unistd.h>
 
 /*
  * Only the low 32 bits of orig_r0 are meaningful, so we return int.
@@ -29,15 +30,40 @@
  */
 static inline int syscall_get_nr(struct task_struct *t, struct pt_regs *regs)
 {
-	return regs->regs[TREG_SYSCALL_NR];
+	if (regs->faultnum == INT_SWINT_1)
+		return regs->regs[TREG_SYSCALL_NR];
+	else
+		return -1;
 }
 
 static inline void syscall_rollback(struct task_struct *task,
 				    struct pt_regs *regs)
 {
+	regs->flags |= PT_FLAGS_CALLER_SAVES;
 	regs->regs[0] = regs->orig_r0;
 }
 
+static inline void
+syscall_clear(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->faultnum = INT_SWINT_1_SIGRETURN;
+}
+
+static inline void
+syscall_restart(struct task_struct *task, struct pt_regs *regs)
+{
+	syscall_rollback(task, regs);
+	regs->pc -= 8;
+}
+
+static inline void
+syscall_do_restartblock(struct task_struct *task, struct pt_regs *regs)
+{
+	regs->flags |= PT_FLAGS_CALLER_SAVES;
+	regs->gpr[TREG_SYSCALL_NR] = __NR_restart_syscall;
+	regs->pc -= 8;
+}
+
 static inline long syscall_get_error(struct task_struct *task,
 				     struct pt_regs *regs)
 {
diff --git a/arch/tile/kernel/signal.c b/arch/tile/kernel/signal.c
index bedaf4e..ab1f4e7 100644
--- a/arch/tile/kernel/signal.c
+++ b/arch/tile/kernel/signal.c
@@ -251,29 +251,6 @@ static int handle_signal(unsigned long sig, siginfo_t *info,
 {
 	int ret;
 
-	/* Are we from a system call? */
-	if (regs->faultnum == INT_SWINT_1) {
-		/* If so, check system call restarting.. */
-		switch (regs->regs[0]) {
-		case -ERESTART_RESTARTBLOCK:
-		case -ERESTARTNOHAND:
-			regs->regs[0] = -EINTR;
-			break;
-
-		case -ERESTARTSYS:
-			if (!(ka->sa.sa_flags & SA_RESTART)) {
-				regs->regs[0] = -EINTR;
-				break;
-			}
-			/* fallthrough */
-		case -ERESTARTNOINTR:
-			/* Reload caller-saves to restore r0..r5 and r10. */
-			regs->flags |= PT_FLAGS_CALLER_SAVES;
-			regs->regs[0] = regs->orig_r0;
-			regs->pc -= 8;
-		}
-	}
-
 	/* Set up the stack frame */
 #ifdef CONFIG_COMPAT
 	if (is_compat_task())
@@ -323,6 +300,9 @@ void do_signal(struct pt_regs *regs)
 		oldset = &current->blocked;
 
 	signr = get_signal_to_deliver(&info, &ka, regs, NULL);
+
+	handle_syscall_restart(regs, &ka, (signr > 0));
+
 	if (signr > 0) {
 		/* Whee! Actually deliver the signal.  */
 		if (handle_signal(signr, &info, &ka, oldset, regs) == 0) {
@@ -335,27 +315,7 @@ void do_signal(struct pt_regs *regs)
 			current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		}
 
-		goto done;
-	}
-
-	/* Did we come from a system call? */
-	if (regs->faultnum == INT_SWINT_1) {
-		/* Restart the system call - no handlers present */
-		switch (regs->regs[0]) {
-		case -ERESTARTNOHAND:
-		case -ERESTARTSYS:
-		case -ERESTARTNOINTR:
-			regs->flags |= PT_FLAGS_CALLER_SAVES;
-			regs->regs[0] = regs->orig_r0;
-			regs->pc -= 8;
-			break;
-
-		case -ERESTART_RESTARTBLOCK:
-			regs->flags |= PT_FLAGS_CALLER_SAVES;
-			regs->regs[TREG_SYSCALL_NR] = __NR_restart_syscall;
-			regs->pc -= 8;
-			break;
-		}
+		return;
 	}
 
 	/* If there's no signal to deliver, just put the saved sigmask back. */
@@ -363,10 +323,6 @@ void do_signal(struct pt_regs *regs)
 		current_thread_info()->status &= ~TS_RESTORE_SIGMASK;
 		sigprocmask(SIG_SETMASK, &current->saved_sigmask, NULL);
 	}
-
-done:
-	/* Avoid double syscall restart if there are nested signals. */
-	regs->faultnum = INT_SWINT_1_SIGRETURN;
 }
 
 int show_unhandled_signals = 1;
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 2/8] blackfin: implement syscall restart generically
  2011-10-23 10:19 ` [PATCH RFC 2/8] blackfin: implement syscall restart generically Jonas Bonn
@ 2011-10-26  0:01   ` Mike Frysinger
  2011-10-26  6:16     ` Jonas Bonn
  0 siblings, 1 reply; 15+ messages in thread
From: Mike Frysinger @ 2011-10-26  0:01 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel, linux-arch, uclinux-dist-devel

On Sun, Oct 23, 2011 at 06:19, Jonas Bonn <jonas@southpole.se> wrote:
> --- a/arch/blackfin/include/asm/syscall.h
> +++ b/arch/blackfin/include/asm/syscall.h
>
>  static inline long
>  syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
>  {
> -       return regs->p0;
> +       return regs->orig_p0;
>  }

i'm not sure this is correct.  we set the orig_p0 to -1 when forcing
the syscall to go to restart.  shouldn't syscall_get_nr() still return
the right value ?

>  syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
>                          int error, long val)
>  {
> -       regs->r0 = error ? -error : val;
> +       regs->r0 = error ? error : val;
>  }

this fix is unrelated (and unmentioned in the changelog).  i also see
a bunch of other arches doing this.  so we should pull this change out
into a dedicated patchset, and fix all the arches at the same time.
-mike

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 2/8] blackfin: implement syscall restart generically
  2011-10-26  0:01   ` Mike Frysinger
@ 2011-10-26  6:16     ` Jonas Bonn
  2011-10-26  7:35       ` [uclinux-dist-devel] " Mike Frysinger
  0 siblings, 1 reply; 15+ messages in thread
From: Jonas Bonn @ 2011-10-26  6:16 UTC (permalink / raw)
  To: Mike Frysinger; +Cc: linux-kernel, linux-arch, uclinux-dist-devel


On Tue, 2011-10-25 at 20:01 -0400, Mike Frysinger wrote:
> On Sun, Oct 23, 2011 at 06:19, Jonas Bonn <jonas@southpole.se> wrote:
> > --- a/arch/blackfin/include/asm/syscall.h
> > +++ b/arch/blackfin/include/asm/syscall.h
> >
> >  static inline long
> >  syscall_get_nr(struct task_struct *task, struct pt_regs *regs)
> >  {
> > -       return regs->p0;
> > +       return regs->orig_p0;
> >  }
> 
> i'm not sure this is correct.  we set the orig_p0 to -1 when forcing
> the syscall to go to restart.  shouldn't syscall_get_nr() still return
> the right value ?

No, syscall_get_nr is only valid while processing a syscall (should
return -1 otherwise) and the syscall processing is effectively finished
once you've handled the restart requirements so returning -1 at that
point makes sense... patch 1/8 relies on that fact.

> 
> >  syscall_set_return_value(struct task_struct *task, struct pt_regs *regs,
> >                          int error, long val)
> >  {
> > -       regs->r0 = error ? -error : val;
> > +       regs->r0 = error ? error : val;
> >  }
> 
> this fix is unrelated (and unmentioned in the changelog).  i also see
> a bunch of other arches doing this.  so we should pull this change out
> into a dedicated patchset, and fix all the arches at the same time.

Agreed, this should go into a separate patch.  It's true that some
arches do the negation as above, but those arch's return positive error
numbers and indicate syscall error by setting a separate flag.  I'll
prepare a separate patch for this.

/Jonas


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [uclinux-dist-devel] [PATCH RFC 2/8] blackfin: implement syscall restart generically
  2011-10-26  6:16     ` Jonas Bonn
@ 2011-10-26  7:35       ` Mike Frysinger
  0 siblings, 0 replies; 15+ messages in thread
From: Mike Frysinger @ 2011-10-26  7:35 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-arch, uclinux-dist-devel, linux-kernel

thanks, i look forward to the followups :)
-mike

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 8/8] tile: implement syscall restart generically
  2011-10-23 10:20 ` [PATCH RFC 8/8] tile: " Jonas Bonn
@ 2011-10-31 15:48   ` Chris Metcalf
  0 siblings, 0 replies; 15+ messages in thread
From: Chris Metcalf @ 2011-10-31 15:48 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel, linux-arch

On 10/23/2011 6:20 AM, Jonas Bonn wrote:
> Manipulating task state to effect re-execution of an interrupted syscall
> used to be purely architecture specific code.  However, as most arch's
> were essentially just making minor adjustments to almost identical logic,
> this code could be moved to a common implementation.
>
> The generic variant introduces the function handle_syscall_restart() to be
> called after get_signal_to_deliver().  The architecture specific register
> manipulations required to effect the actual restart are now implemented
> in the generic syscall interface found in asm/syscall.h
>
> This patch transitions this architecture's signal handling code over to
> using the generic syscall restart code by:
>
> i)  Implementing the register manipulations in asm/syscall.h
> ii) Replacing the restart logic with a call to handle_syscall_restart
>
> Cc: Chris Metcalf <cmetcalf@tilera.com>
> Signed-off-by: Jonas Bonn <jonas@southpole.se>

I like the idea, though the patch isn't quite right:

- You need to move the definition of INT_SWINT_1_SIGRETURN from
<asm/sigframe.h> to <asm/syscall.h>.  You then have to add #includes of
<asm/syscall.h> to signal.c and stack.c in arch/tile/kernel.  (And probably
use syscall_clear() in signal.c.)

- In syscall_do_restartblock() you need to use regs->regs[], not regs->gprs[].

-- 
Chris Metcalf, Tilera Corp.
http://www.tilera.com


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 4/8] mips: implement syscall restart generically
  2011-10-23 10:19 ` [PATCH RFC 4/8] mips: " Jonas Bonn
@ 2011-11-04  9:32   ` Ralf Baechle
  2011-11-04  9:50     ` Jonas Bonn
  0 siblings, 1 reply; 15+ messages in thread
From: Ralf Baechle @ 2011-11-04  9:32 UTC (permalink / raw)
  To: Jonas Bonn; +Cc: linux-kernel, linux-arch, linux-mips

On Sun, Oct 23, 2011 at 12:19:58PM +0200, Jonas Bonn wrote:

> Manipulating task state to effect re-execution of an interrupted syscall
> used to be purely architecture specific code.  However, as most arch's
> were essentially just making minor adjustments to almost identical logic,
> this code could be moved to a common implementation.
> 
> The generic variant introduces the function handle_syscall_restart() to be
> called after get_signal_to_deliver().  The architecture specific register
> manipulations required to effect the actual restart are now implemented
> in the generic syscall interface found in asm/syscall.h
> 
> This patch transitions this architecture's signal handling code over to
> using the generic syscall restart code by:
> 
> i)  Implementing the register manipulations in asm/syscall.h
> ii) Replacing the restart logic with a call to handle_syscall_restart

Nice cleanup.

Any reason why you add empty version of syscall_get_arguments and
syscall_set_version?  A non-functional version that causes a silent
failure is way worse than something that fails at compile time.

  Ralf

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH RFC 4/8] mips: implement syscall restart generically
  2011-11-04  9:32   ` Ralf Baechle
@ 2011-11-04  9:50     ` Jonas Bonn
  0 siblings, 0 replies; 15+ messages in thread
From: Jonas Bonn @ 2011-11-04  9:50 UTC (permalink / raw)
  To: Ralf Baechle; +Cc: Jonas Bonn, linux-kernel, linux-arch, linux-mips

On 4 November 2011 10:32, Ralf Baechle <ralf@linux-mips.org> wrote:
>
> Nice cleanup.
>
> Any reason why you add empty version of syscall_get_arguments and
> syscall_set_version?  A non-functional version that causes a silent
> failure is way worse than something that fails at compile time.

Good point... I'll remove those empty functions for this patch series
and look into doing a proper implementation for them later.

Thanks for the review.

/Jonas

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2011-11-04  9:50 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-10-23 10:19 [PATCH RFC 0/8] Signal: harmonize syscall restart logic Jonas Bonn
2011-10-23 10:19 ` [PATCH RFC 1/8] signal: introduce generic " Jonas Bonn
2011-10-23 10:19 ` [PATCH RFC 2/8] blackfin: implement syscall restart generically Jonas Bonn
2011-10-26  0:01   ` Mike Frysinger
2011-10-26  6:16     ` Jonas Bonn
2011-10-26  7:35       ` [uclinux-dist-devel] " Mike Frysinger
2011-10-23 10:19 ` [PATCH RFC 3/8] frv: " Jonas Bonn
2011-10-23 10:19 ` [PATCH RFC 4/8] mips: " Jonas Bonn
2011-11-04  9:32   ` Ralf Baechle
2011-11-04  9:50     ` Jonas Bonn
2011-10-23 10:19 ` [PATCH RFC 5/8] x86: " Jonas Bonn
2011-10-23 10:20 ` [PATCH RFC 6/8] m68k: " Jonas Bonn
2011-10-23 10:20 ` [PATCH RFC 7/8] ia64: " Jonas Bonn
2011-10-23 10:20 ` [PATCH RFC 8/8] tile: " Jonas Bonn
2011-10-31 15:48   ` Chris Metcalf

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).