Re: [PATCH v6 3/5] vmcore: Introduce remap_oldmem_pfn_range()

[HATAYAMA Daisuke]

(2013/07/10 17:42), Michael Holzheu wrote:
> Hello Hatayama,
>
> On Tue, 09 Jul 2013 14:49:48 +0900
> HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxxxxx> wrote:
>
> > (2013/07/08 23:28), Vivek Goyal wrote:
> > > On Mon, Jul 08, 2013 at 11:28:39AM +0200, Michael Holzheu wrote:
> > > > On Mon, 08 Jul 2013 14:32:09 +0900
> > > > HATAYAMA Daisuke <d.hatayama@xxxxxxxxxxxxxx> wrote:
>
> [snip]
>
> > > I personally perfer not to special case it for s390 only and let the
> > > handler be generic.
> > >
> > > If there is a bug in remap_old_pfn_range(), only side affect is that
> > > we will fault in the page when it is accessed and that will be slow. BUG()
> > > sounds excessive. At max it could be WARN_ONCE().
> > >
> > > In regular cases for x86, this path should not even hit. So special casing
> > > it to detect issues with remap_old_pfn_range() does not sound very good
> > > to me. I would rather leave it as it is and if there are bugs and mmap()
> > > slows down, then somebody needs to debug it.
> >
> > I agree to WARN_ONCE(). Then, we can notice bug at least if it occurs.
> >
> > Interface is like this?
> >
> > [generic]
> >
> > bool __weak in_valid_fault_range(pgoff_t pgoff)
> > {
> >       return false;
> > }
> >
> > [s390]
> >
> > bool in_valid_fault_range(pgoff_t pgoff)
> > {
> >       loff_t offset = pgoff << PAGE_CACHE_SHIFT;
> >       u64 paddr = vmcore_offset_to_paddr(offset);
> >
> >       return paddr < ZFCPDUMP_HSA_SIZE;
> > }
> >
> > assuming vmcore_offset_to_paddr() that looks up vmcore_list and returns physical
> > address corresponding to given offset of vmcore. I guess this could return error
> > value if there's no entry corresponding to given offset in vmcore_list.
>
> I think this is too much code (and overhead) just for checking the correctness the
> kdump mmap implementation.
>
> My suggestion is to add the WARN_ONCE() for #ifndef CONFIG_S390. This has the same
> effect as your suggestion for all architectures besides of s390. And for s390 we
> take the risk that a programming error would result in poor /proc/vmcore
> performance.

If you want to avoid looking up vmcore_list that takes linear time w.r.t. the number
of the elements, you can still calculate the range of offsets in /proc/vmcore
corresponding to HSA during /proc/vmcore initialization.

Also, could you tell me how often and how much the HSA region is during crash dumping?
I guess the read to HSA is done mainly during early part of crash dumping process only.
According to the code, it appears at most 64MiB only. Then, I feel performance is not
a big issue.

Also, cost of WARN_ONCE() is one memory access only in the 2nd and later calls. I don't
think it too much overhead...

> So, at least for this patch series I would implement the fault handler as follows:
>
> static int mmap_vmcore_fault(struct vm_area_struct *vma, struct vm_fault *vmf)
> {
> ...
>          char *buf;
>          int rc;
>
> #ifndef CONFIG_S390
>          WARN_ONCE(1, "vmcore: Unexpected call of mmap_vmcore_fault()");
> #endif
>          page = find_or_create_page(mapping, index, GFP_KERNEL);
>
> At this point I have to tell you that we plan another vmcore patch series where
> the fault handler might be called also for other architectures. But I think we
> should *then* discuss your issue again.

Could you explain the plan in more detail? Or I cannot review correctly since I don't
know whether there's really usecase of this generic fault handler for other
architectures. This is the issue for architectures other than s390, not mine; now we
don't need it at all.

--
Thanks.
HATAYAMA, Daisuke

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/