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

[Dhruva Gole]

Hi,

On Feb 12, 2024 at 15:22:41 +0100, Théo Lebrun wrote:
> 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?

Yeah I understand your point, this is something I'd also call as a transaction

> 
> > > 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).

cool!

> 
> > 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.

Thanks for explaining Mark, understood.

> 
> [...]
> 
> > >  /**
> > >   * 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.

Awesome.

> 
> > 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.

Yeah I did see that, but maybe it didn't occur then whether we should
make it optional.

Anyway, I'm ok with this too. Let's worry about optional in future if
required.

> 
> > 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.
> 

Cheers!


-- 
Best regards,
Dhruva Gole <d-gole@xxxxxx>