| Message ID | 1312315615-5739-1-git-send-email-nicolas.2p.debian@free.fr |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
Le 02/08/2011 22:06, Nicolas de Pesloüan a écrit : > Commit 655f8919d549ad1872e24d826b6ce42530516d2e > bonding: add min links parameter to 802.3ad > > and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f > bonding: add all_slaves_active parameter > > introduced new options to bonding, but didn't provide the documentation > for those options. > > Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr> > --- Jay, Andy, While working at this patch, I noticed that the bonding driver version wasn't bumped up at the time those options were introduced. I thought introducing a new option should cause the driver version to change. Am I right? On a different but related topic, the version in Documentation/networking/ifenslave.c (1.1.0) didn't change since the git origin and probably since 2003. Arguably, none of the commit regarding this file introduced a significant change (with the possible exception of commit e6d184e33109010412ad1d59719af74755a935f4, [NET]: Fix ifenslave to not fail on lack of IP information). But if we never change a 3-level version number, whatever the level of change, this version number might be useless. Any comment? Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Nicolas de Pesloüan <nicolas.2p.debian@free.fr> Date: Tue, 2 Aug 2011 22:06:55 +0200 > Commit 655f8919d549ad1872e24d826b6ce42530516d2e > bonding: add min links parameter to 802.3ad > > and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f > bonding: add all_slaves_active parameter > > introduced new options to bonding, but didn't provide the documentation > for those options. > > Signed-off-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr> Please explicitly mention in each new entry what the default setting is. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 3, 2011 at 2:43 AM, Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote: > Le 02/08/2011 22:06, Nicolas de Pesloüan a écrit : >> >> Commit 655f8919d549ad1872e24d826b6ce42530516d2e >> bonding: add min links parameter to 802.3ad >> >> and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f >> bonding: add all_slaves_active parameter >> >> introduced new options to bonding, but didn't provide the documentation >> for those options. >> >> Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr> >> --- > > Jay, Andy, > > While working at this patch, I noticed that the bonding driver version > wasn't bumped up at the time those options were introduced. > > I thought introducing a new option should cause the driver version to > change. Am I right? When a significant change happens, we try to change the version number. The version number probably should have been changed when those were added. Inspecting the module options or sysfs parameters indicate whether or not these patches were added, so it is less of a priority than when some internal infrastructure (like moving to use rx_handler) changes. I consider it more critical to change the bonding module version when something changes that cannot be detected by inspecting the module or sysfs parameters. This is more helpful to users reporting problems. > On a different but related topic, the version in > Documentation/networking/ifenslave.c (1.1.0) didn't change since the git > origin and probably since 2003. > > Arguably, none of the commit regarding this file introduced a significant > change (with the possible exception of commit > e6d184e33109010412ad1d59719af74755a935f4, [NET]: Fix ifenslave to not fail > on lack of IP information). But if we never change a 3-level version number, > whatever the level of change, this version number might be useless. Any > comment? > > Nicolas. > Distributions benefit from version numbers on userspace utils. It would probably be better to keep ifenslave's version number as it is to help those maintaining those distro packages. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 03/08/2011 21:03, Andy Gospodarek a écrit : >> I thought introducing a new option should cause the driver version to >> change. Am I right? > > When a significant change happens, we try to change the version > number. The version number probably should have been changed when > those were added. Inspecting the module options or sysfs parameters > indicate whether or not these patches were added, so it is less of a > priority than when some internal infrastructure (like moving to use > rx_handler) changes. > > I consider it more critical to change the bonding module version when > something changes that cannot be detected by inspecting the module or > sysfs parameters. This is more helpful to users reporting problems. > Ok, 'sounds perfectly sensible to me, thanks. >> On a different but related topic, the version in >> Documentation/networking/ifenslave.c (1.1.0) didn't change since the git >> origin and probably since 2003. >> >> Arguably, none of the commit regarding this file introduced a significant >> change (with the possible exception of commit >> e6d184e33109010412ad1d59719af74755a935f4, [NET]: Fix ifenslave to not fail >> on lack of IP information). But if we never change a 3-level version number, >> whatever the level of change, this version number might be useless. Any >> comment? >> >> Nicolas. >> > > Distributions benefit from version numbers on userspace utils. It > would probably be better to keep ifenslave's version number as it is > to help those maintaining those distro packages. As one of the maintainers for the ifenslave package on Debian, I perfectly understand the need for an upstream version, but as such, I expected the upstream version number to change when the file change... Version numbers in Debian use upstream version numbers when available and add a subversion number for Debian specific changes. I would expect to change the version number and not only the Debian subversion when the only change is a new upstream version. Anyway, it is not that important and I can leave with 1.1.0 for long :-D Thanks again. Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 03/08/2011 12:44, David Miller a écrit : > From: Nicolas de Pesloüan<nicolas.2p.debian@free.fr> > Date: Tue, 2 Aug 2011 22:06:55 +0200 > >> Commit 655f8919d549ad1872e24d826b6ce42530516d2e >> bonding: add min links parameter to 802.3ad >> >> and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f >> bonding: add all_slaves_active parameter >> >> introduced new options to bonding, but didn't provide the documentation >> for those options. >> >> Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr> > > Please explicitly mention in each new entry what the default > setting is. Unfortunately, I failed to find a place in the bonding code where the max_links option is initialized with a default value. So I must assume default value is zero which should cause carrier to always be asserted, or undefined, which should cause interesting side effects... The obvious default value should be 1, but I cannot confirm it is. Stephen, as the author of this feature, can you please clarify what the default value for min_links is? V2 will follow, giving the real default value for all_slaves_active and what I consider the sensible default value for max_links, even if the technical real default value is currently unclear. Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote: >Le 03/08/2011 21:03, Andy Gospodarek a écrit : > >>> I thought introducing a new option should cause the driver version to >>> change. Am I right? >> >> When a significant change happens, we try to change the version >> number. The version number probably should have been changed when >> those were added. Inspecting the module options or sysfs parameters >> indicate whether or not these patches were added, so it is less of a >> priority than when some internal infrastructure (like moving to use >> rx_handler) changes. >> >> I consider it more critical to change the bonding module version when >> something changes that cannot be detected by inspecting the module or >> sysfs parameters. This is more helpful to users reporting problems. >> > >Ok, 'sounds perfectly sensible to me, thanks. > >>> On a different but related topic, the version in >>> Documentation/networking/ifenslave.c (1.1.0) didn't change since the git >>> origin and probably since 2003. >>> >>> Arguably, none of the commit regarding this file introduced a significant >>> change (with the possible exception of commit >>> e6d184e33109010412ad1d59719af74755a935f4, [NET]: Fix ifenslave to not fail >>> on lack of IP information). But if we never change a 3-level version number, >>> whatever the level of change, this version number might be useless. Any >>> comment? >>> >>> Nicolas. >>> >> >> Distributions benefit from version numbers on userspace utils. It >> would probably be better to keep ifenslave's version number as it is >> to help those maintaining those distro packages. > >As one of the maintainers for the ifenslave package on Debian, I perfectly >understand the need for an upstream version, but as such, I expected the >upstream version number to change when the file change... Version numbers >in Debian use upstream version numbers when available and add a subversion >number for Debian specific changes. I would expect to change the version >number and not only the Debian subversion when the only change is a new >upstream version. One thing to remember here is that currently very few (perhaps no) distros use the ifenslave.c that comes with mainline. The distros I'm familiar with configure bonding via sysfs, either directly in initscripts / sysconfig, or via a shell script ifenslave (which I believe is what Debian has). Many distros still install it in /sbin/ifenslave, but it isn't used by the network configuration stuff. The ifenslave.c in mainline is pretty much just a legacy for backwards compatibility; it has not had a bug fix since 2005 (a few typo repairs since then), and no major functional changes since before the git era. I was considering proposing feature removal for ifenslave.c and the ioctl API to add and remove slaves, but some discussion a few months ago indicated that there are apparently still some users out there (I'd guess embedded of some variety). -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 03/08/2011 22:07, Jay Vosburgh a écrit : > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote: >> Le 03/08/2011 21:03, Andy Gospodarek a écrit : >>> Distributions benefit from version numbers on userspace utils. It >>> would probably be better to keep ifenslave's version number as it is >>> to help those maintaining those distro packages. >> >> As one of the maintainers for the ifenslave package on Debian, I perfectly >> understand the need for an upstream version, but as such, I expected the >> upstream version number to change when the file change... Version numbers >> in Debian use upstream version numbers when available and add a subversion >> number for Debian specific changes. I would expect to change the version >> number and not only the Debian subversion when the only change is a new >> upstream version. > > One thing to remember here is that currently very few (perhaps > no) distros use the ifenslave.c that comes with mainline. The distros > I'm familiar with configure bonding via sysfs, either directly in > initscripts / sysconfig, or via a shell script ifenslave (which I > believe is what Debian has). Many distros still install it in > /sbin/ifenslave, but it isn't used by the network configuration stuff. The ifenslave package on Debian provide two things: - The binary ifenslave, simply compiled from mainline ifenslave.c. - A plug-in for ifupdown to allow for bonding related options in /etc/network/interfaces. I can confirm that this plug-in doesn't use the ifenslave command, but sysfs, since version 1.1.0-12 (current stable version of this package is 1.1.0-17). > The ifenslave.c in mainline is pretty much just a legacy for > backwards compatibility; it has not had a bug fix since 2005 (a few typo > repairs since then), and no major functional changes since before the > git era. > > I was considering proposing feature removal for ifenslave.c and > the ioctl API to add and remove slaves, but some discussion a few months > ago indicated that there are apparently still some users out there (I'd > guess embedded of some variety). Unfortunately, there exist *many* how-to that suggest to use ifenslave, causing many users to use it instead of sysfs for bonding setup. At least, we can: - update the bonding documentation to clarify that ifenslave is deprecated (and move most ifenslave related stuffs at the end of the documentation); - possibly issue a warning when the API is used, suggesting to use sysfs instead. I can take care of the documentation update if appropriate. Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote: >Le 03/08/2011 12:44, David Miller a écrit : >> From: Nicolas de Pesloüan<nicolas.2p.debian@free.fr> >> Date: Tue, 2 Aug 2011 22:06:55 +0200 >> >>> Commit 655f8919d549ad1872e24d826b6ce42530516d2e >>> bonding: add min links parameter to 802.3ad >>> >>> and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f >>> bonding: add all_slaves_active parameter >>> >>> introduced new options to bonding, but didn't provide the documentation >>> for those options. >>> >>> Signed-off-by: Nicolas de Pesloüan<nicolas.2p.debian@free.fr> >> >> Please explicitly mention in each new entry what the default >> setting is. > >Unfortunately, I failed to find a place in the bonding code where the >max_links option is initialized with a default value. So I must assume >default value is zero which should cause carrier to always be asserted, or >undefined, which should cause interesting side effects... > >The obvious default value should be 1, but I cannot confirm it is. Looking at it now, I see no initialization, and it's a static, so I believe it will end up being zero. From the code, zero seems like the proper default, since it will make this test never pass: /* are enough slaves available to consider link up? */ if (active->num_of_ports < bond->params.min_links) { if (netif_carrier_ok(bond->dev)) { netif_carrier_off(bond->dev); return 1; } This will cause carrier to be asserted (for 802.3ad mode) whenever there is an active aggregator, regardless of the number of available links in that aggregator. >Stephen, as the author of this feature, can you please clarify what the default value for min_links is? > >V2 will follow, giving the real default value for all_slaves_active and >what I consider the sensible default value for max_links, even if the >technical real default value is currently unclear. I think the actual and sensible default are both zero, although the documentation should probably mention that a value of zero is magic and won't ever set the bond down due to too few ports (links) active. Or, perhaps describe it how it actually works: if there are fewer than "min_links" ports in the active aggregator, the bond is set carrier down. The default min_links value of zero means that the bond will never be set down due to having too few ports active. -J --- -Jay Vosburgh, IBM Linux Technology Center, fubar@us.ibm.com -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 03 Aug 2011 22:38:03 +0200 Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote: > Le 03/08/2011 22:07, Jay Vosburgh a écrit : > > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote: > >> Le 03/08/2011 21:03, Andy Gospodarek a écrit : > > >>> Distributions benefit from version numbers on userspace utils. It > >>> would probably be better to keep ifenslave's version number as it is > >>> to help those maintaining those distro packages. > >> > >> As one of the maintainers for the ifenslave package on Debian, I perfectly > >> understand the need for an upstream version, but as such, I expected the > >> upstream version number to change when the file change... Version numbers > >> in Debian use upstream version numbers when available and add a subversion > >> number for Debian specific changes. I would expect to change the version > >> number and not only the Debian subversion when the only change is a new > >> upstream version. > > > > One thing to remember here is that currently very few (perhaps > > no) distros use the ifenslave.c that comes with mainline. The distros > > I'm familiar with configure bonding via sysfs, either directly in > > initscripts / sysconfig, or via a shell script ifenslave (which I > > believe is what Debian has). Many distros still install it in > > /sbin/ifenslave, but it isn't used by the network configuration stuff. > > The ifenslave package on Debian provide two things: > > - The binary ifenslave, simply compiled from mainline ifenslave.c. > - A plug-in for ifupdown to allow for bonding related options in /etc/network/interfaces. I can > confirm that this plug-in doesn't use the ifenslave command, but sysfs, since version 1.1.0-12 > (current stable version of this package is 1.1.0-17). > > > The ifenslave.c in mainline is pretty much just a legacy for > > backwards compatibility; it has not had a bug fix since 2005 (a few typo > > repairs since then), and no major functional changes since before the > > git era. > > > > I was considering proposing feature removal for ifenslave.c and > > the ioctl API to add and remove slaves, but some discussion a few months > > ago indicated that there are apparently still some users out there (I'd > > guess embedded of some variety). > > Unfortunately, there exist *many* how-to that suggest to use ifenslave, causing many users to use it > instead of sysfs for bonding setup. > > At least, we can: > - update the bonding documentation to clarify that ifenslave is deprecated (and move most ifenslave > related stuffs at the end of the documentation); > - possibly issue a warning when the API is used, suggesting to use sysfs instead. > > I can take care of the documentation update if appropriate. > > Nicolas. Or I could just put a shell script that does the same thing in iproute. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 03/08/2011 23:33, Stephen Hemminger a écrit : > On Wed, 03 Aug 2011 22:38:03 +0200 > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote: <snip> >> At least, we can: >> - update the bonding documentation to clarify that ifenslave is deprecated (and move most ifenslave >> related stuffs at the end of the documentation); >> - possibly issue a warning when the API is used, suggesting to use sysfs instead. >> >> I can take care of the documentation update if appropriate. >> >> Nicolas. > > Or I could just put a shell script that does the same thing in iproute. You mean, to replace ifenslave (from ifenslave.c) by a shell script by the same name that gives users the same command-line interface but use sysfs instead of the ioctl API internally? I had this in mind two years ago, in a private conversation while changing the ifenslave package on Debian. But chance exist that those who expect ifenslave to work would like to disable sysfs support if not used. I assume that totally disabling sysfs at compile time would reduce the size of the kernel far more that disabling the ioctl API. So, for those doing embedded things, this may makes sens to have an ifenslave version that doesn't rely on sysfs. Or do you mean, to replace ifenslave (.c) by a shell script that use netlink internally? Are all the bonding options available through netlink? I remember many conversation about using netlink for bonding setup, but don't remember seeing that in the actual code. Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 03/08/2011 22:59, Jay Vosburgh a écrit : > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote: <snip> >> The obvious default value should be 1, but I cannot confirm it is. > > Looking at it now, I see no initialization, and it's a static, > so I believe it will end up being zero. From the code, zero seems like > the proper default, since it will make this test never pass: > > /* are enough slaves available to consider link up? */ > if (active->num_of_ports< bond->params.min_links) { > if (netif_carrier_ok(bond->dev)) { > netif_carrier_off(bond->dev); > return 1; > } > > This will cause carrier to be asserted (for 802.3ad mode) > whenever there is an active aggregator, regardless of the number of > available links in that aggregator. > >> Stephen, as the author of this feature, can you please clarify what the default value for min_links is? >> >> V2 will follow, giving the real default value for all_slaves_active and >> what I consider the sensible default value for max_links, even if the >> technical real default value is currently unclear. > > I think the actual and sensible default are both zero, although > the documentation should probably mention that a value of zero is magic > and won't ever set the bond down due to too few ports (links) active. > > Or, perhaps describe it how it actually works: if there are > fewer than "min_links" ports in the active aggregator, the bond is set > carrier down. The default min_links value of zero means that the bond > will never be set down due to having too few ports active. Well, you are right, but for as far as I understand, using 1 as the default value for min_links would cause the exact same behavior. And 1 would be less "magical" and as such, more understandable from a user point of view. User might understand min_links=0 as "assert carrier on if at least 0 link have carrier on", which might be understood as "always assert carrier on". Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 04 Aug 2011 07:31:38 +0200 Nicolas de Pesloüan <nicolas.2p.debian@gmail.com> wrote: > Le 03/08/2011 23:33, Stephen Hemminger a écrit : > > On Wed, 03 Aug 2011 22:38:03 +0200 > > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote: > <snip> > >> At least, we can: > >> - update the bonding documentation to clarify that ifenslave is deprecated (and move most ifenslave > >> related stuffs at the end of the documentation); > >> - possibly issue a warning when the API is used, suggesting to use sysfs instead. > >> > >> I can take care of the documentation update if appropriate. > >> > >> Nicolas. > > > > Or I could just put a shell script that does the same thing in iproute. > > You mean, to replace ifenslave (from ifenslave.c) by a shell script by the same name that gives > users the same command-line interface but use sysfs instead of the ioctl API internally? > > I had this in mind two years ago, in a private conversation while changing the ifenslave package on > Debian. > > But chance exist that those who expect ifenslave to work would like to disable sysfs support if not > used. I assume that totally disabling sysfs at compile time would reduce the size of the kernel far > more that disabling the ioctl API. So, for those doing embedded things, this may makes sens to have > an ifenslave version that doesn't rely on sysfs. > > Or do you mean, to replace ifenslave (.c) by a shell script that use netlink internally? Are all the > bonding options available through netlink? I remember many conversation about using netlink for > bonding setup, but don't remember seeing that in the actual code. The sysfs interface is the least desirable. The ifenslave script would just call 'ip link' -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Le 04/08/2011 18:57, Stephen Hemminger a écrit : > On Thu, 04 Aug 2011 07:31:38 +0200 > Nicolas de Pesloüan<nicolas.2p.debian@gmail.com> wrote: <snip> >> Or do you mean, to replace ifenslave (.c) by a shell script that use netlink internally? Are all the >> bonding options available through netlink? I remember many conversation about using netlink for >> bonding setup, but don't remember seeing that in the actual code. > > The sysfs interface is the least desirable. > The ifenslave script would just call 'ip link' For as far as I know, ip link is able to enslave and to detach a slave, but not to change the active slave. So I think you cannot mimic the -c|--change-active option of ifenslave just using iproute. Do I miss something? That being said, I'm not sure this particular option is the most used one, but... Nicolas. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/networking/bonding.txt b/Documentation/networking/bonding.txt index 675612f..058ddea 100644 --- a/Documentation/networking/bonding.txt +++ b/Documentation/networking/bonding.txt @@ -238,6 +238,15 @@ ad_select This option was added in bonding version 3.4.0. +all_slaves_active + + Specifies that duplicate frames (received on inactive ports) should be + dropped (0) or delivered (1). + + Normally, bonding will drop duplicate frames (received on inactive + ports), which is desirable for most users. But there are some times + it is nice to allow duplicate frames to be delivered. + arp_interval Specifies the ARP link monitoring frequency in milliseconds. @@ -433,6 +442,18 @@ miimon determined. See the High Availability section for additional information. The default value is 0. +min_links + + Specifies the minimum number of links that must be active before + asserting carrier. It is similar to the Cisco EtherChannel min-links + feature. This allows setting the minimum number of member ports that + must be up (link-up state) before marking the bond device as up + (carrier on). This is useful for situations where higher level services + such as clustering want to ensure a minimum number of low bandwidth + links are active before switchover. + + This only affect 802.3ad mode. + mode Specifies one of the bonding policies. The default is
Commit 655f8919d549ad1872e24d826b6ce42530516d2e bonding: add min links parameter to 802.3ad and commit ebd8e4977a87cb81d93c62a9bff0102a9713722f bonding: add all_slaves_active parameter introduced new options to bonding, but didn't provide the documentation for those options. Signed-off-by: Nicolas de Pesloüan <nicolas.2p.debian@free.fr> --- Documentation/networking/bonding.txt | 21 +++++++++++++++++++++ 1 files changed, 21 insertions(+), 0 deletions(-)