[Crash-utility] Re: [RFC]Patch to accomodate cpu_pda changesin x86_64 kernels

[Dave Anderson]

Badari Pulavarty wrote:

> On Tue, 2006-01-31 at 21:38 +0530, Rachita Kothiyal wrote:
> > On Tue, Jan 31, 2006 at 09:11:36AM -0500, Dave Anderson wrote:
> > >
> > > Thanks for handling this.  I haven't looked at this, and correct me
> > > if I'm wrong, but the kernel (what version exactly?) now has contains
> > > a _cpu_pda[NR_CPUS] array containing pointers to the actual per-cpu
> > > x8664_pda structures.
> >
> > Hi Dave,
> >
> > This change was introduced 2.6.16-rc1 onwards.
> > >
> > > Unless I'm missing something, that simplifies things and should work
> > > just fine. But I can't test it here other than to verify that it's
> > > backwards compatible.  Can you verify that?
> >
> > Yes, this is infact more simpler and cleaner. I have incorporated the
> > changes and regenerated the patch, which I am sending along. I have
> > also tested it on a 2.6.16-rc1 kernel, and it works fine.
> >
> > Thanks
> > Rachita
>
> Hmm.. Lots of duplicated code. I hacked the same thing earlier to make
> it work. I incorporated my changes into yours.
>
> Does this look better ?
>
> (BTW, is there a way to combine CPU_PDA_READ() and _CPU_PDA_READ() by
> abstracting some of it out ?)
>
> Thanks,
> Badari
>

Rachita, Badari,

I think I agree about the unnecessary duplication of code in
x86_64_cpu_pda_init() and x86_64_get_smp_cpus().

And while abstracting some of it out to CPU_PDA_READ() may be
desirable, keeping them separate may make future maintenance
and/or understandability of the differences may make it worth
keeping them separate.  But I don't have a preference either way.

However, upon further review, I don't see how this piece of the
patch can work for  x86_64_display_cpu_data():

@@ -2828,7 +2855,10 @@ x86_64_display_cpu_data(void)
                boot_cpu = TRUE;
                cpus = 1;
        }
q
-       cpu_pda = symbol_value("cpu_pda");
+       if (symbol_exists("_cpu_data"))
+               cpu_pda = symbol_value("_cpu_pda");
+       else if (symbol_exists("cpu_data"))
+               cpu_pda = symbol_value("cpu_pda");

         for (cpu = 0; cpu < cpus; cpu++) {
                if (boot_cpu)

because here is how "cpu_data" is used later on in the function:

        for (cpu = 0; cpu < cpus; cpu++) {
                if (boot_cpu)
                        fprintf(fp, "BOOT CPU:\n");
                else
                        fprintf(fp, "%sCPU %d:\n", cpu ? "\n" : "", cpu);

                dump_struct("cpuinfo_x86", cpu_data, 0);
                fprintf(fp, "\n");
                dump_struct("x8664_pda", cpu_pda, 0);

                cpu_data += SIZE(cpuinfo_x86);
                cpu_pda += SIZE(x8664_pda);
        }

so not only will it display bogus data when sent to dump_struct(),
the increment at the bottom of the loop would be wrong for the
_cpu_data array.  If you just run "mach -c", the x8664_pda display
is bogus, right?

It would just need get_symbol_data() to be run on the contents of
each entry in the _cpu_data[] array, and that value should be passed
to dump_struct(); and the increment at the bottom would just be the
size of a pointer.

Thanks,
  Dave


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/crash-utility/attachments/20060131/8245f2b6/attachment.htm>