Re: [patch 6/6] statistics infrastructure - exploitation: zfcp

[Martin Peschke <mp3@de.ibm.com>]

Arjan van de Ven wrote:

>On Thu, 2005-12-15 at 12:23 +0100, Martin Peschke wrote:
>
>  
>
>>Given the idea of struct statistic, the lower layer driver could use a 
>>given pointer to an upper layer's struct statistic in order to call 
>>statistic_inc(stat, x).
>>
>>The lower layer driver could call an upper layer driver's function to 
>>have the upper layer update a statistic. 
>>    
>>
>
>Why? it's an open source world, what you suggest is more something for a
>"must hide behind interfaces" closed world ;)
>  
>
Regarding the statistic_inc() & friends proposal, I see it as a handy 
abstraction that allows device driver programmers to worry about other 
things than reimplementing counters and other vehicles conveying 
statistics information.
If statistic_inc() would only be able to hide a single counter the value 
of which is just increased over time, I would agree with you.
But it implements several ways of data processing, including counters, 
fill level indicators (counters for total number of measurements, plus 
minimum, average, maximum), histograms (a set of counters for discrete 
values or ranges of values), as well as statistics that take the 
dimension time into account (for things like megabytes per seconds, or 
queue utilization per whatever-unit-of-time).

Regarding the "lower layer driver could call an upper layer driver's 
function" idea:
I didn't say I like all the listed alternatives the same. Actually I 
don't like this one.
I just wanted to be polite and discuss alternatives in order not to 
appear to be ignorant by simply insisting on my approach ;)

>if done right, the LLDD gets access to the transport class information,
>including the array of stats, so the LLDD can update those just fine.
>Just the API should be clear about who owns updating which field; a
>comment will suffice for that ;)
>  
>
Well, transport classes are fine for transport specific purposes.
I don't think that any statistic, particularly not request latencies and 
request sizes, belong there.

But I like the general idea hidden in what you say:
If done right, the layer that gathers statistics data gets access to the 
statistic, so that it can update this just fine.

>>The lower layer driver could temporarily store some measurement data in 
>>the data structure passed between those two; the upper layer driver 
>>picks it up later and calls whatever statistic library routine is 
>>appropriate. Requires additional bytes and one store/retrieve operation 
>>more than the struct statistic idea.
>>    
>>
>
>way way too complex for no reason.
>  
>
Agreed, another non-preferred way of doing it.

>Remember the scsi layer is a layered concept, but also upside down: even
>though the transport class layers on top of the LLDD, it's the LLDD that
>drives that class, not the other way around. The same could be done with
>selected statistics; have the transport layer do the exporting to sysfs
>and all that stuff, but have the LLDD keep track of them.
>
That's one of the major points of my patch: Have the statistics library 
do the exporting to the user interface, not the LLDD.

Debugfs has been my initial choice instead of sysfs because I don't 
think sysfs appropriate for exporting histograms and stuff.
But whether debugfs is the right choice, or relayfs, or sysfs, or 
something else is another question, I'd like to find an answer for in 
this thread.

If you are interested in the user interface question, this is a sample 
of what some statistics currently look like in a debugfs file:

  latencies_scsi_write <=0 0
  latencies_scsi_write <=1 0
  latencies_scsi_write <=2 0
  latencies_scsi_write <=4 174
  latencies_scsi_write <=8 872
  latencies_scsi_write <=16 2555
  latencies_scsi_write <=32 2483
  ...
  latencies_scsi_write <=1024 1872
  latencies_scsi_write >1024 1637
 
  latencies_scsi_read <=0 0
  latencies_scsi_read <=1 0
  latencies_scsi_read <=2 0
  latencies_scsi_read <=4 57265
  latencies_scsi_read <=8 13610
  latencies_scsi_read <=16 1082
  latencies_scsi_read <=32 319
  latencies_scsi_read <=64 63
  ...
  latencies_scsi_read >1024 0

  ...
  util_qdio_outb [3097394.211992] 865 1 1.052 5
  util_qdio_outb [3097395.211992] 737 1 4.558 125
  util_qdio_outb [3097396.211992] 396 1 11.765 77
  util_qdio_outb [3097397.211992] 270 1 12.863 128
  util_qdio_outb [3097398.211992] 765 1 7.271 26
  util_qdio_outb [3097399.211992] 577 1 4.036 27
  ...

>(of course
>only for those that are relevant in this sense; if the transport class
>is the natural place to update, then it should be done there)
>  
>
yes

Martin