Re: [PATCH 2/2] spi: add statistics gathering and reporting methods

[Martin Sperl <kernel@xxxxxxxxxxxxxxxx>]

> On 04.05.2015, at 15:42, Mark Brown <broonie@xxxxxxxxxx> wrote:
> 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.
that makes it much more complicated to add things per spi_master
implementation (which was my original idea: have a framework that exposes
the distinct transfer modes of the spi-bcm2835 driver (polling, interrupt
and dma driven) and also to get some stats on how often we run into those
“corner” cases - that is also why I want to gather the histogram
statistics, as it may make it easier to understand the use cases of people
who complain about “performance” in case they share those infos…

In those cases a single file is the easiest to share, but that is the way
it is with sysfs...

> 
>> transfer-len-log2-histogram: 0 33 3 9 1 2 0 0 0 0 0 0 0 30 0 0 0
> It would be more friendly to print this out as an actual histogram with
> pretty printing, that's definitely a debugfs thing though.
I could make it more readable or - if split - run it with the same format
as /sys/devices/virtual/block/ram0/stat - just multiple columns...

> 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.
> 
well - I found that it sometimes may help identify what device is responsible
for an issue on a mixed device bus.

> 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.
> 
I did not have it at first but was concerned that someone would complain
about missing locking. That is why I have minimized the spinlock in the
loop.

That is easy to remove again...

> 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.
just a bit more writing when counting twice…

OK, so I will rework the patch looking into the above and will now put
everything into one single patch...--
To unsubscribe from this list: send the line "unsubscribe linux-spi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html