Fix crash on boot in kmalloc_node IDE changes

PATCH In review Andi Kleen 5 participants 24 messages Jul 6, 2005 – Jul 8, 2005

Subsystems (2)

Memory management libata

lore

Share & tools

lore

Andi Kleen

author

2005-07-06 13:30 UTC #0 lore raw

PATCH 1/1 Fix crash on boot in kmalloc_node IDE changes

Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.

Signed-off-by Andi Kleen <[email protected]>

drivers/ide/ide-probe.c +11 -4

Index: linux/drivers/ide/ide-probe.c
===================================================================
@@ -978,8 +978,10 @@ static int ide_init_queue(ide_drive_t *d
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	int node = 0; /* Should be -1 */
+	if (drive->hwif && drive->hwif->pci_dev)
+		node = pcibus_to_node(drive->hwif->pci_dev->bus); 
+	q = blk_init_queue_node(do_ide_request, &ide_lock, node);
 	if (!q)
 		return 1;

@@ -1096,8 +1098,13 @@ static int init_irq (ide_hwif_t *hwif)
 		hwgroup->hwif->next = hwif;
 		spin_unlock_irq(&ide_lock);
 	} else {
-		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+		int node = 0; 
+		if (hwif->drives[0].hwif) { 
+			struct pci_dev *pdev = hwif->drives[0].hwif->pci_dev;  
+			if (pdev)
+				node = pcibus_to_node(pdev->bus);
+		}
+		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,node);
 		if (!hwgroup)
 	       		goto out_up;

Bartlomiej Zolnierkiewicz

2005-07-06 14:05 UTC #1 lore raw

Quoted 14 lines · Andi Kleen

Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.
Signed-off-by Andi Kleen <[email protected]>
Index: linux/drivers/ide/ide-probe.c
===================================================================
drivers/ide/ide-probe.c +0 -0
@@ -978,8 +978,10 @@ static int ide_init_queue(ide_drive_t *d
         *      do not.

drivers/ide/ide-probe.c static int ide_init_queue(ide_drive_t *d · line 986

Quoted 10 lines

drivers/ide/ide-probe.c +4 -2

         */
 
-       q = blk_init_queue_node(do_ide_request, &ide_lock,
-                               pcibus_to_node(drive->hwif->pci_dev->bus));
+       int node = 0; /* Should be -1 */
+       if (drive->hwif && drive->hwif->pci_dev)
+               node = pcibus_to_node(drive->hwif->pci_dev->bus);
+       q = blk_init_queue_node(do_ide_request, &ide_lock, node);
        if (!q)
                return 1;

drive->hwif check is redundant, please remove it

drivers/ide/ide-probe.c static int init_irq (ide_hwif_t *hwif) · line 1106

Quoted 12 lines

drivers/ide/ide-probe.c +6 -2

@@ -1096,8 +1098,13 @@ static int init_irq (ide_hwif_t *hwif)
                hwgroup->hwif->next = hwif;
                spin_unlock_irq(&ide_lock);
        } else {
-               hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-                       pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+               int node = 0;
+               if (hwif->drives[0].hwif) {
+                       struct pci_dev *pdev = hwif->drives[0].hwif->pci_dev;
+                       if (pdev)
+                               node = pcibus_to_node(pdev->bus);
+               }

ditto, moreover hwif->drives[0].hwif == hwif

Quoted 3 lines

drivers/ide/ide-probe.c +1 -0

+               hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,node);
                if (!hwgroup)
                        goto out_up;

Andi Kleen

author

2005-07-06 14:09 UTC #2 lore raw

Quoted 1 line

drive->hwif check is redundant, please remove it

It's not. My first version didn't have it but it still crashed. It's what actually prevents the crash.
I also don't know why, but it's true.

The machine had four IDE controllers BTW (on board an an external Promise card)

-Andi

Andi Kleen

author

2005-07-06 18:22 UTC #4 lore raw

Quoted 7 lines · On Wed, 6 Jul 2005 16:35:11 +0200 Bartlomiej Zolnierkiewicz…

Quoted 5 lines · Andi Kleen
Quoted 1 line
drive->hwif check is redundant, please remove it
It's not. My first version didn't have it but it still crashed. It's what actually prevents the crash.
I also don't know why, but it's true.

Quoted 8 lines

very weird as HWIF(drive) == drive->hwif:

	ide_hwif_t *hwif = HWIF(drive);

...

	q = blk_init_queue_node(do_ide_request, &ide_lock,
				pcibus_to_node(drive->hwif->pci_dev->bus));

                                               ^^^^^^^^^^^^^^

I oops on that one. Maybe it takes the early return.

Quoted 8 lines

	if (!q)
		return 1;
...
	if (!hwif->rqsize) {
you should OOPS here

I would appreciate if some form of this patch is merged asap because it breaks booting on my test machines.

-Andi

Andi Kleen

author

2005-07-06 16:45 UTC #6 lore raw

Quoted 7 lines · Christoph Lameter

drivers/ide/ide-probe.c static int ide_init_queue(ide_drive_t *d · line 981
Quoted 3 lines · Andi Kleen
drivers/ide/ide-probe.c +1 -2
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	int node = 0; /* Should be -1 */
Why is this not -1?

Because there is no code in rc3 that handles -1 in kmalloc_node.

If you add a patch that handles it then feel free to change. But fixing the bootup has the highest priority.

Quoted 5 lines

drivers/ide/ide-probe.c static int init_irq (ide_hwif_t *hwif) · line 1102
Quoted 3 lines
drivers/ide/ide-probe.c +2 -0
+		int node = 0; 
+		if (hwif->drives[0].hwif) { 
Also needs to be -1.

Then it would crash again.

-Andi

Christoph Lameter

2005-07-06 17:07 UTC #7 lore raw

Quoted 15 lines · Andi Kleen

Quoted 7 lines · Christoph Lameter
Review
Quoted 3 lines · Andi Kleen
(quoted diff) +1 -2
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	int node = 0; /* Should be -1 */
Why is this not -1?
Because there is no code in rc3 that handles -1 in kmalloc_node.
If you add a patch that handles it then feel free to change. But fixing the bootup has the highest priority.
Quoted 1 line

Quoted 6 lines

Quoted 4 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		int node = 0; 
+		if (hwif->drives[0].hwif) { 
Also needs to be -1.
Then it would crash again.

The patch follows. Maybe add that and include a signoff by me?:

mm/slab.c +3 -0

Index: linux-2.6.git/mm/slab.c
===================================================================
@@ -2372,6 +2372,9 @@
 	struct slab *slabp;
 	kmem_bufctl_t next;
 
+	if (nodeid == -1)
+		return kmem_cache_alloc_node(cachep, flags);
+
 	for (loop = 0;;loop++) {
 		struct list_head *q;

Christoph Lameter

2005-07-07 16:22 UTC #8 lore raw

PATCH 1/1 Fix crash on boot in kmalloc_node IDE changes

In reply to Andi Kleen

Quoted 3 lines · Andi Kleen

Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.

Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to. Maybe we need a hwif_to_node for ide drivers that is also able to determine the locality of other hardware?

Signed-off-by Christoph Lameter <[email protected]>

drivers/ide/ide-probe.c +4 -2

Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
@@ -979,7 +979,8 @@
 	 */
 
 	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+				(drive->hwif && drive->hwif->pci_dev) ?
+				pcibus_to_node(drive->hwif->pci_dev->bus): -1);
 	if (!q)
 		return 1;

@@ -1097,7 +1098,8 @@
 		spin_unlock_irq(&ide_lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+			(hwif->drives[0].hwif && hwif->drives[0].hwif->pci_dev) ?
+			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus) : -1);
 		if (!hwgroup)
 	       		goto out_up;

Andi Kleen

author

2005-07-07 16:24 UTC #9 lore raw

Quoted 10 lines · Christoph Lameter

Quoted 3 lines · Andi Kleen
Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.
Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to. Maybe we need a hwif_to_node for ide drivers that is also able to determine the locality of other hardware?

Hmm? Where is the difference?

This is 100% equivalent to my code except that you compressed it all into a single expression.

The former one was more readable.

-Andi

Christoph Lameter

2005-07-07 16:32 UTC #10 lore raw

Quoted 7 lines · Andi Kleen

Quoted 6 lines · Christoph Lameter
Quoted 3 lines · Andi Kleen
Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.

Quoted 6 lines

Quoted 4 lines
Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to. Maybe we need a hwif_to_node for ide drivers that is also able to determine the locality of other hardware?
Hmm? Where is the difference?

node = -1 if the node cannot be determined.

Quoted 2 lines

This is 100% equivalent to my code except that you compressed it all into a single expression.

My patch consistently checks for hwif != NULL and pci_dev != NULL. There was someother stuff in your patch. This patch does not add any additional variables and is more readable.

Andi Kleen

author

2005-07-07 16:36 UTC #11 lore raw

Quoted 9 lines · Christoph Lameter

Quoted 7 lines · Andi Kleen
Quoted 6 lines · Christoph Lameter
Quoted 3 lines · Andi Kleen
Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.

Quoted 8 lines

Quoted 6 lines
Quoted 4 lines
Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to. Maybe we need a hwif_to_node for ide drivers that is also able to determine the locality of other hardware?
Hmm? Where is the difference?
node = -1 if the node cannot be determined.

But that will crash right now.

Trading one crash for another doesn't seem to be particularly useful.

Quoted 6 lines

Quoted 2 lines
This is 100% equivalent to my code except that you compressed it all into a single expression.
My patch consistently checks for hwif != NULL and pci_dev != NULL. There was someother stuff in your patch. This patch does not add any additional variables and is more readable.

No it was exactly the same except for a working node number.

-Andi

Christoph Lameter

2005-07-07 17:15 UTC #12 lore raw

Quoted 3 lines · Andi Kleen

Quoted 1 line
node = -1 if the node cannot be determined.
But that will crash right now.

That was fixed. Have a look at the git logs.

Bartlomiej Zolnierkiewicz

2005-07-07 16:46 UTC #13 lore raw

In reply to Christoph Lameter

Quoted 16 lines · Christoph Lameter

Quoted 7 lines · Andi Kleen
Quoted 6 lines · Christoph Lameter
Quoted 3 lines · Andi Kleen
Without this patch a dual Xeon EM64T machine would oops on boot because the hwif pointer here was NULL. I also added a check for pci_dev because it's doubtful that all IDE devices have pci_devs.
Quoted 6 lines
Quoted 4 lines
Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to. Maybe we need a hwif_to_node for ide drivers that is also able to determine the locality of other hardware?
Hmm? Where is the difference?

Quoted 8 lines

node = -1 if the node cannot be determined.
Quoted 2 lines
This is 100% equivalent to my code except that you compressed it all into a single expression.
My patch consistently checks for hwif != NULL and pci_dev != NULL. There was someother stuff in your patch. This patch does not add any additional variables and is more readable.

seconded but hwif != NULL still just hides some other issue

* hwifs and drives are not allocated dynamically
* drive->hwif is initialized to hwif for every hwif very early in the driver initialization

Linus Torvalds

2005-07-07 16:38 UTC #14 lore raw

In reply to Christoph Lameter

Quoted 4 lines · Christoph Lameter

Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to.

I think this is pretty unreadable.

If you make it use a trivial inline function for the thing, I think that would be ok, though.

Complex expressions are bad.

Linus

Andi Kleen

author

2005-07-07 16:40 UTC #15 lore raw

Quoted 8 lines · Linus Torvalds

Quoted 4 lines · Christoph Lameter
Here is IMHO the right way to fix this. Test for the hwif != NULL and test for pci_dev != NULL before determining the node number of the pci bus that the device is connected to.

Quoted 6 lines

I think this is pretty unreadable.
If you make it use a trivial inline function for the thing, I think that would be ok, though.
Complex expressions are bad.

You could just use the previous patch I posted (hint hint...;)

-Andi

Christoph Lameter

2005-07-07 17:23 UTC #16 lore raw

In reply to Linus Torvalds

Quoted 2 lines · Linus Torvalds

If you make it use a trivial inline function for the thing, I think that would be ok, though.

Like this?

drivers/ide/ide-probe.c +11 -3

Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
@@ -960,6 +960,15 @@
 }
 #endif /* MAX_HWIFS > 1 */
 
+static inline int hwif_to_node(ide_hwif_t *hwif)
+{
+	if (hwif && hwif->pci_dev)
+		return pcibus_to_node(hwif->pci_dev->bus);
+	else
+		/* Add ways to determine the node of other busses here */
+		return -1;
+}
+
 /*
  * init request queue
  */

@@ -978,8 +987,7 @@
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	q = blk_init_queue_node(do_ide_request, &ide_lock, hwif_to_node(hwif));
 	if (!q)
 		return 1;

@@ -1097,7 +1105,7 @@
 		spin_unlock_irq(&ide_lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+					hwif_to_node(hwif->drives[0].hwif));
 		if (!hwgroup)
 	       		goto out_up;

Jens Axboe

2005-07-07 17:31 UTC #17 lore raw

Quoted 13 lines · Christoph Lameter

Quoted 2 lines · Linus Torvalds
If you make it use a trivial inline function for the thing, I think that would be ok, though.
Like this?
Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
drivers/ide/ide-probe.c +0 -0
@@ -960,6 +960,15 @@
 }

drivers/ide/ide-probe.c line 970

Quoted 10 lines

drivers/ide/ide-probe.c +8 -0

 #endif /* MAX_HWIFS > 1 */
 
+static inline int hwif_to_node(ide_hwif_t *hwif)
+{
+	if (hwif && hwif->pci_dev)
+		return pcibus_to_node(hwif->pci_dev->bus);
+	else
+		/* Add ways to determine the node of other busses here */
+		return -1;
+}

When is hwif ever NULL?

Signature · 1 line

Jens Axboe

Linus Torvalds

2005-07-07 18:57 UTC #18 lore raw

In reply to Christoph Lameter

Quoted 7 lines · Christoph Lameter

Quoted 2 lines · Linus Torvalds
If you make it use a trivial inline function for the thing, I think that would be ok, though.
Like this?

Yes. Except that if hwif is NULL, we'll have other oopses since we access that in other places.

Why _is_ hwif NULL anyway? That's another, unrelated thing, and should probably have a separate check and an early return.

So there's two unrelated issues: hwif being NULL is a serious error regardless of any node issues, and pci_dev being NULL just means we aren't a PCI device.

Bartlomiej?

Linus

Christoph Lameter

2005-07-07 19:09 UTC #19 lore raw

Quoted 5 lines · Linus Torvalds

Yes. Except that if hwif is NULL, we'll have other oopses since we access that in other places.
Why _is_ hwif NULL anyway? That's another, unrelated thing, and should probably have a separate check and an early return.

I was wondering about that one as well. Andi brought it up.

Andi Kleen

author

2005-07-07 21:15 UTC #20 lore raw

Quoted 9 lines · Christoph Lameter

Quoted 5 lines · Linus Torvalds
Yes. Except that if hwif is NULL, we'll have other oopses since we access that in other places.
Why _is_ hwif NULL anyway? That's another, unrelated thing, and should probably have a separate check and an early return.
I was wondering about that one as well. Andi brought it up.

I don't know why hwif was NULL, but my kernel definitely crashed. hwif was NULL in the first function (I first misread the oops and thought it was pci_dev NULL, but it wasn't). For the second I didn't verify it was hwif or pci_dev NULL, but one of them was too.

The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM and a external Promise PATA controller with two PATA disks.

-Andi

Bartlomiej Zolnierkiewicz

2005-07-07 21:19 UTC #21 lore raw

Quoted 11 lines · Andi Kleen

Quoted 9 lines · Christoph Lameter
Quoted 5 lines · Linus Torvalds
Yes. Except that if hwif is NULL, we'll have other oopses since we access that in other places.
Why _is_ hwif NULL anyway? That's another, unrelated thing, and should probably have a separate check and an early return.
I was wondering about that one as well. Andi brought it up.

Quoted 8 lines

I don't know why hwif was NULL, but my kernel definitely crashed. hwif was NULL in the first function (I first misread the oops and thought it was pci_dev NULL, but it wasn't). For the second I didn't verify it was hwif or pci_dev NULL, but one of them was too.
The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM and a external Promise PATA controller with two PATA disks.

actual OOPS would be very useful

Andi Kleen

author

2005-07-07 21:25 UTC #22 lore raw

Quoted 4 lines

Quoted 2 lines
The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM and a external Promise PATA controller with two PATA disks.
actual OOPS would be very useful

It's difficult because I don't have serial on that machine.

-Andi

Christoph Lameter

2005-07-08 04:44 UTC #23 lore raw

Quoted 6 lines · Andi Kleen

Quoted 4 lines
Quoted 2 lines
The setup was a Intel board with 1 PATA/4 SATA onboard and only a CD-ROM and a external Promise PATA controller with two PATA disks.
actual OOPS would be very useful
It's difficult because I don't have serial on that machine.

Maybe we can settle on this patch until we know for sure that the hwif is also a problem. In that case we can simply patch hwif_to_node:

drivers/ide/ide-probe.c +11 -3

Index: linux-2.6.git/drivers/ide/ide-probe.c
===================================================================
@@ -960,6 +960,15 @@
 }
 #endif /* MAX_HWIFS > 1 */
 
+static inline int hwif_to_node(ide_hwif_t *hwif)
+{
+	if (hwif->pci_dev)
+		return pcibus_to_node(hwif->pci_dev->bus);
+	else
+		/* Add ways to determine the node of other busses here */
+		return -1;
+}
+
 /*
  * init request queue
  */

@@ -978,8 +987,7 @@
 	 *	do not.
 	 */
 
-	q = blk_init_queue_node(do_ide_request, &ide_lock,
-				pcibus_to_node(drive->hwif->pci_dev->bus));
+	q = blk_init_queue_node(do_ide_request, &ide_lock, hwif_to_node(hwif));
 	if (!q)
 		return 1;

@@ -1097,7 +1105,7 @@
 		spin_unlock_irq(&ide_lock);
 	} else {
 		hwgroup = kmalloc_node(sizeof(ide_hwgroup_t), GFP_KERNEL,
-			pcibus_to_node(hwif->drives[0].hwif->pci_dev->bus));
+					hwif_to_node(hwif->drives[0].hwif));
 		if (!hwgroup)
 	       		goto out_up;