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

[Baoquan He]

On 03/23/16 at 05:59pm, Xunlei Pang wrote:
> 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.

OK, sounds good, I am fine with this.

How do you think about Minfei's patch? You pick it up into your patchset
in next post with his author, or just wait for him to repost? Apparently
his patch has conflict with yours.

> 
> 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@xxxxxxxxxxxxxxxxxxx
> >>>> http://lists.infradead.org/mailman/listinfo/kexec
> >>
> >> _______________________________________________
> >> kexec mailing list
> >> kexec@xxxxxxxxxxxxxxxxxxx
> >> http://lists.infradead.org/mailman/listinfo/kexec
>