| Message ID | 20200922133731.33478-1-jarod@redhat.com |
|---|---|
| Headers | show |
| Series | bonding: rename bond components | expand |
Jarod Wilson <jarod@redhat.com> wrote: >The bonding driver's use of master and slave, while largely understood >in technical circles, poses a barrier for inclusion to some potential >members of the development and user community, due to the historical >context of masters and slaves, particularly in the United States. This >is a first full pass at replacing those phrases with more socially >inclusive ones, opting for aggregator to replace master and link to >replace slave, as the bonding driver itself is a link aggregation >driver. First, I think there should be some direction from the kernel development leadership as to whether or not this type of large-scale search and replace of socially sensitive terms of art or other terminology is a task that should be undertaken, given the noted issues it will cause in stable release maintenance going forwards. Second, on the merits of the proposed changes (presuming for the moment that this goes forward), I would prefer different nomenclature that does not reuse existing names for different purposes, i.e., "link" and "aggregator." Both of those have specific meanings in the current code, and old kernels will retain that meaning. Changing them to have new meanings going forward will lead to confusion, in my opinion for no good reason, as there are other names suited that do not conflict. For example, instead of "master" call everything a "bond," which matches common usage in discussion. Changing "master" to "aggregator," the replacement results in cumbersome descriptions like "the aggregator's active aggregator" in the context of LACP. A replacement term for "slave" is trickier; my first choice would be "port," but that may make more churn from a code change point of view, although struct slave could become struct bond_port, and leave the existing struct port for its current LACP use. >There are a few problems with this change. First up, "link" is used for >link state already in the bonding driver, so the first step here is to >rename link to link_state. Second, aggregator is already used in the >802.3ad code, but I feel the usage is actually consistent with referring >to the bonding aggregation virtual device as the aggregator. Third, we >have the issue of not wanting to break any existing userspace, which I >believe this patchset accomplishes, while also adding alternative >interfaces using new terminology, and a Kconfig option that will let >people make the conscious decision to break userspace and no longer >expose the original master/slave interfaces, once their userspace is >able to cope with their removal. I'm opposed to the Kconfig option because it will lead to balkanization of the UAPI, which would be worse than a clean break (which I'm also opposed to). >Lastly, we do still have the issue of ease of backporting fixes to >-stable trees. I've not had a huge amount of time to spend on it, but >brief forays into coccinelle didn't really pay off (since it's meant to >operate on code, not patches), and the best solution I can come up with >is providing a shell script someone could run over git-format-patch >output before git-am'ing the result to a -stable tree, though scripting >these changes in the first place turned out to be not the best thing to >do anyway, due to subtle cases where use of master or slave can NOT yet >be replaced, so a large amount of work was done by hand, inspection, >trial and error, which is why this set is a lot longer in coming than >I'd originally hoped. I don't expect -stable backports to be horrible to >figure out one way or another though, and I don't believe that a bit of >inconvenience on that front is enough to warrant not making these >changes. I'm skeptical that, given the scope of the changes involved, that it's really feasible to have effective automated massaging of patches. I think the reality is that a large fraction of the bonding fixes in the future will have to be backported entirely by hand. The only saving grace here is that the quantity of such patches is generally low (~40 in 2020 year to date). -J >See here for further details on Red Hat's commitment to this work: >https://www.redhat.com/en/blog/making-open-source-more-inclusive-eradicating-problematic-language > >As far as testing goes, I've manually operated on various bonds while >working on this code, and have run it through multiple lnst test runs, >which exercises the existing sysfs interfaces fairly extensively. As far >as I can tell, there is no breakage of existing interfaces with this >set, unless the user consciously opts to do so via Kconfig. > >Jarod Wilson (5): > bonding: rename struct slave member link to link_state > bonding: rename slave to link where possible > bonding: rename master to aggregator where possible > bonding: make Kconfig toggle to disable legacy interfaces > bonding: update Documentation for link/aggregator terminology > >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 > > .clang-format | 4 +- > Documentation/networking/bonding.rst | 440 ++-- > drivers/infiniband/core/cma.c | 2 +- > drivers/infiniband/core/lag.c | 2 +- > drivers/infiniband/core/roce_gid_mgmt.c | 10 +- > drivers/infiniband/hw/mlx4/main.c | 2 +- > drivers/net/Kconfig | 12 + > drivers/net/bonding/Makefile | 2 +- > drivers/net/bonding/bond_3ad.c | 604 ++--- > drivers/net/bonding/bond_alb.c | 687 ++--- > drivers/net/bonding/bond_debugfs.c | 2 +- > drivers/net/bonding/bond_main.c | 2336 +++++++++-------- > drivers/net/bonding/bond_netlink.c | 104 +- > drivers/net/bonding/bond_options.c | 258 +- > drivers/net/bonding/bond_procfs.c | 63 +- > drivers/net/bonding/bond_sysfs.c | 249 +- > drivers/net/bonding/bond_sysfs_link.c | 193 ++ > drivers/net/bonding/bond_sysfs_slave.c | 176 -- > .../ethernet/chelsio/cxgb3/cxgb3_offload.c | 2 +- > .../net/ethernet/mellanox/mlx4/en_netdev.c | 4 +- > .../ethernet/mellanox/mlx5/core/en/rep/bond.c | 2 +- > .../net/ethernet/mellanox/mlx5/core/en_tc.c | 2 +- > .../ethernet/netronome/nfp/flower/lag_conf.c | 2 +- > .../ethernet/qlogic/netxen/netxen_nic_main.c | 12 +- > include/linux/netdevice.h | 20 +- > include/net/bond_3ad.h | 20 +- > include/net/bond_alb.h | 31 +- > include/net/bond_options.h | 19 +- > include/net/bonding.h | 351 +-- > include/net/lag.h | 2 +- > 30 files changed, 2902 insertions(+), 2711 deletions(-) > create mode 100644 drivers/net/bonding/bond_sysfs_link.c > delete mode 100644 drivers/net/bonding/bond_sysfs_slave.c > >-- >2.27.0 > --- -Jay Vosburgh, jay.vosburgh@canonical.com
On Tue, Sep 22, 2020 at 6:19 PM Jay Vosburgh <jay.vosburgh@canonical.com> wrote: > > Jarod Wilson <jarod@redhat.com> wrote: > > >The bonding driver's use of master and slave, while largely understood > >in technical circles, poses a barrier for inclusion to some potential > >members of the development and user community, due to the historical > >context of masters and slaves, particularly in the United States. This > >is a first full pass at replacing those phrases with more socially > >inclusive ones, opting for aggregator to replace master and link to > >replace slave, as the bonding driver itself is a link aggregation > >driver. > > First, I think there should be some direction from the kernel > development leadership as to whether or not this type of large-scale > search and replace of socially sensitive terms of art or other > terminology is a task that should be undertaken, given the noted issues > it will cause in stable release maintenance going forwards. Admittedly, part of the point of this patch is to help drive such conversations. Having a concrete example of how these changes would look makes it easier to discuss, I think. I understand the burden here, though as you noted later, bonding doesn't really churn that much, so in this specific case, the maintenance load wouldn't be terrible, and I think worth it in this case, from a social standpoint. I know this can start to get political and personal real fast though... > Second, on the merits of the proposed changes (presuming for the > moment that this goes forward), I would prefer different nomenclature > that does not reuse existing names for different purposes, i.e., "link" > and "aggregator." > > Both of those have specific meanings in the current code, and > old kernels will retain that meaning. Changing them to have new > meanings going forward will lead to confusion, in my opinion for no good > reason, as there are other names suited that do not conflict. > > For example, instead of "master" call everything a "bond," which > matches common usage in discussion. Changing "master" to "aggregator," > the replacement results in cumbersome descriptions like "the > aggregator's active aggregator" in the context of LACP. > > A replacement term for "slave" is trickier; my first choice > would be "port," but that may make more churn from a code change point > of view, although struct slave could become struct bond_port, and leave > the existing struct port for its current LACP use. I did briefly have the idea of renaming 'port' in the LACP code to 'lacp_port', which would allow reuse of 'port', and would then be consistent with the team driver (and bridge driver, iirc). I could certainly pursue that option, or try going with "bond_port", but I'd like something so widely used throughout the code to be shorter if possible, I think. It really is LACP that throws a wrench into most sensible naming schemes I could think of. Simply renaming current "master" to "bond" does make a fair bit of sense though, didn't really occur to me. But replacing "slave" is definitely the far more involved and messy one. > >There are a few problems with this change. First up, "link" is used for > >link state already in the bonding driver, so the first step here is to > >rename link to link_state. Second, aggregator is already used in the > >802.3ad code, but I feel the usage is actually consistent with referring > >to the bonding aggregation virtual device as the aggregator. Third, we > >have the issue of not wanting to break any existing userspace, which I > >believe this patchset accomplishes, while also adding alternative > >interfaces using new terminology, and a Kconfig option that will let > >people make the conscious decision to break userspace and no longer > >expose the original master/slave interfaces, once their userspace is > >able to cope with their removal. > > I'm opposed to the Kconfig option because it will lead to > balkanization of the UAPI, which would be worse than a clean break > (which I'm also opposed to). I suspected this might be a point of contention. Easy enough to simply omit that bit from the series, if that's the consensus. > >Lastly, we do still have the issue of ease of backporting fixes to > >-stable trees. I've not had a huge amount of time to spend on it, but > >brief forays into coccinelle didn't really pay off (since it's meant to > >operate on code, not patches), and the best solution I can come up with > >is providing a shell script someone could run over git-format-patch > >output before git-am'ing the result to a -stable tree, though scripting > >these changes in the first place turned out to be not the best thing to > >do anyway, due to subtle cases where use of master or slave can NOT yet > >be replaced, so a large amount of work was done by hand, inspection, > >trial and error, which is why this set is a lot longer in coming than > >I'd originally hoped. I don't expect -stable backports to be horrible to > >figure out one way or another though, and I don't believe that a bit of > >inconvenience on that front is enough to warrant not making these > >changes. > > I'm skeptical that, given the scope of the changes involved, > that it's really feasible to have effective automated massaging of > patches. I think the reality is that a large fraction of the bonding > fixes in the future will have to be backported entirely by hand. The > only saving grace here is that the quantity of such patches is generally > low (~40 in 2020 year to date). Yeah, requiring manual backporting by hand is a very distinct possibility. As noted, such patches are usually pretty low in number, and I'll note that they're also generally fairly small patches too. I would be happy to help with any manual backporting as well, as a consolation if scripting them doesn't really work out.