| Message ID | 20240715195449.244268-1-shenwei.wang@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] net: fec: Enable SOC specific rx-usecs coalescence default setting | expand |
On Mon, Jul 15, 2024 at 02:54:49PM -0500, Shenwei Wang wrote: > The current FEC driver uses a single default rx-usecs coalescence setting > across all SoCs. This approach leads to suboptimal latency on newer, high > performance SoCs such as i.MX8QM and i.MX8M. Does ethtool -C work on these devices? Interrupt coalescence is more than latency. It is also about CPU load. Does NAPI polling work correctly on these devices? If so, interrupts should be disabled quite a bit, and then interrupt latency does not matter so much. Have you benchmarked CPU usage with this patch, for a range of traffic bandwidths and burst patterns. How does it differ? > For example, the following are the ping result on a i.MX8QXP board: > > $ ping 192.168.0.195 > PING 192.168.0.195 (192.168.0.195) 56(84) bytes of data. > 64 bytes from 192.168.0.195: icmp_seq=1 ttl=64 time=1.32 ms > 64 bytes from 192.168.0.195: icmp_seq=2 ttl=64 time=1.31 ms > 64 bytes from 192.168.0.195: icmp_seq=3 ttl=64 time=1.33 ms > 64 bytes from 192.168.0.195: icmp_seq=4 ttl=64 time=1.33 ms > > The current default rx-usecs value of 1000us was originally optimized for > CPU-bound systems like i.MX2x and i.MX6x. However, for i.MX8 and later > generations, CPU performance is no longer a limiting factor. Consequently, > the rx-usecs value should be reduced to enhance receive latency. > > The following are the ping result with the 100us setting: > > $ ping 192.168.0.195 > PING 192.168.0.195 (192.168.0.195) 56(84) bytes of data. > 64 bytes from 192.168.0.195: icmp_seq=1 ttl=64 time=0.554 ms > 64 bytes from 192.168.0.195: icmp_seq=2 ttl=64 time=0.499 ms > 64 bytes from 192.168.0.195: icmp_seq=3 ttl=64 time=0.502 ms > 64 bytes from 192.168.0.195: icmp_seq=4 ttl=64 time=0.486 ms > > Fixes: df727d4547de ("net: fec: don't reset irq coalesce settings to defaults on "ip link up"") Fixes is not correct here. It was never broken. This is maybe an optimisation, maybe a deoptimisation, depending on your use case. And next-next is closed at the moment anyway. Andrew
> -----Original Message----- > From: Andrew Lunn <andrew@lunn.ch> > Sent: Monday, July 15, 2024 3:47 PM > To: Shenwei Wang <shenwei.wang@nxp.com> > Cc: Wei Fang <wei.fang@nxp.com>; David S. Miller <davem@davemloft.net>; > Eric Dumazet <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; > Paolo Abeni <pabeni@redhat.com>; Clark Wang <xiaoning.wang@nxp.com>; > Rasmus Villemoes <linux@rasmusvillemoes.dk>; imx@lists.linux.dev; > netdev@vger.kernel.org; dl-linux-imx <linux-imx@nxp.com> > Subject: [EXT] Re: [PATCH net-next] net: fec: Enable SOC specific rx-usecs > coalescence default setting > On Mon, Jul 15, 2024 at 02:54:49PM -0500, Shenwei Wang wrote: > > The current FEC driver uses a single default rx-usecs coalescence > > setting across all SoCs. This approach leads to suboptimal latency on > > newer, high performance SoCs such as i.MX8QM and i.MX8M. > > Does ethtool -C work on these devices? > Yes, ethtool -C also works. This patch is to enable the proper default value so that a user won't have to reconfigure it via ethtool. > Interrupt coalescence is more than latency. It is also about CPU load. > > Does NAPI polling work correctly on these devices? If so, interrupts should be > disabled quite a bit, and then interrupt latency does not matter so much. > > Have you benchmarked CPU usage with this patch, for a range of traffic > bandwidths and burst patterns. How does it differ? > The NAPI polling works correctly. The traffic bandwidth and CPU usage is no notable impact during iperf testing. > > 64 bytes from 192.168.0.195: icmp_seq=4 ttl=64 time=0.486 ms > > > > Fixes: df727d4547de ("net: fec: don't reset irq coalesce settings to > > defaults on "ip link up"") > > Fixes is not correct here. It was never broken. This is maybe an optimisation, > maybe a deoptimisation, depending on your use case. > Em, just an optimization. Regards, Shenwei > And next-next is closed at the moment anyway. > > Andrew
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c index fb19295529a2..820122899691 100644 --- a/drivers/net/ethernet/freescale/fec_main.c +++ b/drivers/net/ethernet/freescale/fec_main.c @@ -99,6 +99,7 @@ static const u16 fec_enet_vlan_pri_to_queue[8] = {0, 0, 1, 1, 1, 2, 2, 2}; struct fec_devinfo { u32 quirks; + unsigned int rx_time_itr; }; static const struct fec_devinfo fec_imx25_info = { @@ -159,6 +160,7 @@ static const struct fec_devinfo fec_imx8mq_info = { FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES | FEC_QUIRK_HAS_EEE | FEC_QUIRK_WAKEUP_FROM_INT2 | FEC_QUIRK_HAS_MDIO_C45, + .rx_time_itr = 100, }; static const struct fec_devinfo fec_imx8qm_info = { @@ -169,6 +171,7 @@ static const struct fec_devinfo fec_imx8qm_info = { FEC_QUIRK_HAS_RACC | FEC_QUIRK_HAS_COALESCE | FEC_QUIRK_CLEAR_SETUP_MII | FEC_QUIRK_HAS_MULTI_QUEUES | FEC_QUIRK_DELAYED_CLKS_SUPPORT | FEC_QUIRK_HAS_MDIO_C45, + .rx_time_itr = 100, }; static const struct fec_devinfo fec_s32v234_info = { @@ -4027,8 +4030,9 @@ static int fec_enet_init(struct net_device *ndev) #endif fep->rx_pkts_itr = FEC_ITR_ICFT_DEFAULT; fep->tx_pkts_itr = FEC_ITR_ICFT_DEFAULT; - fep->rx_time_itr = FEC_ITR_ICTT_DEFAULT; fep->tx_time_itr = FEC_ITR_ICTT_DEFAULT; + if (fep->rx_time_itr == 0) + fep->rx_time_itr = FEC_ITR_ICTT_DEFAULT; /* Check mask of the streaming and coherent API */ ret = dma_set_mask_and_coherent(&fep->pdev->dev, DMA_BIT_MASK(32)); @@ -4325,8 +4329,10 @@ fec_probe(struct platform_device *pdev) dev_info = device_get_match_data(&pdev->dev); if (!dev_info) dev_info = (const struct fec_devinfo *)pdev->id_entry->driver_data; - if (dev_info) + if (dev_info) { fep->quirks = dev_info->quirks; + fep->rx_time_itr = dev_info->rx_time_itr; + } fep->netdev = ndev; fep->num_rx_queues = num_rx_qs;
The current FEC driver uses a single default rx-usecs coalescence setting across all SoCs. This approach leads to suboptimal latency on newer, high performance SoCs such as i.MX8QM and i.MX8M. For example, the following are the ping result on a i.MX8QXP board: $ ping 192.168.0.195 PING 192.168.0.195 (192.168.0.195) 56(84) bytes of data. 64 bytes from 192.168.0.195: icmp_seq=1 ttl=64 time=1.32 ms 64 bytes from 192.168.0.195: icmp_seq=2 ttl=64 time=1.31 ms 64 bytes from 192.168.0.195: icmp_seq=3 ttl=64 time=1.33 ms 64 bytes from 192.168.0.195: icmp_seq=4 ttl=64 time=1.33 ms The current default rx-usecs value of 1000us was originally optimized for CPU-bound systems like i.MX2x and i.MX6x. However, for i.MX8 and later generations, CPU performance is no longer a limiting factor. Consequently, the rx-usecs value should be reduced to enhance receive latency. The following are the ping result with the 100us setting: $ ping 192.168.0.195 PING 192.168.0.195 (192.168.0.195) 56(84) bytes of data. 64 bytes from 192.168.0.195: icmp_seq=1 ttl=64 time=0.554 ms 64 bytes from 192.168.0.195: icmp_seq=2 ttl=64 time=0.499 ms 64 bytes from 192.168.0.195: icmp_seq=3 ttl=64 time=0.502 ms 64 bytes from 192.168.0.195: icmp_seq=4 ttl=64 time=0.486 ms Fixes: df727d4547de ("net: fec: don't reset irq coalesce settings to defaults on "ip link up"") Signed-off-by: Shenwei Wang <shenwei.wang@nxp.com> --- drivers/net/ethernet/freescale/fec_main.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)