From: Minfei Huang <mnfhuang@gmail.com> To: Vivek Goyal <vgoyal@redhat.com> 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 Date: Wed, 8 Jul 2015 20:06:38 +0800 [thread overview] Message-ID: <20150708120638.GA8672@dhcp-128-42.nay.redhat.com> (raw) In-Reply-To: <20150707211840.GA4388@redhat.com> 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 <mnfhuang@gmail.com> > > --- > > 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 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); >
WARNING: multiple messages have this Message-ID (diff)
From: Minfei Huang <mnfhuang@gmail.com> To: Vivek Goyal <vgoyal@redhat.com> 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 Subject: Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Date: Wed, 8 Jul 2015 20:06:38 +0800 [thread overview] Message-ID: <20150708120638.GA8672@dhcp-128-42.nay.redhat.com> (raw) In-Reply-To: <20150707211840.GA4388@redhat.com> 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 <mnfhuang@gmail.com> > > --- > > 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 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
next prev parent reply other threads:[~2015-07-08 12:06 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2015-07-02 1:45 [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Minfei Huang 2015-07-02 1:45 ` Minfei Huang 2015-07-07 21:18 ` Vivek Goyal 2015-07-07 21:18 ` Vivek Goyal 2015-07-08 12:06 ` Minfei Huang [this message] 2015-07-08 12:06 ` Minfei Huang 2015-07-08 12:09 ` Minfei Huang 2015-07-08 12:09 ` Minfei Huang 2015-07-09 15:54 ` Michael Holzheu 2015-07-09 15:54 ` Michael Holzheu 2015-07-09 23:37 ` Minfei Huang 2015-07-09 23:37 ` Minfei Huang 2015-07-10 4:05 ` Minfei Huang 2015-07-10 4:05 ` Minfei Huang 2015-07-10 8:28 ` Michael Holzheu 2015-07-10 8:28 ` Michael Holzheu
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=20150708120638.GA8672@dhcp-128-42.nay.redhat.com \ --to=mnfhuang@gmail.com \ --cc=ebiederm@xmission.com \ --cc=heiko.carstens@de.ibm.com \ --cc=holzheu@linux.vnet.ibm.com \ --cc=kexec@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-s390@vger.kernel.org \ --cc=linux390@de.ibm.com \ --cc=schwidefsky@de.ibm.com \ --cc=vgoyal@redhat.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.