From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932750AbbGHMJq (ORCPT ); Wed, 8 Jul 2015 08:09:46 -0400 Received: from mail-pa0-f43.google.com ([209.85.220.43]:34888 "EHLO mail-pa0-f43.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758193AbbGHMJn (ORCPT ); Wed, 8 Jul 2015 08:09:43 -0400 Date: Wed, 8 Jul 2015 20:09:51 +0800 From: Minfei Huang To: Vivek Goyal Cc: ebiederm@xmission.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, linux390@de.ibm.com, holzheu@linux.vnet.ibm.com, linux-s390@vger.kernel.org, kexec@lists.infradead.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Message-ID: <20150708120951.GB8672@dhcp-128-42.nay.redhat.com> References: <1435801552-1230-1-git-send-email-mnfhuang@gmail.com> <20150707211840.GA4388@redhat.com> <20150708120638.GA8672@dhcp-128-42.nay.redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150708120638.GA8672@dhcp-128-42.nay.redhat.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 07/08/15 at 08:06P, Minfei Huang wrote: > On 07/07/15 at 05:18P, Vivek Goyal wrote: > > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote: > > > For some arch, kexec shall map the reserved pages, then use them, when > > > we try to start the kdump service. > > > > > > Now kexec will never unmap the reserved pages, once it fails to continue > > > starting the kdump service. > > > > > > Make a pair of reserved pages in kdump starting path, whatever kexec > > > fails or not. > > > > > > Signed-off-by: Minfei Huang > > > --- > > > v2: > > > - replace the "failure" label with "fail_unmap_pages" > > > v1: > > > - reconstruct the patch code > > > --- > > > kernel/kexec.c | 26 ++++++++++++++------------ > > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > > > > Hi Minfei, > > > > I am thinking of moving kernel loading code in a separate function to > > make things little simpler. Right now it is confusing. > > In my patch, I think maybe the label confuses with the reviewer, which > does not express the intention clearly. > > > > > Can you please test attached patch. I have only compile tested it. This > > is primarily doing what you are doing but in a separate function. It > > seems more readable now. > > > > The patch passed the simple testcase. Since it does change the code ^^^^^ does not change > logic, I think there is no risky. > > Thanks > Minfei > > > Thanks > > Vivek > > > > > > --- > > kernel/kexec.c | 90 +++++++++++++++++++++++++++++++++++---------------------- > > 1 file changed, 56 insertions(+), 34 deletions(-) > > > > Index: rhvgoyal-linux/kernel/kexec.c > > =================================================================== > > --- rhvgoyal-linux.orig/kernel/kexec.c 2015-07-06 13:59:35.088129148 -0400 > > +++ rhvgoyal-linux/kernel/kexec.c 2015-07-07 17:14:23.593175644 -0400 > > @@ -1247,6 +1247,57 @@ int kexec_load_disabled; > > > > static DEFINE_MUTEX(kexec_mutex); > > > > +static int __kexec_load(struct kimage **rimage, unsigned long entry, > > + unsigned long nr_segments, > > + struct kexec_segment __user * segments, > > + unsigned long flags) > > +{ > > + unsigned long i; > > + int result; > > + struct kimage *image; > > + > > + if (flags & KEXEC_ON_CRASH) { > > + /* > > + * Loading another kernel to switch to if this one > > + * crashes. Free any current crash dump kernel before > > + * we corrupt it. > > + */ > > + > > + kimage_free(xchg(&kexec_crash_image, NULL)); > > + } > > + > > + result = kimage_alloc_init(&image, entry, nr_segments, segments, flags); > > + if (result) > > + return result; > > + > > + if (flags & KEXEC_ON_CRASH) > > + crash_map_reserved_pages(); > > + > > + if (flags & KEXEC_PRESERVE_CONTEXT) > > + image->preserve_context = 1; > > + > > + result = machine_kexec_prepare(image); > > + if (result) > > + goto out; > > + > > + for (i = 0; i < nr_segments; i++) { > > + result = kimage_load_segment(image, &image->segment[i]); > > + if (result) > > + goto out; > > + } > > + > > + kimage_terminate(image); > > + *rimage = image; > > +out: > > + if (flags & KEXEC_ON_CRASH) > > + crash_unmap_reserved_pages(); > > + > > + /* Free image if there was an error */ > > + if (result) > > + kimage_free(image); > > + return result; > > +} > > + > > SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments, > > struct kexec_segment __user *, segments, unsigned long, flags) > > { > > @@ -1292,44 +1343,15 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon > > dest_image = &kexec_image; > > if (flags & KEXEC_ON_CRASH) > > dest_image = &kexec_crash_image; > > - if (nr_segments > 0) { > > - unsigned long i; > > > > - if (flags & KEXEC_ON_CRASH) { > > - /* > > - * Loading another kernel to switch to if this one > > - * crashes. Free any current crash dump kernel before > > - * we corrupt it. > > - */ > > - > > - kimage_free(xchg(&kexec_crash_image, NULL)); > > - result = kimage_alloc_init(&image, entry, nr_segments, > > - segments, flags); > > - crash_map_reserved_pages(); > > - } else { > > - /* Loading another kernel to reboot into. */ > > - > > - result = kimage_alloc_init(&image, entry, nr_segments, > > - segments, flags); > > - } > > - if (result) > > - goto out; > > - > > - if (flags & KEXEC_PRESERVE_CONTEXT) > > - image->preserve_context = 1; > > - result = machine_kexec_prepare(image); > > + /* Load new kernel */ > > + if (nr_segments > 0) { > > + result = __kexec_load(&image, entry, nr_segments, segments, > > + flags); > > if (result) > > goto out; > > - > > - for (i = 0; i < nr_segments; i++) { > > - result = kimage_load_segment(image, &image->segment[i]); > > - if (result) > > - goto out; > > - } > > - kimage_terminate(image); > > - if (flags & KEXEC_ON_CRASH) > > - crash_unmap_reserved_pages(); > > } > > + > > /* Install the new kernel, and Uninstall the old */ > > image = xchg(dest_image, image); > > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from mail-pa0-x22e.google.com ([2607:f8b0:400e:c03::22e]) by bombadil.infradead.org with esmtps (Exim 4.80.1 #2 (Red Hat Linux)) id 1ZCoAu-0006ci-4x for kexec@lists.infradead.org; Wed, 08 Jul 2015 12:10:04 +0000 Received: by pacws9 with SMTP id ws9so132318683pac.0 for ; Wed, 08 Jul 2015 05:09:43 -0700 (PDT) Date: Wed, 8 Jul 2015 20:09:51 +0800 From: Minfei Huang Subject: Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Message-ID: <20150708120951.GB8672@dhcp-128-42.nay.redhat.com> References: <1435801552-1230-1-git-send-email-mnfhuang@gmail.com> <20150707211840.GA4388@redhat.com> <20150708120638.GA8672@dhcp-128-42.nay.redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20150708120638.GA8672@dhcp-128-42.nay.redhat.com> List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "kexec" Errors-To: kexec-bounces+dwmw2=infradead.org@lists.infradead.org To: Vivek Goyal Cc: linux-s390@vger.kernel.org, kexec@lists.infradead.org, heiko.carstens@de.ibm.com, linux-kernel@vger.kernel.org, ebiederm@xmission.com, schwidefsky@de.ibm.com, linux390@de.ibm.com, holzheu@linux.vnet.ibm.com On 07/08/15 at 08:06P, Minfei Huang wrote: > On 07/07/15 at 05:18P, Vivek Goyal wrote: > > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote: > > > For some arch, kexec shall map the reserved pages, then use them, when > > > we try to start the kdump service. > > > > > > Now kexec will never unmap the reserved pages, once it fails to continue > > > starting the kdump service. > > > > > > Make a pair of reserved pages in kdump starting path, whatever kexec > > > fails or not. > > > > > > Signed-off-by: Minfei Huang > > > --- > > > v2: > > > - replace the "failure" label with "fail_unmap_pages" > > > v1: > > > - reconstruct the patch code > > > --- > > > kernel/kexec.c | 26 ++++++++++++++------------ > > > 1 file changed, 14 insertions(+), 12 deletions(-) > > > > > > > Hi Minfei, > > > > I am thinking of moving kernel loading code in a separate function to > > make things little simpler. Right now it is confusing. > > In my patch, I think maybe the label confuses with the reviewer, which > does not express the intention clearly. > > > > > Can you please test attached patch. I have only compile tested it. This > > is primarily doing what you are doing but in a separate function. It > > seems more readable now. > > > > The patch passed the simple testcase. Since it does change the code ^^^^^ does not change > logic, I think there is no risky. > > Thanks > Minfei > > > Thanks > > Vivek > > > > > > --- > > kernel/kexec.c | 90 +++++++++++++++++++++++++++++++++++---------------------- > > 1 file changed, 56 insertions(+), 34 deletions(-) > > > > Index: rhvgoyal-linux/kernel/kexec.c > > =================================================================== > > --- rhvgoyal-linux.orig/kernel/kexec.c 2015-07-06 13:59:35.088129148 -0400 > > +++ rhvgoyal-linux/kernel/kexec.c 2015-07-07 17:14:23.593175644 -0400 > > @@ -1247,6 +1247,57 @@ int kexec_load_disabled; > > > > static DEFINE_MUTEX(kexec_mutex); > > > > +static int __kexec_load(struct kimage **rimage, unsigned long entry, > > + unsigned long nr_segments, > > + struct kexec_segment __user * segments, > > + unsigned long flags) > > +{ > > + unsigned long i; > > + int result; > > + struct kimage *image; > > + > > + if (flags & KEXEC_ON_CRASH) { > > + /* > > + * Loading another kernel to switch to if this one > > + * crashes. Free any current crash dump kernel before > > + * we corrupt it. > > + */ > > + > > + kimage_free(xchg(&kexec_crash_image, NULL)); > > + } > > + > > + result = kimage_alloc_init(&image, entry, nr_segments, segments, flags); > > + if (result) > > + return result; > > + > > + if (flags & KEXEC_ON_CRASH) > > + crash_map_reserved_pages(); > > + > > + if (flags & KEXEC_PRESERVE_CONTEXT) > > + image->preserve_context = 1; > > + > > + result = machine_kexec_prepare(image); > > + if (result) > > + goto out; > > + > > + for (i = 0; i < nr_segments; i++) { > > + result = kimage_load_segment(image, &image->segment[i]); > > + if (result) > > + goto out; > > + } > > + > > + kimage_terminate(image); > > + *rimage = image; > > +out: > > + if (flags & KEXEC_ON_CRASH) > > + crash_unmap_reserved_pages(); > > + > > + /* Free image if there was an error */ > > + if (result) > > + kimage_free(image); > > + return result; > > +} > > + > > SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments, > > struct kexec_segment __user *, segments, unsigned long, flags) > > { > > @@ -1292,44 +1343,15 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon > > dest_image = &kexec_image; > > if (flags & KEXEC_ON_CRASH) > > dest_image = &kexec_crash_image; > > - if (nr_segments > 0) { > > - unsigned long i; > > > > - if (flags & KEXEC_ON_CRASH) { > > - /* > > - * Loading another kernel to switch to if this one > > - * crashes. Free any current crash dump kernel before > > - * we corrupt it. > > - */ > > - > > - kimage_free(xchg(&kexec_crash_image, NULL)); > > - result = kimage_alloc_init(&image, entry, nr_segments, > > - segments, flags); > > - crash_map_reserved_pages(); > > - } else { > > - /* Loading another kernel to reboot into. */ > > - > > - result = kimage_alloc_init(&image, entry, nr_segments, > > - segments, flags); > > - } > > - if (result) > > - goto out; > > - > > - if (flags & KEXEC_PRESERVE_CONTEXT) > > - image->preserve_context = 1; > > - result = machine_kexec_prepare(image); > > + /* Load new kernel */ > > + if (nr_segments > 0) { > > + result = __kexec_load(&image, entry, nr_segments, segments, > > + flags); > > if (result) > > goto out; > > - > > - for (i = 0; i < nr_segments; i++) { > > - result = kimage_load_segment(image, &image->segment[i]); > > - if (result) > > - goto out; > > - } > > - kimage_terminate(image); > > - if (flags & KEXEC_ON_CRASH) > > - crash_unmap_reserved_pages(); > > } > > + > > /* Install the new kernel, and Uninstall the old */ > > image = xchg(dest_image, image); > > _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec