Re: HPET regression in 2.6.26 versus 2.6.25 -- retried 2.6.27-rc3 patch (and patch method)

Re: HPET regression in 2.6.26 versus 2.6.25 -- retried 2.6.27-rc3 patch (and patch method)

[David Witbrodt]

> From: Mike Galbraith <efault@gmx.de>

> > Because of this, I would conclude that your patch for 2.6.27-rc3 was
> > doomed before you began, and we should look more carefully at the
> > commits from February instead of trying to revert at the 2.6.27 HEAD.

> I've only followed this thread casually, but I suspect that Yinghai is
> trying to determine whether or not 3def3d6d is in fact causing your
> regression directly, or whether it is interacting with other changes.

Yes, Yinghai's first response when I tried his 2.6.27-rc3 patch and the 
kernel still locked was "there is some other problem" (besides the changes
in 3def3d6d).

You may have misunderstood my meaning:  I was only saying that a further
experiment of mine showed that 3def3d6d... alone is not the problem, so
that trying to undo the changes from that commit all the way up near the
git HEAD was unlikely to work.  I did not mean that his patch was a bad
idea, or that it should not have been tried.  When Yinghai created that 
patch, I had not yet done that experiment.  I was just offering an 
explanation for why the patch did not succeed.


> What _could_ be happening is that 3def3d6d itself isn't directly causing
> your problem, rather interacting with earlier changes such that you see
> the problem as soon as 3def3d6d hit.

I am convinced this is the case, as a result of a bisection I performed
Wednesday night -- where I tried to revert the 3def3d6d changes alone in
kernel revisions much closer to that commit, only to discover that the
very next commit after 3def3d6d actually prevented the revert from working!


> Such cases can/do happen, and can cause much confusion.  To find such a
> problem without actually troubleshooting it (if such a thing is
> happening in your case), you'd have to work backward, ie apply 3def3d6d
> to earlier kernels and test.  If you find one earlier than 3def3d6d
> which works with 3def3d6d applied, you can be pretty sure that what
> you've got is a nasty interaction.  At that point, you'd start your
> bisection _with virgin source_ via git bisect good "the point that
> worked _with_ 3def3d6d applied", and git bisect bad any later point that
> failed.  During each and every bisection point, you'd have to apply
> 3def3d6d before testing (fixing any rejects), and _before_ saying git
> bisect good/bad after building/testing, you must revert it first so git
> bisect can proceed without encountering conflicts.

Thank you *very* much for this suggestion, Mike!  I am an "end user," not
a developer, so I never would have thought of this.

Your explanation of how to carry out this bisect process is very much like
the process I used on Wednesday night.  The only difference is that I 
manually edited the 3 files involved at each step, instead of trying to
patch using the 3def3d6d diff.  I can see that your way would have been
much easier, as long as I am careful to watch for rejected patches.

I have to go to work for a few hours, but should be able to put a lot of
time in on this method later today.  Much thanks!


> Before attempting any of that, you'd want to make damn sure that the
> simpler path (forward) is in fact good with the suspect commit fully
> reverted.  If the kernel with full revert does not work, and symptom
> does not change an any way (very important), it's not 3def3d6d directly.
> The patch forward then becomes git bisect start, git bisect good
> 3def3d6d, revert/restore 3def3d6d as above at each and every bisection
> interval until done.

Ahh, this is the experiment I performed Wednesday night.  The results:
3def3d6d is not the true cause.  Beginning with 3def3d6d..., it looks
like anything that touches insert_resource() causes kernels to lock up
on my two ECS AMD690GM-M2 mboards, but not on my other machine (or your
machine, or anyone else's machine....).


> Even if the kernel with full revert does work, it _can_ still be an
> interaction involving multiple changes.  Such interaction problems don't
> pop up often, and can be a real pain to dig out, so I hope it turns out
> to be something straight forward... and I just wasted all those words.

No such luck.  Can you say, "Preparation H"?  (I know I can....)


Thanks Mike,
Dave W.

Re: HPET regression in 2.6.26 versus 2.6.25 -- retried 2.6.27-rc3 patch (and patch method)

[Ingo Molnar]

* David Witbrodt <dawitbro@sbcglobal.net> wrote:

> I found something very interesting about the commit that first causes 
> the lockup (3def3d6d...), and the very next commit (1e934dda...) -- if 
> I checkout 1e94... and try to revert the changes made in 3def..., the 
> kernel freezes in spite of the revert.
> 
> Because of this, I would conclude that your patch for 2.6.27-rc3 was 
> doomed before you began, and we should look more carefully at the 
> commits from February instead of trying to revert at the 2.6.27 HEAD.

i'm still wondering whether we could try to figure out something about 
the nature of the hard lockup itself.

Have you tried to activate the NMI watchdog? It _usually_ works fine if 
you use a boot option along the lines of:

   "lapic nmi_watchdog=2 idle=poll"

The best test would be to first boot the broken kernel with also 
hpet=disable and the above options, and check in /proc/interrupts 
whether the NMI count is increasing. If the NMI watchdog is working, you 
should see a steady trickle of NMI irqs:

 rhea:~> while sleep 1; do grep NMI /proc/interrupts ; done
 NMI:       4395   Non-maskable interrupts
 NMI:       4396   Non-maskable interrupts
 NMI:       4397   Non-maskable interrupts
 NMI:       4398   Non-maskable interrupts
 ^C

if it does not work, you'll see:

 pluto:~> while sleep 1; do grep NMI /proc/interrupts ; done
 NMI:          0   Non-maskable interrupts
 NMI:          0   Non-maskable interrupts
 NMI:          0   Non-maskable interrupts
 NMI:          0   Non-maskable interrupts
 ^C

NOTE: the NMI watchdog disables high-res timers so it might change your 
test enough to make the lockup go away. Hopefully it wont :-)

So, in the ideal situation, your test of the NMI watchdog will show a 
steady trickle of watchdog NMI. Then i'd suggest to remove the 
hpet=disable, to provoke the lockup. Hopefully it occurs, _and_ after 
the hard lockup has happened, you should see a nice stack backtrace 
printed out by the NMI watchdog. That gives us the exact location of 
lockup.

One theory is that the changed resource allocations are buggy in certain 
circumstances and cause us to stomp over key kernel data structures. We 
could for example overwrite a networking lock - that's why you lock up 
in the networking code. hpet=disable deactivates those resource 
allocations and works around the symptoms of the bug.

	Ingo

Re: HPET regression in 2.6.26 versus 2.6.25 -- retried 2.6.27-rc3 patch (and patch method)

[David Witbrodt]

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



> > I used 'git apply --check ' first, and got no errors, so
> > I applied it, built, installed, and rebooted.
> 
> could apply with
> cat revert_*.;patch | patch -p1

Yinghai,

When I got home from work tonight, I decided to make sure that I applied
your patch correctly.  I considered trying it once with cat/patch and
once with 'git apply', and then comparing the results with 'diff'.  I
ended up becoming convinced that 'git apply' worked just fine, and
never tried the cat/patch method:

=================================
$ git show |head
commit 30a2f3c60a84092c8084dfe788b710f8d0768cd4
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Tue Aug 12 18:55:39 2008 -0700

    Linux 2.6.27-rc3

diff --git a/Makefile b/Makefile
index fd3ca6e..53bf6ec 100644
--- a/Makefile
+++ b/Makefile

$ git apply --check --verbose ../yh-patch1-2.6.27-rc3.diff 
Checking patch arch/x86/kernel/e820.c...
Checking patch arch/x86/kernel/setup.c...
Checking patch include/asm-x86/e820.h...

$ git apply --verbose ../yh-patch1-2.6.27-rc3.diff 
Checking patch arch/x86/kernel/e820.c...
Checking patch arch/x86/kernel/setup.c...
Checking patch include/asm-x86/e820.h...
Applied patch arch/x86/kernel/e820.c cleanly.
Applied patch arch/x86/kernel/setup.c cleanly.
Applied patch include/asm-x86/e820.h cleanly.

dawitbro@fileserver:~/sandbox/git-kernel/linux-2.6$ git status
# Not currently on any branch.
# Changed but not updated:
#   (use "git add <file>..." to update what will be committed)
#
#    modified:   arch/x86/kernel/e820.c
#    modified:   arch/x86/kernel/setup.c
#    modified:   include/asm-x86/e820.h
#
no changes added to commit (use "git add" and/or "git commit -a")

$ git diff > ../yh-patch1-2.6.27-rc3.git-apply.diff 
=================================

As you can see, I renamed your patch file to 'yh-patch1-2.6.27-rc3.diff', and
after applying your patch I created a diff against 2.6.27-rc3 called
'yh-patch1-2.6.27-rc3.git-apply.diff'.

I have attached the output of the following command so that you can see that
your patch applied correctly:

diff -y -W 200  yh-patch1-2.6.27-rc3.diff  yh-patch1-2.6.27-rc3.git-apply.diff

At this point, I retried a kernel with your patch...


> can you try enable kexec and kdump in you .config.
>
> it should works. my .config have config_kexec

I ran 'make oldconfig' to get my .config up to date, then 'make menuconfig'
to make sure I had enabled CONFIG_KEXEC and CONFIG_CRASH_DUMP:

$ egrep 'KEXEC|DUMP' .config
CONFIG_KEXEC=y
CONFIG_CRASH_DUMP=y


I took these steps, and posted this info, for the sake of our individual and
collective sanity!  ;-)  I want you all to be sure that I applied the patch
correctly, and adjusted the .config as requested, before building the kernel.

The kernel built without error, so I installed and rebooted.  It locked up in
the usual way.  I rebooted, using the "hpet=disable" parameter, and it booted
just fine, just like all the others since 3def3d6d...


>> I do not know how to bisect with your patch if I have a "bad" but no "good"
>> to start with.  Can you explain how I should proceed when I _do_ get home?
>> (I can just enabled those config options and try the patch again, but I am
>> confused about the bisect you are asking me to perform.)
>
> just like the old way doing git-bisect, but before compiling, apply
> the batch, and before git-bisect good or bad, revert the patch.

Since I am still confused about how I should perform the bisect you are
asking for, I will wait until you can clarify.  I responded earlier from
work, where I had no access to a Linux machine, so I could not quote the
git documentation I have read... to explain better why I am confused.  Here
are the sections of 'git help bisect' that I had in mind:

===== BEGIN QUOTED SECTIONS =================
[...]
This command uses git-rev-list --bisect option to help drive the binary
search process to find which change introduced a bug, given an old
"good" commit object name and a later "bad" commit object name.

[...]
Basic bisect commands: start, bad, good

The way you use it is:


$ git bisect start
$ git bisect bad                 # Current version is bad
$ git bisect good v2.6.13-rc2    # v2.6.13-rc2 was the last version
                                 # tested that was good
When you give at least one bad and one good versions, it will bisect
the revision tree and say something like:

Bisecting: 675 revisions left to test after this

[...]
and you continue along, compiling that one, testing it, and depending
on whether it is good or bad, you say "git bisect good" or "git bisect
bad", and ask for the next bisection.
===== END QUOTED SECTIONS =================

If there is a way to use 'git bisect' beginning with a "bad" version
but no "good" version, then it is an advanced usage that I have not
read about and do not understand how to use.  As soon as you tell me
how to carry out the process, I will do so and report the results.


In the meantime, can you comment on the bisection I did last night?
I found something very interesting about the commit that first causes
the lockup (3def3d6d...), and the very next commit (1e934dda...) -- if
I checkout 1e94... and try to revert the changes made in 3def..., the
kernel freezes in spite of the revert.

Because of this, I would conclude that your patch for 2.6.27-rc3 was
doomed before you began, and we should look more carefully at the
commits from February instead of trying to revert at the 2.6.27 HEAD.

I am not a kernel developer, so my opinions are probably safe to ignore,
but I think we should be trying to extract information from my (faulty)
machines about what is happening differently between the "bad" commit
3def3d6d... and the "good" commit before it.


I am too tired to continue my experiments tonight -- it is summer 
semester final exam time at the college where I tutor, and answer 
questions about calculus 2, linear algebra, nursing math, etc., has 
worn me right out -- but while I wait to find out the next step I
should do with the 2.6.27-rc3 patch, I am going to try to get some
useful info printed to the console before the kernel locks up back
in those Feb. revisions.


Thanks,
Dave W.

[-- Attachment #2: yh-patch1-2.6.27-rc3.compare.txt --]
[-- Type: text/plain, Size: 8169 bytes --]

[PATCH] x86: revert back to use request_resource to reserve kernel res				   |	diff --git a/arch/x86/kernel/e820.c b/arch/x86/kernel/e820.c
												   |	index 9af8907..feff5b7 100644
Signed-off-by: Yinghai Lu <yhlu.kernel@gmail.com>						   |	--- a/arch/x86/kernel/e820.c
												   |	+++ b/arch/x86/kernel/e820.c
---												   |	@@ -1267,7 +1267,7 @@ static inline const char *e820_type_to_string(int e820_type)
 arch/x86/kernel/e820.c  |   17 +++++++++++++++--						   <
 arch/x86/kernel/setup.c |   22 +++++++++++++++-------						   <
 include/asm-x86/e820.h  |    3 ++-								   <
 3 files changed, 32 insertions(+), 10 deletions(-)						   <
												   <
Index: linux-2.6/arch/x86/kernel/e820.c								   <
===================================================================				   <
--- linux-2.6.orig/arch/x86/kernel/e820.c							   <
+++ linux-2.6/arch/x86/kernel/e820.c								   <
@@ -1267,7 +1267,7 @@ static inline const char *e820_type_to_s					   <
 /*													 /*
  * Mark e820 reserved areas as busy for the resource manager.						  * Mark e820 reserved areas as busy for the resource manager.
  */													  */
-void __init e820_reserve_resources(void)								-void __init e820_reserve_resources(void)
+void __init e820_reserve_resources(struct resource **res_kernel, int nr_res_k)				+void __init e820_reserve_resources(struct resource **res_kernel, int nr_res_k)
 {													 {
 	int i;												 	int i;
 	struct resource *res;										 	struct resource *res;
@@ -1287,7 +1287,20 @@ void __init e820_reserve_resources(void)						@@ -1287,7 +1287,20 @@ void __init e820_reserve_resources(void)
 		res->end = end;										 		res->end = end;
 													 
 		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;						 		res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-		insert_resource(&iomem_resource, res);							-		insert_resource(&iomem_resource, res);
+		request_resource(&iomem_resource, res);							+		request_resource(&iomem_resource, res);
+		if (e820.map[i].type == E820_RAM) {							+		if (e820.map[i].type == E820_RAM) {
+			int j;										+			int j;
+			/*										+			/*
+			* We don't know which RAM region contains kernel data,				+			* We don't know which RAM region contains kernel data,
+			* so we try it repeatedly and let the resource manager				+			* so we try it repeatedly and let the resource manager
+			* test it.									+			* test it.
+			*/										+			*/
+			for (j = 0; j < nr_res_k; j++) {						+			for (j = 0; j < nr_res_k; j++) {
+				if (!res_kernel[j])							+				if (!res_kernel[j])
+					continue;							+					continue;
+				request_resource(res, res_kernel[j]);					+				request_resource(res, res_kernel[j]);
+			}										+			}
+		}											+		}
 		res++;											 		res++;
 	}												 	}
 													 
Index: linux-2.6/arch/x86/kernel/setup.c							   |	diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
===================================================================				   |	index 68b48e3..f82c50b 100644
--- linux-2.6.orig/arch/x86/kernel/setup.c							   |	--- a/arch/x86/kernel/setup.c
+++ linux-2.6/arch/x86/kernel/setup.c								   |	+++ b/arch/x86/kernel/setup.c
@@ -437,6 +437,7 @@ static void __init reserve_early_setup_d					   |	@@ -437,6 +437,7 @@ static void __init reserve_early_setup_data(void)
  * --------- Crashkernel reservation ------------------------------					  * --------- Crashkernel reservation ------------------------------
  */													  */
 													 
+static struct resource *crashk_res_ptr;								+static struct resource *crashk_res_ptr;
 #ifdef CONFIG_KEXEC											 #ifdef CONFIG_KEXEC
 													 
 /**													 /**
@@ -517,7 +518,7 @@ static void __init reserve_crashkernel(v					   |	@@ -517,7 +518,7 @@ static void __init reserve_crashkernel(void)
 													 
 	crashk_res.start = crash_base;									 	crashk_res.start = crash_base;
 	crashk_res.end   = crash_base + crash_size - 1;							 	crashk_res.end   = crash_base + crash_size - 1;
-	insert_resource(&iomem_resource, &crashk_res);							-	insert_resource(&iomem_resource, &crashk_res);
+	crashk_res_ptr = &crashk_res;									+	crashk_res_ptr = &crashk_res;
 }													 }
 #else													 #else
 static void __init reserve_crashkernel(void)								 static void __init reserve_crashkernel(void)
@@ -593,6 +594,9 @@ struct x86_quirks *x86_quirks __initdata					   |	@@ -593,6 +594,9 @@ struct x86_quirks *x86_quirks __initdata = &default_x86_quirks;
 													 
 void __init setup_arch(char **cmdline_p)								 void __init setup_arch(char **cmdline_p)
 {													 {
+	struct resource *res_kernel[4];									+	struct resource *res_kernel[4];
+	int num_res;											+	int num_res;
+													+
 #ifdef CONFIG_X86_32											 #ifdef CONFIG_X86_32
 	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));					 	memcpy(&boot_cpu_data, &new_cpu_data, sizeof(new_cpu_data));
 	visws_early_detect();										 	visws_early_detect();
@@ -699,11 +703,6 @@ void __init setup_arch(char **cmdline_p)						@@ -699,11 +703,6 @@ void __init setup_arch(char **cmdline_p)
 	probe_roms();											 	probe_roms();
 #endif													 #endif
 													 
-	/* after parse_early_param, so could debug it */						-	/* after parse_early_param, so could debug it */
-	insert_resource(&iomem_resource, &code_resource);						-	insert_resource(&iomem_resource, &code_resource);
-	insert_resource(&iomem_resource, &data_resource);						-	insert_resource(&iomem_resource, &data_resource);
-	insert_resource(&iomem_resource, &bss_resource);						-	insert_resource(&iomem_resource, &bss_resource);
-													-
 	if (efi_enabled)										 	if (efi_enabled)
 		efi_init();										 		efi_init();
 													 
@@ -865,7 +864,16 @@ void __init setup_arch(char **cmdline_p)					   |	@@ -863,7 +862,16 @@ void __init setup_arch(char **cmdline_p)
 													 
 	kvm_guest_init();										 	kvm_guest_init();
 													 
-	e820_reserve_resources();									-	e820_reserve_resources();
+	res_kernel[0] = &code_resource;									+	res_kernel[0] = &code_resource;
+	res_kernel[1] = &data_resource;									+	res_kernel[1] = &data_resource;
+	res_kernel[2] = &bss_resource;									+	res_kernel[2] = &bss_resource;
+	num_res = 3;											+	num_res = 3;
+	if (crashk_res_ptr) {										+	if (crashk_res_ptr) {
+		res_kernel[num_res] = crashk_res_ptr;							+		res_kernel[num_res] = crashk_res_ptr;
+		num_res++;										+		num_res++;
+	}												+	}
+	e820_reserve_resources(res_kernel, num_res);							+	e820_reserve_resources(res_kernel, num_res);
+													+
 	e820_mark_nosave_regions(max_low_pfn);								 	e820_mark_nosave_regions(max_low_pfn);
 													 
 #ifdef CONFIG_X86_32											 #ifdef CONFIG_X86_32
Index: linux-2.6/include/asm-x86/e820.h								   |	diff --git a/include/asm-x86/e820.h b/include/asm-x86/e820.h
===================================================================				   |	index 16a31e2..a574ad9 100644
--- linux-2.6.orig/include/asm-x86/e820.h							   |	--- a/include/asm-x86/e820.h
+++ linux-2.6/include/asm-x86/e820.h								   |	+++ b/include/asm-x86/e820.h
@@ -120,7 +120,8 @@ extern void e820_register_active_regions					   |	@@ -119,7 +119,8 @@ extern void e820_register_active_regions(int nid, unsigned long start_pfn,
 					 unsigned long end_pfn);					 					 unsigned long end_pfn);
 extern u64 e820_hole_size(u64 start, u64 end);								 extern u64 e820_hole_size(u64 start, u64 end);
 extern void finish_e820_parsing(void);									 extern void finish_e820_parsing(void);
-extern void e820_reserve_resources(void);								-extern void e820_reserve_resources(void);
+struct resource;											+struct resource;
+extern void e820_reserve_resources(struct resource **res, int num);					+extern void e820_reserve_resources(struct resource **res, int num);
 extern void setup_memory_map(void);									 extern void setup_memory_map(void);
 extern char *default_machine_specific_memory_setup(void);						 extern char *default_machine_specific_memory_setup(void);
 extern char *machine_specific_memory_setup(void);							 extern char *machine_specific_memory_setup(void);