Message-Id: <20150512163602.ba6c83e3056702cbd840edef@linux-foundation.org>
Date:	Tue, 12 May 2015 16:36:02 -0700
From:	Andrew Morton <akpm@...ux-foundation.org>
To:	Tejun Heo <tj@...nel.org>
Cc:	davem@...emloft.net, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org, penguin-kernel@...ove.SAKURA.ne.jp,
	sd@...asysnail.net
Subject: Re: [PATCH 4/4] netconsole: implement extended console support

On Mon, 11 May 2015 12:41:34 -0400 Tejun Heo <tj@...nel.org> wrote:

> printk logbuf keeps various metadata and optional key=value dictionary
> for structured messages, both of which are stripped when messages are
> handed to regular console drivers.
> 
> It can be useful to have this metadata and dictionary available to
> netconsole consumers.  This obviously makes logging via netconsole
> more complete and the sequence number in particular is useful in
> environments where messages may be lost or reordered in transit -
> e.g. when netconsole is used to collect messages in a large cluster
> where packets may have to travel congested hops to reach the
> aggregator.  The lost and reordered messages can easily be identified
> and handled accordingly using the sequence numbers.
> 
> printk recently added extended console support which can be selected
> by setting CON_EXTENDED flag.

There's no such thing as CON_EXTENDED.  Not sure what this is trying to
say.

>  From console driver side, not much
> changes.  The only difference is that the text passed to the write
> callback is formatted the same way as /dev/kmsg.
> 
> This patch implements extended console support for netconsole which
> can be enabled by either prepending "+" to a netconsole boot param
> entry or echoing 1 to "extended" file in configfs.  When enabled,
> netconsole transmits extended log messages with headers identical to
> /dev/kmsg output.
> 
> There's one complication due to message fragments.  netconsole limits
> the maximum message size to 1k and messages longer than that are split
> into multiple fragments.  As all extended console messages should
> carry matching headers and be uniquely identifiable, each extended
> message fragment carries full copy of the metadata and an extra header
> field to identify the specific fragment.  The optional header is of
> the form "ncfrag=OFF/LEN" where OFF is the byte offset into the
> message body and LEN is the total length.
> 
> To avoid unnecessarily making printk format extended messages,
> Extended netconsole is registered with printk when the first extended
> netconsole is configured.
>
> ...
>
> +static ssize_t store_extended(struct netconsole_target *nt,
> +			      const char *buf,
> +			      size_t count)
> +{
> +	int extended;
> +	int err;
> +
> +	if (nt->enabled) {
> +		pr_err("target (%s) is enabled, disable to update parameters\n",
> +		       config_item_name(&nt->item));
> +		return -EINVAL;
> +	}

What's the reason for the above?

It's unclear (to me, at least ;)) what "disable" means?  Specifically
what steps must the operator take to successfully perform this
operation?  A sentence detailing those steps in netconsole.txt would be
nice.

> +	err = kstrtoint(buf, 10, &extended);
> +	if (err < 0)
> +		return err;
> +	if (extended < 0 || extended > 1)
> +		return -EINVAL;
> +
> +	nt->extended = extended;
> +
> +	return strnlen(buf, count);
> +}
> +
>
> ...
>
> +static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> +			     int msg_len)
> +{
> +	static char buf[MAX_PRINT_CHUNK];
> +	const char *header, *body;
> +	int offset = 0;
> +	int header_len, body_len;
> +
> +	if (msg_len <= MAX_PRINT_CHUNK) {
> +		netpoll_send_udp(&nt->np, msg, msg_len);
> +		return;
> +	}
> +
> +	/* need to insert extra header fields, detect header and body */
> +	header = msg;
> +	body = memchr(msg, ';', msg_len);
> +	if (WARN_ON_ONCE(!body))
> +		return;
> +
> +	header_len = body - header;
> +	body_len = msg_len - header_len - 1;
> +	body++;
> +
> +	/*
> +	 * Transfer multiple chunks with the following extra header.
> +	 * "ncfrag=<byte-offset>/<total-bytes>"
> +	 */
> +	memcpy(buf, header, header_len);
> +
> +	while (offset < body_len) {
> +		int this_header = header_len;
> +		int this_chunk;
> +
> +		this_header += scnprintf(buf + this_header,
> +					 sizeof(buf) - this_header,
> +					 ",ncfrag=%d/%d;", offset, body_len);
> +
> +		this_chunk = min(body_len - offset,
> +				 MAX_PRINT_CHUNK - this_header);
> +		if (WARN_ON_ONCE(this_chunk <= 0))
> +			return;
> +
> +		memcpy(buf + this_header, body + offset, this_chunk);
> +
> +		netpoll_send_udp(&nt->np, buf, this_header + this_chunk);
> +
> +		offset += this_chunk;
> +	}

What protects `buf'?  console_sem, I assume?

-	static char buf[MAX_PRINT_CHUNK];
+	static char buf[MAX_PRINT_CHUNK];	/* Protected by console_sem */

wouldn't hurt.

> +}
> +
> +static void write_ext_msg(struct console *con, const char *msg,


I've forgotten what's happening with this patchset.  There were a few
design-level issues raised against an earlier version.  What were those
and how have they been addressed?

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Powered by blists - more mailing lists