From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mx1.redhat.com ([209.132.183.28]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1aj0lH-0006HM-PI for kexec@lists.infradead.org; Thu, 24 Mar 2016 08:37:00 +0000 Subject: Re: [PATCH V2 0/2] kexec: Make a pair of map/unmap reserved pages in error path References: <1456819349-8650-1-git-send-email-mnfhuang@gmail.com> <56D56684.9000908@redhat.com> <20160323024836.GC2567@x1.redhat.com> <56F20E34.5040303@redhat.com> <20160323082344.GB4048@x1.redhat.com> <56F268F3.4020806@redhat.com> <20160323123215.GA16421@x1.redhat.com> From: Xunlei Pang Message-ID: <56F3A712.1010902@redhat.com> Date: Thu, 24 Mar 2016 16:36:34 +0800 MIME-Version: 1.0 In-Reply-To: <20160323123215.GA16421@x1.redhat.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Reply-To: xlpang@redhat.com Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Baoquan He Cc: mhuang@redhat.com, kexec@lists.infradead.org, linux-kernel@vger.kernel.org, ebiederm@xmission.com, akpm@linux-foundation.org, Minfei Huang On 2016/03/23 at 20:32, Baoquan He wrote: > 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. Essentially, Minfei's patchset has nothing to do with mine, his is a bug fix, I think bugs have priority. Conflicts are commonplace in upstream patches, should not be a problem. Just my two cents. Regards, Xunlei > >> 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 _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec