On Thursday 14 March 2002 19:04, David Brownell wrote:
> > > The usb-storage code could retry when it gets "-ENOMEM"
> > > when submitting an URB, to get similar effects, yes?
> >
> > Thinking about it again, trying to be perfect in face of OOM
> > is a little silly.
> > Instead of retrying could we simply use a flag in the urb that means
> > retry memory allocations with GFP_ATOMIC if they fail ?
> > This way we are reasonably good and keep the API clean.
>
> It'd get really nasty to modify all the HCDs to retry on -ENOMEM.
> And any retries would make no sense if the urb submission was
> under GFP_ATOMIC ... since that precludes the "block till something
> is (or might be) freed" logic a retry would rely on.
Actually blocking is not what I had in mind.
We must retry with GFP_ATOMIC to get a page from the reserves
the vm keeps for this. We can do several retries to put cream on the ice.
> So I still think it's easier to package that as specific to usb-storage.
> Just a wrapper around usb_submit_urb(), retry N times if -ENOMEM.
>
> (But it's still a good idea to move memory allocations off
> the critical path ... fewer failure modes "by design" is always
> a good thing, just like shorter/faster common paths.)
Definitely yes.
Ok, here's a change set. It even covers this extremely unlikely corner case.
Frankly, I don't think spending more effort on this would be a waste.
Regards
Oliver
This BitKeeper patch contains the following changesets:
1.388
# This is a BitKeeper patch. What follows are the unified diffs for the
# set of deltas contained in the patch. The rest of the patch, the part
# that BitKeeper cares about, is below these diffs.
# User: oliver
# Host: oenone.homelinux.org
# Root: /home/oliver/bk-trees/work
#
#--- 1.6/drivers/usb/storage/scsiglue.c Fri Feb 8 01:34:35 2002
#+++ 1.7/drivers/usb/storage/scsiglue.c Thu Mar 14 22:26:59 2002
#@@ -279,6 +279,8 @@
# down(&(us->irq_urb_sem));
# us->irq_urb->dev = us->pusb_dev;
# result = usb_submit_urb(us->irq_urb, GFP_NOIO);
#+ if (result == -ENOMEM) /* retry from atomic pool */
#+ result = usb_submit_urb(us->irq_urb, GFP_ATOMIC);
# US_DEBUGP("usb_submit_urb() returns %d\n", result);
# up(&(us->irq_urb_sem));
# }
#@@ -897,4 +899,5 @@
# /* Set value of length passed in */
# *length_p = length;
# }
#+
#
#
#--- 1.14/drivers/usb/storage/transport.c Thu Mar 7 05:31:51 2002
#+++ 1.15/drivers/usb/storage/transport.c Thu Mar 14 22:26:59 2002
#@@ -385,6 +385,7 @@
# {
# struct completion urb_done;
# int status;
#+ int retry_count;
# struct usb_ctrlrequest *dr;
#
# /* allocate the device request structure */
#@@ -416,12 +417,24 @@
# /* submit the URB */
# status = usb_submit_urb(us->current_urb, GFP_NOIO);
# if (status) {
#+ /* we must retry from atomic pool to avoid errors paging */
#+ retry_count = RETRY_ON_NOMEM;
#+ do {
#+ if (status && status != -ENOMEM)
#+ break;
#+ status = usb_submit_urb(us->current_urb, GFP_ATOMIC);
#+ if (status >= 0)
#+ goto retry_success;
#+ retry_count--;
#+ } while(retry_count);
#+
# /* something went wrong */
# up(&(us->current_urb_sem));
# kfree(dr);
# return status;
# }
#
#+retry_success:
# /* wait for the completion of the URB */
# up(&(us->current_urb_sem));
# wait_for_completion(&urb_done);
#@@ -446,6 +459,7 @@
# {
# struct completion urb_done;
# int status;
#+ int retry_count;
#
# /* set up data structures for the wakeup system */
# init_completion(&urb_done);
#@@ -463,11 +477,22 @@
# /* submit the URB */
# status = usb_submit_urb(us->current_urb, GFP_NOIO);
# if (status) {
#+ /* we must retry from atomic pool to avoid errors paging */
#+ retry_count = RETRY_ON_NOMEM;
#+ do {
#+ if (status && status != -ENOMEM)
#+ break;
#+ status = usb_submit_urb(us->current_urb, GFP_ATOMIC);
#+ if (status >= 0)
#+ goto retry_success;
#+ retry_count--;
#+ } while(retry_count);
# /* something went wrong */
# up(&(us->current_urb_sem));
# return status;
# }
#
#+retry_success:
# /* wait for the completion of the URB */
# up(&(us->current_urb_sem));
# wait_for_completion(&urb_done);
#@@ -1266,3 +1291,4 @@
# US_DEBUGP("Bulk soft reset completed\n");
# return SUCCESS;
# }
#+
#
#--- 1.1/drivers/usb/storage/transport.h Sun Nov 25 05:09:19 2001
#+++ 1.2/drivers/usb/storage/transport.h Thu Mar 14 22:26:59 2002
#@@ -129,6 +129,11 @@
# #define USB_STOR_TRANSPORT_ABORTED 3 /* Transport aborted */
#
# /*
#+ * number of times we retry on ENOMEM using the ATOMIC pool
#+ */
#+#define RETRY_ON_NOMEM 3
#+
#+/*
# * CBI accept device specific command
# */
#
#@@ -154,3 +159,4 @@
# extern void usb_stor_transfer(Scsi_Cmnd*, struct us_data*);
# extern int usb_stor_clear_halt(struct usb_device*, int );
# #endif
#+
#
# Diff checksum=98a27ee3
# Patch vers: 1.3
# Patch type: REGULAR
== ChangeSet ==
[EMAIL PROTECTED]|ChangeSet|20011125035615|28334|160bd283d279fb69
[EMAIL PROTECTED]|ChangeSet|20020314000949|64492
D 1.388 02/03/14 22:28:27+01:00 [EMAIL PROTECTED] +3 -0
B [EMAIL PROTECTED]|ChangeSet|20011125035615|28334|160bd283d279fb69
C
c retrying from the atomic pool on enomem to avoid
c needlessly failing io on low memory paging
K 2369
P ChangeSet
------------------------------------------------
0a0
>
>[EMAIL PROTECTED]|drivers/usb/storage/transport.c|20011125040919|07689|a9734f3367d31473
> [EMAIL PROTECTED]|drivers/usb/storage/transport.c|20020314212659|12404
>
>[EMAIL PROTECTED]|drivers/usb/storage/transport.h|20011125040919|14807|56166798c3b3993
> [EMAIL PROTECTED]|drivers/usb/storage/transport.h|20020314212659|43154
>
>[EMAIL PROTECTED]|drivers/usb/storage/scsiglue.c|20011125040919|06074|68c061958353a012
> [EMAIL PROTECTED]|drivers/usb/storage/scsiglue.c|20020314212659|17244
== drivers/usb/storage/scsiglue.c ==
[EMAIL PROTECTED]|drivers/usb/storage/scsiglue.c|20011125040919|06074|68c061958353a012
[EMAIL PROTECTED]|drivers/usb/storage/scsiglue.c|20020208003435|08670
D 1.7 02/03/14 22:26:59+01:00 [EMAIL PROTECTED] +3 -0
B [EMAIL PROTECTED]|ChangeSet|20011125035615|28334|160bd283d279fb69
C
c retry from atomic pool on enomem
K 17244
O -rw-rw-r--
P drivers/usb/storage/scsiglue.c
------------------------------------------------
I281 2
if (result == -ENOMEM) /* retry from atomic pool */
result = usb_submit_urb(us->irq_urb, GFP_ATOMIC);
I899 1
\
== drivers/usb/storage/transport.c ==
[EMAIL PROTECTED]|drivers/usb/storage/transport.c|20011125040919|07689|a9734f3367d31473
[EMAIL PROTECTED]|drivers/usb/storage/transport.c|20020307043151|25492
D 1.15 02/03/14 22:26:59+01:00 [EMAIL PROTECTED] +26 -0
B [EMAIL PROTECTED]|ChangeSet|20011125035615|28334|160bd283d279fb69
C
c retry from atomic pool on enomem
K 12404
O -rw-rw-r--
P drivers/usb/storage/transport.c
------------------------------------------------
I387 1
int retry_count;
I418 11
/* we must retry from atomic pool to avoid errors paging */
retry_count = RETRY_ON_NOMEM;
do {
if (status && status != -ENOMEM)
break;
status = usb_submit_urb(us->current_urb, GFP_ATOMIC);
if (status >= 0)
goto retry_success;
retry_count--;
} while(retry_count);
\
I424 1
retry_success:
I448 1
int retry_count;
I465 10
/* we must retry from atomic pool to avoid errors paging */
retry_count = RETRY_ON_NOMEM;
do {
if (status && status != -ENOMEM)
break;
status = usb_submit_urb(us->current_urb, GFP_ATOMIC);
if (status >= 0)
goto retry_success;
retry_count--;
} while(retry_count);
I470 1
retry_success:
I1268 1
\
== drivers/usb/storage/transport.h ==
[EMAIL PROTECTED]|drivers/usb/storage/transport.h|20011125040919|14807|56166798c3b3993
[EMAIL PROTECTED]|drivers/usb/storage/transport.h|20011125040920|35934
D 1.2 02/03/14 22:26:59+01:00 [EMAIL PROTECTED] +6 -0
B [EMAIL PROTECTED]|ChangeSet|20011125035615|28334|160bd283d279fb69
C
c defining the number of retries on enomem
K 43154
O -rw-rw-r--
P drivers/usb/storage/transport.h
------------------------------------------------
I131 5
* number of times we retry on ENOMEM using the ATOMIC pool
*/
#define RETRY_ON_NOMEM 3
\
/*
I156 1
\
# Patch checksum=43dcfcb8
_______________________________________________
[EMAIL PROTECTED]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
