On Tue, Apr 28, 2009 at 11:46:05AM -0400, Alan Stern wrote: > 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. It's ok for the ring to wrap around; I intended for it to be a lossy system. It's just a sampling tool. > 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. Right. So if the ring does wrap around, you might have more completes than there are items on the ring. Definitely not what I intended. > > --- 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? Sure. > > +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. Hmm, that was not what I intended. In all honesty, this was a fast patch to show some numbers for a demo (when I couldn't turn off debugging to use iomeasure or similar tools). I think I may just drop this set of patches from my queue. > 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. I think I understand your proposal. I think it has a flaw because the bus may be idle for a long period of time. If you keep the sum of the three transfer lengths and the start time for them, when the reader asks for the statistics after a long period of inactivity, they will receive falsely low statistics. Sarah -- 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
