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