Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1181398 > unrolled thread
| Started by | Michael Holzheu <holzheu@linux.vnet.ibm.com> |
|---|---|
| First post | 2015-07-10 11:00 +0200 |
| Last post | 2015-07-10 11:30 +0200 |
| Articles | 4 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
Re: [PATCH v4] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Michael Holzheu <holzheu@linux.vnet.ibm.com> - 2015-07-10 11:00 +0200
Re: [PATCH v4] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Minfei Huang <mnfhuang@gmail.com> - 2015-07-10 11:10 +0200
Re: [PATCH v4] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Michael Holzheu <holzheu@linux.vnet.ibm.com> - 2015-07-10 11:20 +0200
Re: [PATCH v4] kexec: Make a pair of map and unmap reserved pages when kdump fails to start Michael Holzheu <holzheu@linux.vnet.ibm.com> - 2015-07-10 11:30 +0200
| From | Michael Holzheu <holzheu@linux.vnet.ibm.com> |
|---|---|
| Date | 2015-07-10 11:00 +0200 |
| Subject | Re: [PATCH v4] kexec: Make a pair of map and unmap reserved pages when kdump fails to start |
| Message-ID | <pKCk2-3Ii-27@gated-at.bofh.it> |
On Fri, 10 Jul 2015 13:12:17 +0800
Minfei Huang <mnfhuang@gmail.com> 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. So we make a pair of map/unmap reserved
> pages whatever kexec fails or not in code path.
>
> In order to make code readable, wrap a new function __kexec_load which
> contains all of the logic to deal with the image loading.
>
> Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> ---
> v3:
> - reconstruct the patch, wrap a new function to deal with the code logic, based on Vivek and Michael's patch
> v2:
> - replace the "failure" label with "fail_unmap_pages"
> v1:
> - reconstruct the patch code
> ---
> kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> 1 file changed, 63 insertions(+), 49 deletions(-)
>
> diff --git a/kernel/kexec.c b/kernel/kexec.c
> index a785c10..2232c90 100644
> --- a/kernel/kexec.c
> +++ b/kernel/kexec.c
> @@ -1247,10 +1247,71 @@ int kexec_load_disabled;
>
> static DEFINE_MUTEX(kexec_mutex);
>
> +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
> + struct kexec_segment __user *segments,
> + unsigned long flags)
> +{
> + int result = 0;
> + struct kimage **dest_image, *image;
> +
> + dest_image = &kexec_image;
> +
> + if (flags & KEXEC_ON_CRASH)
> + dest_image = &kexec_crash_image;
> +
> + if (nr_segments == 0) {
> + /* Install the new kernel, and Uninstall the old */
> + image = xchg(dest_image, image);
> + kimage_free(image);
Well this is wrong and should probably be:
if (nr_segments == 0) {
/* Uninstall image */
image = xchg(dest_image, NULL);
kimage_free(image);
> + } else {
> + unsigned long i;
> +
> + if (flags & KEXEC_ON_CRASH) {
> + /*
[snip]
> + result = kimage_load_segment(image, &image->segment[i]);
> + if (result)
> + goto failure_unmap_mem;
> + }
> +
> + kimage_terminate(image);
> +
> + /* Install the new kernel, and Uninstall the old */
Perhaps fix the comment: Remove superfluous blank and lowercase "uninstall"?
> + image = xchg(dest_image, image);
> +
> +failure_unmap_mem:
> + if (flags & KEXEC_ON_CRASH)
> + crash_unmap_reserved_pages();
> + kimage_free(image);
Here the update patch:
---
diff --git a/kernel/kexec.c b/kernel/kexec.c
index e686a39..2f5b4aa 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1249,8 +1249,8 @@ static int __kexec_load(unsigned long entry, unsigned long nr_segments,
dest_image = &kexec_crash_image;
if (nr_segments == 0) {
- /* Install the new kernel, and Uninstall the old */
- image = xchg(dest_image, image);
+ /* Uninstall image */
+ image = xchg(dest_image, NULL);
kimage_free(image);
} else {
unsigned long i;
@@ -1287,7 +1287,7 @@ static int __kexec_load(unsigned long entry, unsigned long nr_segments,
kimage_terminate(image);
- /* Install the new kernel, and Uninstall the old */
+ /* Install the new kernel, and uninstall the old */
image = xchg(dest_image, image);
failure_unmap_mem:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Minfei Huang <mnfhuang@gmail.com> |
|---|---|
| Date | 2015-07-10 11:10 +0200 |
| Message-ID | <pKCtI-417-9@gated-at.bofh.it> |
| In reply to | #1181398 |
On 07/10/15 at 10:54P, Michael Holzheu wrote:
> On Fri, 10 Jul 2015 13:12:17 +0800
> Minfei Huang <mnfhuang@gmail.com> 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. So we make a pair of map/unmap reserved
> > pages whatever kexec fails or not in code path.
> >
> > In order to make code readable, wrap a new function __kexec_load which
> > contains all of the logic to deal with the image loading.
> >
> > Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> > ---
> > v3:
> > - reconstruct the patch, wrap a new function to deal with the code logic, based on Vivek and Michael's patch
> > v2:
> > - replace the "failure" label with "fail_unmap_pages"
> > v1:
> > - reconstruct the patch code
> > ---
> > kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> > 1 file changed, 63 insertions(+), 49 deletions(-)
> >
> > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > index a785c10..2232c90 100644
> > --- a/kernel/kexec.c
> > +++ b/kernel/kexec.c
> > @@ -1247,10 +1247,71 @@ int kexec_load_disabled;
> >
> > static DEFINE_MUTEX(kexec_mutex);
> >
> > +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
> > + struct kexec_segment __user *segments,
> > + unsigned long flags)
> > +{
> > + int result = 0;
> > + struct kimage **dest_image, *image;
> > +
> > + dest_image = &kexec_image;
> > +
> > + if (flags & KEXEC_ON_CRASH)
> > + dest_image = &kexec_crash_image;
> > +
> > + if (nr_segments == 0) {
> > + /* Install the new kernel, and Uninstall the old */
> > + image = xchg(dest_image, image);
> > + kimage_free(image);
>
> Well this is wrong and should probably be:
>
> if (nr_segments == 0) {
> /* Uninstall image */
> image = xchg(dest_image, NULL);
> kimage_free(image);
>
You are right. It should be what you commented.
> > + } else {
> > + unsigned long i;
> > +
> > + if (flags & KEXEC_ON_CRASH) {
> > + /*
>
> [snip]
>
> > + result = kimage_load_segment(image, &image->segment[i]);
> > + if (result)
> > + goto failure_unmap_mem;
> > + }
> > +
> > + kimage_terminate(image);
> > +
> > + /* Install the new kernel, and Uninstall the old */
>
> Perhaps fix the comment: Remove superfluous blank and lowercase "uninstall"?
>
Thanks.
Minfei
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Michael Holzheu <holzheu@linux.vnet.ibm.com> |
|---|---|
| Date | 2015-07-10 11:20 +0200 |
| Message-ID | <pKCDo-44s-11@gated-at.bofh.it> |
| In reply to | #1181401 |
On Fri, 10 Jul 2015 17:03:22 +0800
Minfei Huang <mnfhuang@gmail.com> wrote:
> On 07/10/15 at 10:54P, Michael Holzheu wrote:
> > On Fri, 10 Jul 2015 13:12:17 +0800
> > Minfei Huang <mnfhuang@gmail.com> 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. So we make a pair of map/unmap reserved
> > > pages whatever kexec fails or not in code path.
> > >
> > > In order to make code readable, wrap a new function __kexec_load which
> > > contains all of the logic to deal with the image loading.
> > >
> > > Signed-off-by: Minfei Huang <mnfhuang@gmail.com>
> > > ---
> > > v3:
> > > - reconstruct the patch, wrap a new function to deal with the code logic, based on Vivek and Michael's patch
> > > v2:
> > > - replace the "failure" label with "fail_unmap_pages"
> > > v1:
> > > - reconstruct the patch code
> > > ---
> > > kernel/kexec.c | 112 ++++++++++++++++++++++++++++++++-------------------------
> > > 1 file changed, 63 insertions(+), 49 deletions(-)
> > >
> > > diff --git a/kernel/kexec.c b/kernel/kexec.c
> > > index a785c10..2232c90 100644
> > > --- a/kernel/kexec.c
> > > +++ b/kernel/kexec.c
> > > @@ -1247,10 +1247,71 @@ int kexec_load_disabled;
> > >
> > > static DEFINE_MUTEX(kexec_mutex);
> > >
> > > +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
> > > + struct kexec_segment __user *segments,
> > > + unsigned long flags)
> > > +{
> > > + int result = 0;
> > > + struct kimage **dest_image, *image;
> > > +
> > > + dest_image = &kexec_image;
> > > +
> > > + if (flags & KEXEC_ON_CRASH)
> > > + dest_image = &kexec_crash_image;
> > > +
> > > + if (nr_segments == 0) {
> > > + /* Install the new kernel, and Uninstall the old */
> > > + image = xchg(dest_image, image);
> > > + kimage_free(image);
> >
> > Well this is wrong and should probably be:
> >
> > if (nr_segments == 0) {
> > /* Uninstall image */
> > image = xchg(dest_image, NULL);
> > kimage_free(image);
> >
>
> You are right. It should be what you commented.
And after rethinking a bit, I think a one liner and an early exit
would be better in this case:
if (nr_segments == 0) {
/* Uninstall image */
kimage_free(xchg(dest_image, NULL));
return 0;
}
What about the following patch:
---
diff --git a/kernel/kexec.c b/kernel/kexec.c
index 7a36fdc..7837c4e 100644
--- a/kernel/kexec.c
+++ b/kernel/kexec.c
@@ -1236,10 +1236,68 @@ int kexec_load_disabled;
static DEFINE_MUTEX(kexec_mutex);
+static int __kexec_load(unsigned long entry, unsigned long nr_segments,
+ struct kexec_segment __user *segments,
+ unsigned long flags)
+{
+ struct kimage **dest_image, *image;
+ unsigned long i;
+ int result;
+
+ if (flags & KEXEC_ON_CRASH)
+ dest_image = &kexec_crash_image;
+ else
+ dest_image = &kexec_image;
+
+ if (nr_segments == 0) {
+ /* Uninstall image */
+ kfree(xchg(dest_image, NULL));
+ return 0;
+ }
+ 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 failure_unmap_mem;
+
+ for (i = 0; i < nr_segments; i++) {
+ result = kimage_load_segment(image, &image->segment[i]);
+ if (result)
+ goto failure_unmap_mem;
+ }
+
+ kimage_terminate(image);
+
+ /* Install the new kernel and uninstall the old */
+ image = xchg(dest_image, image);
+
+failure_unmap_mem:
+ 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. */
@@ -1264,9 +1322,6 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
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
@@ -1278,53 +1333,9 @@ SYSCALL_DEFINE4(kexec_load, unsigned long, entry, unsigned long, nr_segments,
if (!mutex_trylock(&kexec_mutex))
return -EBUSY;
- 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);
- if (result)
- goto out;
+ result = __kexec_load(entry, nr_segments, segments, flags);
- 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);
-
-out:
mutex_unlock(&kexec_mutex);
- kimage_free(image);
return result;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Michael Holzheu <holzheu@linux.vnet.ibm.com> |
|---|---|
| Date | 2015-07-10 11:30 +0200 |
| Message-ID | <pKCN4-47C-5@gated-at.bofh.it> |
| In reply to | #1181409 |
On Fri, 10 Jul 2015 11:14:06 +0200
Michael Holzheu <holzheu@linux.vnet.ibm.com> wrote:
> On Fri, 10 Jul 2015 17:03:22 +0800
> Minfei Huang <mnfhuang@gmail.com> wrote:
[snip]
> +static int __kexec_load(unsigned long entry, unsigned long nr_segments,
> + struct kexec_segment __user *segments,
> + unsigned long flags)
> +{
> + struct kimage **dest_image, *image;
> + unsigned long i;
> + int result;
> +
> + if (flags & KEXEC_ON_CRASH)
> + dest_image = &kexec_crash_image;
> + else
> + dest_image = &kexec_image;
> +
> + if (nr_segments == 0) {
> + /* Uninstall image */
> + kfree(xchg(dest_image, NULL));
Sorry, too fast today...
Should be of course not kfree, but:
kimage_free(dest_image, NULL));
Michael
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web