[PATCH] kexec,x86: code optimization and adjustment for add_memmap and delete_memmap

[d.hatayama@xxxxxxxxxxxxxx (HATAYAMA Daisuke)]

From: Zhang Yanfei <zhangyanfei@xxxxxxxxxxxxxx>
Subject: [PATCH] kexec,x86: code optimization and adjustment for add_memmap and delete_memmap
Date: Mon, 25 Feb 2013 20:38:41 +0800

> The code in the two functions seems a little messy, So I modify
> some of them, trying to make the logic more clearly.
> 
> For example,
> code before:
> 
>          for (i = 0; i < CRASH_MAX_MEMMAP_NR;  i++) {
>                  mstart = memmap_p[i].start;
>                  mend = memmap_p[i].end;
>                  if (mstart == 0 && mend == 0)
>                          break;
> 
> for we already have nr_entries for this memmap_p array, so we needn't
> have a check in every loop to see if we have go through the whole array.
> code after:
> 
>          for (i = 0; i < nr_entries; i++) {
>  		mstart = memmap_p[i].start;
>  		mend = memmap_p[i].end;
> 

Then, but even if doing so, times of the loop is unchanged after your
fix. The loop doesn't always look up a whole part of memmap_p; it
looks up until it reaches the end of elements with 0 in their members.
Also, CRASH_MAX_MEMMAP_NR is 17 on i386. Is it problematic in
performance?

Rather, it seems problematic to me how length of memmap_p is handled
here. It is initialized first here:

        /* Memory regions which panic kernel can safely use to boot into */
        sz = (sizeof(struct memory_range) * (KEXEC_MAX_SEGMENTS + 1));
        memmap_p = xmalloc(sz);
        memset(memmaSp_p, 0, sz);

and then it is passed to add_memmap,

        add_memmap(memmap_p, info->backup_src_start, info->backup_src_size);

and there the passed memmap_p is assumed to have length of
CRASH_MAX_MEMMAP_NR that is defined as (KEXEC_MAX_SEGMENTS + 1)
according to the condition of the for loop above. (The
(KEXEC_MAX_SEGMENTS + 1) in the allocation should also be
CRASH_MAX_MEMMAP_NR for clarification?)

The ideas I have for cleaning up here, are for example:

- introduce specific for-each statement to iterate memmap_p just like:

  for_each_memmap(memmap_p, start, end) {
    ...
  }

  or

- use struct memory_ranges instead of struct memory_range and pass it
  to add_memmap; the former has size member in addition to ranges
  member, and then in add_memmap:

  for (i = 0; i < ranges.size; i++) {
    mstart = ranges.ranges[i].start;
    mend = ranges.ranges[i].end;
  }

The former hides actual length of memmap_p from users because they
don't need to care about it, while the latter makes length of memmap_p
explicitly handled even in add_memmp.

But sorry, this idea comes from some minuts look on the code around.

Also, you should have divided the patch into two: nr_entires part and
the others.

Thanks.
HATAYAMA, Daisuke