Re: [U-Boot] usb_test_unit_ready called every block read - performance
Hi Marek,
I looked at the ext4 branch. It looks like he has the patch to remove the
usb_test_unit_ready() calls which were not needed. Actually those calls are
commented out on that branch:
#if 0
if (usb_test_unit_ready(srb, ss)) {
printf("Device NOT ready\n Request Sense returned %02X
%02X"
" %02X\n", srb->sense_buf[2], srb->sense_buf[12],
srb->sense_buf[13]);
return 0;
}
#endif
In the u-boot-usb.git, this code is removed so at some point there will be
a merge conflict.
Also the ext4 branch still has the mdelay(5) always being done in
usb_stor_BBB_transport() line 696 which we found to be the largest
performance killer.
Regards,
Jim
On Sun, Aug 12, 2012 at 7:54 PM, Marek Vasut wrote:
> Dear Jim Shimer,
>
> > While tuning ext2load, we found that usb_test_unit_ready was being called
> > every block read. We compared the usb block storage to the scsi block
> > storage cmd_scsi.c, and found that the scsi device was only calling its
> > scsi_setup_test_unit_ready() during scsi_can. It appears that
> > usb_test_unit_ready() really only needs to be called once during
> > usb_stor_scan(), via usb_stor_get_info(). Is there a particular reason
> > usb_test_unit_ready is called for every block read, or do you think its
> ok
> > to only call during usb_stor_scan()? We're finding this speeds up
> ext2load
> > quite a bit.
>
> Jim, did we get anywhere on this one ? Can you try with the new ext4 code
> in
> Wolfgangs' u-boot-master/ext4 branch?
>
> > Regards,
> > Jim
>
> Best regards,
> Marek Vasut
>
--
*James H Shimer*
Motorola Mobility T3-12-HH72
900 Chelmsford Street
Lowell MA 08151
978-614-3550
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] usb_test_unit_ready called every block read - performance
Marek/Benoit, Thanks so much for integrating this. I like the way you reused the flags, and simplified the code. Regards, Jim On Tue, Aug 14, 2012 at 1:57 PM, Steve Heckman wrote: > Marek, > > That looks good to me. Jim? > > Thanks, > Steve > > On Tue, Aug 14, 2012 at 1:50 PM, Marek Vasut wrote: > >> Dear Jim Shimer, >> >> > Hi Marek, >> > >> > Here's a patch. >> > >> > -Jim >> [...] >> >> I applied modified patch, please check u-boot-usb.git if you agree. >> >> btw please read http://www.denx.de/wiki/U-Boot/Patches >> >> Best regards, >> Marek Vasut >> > > -- *James H Shimer* Motorola Mobility T3-12-HH72 900 Chelmsford Street Lowell MA 08151 978-614-3550 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] usb_test_unit_ready called every block read - performance
Hi Marek, Here's a patch. -Jim On Mon, Jul 30, 2012 at 7:41 PM, Marek Vasut wrote: > Dear Jim Shimer, > > > Marek, > > > > I don't have a USB 1.x device to do that regression. Taking the call to > > usb_test_unit_ready out of the block reads/writes and leaving it only in > > get info really speed up ext2load. With the recent cache changes on top > of > > this and the 5ms delay, we're getting fairly close to the Linux USB > > transfer times and are happy with the results. Can you look into > > integrating this? > > Can you send a patch that'll yank it out? I should be able to test it in a > bit. > > > -Jim > > > > On Mon, Jul 30, 2012 at 6:54 PM, Marek Vasut wrote: > > > Dear Jim Shimer, > > > > > > > While tuning ext2load, we found that usb_test_unit_ready was being > > > > called every block read. We compared the usb block storage to the > > > > scsi block storage cmd_scsi.c, and found that the scsi device was > only > > > > calling its scsi_setup_test_unit_ready() during scsi_can. It appears > > > > that usb_test_unit_ready() really only needs to be called once during > > > > usb_stor_scan(), via usb_stor_get_info(). Is there a particular > > > > reason usb_test_unit_ready is called for every block read, or do you > > > > think its > > > > > > ok > > > > > > > to only call during usb_stor_scan()? We're finding this speeds up > > > > > > ext2load > > > > > > > quite a bit. > > > > > > Could it be because the USB is actually quite slower than SCSI and the > > > device > > > might get congested? It seems the code was there ever since (I can't > seem > > > to > > > find the origin of it) and we might actually want to try if this > doesn't > > > break > > > some usb 1.x things. > > > > > > > Regards, > > > > Jim > > > > > > Best regards, > > > Marek Vasut > > Best regards, > Marek Vasut > -- *James H Shimer* Motorola Mobility T3-12-HH72 900 Chelmsford Street Lowell MA 08151 978-614-3550 0001-Optimize-USB-load-ext2load-by-removing-uneeded-test-.patch Description: Binary data ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] usb_test_unit_ready called every block read - performance
Marek, I don't have a USB 1.x device to do that regression. Taking the call to usb_test_unit_ready out of the block reads/writes and leaving it only in get info really speed up ext2load. With the recent cache changes on top of this and the 5ms delay, we're getting fairly close to the Linux USB transfer times and are happy with the results. Can you look into integrating this? -Jim On Mon, Jul 30, 2012 at 6:54 PM, Marek Vasut wrote: > Dear Jim Shimer, > > > While tuning ext2load, we found that usb_test_unit_ready was being called > > every block read. We compared the usb block storage to the scsi block > > storage cmd_scsi.c, and found that the scsi device was only calling its > > scsi_setup_test_unit_ready() during scsi_can. It appears that > > usb_test_unit_ready() really only needs to be called once during > > usb_stor_scan(), via usb_stor_get_info(). Is there a particular reason > > usb_test_unit_ready is called for every block read, or do you think its > ok > > to only call during usb_stor_scan()? We're finding this speeds up > ext2load > > quite a bit. > > Could it be because the USB is actually quite slower than SCSI and the > device > might get congested? It seems the code was there ever since (I can't seem > to > find the origin of it) and we might actually want to try if this doesn't > break > some usb 1.x things. > > > Regards, > > Jim > > Best regards, > Marek Vasut > -- *James H Shimer* Motorola Mobility T3-12-HH72 900 Chelmsford Street Lowell MA 08151 978-614-3550 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
[U-Boot] usb_test_unit_ready called every block read - performance
While tuning ext2load, we found that usb_test_unit_ready was being called every block read. We compared the usb block storage to the scsi block storage cmd_scsi.c, and found that the scsi device was only calling its scsi_setup_test_unit_ready() during scsi_can. It appears that usb_test_unit_ready() really only needs to be called once during usb_stor_scan(), via usb_stor_get_info(). Is there a particular reason usb_test_unit_ready is called for every block read, or do you think its ok to only call during usb_stor_scan()? We're finding this speeds up ext2load quite a bit. Regards, Jim -- *James H Shimer* Motorola Mobility T3-12-HH72 900 Chelmsford Street Lowell MA 08151 978-614-3550 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] usb_stor_BBB_transport 5 ms delay - performance
I agree with everything, its up to you how to apply the change.
I did see a flags field but thought having a new one was conservative (I
had no real reason to have a new field). As for the typecasts I was
following the API which tests for device ready (Monkey See Monkey Do).
Also I have no compelling reason to need a "setter function" either. I
have no compelling feelings towards the implementation other than the 5ms
adds an unnecessary delay when the device is already known to be ready, and
this delay accumulates to a very poor performance for large files.
Thanks for working on this!
On Fri, Jul 27, 2012 at 11:06 AM, Marek Vasut wrote:
> Dear Benoît Thébaudeau,
>
> > Hi Jim,
> >
> > On Thu, Jul 26, 2012 at 10:20:48 PM, Jim Shimer wrote:
> > > I'm seeing a 5ms delay in usb_stor_BBB_transport, which occurs every
> > > 10K of
> > > data for fatload usb or 500ms of delay per 1MB of image size. This
> > > adds up
> > > to quite a bit of delay if you're loading a large ramdisk.
> > >
> > > Does anyone know what the reason for the 5ms delay really is? I'm
> > > assuming
> > > that this delay is to debounce the 5V/100ma USB power up. I made
> > > some
> > > modification, where the delay is skipped if the device has already
> > > been
> > > queried as ready. This has save me 500ms/M on fatload times (eg,
> > > 140M=70seconds). Is there anything wrong with this tweak?
> > >
> > > Here's a diff of what I've done to get the performance I need:
> > >
> > > --- usb_storage.c.orig 2012-07-26 16:06:40.775251000 -0400
> > > +++ usb_storage.c 2012-07-26 13:49:36.0 -0400
> > > @@ -132,6 +132,7 @@ static block_dev_desc_t usb_dev_desc[USB
> > >
> > > struct us_data;
> > > typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);
> > > typedef int (*trans_reset)(struct us_data *data);
> > >
> > > +typedef enum us_status { USB_NOT_READY, USB_READY} us_status;
>
> Can we possibly avoid the typedef?
>
> > > struct us_data {
> > >
> > > struct usb_device *pusb_dev; /* this usb_device */
> > >
> > > @@ -154,6 +155,7 @@ struct us_data {
> > >
> > > ccb *srb; /* current srb */
> > > trans_reset transport_reset;/* reset routine */
> > > trans_cmnd transport; /* transport routine
> > > */
> > >
> > > + us_status status;
>
> Don't we have some flags for it already?
>
> > >
> > > };
> > >
> > > static struct us_data usb_stor[USB_MAX_STOR_DEV];
> > >
> > > @@ -691,7 +693,10 @@ int usb_stor_BBB_transport(ccb *srb, str
> > >
> > > usb_stor_BBB_reset(us);
> > > return USB_STOR_TRANSPORT_FAILED;
> > >
> > > }
> > >
> > > - wait_ms(5);
> > > + if(us->status != USB_READY)
> > > + {
> > > + wait_ms(5);
> > > + }
> > >
> > > pipein = usb_rcvbulkpipe(us->pusb_dev, us->ep_in);
> > > pipeout = usb_sndbulkpipe(us->pusb_dev, us->ep_out);
> > > /* DATA phase + error handling */
> > >
> > > @@ -957,7 +962,10 @@ static int usb_test_unit_ready(ccb *srb,
> > >
> > > srb->datalen = 0;
> > > srb->cmdlen = 12;
> > > if (ss->transport(srb, ss) ==
> > > USB_STOR_TRANSPORT_GOOD)
> > >
> > > + {
> > > + ss->status = USB_READY;
> > >
> > > return 0;
> > >
> > > + }
> > >
> > > usb_request_sense(srb, ss);
> > > wait_ms(100);
> > >
> > > } while (retries--);
> > >
> > > @@ -965,6 +973,11 @@ static int usb_test_unit_ready(ccb *srb,
> > >
> > > return -1;
> > >
> > > }
> > >
> > > +static void usb_set_unit_not_ready(struct us_data *ss)
> > > +{
> > > + ss->status = USB_NOT_READY;
> > > +}
> > > +
>
> We don't need a setter function really.
>
> > > static int usb_read_capacity(ccb *srb, struct us_data *ss)
> > > {
> > >
> > > int retry;
> > >
> >
Re: [U-Boot] usb_stor_BBB_transport 5 ms delay - performance
Feel free to merge it into your work. Thanks. On Fri, Jul 27, 2012 at 10:17 AM, Benoît Thébaudeau < benoit.thebaud...@advansee.com> wrote: > Dear Marek, > > On Fri, Jul 27, 2012 at 04:09:42 PM, Marek Vasut wrote: > > [...] > > > > > Your suggestion is interesting and might be a complement to my > > > > > series. I > > > > > don't have time to check its correctness right now, but I'll > > > > > try > > > > > soon. > > > > > > > > Will you two have time to work these into V2 of your series > > > > somehow > > > > please? > > > > > > Are you asking me to integrate Jim's patch in my series with his > > > SoB once > > > reviewed? > > > > If you can negotiate ... > > > > > Since I have already issued a v2 for 2/5, do you want a v3 of the > > > whole > > > series to be more clear? > > > > Otherwise I'm fine with picking up your series and applying Jims > > patch > > afterwards ... though I'm not quite sure if it'll apply then. So > > guys, what do > > you think? > > I'm fine with merging Jim's patch once reviewed if he agrees. > > Regards, > Benoît > -- *James H Shimer* Motorola Mobility T3-12-HH72 900 Chelmsford Street Lowell MA 08151 978-614-3550 ___ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
Re: [U-Boot] usb_stor_BBB_transport 5 ms delay - performance
With the code that skips the 5 msecond delay if the device is ready, my fat
load time went from 80 seconds to 8 seconds. This is actually fairly close
to what it takes to do the same transfer in Linux (5 seconds). So I assume
the 5 msdelay when the device is already ready is not necessary.
On Thu, Jul 26, 2012 at 8:43 PM, Benoît Thébaudeau <
benoit.thebaud...@advansee.com> wrote:
> Hi Jim,
>
> On Thu, Jul 26, 2012 at 10:20:48 PM, Jim Shimer wrote:
> > I'm seeing a 5ms delay in usb_stor_BBB_transport, which occurs every
> > 10K of
> > data for fatload usb or 500ms of delay per 1MB of image size. This
> > adds up
> > to quite a bit of delay if you're loading a large ramdisk.
> >
> > Does anyone know what the reason for the 5ms delay really is? I'm
> > assuming
> > that this delay is to debounce the 5V/100ma USB power up. I made
> > some
> > modification, where the delay is skipped if the device has already
> > been
> > queried as ready. This has save me 500ms/M on fatload times (eg,
> > 140M=70seconds). Is there anything wrong with this tweak?
> >
> > Here's a diff of what I've done to get the performance I need:
> >
> > --- usb_storage.c.orig 2012-07-26 16:06:40.775251000 -0400
> > +++ usb_storage.c 2012-07-26 13:49:36.0 -0400
> > @@ -132,6 +132,7 @@ static block_dev_desc_t usb_dev_desc[USB
> > struct us_data;
> > typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);
> > typedef int (*trans_reset)(struct us_data *data);
> > +typedef enum us_status { USB_NOT_READY, USB_READY} us_status;
> >
> > struct us_data {
> > struct usb_device *pusb_dev; /* this usb_device */
> > @@ -154,6 +155,7 @@ struct us_data {
> > ccb *srb; /* current srb */
> > trans_reset transport_reset;/* reset routine */
> > trans_cmnd transport; /* transport routine
> > */
> > + us_status status;
> > };
> >
> > static struct us_data usb_stor[USB_MAX_STOR_DEV];
> > @@ -691,7 +693,10 @@ int usb_stor_BBB_transport(ccb *srb, str
> > usb_stor_BBB_reset(us);
> > return USB_STOR_TRANSPORT_FAILED;
> > }
> > - wait_ms(5);
> > + if(us->status != USB_READY)
> > + {
> > + wait_ms(5);
> > + }
> > pipein = usb_rcvbulkpipe(us->pusb_dev, us->ep_in);
> > pipeout = usb_sndbulkpipe(us->pusb_dev, us->ep_out);
> > /* DATA phase + error handling */
> > @@ -957,7 +962,10 @@ static int usb_test_unit_ready(ccb *srb,
> > srb->datalen = 0;
> > srb->cmdlen = 12;
> > if (ss->transport(srb, ss) ==
> > USB_STOR_TRANSPORT_GOOD)
> > + {
> > + ss->status = USB_READY;
> > return 0;
> > + }
> > usb_request_sense(srb, ss);
> > wait_ms(100);
> > } while (retries--);
> > @@ -965,6 +973,11 @@ static int usb_test_unit_ready(ccb *srb,
> > return -1;
> > }
> >
> > +static void usb_set_unit_not_ready(struct us_data *ss)
> > +{
> > + ss->status = USB_NOT_READY;
> > +}
> > +
> > static int usb_read_capacity(ccb *srb, struct us_data *ss)
> > {
> > int retry;
> > @@ -1108,6 +1121,7 @@ retry_it:
> > blks -= smallblks;
> > buf_addr += srb->datalen;
> > } while (blks != 0);
> > + usb_set_unit_not_ready((struct us_data *)dev->privptr);
> >
> > USB_STOR_PRINTF("usb_read: end startblk %lx, blccnt %x buffer
> > %lx\n",
> > start, smallblks, buf_addr);
> > @@ -1188,6 +1202,7 @@ retry_it:
> > blks -= smallblks;
> > buf_addr += srb->datalen;
> > } while (blks != 0);
> > + usb_set_unit_not_ready((struct us_data *)dev->privptr);
> >
> > USB_STOR_PRINTF("usb_write: end startblk %lx, blccnt %x
> > buffer
> > %lx\n",
> > start, smallblks, buf_addr);
> > @@ -1398,6 +1413,7 @@ int usb_stor_get_info(struct usb_device
> > cap[0] = 2880;
> > cap[1] = 0x200;
> > }
> > + usb_set_unit_not_ready((struct us_data *)dev->privptr);
> > USB_STOR_PRINTF("Read Capacity retu
[U-Boot] usb_stor_BBB_transport 5 ms delay - performance
I'm seeing a 5ms delay in usb_stor_BBB_transport, which occurs every 10K of
data for fatload usb or 500ms of delay per 1MB of image size. This adds up
to quite a bit of delay if you're loading a large ramdisk.
Does anyone know what the reason for the 5ms delay really is? I'm assuming
that this delay is to debounce the 5V/100ma USB power up. I made some
modification, where the delay is skipped if the device has already been
queried as ready. This has save me 500ms/M on fatload times (eg,
140M=70seconds). Is there anything wrong with this tweak?
Here's a diff of what I've done to get the performance I need:
--- usb_storage.c.orig 2012-07-26 16:06:40.775251000 -0400
+++ usb_storage.c 2012-07-26 13:49:36.0 -0400
@@ -132,6 +132,7 @@ static block_dev_desc_t usb_dev_desc[USB
struct us_data;
typedef int (*trans_cmnd)(ccb *cb, struct us_data *data);
typedef int (*trans_reset)(struct us_data *data);
+typedef enum us_status { USB_NOT_READY, USB_READY} us_status;
struct us_data {
struct usb_device *pusb_dev; /* this usb_device */
@@ -154,6 +155,7 @@ struct us_data {
ccb *srb; /* current srb */
trans_reset transport_reset;/* reset routine */
trans_cmnd transport; /* transport routine */
+ us_status status;
};
static struct us_data usb_stor[USB_MAX_STOR_DEV];
@@ -691,7 +693,10 @@ int usb_stor_BBB_transport(ccb *srb, str
usb_stor_BBB_reset(us);
return USB_STOR_TRANSPORT_FAILED;
}
- wait_ms(5);
+ if(us->status != USB_READY)
+ {
+ wait_ms(5);
+ }
pipein = usb_rcvbulkpipe(us->pusb_dev, us->ep_in);
pipeout = usb_sndbulkpipe(us->pusb_dev, us->ep_out);
/* DATA phase + error handling */
@@ -957,7 +962,10 @@ static int usb_test_unit_ready(ccb *srb,
srb->datalen = 0;
srb->cmdlen = 12;
if (ss->transport(srb, ss) == USB_STOR_TRANSPORT_GOOD)
+ {
+ ss->status = USB_READY;
return 0;
+ }
usb_request_sense(srb, ss);
wait_ms(100);
} while (retries--);
@@ -965,6 +973,11 @@ static int usb_test_unit_ready(ccb *srb,
return -1;
}
+static void usb_set_unit_not_ready(struct us_data *ss)
+{
+ ss->status = USB_NOT_READY;
+}
+
static int usb_read_capacity(ccb *srb, struct us_data *ss)
{
int retry;
@@ -1108,6 +1121,7 @@ retry_it:
blks -= smallblks;
buf_addr += srb->datalen;
} while (blks != 0);
+ usb_set_unit_not_ready((struct us_data *)dev->privptr);
USB_STOR_PRINTF("usb_read: end startblk %lx, blccnt %x buffer
%lx\n",
start, smallblks, buf_addr);
@@ -1188,6 +1202,7 @@ retry_it:
blks -= smallblks;
buf_addr += srb->datalen;
} while (blks != 0);
+ usb_set_unit_not_ready((struct us_data *)dev->privptr);
USB_STOR_PRINTF("usb_write: end startblk %lx, blccnt %x buffer
%lx\n",
start, smallblks, buf_addr);
@@ -1398,6 +1413,7 @@ int usb_stor_get_info(struct usb_device
cap[0] = 2880;
cap[1] = 0x200;
}
+ usb_set_unit_not_ready((struct us_data *)dev->privptr);
USB_STOR_PRINTF("Read Capacity returns: 0x%lx, 0x%lx\n", cap[0],
cap[1]);
#if 0
I'd appreciate any feedback.
Regards
--
*James H Shimer*
Motorola Mobility T3-12-HH72
900 Chelmsford Street
Lowell MA 08151
978-614-3550
___
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
