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

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

[Dan Smith]

This set includes an updated version of the s390 patch as well as a new
one that adds the common CR_COPY() macros suggested by Dave.

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

[Dan Smith]

As suggested by Dave, this provides us a way to make the copy-in and
copy-out processes symmetric.  I also added CR_COPY_BIT() to use with
the s390 bitfields, since we can't memcpy() those.

Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 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..b94422a 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -149,4 +149,24 @@ static inline void process_deny_checkpointing(struct task_struct *task) {}
 
 #endif
 
+#define CR_CPT 1
+#define CR_RST 2
+
+#define CR_COPY(op, a, b)				\
+	do {						\
+		WARN_ON(sizeof(a) != sizeof(b));	\
+		if (op == CR_CPT)			\
+			memcpy(&a, &b, sizeof(a));	\
+		else					\
+			memcpy(&b, &a, sizeof(a));	\
+	} while (0);
+
+#define CR_COPY_BIT(op, a, b)				\
+	do {						\
+		if (op == CR_CPT)			\
+			a = b;				\
+		else					\
+			b = a;				\
+	} while (0);
+
 #endif /* _CHECKPOINT_CKPT_H_ */
-- 
1.6.1

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

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> As suggested by Dave, this provides us a way to make the copy-in and
> copy-out processes symmetric.  I also added CR_COPY_BIT() to use with
> the s390 bitfields, since we can't memcpy() those.
> 
> Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  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..b94422a 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -149,4 +149,24 @@ static inline void process_deny_checkpointing(struct task_struct *task) {}
> 
>  #endif
> 
> +#define CR_CPT 1
> +#define CR_RST 2
> +
> +#define CR_COPY(op, a, b)				\
> +	do {						\
> +		WARN_ON(sizeof(a) != sizeof(b));	\

Can this be a BUILD_BUG_ON()?

> +		if (op == CR_CPT)			\
> +			memcpy(&a, &b, sizeof(a));	\
> +		else					\
> +			memcpy(&b, &a, sizeof(a));	\
> +	} while (0);
> +
> +#define CR_COPY_BIT(op, a, b)				\
> +	do {						\

maybe add a 
	BUILD_BUG_ON(typeof(a)!=typeof(b));
?

> +		if (op == CR_CPT)			\
> +			a = b;				\
> +		else					\
> +			b = a;				\
> +	} while (0);
> +
>  #endif /* _CHECKPOINT_CKPT_H_ */
> -- 
> 1.6.1
> 
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

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

[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 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-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Dan Smith <danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 arch/s390/include/asm/checkpoint_hdr.h |   84 +++++++++++++++++++++++++
 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              |  104 ++++++++++++++++++++++++++++++++
 arch/s390/mm/restart.c                 |   71 ++++++++++++++++++++++
 checkpoint/Kconfig                     |    2 +-
 8 files changed, 278 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/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..cf84e9c
--- /dev/null
+++ b/arch/s390/include/asm/checkpoint_hdr.h
@@ -0,0 +1,84 @@
+#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>
+
+#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
+ */
+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[3];
+	__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 {
+};
+
+/* 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 /* __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..19c6ae7
--- /dev/null
+++ b/arch/s390/mm/checkpoint.c
@@ -0,0 +1,104 @@
+/*
+ *  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>
+
+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;
+
+	CR_COPY(op, hh->fpc, thr->fp_regs.fpc);
+	CR_COPY(op, hh->fprs, thr->fp_regs.fprs);
+	CR_COPY(op, hh->acrs, thr->acrs);
+	CR_COPY(op, hh->psw_t_mask, regs->psw.mask);
+	CR_COPY(op, hh->psw_t_addr, regs->psw.addr);
+	CR_COPY(op, hh->args, regs->args);
+	CR_COPY(op, hh->svcnr, regs->svcnr);
+	CR_COPY(op, hh->ilc, regs->ilc);
+	CR_COPY(op, hh->gprs, regs->gprs);
+	CR_COPY(op, hh->orig_gpr2, regs->orig_gpr2);
+	CR_COPY(op, hh->per_control_regs, thr->per_info.control_regs.words);
+	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->perc_atmid, thr->per_info.lowcore.words.perc_atmid);
+	CR_COPY(op, hh->address, thr->per_info.lowcore.words.address);
+	CR_COPY(op, hh->access_id, thr->per_info.lowcore.words.access_id);
+	CR_COPY(op, hh->ieee_instruction_pointer,
+		thr->ieee_instruction_pointer);
+
+	CR_COPY_BIT(op, hh->single_step, thr->per_info.single_step);
+	CR_COPY_BIT(op, hh->instruction_fetch,
+		    thr->per_info.instruction_fetch);
+}
+
+void cr_s390_mm(int op, struct cr_hdr_mm_context *hh, struct mm_struct *mm)
+{
+#if 0
+	CR_COPY(op, hh->vdso_base, mm->context.vdso_base);
+#endif
+	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;
+}
+
+int cr_write_head_arch(struct cr_ctx *ctx)
+{
+	return 0;
+}
+
+/* Nothing to do for mm context state */
+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/restart.c b/arch/s390/mm/restart.c
new file mode 100644
index 0000000..d400a72
--- /dev/null
+++ b/arch/s390/mm/restart.c
@@ -0,0 +1,71 @@
+/*
+ *  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>
+
+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);
+
+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));
+	struct pt_regs *regs = task_pt_regs(current);
+	int parent, ret;
+
+	parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_CPU);
+	if  (parent < 0) {
+		ret = parent;
+		goto out;
+	}
+	ret = 0;
+
+	regs->psw.addr &= ~PSW_ADDR_INSN;
+	cr_s390_regs(CR_RST, hh, current);
+	restore_access_regs(hh->acrs);
+out:
+	cr_hbuf_put(ctx, sizeof(*hh));
+	return ret;
+}
+
+int cr_read_head_arch(struct cr_ctx *ctx)
+{
+	return 0;
+}
+
+
+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 2/2] c/r: define s390-specific checkpoint-restart code (v5)

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):

Thanks, Dan, this looks very nice with the CR_COPY, plus you
fixed the bug I couldn't find :)  A few comments:

> +struct cr_hdr_cpu {
> +	__u64 args[1];

Dave wanted this to not be an array, right?

> +	__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;

Since this is a union, and you don't deal with its
members but just memcpy it, why not just change it
to

	__u64 fprs[NUM_FPRS];

and not have a union at all?

> +		struct {
> +			__u32 fp_hi;
> +			__u32 fp_lo;
> +		} fp;
> +	} fprs[NUM_FPRS];
> +
> +	/* per_struct */
> +	__u64 per_control_regs[3];

I assume Dave still wants you to add a

#define PER_NUM_REGS 3

into the arch/s390/include/asm/processor.h or something.

> +void cr_s390_mm(int op, struct cr_hdr_mm_context *hh, struct mm_struct *mm)
> +{
> +#if 0

The comment about why this is ifdefed out for now should stay
here.

> +	CR_COPY(op, hh->vdso_base, mm->context.vdso_base);
> +#endif
> +	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);

BTW - I know Dave mentioened using a generic helper for this
often-used stanza above, but I continue to be against that
bc the helper ends up having to take a bunch of eye-numbing
arguments and I think the code ends up hard to read.  But
maybe you can think of a way to make that clearer...

> +
> +	cr_s390_regs(CR_CPT, hh, t);
> +
> +	ret = cr_write_obj(ctx, &h, hh);
> +	cr_hbuf_put(ctx, sizeof(*hh));
> +
> +	return ret;
> +}
> +
> +int cr_write_head_arch(struct cr_ctx *ctx)
> +{
> +	return 0;
> +}
> +
> +/* Nothing to do for mm context state */

The above comment is clearly wrong :)

> +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;
> +}
...
> +int cr_read_cpu(struct cr_ctx *ctx)
> +{
> +	struct cr_hdr_cpu *hh = cr_hbuf_get(ctx, sizeof(*hh));
> +	struct pt_regs *regs = task_pt_regs(current);
> +	int parent, ret;
> +
> +	parent = cr_read_obj_type(ctx, hh, sizeof(*hh), CR_HDR_CPU);
> +	if  (parent < 0) {
> +		ret = parent;
> +		goto out;
> +	}
> +	ret = 0;
> +
> +	regs->psw.addr &= ~PSW_ADDR_INSN;
> +	cr_s390_regs(CR_RST, hh, current);
> +	restore_access_regs(hh->acrs);

Just a comment explaining why?

> +out:
> +	cr_hbuf_put(ctx, sizeof(*hh));
> +	return ret;
> +}

thanks
-serge

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

[Dave Hansen]

On Tue, 2009-02-24 at 13:37 -0600, Serge E. Hallyn wrote:
> 
> > +/* 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);
> 
> BTW - I know Dave mentioened using a generic helper for this
> often-used stanza above, but I continue to be against that
> bc the helper ends up having to take a bunch of eye-numbing
> arguments and I think the code ends up hard to read.  But
> maybe you can think of a way to make that clearer...

Yeah, that's true.  The plethora of types also makes it hard to do with
a function.  For plain readability you can't beat what we already have
there.

But, I do still wonder why we need the .parent member in the cr_hdr
itself.  Shouldn't that be something that gets pushed down to where we
can actually describe it, like in the cr_hdr_foo structures?

-- Dave

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

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> On Tue, 2009-02-24 at 13:37 -0600, Serge E. Hallyn wrote:
> > 
> > > +/* 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);
> > 
> > BTW - I know Dave mentioened using a generic helper for this
> > often-used stanza above, but I continue to be against that
> > bc the helper ends up having to take a bunch of eye-numbing
> > arguments and I think the code ends up hard to read.  But
> > maybe you can think of a way to make that clearer...
> 
> Yeah, that's true.  The plethora of types also makes it hard to do with
> a function.  For plain readability you can't beat what we already have
> there.
> 
> But, I do still wonder why we need the .parent member in the cr_hdr
> itself.  Shouldn't that be something that gets pushed down to where we
> can actually describe it, like in the cr_hdr_foo structures?

Hmm, yeah, moving it would have two upsides:  make the common
stanza a line shorter, and let us use more descriptive names
for the variables.  parent is really not helpful unless you've
spent years with the code...

OTOH I'm not eager to make such a change right now only to find
months later that there was a good reason to keep it in the hdr
after all  :)

-serge

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

[Dave Hansen]

On Tue, 2009-02-24 at 14:04 -0600, Serge E. Hallyn wrote:
> OTOH I'm not eager to make such a change right now only to find
> months later that there was a good reason to keep it in the hdr
> after all  :)

The thing that bothers me about all of these things is that we can't
truly evaluate them on their merits because we can't see how they are
expected to be used in the future.  Surely there are multiple ways we
can implement details of the incremental checkpoint.

-- Dave

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

[Serge E. Hallyn]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> On Tue, 2009-02-24 at 14:04 -0600, Serge E. Hallyn wrote:
> > OTOH I'm not eager to make such a change right now only to find
> > months later that there was a good reason to keep it in the hdr
> > after all  :)
> 
> The thing that bothers me about all of these things is that we can't
> truly evaluate them on their merits because we can't see how they are
> expected to be used in the future.  Surely there are multiple ways we
> can implement details of the incremental checkpoint.
> 
> -- Dave

Oh I think that was a bogus guess on my part anyway.

Like I say I don't want to encourage churn for the sake of churn
at this point, but you've got me thinking that moving parent into
the details and giving it a more useful name could *really*
dilute some of the mystery in these patches.

It's sounding good to me...

-serge

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

[Dan Smith]

>> +struct cr_hdr_cpu {
>> +	__u64 args[1];

SH> Dave wanted this to not be an array, right?

I think he was okay with it since it matched what is in the rest of
the s390 code.  I think that the use of the CR_COPY() macros makes it
nicer to have matching types as closely as possible.

>> +	union {
>> +		float f;
>> +		double d;
>> +		__u64 ui;

SH> Since this is a union, and you don't deal with its members but
SH> just memcpy it, why not just change it to

That's a fair argument, although I was thinking that there was some
expectation of being able to include this in userspace at some point
to inspect the contents of the CR stream.  The #ifdef __KERNEL__ at
the top of the file makes me think that's true.  If so, does it make
sense to leave this as-is for easier inspection of the contents?

>> +		struct {
>> +			__u32 fp_hi;
>> +			__u32 fp_lo;
>> +		} fp;
>> +	} fprs[NUM_FPRS];
>> +
>> +	/* per_struct */
>> +	__u64 per_control_regs[3];

SH> I assume Dave still wants you to add a

SH> #define PER_NUM_REGS 3

SH> into the arch/s390/include/asm/processor.h or something.

Ah, right, I thought I was getting out of fixing that with the
CR_COPY() macro, but I forgot about this one :)

>> +void cr_s390_mm(int op, struct cr_hdr_mm_context *hh, struct mm_struct *mm)
>> +{
>> +#if 0

SH> The comment about why this is ifdefed out for now should stay
SH> here.

Yep.

>> +/* Nothing to do for mm context state */

SH> The above comment is clearly wrong :)

Oops :)

>> +	restore_access_regs(hh->acrs);

SH> Just a comment explaining why?

But it's *so* obvious, right?  <ahem> ... Yeah, agreed :)

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

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

[Serge E. Hallyn]

Quoting Dan Smith (danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
> >> +struct cr_hdr_cpu {
> >> +	__u64 args[1];
> 
> SH> Dave wanted this to not be an array, right?
> 
> I think he was okay with it since it matched what is in the rest of
> the s390 code.  I think that the use of the CR_COPY() macros makes it
> nicer to have matching types as closely as possible.

Ok with me.  Dave can speak up if he needs to :)

> >> +	union {
> >> +		float f;
> >> +		double d;
> >> +		__u64 ui;
> 
> SH> Since this is a union, and you don't deal with its members but
> SH> just memcpy it, why not just change it to
> 
> That's a fair argument, although I was thinking that there was some
> expectation of being able to include this in userspace at some point
> to inspect the contents of the CR stream.  The #ifdef __KERNEL__ at
> the top of the file makes me think that's true.  If so, does it make
> sense to leave this as-is for easier inspection of the contents?

But what is userspace supposed to gain from seeing that in the
headers?  No matter how many other ways we represent the data
containined in a fprs union, all we know based on the checkpoint
image is what the size of NUM_FPRS*sizeof(*fprs) is, right?

Or do you mean the programmers will see that and be able to tell
more easiliy where in ptrace.h the corresponding union is?

-serge

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

[Dan Smith]

SH> Or do you mean the programmers will see that and be able to tell
SH> more easiliy where in ptrace.h the corresponding union is?

Right, I was thinking about it from the point of view of letting the
user of that structure (which could be in userspace) know what the
contents are.  In other words, I don't see why we should have some of
the fields be opaque outside of the kernel and not others (not
including any that really can't be interpreted by userspace in a
meaningful way).

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org

Re: [PATCH 0/2] c/r: Add s390 support

[Dan Smith]

DS> This set includes an updated version of the s390 patch as well as
DS> a new one that adds the common CR_COPY() macros suggested by Dave.

For the sake of comparison, the (trimmed) diffstat of the original
patch was:

 arch/s390/include/asm/checkpoint_hdr.h |   99 +++++++++++++++++++++++++++
 arch/s390/mm/checkpoint.c              |  106 +++++++++++++++++++++++++++++
 arch/s390/mm/restart.c                 |  117 ++++++++++++++++++++++++++++++++
 8 files changed, 341 insertions(+), 2 deletions(-)

and after the macros it was

 arch/s390/include/asm/checkpoint_hdr.h |   84 +++++++++++++++++++++++++
 arch/s390/mm/checkpoint.c              |  104 ++++++++++++++++++++++++++++++++
 arch/s390/mm/restart.c                 |   71 ++++++++++++++++++++++
 8 files changed, 278 insertions(+), 2 deletions(-)

That's a savings of ~60 lines in the affected bits.  Personally, I
think that it makes the save/load bits significantly easier to read as
well.

-- 
Dan Smith
IBM Linux Technology Center
email: danms-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org