[RFC][PATCH -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[RFC][PATCH -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[0/9] netconsole: Multiple targets and dynamic reconfigurability

This patchset is a rework of the original idea and patches posted by
Keiichi Kii and Takayoshi Kochi at: http://lkml.org/lkml/2007/6/13/72

[ I had initially thought about only modifying Keiichi's patches to
incorporate the suggested stuff, sysfs+ioctl->configfs conversion, etc,
but the changes became too much and I ultimately ended up rewriting the
patchset. This also gave the flexibility to make other changes possible.
Keiichi, Takayoshi, kindly review this, and if OK, please give your
Signed-off-by:'s to the patches herein. ]

This patchset depends upon the earlier netpoll fixlet and the configfs
cleanup patches. Diffed against 2.6.22-rc6-mm1.

[1/9] netconsole: Cleanups, codingstyle, prettyfication
[2/9] netconsole: Code simplification
[3/9] netconsole: Introduce netconsole_target
[4/9] netconsole: Introduce netconsole_netdev_notifier
[5/9] netconsole: Introduce dev_status member
[6/9] netconsole: Update documentation for multiple target support
[7/9] netconsole: Support multiple logging targets
[8/9] netconsole: Update documentation for dynamic reconfigurability
[9/9] netconsole: Support dynamic reconfiguration using configfs

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>

About this patchset:
====================

What?

Support multiple remote logging targets in netconsole. Also, ability to
dynamically add or remove targets or modify parameters (IP, port, remote
MAC address) of targets at runtime, from userspace, using configfs.

Why?

>From Keiichi Kii's original post:

[...] current netconsole is not flexible. For example, if you want to change
ip address for logging agent, in the case of built-in netconsole, you can't
change config except for changing boot parameter and rebooting your system,
or in the case of module netconsole, you need to remove it and reload
with different parameters. [...] and we have been losing serial console
port with PCs and Servers.

(... and especially laptops, I would add.)

The patchset is heavily commented and verbosely changelogged, so hope you
have as much fun reading and reviewing it as I had writing it :-)

Kindly review and comment.

Satyam

[PATCH -mm 1/9] netconsole: Cleanups, codingstyle, prettyfication

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[1/9] netconsole: Cleanups, codingstyle, prettyfication

(1) Remove unwanted headers.
(2) Mark __init and __exit as appropriate.
(3) Various trivial codingstyle and prettification stuffs.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/netconsole.c |   55 +++++++++++++++++++++++------------------------
 1 file changed, 27 insertions(+), 28 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-04-26 08:38:32.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-03 20:27:17.000000000 +0530
@@ -35,35 +35,32 @@
  ****************************************************************/
 
 #include <linux/mm.h>
-#include <linux/tty.h>
 #include <linux/init.h>
 #include <linux/module.h>
 #include <linux/console.h>
-#include <linux/tty_driver.h>
 #include <linux/moduleparam.h>
 #include <linux/string.h>
-#include <linux/sysrq.h>
-#include <linux/smp.h>
 #include <linux/netpoll.h>
 
 MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
 MODULE_LICENSE("GPL");
 
-static char config[256];
-module_param_string(netconsole, config, 256, 0);
+#define MAX_PARAM_LENGTH	256
+#define MAX_PRINT_CHUNK		1000
+
+static char config[MAX_PARAM_LENGTH];
+module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
 MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]\n");
 
 static struct netpoll np = {
-	.name = "netconsole",
-	.dev_name = "eth0",
-	.local_port = 6665,
-	.remote_port = 6666,
-	.remote_mac = {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
+	.name		= "netconsole",
+	.dev_name	= "eth0",
+	.local_port	= 6665,
+	.remote_port	= 6666,
+	.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 };
-static int configured = 0;
-
-#define MAX_PRINT_CHUNK 1000
+static int configured;
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
@@ -75,7 +72,7 @@ static void write_msg(struct console *co
 
 	local_irq_save(flags);
 
-	for(left = len; left; ) {
+	for (left = len; left;) {
 		frag = min(left, MAX_PRINT_CHUNK);
 		netpoll_send_udp(&np, msg, frag);
 		msg += frag;
@@ -86,12 +83,12 @@ static void write_msg(struct console *co
 }
 
 static struct console netconsole = {
-	.name = "netcon",
-	.flags = CON_ENABLED | CON_PRINTBUFFER,
-	.write = write_msg
+	.name	= "netcon",
+	.flags	= CON_ENABLED | CON_PRINTBUFFER,
+	.write	= write_msg,
 };
 
-static int option_setup(char *opt)
+static int __init option_setup(char *opt)
 {
 	configured = !netpoll_parse_options(&np, opt);
 	return 1;
@@ -99,28 +96,30 @@ static int option_setup(char *opt)
 
 __setup("netconsole=", option_setup);
 
-static int init_netconsole(void)
+static int __init init_netconsole(void)
 {
-	int err;
+	int err = 0;
 
-	if(strlen(config))
+	if (strnlen(config, MAX_PARAM_LENGTH))
 		option_setup(config);
 
-	if(!configured) {
-		printk("netconsole: not configured, aborting\n");
-		return 0;
+	if (!configured) {
+		printk(KERN_INFO "netconsole: not configured, aborting\n");
+		goto out;
 	}
 
 	err = netpoll_setup(&np);
 	if (err)
-		return err;
+		goto out;
 
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
-	return 0;
+
+out:
+	return err;
 }
 
-static void cleanup_netconsole(void)
+static void __exit cleanup_netconsole(void)
 {
 	unregister_console(&netconsole);
 	netpoll_cleanup(&np);

[PATCH -mm 2/9] netconsole: Code simplification

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[2/9] netconsole: Code simplification

(1) Extract netpoll_parse_options() out of option_setup(), and into
init_netconsole() itself. So "configured" variable is redundant and
can be removed.

(2) With this change, option_setup() is not required for modular netconsole.

(3) The (!np.dev) check in write_msg() is bogus (always false), because:
np.dev is set by netpoll_setup(), which is called by the target init
code in init_netconsole() _before_ register_console() => write_msg() cannot
be triggered unless netpoll_setup() returns with success. And that will not
happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
under us while netconsole is loaded. This is because netpoll_setup() grabs
a reference for us on that dev. So let's remove the pointless check.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/netconsole.c |   31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-03 21:45:16.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-03 21:45:37.000000000 +0530
@@ -53,6 +53,16 @@ static char config[MAX_PARAM_LENGTH];
 module_param_string(netconsole, config, MAX_PARAM_LENGTH, 0);
 MODULE_PARM_DESC(netconsole, " netconsole=[src-port]@[src-ip]/[dev],[tgt-port]@<tgt-ip>/[tgt-macaddr]\n");
 
+#ifndef	MODULE
+static int __init option_setup(char *opt)
+{
+	strlcpy(config, opt, MAX_PARAM_LENGTH);
+	return 1;
+}
+
+__setup("netconsole=", option_setup);
+#endif
+
 static struct netpoll np = {
 	.name		= "netconsole",
 	.dev_name	= "eth0",
@@ -60,16 +70,12 @@ static struct netpoll np = {
 	.remote_port	= 6666,
 	.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
 };
-static int configured;
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
 	unsigned long flags;
 
-	if (!np.dev)
-		return;
-
 	local_irq_save(flags);
 
 	for (left = len; left;) {
@@ -88,26 +94,19 @@ static struct console netconsole = {
 	.write	= write_msg,
 };
 
-static int __init option_setup(char *opt)
-{
-	configured = !netpoll_parse_options(&np, opt);
-	return 1;
-}
-
-__setup("netconsole=", option_setup);
-
 static int __init init_netconsole(void)
 {
 	int err = 0;
 
-	if (strnlen(config, MAX_PARAM_LENGTH))
-		option_setup(config);
-
-	if (!configured) {
+	if (!strnlen(config, MAX_PARAM_LENGTH)) {
 		printk(KERN_INFO "netconsole: not configured, aborting\n");
 		goto out;
 	}
 
+	err = netpoll_parse_options(&np, config);
+	if (err)
+		goto out;
+
 	err = netpoll_setup(&np);
 	if (err)
 		goto out;

Re: [PATCH -mm 2/9] netconsole: Code simplification

[Matt Mackall]

On Wed, Jul 04, 2007 at 04:37:49PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [2/9] netconsole: Code simplification
> 
> (1) Extract netpoll_parse_options() out of option_setup(), and into
> init_netconsole() itself. So "configured" variable is redundant and
> can be removed.
> 
> (2) With this change, option_setup() is not required for modular netconsole.

How is this a simplification? You've taken code with no #ifdefs and
added one! Please have both modular and nonmodular continue to use the
same paths.

> 
> (3) The (!np.dev) check in write_msg() is bogus (always false), because:
> np.dev is set by netpoll_setup(), which is called by the target init
> code in init_netconsole() _before_ register_console() => write_msg() cannot
> be triggered unless netpoll_setup() returns with success. And that will not
> happen if netpoll_setup() failed to set np.dev. Also np.dev cannot go from
> under us while netconsole is loaded. This is because netpoll_setup() grabs
> a reference for us on that dev. So let's remove the pointless check.

This ought to be a separate patch.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH -mm 2/9] netconsole: Code simplification

[Satyam Sharma]

On Wed, 4 Jul 2007, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:37:49PM +0530, Satyam Sharma wrote:
> > [...]
> > (2) With this change, option_setup() is not required for modular netconsole.
> 
> How is this a simplification? You've taken code with no #ifdefs and
> added one! Please have both modular and nonmodular continue to use the
> same paths.

No, modular and non-modular do *not* take the same paths even in the
present code -- note that __setup() is a not even defined for modules.

If anything, the paths become somewhat "similar" *after* this patch.

Basically the present code is quite funny / strange, I'll explain why:

(1) For modular netconsole, the:

"module_param_string(netconsole, config, 256, 0);"

at the top of the file is what (effectively) does a:

strlcpy(config, "whatever", 256);

for us.

Note that __setup(option_setup) would be discarded at the preprocessor
stage itself for a modular build.

Then, in init_netconsole(), we call netpoll_parse_options() through
option_setup() ... the string (already copied into "config" due to the
module_param_string as I mentioned above) is then parsed and the
netpoll structure populated.

Then we proceed to do the netpoll_setup().

(2) For built-in netconsole:

__setup(option_setup) ensures a pointer to the option_setup() function
is put into the .init.text section starting at __setup__start ... then,
obsolete_checksetup() calls option_setup() with the appropriate string
from the kernel's command line [note that all this is happening even
before init_netconsole() gets called during the initcall process].

Anyway, obsolete_checksetup() calls option_setup() ->
netpoll_parse_options() that parses the passed string options that
matches after "netconsole=" [_not_ our "config" variable, which is a
redundant/unused variable in the built-in case] and populates the "np"
netpoll structure.

Then comes the kernel initcalls stage. init_netconsole() is duly called
when its time comes, and sees strlen(config) == 0 (note that config
is static and never populated in the built-in case that used the
"netconsole=" boot option). So, options_setup() is *not* called. But
that's no problem, "np" has already been populated after all.

And then we proceed to netpoll_setup() as usual.

Anyway, what I'm saying is that the _present_ code has radically different
codepaths, and the proposed patch _is_ a simplification because it makes
them somewhat more similar, as follows:

1. For modular netconsole, module_param_string effectively does a
strcpy(config, "whatever", 256); for us.

2. For built-in case, __setup effectively does _the same_ for us.

And for *both* the cases, init_netconsole() will call the
netpoll_parse_options() for us. Somewhat more similar, and simpler
(at least for me -- taste varies, of course).

BTW, you *don't* really want the exact same codepaths for both built-in
and modular netconsole anyway, believe me. If that's what you want, just
get rid of the __setup use in netconsole -- module_param_string works
equally good for modules when they are modular *or* when they are
built-in.

HOWEVER, this will introduce a regression: people will have to specify
the netconsole target specification as "netconsole.netconsole=..." on
the kernel command line, and not simply as "netconsole=" as they have
always been doing.

Perhaps I should've explained this a bit more verbosely in the changelog ...

Satyam

[PATCH -mm 3/9] netconsole: Introduce netconsole_target

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[3/9] netconsole: Introduce netconsole_target

Introduce a wrapper structure over netpoll to represent targets
configured in netconsole. This will get extended with other members in
further patches.

Ok, so the original patchset did this along with (and inside the #ifdef)
of CONFIG_NETCONSOLE_DYNAMIC itself. I decided otherwise, and was able
to drastically cut down on the #ifdef-complexity of final netconsole.c.
Also, struct netconsole_target would be required for multiple targets
support also, and not just dynamic reconfigurability. Previously these
two things were coupled, but I want to de-link that. (more on this later)

Note that this patch in itself looks quite redundant / stupid, but it is
purposefully made this way, so further patches are much more readable.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/netconsole.c |   34 ++++++++++++++++++++++++----------
 1 file changed, 24 insertions(+), 10 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-03 21:16:41.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-03 21:35:32.000000000 +0530
@@ -63,24 +63,35 @@ static int __init option_setup(char *opt
 __setup("netconsole=", option_setup);
 #endif
 
-static struct netpoll np = {
-	.name		= "netconsole",
-	.dev_name	= "eth0",
-	.local_port	= 6665,
-	.remote_port	= 6666,
-	.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
+/**
+ * struct netconsole_target - Represents a configured netconsole target.
+ * @np:		The netpoll structure for this target.
+ */
+struct netconsole_target {
+	struct netpoll		np;
+};
+
+static struct netconsole_target default_target = {
+	.np		= {
+		.name		= "netconsole",
+		.dev_name	= "eth0",
+		.local_port	= 6665,
+		.remote_port	= 6666,
+		.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
+	},
 };
 
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
 	unsigned long flags;
+	struct netconsole_target *nt = &default_target;
 
 	local_irq_save(flags);
 
 	for (left = len; left;) {
 		frag = min(left, MAX_PRINT_CHUNK);
-		netpoll_send_udp(&np, msg, frag);
+		netpoll_send_udp(&nt->np, msg, frag);
 		msg += frag;
 		left -= frag;
 	}
@@ -97,17 +108,18 @@ static struct console netconsole = {
 static int __init init_netconsole(void)
 {
 	int err = 0;
+	struct netconsole_target *nt = &default_target;
 
 	if (!strnlen(config, MAX_PARAM_LENGTH)) {
 		printk(KERN_INFO "netconsole: not configured, aborting\n");
 		goto out;
 	}
 
-	err = netpoll_parse_options(&np, config);
+	err = netpoll_parse_options(&nt->np, config);
 	if (err)
 		goto out;
 
-	err = netpoll_setup(&np);
+	err = netpoll_setup(&nt->np);
 	if (err)
 		goto out;
 
@@ -120,8 +132,10 @@ out:
 
 static void __exit cleanup_netconsole(void)
 {
+	struct netconsole_target *nt = &default_target;
+
 	unregister_console(&netconsole);
-	netpoll_cleanup(&np);
+	netpoll_cleanup(&nt->np);
 }
 
 module_init(init_netconsole);

[PATCH -mm 4/9] netconsole: Introduce netconsole_netdev_notifier

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[4/9] netconsole: Introduce netconsole_netdev_notifier

To update fields of underlying netpoll structure at runtime on
corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/netconsole.c |   36 ++++++++++++++++++++++++++++++++++++
 1 file changed, 36 insertions(+)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-03 21:51:40.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-03 22:02:11.000000000 +0530
@@ -81,6 +81,37 @@ static struct netconsole_target default_
 	},
 };
 
+/* Handle network interface device notifications */
+static int netconsole_netdev_event(struct notifier_block *this,
+				   unsigned long event,
+				   void *ptr)
+{
+	struct net_device *dev = ptr;
+	struct netconsole_target *nt = &default_target;
+
+	if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
+		goto done;
+
+	if (nt->np.dev == dev) {
+		switch (event) {
+		case NETDEV_CHANGEADDR:
+			memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
+			break;
+
+		case NETDEV_CHANGENAME:
+			strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
+			break;
+		}
+	}
+
+done:
+	return NOTIFY_DONE;
+}
+
+static struct notifier_block netconsole_netdev_notifier = {
+	.notifier_call  = netconsole_netdev_event,
+};
+
 static void write_msg(struct console *con, const char *msg, unsigned int len)
 {
 	int frag, left;
@@ -123,6 +154,10 @@ static int __init init_netconsole(void)
 	if (err)
 		goto out;
 
+	err = register_netdevice_notifier(&netconsole_netdev_notifier);
+	if (err)
+		return err;
+
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
@@ -135,6 +170,7 @@ static void __exit cleanup_netconsole(vo
 	struct netconsole_target *nt = &default_target;
 
 	unregister_console(&netconsole);
+	unregister_netdevice_notifier(&netconsole_netdev_notifier);
 	netpoll_cleanup(&nt->np);
 }
 

Re: [PATCH -mm 4/9] netconsole: Introduce netconsole_netdev_notifier

[Matt Mackall]

On Wed, Jul 04, 2007 at 04:37:59PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [4/9] netconsole: Introduce netconsole_netdev_notifier
> 
> To update fields of underlying netpoll structure at runtime on
> corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.

It's not obvious that we want to do that.

I can have a host with DHCP that uses netconsole to log to a private
network address. This breaks that.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH -mm 4/9] netconsole: Introduce netconsole_netdev_notifier

[Satyam Sharma]

On Wed, 4 Jul 2007, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:37:59PM +0530, Satyam Sharma wrote:
> > From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> > 
> > [4/9] netconsole: Introduce netconsole_netdev_notifier
> > 
> > To update fields of underlying netpoll structure at runtime on
> > corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
> [...]
> I can have a host with DHCP that uses netconsole to log to a private
> network address. This breaks that.

Hmm, could you explain that a bit more?

(and the breakage that could happen?)

(1) Please note that NETDEV_CHANGEADDR is a response to SIOCSIFHWADDR,
i.e. when the _hardware_ or MAC address of the particular _local_
(ethernet) network interface device that the netpoll is attached to
(i.e. np->dev) is changed -- say using:

# ip link set eth0 set address 01:23:45:67:89:ab

This has nothing to do with DHCP / IP addresses, AFAICT.

(2) NETDEV_CHANGENAME is a response to SIOCSIFNAME, i.e. when the name
of the particular (ethernet) network interface device is changed,
say something like:

# ip link set eth0 name ieee80230

(which would rename "eth0" to "ieee80230")

So I'm not following how DHCP could pose problems for us (or private
network addresses of remote logging agent), sorry ...


On Wed, 4 Jul 2007, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:37:59PM +0530, Satyam Sharma wrote:
> > [...]
> > To update fields of underlying netpoll structure at runtime on
> > corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
> 
> It's not obvious that we want to do that.

I'd want to retain this, if possible, please. Kindly note that configfs
makes all these attributes (that are members of struct netpoll) visible
from userspace -- so that user can both read and write to them. We
wouldn't want to display obsolete values for any of these parameters.
Say the interface was called "eth0" when the user / admin created the
target and "enabled" it. And say somewhere in between the interface
name got changed to "eth2". We wouldn't want the user to get obsolete/
incorrect information if / when he does a "cat dev_name" from
/config/netconsole/mytarget123/, for example.

Satyam

Re: [PATCH -mm 4/9] netconsole: Introduce netconsole_netdev_notifier

[KII Keiichi]

Hi Satyam,

> [4/9] netconsole: Introduce netconsole_netdev_notifier
> 
> To update fields of underlying netpoll structure at runtime on
> corresponding NETDEV_CHANGEADDR or NETDEV_CHANGENAME notifications.
> 
> Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
> Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

Should we avoid the automatic update of netconsole_target until we can notice all 
necessary members of netconsole_target updated automatically?

I had thought about automatic updating of the netconsole_target(i.e. local IP address 
and network device).
But I thought that the netconsole(netpoll) can only be related to a network device
because the user of netconsole is free to be able to set local IP address and 
MAC address of netconsole and it is difficult to monitor change of local IP address.
So, my patches only updates the network device automatically by using a symbolic link 
to the network device in sysfs when the event of NETDEV_CHANGEADDR occurs. 
The symbolic link to the network device in sysfs has the same meaning as following change
of MAC address and hardware.
(but I hadn't known the changing ability of MAC address using ip(8) command, 
thanks for your info)

Or I propose updating netconsole_taget informations when the network device up/down.
The MAC address and network interface name can be changed when 
the network device is disabled.
But, in the case, we must think of the case of changing IP address 
because we can change IP address without disabling the network device and 
can't hook the event of changing IP address.

How do you think?

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH -mm 4/9] netconsole: Introduce netconsole_netdev_notifier

[Satyam Sharma]

Hi,

On Sun, 8 Jul 2007, KII Keiichi wrote:
> [...]
> Should we avoid the automatic update of netconsole_target until we can notice all 
> necessary members of netconsole_target updated automatically?
> 
> I had thought about automatic updating of the netconsole_target(i.e. local IP address 
> and network device).
> But I thought that the netconsole(netpoll) can only be related to a network device
> because the user of netconsole is free to be able to set local IP address and 
> MAC address of netconsole and it is difficult to monitor change of local IP address.
[...]
> But, in the case, we must think of the case of changing IP address 
> because we can change IP address without disabling the network device and 
> can't hook the event of changing IP address.

Well, updating the local _IP_ address (for a particular interface) as
and when it changes would also be good ... it is another piece of info
exposed (read-write) to userspace through configfs, so makes sense to
keep it uptodate. But note that the two event notifications that we
care about currently are simply to keep up with changes in local _MAC_
address and interface name.

Regarding local_ip, the present behaviour is:

(1) If the user did not specify any local IP address (that is left that
field empty or zero), netpoll_setup() automatically fills it up with the
IP of the local network interface device that it is attached to: from
net_device->ip_ptr->ifa_list->ifa_local

(2) If user specified some local IP address, netpoll_setup() blindly
copies that into np->local_ip and that would later get used when
constructing the eth/ip/udp packet to be sent (containing the logging
data), as can be seen in netpoll_send_udp(). If the local IP changes
at some point after netpoll_setup(), nothing updates np->local_ip
accordingly.

So this patch just continues preserving the current behaviour, but yes,
it could make sense to trap protocol address changes and update the
local_ip field too when that happens (as long as it doesn't break
anything, as Matt warned earlier). I'm not aware of any existing
netdevice notifier that'll do this, so I'll look around (or trace
an ifconfig(8) / ip(8) to see what actually happens).

> Or I propose updating netconsole_taget informations when the network device up/down.
> The MAC address and network interface name can be changed when 
> the network device is disabled.

Actually, what happens is that if you: (1) disable / down the interface,
(2) change name / MAC address while it was down, and then (3) bring it
back up, then on coming back up, *both* the NETDEV_CHANGE{NAME,ADDR} and
the NETDEV_UP notifier chains are called out. If you just brought it
down and back up without setting any params, the other chains would not
be called out, but NETDEV_UP would. In this case, it's unnecessary to
update the params, because nothing got changed. And if something did
get changed, the corresponding chain would get called anyway, so it's
clearly best to update only the appropriate params, only when the
corresponding event notification comes along.

Satyam

Re: [PATCH -mm 4/9] netconsole: Introduce netconsole_netdev_notifier

[Satyam Sharma]

On Sun, 8 Jul 2007, Satyam Sharma wrote:
> On Sun, 8 Jul 2007, KII Keiichi wrote:
> [...]
> > But, in the case, we must think of the case of changing IP address 
> > because we can change IP address without disabling the network device and 
> > can't hook the event of changing IP address.
> 
> Well, updating the local _IP_ address (for a particular interface) as
> and when it changes would also be good ... it is another piece of info
> exposed (read-write) to userspace through configfs, so makes sense to
> keep it uptodate.
> [...] (as long as it doesn't break
> anything, as Matt warned earlier). I'm not aware of any existing
> netdevice notifier that'll do this, so I'll look around (or trace
> an ifconfig(8) / ip(8) to see what actually happens).

I think inetaddr_chain / register_inetaddr_notifier() are what I'm
looking for. I'll see if this does the job for us without breaking
anything else.

Satyam

[PATCH -mm 5/9] netconsole: Introduce dev_status member

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[5/9] netconsole: Introduce dev_status member

Introduce a new member in netconsole_target that tracks the status (up or
down) of the underlying interface network device that the specific logging
target netpoll is attached to.

We then join this up with the just-introduced net_device notifier, and
introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
when the corresponding local interface is down, we save the overhead of
unnecessarily disabling interrupts and calling into the netpoll stack in
console->write().

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/netconsole.c |   42 +++++++++++++++++++++++++++++++-----------
 1 file changed, 31 insertions(+), 11 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-03 22:05:51.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-04 03:04:45.000000000 +0530
@@ -63,11 +63,23 @@ static int __init option_setup(char *opt
 __setup("netconsole=", option_setup);
 #endif
 
+/*
+ * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
+ * weight if only netdevice.h had the good sense to export such a function.
+ * Oh well ...
+ */
+static inline int net_dev_is_up(struct net_device *net_dev)
+{
+	return ((net_dev->flags & IFF_UP) == IFF_UP);
+}
+
 /**
  * struct netconsole_target - Represents a configured netconsole target.
+ * @dev_status:	Tracks whether the underlying interface is up or down.
  * @np:		The netpoll structure for this target.
  */
 struct netconsole_target {
+	int			dev_status;
 	struct netpoll		np;
 };
 
@@ -89,11 +101,17 @@ static int netconsole_netdev_event(struc
 	struct net_device *dev = ptr;
 	struct netconsole_target *nt = &default_target;
 
-	if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
-		goto done;
+	if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
+	      event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
+	      	goto done;
 
 	if (nt->np.dev == dev) {
 		switch (event) {
+		case NETDEV_UP:
+		case NETDEV_DOWN:
+			nt->dev_status = net_dev_is_up(nt->np.dev);
+			break;
+
 		case NETDEV_CHANGEADDR:
 			memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
 			break;
@@ -118,16 +136,16 @@ static void write_msg(struct console *co
 	unsigned long flags;
 	struct netconsole_target *nt = &default_target;
 
-	local_irq_save(flags);
-
-	for (left = len; left;) {
-		frag = min(left, MAX_PRINT_CHUNK);
-		netpoll_send_udp(&nt->np, msg, frag);
-		msg += frag;
-		left -= frag;
+	if (nt->dev_status) {
+		local_irq_save(flags);
+		for (left = len; left;) {
+			frag = min(left, MAX_PRINT_CHUNK);
+			netpoll_send_udp(&nt->np, msg, frag);
+			msg += frag;
+			left -= frag;
+		}
+		local_irq_restore(flags);
 	}
-
-	local_irq_restore(flags);
 }
 
 static struct console netconsole = {
@@ -154,6 +172,8 @@ static int __init init_netconsole(void)
 	if (err)
 		goto out;
 
+	nt->dev_status = net_dev_is_up(nt->np.dev);
+
 	err = register_netdevice_notifier(&netconsole_netdev_notifier);
 	if (err)
 		return err;

Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member

[Matt Mackall]

On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [5/9] netconsole: Introduce dev_status member
> 
> Introduce a new member in netconsole_target that tracks the status (up or
> down) of the underlying interface network device that the specific logging
> target netpoll is attached to.
> 
> We then join this up with the just-introduced net_device notifier, and
> introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
> when the corresponding local interface is down, we save the overhead of
> unnecessarily disabling interrupts and calling into the netpoll stack in
> console->write().

Yuck. 

> +/*
> + * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
> + * weight if only netdevice.h had the good sense to export such a function.
> + * Oh well ...
> + */
> +static inline int net_dev_is_up(struct net_device *net_dev)
> +{
> +	return ((net_dev->flags & IFF_UP) == IFF_UP);
> +}

Why editorialize? Why not just add this to netdevice.h?

> +	if (nt->dev_status) {

Why not simply call net_dev_is_up?

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member

[Satyam Sharma]

On Wed, 4 Jul 2007, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> > [...]
> > +/*
> > + * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
> > + * weight if only netdevice.h had the good sense to export such a function.
> > + * Oh well ...
> > + */
> > +static inline int net_dev_is_up(struct net_device *net_dev)
> > +{
> > +	return ((net_dev->flags & IFF_UP) == IFF_UP);
> > +}
> 
> Why editorialize? Why not just add this to netdevice.h?

Hmm, I wanted to keep all changes local to netconsole, but ...
ok, will do that.

> > +	if (nt->dev_status) {
> 
> Why not simply call net_dev_is_up?

Yes, I did consider just calling the function here instead of
introducing another member ... again, ok will do that ...

> > We then join this up with the just-introduced net_device notifier, and
> > introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
> > when the corresponding local interface is down, we save the overhead of
> > unnecessarily disabling interrupts and calling into the netpoll stack in
> > console->write().
> 
> Yuck. 

... so all this will go too.

Satyam

Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member

[Stephen Hemminger]

On Wed, 4 Jul 2007 08:56:42 -0500
Matt Mackall <mpm@selenic.com> wrote:

> On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> > From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> > 
> > [5/9] netconsole: Introduce dev_status member
> > 
> > Introduce a new member in netconsole_target that tracks the status (up or
> > down) of the underlying interface network device that the specific logging
> > target netpoll is attached to.
> > 
> > We then join this up with the just-introduced net_device notifier, and
> > introduce NETDEV_UP and NETDEV_DOWN notifications. By disabling the target
> > when the corresponding local interface is down, we save the overhead of
> > unnecessarily disabling interrupts and calling into the netpoll stack in
> > console->write().
> 
> Yuck. 
> 
> > +/*
> > + * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
> > + * weight if only netdevice.h had the good sense to export such a function.
> > + * Oh well ...
> > + */
> > +static inline int net_dev_is_up(struct net_device *net_dev)
> > +{
> > +	return ((net_dev->flags & IFF_UP) == IFF_UP);
> > +}
> 
> Why editorialize? Why not just add this to netdevice.h?
> 
> > +	if (nt->dev_status) {
> 
> Why not simply call net_dev_is_up?

The flags field values are really BSD legacy-ish stuff and should not be
used internally. IFF_UP etc, do not have the necessary atomic properties.

Use netif_running() instead.

Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member

[Satyam Sharma]

Hi Stephen,

On Thu, 5 Jul 2007, Stephen Hemminger wrote:
> On Wed, 4 Jul 2007 08:56:42 -0500
> Matt Mackall <mpm@selenic.com> wrote:
> [...]
> > > +	if (nt->dev_status) {
> > 
> > Why not simply call net_dev_is_up?
> 
> The flags field values are really BSD legacy-ish stuff and should not be
> used internally. IFF_UP etc, do not have the necessary atomic properties.
> 
> Use netif_running() instead.

Ok, thanks for letting me know about that. I'll make the change.

Satyam

Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member

[Joel Becker]

On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> -	if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> -		goto done;
> +	if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
> +	      event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> +	      	goto done;
>
>  	if (nt->np.dev == dev) {
>  		switch (event) {

	It's a small nit, but isn't the large if() just the degenerate
default case of the switch()?  Why have it at all?  

Joel

-- 

Life's Little Instruction Book #396

	"Never give anyone a fruitcake."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH -mm 5/9] netconsole: Introduce dev_status member

[Satyam Sharma]

On Wed, 4 Jul 2007, Joel Becker wrote:
> On Wed, Jul 04, 2007 at 04:38:04PM +0530, Satyam Sharma wrote:
> > -	if (!(event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> > -		goto done;
> > +	if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
> > +	      event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> > +	      	goto done;
> >
> >  	if (nt->np.dev == dev) {
> >  		switch (event) {
> 
> 	It's a small nit, but isn't the large if() just the degenerate
> default case of the switch()?  Why have it at all?  

Yes, the large if() is redundant in this particular patch.

But in further patches, the switch() is enclosed inside a 
spin_lock_irqsave() and list_for_each_entry(target_list) and so the
large if() upfront saves us from unnecessarily disabling interrupts
and iterating through the entire target_list in the case that it is
a notification for an event that we don't care about.

So I'll remove this if() from this patch and introduce it in the
later one itself, which is where it starts making some sense, anyway.

Satyam

[PATCH -mm 6/9] netconsole: Update documentation for multiple target support

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[6/9] netconsole: Update documentation for multiple target support

... and add a few useful general purpose tips as well while we're at it.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 Documentation/networking/netconsole.txt |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

---

diff -ruNp a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
--- a/Documentation/networking/netconsole.txt	2007-04-26 08:38:32.000000000 +0530
+++ b/Documentation/networking/netconsole.txt	2007-07-02 11:44:53.000000000 +0530
@@ -34,6 +34,12 @@ Examples:
 
  insmod netconsole netconsole=@/,@10.0.0.2/
 
+It also supports logging to multiple remote agents by specifying
+parameters for the multiple agents separated by semicolons and the
+complete string enclosed in "quotes", thusly:
+
+modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/"
+
 Built-in netconsole starts immediately after the TCP stack is
 initialized and attempts to bring up the supplied dev at the supplied
 address.
@@ -44,6 +50,18 @@ WARNING: the default target ethernet set
 ethernet address to send packets, which can cause increased load on
 other systems on the same ethernet segment.
 
+TIP: some LAN switches may be configured to suppress ethernet broadcasts
+so it is advised to explicitly specify the remote agents' MAC addresses
+from the config parameters passed to netconsole.
+
+TIP: to find out the MAC address of 10.0.0.2, you may try using:
+ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2
+
+TIP: in case the remote logging agent is on a separate LAN subnet than
+the sender, it is suggested to try specifying the default gateway's
+(you may use /sbin/route -n to find out) MAC address as the remote MAC
+address instead.
+
 NOTE: the network device (eth1 in the above case) can run any kind
 of other network traffic, netconsole is not intrusive. Netconsole
 might cause slight delays in other traffic if the volume of kernel

Re: [PATCH -mm 6/9] netconsole: Update documentation for multiple target support

[Matt Mackall]

On Wed, Jul 04, 2007 at 04:38:09PM +0530, Satyam Sharma wrote:
> From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> 
> [6/9] netconsole: Update documentation for multiple target support
> 
> ... and add a few useful general purpose tips as well while we're at it.

The tips are fine and should go in their own patch at the beginning of
the series. The docs for multiple interfaces should go either in the
patch that adds that (ideally) or after.

-- 
Mathematics is the supreme nostalgia of our time.

Re: [PATCH -mm 6/9] netconsole: Update documentation for multiple target support

[Satyam Sharma]

Hi Matt,

On Wed, 4 Jul 2007, Matt Mackall wrote:
> On Wed, Jul 04, 2007 at 04:38:09PM +0530, Satyam Sharma wrote:
> > From: Satyam Sharma <ssatyam@cse.iitk.ac.in>
> > 
> > [6/9] netconsole: Update documentation for multiple target support
> > 
> > ... and add a few useful general purpose tips as well while we're at it.
> 
> The tips are fine and should go in their own patch at the beginning of
> the series. The docs for multiple interfaces should go either in the
> patch that adds that (ideally) or after.

Ok, will do that in the next iteration -- along with the change to get
rid of the redundant ->dev_status member, as you mentioned.

Thanks a lot for reviewing this, Matt.

Satyam

[PATCH -mm 7/9] netconsole: Support multiple logging targets

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[7/9] netconsole: Support multiple logging targets

This patch introduces support for multiple targets:

Let's keep this out of CONFIG_NETCONSOLE_DYNAMIC as well -- this is useful
even in the default case and (including the infrastructure introduced in
previous patches) doesn't really add too many bytes to module text. All the
complexity (and size) comes with the dynamic reconfigurability / userspace
interface patch, and so it's plausible users may want to keep this enabled
but that disabled (say to avoid a dep on CONFIG_CONFIGFS_FS).

Brief overview:

We maintain a target_list (and corresponding lock). Get rid of the static
"default_target" and introduce allocation and release functions for our
netconsole_target objects (but keeping sure to preserve previous behaviour
such as default values). During init_netconsole(), ";" is used as the
separator to identify multiple target specifications in the input boot/module
option string, and sent off to netpoll_parse_options(), and then finally
netpoll's are setup. During exit, the target_list is torn down and all items
released.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/netconsole.c |  193 +++++++++++++++++++++++++++++++++++------------
 1 file changed, 145 insertions(+), 48 deletions(-)

---

diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-04 03:05:32.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-04 12:03:01.000000000 +0530
@@ -63,6 +63,12 @@ static int __init option_setup(char *opt
 __setup("netconsole=", option_setup);
 #endif
 
+/* Linked list of all configured targets */
+static LIST_HEAD(target_list);
+
+/* This needs to be a spinlock because write_msg() cannot sleep */
+static DEFINE_SPINLOCK(target_list_lock);
+
 /*
  * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
  * weight if only netdevice.h had the good sense to export such a function.
@@ -75,52 +81,99 @@ static inline int net_dev_is_up(struct n
 
 /**
  * struct netconsole_target - Represents a configured netconsole target.
+ * @list:	Links this target into the target_list.
  * @dev_status:	Tracks whether the underlying interface is up or down.
  * @np:		The netpoll structure for this target.
  */
 struct netconsole_target {
+	struct list_head	list;
 	int			dev_status;
 	struct netpoll		np;
 };
 
-static struct netconsole_target default_target = {
-	.np		= {
-		.name		= "netconsole",
-		.dev_name	= "eth0",
-		.local_port	= 6665,
-		.remote_port	= 6666,
-		.remote_mac	= {0xff, 0xff, 0xff, 0xff, 0xff, 0xff},
-	},
-};
+/*
+ * Allocate new target and setup netpoll for it.
+ * Caller must then add this to target_list itself.
+ */
+static struct netconsole_target *alloc_target(char *target_config)
+{
+	int err = -ENOMEM;
+	struct netconsole_target *nt;
+
+	/* Allocate and initialize with defaults */
+	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
+	if (!nt) {
+		printk(KERN_ERR "netconsole: failed to allocate memory\n");
+		goto fail;
+	}
+
+	nt->np.name = "netconsole";
+	strlcpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+	nt->np.local_port = 6665;
+	nt->np.remote_port = 6666;
+	memset(nt->np.remote_mac, 0xff, ETH_ALEN);
+
+	/* Parse parameters and setup netpoll */
+	err = netpoll_parse_options(&nt->np, target_config);
+	if (err)
+		goto fail;
+
+	err = netpoll_setup(&nt->np);
+	if (err)
+		goto fail;
+
+	nt->dev_status = net_dev_is_up(nt->np.dev);
+
+	return nt;
+
+fail:
+	kfree(nt);
+	return ERR_PTR(err);
+}
+
+/*
+ * Cleanup netpoll for given target and free it.
+ * Caller must have removed this from target_list already.
+ */
+static void free_target(struct netconsole_target *nt)
+{
+	netpoll_cleanup(&nt->np);
+	kfree(nt);
+}
 
 /* Handle network interface device notifications */
 static int netconsole_netdev_event(struct notifier_block *this,
 				   unsigned long event,
 				   void *ptr)
 {
+	unsigned long flags;
+	struct netconsole_target *nt;
 	struct net_device *dev = ptr;
-	struct netconsole_target *nt = &default_target;
 
 	if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
 	      event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
-	      	goto done;
+		goto done;
 
-	if (nt->np.dev == dev) {
-		switch (event) {
-		case NETDEV_UP:
-		case NETDEV_DOWN:
-			nt->dev_status = net_dev_is_up(nt->np.dev);
-			break;
-
-		case NETDEV_CHANGEADDR:
-			memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
-			break;
-
-		case NETDEV_CHANGENAME:
-			strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
-			break;
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_for_each_entry(nt, &target_list, list) {
+		if (nt->np.dev == dev) {
+			switch (event) {
+			case NETDEV_UP:
+			case NETDEV_DOWN:
+				nt->dev_status = net_dev_is_up(nt->np.dev);
+				break;
+
+			case NETDEV_CHANGEADDR:
+				memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
+				break;
+
+			case NETDEV_CHANGENAME:
+				strlcpy(nt->np.dev_name, dev->name, IFNAMSIZ);
+				break;
+			}
 		}
 	}
+	spin_unlock_irqrestore(&target_list_lock, flags);
 
 done:
 	return NOTIFY_DONE;
@@ -134,18 +187,33 @@ static void write_msg(struct console *co
 {
 	int frag, left;
 	unsigned long flags;
-	struct netconsole_target *nt = &default_target;
+	struct netconsole_target *nt;
+	const char *tmp;
 
-	if (nt->dev_status) {
-		local_irq_save(flags);
-		for (left = len; left;) {
-			frag = min(left, MAX_PRINT_CHUNK);
-			netpoll_send_udp(&nt->np, msg, frag);
-			msg += frag;
-			left -= frag;
+	/* Avoid taking lock and disabling interrupts unnecessarily */
+	if (unlikely(list_empty(&target_list)))
+		return;
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_for_each_entry(nt, &target_list, list) {
+		if (nt->dev_status) {
+			/*
+			 * Note the double-loop nesting (for-in-buffer inside
+			 * for-all-targets) and use of temporary buf pointer.
+			 * This could be useful in getting as much logging out
+			 * to at least one target if we die inside here,
+			 * rather than unnecessarily keeping all in lock-step.
+			 */
+			tmp = msg;
+			for (left = len; left;) {
+				frag = min(left, MAX_PRINT_CHUNK);
+				netpoll_send_udp(&nt->np, tmp, frag);
+				tmp += frag;
+				left -= frag;
+			}
 		}
-		local_irq_restore(flags);
 	}
+	spin_unlock_irqrestore(&target_list_lock, flags);
 }
 
 static struct console netconsole = {
@@ -157,41 +225,70 @@ static struct console netconsole = {
 static int __init init_netconsole(void)
 {
 	int err = 0;
-	struct netconsole_target *nt = &default_target;
+	struct netconsole_target *nt, *tmp;
+	char *target_config;
+	char *input = config;
 
-	if (!strnlen(config, MAX_PARAM_LENGTH)) {
+	if (!strnlen(input, MAX_PARAM_LENGTH)) {
 		printk(KERN_INFO "netconsole: not configured, aborting\n");
 		goto out;
 	}
 
-	err = netpoll_parse_options(&nt->np, config);
-	if (err)
-		goto out;
-
-	err = netpoll_setup(&nt->np);
-	if (err)
-		goto out;
-
-	nt->dev_status = net_dev_is_up(nt->np.dev);
+	/*
+	 * Neither the netdev notifier, nor the console have been
+	 * registered so far. Nobody's racing us, so skip the lock.
+	 */
+	while ((target_config = strsep(&input, ";"))) {
+		nt = alloc_target(target_config);
+		if (IS_ERR(nt)) {
+			err = PTR_ERR(nt);
+			goto fail;
+		}
+		list_add(&nt->list, &target_list);
+	}
 
 	err = register_netdevice_notifier(&netconsole_netdev_notifier);
 	if (err)
-		return err;
+		goto fail;
 
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
 out:
 	return err;
+
+fail:
+	printk(KERN_ERR "netconsole: cleaning up\n");
+
+	/*
+	 * Remove all targets and destroy them.
+	 * The notifier is off, so nobody's racing us. So let's skip
+	 * the lock, netpoll_cleanup() wants to sleep anyway.
+	 */
+	list_for_each_entry_safe(nt, tmp, &target_list, list) {
+		list_del(&nt->list);
+		free_target(nt);
+	}
+
+	return err;
 }
 
 static void __exit cleanup_netconsole(void)
 {
-	struct netconsole_target *nt = &default_target;
+	struct netconsole_target *nt, *tmp;
 
 	unregister_console(&netconsole);
 	unregister_netdevice_notifier(&netconsole_netdev_notifier);
-	netpoll_cleanup(&nt->np);
+
+	/*
+	 * Remove all targets and destroy them.
+	 * Nobody's racing us, and netpoll_cleanup() wants to
+	 * sleep -- so skip the lock.
+	 */
+	list_for_each_entry_safe(nt, tmp, &target_list, list) {
+		list_del(&nt->list);
+		free_target(nt);
+	}
 }
 
 module_init(init_netconsole);

Re: [PATCH -mm 7/9] netconsole: Support multiple logging targets

[KII Keiichi]

Hi Satyam,

The following comments aren't essential.

>  	if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
>  	      event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> -	      	goto done;
> +		goto done;

The above diff lines are extra.

> +	spin_lock_irqsave(&target_list_lock, flags);
> +	list_for_each_entry(nt, &target_list, list) {
> +		if (nt->np.dev == dev) {
> +			switch (event) {
> +			case NETDEV_UP:
> +			case NETDEV_DOWN:
> +				nt->dev_status = net_dev_is_up(nt->np.dev);
> +				break;
> +
> +			case NETDEV_CHANGEADDR:
> +				memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);

The above line is over 80 characters.

Thanks,
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH -mm 7/9] netconsole: Support multiple logging targets

[Satyam Sharma]

Hi,

On Sun, 8 Jul 2007, KII Keiichi wrote:
> Hi Satyam,
> 
> The following comments aren't essential.
> 
> >  	if (!(event == NETDEV_UP || event == NETDEV_DOWN ||
> >  	      event == NETDEV_CHANGEADDR || event == NETDEV_CHANGENAME))
> > -	      	goto done;
> > +		goto done;
> 
> The above diff lines are extra.

Eek, looks like some whitespace correction leaked in here. I'll take care
not to produce the bad whitespace in the first place the first time this
line comes in.

> > +	spin_lock_irqsave(&target_list_lock, flags);
> > +	list_for_each_entry(nt, &target_list, list) {
> > +		if (nt->np.dev == dev) {
> > +			switch (event) {
> > +			case NETDEV_UP:
> > +			case NETDEV_DOWN:
> > +				nt->dev_status = net_dev_is_up(nt->np.dev);
> > +				break;
> > +
> > +			case NETDEV_CHANGEADDR:
> > +				memcpy(nt->np.local_mac, dev->dev_addr, ETH_ALEN);
> 
> The above line is over 80 characters.

Hmm, yes, but only by 2 columns. It's a simple and readable line
in its present form, so I'm not sure I'd want to be an extremist
and cut it into two ...

Satyam

[PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[8/9] netconsole: Update documentation for dynamic reconfigurability

Add myself to parties interested in receiving bug reports, and give lots
of examples. Also fix some whitespace inconsistencies I introduced earlier.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 Documentation/networking/netconsole.txt |   70 +++++++++++++++++++++++++++++++-
 1 file changed, 68 insertions(+), 2 deletions(-)

---

diff -ruNp a/Documentation/networking/netconsole.txt b/Documentation/networking/netconsole.txt
--- a/Documentation/networking/netconsole.txt	2007-07-04 03:12:25.000000000 +0530
+++ b/Documentation/networking/netconsole.txt	2007-07-04 12:24:44.000000000 +0530
@@ -3,6 +3,10 @@ started by Ingo Molnar <mingo@redhat.com
 2.6 port and netpoll api by Matt Mackall <mpm@selenic.com>, Sep 9 2003
 
 Please send bug reports to Matt Mackall <mpm@selenic.com>
+and Satyam Sharma <satyam.sharma@gmail.com>
+
+Introduction:
+=============
 
 This module logs kernel printk messages over UDP allowing debugging of
 problem where disk logging fails and serial consoles are impractical.
@@ -13,6 +17,9 @@ the specified interface as soon as possi
 capture of early kernel panics, it does capture most of the boot
 process.
 
+Sender and receiver configuration:
+==================================
+
 It takes a string configuration parameter "netconsole" in the
 following format:
 
@@ -38,7 +45,7 @@ It also supports logging to multiple rem
 parameters for the multiple agents separated by semicolons and the
 complete string enclosed in "quotes", thusly:
 
-modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/"
+ modprobe netconsole netconsole="@/,@10.0.0.2/;@/eth1,6892@10.0.0.3/"
 
 Built-in netconsole starts immediately after the TCP stack is
 initialized and attempts to bring up the supplied dev at the supplied
@@ -46,6 +53,64 @@ address.
 
 The remote host can run either 'netcat -u -l -p <port>' or syslogd.
 
+Dynamic reconfiguration:
+========================
+
+Dynamic reconfigurability is a useful addition to netconsole that enables
+remote logging targets to be dynamically added, removed, or have their
+parameters reconfigured at runtime from a configfs-based userspace interface.
+
+To include this feature, select CONFIG_NETCONSOLE_DYNAMIC when building the
+netconsole module (or kernel, if netconsole is built-in).
+
+Some examples follow (configfs is mounted at the /config mountpoint, say).
+
+To add a remote logging target (target names can be arbitrary):
+
+ cd /config/netconsole/
+ mkdir target1
+
+Note that newly created targets have default parameter values (as mentioned
+above) and are disabled by default -- they must first be enabled by writing
+"1" to the "enabled" attribute (usually after setting parameters accordingly)
+as described below.
+
+To remove a target:
+
+ rmdir /config/netconsole/othertarget/
+
+The interface exposes these parameters of a netconsole target to userspace:
+
+	id		Unique identifier for this target	(read-only)
+	enabled		Is this target currently enabled?	(read-write)
+	dev_name	Local network interface name		(read-only)
+	local_ip	Source IP address to use		(read-write)
+	remote_ip	Remote agent's IP address		(read-write)
+	local_port	Source UDP port to use			(read-write)
+	remote_port	Remote agent's UDP port			(read-write)
+	local_mac	Local interface's MAC address		(read-only)
+	remote_mac	Remote agent's MAC address		(read-write)
+
+The "enabled" attribute is also used to control whether the parameters of
+a target can be updated or not -- you can modify the parameters of only
+disabled targets (i.e. if "enabled" is 0).
+
+To update a target's parameters:
+
+ cat enabled				# check if enabled is 1
+ echo 0 > enabled			# disable the target (if required)
+ echo eth2 > dev_name			# set local interface
+ echo 10.0.0.4 > remote_ip		# update some parameter
+ echo cb:a9:87:65:43:21 > remote_mac	# update more parameters
+ echo 1 > enabled			# enable target again
+
+Note that you can even update the local interface dynamically. This is
+especially useful if you want to use interfaces that have newly come up
+and didn't even exist when netconsole was loaded / initialized.
+
+Miscellaneous notes:
+====================
+
 WARNING: the default target ethernet setting uses the broadcast
 ethernet address to send packets, which can cause increased load on
 other systems on the same ethernet segment.
@@ -55,7 +120,8 @@ so it is advised to explicitly specify t
 from the config parameters passed to netconsole.
 
 TIP: to find out the MAC address of 10.0.0.2, you may try using:
-ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2
+
+ ping -c 1 10.0.0.2 ; /sbin/arp -n | grep 10.0.0.2
 
 TIP: in case the remote logging agent is on a separate LAN subnet than
 the sender, it is suggested to try specifying the default gateway's

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Joel Becker]

On Wed, Jul 04, 2007 at 04:38:19PM +0530, Satyam Sharma wrote:
> +Some examples follow (configfs is mounted at the /config mountpoint, say).

	configfs is generally expected to be mounted at
/sys/kernel/config, and it even creates that mountpoint for itself.  The
documentation should probably be consistent with that.

Joel

-- 

"Also, all of life's big problems include the words 'indictment' or
	'inoperable.' Everything else is small stuff."
	- Alton Brown

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Satyam Sharma]

On Wed, 4 Jul 2007, Joel Becker wrote:
> On Wed, Jul 04, 2007 at 04:38:19PM +0530, Satyam Sharma wrote:
> > +Some examples follow (configfs is mounted at the /config mountpoint, say).
> 
> 	configfs is generally expected to be mounted at
> /sys/kernel/config, and it even creates that mountpoint for itself.  The
> documentation should probably be consistent with that.

Ok, will do that in the next iteration -- I'll wait for a couple more
days for comments from others, and then post the v2 patches sometime
during the weekend.

Satyam

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Keiichi KII]

Hi Satyam,

> + cat enabled				# check if enabled is 1
> + echo 0 > enabled			# disable the target (if required)
> + echo eth2 > dev_name			# set local interface

I think that the above line should change from "echo eth2 > dev_name" to 
"echo -n eth2 > dev_name" or the newline should be removed at store_dev_name().
The default behavior of echo(1) outputs the newline. 
So, if we write appropriate network device name to dev_name, 
the netpoll can't find net_device in netpoll_setup(). 

I don't have enough time now. So I will check your patches more specific 
at the weekend.
Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Satyam Sharma]

Hi Keiichi,

On Thu, 5 Jul 2007, Keiichi KII wrote:
> Hi Satyam,
> 
> > + cat enabled				# check if enabled is 1
> > + echo 0 > enabled			# disable the target (if required)
> > + echo eth2 > dev_name			# set local interface
> 
> I think that the above line should change from "echo eth2 > dev_name" to 
> "echo -n eth2 > dev_name" or the newline should be removed at store_dev_name().
> The default behavior of echo(1) outputs the newline. 
> So, if we write appropriate network device name to dev_name, 
> the netpoll can't find net_device in netpoll_setup(). 

Yes, you're right. I'll fix this in store_dev_name() itself, it isn't
right to limit users to only -n option. Thanks for pointing this out.

[ BTW I also noticed a typo in that file just above your extract that
mentions dev_name as a read-only attribute! That also must be fixed ... ]

> I don't have enough time now. So I will check your patches more specific 
> at the weekend.

Ok, thanks a lot. I'll wait for your comments / testing results over the
weekend, before sending out the next iteration.

BTW I did some testing myself, and have found another *embarrassing* bug:
if netconsole is loaded _without_ specifying any "netconsole=" parameter,
module is still kept loaded, but on unloading configfs_unregister_...()
obviously panics! :-) This should've been found by me earlier, just that
I never tested without specifying the parameter to modprobe :-(

Thanks,
Satyam

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Satyam Sharma]

On Thu, 5 Jul 2007, Satyam Sharma wrote:
> [...]
> BTW I did some testing myself, and have found another *embarrassing* bug:
> if netconsole is loaded _without_ specifying any "netconsole=" parameter,
> module is still kept loaded, but on unloading configfs_unregister_...()
> obviously panics! :-) This should've been found by me earlier, just that
> I never tested without specifying the parameter to modprobe :-(

Basically that check upfront in init_netconsole() checking if the boot/
module param was provided now becomes unnecessary -- with dynamic
reconfigurability, a user might plausibly want to load netconsole without
specifying any params, but add targets later ...

Satyam

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[KII Keiichi]

Hi Satyam,

> BTW I did some testing myself, and have found another *embarrassing* bug:
> if netconsole is loaded _without_ specifying any "netconsole=" parameter,
> module is still kept loaded, but on unloading configfs_unregister_...()
> obviously panics! :-) This should've been found by me earlier, just that
> I never tested without specifying the parameter to modprobe :-(

I tested your patches on the x86 architecture and report the results.
It is no problem except for the above problem.
I think that it is useful to use configfs(especially, mkdir and rmdir 
because of adding/removing netconsole_target).

I am going to test the your next iteration patches on x86 and IA64 
architecutures. 

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH -mm 8/9] netconsole: Update documentation for dynamic reconfigurability

[Satyam Sharma]

Hi Keiichi,

On Sun, 8 Jul 2007, KII Keiichi wrote:
> [...]
> I tested your patches on the x86 architecture and report the results.
> It is no problem except for the above problem.
> I think that it is useful to use configfs(especially, mkdir and rmdir because
> of adding/removing netconsole_target).
> 
> I am going to test the your next iteration patches on x86 and IA64
> architecutures. 

Thanks a lot for reviewing and testing this. I'll send out the next
iteration by tommorrow.

Thanks,
Satyam

[PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Satyam Sharma]

From: Satyam Sharma <ssatyam@cse.iitk.ac.in>

[9/9] netconsole: Support dynamic reconfiguration using configfs

This patch introduces support for dynamic reconfiguration (adding, removing
and/or modifying parameters of netconsole targets at runtime) using a
userspace interface exported via configfs.

Issues and brief design overview:

(1) Kernel-initiated creation / destruction of kernel objects is not
possible with configfs -- the lifetimes of the "config items" is managed
exclusively from userspace. But netconsole must support boot/module params
too, and these are parsed in kernel and hence netpolls must be setup from
the kernel. I initially thought of extending configfs with this functionality
(item creation/destroy by kernel subsystems) but Joel Becker pointed me to
the locking and refcounting complexities such a project would entail.

After more discussion, Joel suggested the idea to separately manage the
lifetimes of the two kinds of netconsole_target objects, those created while
parsing boot/module options and those created via configfs mkdir(2) from
userspace, in my driver itself. That adds a little bit of complexity and
redundancy (multiple item creation and destruction points) in this driver.
Note that the latter type (configfs-created netconsole_targets) can be
modified / updated / destroyed at runtime from userspace but the former
(param string created) cannot, as they are not exposed through our configfs
namespace. However, this is not really a problem and is similar to current
behaviour in any case.

(2) Enhancing the semantics of the "enabled" attribute. In the original
patchset submitted, "enabled" was just an on / off knob to switch logging
to particular targets on or off in the console->write() function itself.

But there is a problem. The original patchset had interface device name
as a read-only attribute and assumed ioctl()'s were to be used to create
new targets. This worked only because ioctl()'s deal with full structures
and we can fill out the structure members (including local interface) in
the special userspace program that implements that ioctl(). But configfs
does this with simple mkdir's that can be run off the shell -- and the
corresponding kernel object is created as result of the mkdir(2) call
chain in our driver subsystem. But at that point we don't really know
what interface device should the netpoll be attached.

So, let's solve it this way: newly-created netconsole_target's are not
automatically enabled, and their parameters are not parsed (and the
netpoll_setup() not called). Then, the user may set values to the
various attributes, ending with setting "1" to "enabled" itself. The
netpoll_setup() is then called on the set parameters in the context of
_this_ write(2) on the "enabled" attribute.

The upside of this solution is that existing netconsole_targets can be
reconfigured to be attached to newly-come-up interfaces, that might not
have even existed when netconsole was loaded, or even when the targets
were originally created. Also, we are able to completely get rid of the
custom ioctl()'s based solution.

(3) Ultra-paranoid configfs attribute show() and store() operations, with
sanity and input range checking, using only safe string primitives, and
compliant with the recommendations in Documentation/filesystems/sysfs.txt.

Signed-off-by: Satyam Sharma <ssatyam@cse.iitk.ac.in>
Cc: Keiichi Kii <k-keiichi@bx.jp.nec.com>
Cc: Takayoshi Kochi <t-kochi@bq.jp.nec.com>

---

 drivers/net/Kconfig      |   10
 drivers/net/netconsole.c |  601 +++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 596 insertions(+), 15 deletions(-)

---

diff -ruNp a/drivers/net/Kconfig b/drivers/net/Kconfig
--- a/drivers/net/Kconfig	2007-07-04 13:56:32.000000000 +0530
+++ b/drivers/net/Kconfig	2007-07-04 13:58:03.000000000 +0530
@@ -3030,6 +3030,16 @@ config NETCONSOLE
 	If you want to log kernel messages over the network, enable this.
 	See <file:Documentation/networking/netconsole.txt> for details.
 
+config NETCONSOLE_DYNAMIC
+	bool "Dynamic reconfiguration of logging targets (EXPERIMENTAL)"
+	depends on NETCONSOLE && SYSFS && EXPERIMENTAL
+	select CONFIGFS_FS
+	help
+	  This option enables the ability to dynamically reconfigure target
+	  parameters (interface, IP addresses, port numbers, MAC addresses)
+	  at runtime through a userspace interface exported using configfs.
+	  See <file:Documentation/networking/netconsole.txt> for details.
+
 config NETPOLL
 	def_bool NETCONSOLE
 
diff -ruNp a/drivers/net/netconsole.c b/drivers/net/netconsole.c
--- a/drivers/net/netconsole.c	2007-07-04 12:06:47.000000000 +0530
+++ b/drivers/net/netconsole.c	2007-07-04 13:53:14.000000000 +0530
@@ -41,6 +41,8 @@
 #include <linux/moduleparam.h>
 #include <linux/string.h>
 #include <linux/netpoll.h>
+#include <linux/inet.h>
+#include <linux/configfs.h>
 
 MODULE_AUTHOR("Maintainer: Matt Mackall <mpm@selenic.com>");
 MODULE_DESCRIPTION("Console driver for network interfaces");
@@ -69,6 +71,9 @@ static LIST_HEAD(target_list);
 /* This needs to be a spinlock because write_msg() cannot sleep */
 static DEFINE_SPINLOCK(target_list_lock);
 
+/* Target ids. Only for information, we don't care about wrap-wround */
+static atomic_t monotonic_count = ATOMIC_INIT(0);
+
 /*
  * Why no net_dev_is_up() in netdevice.h? The kernel could lose a lot of
  * weight if only netdevice.h had the good sense to export such a function.
@@ -82,25 +87,88 @@ static inline int net_dev_is_up(struct n
 /**
  * struct netconsole_target - Represents a configured netconsole target.
  * @list:	Links this target into the target_list.
+ * @item:	Links us into the configfs subsystem hierarchy.
+ * @id:		Unique identifier for this target.
+ *		Visible from userspace (read-only).
+ * @enabled:	On / off knob to enable / disable target.
+ *		Visible from userspace (read-write).
+ *		Note that we maintain a strict, reliable 1:1 correspondence
+ *		between this and whether netpoll is active / inactive.
+ *		Also, other parameters of a target may be modified at
+ *		runtime only when it is disabled (enabled = 0).
  * @dev_status:	Tracks whether the underlying interface is up or down.
+ *		Not visible from userspace.
  * @np:		The netpoll structure for this target.
+ *		Contains the other userspace visible parameters, namely:
+ *		dev_name	(read-write)
+ *		local_ip	(read-write)
+ *		remote_ip	(read-write)
+ *		local_port	(read-write)
+ *		remote_port	(read-write)
+ *		local_mac	(read-only)
+ *		remote_mac	(read-write)
  */
 struct netconsole_target {
 	struct list_head	list;
+	struct config_item	item;
+	int			id;
+	int			enabled;
 	int			dev_status;
 	struct netpoll		np;
 };
 
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+
+/*
+ * Targets that were created by parsing the boot/module option string
+ * do not exist in the configfs hierarchy and will never go away (and
+ * have zeroed-out config_item members). So make these a no-op for them.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+	static struct config_item empty_item;	/* Zeroed-out config_item */
+
+	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
+		config_item_get(&nt->item);
+}
+
+static void netconsole_target_put(struct netconsole_target *nt)
+{
+	static struct config_item empty_item;	/* Zeroed-out config_item */
+
+	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
+		config_item_put(&nt->item);
+}
+
+#else	/* !CONFIG_NETCONSOLE_DYNAMIC */
+
 /*
- * Allocate new target and setup netpoll for it.
+ * No danger of targets going away from under us when dynamic
+ * reconfigurability is off.
+ */
+static void netconsole_target_get(struct netconsole_target *nt)
+{
+}
+
+static void netconsole_target_put(struct netconsole_target *nt)
+{
+}
+
+#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
+
+/*
+ * Allocate new target (from boot/module param) and setup netpoll for it.
  * Caller must then add this to target_list itself.
  */
-static struct netconsole_target *alloc_target(char *target_config)
+static struct netconsole_target *alloc_param_target(char *target_config)
 {
 	int err = -ENOMEM;
 	struct netconsole_target *nt;
 
-	/* Allocate and initialize with defaults */
+	/*
+	 * Allocate and initialize with defaults.
+	 * Note that these targets get their config_item fields zeroed-out.
+	 */
 	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
 	if (!nt) {
 		printk(KERN_ERR "netconsole: failed to allocate memory\n");
@@ -122,7 +190,9 @@ static struct netconsole_target *alloc_t
 	if (err)
 		goto fail;
 
+	nt->id = atomic_inc_return(&monotonic_count);
 	nt->dev_status = net_dev_is_up(nt->np.dev);
+	nt->enabled = 1;
 
 	return nt;
 
@@ -132,15 +202,485 @@ fail:
 }
 
 /*
- * Cleanup netpoll for given target and free it.
+ * Cleanup netpoll for given target (from boot/module param) and free it.
  * Caller must have removed this from target_list already.
  */
-static void free_target(struct netconsole_target *nt)
+static void free_param_target(struct netconsole_target *nt)
 {
 	netpoll_cleanup(&nt->np);
 	kfree(nt);
 }
 
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+
+/*
+ * Our subsystem hierarchy is:
+ *
+ * /config/netconsole/
+ *		     |
+ *		     <target>/
+ *		     |	     id
+ *		     |	     enabled
+ *		     |	     dev_name
+ *		     |	     local_port
+ *		     |	     remote_port
+ *		     |	     local_ip
+ *		     |	     remote_ip
+ *		     |	     local_mac
+ *		     |	     remote_mac
+ *		     |
+ *		     <target>/...
+ */
+
+struct netconsole_target_attr {
+	struct configfs_attribute	attr;
+	ssize_t				(*show)(struct netconsole_target *nt,
+						char *buf);
+	ssize_t				(*store)(struct netconsole_target *nt,
+						 const char *buf,
+						 size_t count);
+};
+
+static struct netconsole_target *to_target(struct config_item *item)
+{
+	return item ?
+		container_of(item, struct netconsole_target, item) :
+		NULL;
+}
+
+/*
+ * Wrapper over simple_strtol (base 10) with sanity and range checking.
+ * We return (signed) long only because we may want to return errors.
+ * Thus, do not use this to convert numbers that are allowed to be negative.
+ */
+static long strtol10_check_range(const char *cp, long min, long max)
+{
+	long ret;
+	char *p = (char *) cp;
+
+	WARN_ON(min < 0);
+	WARN_ON(max < min);
+
+	ret = simple_strtol(p, &p, 10);
+
+	if (*p && (*p != '\n')) {
+		printk(KERN_ERR "netconsole: invalid input\n");
+		return -EINVAL;
+	}
+	if ((ret < min) || (ret > max)) {
+		printk(KERN_ERR "netconsole: input %ld must be between "
+				"%ld and %ld\n", ret, min, max);
+		return -EINVAL;
+	}
+
+	return ret;
+}
+
+/*
+ * Attribute operations for netconsole_target.
+ */
+
+static ssize_t show_id(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->id);
+}
+
+static ssize_t show_enabled(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->enabled);
+}
+
+static ssize_t show_dev_name(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%s\n", nt->np.dev_name);
+}
+
+static ssize_t show_local_ip(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+			HIPQUAD(nt->np.local_ip));
+}
+
+static ssize_t show_remote_ip(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d.%d.%d.%d\n",
+			HIPQUAD(nt->np.remote_ip));
+}
+
+static ssize_t show_local_port(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->np.local_port);
+}
+
+static ssize_t show_remote_port(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%d\n", nt->np.remote_port);
+}
+
+static ssize_t show_local_mac(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+			nt->np.local_mac[0], nt->np.local_mac[1],
+			nt->np.local_mac[2], nt->np.local_mac[3],
+			nt->np.local_mac[4], nt->np.local_mac[5]);
+}
+
+static ssize_t show_remote_mac(struct netconsole_target *nt, char *buf)
+{
+	return snprintf(buf, PAGE_SIZE, "%02x:%02x:%02x:%02x:%02x:%02x\n",
+			nt->np.remote_mac[0], nt->np.remote_mac[1],
+			nt->np.remote_mac[2], nt->np.remote_mac[3],
+			nt->np.remote_mac[4], nt->np.remote_mac[5]);
+}
+
+/*
+ * This one's special and important -- targets created through the
+ * configfs interface are not enabled / netpoll activated by default.
+ * The user is expected to set the desired parameters first (which
+ * would enable him to dynamically add new netpoll targets for new
+ * network interfaces as and when they come up).
+ */
+static ssize_t store_enabled(struct netconsole_target *nt,
+			     const char *buf,
+			     size_t count)
+{
+	int err;
+	long enabled;
+	struct netpoll *np = &nt->np;
+
+	enabled = strtol10_check_range(buf, 0, 1);
+	if (enabled < 0)
+		return enabled;
+
+	if (enabled) {	/* 1 */
+
+		/*
+		 * Skip netpoll_parse_options() -- all the attributes are
+		 * already configured in nt->np through configfs. But at
+		 * least let's print the useful stuff it used to output :-)
+		 */
+		printk(KERN_INFO "%s: local port %d\n",
+				 np->name, np->local_port);
+		printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
+				 np->name, HIPQUAD(np->local_ip));
+		printk(KERN_INFO "%s: interface %s\n",
+				 np->name, np->dev_name);
+		printk(KERN_INFO "%s: remote port %d\n",
+				 np->name, np->remote_port);
+		printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
+				 np->name, HIPQUAD(np->remote_ip));
+		printk(KERN_INFO "%s: remote ethernet address "
+				 "%02x:%02x:%02x:%02x:%02x:%02x\n",
+				 np->name,
+				 np->remote_mac[0], np->remote_mac[1],
+				 np->remote_mac[2], np->remote_mac[3],
+				 np->remote_mac[4], np->remote_mac[5]);
+
+		err = netpoll_setup(np);
+		if (err)
+			return err;
+
+		nt->dev_status = net_dev_is_up(np->dev);
+		printk(KERN_INFO "netconsole: network logging started\n");
+
+	} else {	/* 0 */
+		netpoll_cleanup(np);
+	}
+
+	nt->enabled = enabled;
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_dev_name(struct netconsole_target *nt,
+			      const char *buf,
+			      size_t count)
+{
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+				"disable to update parameters\n", nt->id);
+		return -EINVAL;
+	}
+
+	strlcpy(nt->np.dev_name, buf, IFNAMSIZ);
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_local_port(struct netconsole_target *nt,
+				const char *buf,
+				size_t count)
+{
+	long local_port;
+#define __U16_MAX	((__u16) ~0U)
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+				"disable to update parameters\n", nt->id);
+		return -EINVAL;
+	}
+
+	local_port = strtol10_check_range(buf, 0, __U16_MAX);
+	if (local_port < 0)
+		return local_port;
+
+	nt->np.local_port = local_port;
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_remote_port(struct netconsole_target *nt,
+				 const char *buf,
+				 size_t count)
+{
+	long remote_port;
+#define __U16_MAX	((__u16) ~0U)
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+				"disable to update parameters\n", nt->id);
+		return -EINVAL;
+	}
+
+	remote_port = strtol10_check_range(buf, 0, __U16_MAX);
+	if (remote_port < 0)
+		return remote_port;
+
+	nt->np.remote_port = remote_port;
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_local_ip(struct netconsole_target *nt,
+			      const char *buf,
+			      size_t count)
+{
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+				"disable to update parameters\n", nt->id);
+		return -EINVAL;
+	}
+
+	nt->np.local_ip = ntohl(in_aton(buf));
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_remote_ip(struct netconsole_target *nt,
+			       const char *buf,
+			       size_t count)
+{
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+				"disable to update parameters\n", nt->id);
+		return -EINVAL;
+	}
+
+	nt->np.remote_ip = ntohl(in_aton(buf));
+
+	return strnlen(buf, count);
+}
+
+static ssize_t store_remote_mac(struct netconsole_target *nt,
+				const char *buf,
+				size_t count)
+{
+	u8 remote_mac[ETH_ALEN];
+	char *p = (char *) buf;
+	int i;
+
+	if (nt->enabled) {
+		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
+				"disable to update parameters\n", nt->id);
+		return -EINVAL;
+	}
+
+	for (i = 0; i < ETH_ALEN - 1; i++) {
+		remote_mac[i] = simple_strtoul(p, &p, 16);
+		if (*p != ':')
+			goto invalid;
+		p++;
+	}
+	remote_mac[ETH_ALEN - 1] = simple_strtoul(p, &p, 16);
+	if (*p && (*p != '\n'))
+		goto invalid;
+
+	memcpy(nt->np.remote_mac, remote_mac, ETH_ALEN);
+
+	return strnlen(buf, count);
+
+invalid:
+	printk(KERN_ERR "netconsole: invalid input\n");
+	return -EINVAL;
+}
+
+/*
+ * Attribute definitions for netconsole_target.
+ */
+
+#define NETCONSOLE_TARGET_ATTR_RO(_name)				\
+static struct netconsole_target_attr netconsole_target_##_name =	\
+__CONFIGFS_ATTR(_name, S_IRUGO, show_##_name, NULL)
+
+#define NETCONSOLE_TARGET_ATTR_RW(_name)				\
+static struct netconsole_target_attr netconsole_target_##_name =	\
+__CONFIGFS_ATTR(_name, S_IRUGO | S_IWUSR, show_##_name, store_##_name)
+
+NETCONSOLE_TARGET_ATTR_RO(id);
+NETCONSOLE_TARGET_ATTR_RW(enabled);
+NETCONSOLE_TARGET_ATTR_RW(dev_name);
+NETCONSOLE_TARGET_ATTR_RW(local_port);
+NETCONSOLE_TARGET_ATTR_RW(remote_port);
+NETCONSOLE_TARGET_ATTR_RW(local_ip);
+NETCONSOLE_TARGET_ATTR_RW(remote_ip);
+NETCONSOLE_TARGET_ATTR_RO(local_mac);
+NETCONSOLE_TARGET_ATTR_RW(remote_mac);
+
+static struct configfs_attribute *netconsole_target_attrs[] = {
+	&netconsole_target_id.attr,
+	&netconsole_target_enabled.attr,
+	&netconsole_target_dev_name.attr,
+	&netconsole_target_local_port.attr,
+	&netconsole_target_remote_port.attr,
+	&netconsole_target_local_ip.attr,
+	&netconsole_target_remote_ip.attr,
+	&netconsole_target_local_mac.attr,
+	&netconsole_target_remote_mac.attr,
+	NULL,
+};
+
+/*
+ * Item operations and type for netconsole_target.
+ */
+
+static void netconsole_target_release(struct config_item *item)
+{
+	kfree(to_target(item));
+}
+
+static ssize_t netconsole_target_attr_show(struct config_item *item,
+					   struct configfs_attribute *attr,
+					   char *buf)
+{
+	ssize_t ret = -EINVAL;
+	struct netconsole_target *nt = to_target(item);
+	struct netconsole_target_attr *na =
+		container_of(attr, struct netconsole_target_attr, attr);
+
+	if (na->show)
+		ret = na->show(nt, buf);
+
+	return ret;
+}
+
+static ssize_t netconsole_target_attr_store(struct config_item *item,
+					    struct configfs_attribute *attr,
+					    const char *buf,
+					    size_t count)
+{
+	ssize_t ret = -EINVAL;
+	struct netconsole_target *nt = to_target(item);
+	struct netconsole_target_attr *na =
+		container_of(attr, struct netconsole_target_attr, attr);
+
+	if (na->store)
+		ret = na->store(nt, buf, count);
+
+	return ret;
+}
+
+static struct configfs_item_operations netconsole_target_item_ops = {
+	.release		= netconsole_target_release,
+	.show_attribute		= netconsole_target_attr_show,
+	.store_attribute	= netconsole_target_attr_store,
+};
+
+static struct config_item_type netconsole_target_type = {
+	.ct_attrs		= netconsole_target_attrs,
+	.ct_item_ops		= &netconsole_target_item_ops,
+	.ct_owner		= THIS_MODULE,
+};
+
+/*
+ * Group operations and type for netconsole_subsys.
+ */
+
+static struct config_item *make_netconsole_target(struct config_group *group,
+						  const char *name)
+{
+	unsigned long flags;
+	struct netconsole_target *nt;
+
+	/*
+	 * Allocate and initialize with defaults.
+	 * Note that ->enabled and ->dev_status would be zero.
+	 */
+	nt = kzalloc(sizeof(*nt), GFP_KERNEL);
+	if (!nt) {
+		printk(KERN_ERR "netconsole: failed to allocate memory\n");
+		return NULL;
+	}
+
+	nt->np.name = "netconsole";
+	strlcpy(nt->np.dev_name, "eth0", IFNAMSIZ);
+	nt->np.local_port = 6665;
+	nt->np.remote_port = 6666;
+	memset(nt->np.remote_mac, 0xff, ETH_ALEN);
+
+	/* Get an id, fill out config_item and attach us to the filesystem */
+	nt->id = atomic_inc_return(&monotonic_count);
+	config_item_init_type_name(&nt->item, name, &netconsole_target_type);
+
+	/* Adding ourselves, but netpoll is not active and we're disabled */
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_add(&nt->list, &target_list);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+
+	return &nt->item;
+}
+
+static void drop_netconsole_target(struct config_group *group,
+				   struct config_item *item)
+{
+	unsigned long flags;
+	struct netconsole_target *nt = to_target(item);
+
+	spin_lock_irqsave(&target_list_lock, flags);
+	list_del(&nt->list);
+	spin_unlock_irqrestore(&target_list_lock, flags);
+
+	/*
+	 * The target may have never been enabled, or was manually disabled
+	 * before being removed so netpoll may have already been cleaned up.
+	 */
+	if (nt->enabled)
+		netpoll_cleanup(&nt->np);
+
+	config_item_put(&nt->item);
+}
+
+static struct configfs_group_operations netconsole_subsys_group_ops = {
+	.make_item	= make_netconsole_target,
+	.drop_item	= drop_netconsole_target,
+};
+
+static struct config_item_type netconsole_subsys_type = {
+	.ct_group_ops	= &netconsole_subsys_group_ops,
+	.ct_owner	= THIS_MODULE,
+};
+
+/* The netconsole configfs subsystem */
+static struct configfs_subsystem netconsole_subsys = {
+	.su_group	= {
+		.cg_item	= {
+			.ci_namebuf	= "netconsole",
+			.ci_type	= &netconsole_subsys_type,
+		},
+	},
+};
+
+#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
+
 /* Handle network interface device notifications */
 static int netconsole_netdev_event(struct notifier_block *this,
 				   unsigned long event,
@@ -156,6 +696,7 @@ static int netconsole_netdev_event(struc
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
+		netconsole_target_get(nt);
 		if (nt->np.dev == dev) {
 			switch (event) {
 			case NETDEV_UP:
@@ -172,6 +713,7 @@ static int netconsole_netdev_event(struc
 				break;
 			}
 		}
+		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 
@@ -196,7 +738,8 @@ static void write_msg(struct console *co
 
 	spin_lock_irqsave(&target_list_lock, flags);
 	list_for_each_entry(nt, &target_list, list) {
-		if (nt->dev_status) {
+		netconsole_target_get(nt);
+		if (nt->enabled && nt->dev_status) {
 			/*
 			 * Note the double-loop nesting (for-in-buffer inside
 			 * for-all-targets) and use of temporary buf pointer.
@@ -212,6 +755,7 @@ static void write_msg(struct console *co
 				left -= frag;
 			}
 		}
+		netconsole_target_put(nt);
 	}
 	spin_unlock_irqrestore(&target_list_lock, flags);
 }
@@ -235,11 +779,12 @@ static int __init init_netconsole(void)
 	}
 
 	/*
-	 * Neither the netdev notifier, nor the console have been
-	 * registered so far. Nobody's racing us, so skip the lock.
+	 * Neither the netdev notifier, not the configfs subsystem and
+	 * nor the console have been registered so far. Nobody's racing us,
+	 * so skip the lock.
 	 */
 	while ((target_config = strsep(&input, ";"))) {
-		nt = alloc_target(target_config);
+		nt = alloc_param_target(target_config);
 		if (IS_ERR(nt)) {
 			err = PTR_ERR(nt);
 			goto fail;
@@ -251,6 +796,17 @@ static int __init init_netconsole(void)
 	if (err)
 		goto fail;
 
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+	config_group_init(&netconsole_subsys.su_group);
+	mutex_init(&netconsole_subsys.su_mtx);
+
+	err = configfs_register_subsystem(&netconsole_subsys);
+	if (err) {
+		unregister_netdevice_notifier(&netconsole_netdev_notifier);
+		goto fail;
+	}
+#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
+
 	register_console(&netconsole);
 	printk(KERN_INFO "netconsole: network logging started\n");
 
@@ -261,13 +817,17 @@ fail:
 	printk(KERN_ERR "netconsole: cleaning up\n");
 
 	/*
-	 * Remove all targets and destroy them.
-	 * The notifier is off, so nobody's racing us. So let's skip
+	 * Remove all targets and destroy them -- note that we failed
+	 * while initing the module, so only targets created while parsing
+	 * the boot/module option string could exist (this is important
+	 * because we cannot destroy configfs items from within kernel).
+	 *
+	 * Also, the notifier is off, so nobody's racing us. So let's skip
 	 * the lock, netpoll_cleanup() wants to sleep anyway.
 	 */
 	list_for_each_entry_safe(nt, tmp, &target_list, list) {
 		list_del(&nt->list);
-		free_target(nt);
+		free_param_target(nt);
 	}
 
 	return err;
@@ -278,16 +838,27 @@ static void __exit cleanup_netconsole(vo
 	struct netconsole_target *nt, *tmp;
 
 	unregister_console(&netconsole);
+
+#ifdef	CONFIG_NETCONSOLE_DYNAMIC
+	configfs_unregister_subsystem(&netconsole_subsys);
+#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
+
 	unregister_netdevice_notifier(&netconsole_netdev_notifier);
 
 	/*
-	 * Remove all targets and destroy them.
-	 * Nobody's racing us, and netpoll_cleanup() wants to
+	 * Targets created via configfs would have pinned references on
+	 * our module and wouldn't let us unload at all. They would first
+	 * need to be rmdir(2)'ed from userspace (and hence would be
+	 * removed and released already when we reach here). Only those
+	 * targets that were allocated for the boot/module option string
+	 * are left, remove and destroy them.
+	 *
+	 * Again, nobody's racing us, and netpoll_cleanup() wants to
 	 * sleep -- so skip the lock.
 	 */
 	list_for_each_entry_safe(nt, tmp, &target_list, list) {
 		list_del(&nt->list);
-		free_target(nt);
+		free_param_target(nt);
 	}
 }
 

Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Satyam Sharma]

[ Some more thoughts I had on this, for those interested. ]

> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +
> +/*
> + * Targets that were created by parsing the boot/module option string
> + * do not exist in the configfs hierarchy and will never go away (and
> + * have zeroed-out config_item members). So make these a no-op for them.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_get(&nt->item);
> +}
> +
> +static void netconsole_target_put(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_put(&nt->item);
> +}
> +
> +#else	/* !CONFIG_NETCONSOLE_DYNAMIC */
> +
>  /*
> - * Allocate new target and setup netpoll for it.
> + * No danger of targets going away from under us when dynamic
> + * reconfigurability is off.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +}
> +
> +static void netconsole_target_put(struct netconsole_target *nt)
> +{
> +}
> +
> +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */

This whole stuff (and calls to netconsole_target_get and _put from code
below) is not necessary. All the access to the netconsole_target objects
(that are present in the target_list) happens inside
spin_lock_irqsave(target_list_lock), so there's no need to do a
config_item_get() on these as there is no race window in which userspace
can delete (and thus release) these items from under us. Note that
drop_netconsole_target() removes target-to-delete from target_list
(under spin_lock_irqsave) _before_ free'ing the objects, and that both
_get() and _put() are balanced *inside* a spin_lock_irqsave /
spin_unlock_irqrestore pair -- so all this business is quite pointless.

I've kept this stuff in for now, but would like to get rid of it, really.
It's an unnecessary #ifdef-#else-#endif kludge. Comments?

> +	if (nt->enabled) {
> +		printk(KERN_ERR "netconsole: target (id %d) is enabled, "
> +				"disable to update parameters\n", nt->id);
> +		return -EINVAL;
> +	}

[ Such code exists in all the store() operations for all attributes other
than "enabled" itself. ]

Anyway, the problem is that the error does not show up violently in the
shell. The write fails, of course, values are unchanged, and dmesg shows
the diagnostic outputted above -- but "echo" does not complain about
-EINVAL returning from write(2) for some reason, neither did -EACCES.
I guess "echo" needs to be fixed. BTW would -EACCES be more appropriate?

Satyam

Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Joel Becker]

On Wed, Jul 04, 2007 at 04:38:24PM +0530, Satyam Sharma wrote:
>  struct netconsole_target {
>  	struct list_head	list;
> +	struct config_item	item;
> +	int			id;
> +	int			enabled;
>  	int			dev_status;
>  	struct netpoll		np;
>  };

	If you're trying to be good with your CONFIG_NETCONSOLE_DYNAMIC
ifdefs, you probably want to ifdef the item.  You'll save space when
NETCONSOLE_DYNAMIC is off.

> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +
> +/*
> + * Targets that were created by parsing the boot/module option string
> + * do not exist in the configfs hierarchy and will never go away (and
> + * have zeroed-out config_item members). So make these a no-op for them.
> + */
> +static void netconsole_target_get(struct netconsole_target *nt)
> +{
> +	static struct config_item empty_item;	/* Zeroed-out config_item */
> +
> +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> +		config_item_get(&nt->item);
> +}

	I was going to point out that you could merely check
config_item_name(&nt->item) != NULL, because a valid configfs object has
a name and your zeroed object does not.  But your followup email
suggests you'll be removing this code anyway.
	I don't, off the top of my head, see a problem with removing the
_get/_put cycle, because you do have them under the spinlock.  Things
should behave correctly.  However, the _get/_put pair is "cleaner", in
that it expresses the relationship and doesn't add a special case of "I
happen to know this is safe".

> + * Our subsystem hierarchy is:
> + *
> + * /config/netconsole/
> + *		     |
> + *		     <target>/
> + *		     |	     id
> + *		     |	     enabled

	Your configfs bits seem to be pretty straightforward.

> +		/*
> +		 * Skip netpoll_parse_options() -- all the attributes are
> +		 * already configured in nt->np through configfs. But at
> +		 * least let's print the useful stuff it used to output :-)
> +		 */
> +		printk(KERN_INFO "%s: local port %d\n",
> +				 np->name, np->local_port);
> +		printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
> +				 np->name, HIPQUAD(np->local_ip));
> +		printk(KERN_INFO "%s: interface %s\n",
> +				 np->name, np->dev_name);
> +		printk(KERN_INFO "%s: remote port %d\n",
> +				 np->name, np->remote_port);
> +		printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
> +				 np->name, HIPQUAD(np->remote_ip));
> +		printk(KERN_INFO "%s: remote ethernet address "
> +				 "%02x:%02x:%02x:%02x:%02x:%02x\n",
> +				 np->name,
> +				 np->remote_mac[0], np->remote_mac[1],
> +				 np->remote_mac[2], np->remote_mac[3],
> +				 np->remote_mac[4], np->remote_mac[5]);

	Shouldn't you break this out into a function so that both places
can use it?

> +#define NETCONSOLE_TARGET_ATTR_RO(_name)				\
> +static struct netconsole_target_attr netconsole_target_##_name =	\
> +__CONFIGFS_ATTR(_name, S_IRUGO, show_##_name, NULL)
> +
> +#define NETCONSOLE_TARGET_ATTR_RW(_name)				\
> +static struct netconsole_target_attr netconsole_target_##_name =	\
> +__CONFIGFS_ATTR(_name, S_IRUGO | S_IWUSR, show_##_name, store_##_name)

	Perhaps an indent would be clearer, but that's a tiny nitpick.

>  	/*
> -	 * Neither the netdev notifier, nor the console have been
> -	 * registered so far. Nobody's racing us, so skip the lock.
> +	 * Neither the netdev notifier, not the configfs subsystem and
> +	 * nor the console have been registered so far. Nobody's racing us,
> +	 * so skip the lock.

	Once again, while you know you can skip the lock, it's unclear
without the comment.  Perhaps using the lock makes it explicitly
"correct"?  Food for thought.

> @@ -251,6 +796,17 @@ static int __init init_netconsole(void)
>  	if (err)
>  		goto fail;
>  
> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +	config_group_init(&netconsole_subsys.su_group);
> +	mutex_init(&netconsole_subsys.su_mtx);
> +
> +	err = configfs_register_subsystem(&netconsole_subsys);
> +	if (err) {
> +		unregister_netdevice_notifier(&netconsole_netdev_notifier);
> +		goto fail;
> +	}
> +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */

	I'd abstract this to a dynamic_init() function.

> +
> +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> +	configfs_unregister_subsystem(&netconsole_subsys);
> +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
> +

	and this to an dynamic_fini() function.  Basically, do what you
did with _get()/_put().  This keeps the ifdef up above and out of the
functions themselves.

#ifdef CONFIG_NETCONSOLE_DYNAMIC
static int __init dynamic_netconsole_init(void)
{
	config_group_init(&netconsole_subsys.su_group);
	mutex_init(&netconsole_subsys.su_mtx);
	return configfs_register_subsystem(&netconsole_subsys);
}
#else
static int __init dynamic_netconsole_init(void)
{
	return 0;
}
#endif   


Joel

-- 

Life's Little Instruction Book #3

	"Watch a sunrise at least once a year."

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Satyam Sharma]

Hi Joel,

On Fri, 6 Jul 2007, Joel Becker wrote:
> On Wed, Jul 04, 2007 at 04:38:24PM +0530, Satyam Sharma wrote:
> >  struct netconsole_target {
> >  	struct list_head	list;
> > +	struct config_item	item;
> > +	int			id;
> > +	int			enabled;
> >  	int			dev_status;
> >  	struct netpoll		np;
> >  };
> 
> 	If you're trying to be good with your CONFIG_NETCONSOLE_DYNAMIC
> ifdefs, you probably want to ifdef the item.  You'll save space when
> NETCONSOLE_DYNAMIC is off.

Hmm, I had thought about this, but thought people might not like
another #ifdef. But then this kind of thing is very idiomatic throughout
the kernel, so ok, I'll do this.

[ BTW that .id field also looks like it can be gotten rid of altogether.
I was using it to print informational / error messages in the store()
operations below, but I guess I'll just use config_item_name() there
as well. ]

> > +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> > +
> > +/*
> > + * Targets that were created by parsing the boot/module option string
> > + * do not exist in the configfs hierarchy and will never go away (and
> > + * have zeroed-out config_item members). So make these a no-op for them.
> > + */
> > +static void netconsole_target_get(struct netconsole_target *nt)
> > +{
> > +	static struct config_item empty_item;	/* Zeroed-out config_item */
> > +
> > +	if (memcmp(&nt->item, &empty_item, sizeof(struct config_item)))
> > +		config_item_get(&nt->item);
> > +}
> 
> 	I was going to point out that you could merely check
> config_item_name(&nt->item) != NULL, because a valid configfs object has
> a name and your zeroed object does not.

Ok.

[ BTW: not related to dynamic netconsole / issue at hand, but ... I
just noticed in fs/configfs/item.c that ci_name is set to point to
embedded ci_namebuf array in config_item_set_name (if name was small
enough to be set in ci_namebuf itself), which is called from
config_item_init_type_name(). So, this keeps ci_name and ci_namebuf
in sync for such items.

However, for items that are statically initialized (often the
group->cg_item members of subsystems or default groups) we often simply
set ci_namebuf and then call config_item_init() -- say via
config_group_init(), like I've done with the netconsole subsystem in this
patch -- but config_item_init() does not set ci_name for such items to
ci_namebuf. This means the ci_name member of _initialized_ config_items
with their names in ci_namebuf is left un-initialized (NULL, actually,
because the subsys / default group would likely be static).

This doesn't really affect dynamic netconsole, because all config_items
that will be checked in the get() / put() check above are targets that
were initialized with config_item_init_type_name(), but something to
think about for perhaps other users?

Perhaps users who want to statically initialize their config_items must
be asked to just use ci_name and avoid ci_namebuf altogether? ]

> 	I don't, off the top of my head, see a problem with removing the
> _get/_put cycle, because you do have them under the spinlock.  Things
> should behave correctly.  However, the _get/_put pair is "cleaner", in
> that it expresses the relationship and doesn't add a special case of "I
> happen to know this is safe".

Right. We shouldn't special case (at least not without adding a comment
why that would be right) and we never know what might happen to the code
at some later day. So let's keep the get() / put() pair.

> > +		/*
> > +		 * Skip netpoll_parse_options() -- all the attributes are
> > +		 * already configured in nt->np through configfs. But at
> > +		 * least let's print the useful stuff it used to output :-)
> > +		 */
> > +		printk(KERN_INFO "%s: local port %d\n",
> > +				 np->name, np->local_port);
> > +		printk(KERN_INFO "%s: local IP %d.%d.%d.%d\n",
> > +				 np->name, HIPQUAD(np->local_ip));
> > +		printk(KERN_INFO "%s: interface %s\n",
> > +				 np->name, np->dev_name);
> > +		printk(KERN_INFO "%s: remote port %d\n",
> > +				 np->name, np->remote_port);
> > +		printk(KERN_INFO "%s: remote IP %d.%d.%d.%d\n",
> > +				 np->name, HIPQUAD(np->remote_ip));
> > +		printk(KERN_INFO "%s: remote ethernet address "
> > +				 "%02x:%02x:%02x:%02x:%02x:%02x\n",
> > +				 np->name,
> > +				 np->remote_mac[0], np->remote_mac[1],
> > +				 np->remote_mac[2], np->remote_mac[3],
> > +				 np->remote_mac[4], np->remote_mac[5]);
> 
> 	Shouldn't you break this out into a function so that both places
> can use it?

Hmm, netpoll_parse_options() currently prints these separately as and
how it walks through the input string parsing it. But changing that to
just print all these out together at the end if / when the parse is
successful seems better than what it's doing right now. I'll do this too.

> > +#define NETCONSOLE_TARGET_ATTR_RO(_name)				\
> > +static struct netconsole_target_attr netconsole_target_##_name =	\
> > +__CONFIGFS_ATTR(_name, S_IRUGO, show_##_name, NULL)
> > +
> > +#define NETCONSOLE_TARGET_ATTR_RW(_name)				\
> > +static struct netconsole_target_attr netconsole_target_##_name =	\
> > +__CONFIGFS_ATTR(_name, S_IRUGO | S_IWUSR, show_##_name, store_##_name)
> 
> 	Perhaps an indent would be clearer, but that's a tiny nitpick.

Yes, an indent would be good there.

> >  	/*
> > -	 * Neither the netdev notifier, nor the console have been
> > -	 * registered so far. Nobody's racing us, so skip the lock.
> > +	 * Neither the netdev notifier, not the configfs subsystem and
> > +	 * nor the console have been registered so far. Nobody's racing us,
> > +	 * so skip the lock.
> 
> 	Once again, while you know you can skip the lock, it's unclear
> without the comment.  Perhaps using the lock makes it explicitly
> "correct"?  Food for thought.

Yes. That special-casing was really ugly and unnecessary -- we just need
to hold the lock around list_add(). I was going to set that right in the
next iteration myself.

> > @@ -251,6 +796,17 @@ static int __init init_netconsole(void)
> >  	if (err)
> >  		goto fail;
> >  
> > +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> > +	config_group_init(&netconsole_subsys.su_group);
> > +	mutex_init(&netconsole_subsys.su_mtx);
> > +
> > +	err = configfs_register_subsystem(&netconsole_subsys);
> > +	if (err) {
> > +		unregister_netdevice_notifier(&netconsole_netdev_notifier);
> > +		goto fail;
> > +	}
> > +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
> 
> 	I'd abstract this to a dynamic_init() function.
> 
> > +
> > +#ifdef	CONFIG_NETCONSOLE_DYNAMIC
> > +	configfs_unregister_subsystem(&netconsole_subsys);
> > +#endif	/* CONFIG_NETCONSOLE_DYNAMIC */
> > +
> 
> 	and this to an dynamic_fini() function.  Basically, do what you
> did with _get()/_put().  This keeps the ifdef up above and out of the
> functions themselves.
> 
> #ifdef CONFIG_NETCONSOLE_DYNAMIC
> static int __init dynamic_netconsole_init(void)
> {
> 	config_group_init(&netconsole_subsys.su_group);
> 	mutex_init(&netconsole_subsys.su_mtx);
> 	return configfs_register_subsystem(&netconsole_subsys);
> }
> #else
> static int __init dynamic_netconsole_init(void)
> {
> 	return 0;
> }
> #endif   

Right, this looks neater and is again the idiom followed throughout the
kernel.

Thanks,
Satyam

Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Joel Becker]

On Sat, Jul 07, 2007 at 01:18:45PM +0530, Satyam Sharma wrote:
> However, for items that are statically initialized (often the
> group->cg_item members of subsystems or default groups) we often simply
> set ci_namebuf and then call config_item_init() -- say via
> config_group_init(), like I've done with the netconsole subsystem in this
> patch -- but config_item_init() does not set ci_name for such items to
> ci_namebuf. This means the ci_name member of _initialized_ config_items
> with their names in ci_namebuf is left un-initialized (NULL, actually,
> because the subsys / default group would likely be static).

	Configfs notices subsystems and default groups and handles this.
See configfs_register_subsystem() and create_default_group() in
fs/configfs/dir.c

> Right. We shouldn't special case (at least not without adding a comment
> why that would be right) and we never know what might happen to the code
> at some later day. So let's keep the get() / put() pair.

	If you're keeping them, don't do the "empty_item", check the
name.
	Otherwise, the changes you describe sound good.

Joel


-- 

"I have never let my schooling interfere with my education."
        - Mark Twain

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127

Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[KII Keiichi]

Hi Satyam,

> After more discussion, Joel suggested the idea to separately manage the
> lifetimes of the two kinds of netconsole_target objects, those created while
> parsing boot/module options and those created via configfs mkdir(2) from
> userspace, in my driver itself. That adds a little bit of complexity and
> redundancy (multiple item creation and destruction points) in this driver.
> Note that the latter type (configfs-created netconsole_targets) can be
> modified / updated / destroyed at runtime from userspace but the former
> (param string created) cannot, as they are not exposed through our configfs
> namespace. However, this is not really a problem and is similar to current
> behaviour in any case.

Maybe you should add the above description into 
Documentation/networking/netconsole.txt.
The users of netconsole applied these patches may think that they can change 
parameters of netconsole_target(param string created at the time of parsing 
boot/options).

And I'm going to search the another solution of this problem and read the 
code of configfs.

Thanks
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs

[Satyam Sharma]

Hi Keiichi,

On Sun, 8 Jul 2007, KII Keiichi wrote:
> Hi Satyam,
> 
> > After more discussion, Joel suggested the idea to separately manage the
> > lifetimes of the two kinds of netconsole_target objects, those created while
> > parsing boot/module options and those created via configfs mkdir(2) from
> > userspace, in my driver itself. That adds a little bit of complexity and
> > redundancy (multiple item creation and destruction points) in this driver.
> > Note that the latter type (configfs-created netconsole_targets) can be
> > modified / updated / destroyed at runtime from userspace but the former
> > (param string created) cannot, as they are not exposed through our configfs
> > namespace. However, this is not really a problem and is similar to current
> > behaviour in any case.
> 
> Maybe you should add the above description into 
> Documentation/networking/netconsole.txt.
> The users of netconsole applied these patches may think that they can change 
> parameters of netconsole_target(param string created at the time of parsing 
> boot/options).

Yes, you're absolutely right. This definitely needs to be mentioned in the
documenation -- I'll take care of this in the next iteration.

Satyam

Re: [RFC][PATCH -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Keiichi KII]

Hi Satyam,
 
At first, thank you for your helping.
I'm going to try/review your patches.

>[ I had initially thought about only modifying Keiichi's patches to
>incorporate the suggested stuff, sysfs+ioctl->configfs conversion, etc,
>but the changes became too much and I ultimately ended up rewriting the
>patchset. This also gave the flexibility to make other changes possible.
>Keiichi, Takayoshi, kindly review this, and if OK, please give your
>Signed-off-by:'s to the patches herein. ]

By the way, how can I add Signed-off-by:'s to your patches?
Should I add it to each patch and submit them to this list again?

Anybody could give me an advice?
-- 
Keiichi KII
NEC Corporation OSS Platform Development Division
E-mail: k-keiichi@bx.jp.nec.com

Re: [RFC][PATCH -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Satyam Sharma]

Hi Keiichi,

On Wed, 4 Jul 2007, Keiichi KII wrote:
> [...]
> At first, thank you for your helping.
> I'm going to try/review your patches.
> 
> >[ I had initially thought about only modifying Keiichi's patches to
> >incorporate the suggested stuff, sysfs+ioctl->configfs conversion, etc,
> >but the changes became too much and I ultimately ended up rewriting the
> >patchset. This also gave the flexibility to make other changes possible.
> >Keiichi, Takayoshi, kindly review this, and if OK, please give your
> >Signed-off-by:'s to the patches herein. ]
> 
> By the way, how can I add Signed-off-by:'s to your patches?
> Should I add it to each patch and submit them to this list again?
> 
> Anybody could give me an advice?

I think you could just individually reply to all the mails and replace
the "Cc:" in front of your name + email ID with "Signed-off-by:" --
after reviewing & testing this, of course!

Thanks,
Satyam

Re: [RFC][PATCH -mm 0/9] netconsole: Multiple targets and dynamic reconfigurability

[Joel Becker]

On Wed, Jul 04, 2007 at 05:55:15PM +0530, Satyam Sharma wrote:
> On Wed, 4 Jul 2007, Keiichi KII wrote:
> > By the way, how can I add Signed-off-by:'s to your patches?
> > Should I add it to each patch and submit them to this list again?
> > 
> > Anybody could give me an advice?
> 
> I think you could just individually reply to all the mails and replace
> the "Cc:" in front of your name + email ID with "Signed-off-by:" --
> after reviewing & testing this, of course!

	If the chain is not going through Keiichi, he should be
responding with "Acked-by:", not "Signed-off-by:".  Then Satyam submits
the patches upstream with the Acked-by: line and his own Signed-off-by:

Joel

-- 

"Anything that is too stupid to be spoken is sung."  
        - Voltaire

Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127