| Message ID | 20220719235002.1944800-1-sean.anderson@seco.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | net: phy: Add support for rate adaptation | expand |
On Tue, Jul 19, 2022 at 07:49:50PM -0400, Sean Anderson wrote: > Second, to reduce packet loss it may be desirable to throttle packet > throughput. In past discussions [2-4], this behavior has been > controversial. It isn't controversial at all. It's something we need to support, but the point I've been making is that if we're adding rate adaption, then we need to do a better job when designing the infrastructure to cater for all currently known forms of rate adaption amongst the knowledge pool that we have, not just one. That's why I brought up the IPG-based method used by 88x3310. Phylink development is extremely difficult, and takes months or years for changes to get into mainline when updates to drivers are required - this is why I have a massive queue of changes all the time. > It is the opinion of several developers that it is the > responsibility of the system integrator or end user to set the link > settings appropriately for rate adaptation. In particular, it was argued > that it is difficult to determine whether a particular phy has rate > adaptation enabled, and it is simpler to keep such determinations out of > the kernel. I don't think I've ever said that... > Another criticism is that packet loss may happen anyway, such > as if a faster link is used with a switch or repeater that does not support > pause frames. That isn't what I've said. Packet loss may happen if (a) pause frames can not be sent by a PHY in rate adaption mode and (b) if the MAC can't pace its transmission for the media/line speed. This is a fundamental fact where a PHY will only have so much buffering capability, that if the MAC sends packets at a faster rate than the PHY can get them out, it runs out of buffer space. That isn't a criticism, it's a statement of fact. > I believe that our current approach is limiting, especially when > considering that rate adaptation (in two forms) has made it into IEEE > standards. In general, When we have appropriate information we should set > sensible defaults. To consider use a contrasting example, we enable pause > frames by default for switches which autonegotiate for them. When it's the > phy itself generating these frames, we don't even have to autonegotiate to > know that we should enable pause frames. I'm not sure I understand what you're saying, because it doesn't match what I've seen. "we enable pause frames by default for swithes which autonegotiate for them" - what are you talking about there? The "user" ports on the switch, or the DSA/CPU ports? It has been argued that pause frames should not be enabled for the CPU port, particularly when the CPU port runs at a slower speed than the switch - which happens e.g. on the VF610 platforms. Most CPU ports to switches I'm aware of are specified either using a fixed link in firmware or default to a fixed link both without pause frames. Maybe this is just a quirk of the mv88e6xxx setup. "when it's the phy itself generating these frames, we don't even have to autonegotiate to know that we should enable pause frames." I'm not sure that's got any relevance. When a PHY is in rate adapting mode, there are two separate things that are going on. There's the media side link negotiation and parameters, and then there's the requirements of the host-side link. The parameters of the host-side link do not need to be negotiated with the link partner, but they do potentially affect what link modes we can negotiate with our link partner (for example, if the PHY can't handle HD on the media side with the MAC operating FD). In any case, if the PHY requires the MAC to receive pause frames for its rate adaption to work, then this doesn't affect the media side autonegotiation at all. Hence, I don't understand this comment. > Note that > even when we determine (e.g.) the pause settings based on whether rate > adaptation is enabled, they can still be overridden by userspace (using > ethtool). It might be prudent to allow disabling of rate adaptation > generally in ethtool as well. This is no longer true as this patch set overrides whatever receive pause state has been negotiated or requested by userspace so that rate adaption can still work. The future work here is to work out whether we should disable rate adaption if userspace requests receive pause frames to be disabled, or whether switching to another form of controlling rate adaption would be appropriate and/or possible.
On 7/20/22 8:03 AM, Russell King (Oracle) wrote: > On Tue, Jul 19, 2022 at 07:49:50PM -0400, Sean Anderson wrote: >> Second, to reduce packet loss it may be desirable to throttle packet >> throughput. In past discussions [2-4], this behavior has been >> controversial. > > It isn't controversial at all. It's something we need to support, but > the point I've been making is that if we're adding rate adaption, then > we need to do a better job when designing the infrastructure to cater > for all currently known forms of rate adaption amongst the knowledge > pool that we have, not just one. That's why I brought up the IPG-based > method used by 88x3310. > > Phylink development is extremely difficult, and takes months or years > for changes to get into mainline when updates to drivers are required - > this is why I have a massive queue of changes all the time. > >> It is the opinion of several developers that it is the >> responsibility of the system integrator or end user to set the link >> settings appropriately for rate adaptation. In particular, it was argued >> that it is difficult to determine whether a particular phy has rate >> adaptation enabled, and it is simpler to keep such determinations out of >> the kernel. > > I don't think I've ever said that... You haven't. This mostly stems from https://lore.kernel.org/netdev/DB8PR04MB6985139D4ABED85B701445A9EC050@DB8PR04MB6985.eurprd04.prod.outlook.com/ where there was some discussion about whose responsibility it was to determine whether rate adaptation was supported. The implication being that we should delay support for rate adaptation until we could reliably determine whether it was supported. This of course is not quite implemented yet. While we can determine whether rate adaptation is actually in-use, I don't know if we can determine whether it is available before trying to bring the link up. >> Another criticism is that packet loss may happen anyway, such >> as if a faster link is used with a switch or repeater that does not support >> pause frames. > > That isn't what I've said. Packet loss may happen if (a) pause frames > can not be sent by a PHY in rate adaption mode and (b) if the MAC can't > pace its transmission for the media/line speed. This is a fundamental > fact where a PHY will only have so much buffering capability, that if > the MAC sends packets at a faster rate than the PHY can get them out, it > runs out of buffer space. That isn't a criticism, it's a statement of > fact. You're right. I mainly wanted to bring up what you just noted: that we may have packet loss anyway, and that higher-layer protocols already deal with packet loss. So a MAC unaware of the rate adaptation is not necessarily the worst thing. >> I believe that our current approach is limiting, especially when >> considering that rate adaptation (in two forms) has made it into IEEE >> standards. In general, When we have appropriate information we should set >> sensible defaults. To consider use a contrasting example, we enable pause >> frames by default for switches which autonegotiate for them. When it's the >> phy itself generating these frames, we don't even have to autonegotiate to >> know that we should enable pause frames. > > I'm not sure I understand what you're saying, because it doesn't match > what I've seen. > Sorry, I was unclear here. I meant link partners, not local (DSA) switches. > "we enable pause frames by default for swithes which autonegotiate for > them" - what are you talking about there? The "user" ports on the > switch, or the DSA/CPU ports? It has been argued that pause frames > should not be enabled for the CPU port, particularly when the CPU port > runs at a slower speed than the switch - which happens e.g. on the VF610 > platforms. > > Most CPU ports to switches I'm aware of are specified either using a > fixed link in firmware or default to a fixed link both without pause > frames. Maybe this is just a quirk of the mv88e6xxx setup. > > "when it's the phy itself generating these frames, we don't even have to > autonegotiate to know that we should enable pause frames." I'm not sure > that's got any relevance. When a PHY is in rate adapting mode, there are > two separate things that are going on. There's the media side link > negotiation and parameters, and then there's the requirements of the > host-side link. The parameters of the host-side link do not need to be > negotiated with the link partner, but they do potentially affect what > link modes we can negotiate with our link partner (for example, if the > PHY can't handle HD on the media side with the MAC operating FD). In any > case, if the PHY requires the MAC to receive pause frames for its rate > adaption to work, then this doesn't affect the media side > autonegotiation at all. Hence, I don't understand this comment. > >> Note that >> even when we determine (e.g.) the pause settings based on whether rate >> adaptation is enabled, they can still be overridden by userspace (using >> ethtool). It might be prudent to allow disabling of rate adaptation >> generally in ethtool as well. > > This is no longer true as this patch set overrides whatever receive > pause state has been negotiated or requested by userspace so that rate > adaption can still work. Right, I forgot to edit this. > The future work here is to work out whether we should disable rate > adaption if userspace requests receive pause frames to be disabled, or > whether switching to another form of controlling rate adaption would be > appropriate and/or possible. > I'm not sure what the best course here is either. --Sean
This adds support for phy rate adaptation: when a phy adapts between differing phy interface and link speeds. It was originally submitted as part of [1], which is considered "v1" of this series. We need support for rate adaptation for two reasons. First, the phy consumer needs to know if the phy will perform rate adaptation in order to program the correct advertising. An unaware consumer will only program support for link modes at the phy interface mode's native speed. This will cause autonegotiation to fail if the link partner only advertises support for lower speed link modes. Second, to reduce packet loss it may be desirable to throttle packet throughput. In past discussions [2-4], this behavior has been controversial. It is the opinion of several developers that it is the responsibility of the system integrator or end user to set the link settings appropriately for rate adaptation. In particular, it was argued that it is difficult to determine whether a particular phy has rate adaptation enabled, and it is simpler to keep such determinations out of the kernel. Another criticism is that packet loss may happen anyway, such as if a faster link is used with a switch or repeater that does not support pause frames. I believe that our current approach is limiting, especially when considering that rate adaptation (in two forms) has made it into IEEE standards. In general, When we have appropriate information we should set sensible defaults. To consider use a contrasting example, we enable pause frames by default for switches which autonegotiate for them. When it's the phy itself generating these frames, we don't even have to autonegotiate to know that we should enable pause frames. Our current approach also encourages workarounds, such as commit 73a21fa817f0 ("dpaa_eth: support all modes with rate adapting PHYs"). These workarounds are fine for phylib drivers, but phylink drivers cannot use this approach (since there is no direct access to the phy). Note that even when we determine (e.g.) the pause settings based on whether rate adaptation is enabled, they can still be overridden by userspace (using ethtool). It might be prudent to allow disabling of rate adaptation generally in ethtool as well. [1] https://lore.kernel.org/netdev/20220715215954.1449214-1-sean.anderson@seco.com/T/#t [2] https://lore.kernel.org/netdev/1579701573-6609-1-git-send-email-madalin.bucur@oss.nxp.com/ [3] https://lore.kernel.org/netdev/1580137671-22081-1-git-send-email-madalin.bucur@oss.nxp.com/ [4] https://lore.kernel.org/netdev/20200116181933.32765-1-olteanv@gmail.com/ Changes in v2: - Use int/defines instead of enum to allow for use in ioctls/netlink - Add locking to phy_get_rate_adaptation - Add (read-only) ethtool support for rate adaptation - Move part of commit message to cover letter, as it gives a good overview of the whole series, and allows this patch to focus more on the specifics. - Support keeping track of link duplex - Rewrite commit message for clarity - Expand documentation of (link_)?(speed|duplex) - Fix handling of speed/duplex gotten from MAC drivers - Use the phy's rate adaptation setting to determine whether to use its link speed/duplex or the MAC's speed/duplex with MLO_AN_INBAND. - Always use the rate adaptation setting to determine the interface speed/duplex (instead of sometimes using the interface mode). - Determine the interface speed and max mac speed directly instead of guessing based on the caps. - Add comments clarifying the register defines - Reorder variables in aqr107_read_rate Sean Anderson (11): net: dpaa: Fix <1G ethernet on LS1046ARDB net: phy: Add 1000BASE-KX interface mode net: phylink: Export phylink_caps_to_linkmodes net: phylink: Generate caps and convert to linkmodes separately net: phy: Add support for rate adaptation net: phylink: Support differing link/interface speed/duplex net: phylink: Adjust link settings based on rate adaptation net: phylink: Adjust advertisement based on rate adaptation [RFC] net: phylink: Add support for CRS-based rate adaptation net: phy: aquantia: Add some additional phy interfaces net: phy: aquantia: Add support for rate adaptation Documentation/networking/ethtool-netlink.rst | 2 + .../net/ethernet/freescale/dpaa/dpaa_eth.c | 6 +- drivers/net/phy/aquantia_main.c | 68 +++- drivers/net/phy/phy-core.c | 15 + drivers/net/phy/phy.c | 28 ++ drivers/net/phy/phylink.c | 340 +++++++++++++++--- include/linux/phy.h | 26 +- include/linux/phylink.h | 23 +- include/uapi/linux/ethtool.h | 18 +- include/uapi/linux/ethtool_netlink.h | 1 + net/ethtool/ioctl.c | 1 + net/ethtool/linkmodes.c | 5 + 12 files changed, 470 insertions(+), 63 deletions(-)