Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[David Witbrodt]

[Yinghai, please note that I did not request a patch to revert the
problem commit.  I was merely experimenting -- on my own time, so
you folks would not have to bother -- to see if I could make it
work.  I should have made that more clear!  Having said that, I am
glad to test changes of any kind on my machine:  reverts, code for
debugging or info, experiments, etc.]


I have two agendas here:

1.  A selfish interest in seeing future kernels work properly on
my "server" machines.  (I did not request a revert because I'm 
not yet convinced that the commit which introduced the problem
was broken.  That commit certainly is NOT broken on 1 of my 3
machines, and on none of your machines.  It may just expose
breakage elsewhere.)

2.  To provide a patch for Debian against the 2.6.26 line, in case
they get flooded with bug reports when their next stable release
happens.  (Such a flood is unlikely since no one here has the same
problem, and the users of Debian unstable haven't filed similar bug
reports in the last 2 weeks.  But, just in case...)


Pursuing #2 above, my first experiment was to attempt a revert for
2.6.26-rc1.  The files involved are much more similar to those in 
the problem commit, so performing the revert was much easier than
what I tried with 2.6.27-rc3.  I hoped to inch my way forward from
2.6.26-rc1 to 2.6.26, and end up with a patch useful for the Debian
Kernel Team.

Assuming I didn't botch the revert -- and I successfully carried
out this revert yesterday using the git version where the lockup
first occured -- it looks like there were enough changes between
Feb. 22 and May 3 that the simple, naive sort of revert that I
can do already cannot work.  The kernel freezes at boot, in spite
of the changes.


I will continue to try to work on identifying the first commit for
which my naive revert process will not work.  If I play a game 
where revertible = "good" and nonrevertible = "bad", I can use 
'git bisect' to locate the place where it no longer works.  I 
already have a "good" (3def3d6d...) and a "bad" (v2.6.26-rc1) by 
those rules.

If this process succeeds, it could provide evidence about why
reverting in 2.6.27 is not working.


====== DETAILS ==================

$ git show
commit 2ddcca36c8bcfa251724fe342c8327451988be0d
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Sat May 3 11:59:44 2008 -0700

    Linux 2.6.26-rc1

diff --git a/Makefile b/Makefile
index 5cf8258..4492984 100644
--- a/Makefile
+++ b/Makefile
@@ -1,7 +1,7 @@
 VERSION = 2
 PATCHLEVEL = 6
-SUBLEVEL = 25
-EXTRAVERSION =
+SUBLEVEL = 26
+EXTRAVERSION = -rc1
 NAME = Funky Weasel is Jiggy wit it
 
 # *DOCUMENTATION*

$ git diff
diff --git a/arch/x86/kernel/e820_64.c b/arch/x86/kernel/e820_64.c
index 124480c..e168fa0 100644
--- a/arch/x86/kernel/e820_64.c
+++ b/arch/x86/kernel/e820_64.c
@@ -325,7 +325,8 @@ unsigned long __init e820_end_of_ram(void)
 /*
  * Mark e820 reserved areas as busy for the resource manager.
  */
-void __init e820_reserve_resources(void)
+void __init e820_reserve_resources(struct resource *code_resource,
+        struct resource *data_resource, struct resource *bss_resource)
 {
     int i;
     struct resource *res;
@@ -341,7 +342,21 @@ void __init e820_reserve_resources(void)
         res->start = e820.map[i].addr;
         res->end = res->start + e820.map[i].size - 1;
         res->flags = IORESOURCE_MEM | IORESOURCE_BUSY;
-        insert_resource(&iomem_resource, res);
+        request_resource(&iomem_resource, res);
+        if (e820.map[i].type == E820_RAM) {
+            /*
+             * We don't know which RAM region contains kernel data,
+             * so we try it repeatedly and let the resource manager
+             * test it.
+             */
+            request_resource(res, code_resource);
+            request_resource(res, data_resource);
+            request_resource(res, bss_resource);
+#ifdef CONFIG_KEXEC
+            if (crashk_res.start != crashk_res.end)
+                request_resource(res, &crashk_res);
+#endif
+        }
         res++;
     }
 }
diff --git a/arch/x86/kernel/setup_64.c b/arch/x86/kernel/setup_64.c
index 22c14e2..368c672 100644
--- a/arch/x86/kernel/setup_64.c
+++ b/arch/x86/kernel/setup_64.c
@@ -254,7 +254,6 @@ static void __init reserve_crashkernel(void)
                 (unsigned long)(total_mem >> 20));
         crashk_res.start = crash_base;
         crashk_res.end   = crash_base + crash_size - 1;
-        insert_resource(&iomem_resource, &crashk_res);
     }
 }
 #else
@@ -365,11 +364,6 @@ void __init setup_arch(char **cmdline_p)
 
     finish_e820_parsing();
 
-    /* after parse_early_param, so could debug it */
-    insert_resource(&iomem_resource, &code_resource);
-    insert_resource(&iomem_resource, &data_resource);
-    insert_resource(&iomem_resource, &bss_resource);
-
     early_gart_iommu_check();
 
     e820_register_active_regions(0, 0, -1UL);
@@ -512,7 +506,7 @@ void __init setup_arch(char **cmdline_p)
     /*
      * We trust e820 completely. No explicit ROM probing in memory.
      */
-    e820_reserve_resources();
+    e820_reserve_resources(&code_resource, &data_resource, &bss_resource);
     e820_mark_nosave_regions();
 
     /* request I/O space for devices used on all i[345]86 PCs */
diff --git a/include/asm-x86/e820_64.h b/include/asm-x86/e820_64.h
index 71c4d68..10d6288 100644
--- a/include/asm-x86/e820_64.h
+++ b/include/asm-x86/e820_64.h
@@ -26,7 +26,8 @@ extern void update_memory_range(u64 start, u64 size, unsigned old_type,
 extern void setup_memory_region(void);
 extern void contig_e820_setup(void);
 extern unsigned long e820_end_of_ram(void);
-extern void e820_reserve_resources(void);
+extern void e820_reserve_resources(struct resource *code_resource,
+        struct resource *data_resource, struct resource *bss_resource);
 extern void e820_mark_nosave_regions(void);
 extern int e820_any_mapped(unsigned long start, unsigned long end,
                unsigned type);

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[Bill Fink]

Hi David,

On Wed, 13 Aug 2008, David Witbrodt wrote:

> [Yinghai, please note that I did not request a patch to revert the
> problem commit.  I was merely experimenting -- on my own time, so
> you folks would not have to bother -- to see if I could make it
> work.  I should have made that more clear!  Having said that, I am
> glad to test changes of any kind on my machine:  reverts, code for
> debugging or info, experiments, etc.]

I'm not sure Yinghai's revert patch is completely equivalent to
a revert of the original problematic commit, by a side-by-side
comparison of the original commit with his recent revert patch,
but then I don't really know that code at all.

In the original code there was a section (in e820_reserve_resources()):

#ifdef CONFIG_KEXEC
		   if (crashk_res.start != crashk_res.end)
			   request_resource(res, &crashk_res);
#endif

If you don't have CONFIG_KEXEC defined in your .config, which is
probably the case, then you would never request a crashk_res resource.
But in the code after the original commit, it unconditionally calls
(in reserve_crashkernel()):

	   crashk_res.start = crash_base;
	   crashk_res.end   = crash_base + crash_size - 1;
	   insert_resource(&iomem_resource, &crashk_res);

And after Yinghai's revert patch it still does (in reserve_crashkernel()):

        crashk_res.start = crash_base;
        crashk_res.end   = crash_base + crash_size - 1;
        crashk_res_ptr = &crashk_res;

and (in setup_arch()):

	num_res = 3;
	if (crashk_res_ptr) {
		res_kernel[num_res] = crashk_res_ptr;
		num_res++;
	}
	e820_reserve_resources(res_kernel, num_res);

then (in e820_reserve_resources()):

			for (j = 0; j < nr_res_k; j++) {
				if (!res_kernel[j])
					continue;
				request_resource(res, res_kernel[j]);
			}

which for j == 3 is:

	request_resource(res, &crashk_res);

Now it would appear that the new:

	insert_resource(&iomem_resource, &crashk_res);

or new:

	request_resource(res, &crashk_res);

should be noops.  But if for any reason crash_size is not zero,
then there could be a difference.  I have no idea if this is at all
significant, but I thought I'd point it out just in case.

						-Bill

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[Yinghai Lu]

On Thu, Aug 14, 2008 at 3:04 AM, Bill Fink <billfink@mindspring.com> wrote:
> Hi David,
>
> On Wed, 13 Aug 2008, David Witbrodt wrote:
>
>> [Yinghai, please note that I did not request a patch to revert the
>> problem commit.  I was merely experimenting -- on my own time, so
>> you folks would not have to bother -- to see if I could make it
>> work.  I should have made that more clear!  Having said that, I am
>> glad to test changes of any kind on my machine:  reverts, code for
>> debugging or info, experiments, etc.]
>
> I'm not sure Yinghai's revert patch is completely equivalent to
> a revert of the original problematic commit, by a side-by-side
> comparison of the original commit with his recent revert patch,
> but then I don't really know that code at all.
>
> In the original code there was a section (in e820_reserve_resources()):
>
> #ifdef CONFIG_KEXEC
>                   if (crashk_res.start != crashk_res.end)
>                           request_resource(res, &crashk_res);
> #endif
>
> If you don't have CONFIG_KEXEC defined in your .config, which is
> probably the case, then you would never request a crashk_res resource.
> But in the code after the original commit, it unconditionally calls
> (in reserve_crashkernel()):
>
>           crashk_res.start = crash_base;
>           crashk_res.end   = crash_base + crash_size - 1;
>           insert_resource(&iomem_resource, &crashk_res);
>
> And after Yinghai's revert patch it still does (in reserve_crashkernel()):
>
>        crashk_res.start = crash_base;
>        crashk_res.end   = crash_base + crash_size - 1;
>        crashk_res_ptr = &crashk_res;
>
> and (in setup_arch()):
>
>        num_res = 3;
>        if (crashk_res_ptr) {
>                res_kernel[num_res] = crashk_res_ptr;
>                num_res++;
>        }
>        e820_reserve_resources(res_kernel, num_res);
>
> then (in e820_reserve_resources()):
>
>                        for (j = 0; j < nr_res_k; j++) {
>                                if (!res_kernel[j])
>                                        continue;
>                                request_resource(res, res_kernel[j]);
>                        }
>
> which for j == 3 is:
>
>        request_resource(res, &crashk_res);
>
> Now it would appear that the new:
>
>        insert_resource(&iomem_resource, &crashk_res);
>
> or new:
>
>        request_resource(res, &crashk_res);
>
> should be noops.  But if for any reason crash_size is not zero,
> then there could be a difference.  I have no idea if this is at all
> significant, but I thought I'd point it out just in case.

why oops ?
if not valid crash kernel size etc is input, crashk_res_ptr will be null

>        if (crashk_res_ptr) {
>                res_kernel[num_res] = crashk_res_ptr;
>                num_res++;
>        }

it that is not appended to res_kernel...

YH

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[David Witbrodt]

> > I'm not sure Yinghai's revert patch is completely equivalent to
> > a revert of the original problematic commit, by a side-by-side
> > comparison of the original commit with his recent revert patch,
> > but then I don't really know that code at all.
> >
> > In the original code there was a section (in e820_reserve_resources()):
> >
> > #ifdef CONFIG_KEXEC
> >                   if (crashk_res.start != crashk_res.end)
> >                           request_resource(res, &crashk_res);
> > #endif
> >
> > If you don't have CONFIG_KEXEC defined in your .config, which is
> > probably the case, then you would never request a crashk_res resource.
> > But in the code after the original commit, it unconditionally calls
> > (in reserve_crashkernel()):
> >
> >           crashk_res.start = crash_base;
> >           crashk_res.end   = crash_base + crash_size - 1;
> >           insert_resource(&iomem_resource, &crashk_res);
> >
> > And after Yinghai's revert patch it still does (in reserve_crashkernel()):
> >
> >        crashk_res.start = crash_base;
> >        crashk_res.end   = crash_base + crash_size - 1;
> >        crashk_res_ptr = &crashk_res;
> >
> > and (in setup_arch()):
> >
> >        num_res = 3;
> >        if (crashk_res_ptr) {
> >                res_kernel[num_res] = crashk_res_ptr;
> >                num_res++;
> >        }
> >        e820_reserve_resources(res_kernel, num_res);
> >
> > then (in e820_reserve_resources()):
> >
> >                        for (j = 0; j < nr_res_k; j++) {
> >                                if (!res_kernel[j])
> >                                        continue;
> >                                request_resource(res, res_kernel[j]);
> >                        }
> >
> > which for j == 3 is:
> >
> >        request_resource(res, &crashk_res);
> >
> > Now it would appear that the new:
> >
> >        insert_resource(&iomem_resource, &crashk_res);
> >
> > or new:
> >
> >        request_resource(res, &crashk_res);
> >
> > should be noops.  But if for any reason crash_size is not zero,
> > then there could be a difference.  I have no idea if this is at all
> > significant, but I thought I'd point it out just in case.
> 
> why oops ?

I think he meant no-op's.


> if not valid crash kernel size etc is input, crashk_res_ptr will be null
> 
> >        if (crashk_res_ptr) {
> >                res_kernel[num_res] = crashk_res_ptr;
> >                num_res++;
> >        }
> 
> it that is not appended to res_kernel...

So your patch code protects against problem that Bill is mentioning
without using "#ifdef CONFIG_KEXEC", right Yinghai?

For the record:  my configs, including the kernel I built with Yinghai's
revert patch, have CONFIG_KEXEC not set.


Some experiments I did last night may render these questions moot, though.
My problem is very specific to the hardware on two of my machines, and it
has something to do with setting up the system resources that 
insert_resource() touches.


Dave W.

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[Yinghai Lu]

On Thu, Aug 14, 2008 at 5:03 AM, David Witbrodt <dawitbro@sbcglobal.net> wrote:
>
>
>> > I'm not sure Yinghai's revert patch is completely equivalent to
>> > a revert of the original problematic commit, by a side-by-side
>> > comparison of the original commit with his recent revert patch,
>> > but then I don't really know that code at all.
>> >
>> > In the original code there was a section (in e820_reserve_resources()):
>> >
>> > #ifdef CONFIG_KEXEC
>> >                   if (crashk_res.start != crashk_res.end)
>> >                           request_resource(res, &crashk_res);
>> > #endif
>> >
>> > If you don't have CONFIG_KEXEC defined in your .config, which is
>> > probably the case, then you would never request a crashk_res resource.
>> > But in the code after the original commit, it unconditionally calls
>> > (in reserve_crashkernel()):
>> >
>> >           crashk_res.start = crash_base;
>> >           crashk_res.end   = crash_base + crash_size - 1;
>> >           insert_resource(&iomem_resource, &crashk_res);
>> >
>> > And after Yinghai's revert patch it still does (in reserve_crashkernel()):
>> >
>> >        crashk_res.start = crash_base;
>> >        crashk_res.end   = crash_base + crash_size - 1;
>> >        crashk_res_ptr = &crashk_res;
>> >
>> > and (in setup_arch()):
>> >
>> >        num_res = 3;
>> >        if (crashk_res_ptr) {
>> >                res_kernel[num_res] = crashk_res_ptr;
>> >                num_res++;
>> >        }
>> >        e820_reserve_resources(res_kernel, num_res);
>> >
>> > then (in e820_reserve_resources()):
>> >
>> >                        for (j = 0; j < nr_res_k; j++) {
>> >                                if (!res_kernel[j])
>> >                                        continue;
>> >                                request_resource(res, res_kernel[j]);
>> >                        }
>> >
>> > which for j == 3 is:
>> >
>> >        request_resource(res, &crashk_res);
>> >
>> > Now it would appear that the new:
>> >
>> >        insert_resource(&iomem_resource, &crashk_res);
>> >
>> > or new:
>> >
>> >        request_resource(res, &crashk_res);
>> >
>> > should be noops.  But if for any reason crash_size is not zero,
>> > then there could be a difference.  I have no idea if this is at all
>> > significant, but I thought I'd point it out just in case.
>>
>> why oops ?
>
> I think he meant no-op's.
>
>
>> if not valid crash kernel size etc is input, crashk_res_ptr will be null
>>
>> >        if (crashk_res_ptr) {
>> >                res_kernel[num_res] = crashk_res_ptr;
>> >                num_res++;
>> >        }
>>
>> it that is not appended to res_kernel...
>
> So your patch code protects against problem that Bill is mentioning
> without using "#ifdef CONFIG_KEXEC", right Yinghai?

can you try enable kexec and kdump in you .config.

it should works. my .config have config_kexec

> Some experiments I did last night may render these questions moot, though.
> My problem is very specific to the hardware on two of my machines, and it
> has something to do with setting up the system resources that
> insert_resource() touches.

please try to bisect on current tree. and every time apply the revert patch...

YH

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[David Witbrodt]

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

First, I apologize.  I see that you were assuming I had KEXEC enabled when you
gave me the patch against 2.6.27-rc3.  I have only posted parts of my custom
.config to LKML, thinking that posting the whole thing was not appropriate.

Did you mean CRASH_DUMP instead of "kdump" above?

[N] kexec system call    (CONFIG_KEXEC)  (bool)  (Help)
[N] kernel crash dumps (EXPERIMENTAL)    (CONFIG_CRASH_DUMP)  (bool)  (Help)

My custom .config has both of these disabled -- either because they default
to "N" when running 'make oldconfig', or because I read the help text with
those options and decided I would not be using those features.


> > Some experiments I did last night may render these questions moot, though.
> > My problem is very specific to the hardware on two of my machines, and it
> > has something to do with setting up the system resources that
> > insert_resource() touches.
> 
> please try to bisect on current tree. and every time apply the revert patch...

I will try your patch against 2.6.27-rc3 again, with KEXEC and CRASH_DUMP 
enabled, when I get home from work.  I still have not retried using 'patch'
instead of 'git apply', though I am sure that 'git apply' worked just fine.

I was trying other experiments last night, and have posted the results of
those already -- and I'm guessing that your 2.6.27-rc3 patch will not work
unless it factors in what I discovered there.  (But I will do all of these
changes that you asked, as well.)

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.)

Does my other post, about trying to revert those February commits, mean
anything?


Thanks,
Dave W.

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[Yinghai Lu]

> 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.

YH

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[David Witbrodt]

> > 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.

Let's say I start with this:

1.  'git checkout v2.6.27-rc3'
2.  [apply patch]
3.  build kernel + reboot

If the kernel locks up, you want me to:

4.  [un-apply the patch]
5.  'git bisect start'
6.  'git bisect bad'

Of course, we both hope that the kernel will NOT lock up,
but let's say it does.  My confusion is the what step to take
next.  To use 'git bisect' I have to select the next kernel
version manually until I find a "good" and "bad" version...
then git can automatically do its bisecting process.

I assume you would want me to choose a previous kernel
version, like "v2.6.27-rc2".  If I keep getting "bad"
kernels, do you want me to just keep using earlier
versions... until the patch will no longer apply?

I must have misunderstood the git man pages:  I thought
that the purpose of bisecting was to find a commit that
caused a problem, which presumes that you already know
a version of the kernel which works and a version which
does not work.  Have I misunderstood?


Dave W.

PS:  My apologies if I seem obtuse.  Before this LKML
thread, I had only used git once -- and only to do
a checkout... no higher functionality like bisecting,
branching, merging, etc.  I am learning quickly, but
frankly I am baffled about how to do what you are 
asking!

Re: HPET regression in 2.6.26 versus 2.6.25 -- revert for 2.6.26-rc1 failed

[Bill Fink]

On Thu, 14 Aug 2008, Yinghai Lu wrote:

> On Thu, Aug 14, 2008 at 3:04 AM, Bill Fink <billfink@mindspring.com> wrote:
> > Hi David,
> >
> > On Wed, 13 Aug 2008, David Witbrodt wrote:
> >
> >> [Yinghai, please note that I did not request a patch to revert the
> >> problem commit.  I was merely experimenting -- on my own time, so
> >> you folks would not have to bother -- to see if I could make it
> >> work.  I should have made that more clear!  Having said that, I am
> >> glad to test changes of any kind on my machine:  reverts, code for
> >> debugging or info, experiments, etc.]
> >
> > I'm not sure Yinghai's revert patch is completely equivalent to
> > a revert of the original problematic commit, by a side-by-side
> > comparison of the original commit with his recent revert patch,
> > but then I don't really know that code at all.
> >
> > In the original code there was a section (in e820_reserve_resources()):
> >
> > #ifdef CONFIG_KEXEC
> >                   if (crashk_res.start != crashk_res.end)
> >                           request_resource(res, &crashk_res);
> > #endif
> >
> > If you don't have CONFIG_KEXEC defined in your .config, which is
> > probably the case, then you would never request a crashk_res resource.
> > But in the code after the original commit, it unconditionally calls
> > (in reserve_crashkernel()):
> >
> >           crashk_res.start = crash_base;
> >           crashk_res.end   = crash_base + crash_size - 1;
> >           insert_resource(&iomem_resource, &crashk_res);
> >
> > And after Yinghai's revert patch it still does (in reserve_crashkernel()):
> >
> >        crashk_res.start = crash_base;
> >        crashk_res.end   = crash_base + crash_size - 1;
> >        crashk_res_ptr = &crashk_res;
> >
> > and (in setup_arch()):
> >
> >        num_res = 3;
> >        if (crashk_res_ptr) {
> >                res_kernel[num_res] = crashk_res_ptr;
> >                num_res++;
> >        }
> >        e820_reserve_resources(res_kernel, num_res);
> >
> > then (in e820_reserve_resources()):
> >
> >                        for (j = 0; j < nr_res_k; j++) {
> >                                if (!res_kernel[j])
> >                                        continue;
> >                                request_resource(res, res_kernel[j]);
> >                        }
> >
> > which for j == 3 is:
> >
> >        request_resource(res, &crashk_res);
> >
> > Now it would appear that the new:
> >
> >        insert_resource(&iomem_resource, &crashk_res);
> >
> > or new:
> >
> >        request_resource(res, &crashk_res);
> >
> > should be noops.  But if for any reason crash_size is not zero,
> > then there could be a difference.  I have no idea if this is at all
> > significant, but I thought I'd point it out just in case.
> 
> why oops ?
> if not valid crash kernel size etc is input, crashk_res_ptr will be null
> 
> >        if (crashk_res_ptr) {
> >                res_kernel[num_res] = crashk_res_ptr;
> >                num_res++;
> >        }
> 
> it that is not appended to res_kernel...

You're right.  Looking just at the diffs, I didn't realize that all
of reserve_crashkernel() is inside "#ifdef CONFIG_KEXEC" and thus
crashk_res_ptr is probably null in David's kernel.  Unless of course,
in the unlikey event that the memory location for crashk_res_ptr was
being corrupted somehow.

						-Bill