On Mon, May 04, 2015 at 12:12:43PM +0000, kernel@xxxxxxxxxxxxxxxx wrote:
> From: Martin Sperl <kernel@xxxxxxxxxxxxxxxx>
>
> the statistics are available:
> for each master via:
> /sys/class/spi_master/spi*/stat
> for each device via:
> /sys/class/spi_master/spi32766/spi32766.4/stat
> /sys/bus/spi/devices/spi32766.*/stat
This is unsuitable for sysfs as it is, sysfs is strictly one value per
file. debugfs would be a more suitable place for a file like this, or
if they are going to go into sysfs they should go in as a directory of
files with one value per file like the network device statistics
directory. I'm ambivalent about which is the best place, both have
their merits.
> transfer-len-log2-histogram: 0 33 3 9 1 2 0 0 0 0 0 0 0 30 0 0 0
>
> Note that log2-histogram is defined for bin n as:
> spi_transfer.len in the range: [ 2^n : 2^(n+1) [
> with n=0 for spi_transfer.len = 0
It would be more friendly to print this out as an actual histogram with
pretty printing, that's definitely a debugfs thing though.
> +/* helper macros to help updating statistics (locked and unlocked) */
> +#define SPI_STATS_FIELD_ADD_NOLOCK(master, spi, key, value) \
> + do { \
> + master->stats.key += value; \
> + spi->stats.key += value; \
> + } while (0)
It's a bit unclear to me that we need to collect the stats on both a
per device and a per master basis, if we've got device statistics we can
add up the various devices to get the master values easily enough when
we need them. It does mean that removing devices would discard some of
the statistics but it's not clear to me that this is actually going to
concern anyone.
It'd be good to have a better name too - _FIELD_ADD makes it sound like
it's adding a new field which is obviously not what we're doing here.
Perhaps just SPI_STATS_ADD?
> +#define SPI_STATS_FIELD_ADD(master, spi, key, value) \
> + do { \
> + unsigned long flags; \
> + \
> + spin_lock_irqsave(&master->stats_spinlock, flags); \
> + SPI_STATS_FIELD_ADD_NOLOCK(master, spi, key, value); \
> + spin_unlock_irqrestore(&master->stats_spinlock, flags); \
> + } while (0)
Do we really need the locked version with a separate lock? I know
you're very concerned about performance for some of your applications
and the fact that we're going to be spending a lot of time taking a
separate spinlock (with interrupt disabling... ) seems like it's going
to be adding overhead we could live without.
> + /* update statistics */
> + l2size = (xfer->len) ? fls(xfer->len - 1) + 1 : 0;
> + l2size = min(l2size, SPI_STATISTICS_L2HISTO_SIZE - 1);
> +
> + spin_lock_irqsave(&master->stats_spinlock, flags);
> + SPI_STATS_FIELD_ADD_NOLOCK(master, msg->spi, transfers, 1);
> + SPI_STATS_FIELD_ADD_NOLOCK(master, msg->spi,
> + bytes, xfer->len);
> + if (xfer->tx_buf)
> + SPI_STATS_FIELD_ADD_NOLOCK(master, msg->spi,
> + bytes_tx, xfer->len);
> + if (xfer->rx_buf)
> + SPI_STATS_FIELD_ADD_NOLOCK(master, msg->spi,
> + bytes_rx, xfer->len);
> + SPI_STATS_FIELD_ADD_NOLOCK(master, msg->spi,
> + bytes_l2histo[l2size], 1);
> + spin_unlock_irqrestore(&master->stats_spinlock, flags);
I can't help but feel that this would be a bit more legible without the
macro for adding to a field - just a function that takes the transfer
and a stats buffer to add to.
Attachment:
signature.asc
Description: Digital signature
