On Thu, Dec 14, 2006 at 03:08:29PM +0530, Sachin P. Sant wrote:
> Horms wrote:
> >Hi Sachin,
> >
> >I took a quick look over this. And it seems to be fine.
> >I've put some minor comments inline.
> >
> >> */
> >>-static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> >>+static struct memory_range *crash_memory_range;
> >>
> >
> >I think that crash_memory_range needs to be initialised to NULL here.
> >Otherwise the err check in get_crash_memory_ranges() may be bogus.
> >
> Hmm .. since all these variables are static globals they will be initialized
> to NULL. But i think for clarity sake we should
> explicitly initialize them to NULL. Have changed it.
>
> >>+ fprintf(stderr, "Memory allocation for crash memory range
> >>failed\n");
> >>Can you make this <= 80col wide? I think it is just a little over.
> >>
> Done.
>
> >I think that memory_range, base_memory_range and exclude_range all
> >need to be initialised to NULL here. Otherwise the checks
> >in cleanup_memory_ranges() may be bogus.
Are globals gauranteed to be NULL? I know that in kernel land they are.
But I'm not sure about user-space.
> Hmm ok done.
>
> >>+
> >>+static void cleanup_memory_ranges()
> >>+{
> >>+ if (memory_range)
> >>+ free(memory_range);
> >>+ if (base_memory_range)
> >>+ free(base_memory_range);
> >>+ if (exclude_range)
> >>+ free(exclude_range);
> >>+ if (usablemem_rgns.ranges)
> >>+ free(usablemem_rgns.ranges);
> >>+}
> >>
> >
> >Should this function set any freed pointers to NULL ?
> >
> I guess it should not be necessary to set them to NULL. This is an exit
> path and the program will end.
Ok, thanks for clarifying that.
> >>+
> >>+/*
> >>+ * Allocate memory for various data structures used to hold
> >>+ * values of different memory ranges
> >>+ */
> >>+static int alloc_memory_ranges()
> >>+{
> >>+ memory_range = (struct memory_range *) malloc(
> >>+ (sizeof(struct memory_range) * max_memory_ranges));
> >>+ base_memory_range = (struct memory_range *) malloc(
> >>+ (sizeof(struct memory_range) * max_memory_ranges));
> >>+ exclude_range = (struct exclude_range *) malloc(
> >>+ (sizeof(struct exclude_range) * max_memory_ranges));
> >>+ usablemem_rgns.ranges = (struct exclude_range *) malloc(
> >>+ (sizeof(struct exclude_range) * max_memory_ranges));
> >>+ if ((memory_range == NULL) ||
> >>+ (base_memory_range == NULL) ||
> >>+ (exclude_range == NULL) ||
> >>+ (usablemem_rgns.ranges == NULL)) {
> >>
> >
> >I personally prefer the following
> > if (!memory_range || !base_memory_range || !exclude_range ||
> > !usablemem_rgns.ranges)
> >
> >Actually, I really prefer doing the error check after each malloc(),
> >but thats just me.
> >
> Done.
>
> >
> >>+ fprintf(stderr, "Memory allocation for memory range structure
> >>failed\n");
> >>
> >
> >Can you make this <= 80col wide? I think it is just a little over.
> >
> Done
>
> >+extern mem_rgns_t usablemem_rgns;
> >
> >usablemem_rgns.ranges needs to be initialised to NULL somehow.
> >Perhaps you could just change the declaration (wherever that is)
> >to mem_rgns_t usablemem_rgns = { 0, NULL }; I think that would work.
> >
> Done.
>
> Here is the modified patch. Thanks for the review.
Thanks. I have one more minor comment, sorry for missing it yesterday.
> I have tested this on few PPC64 boxes and haven't found any problem
> with it.
>
> * kexec tools for ppc64 uses static arrays [of length MAX_MEMORY_RANGES] to
> * store data related to memory regions. This used to eat up lot's of memory.
> * There were some instances where more memory regions existed, which made
> * kexec tools unusable on those machines. The following patch gets rid of
> * MAX_MEMORY_RANGES macro and uses dynamic memory allocation for the
> * above mentioned structures.
>
> Signed-off-by : Sachin Sant <[EMAIL PROTECTED]>
> ---
>
>
> * kexec tools for ppc64 uses static arrays [of length MAX_MEMORY_RANGES] to
> * store data related to memory regions. This used to eat up lot's of memory.
> * There were some instances where more memory regions existed, which made
> * kexec tools unusable on those machines. The following patch gets rid of
> * MAX_MEMORY_RANGES macro and uses dynamic memory allocation for the
> * above mentioned structures.
>
> Signed-off-by : Sachin Sant <[EMAIL PROTECTED]>
> ---
>
> diff -Naurp hormsgittree/kexec/arch/ppc64/crashdump-ppc64.c
> hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/crashdump-ppc64.c
> --- hormsgittree/kexec/arch/ppc64/crashdump-ppc64.c 2006-12-05
> 17:08:18.000000000 +0530
> +++ hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/crashdump-ppc64.c
> 2006-12-14 14:39:51.000000000 +0530
> @@ -62,13 +62,16 @@ extern struct arch_options_t arch_option
> /* Stores a sorted list of RAM memory ranges for which to create elf headers.
> * A separate program header is created for backup region
> */
> -static struct memory_range crash_memory_range[CRASH_MAX_MEMORY_RANGES];
> +static struct memory_range *crash_memory_range = NULL;
> +
> +/* Define a variable to replace the CRASH_MAX_MEMORY_RANGES macro */
> +static int crash_max_memory_ranges;
>
> /*
> * Used to save various memory ranges/regions needed for the captured
> * kernel to boot. (lime memmap= option in other archs)
> */
> -mem_rgns_t usablemem_rgns = {0, };
> +mem_rgns_t usablemem_rgns = {0, NULL};
>
> /*
> * To store the memory size of the first kernel and this value will be
> @@ -105,6 +108,15 @@ static int get_crash_memory_ranges(struc
> int i, n;
> unsigned long long start, end, cstart, cend;
>
> + crash_max_memory_ranges = max_memory_ranges + 6;
> +
> + crash_memory_range = (struct memory_range *) malloc(
> + (sizeof(struct memory_range) * (crash_max_memory_ranges)));
> + if (!crash_memory_range) {
> + fprintf(stderr, "Allocation for crash memory range failed\n");
> + return -1;
> + }
> +
> /* create a separate program header for the backup region */
> crash_memory_range[0].start = BACKUP_SRC_START;
> crash_memory_range[0].end = BACKUP_SRC_END;
> @@ -113,7 +125,7 @@ static int get_crash_memory_ranges(struc
>
> if ((dir = opendir(device_tree)) == NULL) {
> perror(device_tree);
> - return -1;
> + goto err;
> }
> while ((dentry = readdir(dir)) != NULL) {
> if (strncmp(dentry->d_name, "memory@", 7))
> @@ -123,7 +135,7 @@ static int get_crash_memory_ranges(struc
> if ((dmem = opendir(fname)) == NULL) {
> perror(fname);
> closedir(dir);
> - return -1;
> + goto err;
> }
> while ((mentry = readdir(dmem)) != NULL) {
> if (strcmp(mentry->d_name, "reg"))
> @@ -133,21 +145,21 @@ static int get_crash_memory_ranges(struc
> perror(fname);
> closedir(dmem);
> closedir(dir);
> - return -1;
> + goto err;
> }
> if ((n = fread(buf, 1, MAXBYTES, file)) < 0) {
> perror(fname);
> fclose(file);
> closedir(dmem);
> closedir(dir);
> - return -1;
> + goto err;
> }
> - if (memory_ranges >= MAX_MEMORY_RANGES) {
> + if (memory_ranges >= max_memory_ranges) {
> /* No space to insert another element. */
> fprintf(stderr,
> "Error: Number of crash memory ranges"
> " excedeed the max limit\n");
> - return -1;
> + goto err;
> }
>
> start = ((unsigned long long *)buf)[0];
> @@ -228,6 +240,11 @@ static int get_crash_memory_ranges(struc
> }
> #endif
> return 0;
> +
> +err:
> + if (crash_memory_range)
> + free(crash_memory_range);
> + return -1;
> }
>
> /* Converts unsigned long to ascii string. */
> diff -Naurp hormsgittree/kexec/arch/ppc64/crashdump-ppc64.h
> hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/crashdump-ppc64.h
> --- hormsgittree/kexec/arch/ppc64/crashdump-ppc64.h 2006-12-05
> 17:08:18.000000000 +0530
> +++ hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/crashdump-ppc64.h
> 2006-12-13 11:26:40.000000000 +0530
> @@ -13,8 +13,6 @@ void add_usable_mem_rgns(unsigned long l
> #define __pa(x) ((unsigned long)(x)-PAGE_OFFSET)
> #define MAXMEM (-KERNELBASE-VMALLOCBASE)
>
> -#define CRASH_MAX_MEMORY_RANGES (MAX_MEMORY_RANGES + 6)
> -
> #define COMMAND_LINE_SIZE 512 /* from kernel */
> /* Backup Region, First 64K of System RAM. */
> #define BACKUP_SRC_START 0x0000
> diff -Naurp hormsgittree/kexec/arch/ppc64/fs2dt.c
> hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/fs2dt.c
> --- hormsgittree/kexec/arch/ppc64/fs2dt.c 2006-12-13 11:23:25.000000000
> +0530
> +++ hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/fs2dt.c 2006-12-13
> 17:29:26.000000000 +0530
> @@ -36,6 +36,7 @@
> #define NAMESPACE 16384 /* max bytes for property names */
> #define TREEWORDS 65536 /* max 32 bit words for property values
> */
> #define MEMRESERVE 256 /* max number of reserved memory blocks
> */
> +#define MAX_MEMORY_RANGES 1024
>
> static char pathname[MAXPATH], *pathstart;
> static char propnames[NAMESPACE] = { 0 };
> diff -Naurp hormsgittree/kexec/arch/ppc64/kexec-ppc64.c
> hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/kexec-ppc64.c
> --- hormsgittree/kexec/arch/ppc64/kexec-ppc64.c 2006-12-13
> 11:25:48.000000000 +0530
> +++ hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/kexec-ppc64.c 2006-12-14
> 14:48:45.000000000 +0530
> @@ -20,6 +20,7 @@
>
> #include <stddef.h>
> #include <stdio.h>
> +#include <stdlib.h>
> #include <errno.h>
> #include <stdint.h>
> #include <string.h>
> @@ -34,17 +35,92 @@
> #include "crashdump-ppc64.h"
> #include <arch/options.h>
>
> -static struct exclude_range exclude_range[MAX_MEMORY_RANGES];
> +static struct exclude_range *exclude_range = NULL;
> +static struct memory_range *memory_range = NULL;
> +static struct memory_range *base_memory_range = NULL;
> static unsigned long long rmo_top;
> -static struct memory_range memory_range[MAX_MEMORY_RANGES];
> -static struct memory_range base_memory_range[MAX_MEMORY_RANGES];
> unsigned long long memory_max = 0;
> static int nr_memory_ranges, nr_exclude_ranges;
> unsigned long long crash_base, crash_size;
> unsigned int rtas_base, rtas_size;
> +int max_memory_ranges;
>
> static int sort_base_ranges();
>
> +
> +static void cleanup_memory_ranges()
> +{
> + if (memory_range)
> + free(memory_range);
> + if (base_memory_range)
> + free(base_memory_range);
> + if (exclude_range)
> + free(exclude_range);
> + if (usablemem_rgns.ranges)
> + free(usablemem_rgns.ranges);
> +}
> +
> +/*
> + * Allocate memory for various data structures used to hold
> + * values of different memory ranges
> + */
> +static int alloc_memory_ranges()
> +{
> + memory_range = (struct memory_range *) malloc(
> + (sizeof(struct memory_range) * max_memory_ranges));
> + if (!memory_range)
> + goto err1;
> +
> + base_memory_range = (struct memory_range *) malloc(
> + (sizeof(struct memory_range) * max_memory_ranges));
> + if (!base_memory_range)
> + goto err1;
> +
> + exclude_range = (struct exclude_range *) malloc(
> + (sizeof(struct exclude_range) * max_memory_ranges));
> + if (!exclude_range)
> + goto err1;
> +
> + usablemem_rgns.ranges = (struct exclude_range *) malloc(
> + (sizeof(struct exclude_range) * max_memory_ranges));
> + if (!(usablemem_rgns.ranges))
> + goto err1;
> +
> + return 0;
> +
> +err1:
if (memory_range)
free(memory_range);
if (base_memory_range)
free(base_memory_range);
if (exclude_range)
free(exclude_range);
> + fprintf(stderr, "memory range structure allocation failure\n");
> + cleanup_memory_ranges();
> + return -1;
> +
Sorry to be a pain, but I think that you really need something like the
above under err1. Sorry that I didn't notice that yesterday.
> +}
> +
> +/*
> + * Count the memory@ nodes under /proc/device-tree and populate the
> + * max_memory_ranges variable. This variable replaces MAX_MEMORY_RANGES
> + * macro used earlier.
> + */
> +static int count_memory_ranges()
> +{
> + char device_tree[256] = "/proc/device-tree/";
> + struct dirent *dentry;
> + DIR *dir;
> +
> + if ((dir = opendir(device_tree)) == NULL) {
> + perror(device_tree);
> + return -1;
> + }
> +
> + while ((dentry = readdir(dir)) != NULL) {
> + if (strncmp(dentry->d_name, "memory@", 7))
> + continue;
> + max_memory_ranges++;
> + }
> + closedir(dir);
> +
> + return 0;
> +}
> +
> /* Get base memory ranges */
> static int get_base_ranges()
> {
> @@ -88,7 +164,7 @@ static int get_base_ranges()
> closedir(dir);
> return -1;
> }
> - if (local_memory_ranges >= MAX_MEMORY_RANGES) {
> + if (local_memory_ranges >= max_memory_ranges) {
> fclose(file);
> break;
> }
> @@ -447,8 +523,10 @@ int setup_memory_ranges(unsigned long ke
> * nodes. Build list of ranges to be excluded from valid memory
> */
>
> - get_base_ranges();
> - get_devtree_details(kexec_flags);
> + if (get_base_ranges())
> + goto out;
> + if (get_devtree_details(kexec_flags))
> + goto out;
>
> for (i = 0; i < nr_exclude_ranges; i++) {
> /* If first exclude range does not start with 0, include the
> @@ -511,12 +589,21 @@ int setup_memory_ranges(unsigned long ke
> fprintf(stderr, "setup_memory_ranges memory_range[%d]
> start:%lx, end:%lx\n", k, memory_range[k].start, memory_range[k].end);
> #endif
> return 0;
> +
> +out:
> + cleanup_memory_ranges();
> + return -1;
> }
>
> /* Return a list of valid memory ranges */
> int get_memory_ranges(struct memory_range **range, int *ranges,
> unsigned long kexec_flags)
> {
> + if (count_memory_ranges())
> + return -1;
> + if (alloc_memory_ranges())
> + return -1;
> +
> setup_memory_ranges(kexec_flags);
> *range = memory_range;
> *ranges = nr_memory_ranges;
> diff -Naurp hormsgittree/kexec/arch/ppc64/kexec-ppc64.h
> hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/kexec-ppc64.h
> --- hormsgittree/kexec/arch/ppc64/kexec-ppc64.h 2006-12-13
> 11:25:48.000000000 +0530
> +++ hormsgittree-dyn-mem-alloc/kexec/arch/ppc64/kexec-ppc64.h 2006-12-13
> 11:43:00.000000000 +0530
> @@ -1,8 +1,6 @@
> #ifndef KEXEC_PPC64_H
> #define KEXEC_PPC64_H
>
> -#define MAX_MEMORY_RANGES 1024 /* TO FIX - needs to be dynamically set */
> -
> #define MAXBYTES 128
> #define MAX_LINE 160
> #define CORE_TYPE_ELF32 1
> @@ -17,6 +15,8 @@ void elf_ppc64_usage(void);
> void reserve(unsigned long long where, unsigned long long length);
>
> extern unsigned long initrd_base, initrd_size;
> +extern int max_memory_ranges;
> +
> /* boot block version 2 as defined by the linux kernel */
> struct bootblock {
> unsigned magic,
> @@ -39,7 +39,9 @@ struct exclude_range {
>
> typedef struct mem_rgns {
> unsigned int size;
> - struct exclude_range ranges[MAX_MEMORY_RANGES];
> + struct exclude_range *ranges;
> } mem_rgns_t;
>
> +extern mem_rgns_t usablemem_rgns;
> +
> #endif /* KEXEC_PPC64_H */
--
Horms
H: http://www.vergenet.net/~horms/
W: http://www.valinux.co.jp/en/
_______________________________________________
fastboot mailing list
[email protected]
https://lists.osdl.org/mailman/listinfo/fastboot
