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

[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.

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