Create and utilize bulk regmap reads instead of single access for gathering
stats. The background reading of statistics happens frequently, and over
a few contiguous memory regions.
High speed PCIe buses and MMIO access will probably see negligible
performance increase. Lower speed buses like SPI and I2C could see
significant performance increase, since the bus configuration and register
access times account for a large percentage of data transfer time.
Signed-off-by: Colin Foster <[email protected]>
---
drivers/net/ethernet/mscc/ocelot.c | 78 +++++++++++++++++++++++++-----
include/soc/mscc/ocelot.h | 8 +++
2 files changed, 73 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 455293aa6343..5efb1f3a1410 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1737,32 +1737,41 @@ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
}
EXPORT_SYMBOL(ocelot_get_strings);
-static void ocelot_update_stats(struct ocelot *ocelot)
+static int ocelot_update_stats(struct ocelot *ocelot)
{
- int i, j;
+ struct ocelot_stats_region *region;
+ int i, j, err = 0;
mutex_lock(&ocelot->stats_lock);
for (i = 0; i < ocelot->num_phys_ports; i++) {
+ unsigned int idx = 0;
+
/* Configure the port to read the stats from */
ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
- for (j = 0; j < ocelot->num_stats; j++) {
- u32 val;
- unsigned int idx = i * ocelot->num_stats + j;
+ list_for_each_entry(region, &ocelot->stats_regions, node) {
+ err = ocelot_bulk_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
+ region->offset, region->buf,
+ region->count);
+ if (err)
+ goto out;
- val = ocelot_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
- ocelot->stats_layout[j].offset);
+ for (j = 0; j < region->count; j++) {
+ if (region->buf[j] < (ocelot->stats[idx + j] & U32_MAX))
+ ocelot->stats[idx + j] += (u64)1 << 32;
- if (val < (ocelot->stats[idx] & U32_MAX))
- ocelot->stats[idx] += (u64)1 << 32;
+ ocelot->stats[idx + j] = (ocelot->stats[idx + j] &
+ ~(u64)U32_MAX) + region->buf[j];
+ }
- ocelot->stats[idx] = (ocelot->stats[idx] &
- ~(u64)U32_MAX) + val;
+ idx += region->count;
}
}
+out:
mutex_unlock(&ocelot->stats_lock);
+ return err;
}
static void ocelot_check_stats_work(struct work_struct *work)
@@ -1779,10 +1788,11 @@ static void ocelot_check_stats_work(struct work_struct *work)
void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data)
{
- int i;
+ int i, err;
/* check and update now */
- ocelot_update_stats(ocelot);
+ err = ocelot_update_stats(ocelot);
+ WARN_ONCE(err, "Error %d updating ethtool stats\n", err);
/* Copy all counters */
for (i = 0; i < ocelot->num_stats; i++)
@@ -1799,6 +1809,41 @@ int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
}
EXPORT_SYMBOL(ocelot_get_sset_count);
+static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
+{
+ struct ocelot_stats_region *region = NULL;
+ unsigned int last;
+ int i;
+
+ INIT_LIST_HEAD(&ocelot->stats_regions);
+
+ for (i = 0; i < ocelot->num_stats; i++) {
+ if (region && ocelot->stats_layout[i].offset == last + 1) {
+ region->count++;
+ } else {
+ region = devm_kzalloc(ocelot->dev, sizeof(*region),
+ GFP_KERNEL);
+ if (!region)
+ return -ENOMEM;
+
+ region->offset = ocelot->stats_layout[i].offset;
+ region->count = 1;
+ list_add_tail(®ion->node, &ocelot->stats_regions);
+ }
+
+ last = ocelot->stats_layout[i].offset;
+ }
+
+ list_for_each_entry(region, &ocelot->stats_regions, node) {
+ region->buf = devm_kcalloc(ocelot->dev, region->count,
+ sizeof(*region->buf), GFP_KERNEL);
+ if (!region->buf)
+ return -ENOMEM;
+ }
+
+ return 0;
+}
+
int ocelot_get_ts_info(struct ocelot *ocelot, int port,
struct ethtool_ts_info *info)
{
@@ -2799,6 +2844,13 @@ int ocelot_init(struct ocelot *ocelot)
ANA_CPUQ_8021_CFG_CPUQ_BPDU_VAL(6),
ANA_CPUQ_8021_CFG, i);
+ ret = ocelot_prepare_stats_regions(ocelot);
+ if (ret) {
+ destroy_workqueue(ocelot->stats_queue);
+ destroy_workqueue(ocelot->owq);
+ return ret;
+ }
+
INIT_DELAYED_WORK(&ocelot->stats_work, ocelot_check_stats_work);
queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
OCELOT_STATS_CHECK_DELAY);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 312b72558659..d3291a5f7e88 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -542,6 +542,13 @@ struct ocelot_stat_layout {
char name[ETH_GSTRING_LEN];
};
+struct ocelot_stats_region {
+ struct list_head node;
+ u32 offset;
+ int count;
+ u32 *buf;
+};
+
enum ocelot_tag_prefix {
OCELOT_TAG_PREFIX_DISABLED = 0,
OCELOT_TAG_PREFIX_NONE,
@@ -673,6 +680,7 @@ struct ocelot {
struct regmap_field *regfields[REGFIELD_MAX];
const u32 *const *map;
const struct ocelot_stat_layout *stats_layout;
+ struct list_head stats_regions;
unsigned int num_stats;
u32 pool_size[OCELOT_SB_NUM][OCELOT_SB_POOL_NUM];
--
2.25.1
On Tue, Feb 08, 2022 at 03:45:15PM +0000, Vladimir Oltean wrote:
> On Tue, Feb 08, 2022 at 07:41:56AM -0800, Colin Foster wrote:
> > I see that now. It is confusing and I'll clear it up. I never caught
> > this because I'm testing in a setup where port 0 is the CPU port, so I
> > can't get ethtool stats. Thanks!
>
> You can retrieve the stats on the CPU port, as they are appended to the
> ethtool stats of the DSA master, prefixed with "p%02d_", cpu_dp->index.
>
> ethtool -S eth0 | grep -v ': 0' # where eth0 is the DSA master
Thanks for this hint. I didn't even think to go looking for it since the
DSA Documenation mentions an "inability to fetch CPU port statistics
counters using ethtool." I see now that this bullet point is from 2015,
and probably should be removed entirely.
On Mon, Feb 07, 2022 at 08:46:44PM -0800, Colin Foster wrote:
> Create and utilize bulk regmap reads instead of single access for gathering
> stats. The background reading of statistics happens frequently, and over
> a few contiguous memory regions.
>
> High speed PCIe buses and MMIO access will probably see negligible
> performance increase. Lower speed buses like SPI and I2C could see
> significant performance increase, since the bus configuration and register
> access times account for a large percentage of data transfer time.
>
> Signed-off-by: Colin Foster <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot.c | 78 +++++++++++++++++++++++++-----
> include/soc/mscc/ocelot.h | 8 +++
> 2 files changed, 73 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 455293aa6343..5efb1f3a1410 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1737,32 +1737,41 @@ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
> }
> EXPORT_SYMBOL(ocelot_get_strings);
>
> -static void ocelot_update_stats(struct ocelot *ocelot)
> +static int ocelot_update_stats(struct ocelot *ocelot)
> {
> - int i, j;
> + struct ocelot_stats_region *region;
> + int i, j, err = 0;
>
> mutex_lock(&ocelot->stats_lock);
>
> for (i = 0; i < ocelot->num_phys_ports; i++) {
> + unsigned int idx = 0;
> +
This is a bug which causes ocelot->stats to be overwritten with the
statistics of port 0, for all ports. Either move the variable
declaration and initialization with 0 in the larger scope (outside the
"for" loop), or initialize idx with i * ocelot->num_stats.
> /* Configure the port to read the stats from */
> ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
>
> - for (j = 0; j < ocelot->num_stats; j++) {
> - u32 val;
> - unsigned int idx = i * ocelot->num_stats + j;
> + list_for_each_entry(region, &ocelot->stats_regions, node) {
> + err = ocelot_bulk_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> + region->offset, region->buf,
> + region->count);
> + if (err)
> + goto out;
>
> - val = ocelot_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> - ocelot->stats_layout[j].offset);
> + for (j = 0; j < region->count; j++) {
> + if (region->buf[j] < (ocelot->stats[idx + j] & U32_MAX))
> + ocelot->stats[idx + j] += (u64)1 << 32;
I'd prefer if you reduce the apparent complexity of this logic by
creating some temporary variables:
u64 *stat = &ocelot->stats[idx + j];
u64 val = region->buf[j];
>
> - if (val < (ocelot->stats[idx] & U32_MAX))
> - ocelot->stats[idx] += (u64)1 << 32;
> + ocelot->stats[idx + j] = (ocelot->stats[idx + j] &
> + ~(u64)U32_MAX) + region->buf[j];
> + }
>
> - ocelot->stats[idx] = (ocelot->stats[idx] &
> - ~(u64)U32_MAX) + val;
> + idx += region->count;
> }
> }
>
> +out:
> mutex_unlock(&ocelot->stats_lock);
> + return err;
> }
>
> static void ocelot_check_stats_work(struct work_struct *work)
> @@ -1779,10 +1788,11 @@ static void ocelot_check_stats_work(struct work_struct *work)
>
> void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data)
> {
> - int i;
> + int i, err;
>
> /* check and update now */
> - ocelot_update_stats(ocelot);
> + err = ocelot_update_stats(ocelot);
Please, as a separate change, introduce a function that reads the
statistics for a single port, and make ethtool call that and not the
entire port array, it's pointless.
> + WARN_ONCE(err, "Error %d updating ethtool stats\n", err);
>
> /* Copy all counters */
> for (i = 0; i < ocelot->num_stats; i++)
and here, in the unseen part of the context, lies:
/* Copy all counters */
for (i = 0; i < ocelot->num_stats; i++)
*data++ = ocelot->stats[port * ocelot->num_stats + i];
I think this is buggy, because this is a reader of ocelot->stats which
is not protected by ocelot->stats_lock (it was taken and dropped by
ocelot_update_stats). But a second ocelot_update_stats() can run
concurrently with ethtool and ruin the day, modifying the array at the
same time as it's being read out.
The new function that you introduce, for reading the stats of a single
port, should require that ocelot->stats_lock is already held, and you
should hold it from top-level (ocelot_get_ethtool_stats).
> @@ -1799,6 +1809,41 @@ int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
> }
> EXPORT_SYMBOL(ocelot_get_sset_count);
>
> +static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> +{
> + struct ocelot_stats_region *region = NULL;
> + unsigned int last;
> + int i;
> +
> + INIT_LIST_HEAD(&ocelot->stats_regions);
> +
> + for (i = 0; i < ocelot->num_stats; i++) {
> + if (region && ocelot->stats_layout[i].offset == last + 1) {
> + region->count++;
> + } else {
> + region = devm_kzalloc(ocelot->dev, sizeof(*region),
> + GFP_KERNEL);
> + if (!region)
> + return -ENOMEM;
> +
> + region->offset = ocelot->stats_layout[i].offset;
> + region->count = 1;
> + list_add_tail(®ion->node, &ocelot->stats_regions);
> + }
> +
> + last = ocelot->stats_layout[i].offset;
> + }
> +
> + list_for_each_entry(region, &ocelot->stats_regions, node) {
> + region->buf = devm_kcalloc(ocelot->dev, region->count,
> + sizeof(*region->buf), GFP_KERNEL);
> + if (!region->buf)
> + return -ENOMEM;
> + }
> +
> + return 0;
> +}
> +
> int ocelot_get_ts_info(struct ocelot *ocelot, int port,
> struct ethtool_ts_info *info)
> {
> @@ -2799,6 +2844,13 @@ int ocelot_init(struct ocelot *ocelot)
> ANA_CPUQ_8021_CFG_CPUQ_BPDU_VAL(6),
> ANA_CPUQ_8021_CFG, i);
>
> + ret = ocelot_prepare_stats_regions(ocelot);
> + if (ret) {
> + destroy_workqueue(ocelot->stats_queue);
> + destroy_workqueue(ocelot->owq);
> + return ret;
> + }
> +
> INIT_DELAYED_WORK(&ocelot->stats_work, ocelot_check_stats_work);
> queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
> OCELOT_STATS_CHECK_DELAY);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 312b72558659..d3291a5f7e88 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -542,6 +542,13 @@ struct ocelot_stat_layout {
> char name[ETH_GSTRING_LEN];
> };
>
> +struct ocelot_stats_region {
> + struct list_head node;
> + u32 offset;
> + int count;
> + u32 *buf;
> +};
> +
> enum ocelot_tag_prefix {
> OCELOT_TAG_PREFIX_DISABLED = 0,
> OCELOT_TAG_PREFIX_NONE,
> @@ -673,6 +680,7 @@ struct ocelot {
> struct regmap_field *regfields[REGFIELD_MAX];
> const u32 *const *map;
> const struct ocelot_stat_layout *stats_layout;
> + struct list_head stats_regions;
> unsigned int num_stats;
>
> u32 pool_size[OCELOT_SB_NUM][OCELOT_SB_POOL_NUM];
> --
> 2.25.1
>