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

["Serge E. Hallyn" <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>]

Quoting Dave Hansen (dave-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org):
> > +	/* s390_fp_regs_t */
> > +	__u32 fpc;
> > +	struct {
> > +		float f;
> > +		double d;
> > +		__u64 ui;
> > +		__u32 fp_hi;
> > +		__u32 fp_lo;
> > +	} fprs[NUM_FPRS];
> 
> I don't see a lot of floating point code go by, so I'm a bit stupid on
> this.  But, this doesn't make a lot of sense to me.  Are there really
> parallel sets of float and double registers?  Or, do we want some kind
> of union here?

Heh, yeah, freg_t in arch/s390/include/asm/ptrace.h is a union.  Which
as you note ought to have been obvious based on the union members.  My
bad.

> I'm struggling to figure out how we're going to keep up with keeping
> checkpoint and restart synchronized.  This is all pretty mindless
> copying in both directions.  Is it possible and worthwhile for us to
> just define it once, but use it for both checkpoint and restart with
> some macro trickery?
> 
> #define CKPT 1
> #define RST  2
> 
> #define CR_COPY(op, a, b)
> 	do {
> 		WARN_ON(sizeof(a) != sizeof(b));
> 		if (op == CKPT)
> 			memcpy(&a, &b, sizeof(b));
> 		else
> 			memcpy(&b, &a, sizeof(b));
> 	} while (0);
> 
> void s390_cr_regs(int op, struct task_struct *thread, *hh)
> {
> 	CR_COPY(op, thread->per_info.lowcore.words.perc_atmid, hh->perc_atmid);
> 	CR_COPY(op, thread->per_info.lowcore.words.address, hh->address);
> 	CR_COPY(op, thread->per_info.lowcore.words.access_id, hh->access_id);
> 	...
> }
> 
> int cr_read_cpu(struct cr_ctx *ctx)
> {
> 	s390_cr_regs(RST, thread, hh);
> }
> 
> int cr_save_cpu(struct cr_ctx *ctx)
> {
> 	s390_cr_regs(CKPT, thread, hh);
> }
> 
> That works for both regular variables and for arrays.  Is the decreased
> line count worth the weirdo non-standard abstraction?

Well it's not necessarily nicer to read, but you're right it will
almost doubtless prevent at least one bug resulting from update
snafu between checkpoint and restart code.

Great idea!  Should we try and do that as much as possible for
the x86 code too?

-serge