Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Manish Ahuja writes: > If Mike and Paul are okay, then I will leave this bit as is and fix all > other issues and comments. Well, part of the problem is the semantic dissonance caused by having a static variable called "global". Please change the name "phyp_dump_global" to "phyp_dump_vars" or something similar - that will only affect two lines of code and will reduce the ugliness a bit. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Wed, 2008-03-12 at 23:29 -0500, Manish Ahuja wrote: > If Mike and Paul are okay, then I will leave this bit as is and fix all > other issues and comments. Well I still don't like it - it uglifies the code _now_, for a potential future benefit that may never come. But I don't care that much, if Paul's happy with it let it go in. cheers > Linas Vepstas wrote: > > On 10/03/2008, Michael Ellerman <[EMAIL PROTECTED]> wrote: > >> On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote: > > > >> > + > >> > +/* Global, used to communicate data between early boot and late boot */ > >> > +static struct phyp_dump phyp_dump_global; > >> > +struct phyp_dump *phyp_dump_info = &phyp_dump_global; > >> > >> I don't see the point of this. You have a static (ie. non-global) struct > >> called phyp_dump_global, then you create a pointer to it and pass that > >> around. > > > > I did this. This is a style used to minimize disruption due to future > > design changes. Basically, the idea is that, at some later time, for > > some unknown reason, we decide that this structure shouldn't > > be global, or maybe shouldn't be statically allocated, or maybe > > should be per-cpu, or who knows. By creating a pointer, and > > just passing that around, you isolate other code from this change. > > > > I learned this trick after spending too many months of my life hunting > > down globals and replacing them by dynamically allocated structs. > > Its a long and painful process, on many levels, often requiring major > > code restructuring. Code that touches globals directly is often > > poorly thought out, designed. But going in the opposite direction > > is easy: if your code always passes everything it needs as args > > to subroutines, then you are free & clear ... if one of those args > > just happens to be a pointer to a global, there's no loss (not even > > a performance loss -- the arg passing overhead is about the same > > as a global TOC lookup!) > > > > So it may look weird if you're not used to seeing it; but the alternative > > is almost always worse. -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
If Mike and Paul are okay, then I will leave this bit as is and fix all other issues and comments. Thanks, Manish Linas Vepstas wrote: > On 10/03/2008, Michael Ellerman <[EMAIL PROTECTED]> wrote: >> On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote: > >> > + >> > +/* Global, used to communicate data between early boot and late boot */ >> > +static struct phyp_dump phyp_dump_global; >> > +struct phyp_dump *phyp_dump_info = &phyp_dump_global; >> >> I don't see the point of this. You have a static (ie. non-global) struct >> called phyp_dump_global, then you create a pointer to it and pass that >> around. > > I did this. This is a style used to minimize disruption due to future > design changes. Basically, the idea is that, at some later time, for > some unknown reason, we decide that this structure shouldn't > be global, or maybe shouldn't be statically allocated, or maybe > should be per-cpu, or who knows. By creating a pointer, and > just passing that around, you isolate other code from this change. > > I learned this trick after spending too many months of my life hunting > down globals and replacing them by dynamically allocated structs. > Its a long and painful process, on many levels, often requiring major > code restructuring. Code that touches globals directly is often > poorly thought out, designed. But going in the opposite direction > is easy: if your code always passes everything it needs as args > to subroutines, then you are free & clear ... if one of those args > just happens to be a pointer to a global, there's no loss (not even > a performance loss -- the arg passing overhead is about the same > as a global TOC lookup!) > > So it may look weird if you're not used to seeing it; but the alternative > is almost always worse. > > --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On 10/03/2008, Michael Ellerman <[EMAIL PROTECTED]> wrote: > On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote: > > + > > +/* Global, used to communicate data between early boot and late boot */ > > +static struct phyp_dump phyp_dump_global; > > +struct phyp_dump *phyp_dump_info = &phyp_dump_global; > > I don't see the point of this. You have a static (ie. non-global) struct > called phyp_dump_global, then you create a pointer to it and pass that > around. I did this. This is a style used to minimize disruption due to future design changes. Basically, the idea is that, at some later time, for some unknown reason, we decide that this structure shouldn't be global, or maybe shouldn't be statically allocated, or maybe should be per-cpu, or who knows. By creating a pointer, and just passing that around, you isolate other code from this change. I learned this trick after spending too many months of my life hunting down globals and replacing them by dynamically allocated structs. Its a long and painful process, on many levels, often requiring major code restructuring. Code that touches globals directly is often poorly thought out, designed. But going in the opposite direction is easy: if your code always passes everything it needs as args to subroutines, then you are free & clear ... if one of those args just happens to be a pointer to a global, there's no loss (not even a performance loss -- the arg passing overhead is about the same as a global TOC lookup!) So it may look weird if you're not used to seeing it; but the alternative is almost always worse. --linas ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Wed, 2008-03-12 at 11:13 +1100, Michael Ellerman wrote: > On Tue, 2008-03-11 at 17:12 +1100, Paul Mackerras wrote: > > Manish Ahuja writes: > > > > > +#else /* CONFIG_PHYP_DUMP */ > > > +int early_init_dt_scan_phyp_dump(unsigned long node, > > > + const char *uname, int depth, void *data) { return 0; } > > > > This shouldn't be in the header file. Either put it in prom.c (and > > make it return 1 so the of_scan_flat_dt call doesn't have to go > > through the entire device tree), or put #ifdef CONFIG_PHYP_DUMP around > > the of_scan_flat_dt call itself. > > It should be in the header file, otherwise we need an #ifdef around the > call site - which is uglier. Right I'm an idiot. It is called via a function pointer, so a static inline (which this should be, but isn't) is no good. An #ifdef around the call site is probably the least ugly option given that otherwise we have to have an empty version in the binary. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Tue, 2008-03-11 at 17:12 +1100, Paul Mackerras wrote: > Manish Ahuja writes: > > > +#else /* CONFIG_PHYP_DUMP */ > > +int early_init_dt_scan_phyp_dump(unsigned long node, > > + const char *uname, int depth, void *data) { return 0; } > > This shouldn't be in the header file. Either put it in prom.c (and > make it return 1 so the of_scan_flat_dt call doesn't have to go > through the entire device tree), or put #ifdef CONFIG_PHYP_DUMP around > the of_scan_flat_dt call itself. It should be in the header file, otherwise we need an #ifdef around the call site - which is uglier. It should definitely return 1 though. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Manish Ahuja writes: > +#else /* CONFIG_PHYP_DUMP */ > +int early_init_dt_scan_phyp_dump(unsigned long node, > + const char *uname, int depth, void *data) { return 0; } This shouldn't be in the header file. Either put it in prom.c (and make it return 1 so the of_scan_flat_dt call doesn't have to go through the entire device tree), or put #ifdef CONFIG_PHYP_DUMP around the of_scan_flat_dt call itself. > +/* Global, used to communicate data between early boot and late boot */ > +static struct phyp_dump phyp_dump_global; > +struct phyp_dump *phyp_dump_info = &phyp_dump_global; It's a little weird to have a static variable with global in its name. > +int __init early_init_dt_scan_phyp_dump(unsigned long node, > + const char *uname, int depth, void *data) > +{ > +#ifdef CONFIG_PHYP_DUMP This is in phyp_dump.c, which only gets compiled if CONFIG_PHYP_DUMP is set, so you don't need this ifdef. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Thu, 2008-02-28 at 18:24 -0600, Manish Ahuja wrote: > Initial patch for reserving memory in early boot, and freeing it later. > If the previous boot had ended with a crash, the reserved memory would contain > a copy of the crashed kernel data. > > Signed-off-by: Manish Ahuja <[EMAIL PROTECTED]> > Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]> Hi Manish, A few comments inline .. > Index: 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c > === > --- /dev/null 1970-01-01 00:00:00.0 + > +++ 2.6.25-rc1/arch/powerpc/platforms/pseries/phyp_dump.c 2008-02-28 > 21:57:52.0 -0600 > @@ -0,0 +1,105 @@ > +/* > + * Hypervisor-assisted dump > + * > + * Linas Vepstas, Manish Ahuja 2008 > + * Copyright 2008 IBM Corp. > + * > + * This program is free software; you can redistribute it and/or > + * modify it under the terms of the GNU General Public License > + * as published by the Free Software Foundation; either version > + * 2 of the License, or (at your option) any later version. > + * > + */ > + > +#include > +#include > +#include > +#include > + > +#include > +#include > +#include > +#include > + > +/* Global, used to communicate data between early boot and late boot */ > +static struct phyp_dump phyp_dump_global; > +struct phyp_dump *phyp_dump_info = &phyp_dump_global; I don't see the point of this. You have a static (ie. non-global) struct called phyp_dump_global, then you create a pointer to it and pass that around. It could just be: phyp_dump.h: extern struct phyp_dump phyp_dump_info; phyp_dump.c: struct phyp_dump phyp_dump_info; phyp_dump_info.foo = bar; I also think the struct should be called phyp_dump_info, not phyp_dump - it contains info about phyp_dump, not the dump itself. > + > +/** > + * release_memory_range -- release memory previously lmb_reserved > + * @start_pfn: starting physical frame number > + * @nr_pages: number of pages to free. > + * > + * This routine will release memory that had been previously > + * lmb_reserved in early boot. The released memory becomes > + * available for genreal use. > + */ > +static void > +release_memory_range(unsigned long start_pfn, unsigned long nr_pages) > +{ > + struct page *rpage; > + unsigned long end_pfn; > + long i; > + > + end_pfn = start_pfn + nr_pages; > + > + for (i = start_pfn; i <= end_pfn; i++) { > + rpage = pfn_to_page(i); > + if (PageReserved(rpage)) { > + ClearPageReserved(rpage); > + init_page_count(rpage); > + __free_page(rpage); > + totalram_pages++; > + } > + } > +} > + > +static int __init phyp_dump_setup(void) > +{ > + unsigned long start_pfn, nr_pages; > + > + /* If no memory was reserved in early boot, there is nothing to do */ > + if (phyp_dump_info->init_reserve_size == 0) > + return 0; > + > + /* Release memory that was reserved in early boot */ > + start_pfn = PFN_DOWN(phyp_dump_info->init_reserve_start); > + nr_pages = PFN_DOWN(phyp_dump_info->init_reserve_size); > + release_memory_range(start_pfn, nr_pages); > + > + return 0; > +} > +machine_subsys_initcall(pseries, phyp_dump_setup); > + > +int __init early_init_dt_scan_phyp_dump(unsigned long node, > + const char *uname, int depth, void *data) > +{ > +#ifdef CONFIG_PHYP_DUMP > + const unsigned int *sizes; > + > + phyp_dump_info->phyp_dump_configured = 0; > + phyp_dump_info->phyp_dump_is_active = 0; > + > + if (depth != 1 || strcmp(uname, "rtas") != 0) > + return 0; > + > + if (of_get_flat_dt_prop(node, "ibm,configure-kernel-dump", NULL)) > + phyp_dump_info->phyp_dump_configured++; > + > + if (of_get_flat_dt_prop(node, "ibm,dump-kernel", NULL)) > + phyp_dump_info->phyp_dump_is_active++; > + > + sizes = of_get_flat_dt_prop(node, "ibm,configure-kernel-dump-sizes", > + NULL); > + if (!sizes) > + return 0; > + > + if (sizes[0] == 1) > + phyp_dump_info->cpu_state_size = *((unsigned long *)&sizes[1]); > + > + if (sizes[3] == 2) > + phyp_dump_info->hpte_region_size = > + *((unsigned long *)&sizes[4]); > +#endif This doesn't need to be inside #ifdef, you have a dummy version already defined in the header file. > Index: 2.6.25-rc1/arch/powerpc/kernel/prom.c > === > --- 2.6.25-rc1.orig/arch/powerpc/kernel/prom.c2008-02-28 > 21:54:57.0 -0600 > +++ 2.6.25-rc1/arch/powerpc/kernel/prom.c 2008-02-28 21:55:27.0 > -0600 > @@ -1039,6 +1040,51 @@ static void __init early_reserve_mem(voi > #endif > } > > +#ifdef CONFIG_PHYP_DUMP > +/** > + * reserve_crashed_mem()
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Olof, I will run it through checkpatch before resubmitting. Thanks, Manish Olof Johansson wrote: > On Thu, Feb 14, 2008 at 02:46:21PM +1100, Tony Breeds wrote: > >> Hi Manish, >> Sorry for the minor nits but this should be: >> >> --- >> * Linas Vepstas, Manish Ahuja 2008 >> * Copyright 2008 IBM Corp. >> --- >> >> You can optionally use the '??' symbol after word 'Copyright' but you >> shouldn't use '(c)' anymore. >> >> Also in at least one place you've misspelt "Copyright" > > If we're going to nitpick, then I'd like to point out that the whole > series needs to be run through checkpatch and at least the whitespace > issues should be taken care of. > > I'm still not convinced that this is a useful feature compared to > hardening kdump, especially now that ehea can handle kexec/kdump (patch > posted the other day). But in the end it's up to Paul if he wants to > take it or not, not me. > > > -Olof ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Thu, Feb 14, 2008 at 02:46:21PM +1100, Tony Breeds wrote: > Hi Manish, > Sorry for the minor nits but this should be: > > --- > * Linas Vepstas, Manish Ahuja 2008 > * Copyright 2008 IBM Corp. > --- > > You can optionally use the '??' symbol after word 'Copyright' but you > shouldn't use '(c)' anymore. > > Also in at least one place you've misspelt "Copyright" If we're going to nitpick, then I'd like to point out that the whole series needs to be run through checkpatch and at least the whitespace issues should be taken care of. I'm still not convinced that this is a useful feature compared to hardening kdump, especially now that ehea can handle kexec/kdump (patch posted the other day). But in the end it's up to Paul if he wants to take it or not, not me. -Olof ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Tue, Feb 12, 2008 at 01:08:25AM -0600, Manish Ahuja wrote: > > Initial patch for reserving memory in early boot, and freeing it later. > If the previous boot had ended with a crash, the reserved memory would contain > a copy of the crashed kernel data. > > Signed-off-by: Manish Ahuja <[EMAIL PROTECTED]> > Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]> > > > arch/powerpc/kernel/prom.c | 50 > arch/powerpc/kernel/rtas.c | 32 + > arch/powerpc/platforms/pseries/Makefile|1 > arch/powerpc/platforms/pseries/phyp_dump.c | 71 > + > include/asm-powerpc/phyp_dump.h| 38 +++ > include/asm/rtas.h |3 + > 6 files changed, 195 insertions(+) > > Index: 2.6.24-rc5/include/asm-powerpc/phyp_dump.h > === > --- /dev/null 1970-01-01 00:00:00.0 + > +++ 2.6.24-rc5/include/asm-powerpc/phyp_dump.h2008-02-12 > 06:12:37.0 -0600 > @@ -0,0 +1,38 @@ > +/* > + * Hypervisor-assisted dump > + * > + * Linas Vepstas, Manish Ahuja 2007 > + * Copyright (c) 2007 IBM Corp. Hi Manish, Sorry for the minor nits but this should be: --- * Linas Vepstas, Manish Ahuja 2008 * Copyright 2008 IBM Corp. --- You can optionally use the '©' symbol after word 'Copyright' but you shouldn't use '(c)' anymore. Also in at least one place you've misspelt "Copyright" Yours Tony linux.conf.auhttp://linux.conf.au/ || http://lca2008.linux.org.au/ Jan 28 - Feb 02 2008 The Australian Linux Technical Conference! ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
On Tue, 2008-02-12 at 01:08 -0600, Manish Ahuja wrote: > Initial patch for reserving memory in early boot, and freeing it later. > If the previous boot had ended with a crash, the reserved memory would contain > a copy of the crashed kernel data. > > Signed-off-by: Manish Ahuja <[EMAIL PROTECTED]> > Signed-off-by: Linas Vepstas <[EMAIL PROTECTED]> > > > arch/powerpc/kernel/prom.c | 50 > arch/powerpc/kernel/rtas.c | 32 + > arch/powerpc/platforms/pseries/Makefile|1 > arch/powerpc/platforms/pseries/phyp_dump.c | 71 > + > include/asm-powerpc/phyp_dump.h| 38 +++ > include/asm/rtas.h |3 + ^^ asm/rtas.h doesn't exist. You need to clean your tree, and patch asm-powerpc/rtas.h instead. cheers -- Michael Ellerman OzLabs, IBM Australia Development Lab wwweb: http://michael.ellerman.id.au phone: +61 2 6212 1183 (tie line 70 21183) We do not inherit the earth from our ancestors, we borrow it from our children. - S.M.A.R.T Person signature.asc Description: This is a digitally signed message part ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Sorry, I think i sent the wrong patch file, it shouldn't have my printk statement in there. Let me re-send the correct file and let me test it once more to make sure it does the right thing. -Manish Paul Mackerras wrote: > Manish Ahuja writes: > >> Initial patch for reserving memory in early boot, and freeing it later. >> If the previous boot had ended with a crash, the reserved memory would >> contain >> a copy of the crashed kernel data. > > [snip] > >> +static void __init reserve_crashed_mem(void) >> +{ >> +unsigned long base, size; >> + >> +if (phyp_dump_info->phyp_dump_is_active) { >> +/* Reserve *everything* above RMR. We'll free this real soon.*/ >> +base = PHYP_DUMP_RMR_END; >> +size = lmb_end_of_DRAM() - base; >> + >> +/* XXX crashed_ram_end is wrong, since it may be beyond >> +* the memory_limit, it will need to be adjusted. */ >> +lmb_reserve(base, size); >> + >> +phyp_dump_info->init_reserve_start = base; >> +phyp_dump_info->init_reserve_size = size; >> +} >> +else { >> +size = phyp_dump_info->cpu_state_size + >> +phyp_dump_info->hpte_region_size + >> +PHYP_DUMP_RMR_END; >> +base = lmb_end_of_DRAM() - size; >> +printk(KERN_ERR "Manish reserve regular kernel space is %ld %ld\n", >> base, size); >> +lmb_reserve(base, size); > > This is still reserving memory even on systems that aren't running on > pHyp at all. Please rework this so that no memory is reserved if the > system doesn't support phyp-assisted dump. > > Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Manish Ahuja writes: > Initial patch for reserving memory in early boot, and freeing it later. > If the previous boot had ended with a crash, the reserved memory would contain > a copy of the crashed kernel data. [snip] > +static void __init reserve_crashed_mem(void) > +{ > + unsigned long base, size; > + > + if (phyp_dump_info->phyp_dump_is_active) { > + /* Reserve *everything* above RMR. We'll free this real soon.*/ > + base = PHYP_DUMP_RMR_END; > + size = lmb_end_of_DRAM() - base; > + > + /* XXX crashed_ram_end is wrong, since it may be beyond > + * the memory_limit, it will need to be adjusted. */ > + lmb_reserve(base, size); > + > + phyp_dump_info->init_reserve_start = base; > + phyp_dump_info->init_reserve_size = size; > + } > + else { > + size = phyp_dump_info->cpu_state_size + > + phyp_dump_info->hpte_region_size + > + PHYP_DUMP_RMR_END; > + base = lmb_end_of_DRAM() - size; > + printk(KERN_ERR "Manish reserve regular kernel space is %ld %ld\n", > base, size); > + lmb_reserve(base, size); This is still reserving memory even on systems that aren't running on pHyp at all. Please rework this so that no memory is reserved if the system doesn't support phyp-assisted dump. Paul. ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev
Re: [PATCH 2/8] pseries: phyp dump: reserve-release proof-of-concept
Reposted this one. I got the email id wrong in this one. Sorry about that. Manish ___ Linuxppc-dev mailing list Linuxppc-dev@ozlabs.org https://ozlabs.org/mailman/listinfo/linuxppc-dev