Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
On Wed, Jul 04, 2007 at 04:38:24PM +0530, Satyam Sharma wrote:
struct netconsole_target {
struct list_headlist;
+ 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
Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
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_headlist;
+ 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
Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
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: [EMAIL PROTECTED] Phone: (650) 506-8127 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
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: [EMAIL PROTECTED] - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
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 - To unsubscribe from this list: send the line unsubscribe netdev in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
From: Satyam Sharma [EMAIL PROTECTED] [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 [EMAIL PROTECTED] Cc: Keiichi Kii [EMAIL PROTECTED] Cc: Takayoshi Kochi [EMAIL PROTECTED] --- 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.0 +0530 +++ b/drivers/net/Kconfig 2007-07-04 13:58:03.0 +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.0 +0530 +++ b/drivers/net/netconsole.c 2007-07-04 13:53:14.0 +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 [EMAIL PROTECTED]); MODULE_DESCRIPTION(Console driver for network interfaces); @@ -69,6
Re: [PATCH -mm 9/9] netconsole: Support dynamic reconfiguration using configfs
[ 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
-
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
