Re: [RFC PATCH 11/17] define function to print error messages to user log

[Oren Laadan <orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org>]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl-RdfvBDnrOixBDgjK7y7TUQ@public.gmane.org):
>>
>> Serge E. Hallyn wrote:
>>> Quoting Matt Helsley (matthltc-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org):
>>>>> @@ -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 ?

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

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

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.