On Mon, 27 Apr 2009, Sarah Sharp wrote: > This patch provides a generic way for USB host controllers (and potentially > other drivers) to report throughput statistics. The caller passes in a dentry > for a debugging directory, and the code sets up a file to report the statistics > under that. > > Userspace can cat that file to receive a pair of numbers that represent the > amount of data transferred (in bytes) and the time it took to send that data > (using ktime_t and ktime_get()). The file will block if no statistics are > available. When the driver has throughput data, it calls hcd_stat_update(), > which will wakeup the blocked reader thread. > > These statistics can be piped through a simple script to sample the data and > feed the traffic data into a real-time graphing program, such as Trend. > http://www.thregr.org/~wavexx/software/trend/ > > The script I used to gather EHCI and xHCI statistics can be found at > http://minilop.net/~sarah/trend-sample.c > > Special thanks to Jamey Sharp for writing this sampling code! In addition to the issues raised by David, I don't see how this can possibly work as it stands. For one thing, there's almost no synchronization between readers and writers. What's to prevent the ring buffer from wrapping around? This certainly will happen if more than 20 URBs are submitted before the stats file is opened. Remember also that struct completions use a counting mechanism. If you call complete() N times in a row then the next N wait_for_completion() calls will return immediately. > --- a/drivers/usb/Makefile > +++ b/drivers/usb/Makefile > @@ -20,6 +20,7 @@ obj-$(CONFIG_USB_U132_HCD) += host/ > obj-$(CONFIG_USB_R8A66597_HCD) += host/ > obj-$(CONFIG_USB_HWA_HCD) += host/ > obj-$(CONFIG_USB_ISP1760_HCD) += host/ > +obj-$(CONFIG_USB_HCD_STAT) += host/ I suspect this line isn't really needed. If no host controller drivers are configured, what good will the stats do? > +void hcd_stat_update(struct hcd_stat *dbg_stat, int len, ktime_t delta) > +{ > + unsigned int write_ptr = dbg_stat->write_ptr; > + > + dbg_stat->len[write_ptr] = len; > + dbg_stat->delta[write_ptr] = delta; > + dbg_stat->write_ptr = (write_ptr + 1) % BUFFER_SIZE; > + complete(&dbg_stat->comp); > +} > +EXPORT_SYMBOL_GPL(hcd_stat_update); No information should be retained if the stats file isn't open, although this is a minor point. See below... > +/* Block the read if there's no statistics to return. > + * Yes, we could just return zero if there's no data. We'd rather block the > + * input stream so that the CPU spends less cycles on passing back and forth > + * zeros if the user is streaming this data to a traffic chart. > + */ > +static ssize_t hcd_stat_in_read(struct file *file, char __user *buf, > + size_t len, loff_t *offset) > +{ > + unsigned int read_ptr; > + struct hcd_stat *dbg_stat; > + wait_queue_head_t write_event; > + unsigned int buffer_len; > + > + init_waitqueue_head(&write_event); > + dbg_stat = (struct hcd_stat *) file->private_data; > + read_ptr = dbg_stat->read_ptr; > + wait_for_completion_interruptible( > + &dbg_stat->comp); > + buffer_len = snprintf(buf, len, "%d %lld\n", > + dbg_stat->len[read_ptr], > + ktime_to_ns(dbg_stat->delta[read_ptr])); > + > + if (dbg_stat->len[read_ptr] == 0 && > + ktime_to_ns(dbg_stat->delta[read_ptr]) == 0) > + return buffer_len; According to the comment above, you don't return zero if there is no data available. So what's the point of this test? To prevent errors from wrap-around? Why not block instead of returning the 0s? > + > + dbg_stat->len[read_ptr] = 0; > + dbg_stat->delta[read_ptr] = ktime_set(0, 0); > + dbg_stat->read_ptr = (read_ptr + 1) % BUFFER_SIZE; > + return buffer_len; > +} Beyond these details, this design has a serious conceptual error. Suppose you want to measure the throughput of a 30-KB transfer, and the transfer is broken up into three 10-KB URBs. It's not unlikely that all three will be submitted at once (that's how the scatter-gather library works, for example). Let's suppose the actual bus throughput is 1 KB/ms (typical of full speed). The delta times are measured from submission to completion. As a result, your statistics file will report: First read: Transfer length of URB 1 (10 KB) and transfer time of URB 1 (10 ms). Second read: Transfer length of URB 2 (10 KB) and combined transfer times of URBs 1 and 2 (20 ms). Third read: Transfer length of URB 3 (10 KB) and combined transfer times of URBs 1, 2, and 3 (30 ms). Surely that isn't what you really want, is it? The reader would calculate estimated throughputs of 1 KB/ms, 0.5 KB/ms, and 0.3 KB/ms, for an average estimated throughput of 0.6 KB/ms. What you really want to do is accumulate lengths as URBs complete, and reset the total to 0 (and reset the start time) after every read. Which means you don't need a ring buffer at all. And you don't need to store a delta time, only a start time. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
