GFP_ATOMIC allocations...

I'm looking through some of the new code that has been put in the kernel scsi subsystem lately and I'm seeing a few of the same mistakes that the old code made. So, I'm pointing out one of them here. There are lots of places in the SCSI mid layer that are called outside of any spinlocks and not from interrupt context where there is absolutely no need to use GFP_ATOMIC allocations for memory. In general, if you don't need to use GFP_ATOMIC, then don't use it. You're more likely to fail the allocation and therefore fail the operation. So, if you are working on a section of code, please think about this when doing allocations and do whatever is right for the code snippet you are currently hacking. Fixing places in the existing code that use GFP_ATOMIC needlessly is a plus ;-)

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

So, you think all (or most) of the GFP_ATOMIC's in scsi_scan.c should be GFP_KERNEL? All the kmalloc calls should be at boot time, or via insmod.

I was wondering about them, but left them to match the previous scsi_scan.c code.

Do GFP_KERNEL alloc failures during boot time just return NULL? (Given that there is really no memory left.) I'd hope so, but was never sure.

I suppose I could change them and boot with mem=something-low and see what happens.

-- Patrick

Yep. Until such time as a LLDD actually calls scsi_scan from an interrupt handler to handle a fiber fabric that has just come back up, it is safe for this code to use GFP_KERNEL. As for the fiber channel issue, I actually think that it needs to notify the eh thread of the change in loop state and let the eh thread do the rescan from the eh thread context instead of interrupt context, so this won't be an issue when we support fiber loop transitions either.

Right, process context with no locks held.

I figured. That's why I pointed it out ;-)

GFP_ATOMIC returns NULL on failure, GFP_KERNEL *should* never fail until we get to a true OOM condition because it will block and wait for the vm subsystem to free us up some space. Under true OOM conditions it will return NULL AFAIK.

I've known people that had actual, real problems because of the GFP_ATOMIC allocations in this code area. It was related to bringing up a fiber controller that had 100s of disks hanging off of it and therefore wanted to do a *LOT* of atomic allocations in a short period of time, but it was a real problem none the less. Making the allocations non-atomic would allow the scanning to block waiting on more ram instead of bailing on devices.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

Or maybe deadlock if its happening due to I/O. If it wants to flush a page out to that scsi device and the scsi device is doing the allocation life gets very unpleasant. SCSI may not be coming from block layer access with PF_MEM* set either, think about /proc and scsi-generic

"Here be dragons"

That would be a trick indeed since Patrick and I were referring to the scanning code so the device isn't even configured for kernel use yet :-P

Of course. This is why the mid layer doesn't typically allocate anything at all once initial setup of a device is complete. No allocations once setup is complete means no deadlock.

That's why I recommended people think each use through instead of blindly changing things ;-)

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

There are some bad allocations in sd.c - there's now a (single) GFP_DMA buffer for test unit ready, spinup and the write protect check, where we used scsi_malloc() in 2.4.

There are a bunch of vmalloc's most of them should be kmalloc's.

Here's the sd.c code that does the vmalloc:

#define init_mem_lth(x,n) x = vmalloc((n) * sizeof(*x))
#define zero_mem_lth(x,n) memset(x, 0, (n) * sizeof(*x))

	init_mem_lth(sd_dsk_arr, sd_template.dev_max);
	if (sd_dsk_arr) {
		zero_mem_lth(sd_dsk_arr, sd_template.dev_max);
		for (k = 0; k < sd_template.dev_max; ++k) {
			sdkp = vmalloc(sizeof(Scsi_Disk));
			if (NULL == sdkp)
				goto cleanup_mem;
			memset(sdkp, 0, sizeof(Scsi_Disk));
			sd_dsk_arr[k] = sdkp;
		}
	}
	init_mem_lth(sd_disks, sd_template.dev_max);
	if (sd_disks)
		zero_mem_lth(sd_disks, sd_template.dev_max);
	init_mem_lth(sd, maxparts);

	if (!sd_dsk_arr || !sd || !sd_disks)
		goto cleanup_mem;

	zero_mem_lth(sd, maxparts);
	return 0;

#undef init_mem_lth
#undef zero_mem_lth

The sdkp = vmalloc() certainly should be kmalloc().

Each of the init_mem_lth and associated variable:

	static Scsi_Disk ** sd_dsk_arr;
	init_mem_lth(sd_dsk_arr, sd_template.dev_max);

	static struct gendisk **sd_disks;
	init_mem_lth(sd_disks, sd_template.dev_max);

	struct hd_struct *sd;
	init_mem_lth(sd, maxparts);

dev_max maximum is N_SD_MAJORS * SCSI_DISKS_PER_MAJOR right now 128 (8 * 16), even doubling won't hit kmalloc limits if we want an array of pointers (256 * 4 or 256 * 8).

So only the last one might need a vmalloc(), since maxparts is max_dev * 16, and hd_structs are larger.

The only use of sd is in sd_attach to set the part - can we now just allocate a hd_struct as needed in sd_attach, as part of the struct *p data? Also it looks like there is never a kfree(p) for the normal non-error case.

Or, can the size be checked and vmalloc() only called when necessary, or called if kmalloc() fails?

-- Patrick Mansfield

There was past discussion about calling into scsi_build_commandblocks() while not in user context (via usb etc.), this implies that we could scan while not in user context.

I don't see how that would work, since the scan sleeps waiting for IO.

Do we really call scan_scsis and scsi_build_commandblocks while not in user context? Who is the caller (to scsi_register_host)?

Here is Pete's response saying he found such a case, but not where:

http://marc.theaimsgroup.com/?l=linux-scsi&m=101678634608966&w=2

-- Patrick Mansfield

I agree we shouldn't not be in user context here.

What about, instead of Pete's patch, changing the GFP_ATOMIC to GFP_KERNEL and adding a BUG_ON(in_interrupt()) at the top and see who sends back bug reports...

James

Works for me.

  Doug Ledford <[email protected]>     919-754-3700 x44233
         Red Hat, Inc. 
         1801 Varsity Dr.
         Raleigh, NC 27606

How about with printk instead of BUG_ON? BUG_ON is better in the long run, but I would rather printk for now.

Here's the patch against 2.5.32:

@@ -502,7 +502,7 @@
 {
 	Scsi_Device *sdev;
 
-	sdev = (Scsi_Device *) kmalloc(sizeof(Scsi_Device), GFP_ATOMIC);
+	sdev = (Scsi_Device *) kmalloc(sizeof(Scsi_Device), GFP_KERNEL);
 	if (sdev == NULL)
 		printk(ALLOC_FAILURE_MSG, __FUNCTION__);
 	else {

@@ -616,7 +616,7 @@
 	int max_lgth = 255;
 
 retry:
-	evpd_page = kmalloc(max_lgth, GFP_ATOMIC |
+	evpd_page = kmalloc(max_lgth, GFP_KERNEL |
 			      (sdev->host->unchecked_isa_dma) ?
 			      GFP_DMA : 0);
 	if (!evpd_page) {

@@ -875,7 +875,7 @@
 	int max_lgth = 255;
 
 retry:
-	id_page = kmalloc(max_lgth, GFP_ATOMIC |
+	id_page = kmalloc(max_lgth, GFP_KERNEL |
 			      (sdev->host->unchecked_isa_dma) ?
 			      GFP_DMA : 0);
 	if (!id_page) {

@@ -982,7 +982,7 @@
 	int max_lgth = 255;
 
  retry:
-	serialnumber_page = kmalloc(max_lgth, GFP_ATOMIC |
+	serialnumber_page = kmalloc(max_lgth, GFP_KERNEL |
 			      (sdev->host->unchecked_isa_dma) ?
 			      GFP_DMA : 0);
 	if (!serialnumber_page) {

@@ -1337,7 +1337,7 @@
 	 * that can correctly scan.
 	 */
 	sdev->inquiry_len = sdevscan->inquiry_len;
-	sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
+	sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_KERNEL);
 	if (sdev->inquiry == NULL) {
 		scsi_free_sdev(sdev);
 		return SCSI_SCAN_NO_RESPONSE;

@@ -1521,7 +1521,7 @@
 	 * The sreq is for use only with sdevscan.
 	 */
 
-	scsi_result = kmalloc(256, GFP_ATOMIC |
+	scsi_result = kmalloc(256, GFP_KERNEL |
 			      (sdevscan->host->unchecked_isa_dma) ?
 			      GFP_DMA : 0);
 	if (scsi_result == NULL)

@@ -1763,7 +1763,7 @@
 	 * prevent us from finding any LUNs on this target.
 	 */
 	length = (max_scsi_report_luns + 1) * sizeof(ScsiLun);
-	lun_data = (ScsiLun *) kmalloc(length, GFP_ATOMIC |
+	lun_data = (ScsiLun *) kmalloc(length, GFP_KERNEL |
 					   (sdevscan->host->unchecked_isa_dma ?
 					    GFP_DMA : 0));
 	if (lun_data == NULL) {

@@ -2053,6 +2053,9 @@
 void scan_scsis(struct Scsi_Host *shost, uint hardcoded, uint hchannel,
 		uint hid, uint hlun)
 {
+	if (in_interrupt())
+		printk(KERN_ERR "scsi: scanning while in interrupt context!\n");
+
 	if (hardcoded == 1) {
 		/*
 		 * XXX Overload hchannel/hid/hlun to figure out what to

I'd still rather use BUG_ON, if we change from GFP_ATOMIC to GFP_KERNEL, since we're guaranteeing to kmalloc that we have a user context if it needs to sleep. Otherwise things that shouldn't work will be fine until the system runs low on memory an then they'll mysteriously panic with "scheduling in interrupt". It's the job of the caller to kmalloc to verify that it is running in a context that can sleep in low memory situations.

A printk is fine for diagnostic purposes, but I bet someone who has a problem device won't report the issue until they see a panic.

James

I did a call graph, but I discarded it since. IIRC it showed some paths, but perhaps I made a mistake. Please ignore it.

The "fix" did not go anywhere, actually. Arjan explained that if a box runs out of atomics, it's bad anyway. We reduced the queue size in the driver that the guy used.

-- Pete