Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path

[Xunlei Pang <xpang@redhat.com>]

On 2016/03/23 at 16:23, Baoquan He wrote:
> On 03/23/16 at 11:32am, Xunlei Pang wrote:
>> On 2016/03/23 at 10:48, Baoquan He wrote:
>>> On 03/01/16 at 05:53pm, Xunlei Pang wrote:
>>>> This is a bug fix.
>>>>
>>>> After this, I will try to do a cleanup for crash_unmap/map_reserved_pages()
>>>> (only used by S390) to consolidate it with arch_kexec_unprotect/protect_crashkres().
>>> Hi Xunlei, Minfei,
>>>
>>> I think you need discuss together about how to do clean up codes in this
>>> place. From my point of view, arch_map/unmap_reserved_pages and
>>> arch_kexec_protect/unprotect_crashkres() are for the same goal but by
>>> different ways on different arch. So for Xunlei's patchset, you might
>>> need to rethink your implementation, the name of function. I personally
>>> think you just implement a x86 specific arch_map/unmap_reserved_pages.
>>> It may need a more generic name, and then add your x86 arch specific
>>> implementation. Sorry I can't see your patches on my mail client,
>> Like what you said, I think arch_kexec_unprotect/protect_crashkres() are
>> generic enough, but any other better name is welcome :-)
>>
>> It also covered the newly-added kexec file path, and we can easily transfer
>> arch_map/unmap_reserved_pages into this new interface.
> I don't know the status of your patchset. If possible I think the 1st
> patch in your patchset shoule rename arch_map/unmap_reserved_pages to
> arch_kexec_protect/unprotect_crashkres, 2nd patch is to add your x86
> specific patch.

Yes, actually when I filed my patchset, I didn't notice arch_map/unmap_reserved_pages,
too much back then, s390 is its only user, and hard to get the purpose from its name.

But from other point of view, they are a bit different, crash_map_reserved_pages()
is also called by crash_shrink_memory(), it is a bit more complex(and needs some
s390 arch code modification) than just simply renaming/consolidating them, so I think
it's  ok to provide a new generic mechanism first and then put renaming/consolidating
arch work back a little as a separate patch.

Regards,
Xunlei

>
>> I was planning doing that, but sick recently, I will try to send a patch
>> doing that later.
> Yeah, totally understand. This is not urgent, please take care of
> yourself.
>
>> Regards,
>> Xunlei
>>
>>> Xunlei. Since Andrew asked, I just checked these.
>>>
>>> I am fine with Minfei's patch 1/2. But for patch 2/2, it's a little
>>> comfortable to me. Is it really necessary to abstract code block from
>>> kexec_load, then wrap them into a newly added function do_kexec_load()?
>>> Without this wrapping is there a way to do your bug fix? Is there
>>> possibility that do_kexec_load will be called in other places? What's
>>> the benefit to wrap it into do_kexec_load against not wrapping?
>>>
>>> Thanks
>>> Baoquan
>>>
>>>> Regards,
>>>> Xunlei
>>>>
>>>> On 03/01/2016 at 04:02 PM, Minfei Huang wrote:
>>>>> v1:
>>>>> - Bisect the patch according to Andrew Morton's suggestion
>>>>>
>>>>> Minfei Huang (2):
>>>>>   kexec: Make a pair of map/unmap reserved pages in error path
>>>>>   kexec: Do a cleanup for function kexec_load
>>>>>
>>>>>  kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
>>>>>  1 file changed, 63 insertions(+), 49 deletions(-)
>>>>>
>>>> _______________________________________________
>>>> kexec mailing list
>>>> kexec@lists.infradead.org
>>>> http://lists.infradead.org/mailman/listinfo/kexec
>>
>> _______________________________________________
>> kexec mailing list
>> kexec@lists.infradead.org
>> http://lists.infradead.org/mailman/listinfo/kexec