[PATCH 0/3] c/r: Add s390 support

[PATCH 0/3] c/r: Add s390 support

[Dan Smith]

This set adds checkpoint/restart support for s390.  It is to be applied on
top of Oren's ckpt-v13 set[1] and has been tested with the single and multi-
process tests in user-cr.

1: http://lkml.org/lkml/2009/1/27/227

[PATCH 1/3] s390: Expose a constant for the number of words representing the CRs

[Dan Smith]

We need to use this value in the checkpoint/restart code and would like to
have a constant instead of a magic '3'.

Signed-off-by: Dan Smith <danms@us.ibm.com>
Cc: linux-s390@vger.kernel.org
---
 arch/s390/include/asm/ptrace.h   |    2 ++
 arch/s390/kernel/compat_ptrace.h |    3 ++-
 2 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/arch/s390/include/asm/ptrace.h b/arch/s390/include/asm/ptrace.h
index 5396f9f..d5b0781 100644
--- a/arch/s390/include/asm/ptrace.h
+++ b/arch/s390/include/asm/ptrace.h
@@ -172,6 +172,8 @@
 #define NUM_CRS		16
 #define NUM_ACRS	16
 
+#define NUM_CR_WORDS	3
+
 #define FPR_SIZE	8
 #define FPC_SIZE	4
 #define FPC_PAD_SIZE	4 /* gcc insists on aligning the fpregs */
diff --git a/arch/s390/kernel/compat_ptrace.h b/arch/s390/kernel/compat_ptrace.h
index a2be3a9..123dd66 100644
--- a/arch/s390/kernel/compat_ptrace.h
+++ b/arch/s390/kernel/compat_ptrace.h
@@ -1,10 +1,11 @@
 #ifndef _PTRACE32_H
 #define _PTRACE32_H
 
+#include <asm/ptrace.h>    /* needed for NUM_CR_WORDS */
 #include "compat_linux.h"  /* needed for psw_compat_t */
 
 typedef struct {
-	__u32 cr[3];
+	__u32 cr[NUM_CR_WORDS];
 } per_cr_words32;
 
 typedef struct {
-- 
1.6.1

[PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Dan Smith]

As suggested by Dave[1], this provides us a way to make the copy-in and
copy-out processes symmetric.  CR_COPY_ARRAY() provides us a way to do
the same thing but for arrays.  It's not critical, but it helps us unify
the checkpoint and restart paths for some things.

Changelog:
    Feb 27:
            . Changed CR_COPY() to use assignment, eliminating the need
              for the CR_COPY_BIT() macro
            . Add CR_COPY_ARRAY() macro to help copying register arrays,
              etc
            . Move the macro definitions inside the CR #ifdef
    Feb 25:
            . Changed WARN_ON() to BUILD_BUG_ON()

Signed-off-by: Dan Smith <danms@us.ibm.com>

1: https://lists.linux-foundation.org/pipermail/containers/2009-February/015821.html (all the way at the bottom)
---
 include/linux/checkpoint.h |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 217cf6e..735668f 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -142,6 +142,26 @@ static inline void __task_deny_checkpointing(struct task_struct *task,
 #define task_deny_checkpointing(p)  \
 	__task_deny_checkpointing(p, __FILE__, __LINE__)
 
+#define CR_CPT 1
+#define CR_RST 2
+
+#define CR_COPY(op, a, b)				        \
+	do {							\
+		if (op == CR_CPT)				\
+			a = b;					\
+		else						\
+			b = a;					\
+	} while (0);
+
+#define CR_COPY_ARRAY(op, a, b, count)				\
+	do {							\
+		BUILD_BUG_ON(sizeof(*a) != sizeof(*b));		\
+		if (op == CR_CPT)				\
+			memcpy(a, b, count * sizeof(*a));	\
+		else						\
+			memcpy(b, a, count * sizeof(*a));	\
+	} while (0);
+
 #else
 
 static inline void task_deny_checkpointing(struct task_struct *task) {}
-- 
1.6.1

[PATCH 3/3] c/r: define s390-specific checkpoint-restart code (v7)

[Dan Smith]

Implement the s390 arch-specific checkpoint/restart helpers.  This
is on top of Oren Laadan's c/r code.

With these, I am able to checkpoint and restart simple programs as per
Oren's patch intro.  While on x86 I never had to freeze a single task
to checkpoint it, on s390 I do need to.  That is a prereq for consistent
snapshots (esp with multiple processes) anyway so I don't see that as
a problem.

Changelog:
    Feb 27:
            . Add checkpoint_s390.h
            . Fixed up save and restore of PSW, with the non-address bits
              properly masked out
    Feb 25:
            . Make checkpoint_hdr.h safe for inclusion in userspace
            . Replace comment about vsdo code
            . Add comment about restoring access registers
            . Write and read an empty cr_hdr_head_arch record to appease
              code (mktree) that expects it to be there
            . Utilize NUM_CR_WORDS in checkpoint_hdr.h
    Feb 24:
            . Use CR_COPY() to unify the un/loading of cpu and mm state
            . Fix fprs definition in cr_hdr_cpu
            . Remove debug WARN_ON() from checkpoint.c
    Feb 23:
            . Macro-ize the un/packing of trace flags
            . Fix the crash when externally-linked
            . Break out the restart functions into restart.c
            . Remove unneeded s390_enable_sie() call
    Jan 30:
            . Switched types in cr_hdr_cpu to __u64 etc.
              (Per Oren suggestion)
            . Replaced direct inclusion of structs in
              cr_hdr_cpu with the struct members.
              (Per Oren suggestion)
            . Also ended up adding a bunch of new things
              into restart (mm_segment, ksp, etc) in vain
              attempt to get code using fpu to not segfault
              after restart.

Signed-off-by: Serge E. Hallyn <serue@us.ibm.com>
Signed-off-by: Dan Smith <danms@us.ibm.com>
---
 arch/s390/include/asm/checkpoint_hdr.h |   88 ++++++++++++++++++++++
 arch/s390/include/asm/unistd.h         |    4 +-
 arch/s390/kernel/compat_wrapper.S      |   12 +++
 arch/s390/kernel/syscalls.S            |    2 +
 arch/s390/mm/Makefile                  |    1 +
 arch/s390/mm/checkpoint.c              |  125 ++++++++++++++++++++++++++++++++
 arch/s390/mm/checkpoint_s390.h         |   22 ++++++
 arch/s390/mm/restart.c                 |   81 +++++++++++++++++++++
 checkpoint/Kconfig                     |    2 +-
 9 files changed, 335 insertions(+), 2 deletions(-)
 create mode 100644 arch/s390/include/asm/checkpoint_hdr.h
 create mode 100644 arch/s390/mm/checkpoint.c
 create mode 100644 arch/s390/mm/checkpoint_s390.h
 create mode 100644 arch/s390/mm/restart.c

diff --git a/arch/s390/include/asm/checkpoint_hdr.h b/arch/s390/include/asm/checkpoint_hdr.h
new file mode 100644
index 0000000..0a405c2
--- /dev/null
+++ b/arch/s390/include/asm/checkpoint_hdr.h
@@ -0,0 +1,88 @@
+#ifndef __ASM_S390_CKPT_HDR_H
+#define __ASM_S390_CKPT_HDR_H
+/*
+ *  Checkpoint/restart - architecture specific headers s/390
+ *
+ *  Copyright IBM Corp. 2009
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License.  See the file COPYING in the main directory of the Linux
+ *  distribution for more details.
+ */
+
+#include <linux/types.h>
+#include <asm/ptrace.h>
+
+#ifdef __KERNEL__
+#include <asm/processor.h>
+#else
+#include <sys/user.h>
+#endif
+
+#ifdef __s390x__
+
+/*
+ * Notes
+ * NUM_GPRS defined in <asm/ptrace.h> to be 16
+ * NUM_FPRS defined in <asm/ptrace.h> to be 16
+ * NUM_APRS defined in <asm/ptrace.h> to be 16
+ * NUM_CR_WORDS defined in <asm/ptrace.h> to be 3
+ */
+struct cr_hdr_cpu {
+	__u64 args[1];
+	__u64 gprs[NUM_GPRS];
+	__u64 orig_gpr2;
+	__u16 svcnr;
+	__u16 ilc;
+	__u32 acrs[NUM_ACRS];
+	__u64 ieee_instruction_pointer;
+
+	/* psw_t */
+	__u64 psw_t_mask;
+	__u64 psw_t_addr;
+
+	/* s390_fp_regs_t */
+	__u32 fpc;
+	union {
+		float f;
+		double d;
+		__u64 ui;
+		struct {
+			__u32 fp_hi;
+			__u32 fp_lo;
+		} fp;
+	} fprs[NUM_FPRS];
+
+	/* per_struct */
+	__u64 per_control_regs[NUM_CR_WORDS];
+	__u64 starting_addr;
+	__u64 ending_addr;
+	__u64 address;
+	__u16 perc_atmid;
+	__u8 access_id;
+	__u8 single_step;
+	__u8 instruction_fetch;
+};
+
+struct cr_hdr_mm_context {
+	unsigned long vdso_base;
+	int noexec;
+	int has_pgste;
+	int alloc_pgste;
+	unsigned long asce_bits;
+	unsigned long asce_limit;
+};
+
+struct cr_hdr_head_arch {
+};
+
+#ifdef __KERNEL__
+/* Functions for copying to/from the header structs */
+extern void cr_s390_regs(int op, struct cr_hdr_cpu *hh, struct task_struct *t);
+extern void cr_s390_mm(int op, struct cr_hdr_mm_context *hh,
+		       struct mm_struct *mm);
+#endif
+
+#endif /* __s390x__ */
+
+#endif /* __ASM_S390_CKPT_HDR__H */
diff --git a/arch/s390/include/asm/unistd.h b/arch/s390/include/asm/unistd.h
index c8ad350..ffe64a0 100644
--- a/arch/s390/include/asm/unistd.h
+++ b/arch/s390/include/asm/unistd.h
@@ -265,7 +265,9 @@
 #define __NR_pipe2		325
 #define __NR_dup3		326
 #define __NR_epoll_create1	327
-#define NR_syscalls 328
+#define __NR_checkpoint		328
+#define __NR_restart		329
+#define NR_syscalls 330
 
 /* 
  * There are some system calls that are not present on 64 bit, some
diff --git a/arch/s390/kernel/compat_wrapper.S b/arch/s390/kernel/compat_wrapper.S
index fc2c971..9546a81 100644
--- a/arch/s390/kernel/compat_wrapper.S
+++ b/arch/s390/kernel/compat_wrapper.S
@@ -1767,3 +1767,15 @@ sys_dup3_wrapper:
 sys_epoll_create1_wrapper:
 	lgfr	%r2,%r2			# int
 	jg	sys_epoll_create1	# branch to system call
+
+	.globl sys_checkpoint_wrapper
+sys_checkpoint_wrapper:
+	lgfr	%r2,%r2			# pid_t
+	lgfr	%r3,%r3			# int
+	llgfr	%r4,%r4			# unsigned long
+
+	.globl sys_restart_wrapper
+sys_restart_wrapper:
+	lgfr	%r2,%r2			# int
+	lgfr	%r3,%r3			# int
+	llgfr	%r4,%r4			# unsigned long
diff --git a/arch/s390/kernel/syscalls.S b/arch/s390/kernel/syscalls.S
index 2d61787..54316c8 100644
--- a/arch/s390/kernel/syscalls.S
+++ b/arch/s390/kernel/syscalls.S
@@ -336,3 +336,5 @@ SYSCALL(sys_inotify_init1,sys_inotify_init1,sys_inotify_init1_wrapper)
 SYSCALL(sys_pipe2,sys_pipe2,sys_pipe2_wrapper) /* 325 */
 SYSCALL(sys_dup3,sys_dup3,sys_dup3_wrapper)
 SYSCALL(sys_epoll_create1,sys_epoll_create1,sys_epoll_create1_wrapper)
+SYSCALL(sys_checkpoint,sys_checkpoint,sys_checkpoint_wrapper)
+SYSCALL(sys_restart,sys_restart,sys_restart_wrapper)
diff --git a/arch/s390/mm/Makefile b/arch/s390/mm/Makefile
index 2a74581..040fbb7 100644
--- a/arch/s390/mm/Makefile
+++ b/arch/s390/mm/Makefile
@@ -6,3 +6,4 @@ obj-y	 := init.o fault.o extmem.o mmap.o vmem.o pgtable.o
 obj-$(CONFIG_CMM) += cmm.o
 obj-$(CONFIG_HUGETLB_PAGE) += hugetlbpage.o
 obj-$(CONFIG_PAGE_STATES) += page-states.o
+obj-$(CONFIG_CHECKPOINT_RESTART) += checkpoint.o restart.o
diff --git a/arch/s390/mm/checkpoint.c b/arch/s390/mm/checkpoint.c
new file mode 100644
index 0000000..113c865
--- /dev/null
+++ b/arch/s390/mm/checkpoint.c
@@ -0,0 +1,125 @@
+/*
+ *  Checkpoint/restart - architecture specific support for s390
+ *
+ *  Copyright IBM Corp. 2009
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License.  See the file COPYING in the main directory of the Linux
+ *  distribution for more details.
+ */
+
+#include <linux/checkpoint.h>
+#include <linux/checkpoint_hdr.h>
+#include <linux/kernel.h>
+#include <asm/system.h>
+#include <asm/pgtable.h>
+
+#include "checkpoint_s390.h"
+
+void cr_s390_regs(int op, struct cr_hdr_cpu *hh, struct task_struct *t)
+{
+	struct pt_regs *regs = task_pt_regs(t);
+	struct thread_struct *thr = &t->thread;
+
+	/* Save the whole PSW to facilitate forensic debugging, but only
+	 * restore the address portion to avoid letting userspace do
+	 * bad things by manipulating its value.
+	 */
+	if (op == CR_CPT) {
+		CR_COPY(op, hh->psw_t_addr, regs->psw.addr);
+	} else {
+		regs->psw.addr &= ~PSW_ADDR_INSN;
+		regs->psw.addr |= hh->psw_t_addr;
+	}
+
+	CR_COPY(op, hh->args[0], regs->args[0]);
+	CR_COPY(op, hh->orig_gpr2, regs->orig_gpr2);
+	CR_COPY(op, hh->svcnr, regs->svcnr);
+	CR_COPY(op, hh->ilc, regs->ilc);
+	CR_COPY(op, hh->ieee_instruction_pointer,
+		thr->ieee_instruction_pointer);
+	CR_COPY(op, hh->psw_t_mask, regs->psw.mask);
+	CR_COPY(op, hh->fpc, thr->fp_regs.fpc);
+	CR_COPY(op, hh->starting_addr, thr->per_info.starting_addr);
+	CR_COPY(op, hh->ending_addr, thr->per_info.ending_addr);
+	CR_COPY(op, hh->address, thr->per_info.lowcore.words.address);
+	CR_COPY(op, hh->perc_atmid, thr->per_info.lowcore.words.perc_atmid);
+	CR_COPY(op, hh->access_id, thr->per_info.lowcore.words.access_id);
+	CR_COPY(op, hh->single_step, thr->per_info.single_step);
+	CR_COPY(op, hh->instruction_fetch, thr->per_info.instruction_fetch);
+
+	CR_COPY_ARRAY(op, hh->gprs, regs->gprs, NUM_GPRS);
+ 	CR_COPY_ARRAY(op, hh->fprs, thr->fp_regs.fprs, NUM_FPRS);
+	CR_COPY_ARRAY(op, hh->acrs, thr->acrs, NUM_ACRS);
+	CR_COPY_ARRAY(op, hh->per_control_regs,
+		      thr->per_info.control_regs.words.cr, NUM_CR_WORDS);
+
+}
+
+void cr_s390_mm(int op, struct cr_hdr_mm_context *hh, struct mm_struct *mm)
+{
+	CR_COPY(op, hh->noexec, mm->context.noexec);
+	CR_COPY(op, hh->has_pgste, mm->context.has_pgste);
+	CR_COPY(op, hh->alloc_pgste, mm->context.alloc_pgste);
+	CR_COPY(op, hh->asce_bits, mm->context.asce_bits);
+	CR_COPY(op, hh->asce_limit, mm->context.asce_limit);
+}
+
+int cr_write_thread(struct cr_ctx *ctx, struct task_struct *t)
+{
+	return 0;
+}
+
+/* dump the cpu state and registers of a given task */
+int cr_write_cpu(struct cr_ctx *ctx, struct task_struct *t)
+{
+	struct cr_hdr h;
+	struct cr_hdr_cpu *hh = cr_hbuf_get(ctx, sizeof(*hh));
+	int ret;
+
+	h.type = CR_HDR_CPU;
+	h.len = sizeof(*hh);
+	h.parent = task_pid_vnr(t);
+
+	cr_s390_regs(CR_CPT, hh, t);
+
+	ret = cr_write_obj(ctx, &h, hh);
+	cr_hbuf_put(ctx, sizeof(*hh));
+
+	return ret;
+}
+
+/* Write an empty header since it is assumed to be there */
+int cr_write_head_arch(struct cr_ctx *ctx)
+{
+	struct cr_hdr h;
+	struct cr_hdr_head_arch *hh = cr_hbuf_get(ctx, sizeof(*hh));
+	int ret;
+
+	h.type = CR_HDR_HEAD_ARCH;
+	h.len = sizeof(*hh);
+	h.parent = 0;
+
+	ret = cr_write_obj(ctx, &h, &hh);
+	cr_hbuf_put(ctx, sizeof(*hh));
+
+	return ret;
+}
+
+int cr_write_mm_context(struct cr_ctx *ctx, struct mm_struct *mm, int parent)
+{
+	struct cr_hdr h;
+	struct cr_hdr_mm_context *hh = cr_hbuf_get(ctx, sizeof(*hh));
+	int ret;
+
+	h.type = CR_HDR_MM_CONTEXT;
+	h.len = sizeof(*hh);
+	h.parent = parent;
+
+	cr_s390_mm(CR_CPT, hh, mm);
+
+	ret = cr_write_obj(ctx, &h, hh);
+	cr_hbuf_put(ctx, sizeof(*hh));
+
+	return ret;
+}
diff --git a/arch/s390/mm/checkpoint_s390.h b/arch/s390/mm/checkpoint_s390.h
new file mode 100644
index 0000000..52a5e6f
--- /dev/null
+++ b/arch/s390/mm/checkpoint_s390.h
@@ -0,0 +1,22 @@
+/*
+ *  Checkpoint/restart - architecture specific support for s390
+ *
+ *  Copyright IBM Corp. 2009
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License.  See the file COPYING in the main directory of the Linux
+ *  distribution for more details.
+ */
+
+#ifndef _S390_CHECKPOINT_H
+#define _S390_CHECKPOINT_H
+
+#include <linux/checkpoint_hdr.h>
+#include <linux/sched.h>
+#include <linux/mm_types.h>
+
+extern void cr_s390_regs(int op, struct cr_hdr_cpu *hh, struct task_struct *t);
+extern void cr_s390_mm(int op, struct cr_hdr_mm_context *hh,
+		       struct mm_struct *mm);
+
+#endif /* _S390_CHECKPOINT_H */
diff --git a/arch/s390/mm/restart.c b/arch/s390/mm/restart.c
new file mode 100644
index 0000000..7da4111
--- /dev/null
+++ b/arch/s390/mm/restart.c
@@ -0,0 +1,81 @@
+/*
+ *  Checkpoint/restart - architecture specific support for s390
+ *
+ *  Copyright IBM Corp. 2009
+ *
+ *  This file is subject to the terms and conditions of the GNU General Public
+ *  License.  See the file COPYING in the main directory of the Linux
+ *  distribution for more details.
+ */
+
+#include <linux/checkpoint.h>
+#include <linux/checkpoint_hdr.h>
+#include <linux/kernel.h>
+#include <asm/system.h>
+#include <asm/pgtable.h>
+
+#include "checkpoint_s390.h"
+
+int cr_read_thread(struct cr_ctx *ctx)
+{
+	return 0;
+}
+
+int cr_read_cpu(struct cr_ctx *ctx)
+{
+	struct cr_hdr_cpu *hh = cr_hbuf_get(ctx, sizeof(*hh));
+	int parent, ret;
+
+	parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_CPU);
+	if  (parent < 0) {
+		ret = parent;
+		goto out;
+	}
+	ret = 0;
+
+	cr_s390_regs(CR_RST, hh, current);
+
+	/* s390 does not restore the access registers after a syscall,
+	 * but does on a task switch.  Since we're switching tasks (in
+	 * a way), we need to replicate that behavior here.
+	 */
+	restore_access_regs(hh->acrs);
+out:
+	cr_hbuf_put(ctx, sizeof(*hh));
+	return ret;
+}
+
+int cr_read_head_arch(struct cr_ctx *ctx)
+{
+	struct cr_hdr_head_arch *hh = cr_hbuf_get(ctx, sizeof(*hh));
+	int parent, ret = 0;
+
+	parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_HEAD_ARCH);
+	if (parent < 0)
+		ret = parent;
+
+	cr_hbuf_put(ctx, sizeof(*hh));
+
+	return ret;
+}
+
+
+int cr_read_mm_context(struct cr_ctx *ctx, struct mm_struct *mm, int rparent)
+{
+	struct cr_hdr_mm_context *hh = cr_hbuf_get(ctx, sizeof(*hh));
+	int parent, ret = -EINVAL;
+
+	parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_MM_CONTEXT);
+	if (parent < 0) {
+		ret = parent;
+		goto out;
+	}
+	if (parent != rparent)
+		goto out;
+
+	cr_s390_mm(CR_RST, hh, mm);
+	ret = 0;
+ out:
+	cr_hbuf_put(ctx, sizeof(*hh));
+	return ret;
+}
diff --git a/checkpoint/Kconfig b/checkpoint/Kconfig
index ffaa635..cb1d29d 100644
--- a/checkpoint/Kconfig
+++ b/checkpoint/Kconfig
@@ -1,7 +1,7 @@
 config CHECKPOINT_RESTART
 	prompt "Enable checkpoint/restart (EXPERIMENTAL)"
 	def_bool n
-	depends on X86_32 && EXPERIMENTAL
+	depends on (X86_32 || (S390 && 64BIT)) && EXPERIMENTAL
 	help
 	  Application checkpoint/restart is the ability to save the
 	  state of a running application so that it can later resume
-- 
1.6.1

Re: [PATCH 1/3] s390: Expose a constant for the number of words representing the CRs

[Dave Hansen]

On Tue, 2009-03-03 at 10:56 -0500, Dan Smith wrote:
> We need to use this value in the checkpoint/restart code and would like to
> have a constant instead of a magic '3'.

This doesn't do a lot of good unless it gets used in the base s390 code.
Otherwise, why not just define it near its use in the c/r code?

-- Dave

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Dave Hansen]

On Tue, 2009-03-03 at 10:56 -0500, Dan Smith wrote:
> As suggested by Dave[1], this provides us a way to make the copy-in and
> copy-out processes symmetric.  CR_COPY_ARRAY() provides us a way to do
> the same thing but for arrays.  It's not critical, but it helps us unify
> the checkpoint and restart paths for some things.

Did you convince Nathan that this ends up being a good idea?

-- Dave

Re: [PATCH 3/3] c/r: define s390-specific checkpoint-restart code (v7)

[Serge E. Hallyn]

Quoting Dan Smith (danms@us.ibm.com):
> Implement the s390 arch-specific checkpoint/restart helpers.  This
> is on top of Oren Laadan's c/r code.

Working nicely for me, thanks.

-serge

Re: [PATCH 1/3] s390: Expose a constant for the number of words representing the CRs

[Dan Smith]

DH> This doesn't do a lot of good unless it gets used in the base s390
DH> code.  Otherwise, why not just define it near its use in the c/r
DH> code?

I intended to replace all uses, but I just found one place I missed,
but I think that's it.  If you know of other places that it's used in
the s390 code, other than in compat_ptrace.h and ptrace.h please point
them out.

-- 
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Dan Smith]

DH> Did you convince Nathan that this ends up being a good idea?

Technically he hasn't seen this version, but my hopes are not high
that he will change his mind.  If the feedback is that they're not
liked, I'll happily remove them.

-- 
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com

Re: [PATCH 1/3] s390: Expose a constant for the number of words representing the CRs

[Dave Hansen]

On Tue, 2009-03-03 at 16:56 -0800, Dan Smith wrote:
> DH> This doesn't do a lot of good unless it gets used in the base s390
> DH> code.  Otherwise, why not just define it near its use in the c/r
> DH> code?
> 
> I intended to replace all uses, but I just found one place I missed,
> but I think that's it.  If you know of other places that it's used in
> the s390 code, other than in compat_ptrace.h and ptrace.h please point
> them out.

My only knowledge of where else it was used came from you. :)

-- Dave

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Dave Hansen]

On Tue, 2009-03-03 at 16:57 -0800, Dan Smith wrote:
> DH> Did you convince Nathan that this ends up being a good idea?
> 
> Technically he hasn't seen this version, but my hopes are not high
> that he will change his mind.  If the feedback is that they're not
> liked, I'll happily remove them.

I just figure if Nathan feels that strongly that we'll encounter more
people who feel even more so.  So, I was curious if he changed his mind
somehow.

-- Dave

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Serge E. Hallyn]

Quoting Dave Hansen (dave@linux.vnet.ibm.com):
> On Tue, 2009-03-03 at 16:57 -0800, Dan Smith wrote:
> > DH> Did you convince Nathan that this ends up being a good idea?
> > 
> > Technically he hasn't seen this version, but my hopes are not high
> > that he will change his mind.  If the feedback is that they're not
> > liked, I'll happily remove them.
> 
> I just figure if Nathan feels that strongly that we'll encounter more
> people who feel even more so.  So, I was curious if he changed his mind
> somehow.

I maintain however that two strong advantages of moving the checkpoint
and restart of simple registers etc into a single function are:

	1. we won't forget to add (or accidentally lose) one or the
		other
	2. any actual special handling at checkpoint or restart, like
		the loading of access registers at restart on s390x,
		stand out

-serge

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Nathan Lynch]

On Tue, 03 Mar 2009 17:00:37 -0800
Dave Hansen <dave@linux.vnet.ibm.com> wrote:

> On Tue, 2009-03-03 at 16:57 -0800, Dan Smith wrote:
> > DH> Did you convince Nathan that this ends up being a good idea?
> > 
> > Technically he hasn't seen this version, but my hopes are not high
> > that he will change his mind.  If the feedback is that they're not
> > liked, I'll happily remove them.
> 
> I just figure if Nathan feels that strongly that we'll encounter more
> people who feel even more so.  So, I was curious if he changed his mind
> somehow.

No, not really, sorry.

I understand why it's nice for the developer to have this sort of
helper, but I don't think it's nice for someone trying to review or
debug the code.

Surely discussing these macros has already consumed more developer time
than they would ever save?  :)

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Nathan Lynch]

Hi Dan.

Dan Smith <danms@us.ibm.com> wrote:
> +#define CR_CPT 1
> +#define CR_RST 2
> +
> +#define CR_COPY(op, a, b)				        \
> +	do {							\
> +		if (op == CR_CPT)				\
> +			a = b;					\
> +		else						\
> +			b = a;					\
> +	} while (0);

Drop the semicolon ^


> +
> +#define CR_COPY_ARRAY(op, a, b, count)				\
> +	do {							\
> +		BUILD_BUG_ON(sizeof(*a) != sizeof(*b));		\
> +		if (op == CR_CPT)				\
> +			memcpy(a, b, count * sizeof(*a));	\
> +		else						\
> +			memcpy(b, a, count * sizeof(*a));	\
> +	} while (0);
> +

You might want to employ __must_be_array() or similar to catch misuse.

Misuse might also be prevented by providing some documentation :)

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Dave Hansen]

On Wed, 2009-03-04 at 13:53 -0600, Nathan Lynch wrote:
> On Tue, 03 Mar 2009 17:00:37 -0800
> Dave Hansen <dave@linux.vnet.ibm.com> wrote:
> 
> > On Tue, 2009-03-03 at 16:57 -0800, Dan Smith wrote:
> > > DH> Did you convince Nathan that this ends up being a good idea?
> > > 
> > > Technically he hasn't seen this version, but my hopes are not high
> > > that he will change his mind.  If the feedback is that they're not
> > > liked, I'll happily remove them.
> > 
> > I just figure if Nathan feels that strongly that we'll encounter more
> > people who feel even more so.  So, I was curious if he changed his mind
> > somehow.
> 
> No, not really, sorry.
> 
> I understand why it's nice for the developer to have this sort of
> helper, but I don't think it's nice for someone trying to review or
> debug the code.

That's funny.  I've only reviewed and debugged these things, but I don't
think I've actually written any code that would have used these macros!
As someone trying to debug and review, I love how this looks.

It gets the point across much more clearly about what is going on to me
as a reviewer and I appreciate that.  memcpy()s contain a lot of gunk
that my brain can't parse easily, but this is rather clean, and it
*HALVES* the number of lines of code I have to look at.  

> Surely discussing these macros has already consumed more developer time
> than they would ever save?  :)

That's exactly my point.  We're not trying to save development time here
at all.  My argument is that this reduces the maintenance and review
burden.  

-- Dave

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Dan Smith]

NL> Drop the semicolon ^

Thanks.

NL> You might want to employ __must_be_array() or similar to catch
NL> misuse.

Ooh, nice.

NL> Misuse might also be prevented by providing some documentation :)

Agreed.

Thanks!

-- 
Dan Smith
IBM Linux Technology Center
email: danms@us.ibm.com

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Dave Hansen (dave@linux.vnet.ibm.com):
>> On Tue, 2009-03-03 at 16:57 -0800, Dan Smith wrote:
>>> DH> Did you convince Nathan that this ends up being a good idea?
>>>
>>> Technically he hasn't seen this version, but my hopes are not high
>>> that he will change his mind.  If the feedback is that they're not
>>> liked, I'll happily remove them.
>> I just figure if Nathan feels that strongly that we'll encounter more
>> people who feel even more so.  So, I was curious if he changed his mind
>> somehow.
> 
> I maintain however that two strong advantages of moving the checkpoint
> and restart of simple registers etc into a single function are:
> 
> 	1. we won't forget to add (or accidentally lose) one or the
> 		other
> 	2. any actual special handling at checkpoint or restart, like
> 		the loading of access registers at restart on s390x,
> 		stand out
> 

I, too, think that this scheme is elegant, and at the same time I, too,
think that it obfuscates the code. Since I only touch arch-dependent code
only if I really really must, I don't have strong opinion about it ;)

However, a problem with this scheme is that checkpoint and restart
are not fully symmetric -- on restart we must sanitize the input data
before restoring the registers to that data. I'm not familiar with
s390, but it is likely that by not doing so we create a security issue.

Oren.

Re: [PATCH 2/3] c/r: Add CR_COPY() macro (v3)

[Serge E. Hallyn]

Quoting Oren Laadan (orenl@cs.columbia.edu):
> 
> 
> Serge E. Hallyn wrote:
> > Quoting Dave Hansen (dave@linux.vnet.ibm.com):
> >> On Tue, 2009-03-03 at 16:57 -0800, Dan Smith wrote:
> >>> DH> Did you convince Nathan that this ends up being a good idea?
> >>>
> >>> Technically he hasn't seen this version, but my hopes are not high
> >>> that he will change his mind.  If the feedback is that they're not
> >>> liked, I'll happily remove them.
> >> I just figure if Nathan feels that strongly that we'll encounter more
> >> people who feel even more so.  So, I was curious if he changed his mind
> >> somehow.
> > 
> > I maintain however that two strong advantages of moving the checkpoint
> > and restart of simple registers etc into a single function are:
> > 
> > 	1. we won't forget to add (or accidentally lose) one or the
> > 		other
> > 	2. any actual special handling at checkpoint or restart, like
> > 		the loading of access registers at restart on s390x,
> > 		stand out
> > 
> 
> I, too, think that this scheme is elegant, and at the same time I, too,
> think that it obfuscates the code. Since I only touch arch-dependent code
> only if I really really must, I don't have strong opinion about it ;)
> 
> However, a problem with this scheme is that checkpoint and restart
> are not fully symmetric -- on restart we must sanitize the input data
> before restoring the registers to that data. I'm not familiar with
> s390, but it is likely that by not doing so we create a security issue.
> 
> Oren.

But that's exactly why I think CR_COPY() helps - the sanitation is
explicit next to some boring CR_COPY()s.  It becomes clearer that
it is being done.

Anyway we've got plenty of other, bigger hurdles to clear, so while
I do have a strong opinion, I'm not planning on pushing hard either
way.

thanks,
-serge