[PATCH] makedumpfile: Use file offset in initialize_mmap()

[PATCH] makedumpfile: Use file offset in initialize_mmap()

[Petr Tesarik]

Hi all,

update_mmap_range() expects a file offset as its first argument, but
initialize_mmap() passes a physical address. Since the first segment
usually starts at physical addr 0 on S/390, but there is no segment 
at file offset 0, update_mmap_range() fails, and makedumpfile falls
back to read().

@Michael: I wonder how you actually tested the kernel mmap patches;
this bug has prevented mmap on all my s390 systems...

Signed-off-by: Petr Tesarik <ptesarik@suse.cz>

---
 makedumpfile.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/makedumpfile.c
+++ b/makedumpfile.c
@@ -347,7 +347,7 @@ initialize_mmap(void) {
 	info->mmap_buf = MAP_FAILED;
 
 	get_pt_load(0, &phys_start, NULL, NULL, NULL);
-	if (!update_mmap_range(phys_start, 1))
+	if (!update_mmap_range(paddr_to_offset(phys_start), 1))
 		return FALSE;
 
 	return TRUE;

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

RE: [PATCH] makedumpfile: Use file offset in initialize_mmap()

[Atsushi Kumagai]

Hello Petr,

>Hi all,
>
>update_mmap_range() expects a file offset as its first argument, but
>initialize_mmap() passes a physical address. Since the first segment
>usually starts at physical addr 0 on S/390, but there is no segment
>at file offset 0, update_mmap_range() fails, and makedumpfile falls
>back to read().

I'll merge it into v1.5.8, thanks.


Thanks
Atsushi Kumagai

>@Michael: I wonder how you actually tested the kernel mmap patches;
>this bug has prevented mmap on all my s390 systems...
>
>Signed-off-by: Petr Tesarik <ptesarik@suse.cz>
>
>---
> makedumpfile.c |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>--- a/makedumpfile.c
>+++ b/makedumpfile.c
>@@ -347,7 +347,7 @@ initialize_mmap(void) {
> 	info->mmap_buf = MAP_FAILED;
>
> 	get_pt_load(0, &phys_start, NULL, NULL, NULL);
>-	if (!update_mmap_range(phys_start, 1))
>+	if (!update_mmap_range(paddr_to_offset(phys_start), 1))
> 		return FALSE;
>
> 	return TRUE;

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

Re: [PATCH] makedumpfile: Use file offset in initialize_mmap()

[Michael Holzheu]

Hello Petr,

Thanks for the fix!

Hard to believe that makedumpfile mmap mode on s390 has never worked.

On Fri, 27 Feb 2015 13:14:09 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> Hi all,
> 
> update_mmap_range() expects a file offset as its first argument, but
> initialize_mmap() passes a physical address. Since the first segment
> usually starts at physical addr 0 on S/390, but there is no segment 
> at file offset 0, update_mmap_range() fails, and makedumpfile falls
> back to read().

And for other architectures the wrong parameter was no problem?

> 
> @Michael: I wonder how you actually tested the kernel mmap patches;
> this bug has prevented mmap on all my s390 systems...

We tested /proc/vmcore mmap with our SCSI stand-alone dump (zfcpdump) and
with small test programs that used mmap.

I did a quick test with your patch and it looks like the mmap mode
on my s390 system is slower than the read mode:

# time ./makedumpfile -d 31 /proc/vmcore out
real    0m1.043s
user    0m0.187s
sys     0m0.433s

# time ./makedumpfile -d 31 /proc/vmcore out --non-mmap
real    0m0.767s
user    0m0.098s
sys     0m0.364s

We will have to look into this.

Michael


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

Re: [PATCH] makedumpfile: Use file offset in initialize_mmap()

[Petr Tesarik]

On Tue, 3 Mar 2015 10:15:43 +0100
Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:

> Hello Petr,
> 
> Thanks for the fix!
> 
> Hard to believe that makedumpfile mmap mode on s390 has never worked.
> 
> On Fri, 27 Feb 2015 13:14:09 +0100
> Petr Tesarik <ptesarik@suse.cz> wrote:
> 
> > Hi all,
> > 
> > update_mmap_range() expects a file offset as its first argument, but
> > initialize_mmap() passes a physical address. Since the first segment
> > usually starts at physical addr 0 on S/390, but there is no segment 
> > at file offset 0, update_mmap_range() fails, and makedumpfile falls
> > back to read().
> 
> And for other architectures the wrong parameter was no problem?

I noticed it while testing mmap on s390x. It is not a problem on
x86_64, because the first LOAD segment is the kernel text mapping, and
due to certain legacy addressing peculiarities on x86 hardware, the
kernel is never loaded at physical offset 0. In fact, it is always
loaded high enough that there is a LOAD segment at the corresponding
file offset. It's not the "correct" one, but initialize_mmap() does not
care. It only checks if it can be mmapped.

Theoretically, you may hit the bug on x86_64 if enough data goes before
the first LOAD segment. However, only program headers and ELF notes do,
so on a typical system (kernel at 16M) you would need an extremely
fragmented memory map (approx. 300k segments; not even possible with
ELF) and/or a lot of CPUs (50k or so).

I haven't checked any other architectures.

> > 
> > @Michael: I wonder how you actually tested the kernel mmap patches;
> > this bug has prevented mmap on all my s390 systems...
> 
> We tested /proc/vmcore mmap with our SCSI stand-alone dump (zfcpdump) and
> with small test programs that used mmap.
> 
> I did a quick test with your patch and it looks like the mmap mode
> on my s390 system is slower than the read mode:

That's sad. OTOH I had similar results on a file mmap some time ago.
The cost of copying data was less than the cost of handling a series of
minor page faults. I wonder if adding MAP_POPULATE to the mmap flags
makes any difference for you.

Petr T

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

Re: [PATCH] makedumpfile: Use file offset in initialize_mmap()

[Michael Holzheu]

On Tue, 3 Mar 2015 11:07:50 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> On Tue, 3 Mar 2015 10:15:43 +0100
> Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:

[snip]

> > I did a quick test with your patch and it looks like the mmap mode
> > on my s390 system is slower than the read mode:
> 
> That's sad. OTOH I had similar results on a file mmap some time ago.
> The cost of copying data was less than the cost of handling a series of
> minor page faults.

I think we understood the problem: As for the read path, also for mmap
the memory is copied into a temporary buffer:

 static int read_with_mmap(off_t offset, void *bufptr, ...)
 {

 ...
        memcpy(bufptr, info->mmap_buf +
               (offset - info->mmap_start_offset), read_size);


Because on s390 copy_to_user() is as fast as userspace memcpy() we
don't have any benefit here. The only saving is due to less
mmap()/munmap() than read() system calls because bigger chunks
are mapped than read.

If you specify -d 31 the dump memory is fragmented and we have to
issue more mmap()/munmap() calls and therefore also the system
call overhead increases.

If we really want to speed up the mmap path on s390 we probably
have to get rid of the temporary buffer.

What do you think?
Michael


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

Re: [PATCH] makedumpfile: Use file offset in initialize_mmap()

[Petr Tesarik]

On Wed, 4 Mar 2015 13:44:18 +0100
Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:

> On Tue, 3 Mar 2015 11:07:50 +0100
> Petr Tesarik <ptesarik@suse.cz> wrote:
> 
> > On Tue, 3 Mar 2015 10:15:43 +0100
> > Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> 
> [snip]
> 
> > > I did a quick test with your patch and it looks like the mmap mode
> > > on my s390 system is slower than the read mode:
> > 
> > That's sad. OTOH I had similar results on a file mmap some time ago.
> > The cost of copying data was less than the cost of handling a series of
> > minor page faults.
> 
> I think we understood the problem: As for the read path, also for mmap
> the memory is copied into a temporary buffer:
> 
>  static int read_with_mmap(off_t offset, void *bufptr, ...)
>  {
> 
>  ...
>         memcpy(bufptr, info->mmap_buf +
>                (offset - info->mmap_start_offset), read_size);
> 
> 
> Because on s390 copy_to_user() is as fast as userspace memcpy() we
> don't have any benefit here. The only saving is due to less
> mmap()/munmap() than read() system calls because bigger chunks
> are mapped than read.
> 
> If you specify -d 31 the dump memory is fragmented and we have to
> issue more mmap()/munmap() calls and therefore also the system
> call overhead increases.
> 
> If we really want to speed up the mmap path on s390 we probably
> have to get rid of the temporary buffer.
> 
> What do you think?

I'm not sure. Clearly, we should get rid of the temporary buffer. OTOH
this slow-down should be observed on all architectures, not just s390.

Now, mmap should have been implemented in the cache code, not above it.
Since I wrote the cache, this task is probably up to me.

Stay tuned,
Petr T

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

Re: [PATCH] makedumpfile: Use file offset in initialize_mmap()

[Michael Holzheu]

On Thu, 5 Mar 2015 22:30:05 +0100
Petr Tesarik <ptesarik@suse.cz> wrote:

> On Wed, 4 Mar 2015 13:44:18 +0100
> Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> 

[snip]

> > What do you think?
> 
> I'm not sure. Clearly, we should get rid of the temporary buffer. OTOH
> this slow-down should be observed on all architectures, not just s390.
> 
> Now, mmap should have been implemented in the cache code, not above it.
> Since I wrote the cache, this task is probably up to me.

Sounds good, thanks!
Michael


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