statistics infrastructure - exploitation: zfcp
Subsystems (3)
PATCH 6/6 statistics infrastructure - exploitation: zfcp
[patch 6/6] statistics infrastructure - exploitation: zfcp
This patch instruments the zfcp driver and makes it feed statistics data into the statistics infrastructure.
---
Makefile | 4 - zfcp_aux.c | 22 ++++++ zfcp_ccw.c | 7 ++ zfcp_def.h | 31 ++++++++- zfcp_erp.c | 4 + zfcp_ext.h | 6 + zfcp_fsf.c | 60 +++++++++++++++++- zfcp_qdio.c | 5 + zfcp_scsi.c | 14 ++++ zfcp_stat.c | 195 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++ 10 files changed, 338 insertions(+), 10 deletions(-)
drivers/s390/scsi/Makefile +2 -2
@@ -3,7 +3,7 @@ # zfcp-objs := zfcp_aux.o zfcp_ccw.o zfcp_scsi.o zfcp_erp.o zfcp_qdio.o \ - zfcp_fsf.o zfcp_dbf.o zfcp_sysfs_adapter.o zfcp_sysfs_port.o \ - zfcp_sysfs_unit.o zfcp_sysfs_driver.o + zfcp_fsf.o zfcp_dbf.o zfcp_stat.o zfcp_sysfs_adapter.o \ + zfcp_sysfs_port.o zfcp_sysfs_unit.o zfcp_sysfs_driver.o obj-$(CONFIG_ZFCP) += zfcp.o
drivers/s390/scsi/zfcp_aux.c +18 -2
@@ -778,15 +778,20 @@ zfcp_unit_enqueue(struct zfcp_port *port unit->sysfs_device.release = zfcp_sysfs_unit_release;dev_set_drvdata (&unit->sysfs_device, unit); +if (zfcp_unit_statistic_register (unit)) +return NULL ; +/* mark unit unusable as long as sysfs registration is not complete */ atomic_set_mask (ZFCP_STATUS_COMMON_REMOVE , &unit->status);if (device_register (&unit->sysfs_device)) { +zfcp_unit_statistic_unregister (unit);kfree (unit);return NULL ; }if (zfcp_sysfs_unit_create_files (&unit->sysfs_device)) { +zfcp_unit_statistic_unregister (unit);device_unregister (&unit->sysfs_device);return NULL ; }
@@ -826,6 +831,7 @@ zfcp_unit_dequeue(struct zfcp_unit *unitlist_del (&unit->list);write_unlock_irq (&zfcp_data.config_lock); unit->port->units--; +zfcp_unit_statistic_unregister (unit);zfcp_port_put (unit->port);zfcp_sysfs_unit_remove_files (&unit->sysfs_device);device_unregister (&unit->sysfs_device);
@@ -837,6 +843,16 @@ zfcp_mempool_alloc(gfp_t gfp_mask, voidreturn kmalloc ((size_t ) size, gfp_mask); } +static void * +zfcp_mempool_alloc_fsf_req_scsi (unsigned int __nocast gfp_mask,void *data) +{ +struct zfcp_adapter *adapter = (struct zfcp_adapter *)data; +void *ptr =kmalloc (sizeof (struct zfcp_fsf_req_pool_element ), gfp_mask); +if (!ptr) +statistic_inc (adapter->stat_low_mem_scsi,0 ); +return ptr; +} +static void zfcp_mempool_free (void *element,void *size) {
@@ -864,8 +880,8 @@ zfcp_allocate_low_mem_buffers(struct zfc adapter->pool.fsf_req_scsi =mempool_create (ZFCP_POOL_FSF_REQ_SCSI_NR , - zfcp_mempool_alloc, zfcp_mempool_free, (void *) -sizeof (struct zfcp_fsf_req_pool_element )); + zfcp_mempool_alloc_fsf_req_scsi, + zfcp_mempool_free, (void *)adapter);if (NULL == adapter->pool.fsf_req_scsi)return -ENOMEM ;
drivers/s390/scsi/zfcp_ccw.c +7 -0
@@ -163,6 +163,10 @@ zfcp_ccw_set_online(struct ccw_device *c retval =zfcp_adapter_debug_register (adapter);if (retval) goto out; + retval =zfcp_adapter_statistic_register (adapter); +if (retval) + goto out_stat_create; + retval =zfcp_erp_thread_setup (adapter);if (retval) { ZFCP_LOG_INFO("error: start of error recovery thread for "
@@ -183,6 +187,8 @@ zfcp_ccw_set_online(struct ccw_device *c out_scsi_register:zfcp_erp_thread_kill (adapter); out_erp_thread: +zfcp_adapter_statistic_unregister (adapter); + out_stat_create:zfcp_adapter_debug_unregister (adapter); out:up (&zfcp_data.config_sema);
@@ -209,6 +215,7 @@ zfcp_ccw_set_offline(struct ccw_device *zfcp_erp_wait (adapter);zfcp_adapter_scsi_unregister (adapter);zfcp_erp_thread_kill (adapter); +zfcp_adapter_statistic_unregister (adapter);zfcp_adapter_debug_unregister (adapter);up (&zfcp_data.config_sema);return 0 ;
drivers/s390/scsi/zfcp_def.h +29 -2
@@ -58,6 +58,8 @@#include <asm/qdio.h> #include <asm/debug.h> #include <asm/ebcdic.h> +#include <asm/timex.h> +#include <linux/statistic.h> #include <linux/mempool.h> #include <linux/syscalls.h> #include <linux/ioctl.h>
@@ -66,7 +68,7 @@/********************* GENERAL DEFINES *********************************/ /* zfcp version number, it consists of major, minor, and patch-level number */ -#define ZFCP_VERSION "4.5.0" +#define ZFCP_VERSION "4.6.0"/** * zfcp_sg_to_address - determine kernel address from struct scatterlist
@@ -978,6 +980,12 @@ struct zfcp_adapter {struct zfcp_adapter_mempool pool;/* Adapter memory pools */ struct qdio_initialize qdio_init_data;/* for qdio_establish */ struct device generic_services;/* directory for WKA ports */ +struct statistic_interface *stat_if; +struct statistic *stat_qdio_outb_full; +struct statistic *stat_qdio_outb; +struct statistic *stat_qdio_inb; +struct statistic *stat_low_mem_scsi; +struct statistic *stat_erp; };/*
@@ -1024,6 +1032,24 @@ struct zfcp_unit {struct scsi_device *device;/* scsi device struct pointer */ struct zfcp_erp_action erp_action;/* pending error recovery */ atomic_t erp_counter; +atomic_t read_num; +atomic_t write_num; +struct statistic_interface *stat_if; +struct statistic *stat_sizes_scsi_write; +struct statistic *stat_sizes_scsi_read; +struct statistic *stat_sizes_scsi_nodata; +struct statistic *stat_sizes_scsi_nofit; +struct statistic *stat_sizes_scsi_nomem; +struct statistic *stat_sizes_timedout_write; +struct statistic *stat_sizes_timedout_read; +struct statistic *stat_sizes_timedout_nodata; +struct statistic *stat_latencies_scsi_write; +struct statistic *stat_latencies_scsi_read; +struct statistic *stat_latencies_scsi_nodata; +struct statistic *stat_pending_scsi_write; +struct statistic *stat_pending_scsi_read; +struct statistic *stat_erp; +struct statistic *stat_eh_reset; };/* FSF request */
@@ -1050,7 +1076,8 @@ struct zfcp_fsf_req {mempool_t *pool;/* used if request was alloacted from emergency pool */ unsigned long long issued;/* request sent time (STCK) */ -struct zfcp_unit *unit; +unsigned long long received; +struct zfcp_unit *unit; };typedef void zfcp_fsf_req_handler_t (struct zfcp_fsf_req *);
drivers/s390/scsi/zfcp_erp.c +4 -0
@@ -1624,10 +1624,12 @@ zfcp_erp_strategy_check_unit(struct zfcpswitch (result) {case ZFCP_ERP_SUCCEEDED :atomic_set (&unit->erp_counter,0 ); +statistic_inc (unit->stat_erp,1 );zfcp_erp_unit_unblock (unit);break ;case ZFCP_ERP_FAILED :atomic_inc (&unit->erp_counter); +statistic_inc (unit->stat_erp,-1 );if (atomic_read (&unit->erp_counter) >ZFCP_MAX_ERPS )zfcp_erp_unit_failed (unit);break ;
@@ -1695,10 +1697,12 @@ zfcp_erp_strategy_check_adapter(struct zswitch (result) {case ZFCP_ERP_SUCCEEDED :atomic_set (&adapter->erp_counter,0 ); +statistic_inc (adapter->stat_erp,1 );zfcp_erp_adapter_unblock (adapter);break ;case ZFCP_ERP_FAILED :atomic_inc (&adapter->erp_counter); +statistic_inc (adapter->stat_erp,-1 );if (atomic_read (&adapter->erp_counter) >ZFCP_MAX_ERPS )zfcp_erp_adapter_failed (adapter);break ;
drivers/s390/scsi/zfcp_ext.h +6 -0
@@ -203,4 +203,10 @@ extern void zfcp_scsi_dbf_event_abort(coextern void zfcp_scsi_dbf_event_devreset (const char *,u8 ,struct zfcp_unit *,struct scsi_cmnd *); +/*************************** stat ********************************************/ +extern int zfcp_adapter_statistic_register (struct zfcp_adapter *); +extern int zfcp_adapter_statistic_unregister (struct zfcp_adapter *); +extern int zfcp_unit_statistic_register (struct zfcp_unit *); +extern int zfcp_unit_statistic_unregister (struct zfcp_unit *); +#endif /* ZFCP_EXT_H */
drivers/s390/scsi/zfcp_fsf.c +56 -4
118 lines not on this page · raw message
drivers/s390/scsi/zfcp_qdio.c +5 -0
@@ -418,6 +418,7 @@ zfcp_qdio_response_handler(struct ccw_de }else { queue->free_index += count; queue->free_index %=QDIO_MAX_BUFFERS_PER_Q ; +statistic_inc (adapter->stat_qdio_inb, count);atomic_set (&queue->free_count,0 );ZFCP_LOG_TRACE ("%i buffers enqueued to response " "queue at position %i\n" , count, start);
@@ -662,6 +663,10 @@ zfcp_qdio_sbals_from_segment(struct zfcp/* get next free SBALE for new piece */ if (NULL ==zfcp_qdio_sbale_next (fsf_req, sbtype)) {/* no SBALE left, clean up and leave */ +statistic_inc ( + fsf_req->adapter->stat_qdio_outb_full, +atomic_read ( + &fsf_req->adapter->request_queue.free_count));zfcp_qdio_sbals_wipe (fsf_req);return -EINVAL ; }
drivers/s390/scsi/zfcp_scsi.c +14 -0
@@ -449,6 +449,16 @@ zfcp_scsi_eh_abort_handler(struct scsi_cZFCP_LOG_INFO ("aborting scsi_cmnd=%p on adapter %s\n" , scpnt,zfcp_get_busid_by_adapter (adapter)); +if (scpnt->sc_data_direction ==DMA_TO_DEVICE ) +statistic_inc (unit->stat_sizes_timedout_write, + scpnt->request_bufflen); +else if (scpnt->sc_data_direction ==DMA_FROM_DEVICE ) +statistic_inc (unit->stat_sizes_timedout_read, + scpnt->request_bufflen); +else +statistic_inc (unit->stat_sizes_timedout_nodata, + scpnt->request_bufflen); +/* avoid race condition between late normal completion and abort */ write_lock_irqsave (&adapter->abort_lock, flags);
@@ -538,12 +548,14 @@ zfcp_scsi_eh_device_reset_handler(structatomic_set_mask (ZFCP_STATUS_UNIT_NOTSUPPUNITRESET , &unit->status); +statistic_inc (unit->stat_eh_reset,-1 );/* fall through and try 'target reset' next */ }else {ZFCP_LOG_DEBUG ("unit reset succeeded (unit=%p)\n" , unit);/* avoid 'target reset' */ retval =SUCCESS ; +statistic_inc (unit->stat_eh_reset,1 ); goto out; } }
@@ -551,9 +563,11 @@ zfcp_scsi_eh_device_reset_handler(structif (retval) {ZFCP_LOG_DEBUG ("target reset failed (unit=%p)\n" , unit); retval =FAILED ; +statistic_inc (unit->stat_eh_reset,-2 ); }else {ZFCP_LOG_DEBUG ("target reset succeeded (unit=%p)\n" , unit); retval =SUCCESS ; +statistic_inc (unit->stat_eh_reset,2 ); } out:return retval;
drivers/s390/scsi/zfcp_stat.c +195 -0
195 lines not on this page · raw message
drivers/s390/scsi/zfcp_aux.c zfcp_unit_enqueue(struct zfcp_port *port · line 797
Quoted 11 lines · Martin Peschke
drivers/s390/scsi/zfcp_aux.c +2
-0if (device_register (&unit->sysfs_device)) { +zfcp_unit_statistic_unregister (unit);kfree (unit);return NULL ; }if (zfcp_sysfs_unit_create_files (&unit->sysfs_device)) { +zfcp_unit_statistic_unregister (unit);device_unregister (&unit->sysfs_device);return NULL ; }
Unrelated, but doesn't that error path forget to release unit?
Quoted 8 lines · Matthew Wilcox
Quoted 6 lines · Martin Peschke
if (device_register(&unit->sysfs_device)) {(quoted diff) +1
-0+ zfcp_unit_statistic_unregister(unit);kfree(unit); return NULL; }
Quoted 10 lines
Quoted 5 lines
if (zfcp_sysfs_unit_create_files(&unit->sysfs_device)) {(quoted diff) +1
-0+ zfcp_unit_statistic_unregister(unit);device_unregister(&unit->sysfs_device); return NULL; }Unrelated, but doesn't that error path forget to release unit?
correct, I guess ... Andreas, could you fix this?
Quoted 12 lines
Quoted 11 lines
Quoted 10 lines
if (device_register(&unit->sysfs_device)) {(quoted diff) +1
-0+ zfcp_unit_statistic_unregister(unit);kfree(unit); return NULL; } if (zfcp_sysfs_unit_create_files(&unit->sysfs_device)) {(quoted diff) +1
-0+ zfcp_unit_statistic_unregister(unit);device_unregister(&unit->sysfs_device); return NULL; }Unrelated, but doesn't that error path forget to release unit?
Quoted 1 line
correct, I guess ... Andreas, could you fix this?
The release function frees the unit. Nothing to fix here.
Heiko
Quoted 8 lines
drivers/s390/scsi/zfcp_def.h +8
-0+atomic_t read_num; +atomic_t write_num; +struct statistic_interface *stat_if; +struct statistic *stat_sizes_scsi_write; +struct statistic *stat_sizes_scsi_read; +struct statistic *stat_sizes_scsi_nodata; +struct statistic *stat_sizes_scsi_nofit; +struct statistic *stat_sizes_scsi_nomem;
drivers/s390/scsi/zfcp_def.h struct zfcp_unit { · line 1052
Quoted 10 lines
drivers/s390/scsi/zfcp_def.h +10
-0+struct statistic *stat_sizes_timedout_write; +struct statistic *stat_sizes_timedout_read; +struct statistic *stat_sizes_timedout_nodata; +struct statistic *stat_latencies_scsi_write; +struct statistic *stat_latencies_scsi_read; +struct statistic *stat_latencies_scsi_nodata; +struct statistic *stat_pending_scsi_write; +struct statistic *stat_pending_scsi_read; +struct statistic *stat_erp; +struct statistic *stat_eh_reset;
NACK. pretty much all of this is generic and doesn't belong into an LLDD. We already had this statistics things with emulex and they added various bits to the core in response.
Quoted 16 lines · Christoph Hellwig
Quoted 16 lines
drivers/s390/scsi/zfcp_def.h +16
-0+atomic_t read_num; +atomic_t write_num; +struct statistic_interface *stat_if; +struct statistic *stat_sizes_scsi_write; +struct statistic *stat_sizes_scsi_read; +struct statistic *stat_sizes_scsi_nodata; +struct statistic *stat_sizes_scsi_nofit; +struct statistic *stat_sizes_scsi_nomem; +struct statistic *stat_sizes_timedout_write; +struct statistic *stat_sizes_timedout_read; +struct statistic *stat_sizes_timedout_nodata; +struct statistic *stat_latencies_scsi_write; +struct statistic *stat_latencies_scsi_read; +struct statistic *stat_latencies_scsi_nodata; +struct statistic *stat_pending_scsi_write; +struct statistic *stat_pending_scsi_read;
Quoted 12 lines
drivers/s390/scsi/zfcp_def.h struct zfcp_unit { · line 1052
Quoted 4 lines
drivers/s390/scsi/zfcp_def.h +2
-0+struct statistic *stat_erp; +struct statistic *stat_eh_reset;NACK. pretty much all of this is generic and doesn't belong into an LLDD. We already had this statistics things with emulex and they added various bits to the core in response.
Agreed. It's not necessarily up to LLDDs to keep track of request sizes, request latencies, I/O queue utilization, and error recovery conditions by means of statistics. This could or maybe should be done in a more central spot.
With regard to latencies, it might make some difference, though, how many layers are in between that cause additional delays. Then the question is which latency one wants to measure.
There is some very basic measurement data on FC-4 or FCP level in the FC transport class code:
&class_device_attr_host_fcp_input_requests.attr, &class_device_attr_host_fcp_output_requests.attr, &class_device_attr_host_fcp_control_requests.attr, &class_device_attr_host_fcp_input_megabytes.attr, &class_device_attr_host_fcp_output_megabytes.attr, &class_device_attr_host_reset_statistics.attr,
Looks like
- counters for the number of read and write requests and requests without any data
- counters for the number of megabytes read and written
- a counter for one out of several recovery conditions
The gap between the statistics posted by me and the FCP transport statistics is
- no information about the actual traffic pattern generated by Linux (no information - e.g. histogram - about request size,
no information - e.g. histogram - about latencies)
- no information about command timeouts
- no information about I/O concurrency caused by TCQ
I am not sure whether the transport statistics refer to the overall utilization of an actual FC port - let's call it physical HBA -, or to the share of an FC port utilized by one out of several sharing OS instances - let's call these shares, carved out of an FC port, virtual HBAs.
I won't object to move some stuff. But neither think I that a transport class would be the right place for latencies etc. nor would I like to give up certain functionality, like histograms. Would it be fine with you to move such statistics to the scsi mid layer, provided I can get lkml's approval for some form of a generic statistic code as it comes with my zfcp patch?
Martin
Quoted 30 lines · Martin Peschke
Quoted 16 lines · Christoph Hellwig
Quoted 16 lines
drivers/s390/scsi/zfcp_def.h +16
-0+atomic_t read_num; +atomic_t write_num; +struct statistic_interface *stat_if; +struct statistic *stat_sizes_scsi_write; +struct statistic *stat_sizes_scsi_read; +struct statistic *stat_sizes_scsi_nodata; +struct statistic *stat_sizes_scsi_nofit; +struct statistic *stat_sizes_scsi_nomem; +struct statistic *stat_sizes_timedout_write; +struct statistic *stat_sizes_timedout_read; +struct statistic *stat_sizes_timedout_nodata; +struct statistic *stat_latencies_scsi_write; +struct statistic *stat_latencies_scsi_read; +struct statistic *stat_latencies_scsi_nodata; +struct statistic *stat_pending_scsi_write; +struct statistic *stat_pending_scsi_read;Quoted 12 lines
drivers/s390/scsi/zfcp_def.h struct zfcp_unit { · line 1052
Quoted 4 lines
drivers/s390/scsi/zfcp_def.h +2
-0+struct statistic *stat_erp; +struct statistic *stat_eh_reset;NACK. pretty much all of this is generic and doesn't belong into an LLDD. We already had this statistics things with emulex and they added various bits to the core in response.
Quoted 8 lines
Agreed. It's not necessarily up to LLDDs to keep track of request sizes, request latencies, I/O queue utilization, and error recovery conditions by means of statistics. This could or maybe should be done in a more central spot.
With regard to latencies, it might make some difference, though, how many layers are in between that cause additional delays. Then the question is which latency one wants to measure.
even if the LLDD measures these, the stats belong a level up, so that all LLDD's export the same. I think you got half of Christophs point, but not this last bit: even when it's the LLDD that needs to measure the stat, it still shouldn't be LLDD specific, and thus defined one if not two layers up.
Quoted 44 lines · Arjan van de Ven
Quoted 35 lines · Martin Peschke
Quoted 17 lines · Christoph Hellwig
Quoted 17 lines
drivers/s390/scsi/zfcp_def.h +17
-0+atomic_t read_num; +atomic_t write_num; +struct statistic_interface *stat_if; +struct statistic *stat_sizes_scsi_write; +struct statistic *stat_sizes_scsi_read; +struct statistic *stat_sizes_scsi_nodata; +struct statistic *stat_sizes_scsi_nofit; +struct statistic *stat_sizes_scsi_nomem; +struct statistic *stat_sizes_timedout_write; +struct statistic *stat_sizes_timedout_read; +struct statistic *stat_sizes_timedout_nodata; +struct statistic *stat_latencies_scsi_write; +struct statistic *stat_latencies_scsi_read; +struct statistic *stat_latencies_scsi_nodata; +struct statistic *stat_pending_scsi_write; +struct statistic *stat_pending_scsi_read; +struct statistic *stat_erp;Quoted 14 lines
drivers/s390/scsi/zfcp_def.h struct zfcp_unit { · line 1052
Quoted 5 lines
drivers/s390/scsi/zfcp_def.h +1
-0+struct statistic *stat_eh_reset;NACK. pretty much all of this is generic and doesn't belong into an LLDD. We already had this statistics things with emulex and they added various bits to the core in response.
Quoted 5 lines
Agreed. It's not necessarily up to LLDDs to keep track of request sizes, request latencies, I/O queue utilization, and error recovery conditions by means of statistics. This could or maybe should be done in a more central spot.
With regard to latencies, it might make some difference, though, how many layers are in between that cause additional delays. Then the question is which latency one wants to measure.
Quoted 8 lines
even if the LLDD measures these, the stats belong a level up, so that all LLDD's export the same. I think you got half of Christophs point, but not this last bit: even when it's the LLDD that needs to measure the stat, it still shouldn't be LLDD specific, and thus defined one if not two layers up.
Ah, I see. It makes sense to avoid multiple places where to look for latencies, for example. Several ways to accomplish this come to mind:
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. This causes a proliferation of such functions (one upper layer function per statistic class). Since control goes back and force between upper and lower layer drivers anyway, adding another call to the backchain doesn't seem to be the most efficient way. Not sure an addional indirect function call to the layer actually owning a particular statistic could be avoided in any case (depends on interface between the two layers).
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.
Quoted 6 lines · Martin Peschke
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 ;)
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 ;)
Quoted 5 lines
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.
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. (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)
Quoted 4 lines · Arjan van de Ven
On Thu, 2005-12-15 at 12:23 +0100, Martin Peschke wrote:
Quoted 10 lines
Quoted 5 lines
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 ;)
Quoted 6 lines
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 6 lines
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
...
Quoted 5 lines
(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