| Message ID | 20240216-spi-mem-stats-v2-1-9256dfe4887d@bootlin.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | e63aef9c9121e5061cbf5112d12cadc9da399692 |
| Headers | show |
| Series | [v2] spi: spi-mem: add statistics support to ->exec_op() calls | expand |
On 2/16/24 16:42, Théo Lebrun wrote: > Current behavior is that spi-mem operations do not increment statistics, > neither per-controller nor per-device, if ->exec_op() is used. For > operations that do NOT use ->exec_op(), stats are increased as the > usual spi_sync() is called. > > The newly implemented spi_mem_add_op_stats() function is strongly > inspired by spi_statistics_add_transfer_stats(); locking logic and > l2len computation comes from there. > > Statistics that are being filled: bytes{,_rx,_tx}, messages, transfers, > errors, timedout, transfer_bytes_histo_*. > > Note about messages & transfers counters: in the fallback to spi_sync() > case, there are from 1 to 4 transfers per message. We only register one > big transfer in the ->exec_op() case as that is closer to reality. > > This patch is NOT touching: > - spi_async, spi_sync, spi_sync_immediate: those counters describe > precise function calls, incrementing them would be lying. I believe > comparing the messages counter to spi_async+spi_sync is a good way > to detect ->exec_op() calls, but I might be missing edge cases > knowledge. > - transfers_split_maxsize: splitting cannot happen if ->exec_op() is > provided. > > Reviewed-by: Dhruva Gole <d-gole@ti.com> > Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com > --- > Changes in v2: > - Turn len and l2len into u64. Remove casts on all 4 nbytes fields. > Remove clamp of l2len to be positive. > - Replace "xferred" in comment by "transferred". > - Remove sysfs demo from commit message. Moved to below the tear line. > - Take Reviewed-by Dhruva Gole. > - Link to v1: https://lore.kernel.org/r/20240209-spi-mem-stats-v1-1-dd1a422fc015@bootlin.com Reviewed-by: Tudor Ambarus <tudor.ambarus@linaro.org> > --- > > Testing this patch: > > $ cd /sys/devices/platform/soc > $ find . -type d -path "*spi*" -name statistics > ./2100000.spi/spi_master/spi0/statistics > ./2100000.spi/spi_master/spi0/spi0.0/statistics > $ cd ./2100000.spi/spi_master/spi0/statistics > > $ for f in *; do printf "%s\t" $f; cat $f; done | \ > grep -v transfer_bytes_histo | column -t > bytes 240745444 > bytes_rx 240170907 > bytes_tx 126320 > errors 0 > messages 97354 > spi_async 0 > spi_sync 0 > spi_sync_immediate 0 > timedout 0 > transfers 97354 > transfers_split_maxsize 0 > --- > drivers/spi/spi-mem.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- > 1 file changed, 48 insertions(+), 1 deletion(-) > > diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c > index 2dc8ceb85374..c9d6d42a88f5 100644 > --- a/drivers/spi/spi-mem.c > +++ b/drivers/spi/spi-mem.c > @@ -297,6 +297,49 @@ static void spi_mem_access_end(struct spi_mem *mem) > pm_runtime_put(ctlr->dev.parent); > } > > +static void spi_mem_add_op_stats(struct spi_statistics __percpu *pcpu_stats, > + const struct spi_mem_op *op, int exec_op_ret) > +{ > + struct spi_statistics *stats; > + u64 len, l2len; > + > + get_cpu(); > + stats = this_cpu_ptr(pcpu_stats); > + u64_stats_update_begin(&stats->syncp); > + > + /* > + * We do not have the concept of messages or transfers. Let's consider > + * that one operation is equivalent to one message and one transfer. > + */ > + u64_stats_inc(&stats->messages); > + u64_stats_inc(&stats->transfers); > + > + /* Use the sum of all lengths as bytes count and histogram value. */ > + len = op->cmd.nbytes + op->addr.nbytes; > + len += op->dummy.nbytes + op->data.nbytes; > + u64_stats_add(&stats->bytes, len); > + l2len = min(fls(len), SPI_STATISTICS_HISTO_SIZE) - 1; > + u64_stats_inc(&stats->transfer_bytes_histo[l2len]); > + > + /* Only account for data bytes as transferred bytes. */ > + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT) > + u64_stats_add(&stats->bytes_tx, op->data.nbytes); > + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_IN) > + u64_stats_add(&stats->bytes_rx, op->data.nbytes); > + > + /* > + * A timeout is not an error, following the same behavior as > + * spi_transfer_one_message(). > + */ > + if (exec_op_ret == -ETIMEDOUT) > + u64_stats_inc(&stats->timedout); > + else if (exec_op_ret) > + u64_stats_inc(&stats->errors); > + > + u64_stats_update_end(&stats->syncp); > + put_cpu(); > +} > + > /** > * spi_mem_exec_op() - Execute a memory operation > * @mem: the SPI memory > @@ -339,8 +382,12 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) > * read path) and expect the core to use the regular SPI > * interface in other cases. > */ > - if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP) > + if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP) { > + spi_mem_add_op_stats(ctlr->pcpu_statistics, op, ret); > + spi_mem_add_op_stats(mem->spi->pcpu_statistics, op, ret); > + > return ret; > + } > } > > tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes; > > --- > base-commit: 19b50f80b3a4865bd477aa5c026dd234d39a50d2 > change-id: 20240209-spi-mem-stats-ff9bf91c0f7e > > Best regards,
On Fri, 16 Feb 2024 17:42:19 +0100, Théo Lebrun wrote: > Current behavior is that spi-mem operations do not increment statistics, > neither per-controller nor per-device, if ->exec_op() is used. For > operations that do NOT use ->exec_op(), stats are increased as the > usual spi_sync() is called. > > The newly implemented spi_mem_add_op_stats() function is strongly > inspired by spi_statistics_add_transfer_stats(); locking logic and > l2len computation comes from there. > > [...] Applied to https://git.kernel.org/pub/scm/linux/kernel/git/broonie/spi.git for-next Thanks! [1/1] spi: spi-mem: add statistics support to ->exec_op() calls commit: e63aef9c9121e5061cbf5112d12cadc9da399692 All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted. You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed. If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced. Please add any relevant lists and maintainers to the CCs when replying to this mail. Thanks, Mark
diff --git a/drivers/spi/spi-mem.c b/drivers/spi/spi-mem.c index 2dc8ceb85374..c9d6d42a88f5 100644 --- a/drivers/spi/spi-mem.c +++ b/drivers/spi/spi-mem.c @@ -297,6 +297,49 @@ static void spi_mem_access_end(struct spi_mem *mem) pm_runtime_put(ctlr->dev.parent); } +static void spi_mem_add_op_stats(struct spi_statistics __percpu *pcpu_stats, + const struct spi_mem_op *op, int exec_op_ret) +{ + struct spi_statistics *stats; + u64 len, l2len; + + get_cpu(); + stats = this_cpu_ptr(pcpu_stats); + u64_stats_update_begin(&stats->syncp); + + /* + * We do not have the concept of messages or transfers. Let's consider + * that one operation is equivalent to one message and one transfer. + */ + u64_stats_inc(&stats->messages); + u64_stats_inc(&stats->transfers); + + /* Use the sum of all lengths as bytes count and histogram value. */ + len = op->cmd.nbytes + op->addr.nbytes; + len += op->dummy.nbytes + op->data.nbytes; + u64_stats_add(&stats->bytes, len); + l2len = min(fls(len), SPI_STATISTICS_HISTO_SIZE) - 1; + u64_stats_inc(&stats->transfer_bytes_histo[l2len]); + + /* Only account for data bytes as transferred bytes. */ + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_OUT) + u64_stats_add(&stats->bytes_tx, op->data.nbytes); + if (op->data.nbytes && op->data.dir == SPI_MEM_DATA_IN) + u64_stats_add(&stats->bytes_rx, op->data.nbytes); + + /* + * A timeout is not an error, following the same behavior as + * spi_transfer_one_message(). + */ + if (exec_op_ret == -ETIMEDOUT) + u64_stats_inc(&stats->timedout); + else if (exec_op_ret) + u64_stats_inc(&stats->errors); + + u64_stats_update_end(&stats->syncp); + put_cpu(); +} + /** * spi_mem_exec_op() - Execute a memory operation * @mem: the SPI memory @@ -339,8 +382,12 @@ int spi_mem_exec_op(struct spi_mem *mem, const struct spi_mem_op *op) * read path) and expect the core to use the regular SPI * interface in other cases. */ - if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP) + if (!ret || ret != -ENOTSUPP || ret != -EOPNOTSUPP) { + spi_mem_add_op_stats(ctlr->pcpu_statistics, op, ret); + spi_mem_add_op_stats(mem->spi->pcpu_statistics, op, ret); + return ret; + } } tmpbufsize = op->cmd.nbytes + op->addr.nbytes + op->dummy.nbytes;
Current behavior is that spi-mem operations do not increment statistics, neither per-controller nor per-device, if ->exec_op() is used. For operations that do NOT use ->exec_op(), stats are increased as the usual spi_sync() is called. The newly implemented spi_mem_add_op_stats() function is strongly inspired by spi_statistics_add_transfer_stats(); locking logic and l2len computation comes from there. Statistics that are being filled: bytes{,_rx,_tx}, messages, transfers, errors, timedout, transfer_bytes_histo_*. Note about messages & transfers counters: in the fallback to spi_sync() case, there are from 1 to 4 transfers per message. We only register one big transfer in the ->exec_op() case as that is closer to reality. This patch is NOT touching: - spi_async, spi_sync, spi_sync_immediate: those counters describe precise function calls, incrementing them would be lying. I believe comparing the messages counter to spi_async+spi_sync is a good way to detect ->exec_op() calls, but I might be missing edge cases knowledge. - transfers_split_maxsize: splitting cannot happen if ->exec_op() is provided. Reviewed-by: Dhruva Gole <d-gole@ti.com> Signed-off-by: Théo Lebrun <theo.lebrun@bootlin.com> --- Changes in v2: - Turn len and l2len into u64. Remove casts on all 4 nbytes fields. Remove clamp of l2len to be positive. - Replace "xferred" in comment by "transferred". - Remove sysfs demo from commit message. Moved to below the tear line. - Take Reviewed-by Dhruva Gole. - Link to v1: https://lore.kernel.org/r/20240209-spi-mem-stats-v1-1-dd1a422fc015@bootlin.com --- Testing this patch: $ cd /sys/devices/platform/soc $ find . -type d -path "*spi*" -name statistics ./2100000.spi/spi_master/spi0/statistics ./2100000.spi/spi_master/spi0/spi0.0/statistics $ cd ./2100000.spi/spi_master/spi0/statistics $ for f in *; do printf "%s\t" $f; cat $f; done | \ grep -v transfer_bytes_histo | column -t bytes 240745444 bytes_rx 240170907 bytes_tx 126320 errors 0 messages 97354 spi_async 0 spi_sync 0 spi_sync_immediate 0 timedout 0 transfers 97354 transfers_split_maxsize 0 --- drivers/spi/spi-mem.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) --- base-commit: 19b50f80b3a4865bd477aa5c026dd234d39a50d2 change-id: 20240209-spi-mem-stats-ff9bf91c0f7e Best regards,