On Mon, 2009-02-23 at 15:04 -0800, Dan Smith wrote: > > +#define CR_390_PACK_TRACE_FLAGS(hdr, thread) \ > >> + do { \ > >> + hdr->em_instr = 0; \ > >> + if (thread->per_info.single_step) \ > >> + hdr->em_instr |= 1; \ > >> + if (thread->per_info.instruction_fetch) \ > >> + hdr->em_instr |= 2; \ > >> +} while (0) > >> + > >> +#define CR_390_UNPACK_TRACE_FLAGS(hdr, thread) \ > >> + do { \ > >> + if (hdr->em_instr & 1) \ > >> + thread->per_info.single_step = 1; \ > >> + if (hdr->em_instr & 2) \ > >> + thread->per_info.instruction_fetch = 1; \ > >> + } while (0) > > DH> Why are these macros? > > Well, almost the exact code above was spread between the checkpoint > and restart paths. I think that this makes it a little clearer what 1 > and 2 are since you can see them together. I'm just saying to use a 'static inline' instead. Can they be functions instead of macros. > DH> Why do we need to pack them, anyway? > > I assume Serge was trying not to waste a word per flag, and expecting > more flags to be needed to justify the savings. I actually think consistency with all the other fields and uniformity in looking at the code is probably worth 14 bits of waste. > >> + hh->svcnr = regs->svcnr; > >> + hh->ilc = regs->ilc; > >> + memcpy(hh->gprs, regs->gprs, NUM_GPRS*sizeof(unsigned long)); > >> + hh->orig_gpr2 = regs->orig_gpr2; > >> + > >> + hh->ieee_instruction_pointer = thread->ieee_instruction_pointer; > >> + > >> + /* per_info */ > >> + memcpy(&hh->per_control_regs, &thread->per_info.control_regs.words, > >> + 3 * sizeof(unsigned long)); > > DH> This '3' is a magic number and is used in a few places. Does it > DH> make sense to define it as a variable. > > You know, I said the same thing when I reviewed this set initially. > However the actual s390 code where its defined uses the magic 3, so if > we define a constant we could still be out of sync with the actual > definition. Then go fix the dang s390 code! It is already bad style, so let's not propagate it further. The fact that you (Serge) could do this patch without touching *any* current s390 code is really a sign that it is done wrong, not right. ;) > > DH> I'm struggling to figure out how we're going to keep up with > DH> keeping checkpoint and restart synchronized. This is all pretty > DH> mindless copying in both directions. Is it possible and > DH> worthwhile for us to just define it once, but use it for both > DH> checkpoint and restart with some macro trickery? > > DH> #define CKPT 1 > DH> #define RST 2 > > DH> #define CR_COPY(op, a, b) > DH> do { > DH> WARN_ON(sizeof(a) != sizeof(b)); > DH> if (op == CKPT) > DH> memcpy(&a, &b, sizeof(b)); > DH> else > DH> memcpy(&b, &a, sizeof(b)); > DH> } while (0); > > DH> void s390_cr_regs(int op, struct task_struct *thread, *hh) > DH> { > DH> CR_COPY(op, thread->per_info.lowcore.words.perc_atmid, hh->perc_atmid); > DH> CR_COPY(op, thread->per_info.lowcore.words.address, hh->address); > DH> CR_COPY(op, thread->per_info.lowcore.words.access_id, hh->access_id); > DH> ... > DH> } > > DH> int cr_read_cpu(struct cr_ctx *ctx) > DH> { > DH> s390_cr_regs(RST, thread, hh); > DH> } > > DH> int cr_save_cpu(struct cr_ctx *ctx) > DH> { > DH> s390_cr_regs(CKPT, thread, hh); > DH> } > > DH> That works for both regular variables and for arrays. Is the > DH> decreased line count worth the weirdo non-standard abstraction? > > It looks cleaner to me, so I'm happy to change it if people think that > would be better. Yeah, I'm just curious what everyone thinks as a whole. Keep it in the back of your mind. -- Dave _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers