Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
Jarod Wilson wrote: >On Tue, Sep 22, 2020 at 8:01 PM Stephen Hemminger > wrote: >> >> On Tue, 22 Sep 2020 16:47:07 -0700 >> Jay Vosburgh wrote: >> >> > Stephen Hemminger wrote: >> > >> > >On Tue, 22 Sep 2020 09:37:30 -0400 >> > >Jarod Wilson 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 >> > >> Cc: Veaceslav Falico >> > >> Cc: Andy Gospodarek >> > >> Cc: "David S. Miller" >> > >> Cc: Jakub Kicinski >> > >> Cc: Thomas Davis >> > >> Cc: net...@vger.kernel.org >> > >> Signed-off-by: Jarod Wilson >> > > >> > >Why not just have a config option to remove all the /proc and sysfs >> > >options >> > >in bonding (and bridging) and only use netlink? New tools should be only >> > >able >> > >to use netlink only. >> > >> > I agree that new tooling should be netlink, but what value is >> > provided by such an option that distros are unlikely to enable, and >> > enabling will break the UAPI? > >Do you mean the initial proposed option, or what Stephen is >suggesting? I think Red Hat actually will consider the former, the >latter is less likely in the immediate future, since so many people >still rely on the output of /proc/net/bonding/* for an overall view of >their bonds' health and status. I don't know how close we are to >having something comparable that could be spit out with a single >invocation of something like 'ip' that would only be using netlink. >It's entirely possible there's something akin to 'ip link bondX >overview' already that outputs something similar, and I'm just not >aware of it, but something like that would definitely need to exist >and be well-documented for Red Hat to remove the procfs bits, I think. At the present time, as much as the idea spurs the imagination, removing the bonding /proc and sysfs stuff wholesale is not feasible. As you explain, not everything in the proc file is available from other sources. I would rather freeze the /proc and sysfs bonding functionality and move to a netlink / iproute API for all of it, and then down the road remove the then-legacy APIs. Even though "down the road" may practically be "never" (because the removal breaks backwards compatibility for user space), unifying all of the configuration and reporting to one place would be worthwhile. For "initial proposed option," I'm not sure right off if that's referring to CONFIG_BONDING_LEGACY_INTERFACES or "duplicate lines in /proc/net/bonding." I'm not sure it matters, since both have the same problem, in that they create a Venn diagram of mutually incompatible bonding UAPIs. Portable user space code ends up having to handle all of the permutations. -J >> > >Then you might convince maintainers to update documentation as well. >> > >Last I checked there were still references to ifenslave. >> > >> > Distros still include ifenslave, but it's now a shell script >> > that uses sysfs. I see it used in scripts from time to time. >> >> Some bleeding edge distros have already dropped ifenslave and even ifconfig. >> The Enterprise ones never will. >> >> The one motivation would be for the embedded folks which are always looking >> to trim out the fat. Although not sure if the minimal versions of commands >> in busybox are pure netlink yet. > >Yeah, the bonding documentation is still filled with references to >ifenslave. I believe Red Hat still includes it, though it's >"deprecated" in documentation in favor of using ip. Similar with >ifconfig. I could see them both getting dropped in a future major >release of Red Hat Enterprise Linux, but they're definitely still here >for at least the life of RHEL8. As ifconfig is typically bundled in with the much-loved netstat in the net-tools package, it will be difficult to remove. Having an /sbin/ifenslave program isn't really the issue so much as its reliance on the bonding sysfs UAPI. It's a shell script, and could likely be reworked to use ip link. -J --- -Jay Vosburgh, jay.vosbu...@canonical.com
Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
On Tue, Sep 22, 2020 at 8:01 PM Stephen Hemminger wrote: > > On Tue, 22 Sep 2020 16:47:07 -0700 > Jay Vosburgh wrote: > > > Stephen Hemminger wrote: > > > > >On Tue, 22 Sep 2020 09:37:30 -0400 > > >Jarod Wilson 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 > > >> Cc: Veaceslav Falico > > >> Cc: Andy Gospodarek > > >> Cc: "David S. Miller" > > >> Cc: Jakub Kicinski > > >> Cc: Thomas Davis > > >> Cc: net...@vger.kernel.org > > >> Signed-off-by: Jarod Wilson > > > > > >Why not just have a config option to remove all the /proc and sysfs options > > >in bonding (and bridging) and only use netlink? New tools should be only > > >able > > >to use netlink only. > > > > I agree that new tooling should be netlink, but what value is > > provided by such an option that distros are unlikely to enable, and > > enabling will break the UAPI? Do you mean the initial proposed option, or what Stephen is suggesting? I think Red Hat actually will consider the former, the latter is less likely in the immediate future, since so many people still rely on the output of /proc/net/bonding/* for an overall view of their bonds' health and status. I don't know how close we are to having something comparable that could be spit out with a single invocation of something like 'ip' that would only be using netlink. It's entirely possible there's something akin to 'ip link bondX overview' already that outputs something similar, and I'm just not aware of it, but something like that would definitely need to exist and be well-documented for Red Hat to remove the procfs bits, I think. > > >Then you might convince maintainers to update documentation as well. > > >Last I checked there were still references to ifenslave. > > > > Distros still include ifenslave, but it's now a shell script > > that uses sysfs. I see it used in scripts from time to time. > > Some bleeding edge distros have already dropped ifenslave and even ifconfig. > The Enterprise ones never will. > > The one motivation would be for the embedded folks which are always looking > to trim out the fat. Although not sure if the minimal versions of commands > in busybox are pure netlink yet. Yeah, the bonding documentation is still filled with references to ifenslave. I believe Red Hat still includes it, though it's "deprecated" in documentation in favor of using ip. Similar with ifconfig. I could see them both getting dropped in a future major release of Red Hat Enterprise Linux, but they're definitely still here for at least the life of RHEL8. -- Jarod Wilson ja...@redhat.com
Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
Hi Jarod, Thank you for the patch! Perhaps something to improve: [auto build test WARNING on net-next/master] url: https://github.com/0day-ci/linux/commits/Jarod-Wilson/bonding-rename-bond-components/20200922-214046 base: https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 92ec804f3dbf0d986f8e10850bfff14f316d7aaf config: i386-randconfig-s031-20200921 (attached as .config) compiler: gcc-9 (Debian 9.3.0-15) 9.3.0 reproduce: # apt-get install sparse # sparse version: v0.6.2-201-g24bdaac6-dirty # save the attached .config to linux build tree make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386 If you fix the issue, kindly add following tag as appropriate Reported-by: kernel test robot sparse warnings: (new ones prefixed by >>) >> drivers/net/bonding/bond_procfs.c:11:12: sparse: sparse: symbol 'linkdesc' >> was not declared. Should it be static? Please review and possibly fold the followup patch. --- 0-DAY CI Kernel Test Service, Intel Corporation https://lists.01.org/hyperkitty/list/kbuild-...@lists.01.org .config.gz Description: application/gzip
Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
On Tue, 22 Sep 2020 16:47:07 -0700 Jay Vosburgh wrote: > Stephen Hemminger wrote: > > >On Tue, 22 Sep 2020 09:37:30 -0400 > >Jarod Wilson 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 > >> Cc: Veaceslav Falico > >> Cc: Andy Gospodarek > >> Cc: "David S. Miller" > >> Cc: Jakub Kicinski > >> Cc: Thomas Davis > >> Cc: net...@vger.kernel.org > >> Signed-off-by: Jarod Wilson > > > >Why not just have a config option to remove all the /proc and sysfs options > >in bonding (and bridging) and only use netlink? New tools should be only able > >to use netlink only. > > I agree that new tooling should be netlink, but what value is > provided by such an option that distros are unlikely to enable, and > enabling will break the UAPI? > > >Then you might convince maintainers to update documentation as well. > >Last I checked there were still references to ifenslave. > > Distros still include ifenslave, but it's now a shell script > that uses sysfs. I see it used in scripts from time to time. Some bleeding edge distros have already dropped ifenslave and even ifconfig. The Enterprise ones never will. The one motivation would be for the embedded folks which are always looking to trim out the fat. Although not sure if the minimal versions of commands in busybox are pure netlink yet.
Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
Stephen Hemminger wrote: >On Tue, 22 Sep 2020 09:37:30 -0400 >Jarod Wilson 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 >> Cc: Veaceslav Falico >> Cc: Andy Gospodarek >> Cc: "David S. Miller" >> Cc: Jakub Kicinski >> Cc: Thomas Davis >> Cc: net...@vger.kernel.org >> Signed-off-by: Jarod Wilson > >Why not just have a config option to remove all the /proc and sysfs options >in bonding (and bridging) and only use netlink? New tools should be only able >to use netlink only. I agree that new tooling should be netlink, but what value is provided by such an option that distros are unlikely to enable, and enabling will break the UAPI? >Then you might convince maintainers to update documentation as well. >Last I checked there were still references to ifenslave. Distros still include ifenslave, but it's now a shell script that uses sysfs. I see it used in scripts from time to time. -J --- -Jay Vosburgh, jay.vosbu...@canonical.com
Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
On Tue, 22 Sep 2020 09:37:30 -0400 Jarod Wilson 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 > Cc: Veaceslav Falico > Cc: Andy Gospodarek > Cc: "David S. Miller" > Cc: Jakub Kicinski > Cc: Thomas Davis > Cc: net...@vger.kernel.org > Signed-off-by: Jarod Wilson Why not just have a config option to remove all the /proc and sysfs options in bonding (and bridging) and only use netlink? New tools should be only able to use netlink only. Then you might convince maintainers to update documentation as well. Last I checked there were still references to ifenslave.
Re: [PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
On Tue, Sep 22, 2020 at 9:38 AM Jarod Wilson 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. > diff --git a/drivers/net/bonding/bond_procfs.c > b/drivers/net/bonding/bond_procfs.c > index abd265d6e975..91ece68607b2 100644 > --- a/drivers/net/bonding/bond_procfs.c > +++ b/drivers/net/bonding/bond_procfs.c > @@ -7,6 +7,12 @@ > > #include "bonding_priv.h" > > +#ifdef CONFIG_BONDING_LEGACY_INTERFACES > +const char *linkdesc = "Slave"; > +#else > +const char *linkdesc = "Link"; > +#endif I've been asked if it would be okay to add extra lines to the /proc/net/bonding/ output, so that for example, both "Slave Interface: " and "Link Interface: " are both in the default output, with the Slave bits then suppressed by the Kconfig option being unset, versus the Kconfig option currently swapping out Slave for Link when disabled. It would bloat the output by a fair number of lines, but all the same data would be there and parseable. Wasn't sure on this one, so I wanted to check on it. If it would be acceptable, I'll rework that bit of code. -- Jarod Wilson ja...@redhat.com
[PATCH net-next 4/5] bonding: make Kconfig toggle to disable legacy interfaces
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
Cc: Veaceslav Falico
Cc: Andy Gospodarek
Cc: "David S. Miller"
Cc: Jakub Kicinski
Cc: Thomas Davis
Cc: net...@vger.kernel.org
Signed-off-by: Jarod Wilson
---
drivers/net/Kconfig | 12
drivers/net/bonding/bond_options.c| 4 ++--
drivers/net/bonding/bond_procfs.c | 14 ++
drivers/net/bonding/bond_sysfs.c | 15 ++-
drivers/net/bonding/bond_sysfs_link.c | 12
include/net/bond_options.h| 4 ++--
include/net/bonding.h | 2 ++
7 files changed, 46 insertions(+), 17 deletions(-)
diff --git a/drivers/net/Kconfig b/drivers/net/Kconfig
index c3dbe64e628e..3640694be34d 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 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 "aggregator" and "link" 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_options.c
b/drivers/net/bonding/bond_options.c
index 437df9a207a6..7bf1a13a3c17 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 interface names */
+#ifdef CONFIG_BONDING_LEGACY_INTERFACES
[BOND_OPT_PACKETS_PER_SLAVE] = {
.id = BOND_OPT_PACKETS_PER_SLAVE,
.name = "packets_per_slave",
@@ -474,7 +474,7 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
.flags = BOND_OPTFLAG_RAWVAL,
.set = bond_option_links_set
},
-/* end legacy sysfs interface names */
+#endif
};
/* Searches for an option by name */
diff --git a/drivers/net/bonding/bond_procfs.c
b/drivers/net/bonding/bond_procfs.c
index abd265d6e975..91ece68607b2 100644
--- a/drivers/net/bonding/bond_procfs.c
+++ b/drivers/net/bonding/bond_procfs.c
@@ -7,6 +7,12 @@
#include "bonding_priv.h"
+#ifdef CONFIG_BONDING_LEGACY_INTERFACES
+const char *linkdesc = "Slave";
+#else
+const char *linkdesc = "Link";
+#endif
+
static void *bond_info_seq_start(struct seq_file *seq, loff_t *pos)
__acquires(RCU)
{
@@ -84,7 +90,7 @@ static void bond_info_show_aggregator(struct seq_file *seq)
if (bond_uses_primary(bond)) {
primary = rcu_dereference(bond->primary_link);
- seq_printf(seq, "Primary Slave: %s",
+ seq_printf(seq, "Primary %s: %s", linkdesc,
primary ? primary->dev->name : "None");
if (primary) {
optval = bond_opt_get_val(BOND_OPT_PRIMARY_RESELECT,
@@ -93,7 +99,7 @@ static void bond_info_show_aggregator(struct seq_file *seq)
optval->string);
}
- seq_printf(seq, "\nCurrently Active Slave: %s\n",
+ seq_printf(seq, "\nCurrently Active %s: %s\n", linkdesc,
(curr) ? curr->dev->name : "None");
}
@@ -171,7 +177,7 @@ static void bond_info_show_link(struct seq_file *seq,
{
struct bonding *bond = PDE_DATA(file_inode(seq->file));
- seq_printf(seq, "\nSlave Interface: %s\n", link->dev->name);
+ seq_printf(seq, "\n%s Interface: %s\n", linkdesc, link->dev->name);
seq_printf(seq, "MII Status: %s\n",
bond_link_status(link->link_state));
if (link->speed == SPEED_UNKNOWN)
@@ -189,7 +195,7 @@ static void bond_info_show_link(struct seq_file *seq,
seq_printf(seq, "Permanent HW addr: %*phC\n",
link->dev->addr_len, link->perm_hwaddr);
- seq_printf(seq, "Slave queue ID: %d\n", link->queue_id);
+ seq_printf(seq, "%s queue ID: %d\n", linkdesc, link->queue_id);
if (BOND_MODE(bond) == BOND_MODE_8023AD) {
const struct port *port = _AD_INFO(link)->port;
diff --git a/drivers/net/bonding/bond_sysfs.c b/drivers/net/bonding/bond_sysfs.c
index 0a4d095b8c3d..9065f24e31c0 100644
---
