Message-ID: <CAKfmpSd145TDcgi6t0+BFfXH2+4Q0J-UB6uA+bm4vfpDrgy1sA@mail.gmail.com>
Date:   Fri, 25 Sep 2020 08:13:24 -0400
From:   Jarod Wilson <jarod@...hat.com>
To:     Jay Vosburgh <jay.vosburgh@...onical.com>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Thomas Davis <tadavis@....gov>, Netdev <netdev@...r.kernel.org>
Subject: Re: [PATCH net-next 0/5] bonding: rename bond components

On Tue, Sep 22, 2020 at 6:19 PM Jay Vosburgh <jay.vosburgh@...onical.com> wrote:
>
> Jarod Wilson <jarod@...hat.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.

-- 
Jarod Wilson
jarod@...hat.com

Powered by blists - more mailing lists