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

[HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com>]

(2013/07/10 17:42), Michael Holzheu wrote:
> Hello Hatayama,
>
> On Tue, 09 Jul 2013 14:49:48 +0900
> HATAYAMA Daisuke <d.hatayama@jp.fujitsu.com> 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@jp.fujitsu.com> 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