Quoting Oren Laadan (orenl@xxxxxxxxxxx):
>
>
> Serge E. Hallyn wrote:
> > Quoting Oren Laadan (orenl@xxxxxxxxxxx):
> >>
> >> Serge E. Hallyn wrote:
> >>> Quoting Matt Helsley (matthltc@xxxxxxxxxx):
> >>>>> @@ -401,6 +409,9 @@ char *ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
> >>>>> case 'E':
> >>>>> len += sprintf(format+len, "[%s]", "err %d");
> >>>>> break;
> >>>>> + case 'C': /* count of bytes read/written to checkpoint image */
> >>>>> + len += sprintf(format+len, "[%s]", "pos %d");
> >>>>> + break;
> >>>> Instead we could always output ckpt->total and then we wouldn't need %(C). I
> >>>> suspect it's such a useful piece of information that it'll be repeated
> >>>> in many/all format strings eventually.
> >>> Yes, likewise %(T). If that's what we want to do.
> >> I agree. For the cases when there is not task, can put "none"
> >>
> >>> Should we discuss here what we want an entry to look like? For both
> >>> ckpt_write_err (to the checkpoint image) and ckpt_error()?
> >>>
> >> Yes please !
> >
> > Actually %T isn't the current task, right, so it shouldn't always be prepended?
> > It actually is only meaningful during checkpoint_task(), collect_objs(), and
> > __tree_count_tasks?
> >
> > Ok, so how about:
> >
> > 1. ckpt_write_err() always also calls ckpt_error() (which in turn calls
> > ckpt_debug). Avoid duplication which exists in several places
> > right now.
> > 2. We always prepend:
> >
> > [current->pid]:[ctx->root_pid]:[ctx->active_pid]:[ctx->errno][ctx->total]
> >
> > The %(X) expansions if specified come whereever they are in the fmt
> > string (which is what's happening now with my patchset).
>
> So somewhere should set ctx->errno during a checkpoint.
>
> I suppose active_pid is for restart, but it's redundant isn't it ?
> (it's always active_pid) - is it the different between top-level pid-ns
> and "current" pid-ns ?
No, I figured it would be meaningful for instance in places like
wait_task_active().
> Instead of writing root_pid repeatedly, why not write sometime at the
> beginning some "global" info about the checkpoint/restart ? (e.g.
> the root_pid ...)
Well it is written out (for restart) at the end, so I suppose I should
switch restore_debug_free() to using ckpt_error() instead of ckpt_debug().
> > Kind of long, but again this is for ckpt_error and ckpt_write_err, not for all
> > ckpt_debugs().
> >
> >>>>> case 'O':
> >>>>> len += sprintf(format+len, "[%s]", "obj %d");
> >>>>> break;
> >>>>> @@ -435,6 +446,51 @@ char *ckpt_generate_fmt(struct ckpt_ctx *ctx, char *fmt)
> >>>>> return format;
> >>>>> }
> >>>>>
> >>>>> +void ckpt_log_error(struct ckpt_ctx *ctx, char *fmt, ...)
> >>>>> +{
> >>>>> + mm_segment_t fs;
> >>>>> + struct file *file;
> >>>>> + int count;
> >>>>> + va_list ap, aq, az;
> >>>>> + char *format;
> >>>>> + char buf[200], *bufp = buf;
> >>>> I believe this buffer is too big for a kernel stack -- especially
> >>>> for ckpt_log_error() which might be invoked "deep" in
> >>>> the kernel stack.
> >>> 200 bytes? Well, I guess I can try with 50 which still may often be
> >>> enough.
> >> How about using a dedicated buffer on @ctx for that ?
> >
> > I was going to do that originally, but then thought back to your
> > comments about parallel checkpoint, and didn't feel like also adding
> > a spinlock.
>
> We _will_ have some sort of locking when doing a parallel checkpoint.
Ok, so I'll set aside a big buffer and I'll just do a spinlock for now.
> So when we get there either use that lock, or (what I believe is more
> likely) create a per-checkpointer sub-data structure (a-la per-cpu).
>
> Oren.
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
