On Wed, Dec 22, 2010 at 12:40:02AM +0800, Ming Lei wrote:
Hi,
2010/12/21 Ming Lei <tom.leiming@xxxxxxxxx>:
Hi,
2010/12/21 Felipe Balbi <balbi@xxxxxx>:
Hi,
On Tue, Dec 21, 2010 at 07:27:10PM +0800, Ming Lei wrote:
OK, I will do a performance comparison between inline and function
pointers.
The comparisson should be between your version and struct musb_io_ops
version.
OK, I will do it.
1, test with musb cleanup v1 patches against 2.6.37-rc6-next-20101217
1), test case(running on host machine and musb working at gadget zero mode):
$sudo ./tu -D /proc/bus/usb/BUS/DEV -c 2048 -t 5 -s 32768 -g 8
2), result:
- cpu utilization: 22%~23%(observed by top)
- performance: 17MB/sec
3), code size
[tom@xxxxxxxxxxxxxx]$ size
../nfs/t/lib/modules/2.6.37-rc6-next-20101217+/kernel/drivers/usb/musb/*.ko
text data bss dec hex filename
6782 488 56 7326 1c9e drivers/usb/musb/am35x.ko
63799 948 28 64775 fd07 drivers/usb/musb/musb_hdrc.ko
3744 328 0 4072 fe8 kernel/drivers/usb/musb/musbhsdma.ko
6708 488 56 7252 1c54 drivers/usb/musb/omap2430.ko
11031 416 72 11519 2cff drivers/usb/musb/tusb6010.ko
6476 328 20 6824 1aa8 drivers/usb/musb/tusb6010_omap.ko
2, test with attachment patch against musb cleanup v1 patches plus
2.6.37-rc6-next-20101217
1), test case(running on host machine and musb working at gadget zero mode):
$sudo ./tu -D /proc/bus/usb/BUS/DEV -c 2048 -t 5 -s 32768 -g 8
2), result:
- cpu utilization: 23%~24%(observed by top)
- performance: 17MB/sec
3), code size
[tom@xxxxxxxxxxxxxx]$ size
../nfs/t/lib/modules/2.6.37-rc6-next-20101217+/kernel/drivers/usb/musb/*.ko
text data bss dec hex filename
7234 488 56 7778 1e62 drivers/usb/musb/am35x.ko
67329 948 44 68321 10ae1 drivers/usb/musb/musb_hdrc.ko
4296 328 0 4624 1210 drivers/usb/musb/musbhsdma.ko
7324 488 56 7868 1ebc drivers/usb/musb/omap2430.ko
12783 416 72 13271 33d7 drivers/usb/musb/tusb6010.ko
7236 328 20 7584 1da0 drivers/usb/musb/tusb6010_omap.ko
3, conclusion
- almost same performance wrt. usbtest #5
- a little higher cpu utilization if functions pointers to musb_read{w,l} and
musb_write{w,l} are taken, but the difference is not very obvious
- there is obvious difference in code size of modules, functions pointers
introduce larger code size both for caller and for callee
Seems we should avoid to use function pointer to musb_read{w,l} and
musb_write{w,l} as far as possible from above.
Any suggestions or objections?
BTW:
- Attachment patch is only for test purpose;
- DMA mode 0 is used in musb gadget OUT dir, which may consume
much more cpu than mode 1, so usbtest #5 is taken to evaluate the effect
of function pointers to musb_read{w,l} and musb_write{w,l}.
We don't want to involve the entire stack on this test. It's better that
you make a simple sysfs entry that will do something like:
int count = 100000;
u8 devctl;
/* don't forget locking */
while (--count)
devctl = musb_readb(musb->regs, MUSB_DEVCTL);
Then you use your function pointers and you can "emulate" the behavior
with struct musb_io_ops with something like below:
diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
index 07cf394..a1682fc 100644
--- a/drivers/usb/musb/musb_core.c
+++ b/drivers/usb/musb/musb_core.c
@@ -1709,6 +1709,24 @@ void musb_dma_completion(struct musb *musb, u8 epnum, u8 transmit)
#ifdef CONFIG_SYSFS
static ssize_t
+musb_readb_show(struct device *dev, struct device_attribute *attr, char *buf)
+{
+ struct musb *musb = dev_to_musb(dev);
+ unsigned long flags;
+ int count = 100000;
+ u8 devctl;
+
+ spin_lock_irqsave(&musb->lock, flags);
+ while(--count)
+ //devctl = musb_readb(musb->mregs, MUSB_DEVCTL);
+ devctl = musb->ops->io->musb_readb(musb->mregs, MUSB_DEVCTL);
+ spin_unlock_irqrestore(&musb->lock, flags);
+
+ return PAGE_SIZE;
+}
+static DEVICE_ATTR(readb, 0444, musb_readb_show, NULL);
+
+static ssize_t
musb_mode_show(struct device *dev, struct device_attribute *attr, char *buf)
{
struct musb *musb = dev_to_musb(dev);
@@ -1818,6 +1836,7 @@ static DEVICE_ATTR(srp, 0644, NULL, musb_srp_store);
#endif /* CONFIG_USB_GADGET_MUSB_HDRC */
static struct attribute *musb_attributes[] = {
+ &dev_attr_readb.attr,
&dev_attr_mode.attr,
&dev_attr_vbus.attr,
#ifdef CONFIG_USB_GADGET_MUSB_HDRC
diff --git a/drivers/usb/musb/musb_core.h b/drivers/usb/musb/musb_core.h
index d0c236f..7caefbc 100644
--- a/drivers/usb/musb/musb_core.h
+++ b/drivers/usb/musb/musb_core.h
@@ -253,6 +253,10 @@ enum musb_g_ep0_state {
/******************************** TYPES *************************************/
+struct musb_io_ops {
+ u8 (*musb_readb)(void __iomem *base, u8 reg);
+};
+
/**
* struct musb_platform_ops - Operations passed to musb_core by HW glue layer
* @init: turns on clocks, sets up platform-specific registers, etc
@@ -274,6 +278,8 @@ struct musb_platform_ops {
int (*vbus_status)(struct musb *musb);
void (*set_vbus)(struct musb *musb, int on);
+
+ const struct musb_io_ops *io;
};
/*
diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
index a3f1233..1db8642 100644
--- a/drivers/usb/musb/omap2430.c
+++ b/drivers/usb/musb/omap2430.c
@@ -46,6 +46,15 @@ struct omap2430_glue {
static struct timer_list musb_idle_timer;
+static u8 omap2430_readb(void __iomem *base, u8 reg)
+{
+ return __raw_readl(base + reg);
+}
+
+static const struct musb_io_ops omap2430_io_ops = {
+ .musb_readb = omap2430_readb,
+};
+
static void musb_do_idle(unsigned long _musb)
{
struct musb *musb = (void *)_musb;
@@ -377,6 +386,7 @@ static const struct musb_platform_ops omap2430_ops = {
.try_idle = omap2430_musb_try_idle,
.set_vbus = omap2430_musb_set_vbus,
+ .io = &omap2430_io_ops,
};
static u64 omap2430_dmamask = DMA_BIT_MASK(32);
Then you change the comment on musb_readb_show() to use the different
versions and, maybe, a simple "time cat
/sys/devices/platform/musb-omap2430/musb-hdrc/readb" will do to measure
the difference.
--
balbi
--
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
