| Message ID | 20201002174001.3012643-7-jarod@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | bonding: rename bond components | expand |
On Fri, 2 Oct 2020 13:40:01 -0400 Jarod Wilson <jarod@redhat.com> wrote: > By default, enable retaining all user-facing API that includes the use of > master and slave, but add a Kconfig knob that allows those that wish to > remove it entirely do so in one shot. > > Cc: Jay Vosburgh <j.vosburgh@gmail.com> > Cc: Veaceslav Falico <vfalico@gmail.com> > Cc: Andy Gospodarek <andy@greyhouse.net> > Cc: "David S. Miller" <davem@davemloft.net> > Cc: Jakub Kicinski <kuba@kernel.org> > Cc: Thomas Davis <tadavis@lbl.gov> > Cc: netdev@vger.kernel.org > Signed-off-by: Jarod Wilson <jarod@redhat.com> > --- > drivers/net/Kconfig | 12 ++++++++++++ > drivers/net/bonding/bond_main.c | 4 ++-- > drivers/net/bonding/bond_options.c | 4 ++-- > drivers/net/bonding/bond_procfs.c | 8 ++++++++ > drivers/net/bonding/bond_sysfs.c | 14 ++++++++++---- > drivers/net/bonding/bond_sysfs_port.c | 6 ++++-- > 6 files changed, 38 insertions(+), 10 deletions(-) > This is problematic. You are printing both old and new values. Also every distribution will have to enable it. This looks like too much of change to users.
On Fri, Oct 2, 2020 at 3:13 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Fri, 2 Oct 2020 13:40:01 -0400 > Jarod Wilson <jarod@redhat.com> wrote: > > > By default, enable retaining all user-facing API that includes the use of > > master and slave, but add a Kconfig knob that allows those that wish to > > remove it entirely do so in one shot. > > > > Cc: Jay Vosburgh <j.vosburgh@gmail.com> > > Cc: Veaceslav Falico <vfalico@gmail.com> > > Cc: Andy Gospodarek <andy@greyhouse.net> > > Cc: "David S. Miller" <davem@davemloft.net> > > Cc: Jakub Kicinski <kuba@kernel.org> > > Cc: Thomas Davis <tadavis@lbl.gov> > > Cc: netdev@vger.kernel.org > > Signed-off-by: Jarod Wilson <jarod@redhat.com> > > --- > > drivers/net/Kconfig | 12 ++++++++++++ > > drivers/net/bonding/bond_main.c | 4 ++-- > > drivers/net/bonding/bond_options.c | 4 ++-- > > drivers/net/bonding/bond_procfs.c | 8 ++++++++ > > drivers/net/bonding/bond_sysfs.c | 14 ++++++++++---- > > drivers/net/bonding/bond_sysfs_port.c | 6 ++++-- > > 6 files changed, 38 insertions(+), 10 deletions(-) > > > > This is problematic. You are printing both old and new values. > Also every distribution will have to enable it. > > This looks like too much of change to users. I'd had a bit of feedback that people would rather see both, and be able to toggle off the old ones, rather than only having one or the other, depending on the toggle, so I thought I'd give this a try. I kind of liked the one or the other route, but I see the problems with that too. For simplicity, I'm kind of liking the idea of just not updating the proc and sysfs interfaces, have a toggle entirely disable them, and work on enhancing userspace to only use netlink, but ... it's going to be a while before any such work makes its way to any already shipping distros. I don't have a satisfying answer here. -- Jarod Wilson jarod@redhat.com
On Fri, 2 Oct 2020 16:23:46 -0400 Jarod Wilson <jarod@redhat.com> wrote: > On Fri, Oct 2, 2020 at 3:13 PM Stephen Hemminger > <stephen@networkplumber.org> wrote: > > > > On Fri, 2 Oct 2020 13:40:01 -0400 > > Jarod Wilson <jarod@redhat.com> wrote: > > > > > By default, enable retaining all user-facing API that includes the use of > > > master and slave, but add a Kconfig knob that allows those that wish to > > > remove it entirely do so in one shot. > > > > > > Cc: Jay Vosburgh <j.vosburgh@gmail.com> > > > Cc: Veaceslav Falico <vfalico@gmail.com> > > > Cc: Andy Gospodarek <andy@greyhouse.net> > > > Cc: "David S. Miller" <davem@davemloft.net> > > > Cc: Jakub Kicinski <kuba@kernel.org> > > > Cc: Thomas Davis <tadavis@lbl.gov> > > > Cc: netdev@vger.kernel.org > > > Signed-off-by: Jarod Wilson <jarod@redhat.com> > > > --- > > > drivers/net/Kconfig | 12 ++++++++++++ > > > drivers/net/bonding/bond_main.c | 4 ++-- > > > drivers/net/bonding/bond_options.c | 4 ++-- > > > drivers/net/bonding/bond_procfs.c | 8 ++++++++ > > > drivers/net/bonding/bond_sysfs.c | 14 ++++++++++---- > > > drivers/net/bonding/bond_sysfs_port.c | 6 ++++-- > > > 6 files changed, 38 insertions(+), 10 deletions(-) > > > > > > > This is problematic. You are printing both old and new values. > > Also every distribution will have to enable it. > > > > This looks like too much of change to users. > > I'd had a bit of feedback that people would rather see both, and be > able to toggle off the old ones, rather than only having one or the > other, depending on the toggle, so I thought I'd give this a try. I > kind of liked the one or the other route, but I see the problems with > that too. > > For simplicity, I'm kind of liking the idea of just not updating the > proc and sysfs interfaces, have a toggle entirely disable them, and > work on enhancing userspace to only use netlink, but ... it's going to > be a while before any such work makes its way to any already shipping > distros. I don't have a satisfying answer here. > I like the idea of having bonding proc and sysf apis optional.
From: Stephen Hemminger <stephen@networkplumber.org> Date: Fri, 2 Oct 2020 12:13:17 -0700 > On Fri, 2 Oct 2020 13:40:01 -0400 > Jarod Wilson <jarod@redhat.com> wrote: > >> By default, enable retaining all user-facing API that includes the use of >> master and slave, but add a Kconfig knob that allows those that wish to >> remove it entirely do so in one shot. > > This is problematic. You are printing both old and new values. > Also every distribution will have to enable it. > > This looks like too much of change to users. Agreed, no Kconfig knob. Keep everything during the deprecation period, then you can kill off the old names at the end of the deprecation period. I don't want to create a situation where distribution X lists as a "feature" that they are able to enable this Kconfig value even though it breaks UAPI for legacy external code out there. That's the wrong way to go about this and creates the wrong incentive system. I also agree that you can't change the docs to stop mentioning the old names. It's almost like we are pretending we aren't doing the transition and that only the new names matter. The old names matter and must therefore be acknowledged, exist, and be referencable in the documentation until the end of the deprecation period. You can mark them "(DEPRECATED)" or similar, but they can't be removed just yet. Thank you.
From: Jarod Wilson <jarod@redhat.com> Date: Fri, 2 Oct 2020 16:23:46 -0400 > I'd had a bit of feedback that people would rather see both, and be > able to toggle off the old ones, rather than only having one or the > other, depending on the toggle, so I thought I'd give this a try. I > kind of liked the one or the other route, but I see the problems with > that too. Please keep everything for the entire deprecation period, unconditionally. Thank you.
On Fri, Oct 2, 2020 at 6:57 PM David Miller <davem@davemloft.net> wrote: > > From: Jarod Wilson <jarod@redhat.com> > Date: Fri, 2 Oct 2020 16:23:46 -0400 > > > I'd had a bit of feedback that people would rather see both, and be > > able to toggle off the old ones, rather than only having one or the > > other, depending on the toggle, so I thought I'd give this a try. I > > kind of liked the one or the other route, but I see the problems with > > that too. > > Please keep everything for the entire deprecation period, unconditionally. Okay, so 100% drop the Kconfig flag patch, but duplicate sysfs interface names are acceptable, correct? Then what about the procfs file having duplicate Slave and Port lines? Just leave them all as Slave? -- Jarod Wilson jarod@redhat.com
On Fri, Oct 2, 2020 at 6:42 PM Stephen Hemminger <stephen@networkplumber.org> wrote: > > On Fri, 2 Oct 2020 16:23:46 -0400 > Jarod Wilson <jarod@redhat.com> wrote: > > > On Fri, Oct 2, 2020 at 3:13 PM Stephen Hemminger > > <stephen@networkplumber.org> wrote: > > > > > > On Fri, 2 Oct 2020 13:40:01 -0400 > > > Jarod Wilson <jarod@redhat.com> wrote: > > > > > > > By default, enable retaining all user-facing API that includes the use of > > > > master and slave, but add a Kconfig knob that allows those that wish to > > > > remove it entirely do so in one shot. ... > > > This is problematic. You are printing both old and new values. > > > Also every distribution will have to enable it. > > > > > > This looks like too much of change to users. > > > > I'd had a bit of feedback that people would rather see both, and be > > able to toggle off the old ones, rather than only having one or the > > other, depending on the toggle, so I thought I'd give this a try. I > > kind of liked the one or the other route, but I see the problems with > > that too. > > > > For simplicity, I'm kind of liking the idea of just not updating the > > proc and sysfs interfaces, have a toggle entirely disable them, and > > work on enhancing userspace to only use netlink, but ... it's going to > > be a while before any such work makes its way to any already shipping > > distros. I don't have a satisfying answer here. > > > > I like the idea of having bonding proc and sysf apis optional. I do too, but I'd see it more as something only userspace developers would care about for a while, as an easy way to make absolutely certain their code/distro is no longer reliant on them and only uses netlink, not as something any normal user really has any reason to do.
Jarod Wilson <jarod@redhat.com> wrote: >On Fri, Oct 2, 2020 at 6:57 PM David Miller <davem@davemloft.net> wrote: >> >> From: Jarod Wilson <jarod@redhat.com> >> Date: Fri, 2 Oct 2020 16:23:46 -0400 >> >> > I'd had a bit of feedback that people would rather see both, and be >> > able to toggle off the old ones, rather than only having one or the >> > other, depending on the toggle, so I thought I'd give this a try. I >> > kind of liked the one or the other route, but I see the problems with >> > that too. >> >> Please keep everything for the entire deprecation period, unconditionally. > >Okay, so 100% drop the Kconfig flag patch, but duplicate sysfs >interface names are acceptable, correct? Then what about the procfs >file having duplicate Slave and Port lines? Just leave them all as >Slave? My preference is to not alter the existing sysfs / proc behavior at all, and instead create a netlink / iproute UAPI that becomes the "preferred" UAPI going forward. Any new functionality would only be added to netlink as incentive to switch. I don't see the value in adding duplicate fields, as userspace code that parses them will not be portable if it only checks for the new field name. Then, down the road, deleting the old names will break the userspace code that was never updated (which will likely be most of it). I would rather see a single clean break from proc and sysfs to netlink in one step than a piecemeal addition and removal from the existing UAPI. That makes for a much clearer flag day event for end users. By this I mean leave proc / sysfs as-is today, and then after a suitable deprecation period, remove it wholesale (rather than a compile time option). -J --- -Jay Vosburgh, jay.vosburgh@canonical.com
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig index c3dbe64e628e..1a13894820cb 100644 --- a/drivers/net/Kconfig +++ b/drivers/net/Kconfig @@ -56,6 +56,18 @@ config BONDING To compile this driver as a module, choose M here: the module will be called bonding. +config BONDING_LEGACY_INTERFACES + default y + bool "Maintain legacy bonding interface names" + help + The bonding driver historically made use of the terms "master" and + "slave" to describe it's component members. This has since been + changed to "bond" and "port" as part of a broader effort to remove + the use of socially problematic language from the kernel. However, + removing all such cases requires breaking long-standing user-facing + interfaces in /proc and /sys, which will not be done, unless you + opt out of them here, by selecting 'N'. + config DUMMY tristate "Dummy net driver support" help diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c index b8a351d85da4..226d5fb76221 100644 --- a/drivers/net/bonding/bond_main.c +++ b/drivers/net/bonding/bond_main.c @@ -194,7 +194,7 @@ module_param(lp_interval, uint, 0); MODULE_PARM_DESC(lp_interval, "The number of seconds between instances where " "the bonding driver sends learning packets to " "each port's peer switch. The default is 1."); -/* legacy compatability module parameters */ +#ifdef CONFIG_BONDING_LEGACY_INTERFACES module_param_named(all_slaves_active, apa, int, 0644); MODULE_PARM_DESC(all_slaves_active, "Keep all frames received on an interface " "by setting active flag for all slaves; " @@ -205,7 +205,7 @@ MODULE_PARM_DESC(packets_per_slave, "Packets to send per slave in balance-rr " "mode; 0 for a random slave, 1 packet per " "slave (default), >1 packets per slave. " "(Legacy compat synonym for packets_per_port)."); -/* end legacy compatability module parameters */ +#endif /*----------------------------- Global variables ----------------------------*/ diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c index 8e4050c2b08e..630079ba5452 100644 --- a/drivers/net/bonding/bond_options.c +++ b/drivers/net/bonding/bond_options.c @@ -434,7 +434,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = { .values = bond_intmax_tbl, .set = bond_option_peer_notif_delay_set }, -/* legacy sysfs interfaces */ +#ifdef CONFIG_BONDING_LEGACY_INTERFACES [BOND_OPT_PACKETS_PER_SLAVE] = { .id = BOND_OPT_PACKETS_PER_SLAVE, .name = "packets_per_slave", @@ -467,7 +467,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = { .flags = BOND_OPTFLAG_RAWVAL, .set = bond_option_ports_set }, -/* end legacy sysfs interfaces */ +#endif }; /* Searches for an option by name */ diff --git a/drivers/net/bonding/bond_procfs.c b/drivers/net/bonding/bond_procfs.c index 2e65472e3c58..8e4a03d86329 100644 --- a/drivers/net/bonding/bond_procfs.c +++ b/drivers/net/bonding/bond_procfs.c @@ -86,8 +86,10 @@ static void bond_info_show_bond_dev(struct seq_file *seq) primary = rcu_dereference(bond->primary_port); seq_printf(seq, "Primary Port: %s", primary ? primary->dev->name : "None"); +#ifdef CONFIG_BONDING_LEGACY_INTERFACES seq_printf(seq, "Primary Slave: %s", primary ? primary->dev->name : "None"); +#endif if (primary) { optval = bond_opt_get_val(BOND_OPT_PRIMARY_RESELECT, bond->params.primary_reselect); @@ -97,8 +99,10 @@ static void bond_info_show_bond_dev(struct seq_file *seq) seq_printf(seq, "\nCurrently Active Port: %s\n", (curr) ? curr->dev->name : "None"); +#ifdef CONFIG_BONDING_LEGACY_INTERFACES seq_printf(seq, "Currently Active Slave: %s\n", (curr) ? curr->dev->name : "None"); +#endif } seq_printf(seq, "MII Status: %s\n", netif_carrier_ok(bond->dev) ? @@ -176,7 +180,9 @@ static void bond_info_show_port(struct seq_file *seq, struct bonding *bond = PDE_DATA(file_inode(seq->file)); seq_printf(seq, "\nPort Interface: %s\n", port->dev->name); +#ifdef CONFIG_BONDING_LEGACY_INTERFACES seq_printf(seq, "Slave Interface: %s\n", port->dev->name); +#endif seq_printf(seq, "MII Status: %s\n", bond_port_link_status(port->link)); if (port->speed == SPEED_UNKNOWN) seq_printf(seq, "Speed: %s\n", "Unknown"); @@ -194,7 +200,9 @@ static void bond_info_show_port(struct seq_file *seq, seq_printf(seq, "Permanent HW addr: %*phC\n", port->dev->addr_len, port->perm_hwaddr); seq_printf(seq, "Port queue ID: %d\n", port->queue_id); +#ifdef CONFIG_BONDING_LEGACY_INTERFACES seq_printf(seq, "Slave queue ID: %d\n", port->queue_id); +#endif if (BOND_MODE(bond) == BOND_MODE_8023AD) { const struct ad_port *ad_port = &PORT_AD_INFO(port)->ad_port; diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c index 1c2f44a76f31..1911027ea2e1 100644 --- a/drivers/net/bonding/bond_sysfs.c +++ b/drivers/net/bonding/bond_sysfs.c @@ -150,6 +150,7 @@ static const struct class_attribute class_attr_bonding_devs = { .store = bonding_store_bonds, }; +#ifdef CONFIG_BONDING_LEGACY_INTERFACES /* "show" function for the bond_masters attribute. * The class parameter is ignored. */ @@ -187,6 +188,7 @@ static const struct class_attribute class_attr_bonding_masters = { .show = bonding_show_bonds_legacy, .store = bonding_store_bonds_legacy, }; +#endif /* Generic "store" method for bonding sysfs option setting */ static ssize_t bonding_sysfs_store_option(struct device *d, @@ -771,7 +773,7 @@ static ssize_t bonding_show_ad_user_port_key(struct device *d, static DEVICE_ATTR(ad_user_port_key, 0644, bonding_show_ad_user_port_key, bonding_sysfs_store_option); -/* legacy sysfs interfaces */ +#ifdef CONFIG_BONDING_LEGACY_INTERFACES static DEVICE_ATTR(slaves, 0644, bonding_show_ports, bonding_sysfs_store_option); static DEVICE_ATTR(active_slave, 0644, @@ -780,7 +782,7 @@ static DEVICE_ATTR(all_slaves_active, 0644, bonding_show_ports_active, bonding_sysfs_store_option); static DEVICE_ATTR(packets_per_slave, 0644, bonding_show_packets_per_port, bonding_sysfs_store_option); -/* end legacy sysfs interfaces */ +#endif static struct attribute *per_bond_attrs[] = { &dev_attr_ports.attr, @@ -819,12 +821,12 @@ static struct attribute *per_bond_attrs[] = { &dev_attr_ad_actor_sys_prio.attr, &dev_attr_ad_actor_system.attr, &dev_attr_ad_user_port_key.attr, -/* legacy sysfs interfaces */ +#ifdef CONFIG_BONDING_LEGACY_INTERFACES &dev_attr_slaves.attr, &dev_attr_active_slave.attr, &dev_attr_all_slaves_active.attr, &dev_attr_packets_per_slave.attr, -/* end legacy sysfs interfaces */ +#endif NULL, }; @@ -871,6 +873,7 @@ int bond_create_sysfs(struct bond_net *bn) return ret; } +#ifdef CONFIG_BONDING_LEGACY_INTERFACES bn->class_attr_bonding_masters = class_attr_bonding_masters; sysfs_attr_init(&bn->class_attr_bonding_masters.attr); @@ -884,6 +887,7 @@ int bond_create_sysfs(struct bond_net *bn) class_attr_bonding_masters.attr.name); ret = 0; } +#endif return ret; @@ -893,7 +897,9 @@ int bond_create_sysfs(struct bond_net *bn) void bond_destroy_sysfs(struct bond_net *bn) { netdev_class_remove_file_ns(&bn->class_attr_bonding_devs, bn->net); +#ifdef CONFIG_BONDING_LEGACY_INTERFACES netdev_class_remove_file_ns(&bn->class_attr_bonding_masters, bn->net); +#endif } /* Initialize sysfs for each bond. This sets up and registers diff --git a/drivers/net/bonding/bond_sysfs_port.c b/drivers/net/bonding/bond_sysfs_port.c index 0d427b407fcb..81fbe3deeb3e 100644 --- a/drivers/net/bonding/bond_sysfs_port.c +++ b/drivers/net/bonding/bond_sysfs_port.c @@ -152,11 +152,12 @@ int bond_sysfs_port_add(struct bond_port *port) if (err) goto err_kobject_put; - /* legacy sysfs interface */ +#ifdef CONFIG_BONDING_LEGACY_INTERFACES err = sysfs_create_link(&(port->dev->dev.kobj), &port->kobj, "bonding_slave"); if (err) goto err_kobject_put; +#endif for (a = port_attrs; *a; ++a) { err = sysfs_create_file(&port->kobj, &((*a)->attr)); @@ -178,8 +179,9 @@ void bond_sysfs_port_del(struct bond_port *port) for (a = port_attrs; *a; ++a) sysfs_remove_file(&port->kobj, &((*a)->attr)); - /* legacy sysfs interface */ +#ifdef CONFIG_BONDING_LEGACY_INTERFACES sysfs_remove_link(&(port->dev->dev.kobj), "bonding_slave"); +#endif kobject_put(&port->kobj); }
By default, enable retaining all user-facing API that includes the use of master and slave, but add a Kconfig knob that allows those that wish to remove it entirely do so in one shot. Cc: Jay Vosburgh <j.vosburgh@gmail.com> Cc: Veaceslav Falico <vfalico@gmail.com> Cc: Andy Gospodarek <andy@greyhouse.net> Cc: "David S. Miller" <davem@davemloft.net> Cc: Jakub Kicinski <kuba@kernel.org> Cc: Thomas Davis <tadavis@lbl.gov> Cc: netdev@vger.kernel.org Signed-off-by: Jarod Wilson <jarod@redhat.com> --- drivers/net/Kconfig | 12 ++++++++++++ drivers/net/bonding/bond_main.c | 4 ++-- drivers/net/bonding/bond_options.c | 4 ++-- drivers/net/bonding/bond_procfs.c | 8 ++++++++ drivers/net/bonding/bond_sysfs.c | 14 ++++++++++---- drivers/net/bonding/bond_sysfs_port.c | 6 ++++-- 6 files changed, 38 insertions(+), 10 deletions(-)