Re: [patch v3 2/8] kdump: Make kimage_load_crash_segment() weak

[Vivek Goyal <vgoyal@redhat.com>]

On Fri, Aug 19, 2011 at 03:27:52PM +0200, Michael Holzheu wrote:
> Hello Vivek,
> 
> On Thu, 2011-08-18 at 13:15 -0400, Vivek Goyal wrote:
> > On Fri, Aug 12, 2011 at 03:48:51PM +0200, Michael Holzheu wrote:
> > > From: Michael Holzheu <holzheu@linux.vnet.ibm.com>
> > > 
> > > On s390 we do not create page tables at all for the crashkernel memory.
> > > This requires a s390 specific version for kimage_load_crash_segment().
> > > Therefore this patch declares this function as "__weak". The s390 version is
> > > very simple. It just copies the kexec segment to real memory without using
> > > page tables:
> > > 
> > > int kimage_load_crash_segment(struct kimage *image,
> > >                               struct kexec_segment *segment)
> > > {
> > >         return copy_from_user_real((void *) segment->mem, segment->buf,
> > >                                    segment->bufsz);
> > > }
> > > 
> > > There are two main advantages of not creating page tables for the
> > > crashkernel memory:
> > > 
> > > a) It saves memory. We have scenarios in mind, where crashkernel
> > >    memory can be very large and saving page table space is important.
> > > b) We protect the crashkernel memory from being overwritten.
> > 
> > Michael,
> > 
> > Thinking more about it. Can't we provide a arch specific version of
> > kmap() and kunmap() so that we create temporary mappings to copy
> > the pages and then these are torn off.
> 
> Isn't kmap/kunmap() used for higmem? These functions are called from
> many different functions in the Linux kernel, not only for kdump. I
> would assume that creating and removing mappings with these functions is
> not what a caller would expect and probably would break the Linux kernel
> at many other places, no?

[CCing linux-mm]

Yes it is being used for highmem pages. If arch has not defined kmap()
then generic definition is just returning page_address(page), expecting
that page will be mapped.

I was wondering that what will be broken if arch decides to extend this
to create temporary mappings for pages which are not HIGHMEM but do
not have any mapping. (Like this special case of s390).

I guess memory management guys can give a better answer here. As a layman,
kmap() seems to be the way to get a kernel mapping for any page frame
and if one is not already there, then arch might create one on the fly,
like we do for HIGHMEM pages. So the question is can be extend this
to also cover pages which are not highmem but do not have any mappings
on s390.

> 
> Perhaps we can finish this discussion after my vacation. I will change
> my patch series that we even do not need this patch...

So how are you planning to get rid of this patch without modifying kmap(),
kunmap() implementation for s390?

> 
> So only two common code patches are remaining. I will send the common
> code patches again and will ask Andrew Morton to integrate them in the
> next merge window.The s390 patches will be integrated by Martin.

I am fine with merge of other 2 common patches. Once you repost the
series, I will ack those.

Thanks
Vivek

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>