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

[Serge E. Hallyn]

Quoting Nathan Lynch (ntl at pobox.com):
> On Wed, 25 Feb 2009 13:12:08 -0500
> Dan Smith <danms at us.ibm.com> wrote:
> 
> > 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.
> > 
> > Changelog:
> >     Feb 25:
> >             . Changed WARN_ON() to BUILD_BUG_ON()
> > 
> > Signed-off-by: Dan Smith <danms at us.ibm.com>
> > ---
> >  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..3add90e 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 {						\
> > +		BUILD_BUG_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_ */
> 
> No typechecking.  Multiple expansion of arguments (probably harmless
> for the current use case, but still).  Generates a memcpy where,
> depending on the arguments, simple assignment would be sufficient and

well is there any case where we must do a memcpy?  Or, if the headers
in the checkpoint image also give a bounded array size, can we just
copy that array using 'a = b'?

In any case we guarantee the sizes are correct, which may be all that
we want anyway.

> preferred.  Not useful for targets where fields in the checkpoint image
> are larger than the register state they reflect (32-bit variants of x86
> and powerpc).
> 
> Anyway, checkpoint and restart should not be "symmetric" -- the restart
> code has to validate certain values, such as privileged registers, in
> the image before committing them.

The argument in favor is that using the CR_COPY() macros in a single fn
for the common code allows us to

	1. make sure that none of the common code is accidentally
		overlooked (which does in fact make it more
		maintainable)
	2. lets any code which must be specially done at checkpoint
		or restart stand out.  Like resetting access registers
		in Dan's version.

> And while these macros may reduce initial development effort, the
> reading effort incurred is non-trivial.  It doesn't seem worth it.

-serge
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers