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

[Simon Horman <simon@horms.net>]

On Tue, Feb 26, 2013 at 09:58:14AM +0900, HATAYAMA Daisuke wrote:
> From: Zhang Yanfei <zhangyanfei@cn.fujitsu.com>
> 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.

For the record, I am not taking this change unless some
consensus is reached.

_______________________________________________
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec