| Message ID | 8592ada26b28995d038ef67f15c145b6cebf4165.1657956652.git.lorenzo@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mtk_eth_soc: add xdp support | expand |
On Sat, 2022-07-16 at 09:34 +0200, Lorenzo Bianconi wrote: > Introduce support for the page_pool stats API into mtk_eth_soc driver. > Report page_pool stats through ethtool. > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > --- > drivers/net/ethernet/mediatek/Kconfig | 1 + > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 40 +++++++++++++++++++-- > 2 files changed, 38 insertions(+), 3 deletions(-) > > diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig > index d2422c7b31b0..97374fb3ee79 100644 > --- a/drivers/net/ethernet/mediatek/Kconfig > +++ b/drivers/net/ethernet/mediatek/Kconfig > @@ -18,6 +18,7 @@ config NET_MEDIATEK_SOC > select PHYLINK > select DIMLIB > select PAGE_POOL > + select PAGE_POOL_STATS > help > This driver supports the gigabit ethernet MACs in the > MediaTek SoC family. > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > index abb8bc281015..eba95a86086d 100644 > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > @@ -3517,11 +3517,19 @@ static void mtk_get_strings(struct net_device *dev, u32 stringset, u8 *data) > int i; > > switch (stringset) { > - case ETH_SS_STATS: > + case ETH_SS_STATS: { > + struct mtk_mac *mac = netdev_priv(dev); > + struct mtk_eth *eth = mac->hw; > + > for (i = 0; i < ARRAY_SIZE(mtk_ethtool_stats); i++) { > memcpy(data, mtk_ethtool_stats[i].str, ETH_GSTRING_LEN); > data += ETH_GSTRING_LEN; > } > + if (!eth->hwlro) I see the page_pool is enabled if and only if !hwlro, but I think it would be more clear if you explicitly check for page_pool here (and in a few other places below), so that if the condition to enable page_pool someday will change, this code will still be fine. Thanks! Paolo
> On Sat, 2022-07-16 at 09:34 +0200, Lorenzo Bianconi wrote: > > Introduce support for the page_pool stats API into mtk_eth_soc driver. > > Report page_pool stats through ethtool. > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > --- > > drivers/net/ethernet/mediatek/Kconfig | 1 + > > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 40 +++++++++++++++++++-- > > 2 files changed, 38 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig > > index d2422c7b31b0..97374fb3ee79 100644 > > --- a/drivers/net/ethernet/mediatek/Kconfig > > +++ b/drivers/net/ethernet/mediatek/Kconfig > > @@ -18,6 +18,7 @@ config NET_MEDIATEK_SOC > > select PHYLINK > > select DIMLIB > > select PAGE_POOL > > + select PAGE_POOL_STATS > > help > > This driver supports the gigabit ethernet MACs in the > > MediaTek SoC family. > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > index abb8bc281015..eba95a86086d 100644 > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > @@ -3517,11 +3517,19 @@ static void mtk_get_strings(struct net_device *dev, u32 stringset, u8 *data) > > int i; > > > > switch (stringset) { > > - case ETH_SS_STATS: > > + case ETH_SS_STATS: { > > + struct mtk_mac *mac = netdev_priv(dev); > > + struct mtk_eth *eth = mac->hw; > > + > > for (i = 0; i < ARRAY_SIZE(mtk_ethtool_stats); i++) { > > memcpy(data, mtk_ethtool_stats[i].str, ETH_GSTRING_LEN); > > data += ETH_GSTRING_LEN; > > } > > + if (!eth->hwlro) > > I see the page_pool is enabled if and only if !hwlro, but I think it > would be more clear if you explicitly check for page_pool here (and in > a few other places below), so that if the condition to enable page_pool > someday will change, this code will still be fine. Hi Paolo, page_pool pointer is defined in mtk_rx_ring structure, so theoretically we can have a page_pool defined for queue 0 but not for queues {1, 2, 3}. "!eth->hwlro" means there is at least one page_pool allocated. Do you prefer to do something like: bool mtk_is_pp_enabled(struct mtk_eth *eth) { for (i = 0; i < ARRAY_SIZE(eth->rx_ring); i++) { struct mtk_rx_ring *ring = ð->rx_ring[i]; if (ring->page_pool) return true; } return false; } Regards, Lorenzo > > Thanks! > > Paolo >
On Tue, 2022-07-19 at 12:18 +0200, Lorenzo Bianconi wrote: > > On Sat, 2022-07-16 at 09:34 +0200, Lorenzo Bianconi wrote: > > > Introduce support for the page_pool stats API into mtk_eth_soc > > > driver. > > > Report page_pool stats through ethtool. > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > --- > > > drivers/net/ethernet/mediatek/Kconfig | 1 + > > > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 40 > > > +++++++++++++++++++-- > > > 2 files changed, 38 insertions(+), 3 deletions(-) > > > > > > diff --git a/drivers/net/ethernet/mediatek/Kconfig > > > b/drivers/net/ethernet/mediatek/Kconfig > > > index d2422c7b31b0..97374fb3ee79 100644 > > > --- a/drivers/net/ethernet/mediatek/Kconfig > > > +++ b/drivers/net/ethernet/mediatek/Kconfig > > > @@ -18,6 +18,7 @@ config NET_MEDIATEK_SOC > > > select PHYLINK > > > select DIMLIB > > > select PAGE_POOL > > > + select PAGE_POOL_STATS > > > help > > > This driver supports the gigabit ethernet MACs in the > > > MediaTek SoC family. > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > index abb8bc281015..eba95a86086d 100644 > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > @@ -3517,11 +3517,19 @@ static void mtk_get_strings(struct > > > net_device *dev, u32 stringset, u8 *data) > > > int i; > > > > > > switch (stringset) { > > > - case ETH_SS_STATS: > > > + case ETH_SS_STATS: { > > > + struct mtk_mac *mac = netdev_priv(dev); > > > + struct mtk_eth *eth = mac->hw; > > > + > > > for (i = 0; i < ARRAY_SIZE(mtk_ethtool_stats); > > > i++) { > > > memcpy(data, mtk_ethtool_stats[i].str, > > > ETH_GSTRING_LEN); > > > data += ETH_GSTRING_LEN; > > > } > > > + if (!eth->hwlro) > > > > I see the page_pool is enabled if and only if !hwlro, but I think > > it > > would be more clear if you explicitly check for page_pool here (and > > in > > a few other places below), so that if the condition to enable > > page_pool > > someday will change, this code will still be fine. > > Hi Paolo, > > page_pool pointer is defined in mtk_rx_ring structure, so > theoretically we can have a > page_pool defined for queue 0 but not for queues {1, 2, 3}. I see. I missed hwlro is a per device setting. > "!eth->hwlro" means > there is at least one page_pool allocated. Do you prefer to do > something like: > > bool mtk_is_pp_enabled(struct mtk_eth *eth) > { > for (i = 0; i < ARRAY_SIZE(eth->rx_ring); i++) { > struct mtk_rx_ring *ring = ð->rx_ring[i]; > > if (ring->page_pool) > return true; > } > return false; > } Even: bool mtk_is_pp_enabled(struct mtk_eth *eth) { return !eth->hwlro; } will suffice to encaspulate the logic behind page pool enabling in a single place. /P
> On Tue, 2022-07-19 at 12:18 +0200, Lorenzo Bianconi wrote: > > > On Sat, 2022-07-16 at 09:34 +0200, Lorenzo Bianconi wrote: > > > > Introduce support for the page_pool stats API into mtk_eth_soc > > > > driver. > > > > Report page_pool stats through ethtool. > > > > > > > > Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> > > > > --- > > > > drivers/net/ethernet/mediatek/Kconfig | 1 + > > > > drivers/net/ethernet/mediatek/mtk_eth_soc.c | 40 > > > > +++++++++++++++++++-- > > > > 2 files changed, 38 insertions(+), 3 deletions(-) > > > > > > > > diff --git a/drivers/net/ethernet/mediatek/Kconfig > > > > b/drivers/net/ethernet/mediatek/Kconfig > > > > index d2422c7b31b0..97374fb3ee79 100644 > > > > --- a/drivers/net/ethernet/mediatek/Kconfig > > > > +++ b/drivers/net/ethernet/mediatek/Kconfig > > > > @@ -18,6 +18,7 @@ config NET_MEDIATEK_SOC > > > > select PHYLINK > > > > select DIMLIB > > > > select PAGE_POOL > > > > + select PAGE_POOL_STATS > > > > help > > > > This driver supports the gigabit ethernet MACs in the > > > > MediaTek SoC family. > > > > diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > index abb8bc281015..eba95a86086d 100644 > > > > --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c > > > > @@ -3517,11 +3517,19 @@ static void mtk_get_strings(struct > > > > net_device *dev, u32 stringset, u8 *data) > > > > int i; > > > > > > > > switch (stringset) { > > > > - case ETH_SS_STATS: > > > > + case ETH_SS_STATS: { > > > > + struct mtk_mac *mac = netdev_priv(dev); > > > > + struct mtk_eth *eth = mac->hw; > > > > + > > > > for (i = 0; i < ARRAY_SIZE(mtk_ethtool_stats); > > > > i++) { > > > > memcpy(data, mtk_ethtool_stats[i].str, > > > > ETH_GSTRING_LEN); > > > > data += ETH_GSTRING_LEN; > > > > } > > > > + if (!eth->hwlro) > > > > > > I see the page_pool is enabled if and only if !hwlro, but I think > > > it > > > would be more clear if you explicitly check for page_pool here (and > > > in > > > a few other places below), so that if the condition to enable > > > page_pool > > > someday will change, this code will still be fine. > > > > Hi Paolo, > > > > page_pool pointer is defined in mtk_rx_ring structure, so > > theoretically we can have a > > page_pool defined for queue 0 but not for queues {1, 2, 3}. > > I see. I missed hwlro is a per device setting. > > > "!eth->hwlro" means > > there is at least one page_pool allocated. Do you prefer to do > > something like: > > > > bool mtk_is_pp_enabled(struct mtk_eth *eth) > > { > > > for (i = 0; i < ARRAY_SIZE(eth->rx_ring); i++) { > > struct mtk_rx_ring *ring = ð->rx_ring[i]; > > > > if (ring->page_pool) > > return true; > > } > > return false; > > } > > Even: > > bool mtk_is_pp_enabled(struct mtk_eth *eth) > { > return !eth->hwlro; > } > > will suffice to encaspulate the logic behind page pool enabling in a > single place. ack, I am fine with it. I will fix in v4. Regards, Lorenzo > > /P >
diff --git a/drivers/net/ethernet/mediatek/Kconfig b/drivers/net/ethernet/mediatek/Kconfig index d2422c7b31b0..97374fb3ee79 100644 --- a/drivers/net/ethernet/mediatek/Kconfig +++ b/drivers/net/ethernet/mediatek/Kconfig @@ -18,6 +18,7 @@ config NET_MEDIATEK_SOC select PHYLINK select DIMLIB select PAGE_POOL + select PAGE_POOL_STATS help This driver supports the gigabit ethernet MACs in the MediaTek SoC family. diff --git a/drivers/net/ethernet/mediatek/mtk_eth_soc.c b/drivers/net/ethernet/mediatek/mtk_eth_soc.c index abb8bc281015..eba95a86086d 100644 --- a/drivers/net/ethernet/mediatek/mtk_eth_soc.c +++ b/drivers/net/ethernet/mediatek/mtk_eth_soc.c @@ -3517,11 +3517,19 @@ static void mtk_get_strings(struct net_device *dev, u32 stringset, u8 *data) int i; switch (stringset) { - case ETH_SS_STATS: + case ETH_SS_STATS: { + struct mtk_mac *mac = netdev_priv(dev); + struct mtk_eth *eth = mac->hw; + for (i = 0; i < ARRAY_SIZE(mtk_ethtool_stats); i++) { memcpy(data, mtk_ethtool_stats[i].str, ETH_GSTRING_LEN); data += ETH_GSTRING_LEN; } + if (!eth->hwlro) + page_pool_ethtool_stats_get_strings(data); + break; + } + default: break; } } @@ -3529,18 +3537,42 @@ static void mtk_get_strings(struct net_device *dev, u32 stringset, u8 *data) static int mtk_get_sset_count(struct net_device *dev, int sset) { switch (sset) { - case ETH_SS_STATS: - return ARRAY_SIZE(mtk_ethtool_stats); + case ETH_SS_STATS: { + int count = ARRAY_SIZE(mtk_ethtool_stats); + struct mtk_mac *mac = netdev_priv(dev); + struct mtk_eth *eth = mac->hw; + + if (!eth->hwlro) + count += page_pool_ethtool_stats_get_count(); + return count; + } default: return -EOPNOTSUPP; } } +static void mtk_ethtool_pp_stats(struct mtk_eth *eth, u64 *data) +{ + struct page_pool_stats stats = {}; + int i; + + for (i = 0; i < ARRAY_SIZE(eth->rx_ring); i++) { + struct mtk_rx_ring *ring = ð->rx_ring[i]; + + if (!ring->page_pool) + continue; + + page_pool_get_stats(ring->page_pool, &stats); + } + page_pool_ethtool_stats_get(data, &stats); +} + static void mtk_get_ethtool_stats(struct net_device *dev, struct ethtool_stats *stats, u64 *data) { struct mtk_mac *mac = netdev_priv(dev); struct mtk_hw_stats *hwstats = mac->hw_stats; + struct mtk_eth *eth = mac->hw; u64 *data_src, *data_dst; unsigned int start; int i; @@ -3563,6 +3595,8 @@ static void mtk_get_ethtool_stats(struct net_device *dev, for (i = 0; i < ARRAY_SIZE(mtk_ethtool_stats); i++) *data_dst++ = *(data_src + mtk_ethtool_stats[i].offset); + if (!eth->hwlro) + mtk_ethtool_pp_stats(eth, data_dst); } while (u64_stats_fetch_retry_irq(&hwstats->syncp, start)); }
Introduce support for the page_pool stats API into mtk_eth_soc driver. Report page_pool stats through ethtool. Signed-off-by: Lorenzo Bianconi <lorenzo@kernel.org> --- drivers/net/ethernet/mediatek/Kconfig | 1 + drivers/net/ethernet/mediatek/mtk_eth_soc.c | 40 +++++++++++++++++++-- 2 files changed, 38 insertions(+), 3 deletions(-)