Re: [PATCH] spi: spi-mem: add statistics support to ->exec_op() calls

[Théo Lebrun <theo.lebrun@xxxxxxxxxxx>]

Hello,

On Mon Feb 12, 2024 at 12:13 PM CET, Dhruva Gole wrote:
> Hi!
>
> On Feb 09, 2024 at 14:51:23 +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.
> > 
> > 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.
>
> Can you please elaborate on this point a bit? To me it feels then that
> the reported stats in this case will be less than the true value then?

To me, a transfer is one transaction with the SPI controller. In most
implementations of ->exec_op(), the controller gets configured once for
the full transfer to take place. This contrasts with the fallback case
that does from 1 to 4 transfers (cmd, addr, dummy & data, with the last
three being optional).

One transfer feels closer to what happens from my point-of-view. What
would be your definition of a transfer? Or the "official" one that the
sysfs entry represents?

> > 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.
>
> Credit where it's due - This is a very well written and verbose commit
> message.

Thanks!

> Just my personal opinion maybe but all this data about testing can go
> below the tear line in the description?

I see where you are coming from. I'll do so on the next revision (if
there is one).

> Or somewhere in the kernel docs would also be just fine. (I know we
> kernel developers consider git log as the best source of documentation
> :) ) but still.. if you feel like adding ;)

A first step would be to have the sysfs SPI statistics API be described
inside Documentation/. That is outside the scope of this patch
though. :-)

> No strong opinions there though.

Same.

[...]

> > +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;
> > +	int 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.
>
> Why 1 message _and_ 1 xfer and not simply 1 xfer?
> Even in the example of testing that you showed above the values for
> message and xfer are anyway going to be same, then why have these 2
> members in the first place? Can we not do away with one of these?

Mark Brown gave an answer to this. Indeed, with regular SPI operations,
one message doesn't map to one transfer.

[...]

> >  /**
> >   * spi_mem_exec_op() - Execute a memory operation
> >   * @mem: the SPI memory
> > @@ -339,8 +383,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);
> > +
>
> Just curious, how much does this impact performance? Have you been able
> to do some before / after profiling with/out this patch?
>
> For eg. for every single spimem op I'm constantly going to incur the
> penalty of these calls right?

I have indeed done some benchmarking. I was not able to measure anything
significant. Neither doing timings of end-to-end testing by reading
loads of data over UBIFS, nor by using ftrace's function_graph.

> Just wondering if we can / should make this optional to have the
> op_stats. If there is a perf penalty, like if my ospi operations start
> being impacted by these calls then I may not be okay with this patch.

I've asked myself the same question. It is being done unconditionally on
regular SPI ops, so I guess the question has been answered previously:
no need to make this conditional.

See spi_statistics_add_transfer_stats() in drivers/spi/spi.c.

> But if you have tested and not found it to be the case I am okay with
> these changes.
>
> If I find some time later, I'll try to test but I'm caught up with some
> other work. For now I'll leave my R-by with the above conditions
> addressed / answered.
>
> Mostly LGTM,
>
> Reviewed-by: Dhruva Gole <d-gole@xxxxxx>

Thanks for your review! I don't regret adding you to the Cc list.

Regards,

--
Théo Lebrun, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com