Re: [Fastboot] [PATCH] Reserving backup region for kexec based crashdumps.

[ebiederm@xmission.com (Eric W. Biederman)]

Right now I am very frustrated with reviewing any of the crashdump
patches.  When I make comments usually things change just enough that
what I said is addressed but things are addressed very much at
a surface level.  Which means that if I think any kind of substantial
change is needed the only way I seem to be able to communicate
that is by actually implementing it myself.

Code that works today is great it does manages the job of requirements
capture.   But just throwing code together when you are dealing
with fundamental interface boundaries is not a good way to build
a sustainable design.  And with the crashdump code I want an
interface that is at least as simple and as stable as the syscall
interface.

At the very least if a patch is just a snapshot of your development
process up for comment and you are going to continue on making
headway please say as much.  If I know the code is quite possibly
going to change in some pretty fundamental ways I can stop worrying
about it.  This patch is certainly nothing I would want for more
than a couple of day hack, in my personal development tree.

I will try once again...

There is evil intermingling and false dependency sharing between
the dying kernel and the crash capture kernel in this patch, and
virtually all of the code is unnecessary.  I have already addressed
why.

Vivek Goyal <vgoyal@in.ibm.com> writes:

> On Fri, 2005-01-21 at 16:43, Eric W. Biederman wrote:
> > On deeper review your patch as it stands is incomplete.  In particular
> > you don't provide a way to either hardcode or dynamically set
> > the area you are attempt to reserve to hold the backup region.
> 
> Well. Here is the new patch. This one steals the 640k from top of memory
> region reserved for crash kernel. 
> 
> A new command line parameter (crashbackup=) has been introduced for
> crash dump kernels. This parameter specifies the location of backup
> region from where to retrieve the backup data.

What is wrong with user space doing all of the extra space
reservation?

Could you send this fairly obvious kexec fix, as a separate patch? 

> diff -puN include/linux/kexec.h~crashdump-x86-reserve-640k-memory
> include/linux/kexec.h
> 
> --- linux-2.6.11-rc1/include/linux/kexec.h~crashdump-x86-reserve-640k-memory
> 2005-01-22 14:16:27.000000000 +0530
> 
> +++ linux-2.6.11-rc1-root/include/linux/kexec.h 2005-01-22 14:16:27.000000000
> +0530
> 
> @@ -79,7 +79,7 @@ struct kimage {
>  	unsigned long control_page;
>  
>  	/* Flags to indicate special processing */
> -	int type : 1;
> +	unsigned int type : 1;
>  #define KEXEC_TYPE_DEFAULT 0
>  #define KEXEC_TYPE_CRASH   1
>  };

Eric