From: Michael Holzheu <holzheu@linux.vnet.ibm.com> To: Vivek Goyal <vgoyal@redhat.com> Cc: Minfei Huang <mnfhuang@gmail.com>, ebiederm@xmission.com, schwidefsky@de.ibm.com, heiko.carstens@de.ibm.com, linux390@de.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: Thu, 9 Jul 2015 17:54:11 +0200 [thread overview] Message-ID: <20150709175411.7037a4fa@holzheu> (raw) In-Reply-To: <20150707211840.GA4388@redhat.com> On Tue, 7 Jul 2015 17:18:40 -0400 Vivek Goyal <vgoyal@redhat.com> wrote: > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote: [snip] > I am thinking of moving kernel loading code in a separate function to > make things little simpler. Right now it is confusing. > > 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 looks good to me. What about the following patch on top to make things even more readable? --- kernel/kexec.c | 50 +++++++++++++++++--------------------------------- 1 file changed, 17 insertions(+), 33 deletions(-) --- a/kernel/kexec.c +++ b/kernel/kexec.c @@ -1236,14 +1236,18 @@ int kexec_load_disabled; static DEFINE_MUTEX(kexec_mutex); -static int __kexec_load(struct kimage **rimage, unsigned long entry, - unsigned long nr_segments, +static int __kexec_load(unsigned long entry, unsigned long nr_segments, struct kexec_segment __user * segments, unsigned long flags) { + struct kimage *image, **dest_image; unsigned long i; int result; - struct kimage *image; + + dest_image = (flags & KEXEC_ON_CRASH) ? &kexec_crash_image : &kexec_image; + + if (nr_segments == 0) + return 0; if (flags & KEXEC_ON_CRASH) { /* @@ -1251,7 +1255,6 @@ static int __kexec_load(struct kimage ** * crashes. Free any current crash dump kernel before * we corrupt it. */ - kimage_free(xchg(&kexec_crash_image, NULL)); } @@ -1267,30 +1270,29 @@ static int __kexec_load(struct kimage ** result = machine_kexec_prepare(image); if (result) - goto out; + goto fail; for (i = 0; i < nr_segments; i++) { result = kimage_load_segment(image, &image->segment[i]); if (result) - goto out; + goto fail; } - kimage_terminate(image); - *rimage = image; -out: + /* Install the new kernel, and uninstall the old */ + kimage_free(xchg(dest_image, image)); if (flags & KEXEC_ON_CRASH) crash_unmap_reserved_pages(); - - /* Free image if there was an error */ - if (result) - kimage_free(image); + return 0; +fail: + if (flags & KEXEC_ON_CRASH) + crash_unmap_reserved_pages(); + kimage_free(image); return result; } SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments, struct kexec_segment __user *, segments, unsigned long, flags) { - struct kimage **dest_image, *image; int result; /* We only trust the superuser with rebooting the system. */ @@ -1315,9 +1317,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon if (nr_segments > KEXEC_SEGMENT_MAX) return -EINVAL; - image = NULL; - result = 0; - /* Because we write directly to the reserved memory * region when loading crash kernels we need a mutex here to * prevent multiple crash kernels from attempting to load @@ -1329,24 +1328,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon if (!mutex_trylock(&kexec_mutex)) return -EBUSY; - dest_image = &kexec_image; - if (flags & KEXEC_ON_CRASH) - dest_image = &kexec_crash_image; - /* Load new kernel */ - if (nr_segments > 0) { - result = __kexec_load(&image, entry, nr_segments, segments, - flags); - if (result) - goto out; - } - - /* Install the new kernel, and Uninstall the old */ - image = xchg(dest_image, image); - -out: + result = __kexec_load(entry, nr_segments, segments, flags); mutex_unlock(&kexec_mutex); - kimage_free(image); return result; }
WARNING: multiple messages have this Message-ID (diff)
From: Michael Holzheu <holzheu@linux.vnet.ibm.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, Minfei Huang <mnfhuang@gmail.com>, schwidefsky@de.ibm.com, linux390@de.ibm.com, ebiederm@xmission.com Subject: Re: [PATCH v3] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Date: Thu, 9 Jul 2015 17:54:11 +0200 [thread overview] Message-ID: <20150709175411.7037a4fa@holzheu> (raw) In-Reply-To: <20150707211840.GA4388@redhat.com> On Tue, 7 Jul 2015 17:18:40 -0400 Vivek Goyal <vgoyal@redhat.com> wrote: > On Thu, Jul 02, 2015 at 09:45:52AM +0800, Minfei Huang wrote: [snip] > I am thinking of moving kernel loading code in a separate function to > make things little simpler. Right now it is confusing. > > 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 looks good to me. What about the following patch on top to make things even more readable? --- kernel/kexec.c | 50 +++++++++++++++++--------------------------------- 1 file changed, 17 insertions(+), 33 deletions(-) --- a/kernel/kexec.c +++ b/kernel/kexec.c @@ -1236,14 +1236,18 @@ int kexec_load_disabled; static DEFINE_MUTEX(kexec_mutex); -static int __kexec_load(struct kimage **rimage, unsigned long entry, - unsigned long nr_segments, +static int __kexec_load(unsigned long entry, unsigned long nr_segments, struct kexec_segment __user * segments, unsigned long flags) { + struct kimage *image, **dest_image; unsigned long i; int result; - struct kimage *image; + + dest_image = (flags & KEXEC_ON_CRASH) ? &kexec_crash_image : &kexec_image; + + if (nr_segments == 0) + return 0; if (flags & KEXEC_ON_CRASH) { /* @@ -1251,7 +1255,6 @@ static int __kexec_load(struct kimage ** * crashes. Free any current crash dump kernel before * we corrupt it. */ - kimage_free(xchg(&kexec_crash_image, NULL)); } @@ -1267,30 +1270,29 @@ static int __kexec_load(struct kimage ** result = machine_kexec_prepare(image); if (result) - goto out; + goto fail; for (i = 0; i < nr_segments; i++) { result = kimage_load_segment(image, &image->segment[i]); if (result) - goto out; + goto fail; } - kimage_terminate(image); - *rimage = image; -out: + /* Install the new kernel, and uninstall the old */ + kimage_free(xchg(dest_image, image)); if (flags & KEXEC_ON_CRASH) crash_unmap_reserved_pages(); - - /* Free image if there was an error */ - if (result) - kimage_free(image); + return 0; +fail: + if (flags & KEXEC_ON_CRASH) + crash_unmap_reserved_pages(); + kimage_free(image); return result; } SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments, struct kexec_segment __user *, segments, unsigned long, flags) { - struct kimage **dest_image, *image; int result; /* We only trust the superuser with rebooting the system. */ @@ -1315,9 +1317,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon if (nr_segments > KEXEC_SEGMENT_MAX) return -EINVAL; - image = NULL; - result = 0; - /* Because we write directly to the reserved memory * region when loading crash kernels we need a mutex here to * prevent multiple crash kernels from attempting to load @@ -1329,24 +1328,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned lon if (!mutex_trylock(&kexec_mutex)) return -EBUSY; - dest_image = &kexec_image; - if (flags & KEXEC_ON_CRASH) - dest_image = &kexec_crash_image; - /* Load new kernel */ - if (nr_segments > 0) { - result = __kexec_load(&image, entry, nr_segments, segments, - flags); - if (result) - goto out; - } - - /* Install the new kernel, and Uninstall the old */ - image = xchg(dest_image, image); - -out: + result = __kexec_load(entry, nr_segments, segments, flags); mutex_unlock(&kexec_mutex); - kimage_free(image); return result; } _______________________________________________ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
next prev parent reply other threads:[~2015-07-09 15:54 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 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 [this message] 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=20150709175411.7037a4fa@holzheu \ --to=holzheu@linux.vnet.ibm.com \ --cc=ebiederm@xmission.com \ --cc=heiko.carstens@de.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=mnfhuang@gmail.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.