Re: [linux-usb-devel] Re: USB hangs

Re: [linux-usb-devel] Re: USB hangs

[Martin Bogomolni]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
 
On Mon 12 Jan, Oliver Neukum wrote:
 
[...]
 
ChangeSet@1.1218, 2004-01-12 00:26:13+01:00, oliver@vermuden.neukum.org -
avoid GFP_KERNEL in block IO path
 
[...]
 
Oliver, Alan, et. al.
 
I applied Oliver & Alan Cox's patches to the 2.4.24 kernel, and am still
seeing hangs when copying large amounts of data over USB on IBM systems
containing two EHCI controllers (Intel 82801EB).
 
I see similar errors to the ones Lucas Postupa had in the kernel logs :
 
Buffer I/O error on device sda1, logical block 134522 lost page write due to
I/O error on sda1
 
Unloading the EHCI driver, and reverting to 1.1 speeds, does not solve the issue.
 
- -Martin

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.2 (GNU/Linux)
 
iD8DBQFAF+7vgZQNxll+EIcRAtkKAJ476IhZ+fLcyj73mDjNptU3rWmQ/ACeIIpz
M5S8ast+WlfEHhKrbAtuPg0=
=NH9l
-----END PGP SIGNATURE-----

Re: [linux-usb-devel] Re: USB hangs

[Pavel Machek]

Hi!


> > In 2.4 they all run in interrupt or thread context IIRC.
> > Problematic is the SCSI error handling thread. It can call usb_reset_device()
> > which calls down and does allocations.
> > Does that thread also do the PF_MEMALLOC trick?
> 
> In 2.4 it doesn't, which is rather surpising considering how many storage 
> devices run over SCSI transports.
> 
> In 2.6 it sets PF_IOTHREAD.  I don't know if that subsumes the function of 
> PF_MEMALLOC or not.  The state of kerneldoc for much of the Linux core 
> functionality is shocking.

PF_IOTHREAD is there for suspend/resume. It does not affect anything
else.
									Pavel
-- 
When do you have a heart between your knees?
[Johanka's followup: and *two* hearts?]

Re: [linux-usb-devel] Re: USB hangs

[Alan Cox]

On Llu, 2004-01-12 at 16:27, Alan Stern wrote:
> On Mon, 12 Jan 2004, Oliver Neukum wrote:
> 
> > In 2.4 they all run in interrupt or thread context IIRC.
> > Problematic is the SCSI error handling thread. It can call usb_reset_device()
> > which calls down and does allocations.
> > Does that thread also do the PF_MEMALLOC trick?
> 
> In 2.4 it doesn't, which is rather surpising considering how many storage 
> devices run over SCSI transports.
> 
> In 2.6 it sets PF_IOTHREAD.  I don't know if that subsumes the function of 
> PF_MEMALLOC or not.  The state of kerneldoc for much of the Linux core 
> functionality is shocking.

Core scsi assumes that scsi drivers will use scsi_malloc/free so really
ignores the issue as someone elses problem.

Re: [linux-usb-devel] Re: USB hangs

[Alan Stern]

On Mon, 12 Jan 2004, Oliver Neukum wrote:

> In 2.4 they all run in interrupt or thread context IIRC.
> Problematic is the SCSI error handling thread. It can call usb_reset_device()
> which calls down and does allocations.
> Does that thread also do the PF_MEMALLOC trick?

In 2.4 it doesn't, which is rather surpising considering how many storage 
devices run over SCSI transports.

In 2.6 it sets PF_IOTHREAD.  I don't know if that subsumes the function of 
PF_MEMALLOC or not.  The state of kerneldoc for much of the Linux core 
functionality is shocking.

Alan Stern

Re: [linux-usb-devel] Re: USB hangs

[Alan Stern]

On Mon, 12 Jan 2004, Oliver Neukum wrote:

> diff -Nru a/drivers/usb/usb.c b/drivers/usb/usb.c
> --- a/drivers/usb/usb.c	Mon Jan 12 00:27:37 2004
> +++ b/drivers/usb/usb.c	Mon Jan 12 00:27:37 2004
> @@ -1198,7 +1198,7 @@
>  int usb_control_msg(struct usb_device *dev, unsigned int pipe, __u8 request, __u8 requesttype,
>  			 __u16 value, __u16 index, void *data, __u16 size, int timeout)
>  {
> -	struct usb_ctrlrequest *dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
> +	struct usb_ctrlrequest *dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
>  	int ret;
>  	
>  	if (!dr)
> @@ -1958,7 +1958,7 @@
>  	if (result < 0)
>  		return result;
>  
> -	buffer = kmalloc(sizeof(status), GFP_KERNEL);
> +	buffer = kmalloc(sizeof(status), GFP_NOIO);
>  	if (!buffer) {
>  		err("unable to allocate memory for configuration descriptors");
>  		return -ENOMEM;

Note that these changes have essentially already been incorporated into 
2.6, so only 2.4 needs updating.

Alan Stern

Re: [linux-usb-devel] Re: USB hangs

[Oliver Neukum]

Am Montag, 12. Januar 2004 08:39 schrieb Matthew Dharm:
> On Sun, Jan 11, 2004 at 08:11:58PM -0800, David Brownell wrote:
> > 
> > >>>	 Plus I'd
> > >>>argue PF_MEMALLOC is a better solution anyway.
> > >>
> > >>It certainly seems like a more comprehensive fix for that
> > >>particular class of problems!  :)
> > >
> > >
> > >Is it really more comprehensive?  As I see it, it will only affect code
> > >executed in the context of the usb-storage thread.  But, what about code
> > >which is invoked in tasklets or other contexts?
> > 
> > Isn't it true that only that thread is allowed to
> > submit usb-storage i/o requests?
> 
> That's very true.
> 
> What I'm concerned about is the downstream effects of a usb_submit_urb() or
> the corresponding scatter-gather equivalents.

In 2.4 they all run in interrupt or thread context IIRC.
Problematic is the SCSI error handling thread. It can call usb_reset_device()
which calls down and does allocations.
Does that thread also do the PF_MEMALLOC trick?

	Regards
		Oliver

Re: [linux-usb-devel] Re: USB hangs

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 946 bytes --]

On Sun, Jan 11, 2004 at 08:11:58PM -0800, David Brownell wrote:
> 
> >>>	 Plus I'd
> >>>argue PF_MEMALLOC is a better solution anyway.
> >>
> >>It certainly seems like a more comprehensive fix for that
> >>particular class of problems!  :)
> >
> >
> >Is it really more comprehensive?  As I see it, it will only affect code
> >executed in the context of the usb-storage thread.  But, what about code
> >which is invoked in tasklets or other contexts?
> 
> Isn't it true that only that thread is allowed to
> submit usb-storage i/o requests?

That's very true.

What I'm concerned about is the downstream effects of a usb_submit_urb() or
the corresponding scatter-gather equivalents.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

You should try to see the techs say "three piece suit".
					-- The Chief
User Friendly, 11/23/1997

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-usb-devel] Re: USB hangs

[David Brownell]

>>>	 Plus I'd
>>>argue PF_MEMALLOC is a better solution anyway.
>>
>>It certainly seems like a more comprehensive fix for that
>>particular class of problems!  :)
> 
> 
> Is it really more comprehensive?  As I see it, it will only affect code
> executed in the context of the usb-storage thread.  But, what about code
> which is invoked in tasklets or other contexts?

Isn't it true that only that thread is allowed to
submit usb-storage i/o requests?

- Dave

Re: [linux-usb-devel] Re: USB hangs

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1087 bytes --]

On Mon, Jan 12, 2004 at 12:09:41AM +0000, Alan Cox wrote:
> On Sul, 2004-01-11 at 23:33, Oliver Neukum wrote:
> > For users of a kernel thread it helps. But what affects storage
> > also make affect anything else that has a filesystem running
> > over it. Plus it forces us to keep the storage thread model, which
> > might be a solution that needs to be revisited.
> 
> Its a larger hammer, for 2.6 I agree that moving the right code to
> GFP_NOIO is far better a solution. For 2.4 I just want it working with
> minimal risk of screwups.

Well, I have no objection to adding that to 2.4 -- either push to Marcelo
yourself or send it to Greg K-H for inclusion in his 2.4 tree and eventual
push upstream.

But we do need to do some sort of 2.6 audit for this sort of thing.

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

G:   Baaap booop BAHHHP.
Mir: 9600 Baud?
Mik: No, no!  9600 goes baap booop, not booop bahhhp!
					-- Greg, Miranda and Mike
User Friendly, 12/31/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-usb-devel] Re: USB hangs

[Alan Cox]

On Sul, 2004-01-11 at 23:33, Oliver Neukum wrote:
> For users of a kernel thread it helps. But what affects storage
> also make affect anything else that has a filesystem running
> over it. Plus it forces us to keep the storage thread model, which
> might be a solution that needs to be revisited.

Its a larger hammer, for 2.6 I agree that moving the right code to
GFP_NOIO is far better a solution. For 2.4 I just want it working with
minimal risk of screwups.

Re: [linux-usb-devel] Re: USB hangs

[Oliver Neukum]

Am Montag, 12. Januar 2004 00:25 schrieb David Brownell:
> Alan Cox wrote:
> > On Sul, 2004-01-11 at 00:23, Matthew Dharm wrote:
> > 
> >>Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
> >>down and eliminated them.
> > 
> > 
> > Not sure. I just worked from tracebacks. I needed it to work rather
> > than having the time to go hunting for specific faults. Plus I'd
> > argue PF_MEMALLOC is a better solution anyway.
> 
> It certainly seems like a more comprehensive fix for that
> particular class of problems!  :)

For users of a kernel thread it helps. But what affects storage
also make affect anything else that has a filesystem running
over it. Plus it forces us to keep the storage thread model, which
might be a solution that needs to be revisited.

	Regards
		Oliver

Re: [linux-usb-devel] Re: USB hangs

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1012 bytes --]

On Sun, Jan 11, 2004 at 03:25:06PM -0800, David Brownell wrote:
> Alan Cox wrote:
> >On Sul, 2004-01-11 at 00:23, Matthew Dharm wrote:
> >
> >>Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
> >>down and eliminated them.
> >
> >
> >Not sure. I just worked from tracebacks. I needed it to work rather
> >than having the time to go hunting for specific faults. Plus I'd
> >argue PF_MEMALLOC is a better solution anyway.
> 
> It certainly seems like a more comprehensive fix for that
> particular class of problems!  :)

Is it really more comprehensive?  As I see it, it will only affect code
executed in the context of the usb-storage thread.  But, what about code
which is invoked in tasklets or other contexts?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

I'm just trying to think of a way to say "up yours" without getting fired.
					-- Stef
User Friendly, 10/8/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-usb-devel] Re: USB hangs

[Oliver Neukum]

Am Sonntag, 11. Januar 2004 23:39 schrieb Alan Cox:
> On Sul, 2004-01-11 at 08:02, Oliver Neukum wrote:
> > Until recently this line from usb-ohci.h read GFP_KERNEL instead of GFP_NOIO
> > 
> > #define ALLOC_FLAGS (in_interrupt () || current->state != TASK_RUNNING ? GFP_ATOMIC : GFP_NOIO)
> > 
> > Was it an earlier kernel without that change?
> 
> Its an UHCI controller.

OK. I got some more.
Greg please apply.

	Regards
		Oliver
You can import this changeset into BK by piping this whole message to:
'| bk receive [path to repository]' or apply the patch as usual.

===================================================================


ChangeSet@1.1218, 2004-01-12 00:26:13+01:00, oliver@vermuden.neukum.org
  - avoid GFP_KERNEL in block IO path


 usb.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)


diff -Nru a/drivers/usb/usb.c b/drivers/usb/usb.c
--- a/drivers/usb/usb.c	Mon Jan 12 00:27:37 2004
+++ b/drivers/usb/usb.c	Mon Jan 12 00:27:37 2004
@@ -1198,7 +1198,7 @@
 int usb_control_msg(struct usb_device *dev, unsigned int pipe, __u8 request, __u8 requesttype,
 			 __u16 value, __u16 index, void *data, __u16 size, int timeout)
 {
-	struct usb_ctrlrequest *dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_KERNEL);
+	struct usb_ctrlrequest *dr = kmalloc(sizeof(struct usb_ctrlrequest), GFP_NOIO);
 	int ret;
 	
 	if (!dr)
@@ -1958,7 +1958,7 @@
 	if (result < 0)
 		return result;
 
-	buffer = kmalloc(sizeof(status), GFP_KERNEL);
+	buffer = kmalloc(sizeof(status), GFP_NOIO);
 	if (!buffer) {
 		err("unable to allocate memory for configuration descriptors");
 		return -ENOMEM;

===================================================================


This BitKeeper patch contains the following changesets:
1.1218
## Wrapped with gzip_uu ##


begin 664 bkpatch26304
M'XL(`.G;`4```[V4;V_3,!#&7\>?XJ2]V1A-[FPG:8*""EL9U::U*NSUE#IN
M4Z5I1OX,@?+A<1M4F!H0#(G(D2/?^7>/SX]R`G>5+D.KV*P?=<E.X'U1U:%E
MOO,FT5M[JYNLR>VB7)G8O"A,S$F+7#O=!N=CJ77EK$J]XI*9E%E<JQ1,I`HM
MLL5AI?[RH$-K/KZZNWDS9RR*X"*-MRO]0=<016R1C9)&;^RL+.)T5ZT]A%N.
M2.@B<2$]Y"WZ2&[K^4GL2X&!Z\J$-+%.SZA'^%.41"(DCWO";VGHRX!=`MG$
M:0@H'22'.""&W`M)G".%B/!K-)P3#)"]A7_7?\$4#"!^+-8)7+V;W5^/Y[?C
M&UAO8;$I5`:3*3S$=<JNP?`XL=F/!K+!7SZ,88SL=8_JI-P=M7*::K%[;?63
M>I>,>BFD'[3#P(_=6+E2J2%*J7_3HG[D[A8,DGN$K4`,Y-X21ZG]UGBF2+9S
MZ:C#J"+OQP@<"D*/^^098<*7>WM(<60._`-S<!CP_VR.KIE3&)2?]\-<]NRX
MK\]PS"5Q)"`V^3Y;55TVJ@;#NU=UN2GUIT97-;Q(2H@@R^.-479:K;_J8GG:
MGWOV<G^8V^ED>O;*%`B\KD`W6XMFN=2]L+ANJJ>;#_\9E6J554T>Q<E22"67
*[!M1:89=X@0`````
`
end

Re: [linux-usb-devel] Re: USB hangs

[David Brownell]

Alan Cox wrote:
> On Sul, 2004-01-11 at 00:23, Matthew Dharm wrote:
> 
>>Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
>>down and eliminated them.
> 
> 
> Not sure. I just worked from tracebacks. I needed it to work rather
> than having the time to go hunting for specific faults. Plus I'd
> argue PF_MEMALLOC is a better solution anyway.

It certainly seems like a more comprehensive fix for that
particular class of problems!  :)

Re: [linux-usb-devel] Re: USB hangs

[Alan Cox]

On Sul, 2004-01-11 at 08:02, Oliver Neukum wrote:
> Until recently this line from usb-ohci.h read GFP_KERNEL instead of GFP_NOIO
> 
> #define ALLOC_FLAGS (in_interrupt () || current->state != TASK_RUNNING ? GFP_ATOMIC : GFP_NOIO)
> 
> Was it an earlier kernel without that change?

Its an UHCI controller.

Re: [linux-usb-devel] Re: USB hangs

[Oliver Neukum]

Am Sonntag, 11. Januar 2004 03:33 schrieb Alan Cox:
> On Sul, 2004-01-11 at 00:23, Matthew Dharm wrote:
> > Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
> > down and eliminated them.
> 
> Not sure. I just worked from tracebacks. I needed it to work rather
> than having the time to go hunting for specific faults. Plus I'd
> argue PF_MEMALLOC is a better solution anyway.

Until recently this line from usb-ohci.h read GFP_KERNEL instead of GFP_NOIO

#define ALLOC_FLAGS (in_interrupt () || current->state != TASK_RUNNING ? GFP_ATOMIC : GFP_NOIO)

Was it an earlier kernel without that change?

	Regards
		Oliver

Re: [linux-usb-devel] Re: USB hangs

[David Brownell]

Oliver Neukum wrote:
> Am Sonntag, 11. Januar 2004 01:23 schrieb Matthew Dharm:
> 
>>Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
>>down and eliminated them.
>>
> 
> 
> static int ohci_mem_init (struct ohci *ohci)
> {
> 	...
> }
> 
> This one here looks dangerous.

Since that happens as part of controller initialization,
long before usb-storage could possibly come into play ...
it doesn't seem to me like even a remote concern!

Re: [linux-usb-devel] Re: USB hangs

[Oliver Neukum]

> I'll agree that it looks dangerous, tho pci_pool_create() is something I
> know little about.
> 
> Is this 2.4 or 2.6 code here?

2.4 / usb-ohci.h

	Regards
		Oliver

Re: [linux-usb-devel] Re: USB hangs

[Matthew Dharm]

[-- Attachment #1: Type: text/plain, Size: 1238 bytes --]

On Sun, Jan 11, 2004 at 01:49:34AM +0100, Oliver Neukum wrote:
> Am Sonntag, 11. Januar 2004 01:23 schrieb Matthew Dharm:
> > Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
> > down and eliminated them.
> > 
> 
> static int ohci_mem_init (struct ohci *ohci)
> {
> 	ohci->td_cache = pci_pool_create ("ohci_td", ohci->ohci_dev,
> 		sizeof (struct td),
> 		32 /* byte alignment */,
> 		0 /* no page-crossing issues */,
> 		GFP_KERNEL | OHCI_MEM_FLAGS);
> 	if (!ohci->td_cache)
> 		return -ENOMEM;
> 	ohci->dev_cache = pci_pool_create ("ohci_dev", ohci->ohci_dev,
> 		sizeof (struct ohci_device),
> 		16 /* byte alignment */,
> 		0 /* no page-crossing issues */,
> 		GFP_KERNEL | OHCI_MEM_FLAGS);
> 	if (!ohci->dev_cache)
> 		return -ENOMEM;
> 	return 0;
> }
> 
> This one here looks dangerous.

I'll agree that it looks dangerous, tho pci_pool_create() is something I
know little about.

Is this 2.4 or 2.6 code here?

Matt

-- 
Matthew Dharm                              Home: mdharm-usb@one-eyed-alien.net 
Maintainer, Linux USB Mass Storage Driver

Okay, this isn't funny anymore! Let me down!  I'll tell Bill on you!!
					-- Microsoft Salesman
User Friendly, 4/1/1998

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [linux-usb-devel] Re: USB hangs

[Oliver Neukum]

Am Sonntag, 11. Januar 2004 01:23 schrieb Matthew Dharm:
> Where is USB kmalloc'ing with GFP_KERNEL?  I thought we tracked all those
> down and eliminated them.
> 

static int ohci_mem_init (struct ohci *ohci)
{
	ohci->td_cache = pci_pool_create ("ohci_td", ohci->ohci_dev,
		sizeof (struct td),
		32 /* byte alignment */,
		0 /* no page-crossing issues */,
		GFP_KERNEL | OHCI_MEM_FLAGS);
	if (!ohci->td_cache)
		return -ENOMEM;
	ohci->dev_cache = pci_pool_create ("ohci_dev", ohci->ohci_dev,
		sizeof (struct ohci_device),
		16 /* byte alignment */,
		0 /* no page-crossing issues */,
		GFP_KERNEL | OHCI_MEM_FLAGS);
	if (!ohci->dev_cache)
		return -ENOMEM;
	return 0;
}

This one here looks dangerous.

	Regards
		Oliver