4:07 a.m.
The help/man page of the "vm" command suggests that the "-M" option
accepts the mm_struct address as a valid argument. However, the "vm
-M" option always prints the virtual memory data of the current
task, regardless of its arguments.
Without the patch:
crash> ps |grep sshd
1159 1 19 ffff9915858a9980 IN 0.1 15820 9020 sshd
47236 1159 21 ffff991572b40000 IN 0.1 18916 11116 sshd
...
crash> vm -M ffff991572b40000
PID: 49647 TASK: ffff991580b39980 CPU: 10 COMMAND: "crash"
...
With the patch:
crash> vm -M ffff991572b40000
PID: 47236 TASK: ffff991572b40000 CPU: 21 COMMAND: "sshd"
...
Reported-by: Buland Kumar Singh <bsingh(a)redhat.com>
Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
---
memory.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/memory.c b/memory.c
index 592a5ef49d50..c9861dd6d0fb 100644
--- a/memory.c
+++ b/memory.c
@@ -3640,6 +3640,7 @@ cmd_vm(void)
else if (radix == 16)
flag |= PRINT_RADIX_16;
+ optind = 1;
if (!args[optind]) {
if (!ref)
print_task_header(fp, CURRENT_CONTEXT(), 0);
@@ -3650,26 +3651,28 @@ cmd_vm(void)
subsequent = 0;
while (args[optind]) {
- switch (str_to_context(args[optind], &value, &tc))
- {
- case STR_PID:
- for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
- if (!ref)
- print_task_header(fp, tc, subsequent++);
- vm_area_dump(tc->task, flag, single_vma, ref);
- }
- break;
+ if (IS_A_NUMBER(args[optind])) {
+ switch (str_to_context(args[optind], &value, &tc))
+ {
+ case STR_PID:
+ for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
+ if (!ref)
+ print_task_header(fp, tc, subsequent++);
+ vm_area_dump(tc->task, flag, single_vma, ref);
+ }
+ break;
- case STR_TASK:
- if (!ref)
- print_task_header(fp, tc, subsequent++);
- vm_area_dump(tc->task, flag, single_vma, ref);
- break;
+ case STR_TASK:
+ if (!ref)
+ print_task_header(fp, tc, subsequent++);
+ vm_area_dump(tc->task, flag, single_vma, ref);
+ break;
- case STR_INVALID:
- error(INFO, "%sinvalid task or pid value: %s\n",
- subsequent++ ? "\n" : "", args[optind]);
- break;
+ case STR_INVALID:
+ error(INFO, "%sinvalid task or pid value: %s\n",
+ subsequent++ ? "\n" : "", args[optind]);
+ break;
+ }
}
optind++;
--
2.37.1
7:22 a.m.
Hi Lianbo,
On Wed, 15 Mar 2023 17:07:05 +0800
Lianbo Jiang <lijiang(a)redhat.com> wrote:
The help/man page of the "vm" command suggests that the
"-M" option
accepts the mm_struct address as a valid argument. However, the "vm
-M" option always prints the virtual memory data of the current
task, regardless of its arguments.
Without the patch:
crash> ps |grep sshd
1159 1 19 ffff9915858a9980 IN 0.1 15820 9020 sshd
47236 1159 21 ffff991572b40000 IN 0.1 18916 11116 sshd
...
crash> vm -M ffff991572b40000
PID: 49647 TASK: ffff991580b39980 CPU: 10 COMMAND: "crash"
...
With the patch:
crash> vm -M ffff991572b40000
PID: 47236 TASK: ffff991572b40000 CPU: 21 COMMAND: "sshd"
...
Reported-by: Buland Kumar Singh <bsingh(a)redhat.com>
Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
---
memory.c | 39 +++++++++++++++++++++------------------
1 file changed, 21 insertions(+), 18 deletions(-)
diff --git a/memory.c b/memory.c
index 592a5ef49d50..c9861dd6d0fb 100644
--- a/memory.c
+++ b/memory.c
@@ -3640,6 +3640,7 @@ cmd_vm(void)
else if (radix == 16)
flag |= PRINT_RADIX_16;
+ optind = 1;
if (!args[optind]) {
if (!ref)
print_task_header(fp, CURRENT_CONTEXT(), 0);
@@ -3650,26 +3651,28 @@ cmd_vm(void)
subsequent = 0;
while (args[optind]) {
- switch (str_to_context(args[optind], &value, &tc))
- {
- case STR_PID:
- for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
- if (!ref)
- print_task_header(fp, tc, subsequent++);
- vm_area_dump(tc->task, flag, single_vma, ref);
- }
- break;
+ if (IS_A_NUMBER(args[optind])) {
wouldn't it be easier to simply add
if (!IS_A_NUMBER(args[optind])) {
optind++;
continue;
}
instead of moving the whole body of the loop inside the if-block. Or
did I miss something?
Thanks
Philipp
+ switch (str_to_context(args[optind], &value, &tc))
+ {
+ case STR_PID:
+ for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
+ if (!ref)
+ print_task_header(fp, tc, subsequent++);
+ vm_area_dump(tc->task, flag, single_vma, ref);
+ }
+ break;
- case STR_TASK:
- if (!ref)
- print_task_header(fp, tc, subsequent++);
- vm_area_dump(tc->task, flag, single_vma, ref);
- break;
+ case STR_TASK:
+ if (!ref)
+ print_task_header(fp, tc, subsequent++);
+ vm_area_dump(tc->task, flag, single_vma, ref);
+ break;
- case STR_INVALID:
- error(INFO, "%sinvalid task or pid value: %s\n",
- subsequent++ ? "\n" : "", args[optind]);
- break;
+ case STR_INVALID:
+ error(INFO, "%sinvalid task or pid value: %s\n",
+ subsequent++ ? "\n" : "", args[optind]);
+ break;
+ }
}
optind++;
8:17 p.m.
On Wed, Mar 15, 2023 at 8:22 PM Philipp Rudo <prudo(a)redhat.com> wrote:
> + if (IS_A_NUMBER(args[optind])) {
wouldn't it be easier to simply add
if (!IS_A_NUMBER(args[optind])) {
optind++;
continue;
}
instead of moving the whole body of the loop inside the if-block. Or
did I miss something?
Thank you for the comment, Philipp.
I saw the similar code style in the task.c, just followed these changes.
But anyway, the above changes also look better to me.
Thanks.
Lianbo
8:02 p.m.
Hi Lianbo,
On 2023/03/15 18:07, Lianbo Jiang wrote:
The help/man page of the "vm" command suggests that the
"-M" option
accepts the mm_struct address as a valid argument. However, the "vm
-M" option always prints the virtual memory data of the current
task, regardless of its arguments.
I think that it's the intended behavior, isn't it?
-M mm if the mm_struct address has been removed from the task_struct
of an exiting task, the virtual memory data cannot be displayed.
However, if the address can be determined from the kernel stack,
it can be entered manually in order to try to resurrect the
virtual memory data of the task.
Thanks,
Kazu
>
> Without the patch:
> crash> ps |grep sshd
> 1159 1 19 ffff9915858a9980 IN 0.1 15820 9020 sshd
> 47236 1159 21 ffff991572b40000 IN 0.1 18916 11116 sshd
> ...
> crash> vm -M ffff991572b40000
> PID: 49647 TASK: ffff991580b39980 CPU: 10 COMMAND: "crash"
> ...
>
> With the patch:
> crash> vm -M ffff991572b40000
> PID: 47236 TASK: ffff991572b40000 CPU: 21 COMMAND: "sshd"
> ...
>
> Reported-by: Buland Kumar Singh <bsingh(a)redhat.com>
> Signed-off-by: Lianbo Jiang <lijiang(a)redhat.com>
> ---
> memory.c | 39 +++++++++++++++++++++------------------
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/memory.c b/memory.c
> index 592a5ef49d50..c9861dd6d0fb 100644
> --- a/memory.c
> +++ b/memory.c
> @@ -3640,6 +3640,7 @@ cmd_vm(void)
> else if (radix == 16)
> flag |= PRINT_RADIX_16;
>
> + optind = 1;
> if (!args[optind]) {
> if (!ref)
> print_task_header(fp, CURRENT_CONTEXT(), 0);
> @@ -3650,26 +3651,28 @@ cmd_vm(void)
> subsequent = 0;
>
> while (args[optind]) {
> - switch (str_to_context(args[optind], &value, &tc))
> - {
> - case STR_PID:
> - for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
> - if (!ref)
> - print_task_header(fp, tc, subsequent++);
> - vm_area_dump(tc->task, flag, single_vma, ref);
> - }
> - break;
> + if (IS_A_NUMBER(args[optind])) {
> + switch (str_to_context(args[optind], &value, &tc))
> + {
> + case STR_PID:
> + for (tc = pid_to_context(value); tc; tc = tc->tc_next) {
> + if (!ref)
> + print_task_header(fp, tc, subsequent++);
> + vm_area_dump(tc->task, flag, single_vma, ref);
> + }
> + break;
>
> - case STR_TASK:
> - if (!ref)
> - print_task_header(fp, tc, subsequent++);
> - vm_area_dump(tc->task, flag, single_vma, ref);
> - break;
> + case STR_TASK:
> + if (!ref)
> + print_task_header(fp, tc, subsequent++);
> + vm_area_dump(tc->task, flag, single_vma, ref);
> + break;
>
> - case STR_INVALID:
> - error(INFO, "%sinvalid task or pid value: %s\n",
> - subsequent++ ? "\n" : "", args[optind]);
> - break;
> + case STR_INVALID:
> + error(INFO, "%sinvalid task or pid value: %s\n",
> + subsequent++ ? "\n" : "", args[optind]);
> + break;
> + }
> }
>
> optind++;
9:12 p.m.
Hi, Kazu
Thank you for the comment.
On Fri, Mar 17, 2023 at 9:02 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
Hi Lianbo,
On 2023/03/15 18:07, Lianbo Jiang wrote:
> The help/man page of the "vm" command suggests that the "-M"
option
> accepts the mm_struct address as a valid argument. However, the "vm
> -M" option always prints the virtual memory data of the current
> task, regardless of its arguments.
I think that it's the intended behavior, isn't it?
-M mm if the mm_struct address has been removed from the
task_struct
of an exiting task, the virtual memory data cannot be
displayed.
However, if the address can be determined from the kernel
stack,
it can be entered manually in order to try to resurrect
the
virtual memory data of the task.
They are not the same issues.
For the live debugging, take the sshd as an example:
Without the patch:
crash> ps |grep sshd
1159 1 3 ffff9915858a9980 IN 0.1 15820 9020 sshd
51573 1159 2 ffff991572868000 IN 0.1 18916 11168 sshd
51595 51573 2 ffff991575026600 IN 0.0 18784 6892 sshd
crash> vm -M ffff991572868000
PID: 51727 TASK: ffff9914465cb300 CPU: 0 COMMAND: "crash"
<------not a correct result
MM PGD RSS TOTAL_VM
ffff991572665f00 ffff99158565e000 448808k 2426788k
VMA START END FLAGS FILE
ffff991574e20a18 400000 4de000 8000071 /home/crash/crash
ffff991574e20ab0 4de000 a85000 8000075 /home/crash/crash
ffff991574e20260 a85000 e0f000 8000071 /home/crash/crash
ffff991574e204c0 e0f000 e10000 8100071 /home/crash/crash
ffff991443987980 e10000 e37000 8100073 /home/crash/crash
ffff991574e20720 e37000 1105000 8100073
ffff9914439877b8 21ab000 a178000 8100073
ffff991573b4b5f0 7f4304174000 7f4310000000 8100073
ffff9914d387e098 7f4310000000 7f431006c000 8200073
...
With the patch:
crash> ps |grep sshd
1159 1 3 ffff9915858a9980 IN 0.1 15820 9020 sshd
51573 1159 2 ffff991572868000 IN 0.1 18916 11168 sshd
51595 51573 2 ffff991575026600 IN 0.0 18784 6892 sshd
crash> vm -M ffff991572868000
PID: 51573 TASK: ffff991572868000 CPU: 2 COMMAND: "sshd" <---an
expected result
MM PGD RSS TOTAL_VM
ffff991449816900 ffff991442d68000 11168k 18916k
VMA START END FLAGS FILE
ffff991445df8da8 556020069000 556020075000 8000071 /usr/sbin/sshd
ffff991445df8390 556020075000 556020102000 8000075 /usr/sbin/sshd
ffff991445df87b8 556020102000 55602014c000 8000071 /usr/sbin/sshd
ffff991445df8850 55602014c000 556020150000 8100071 /usr/sbin/sshd
ffff99144a5efd10 556020150000 556020151000 8100073 /usr/sbin/sshd
ffff991445df82f8 556020151000 556020155000 8100073
ffff99157750fe40 556020753000 556020838000 8100073
ffff99158602d688 7ff090f6c000 7ff090f73000 8000071
/usr/lib64/libnss_systemd.so.2
...
Another example with an invalid argument:
Without the patch:
crash> vm -M 0xdeadbeef
PID: 51944 TASK: ffff99148c036600 CPU: 2 COMMAND: "crash"
MM PGD RSS TOTAL_VM
ffff9915745e8000 ffff9915728c4000 448684k 2426788k
VMA START END FLAGS FILE
ffff9914c07547b8 400000 4de000 8000071 /home/crash/crash
ffff9914c0754688 4de000 a85000 8000075 /home/crash/crash
ffff9914c0754ed8 a85000 e0f000 8000071 /home/crash/crash
ffff9914c0754c78 e0f000 e10000 8100071 /home/crash/crash
ffff99158554f1c8 e10000 e37000 8100073 /home/crash/crash
ffff9914c07544c0 e37000 1105000 8100073
ffff99158554f098 2b3f000 ab0c000 8100073
ffff9915490871c8 7fde73358000 7fde74321000 8100073
ffff9914c0426260 7fde76b50000 7fde84000000 8100073
...
With the patch:
crash> vm -M 0xdeadbeef
vm: invalid task or pid value: 0xdeadbeef
Hope this helps.
Thanks
Lianbo
9:26 p.m.
On 2023/03/17 11:12, lijiang wrote:
Hi, Kazu
Thank you for the comment.
On Fri, Mar 17, 2023 at 9:02 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com
<mailto:k-hagio-ab@nec.com>> wrote:
Hi Lianbo,
On 2023/03/15 18:07, Lianbo Jiang wrote:
> The help/man page of the "vm" command suggests that the
"-M" option
> accepts the mm_struct address as a valid argument. However, the "vm
> -M" option always prints the virtual memory data of the current
> task, regardless of its arguments.
I think that it's the intended behavior, isn't it?
-M mm if the mm_struct address has been removed from the task_struct
of an exiting task, the virtual memory data cannot be displayed.
However, if the address can be determined from the kernel stack,
it can be entered manually in order to try to resurrect the
virtual memory data of the task.
They are not the same issues.
For the live debugging, take the sshd as an example:
Without the patch:
crash> ps |grep sshd
1159 1 3 ffff9915858a9980 IN 0.1 15820 9020 sshd
51573 1159 2 ffff991572868000 IN 0.1 18916 11168 sshd
51595 51573 2 ffff991575026600 IN 0.0 18784 6892 sshd
crash> vm -M ffff991572868000
PID: 51727 TASK: ffff9914465cb300 CPU: 0 COMMAND: "crash"
<------not a correct result
No, I meant that this is correct. Let me explain,
(1) The "-M mm" option accepts an *mm_struct* address.
(2) The specified mm is enabled only when the mm_struct address has been
removedfrom the task_struct, i.e. mm = 0.
So we can emulate that situation e.g. with kthreadd (pid=2) like this:
crash> vm 1
PID: 1 TASK: ffff900b41599840 CPU: 11 COMMAND: "systemd"
MM PGD RSS TOTAL_VM
ffff9019ff0e3f00 ffff9019ffc10000 12516k 250328k
VMA START END FLAGS FILE
ffff900c94de2488 55e2c48d1000 55e2c4a13000 8000875 /usr/lib/systemd/systemd
...
crash> vm 2
PID: 2 TASK: ffff900b4159b080 CPU: 2 COMMAND: "kthreadd"
MM PGD RSS TOTAL_VM
0 0 0k 0k
^^^
crash> vm 2 -M ffff9019ff0e3f00 <<-- systemd's mm
PID: 2 TASK: ffff900b4159b080 CPU: 2 COMMAND: "kthreadd"
MM PGD RSS TOTAL_VM
0 0 0k 0k
VMA START END FLAGS FILE
ffff900c94de2488 55e2c48d1000 55e2c4a13000 8000875 /usr/lib/systemd/systemd
I think this is the expected usage of the "-M mm" option.
Thanks,
Kazu
3:03 a.m.
On Fri, Mar 17, 2023 at 10:26 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
On 2023/03/17 11:12, lijiang wrote:
> Hi, Kazu
> Thank you for the comment.
> On Fri, Mar 17, 2023 at 9:02 AM HAGIO KAZUHITO(萩尾 一仁) <
k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> wrote:
>
> Hi Lianbo,
>
> On 2023/03/15 18:07, Lianbo Jiang wrote:
> > The help/man page of the "vm" command suggests that the
"-M"
option
> > accepts the mm_struct address as a valid argument. However, the
"vm
> > -M" option always prints the virtual memory data of the current
> > task, regardless of its arguments.
>
> I think that it's the intended behavior, isn't it?
>
> -M mm if the mm_struct address has been removed from the
task_struct
> of an exiting task, the virtual memory data cannot
be displayed.
> However, if the address can be determined from the
kernel stack,
> it can be entered manually in order to try to
resurrect the
> virtual memory data of the task.
>
> They are not the same issues.
>
> For the live debugging, take the sshd as an example:
>
> Without the patch:
> crash> ps |grep sshd
> 1159 1 3 ffff9915858a9980 IN 0.1 15820 9020
sshd
> 51573 1159 2 ffff991572868000 IN 0.1 18916 11168 sshd
> 51595 51573 2 ffff991575026600 IN 0.0 18784 6892
sshd
> crash> vm -M ffff991572868000
> PID: 51727 TASK: ffff9914465cb300 CPU: 0 COMMAND: "crash"
<------not a correct result
No, I meant that this is correct. Let me explain,
(1) The "-M mm" option accepts an *mm_struct* address.
(2) The specified mm is enabled only when the mm_struct address has been
removedfrom the task_struct, i.e. mm = 0.
Thanks for your explanation, Kazu.
Let's consider the following two cases:
[1] crash> vm pid -M mm
[2] crash> vm -M mm
For the case [1], it is expected usage, and requires that the given
arguments must satisfy the above (1) and (2). The documentation needs to
clarify this to avoid misunderstanding. It might be necessary to update the
man page.
For the case [2], if the usage of "vm -M mm" is not allowed, the "vm -M
mm"
needs to output an error instead of always displaying a bogus result.
But anyway, they are confusing. We can not assume that crash tools will
always get the correct usage or arguments. Any thoughts?
Thanks
Lianbo
So we can emulate that situation e.g. with kthreadd (pid=2) like
this:
crash> vm 1
PID: 1 TASK: ffff900b41599840 CPU: 11 COMMAND: "systemd"
MM PGD RSS TOTAL_VM
ffff9019ff0e3f00 ffff9019ffc10000 12516k 250328k
VMA START END FLAGS FILE
ffff900c94de2488 55e2c48d1000 55e2c4a13000 8000875 /usr/lib/systemd/systemd
...
crash> vm 2
PID: 2 TASK: ffff900b4159b080 CPU: 2 COMMAND: "kthreadd"
MM PGD RSS TOTAL_VM
0 0 0k 0k
^^^
crash> vm 2 -M ffff9019ff0e3f00 <<-- systemd's mm
PID: 2 TASK: ffff900b4159b080 CPU: 2 COMMAND: "kthreadd"
MM PGD RSS TOTAL_VM
0 0 0k 0k
VMA START END FLAGS FILE
ffff900c94de2488 55e2c48d1000 55e2c4a13000 8000875 /usr/lib/systemd/systemd
I think this is the expected usage of the "-M mm" option.
Thanks,
Kazu
7:51 p.m.
Hi Lianbo,
sorry, I've been off for a couple of days.
On 2023/03/20 17:03, lijiang wrote:
> On Fri, Mar 17, 2023 at 10:26 AM HAGIO KAZUHITO(萩尾 一仁)
<k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> wrote:
>
> On 2023/03/17 11:12, lijiang wrote:
> > Hi, Kazu
> > Thank you for the comment.
> > On Fri, Mar 17, 2023 at 9:02 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com
<mailto:k-hagio-ab@nec.com> <mailto:k-hagio-ab@nec.com
<mailto:k-hagio-ab@nec.com>>> wrote:
> >
> > Hi Lianbo,
> >
> > On 2023/03/15 18:07, Lianbo Jiang wrote:
> > > The help/man page of the "vm" command suggests that the
"-M" option
> > > accepts the mm_struct address as a valid argument. However, the
"vm
> > > -M" option always prints the virtual memory data of the
current
> > > task, regardless of its arguments.
> >
> > I think that it's the intended behavior, isn't it?
> >
> > -M mm if the mm_struct address has been removed from the
task_struct
> > of an exiting task, the virtual memory data cannot be
displayed.
> > However, if the address can be determined from the
kernel stack,
> > it can be entered manually in order to try to
resurrect the
> > virtual memory data of the task.
> >
> > They are not the same issues.
> >
> > For the live debugging, take the sshd as an example:
> >
> > Without the patch:
> > crash> ps |grep sshd
> > 1159 1 3 ffff9915858a9980 IN 0.1 15820 9020 sshd
> > 51573 1159 2 ffff991572868000 IN 0.1 18916 11168 sshd
> > 51595 51573 2 ffff991575026600 IN 0.0 18784 6892 sshd
> > crash> vm -M ffff991572868000
> > PID: 51727 TASK: ffff9914465cb300 CPU: 0 COMMAND: "crash"
<------not a correct result
>
> No, I meant that this is correct. Let me explain,
>
> (1) The "-M mm" option accepts an *mm_struct* address.
> (2) The specified mm is enabled only when the mm_struct address has been
> removedfrom the task_struct, i.e. mm = 0.
>
>
> Thanks for your explanation, Kazu.
>
> Let's consider the following two cases:
> [1] crash> vm pid -M mm
> [2] crash> vm -M mm
>
> For the case [1], it is expected usage, and requires that the given arguments must
satisfy the above (1) and (2). The documentation needs to clarify this to avoid
misunderstanding. It might be necessary to update the man page.
>
> For the case [2], if the usage of "vm -M mm" is not allowed, the "vm
-M mm" needs to output an error instead of always displaying a bogus result.
I think that "vm -M mm" is also valid, this merely doesn't have a
specified PID, and chooses the current context:
DESCRIPTION
... If no arguments are entered, the current context is used.
In other words, these two are equal and valid:
crash> vm -M mm 1
and
crash> set 1
crash> vm -M mm
>
> But anyway, they are confusing. We can not assume that crash tools will always get
the correct usage or arguments. Any thoughts?
ok, I agree with you that its usage is a bit confusing.
Looking at the code, the mm specified by "-M mm" is used only when the
task_struct's mm is NULL.
if (!tm->mm_struct_addr) {
if (pc->curcmd_flags & MM_STRUCT_FORCE) {
So how about adding a sentence saying it?
-M mm if the mm_struct address has been removed from the task_struct
of an exiting task, the virtual memory data cannot be displayed.
However, if the address can be determined from the kernel stack,
it can be entered manually in order to try to resurrect the
- virtual memory data of the task.
+ virtual memory data of the task. This option is ignored when
+ the task_struct has a non-NULL mm_struct address.
Thanks,
Kazu
8:20 a.m.
On Wed, Mar 22, 2023 at 8:51 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
Hi Lianbo,
sorry, I've been off for a couple of days.
No worries.
On 2023/03/20 17:03, lijiang wrote:
>> On Fri, Mar 17, 2023 at 10:26 AM HAGIO KAZUHITO(萩尾 一仁) <
k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com>> wrote:
>>
>> On 2023/03/17 11:12, lijiang wrote:
>> > Hi, Kazu
>> > Thank you for the comment.
>> > On Fri, Mar 17, 2023 at 9:02 AM HAGIO KAZUHITO(萩尾 一仁) <
k-hagio-ab(a)nec.com <mailto:k-hagio-ab@nec.com> <mailto:k-hagio-ab@nec.com
<mailto:k-hagio-ab@nec.com>>> wrote:
>> >
>> > Hi Lianbo,
>> >
>> > On 2023/03/15 18:07, Lianbo Jiang wrote:
>> > > The help/man page of the "vm" command suggests that
the
"-M" option
>> > > accepts the mm_struct address as a valid argument.
However, the "vm
>> > > -M" option always prints the virtual memory data of the
current
>> > > task, regardless of its arguments.
>> >
>> > I think that it's the intended behavior, isn't it?
>> >
>> > -M mm if the mm_struct address has been removed
from the task_struct
>> > of an exiting task, the virtual memory data
cannot be displayed.
>> > However, if the address can be determined
from the kernel stack,
>> > it can be entered manually in order to try
to resurrect the
>> > virtual memory data of the task.
>> >
>> > They are not the same issues.
>> >
>> > For the live debugging, take the sshd as an example:
>> >
>> > Without the patch:
>> > crash> ps |grep sshd
>> > 1159 1 3 ffff9915858a9980 IN 0.1 15820
9020 sshd
>> > 51573 1159 2 ffff991572868000 IN 0.1 18916 11168
sshd
>> > 51595 51573 2 ffff991575026600 IN 0.0 18784
6892 sshd
>> > crash> vm -M ffff991572868000
>> > PID: 51727 TASK: ffff9914465cb300 CPU: 0 COMMAND:
"crash"
<------not a correct result
>>
>> No, I meant that this is correct. Let me explain,
>>
>> (1) The "-M mm" option accepts an *mm_struct* address.
>> (2) The specified mm is enabled only when the mm_struct address has
been
>> removedfrom the task_struct, i.e. mm = 0.
>>
>>
>> Thanks for your explanation, Kazu.
>>
>> Let's consider the following two cases:
>> [1] crash> vm pid -M mm
>> [2] crash> vm -M mm
>>
>> For the case [1], it is expected usage, and requires that the given
arguments must satisfy the above (1) and (2). The documentation needs to
clarify this to avoid misunderstanding. It might be necessary to update the
man page.
>>
>> For the case [2], if the usage of "vm -M mm" is not allowed, the
"vm -M
mm" needs to output an error instead of always displaying a bogus result.
I think that "vm -M mm" is also valid, this merely doesn't have a
specified PID, and chooses the current context:
DESCRIPTION
... If no arguments are entered, the current context is used.
In other words, these two are equal and valid:
crash> vm -M mm 1
and
crash> set 1
crash> vm -M mm
>>
>> But anyway, they are confusing. We can not assume that crash tools will
always get the correct usage or arguments. Any thoughts?
ok, I agree with you that its usage is a bit confusing.
Looking at the code, the mm specified by "-M mm" is used only when the
task_struct's mm is NULL.
if (!tm->mm_struct_addr) {
if (pc->curcmd_flags & MM_STRUCT_FORCE) {
It's true.
So how about adding a sentence saying it?
-M mm if the mm_struct address has been removed from the
task_struct
of an exiting task, the virtual memory data cannot be
displayed.
However, if the address can be determined from the kernel
stack,
it can be entered manually in order to try to resurrect
the
- virtual memory data of the task.
+ virtual memory data of the task. This option is ignored
when
+ the task_struct has a non-NULL mm_struct address.
This looks much better. But it still accepts an invalid argument, for
example:
crash> vm -M 0xh
crash> vm -M 0xdeadbeef
How about the following changes?
diff --git a/help.c b/help.c
index 56a9d8274525..3006ac9180ae 100644
--- a/help.c
+++ b/help.c
@@ -4695,6 +4695,7 @@ char *help_vm[] = {
" However, if the address can be determined from the kernel
stack,",
" it can be entered manually in order to try to resurrect
the",
" virtual memory data of the task.",
+" NOTE: this option is only used when the task_struct's mm
is NULL.",
" -R reference search for references to this number or filename.",
" -m dump the mm_struct associated with the task.",
" -v dump all of the vm_area_structs associated with the
task.",
diff --git a/memory.c b/memory.c
index 592a5ef49d50..0968ff930eb9 100644
--- a/memory.c
+++ b/memory.c
@@ -3559,6 +3559,8 @@ cmd_vm(void)
case 'M':
pc->curcmd_private = htoll(optarg, FAULT_ON_ERROR, NULL);
pc->curcmd_flags |= MM_STRUCT_FORCE;
+ if (!IS_KVADDR(pc->curcmd_private))
+ argerrs++;
break;
case 'f':
With the above patch, it can properly deal with an invalid argument as
below:
crash> vm -M 0xh
Usage:
vm [-p | -P vma | -M mm | -v | -m | -x | -d | [-R reference] [pid | task]]
[-f vm_flags]
Enter "help vm" for details.
crash> vm -M 0xdeadbeef
Usage:
vm [-p | -P vma | -M mm | -v | -m | -x | -d | [-R reference] [pid | task]]
[-f vm_flags]
Enter "help vm" for details.
Thanks.
Lianbo
Thanks,
Kazu
7:30 p.m.
On 2023/03/22 22:20, lijiang wrote:
> So how about adding a sentence saying it?
>
-M mm if the mm_struct address has been removed from the
> task_struct
> of an exiting task, the virtual memory data cannot be
> displayed.
> However, if the address can be determined from the kernel
> stack,
> it can be entered manually in order to try to resurrect
> the
> - virtual memory data of the task.
> + virtual memory data of the task. This option is ignored
> when
> + the task_struct has a non-NULL mm_struct address.
>
>
This looks much better. But it still accepts an invalid argument, for
example:
crash> vm -M 0xh
crash> vm -M 0xdeadbeef
How about the following changes?
diff --git a/help.c b/help.c
index 56a9d8274525..3006ac9180ae 100644
--- a/help.c
+++ b/help.c
@@ -4695,6 +4695,7 @@ char *help_vm[] = {
" However, if the address can be determined from the kernel
stack,",
" it can be entered manually in order to try to resurrect
the",
" virtual memory data of the task.",
+" NOTE: this option is only used when the task_struct's mm
is NULL.",
" -R reference search for references to this number or filename.",
" -m dump the mm_struct associated with the task.",
" -v dump all of the vm_area_structs associated with the
task.",
diff --git a/memory.c b/memory.c
index 592a5ef49d50..0968ff930eb9 100644
--- a/memory.c
+++ b/memory.c
@@ -3559,6 +3559,8 @@ cmd_vm(void)
case 'M':
pc->curcmd_private = htoll(optarg, FAULT_ON_ERROR, NULL);
pc->curcmd_flags |= MM_STRUCT_FORCE;
+ if (!IS_KVADDR(pc->curcmd_private))
+ argerrs++;
break;
case 'f':
With the above patch, it can properly deal with an invalid argument as
below:
crash> vm -M 0xh
Usage:
vm [-p | -P vma | -M mm | -v | -m | -x | -d | [-R reference] [pid | task]]
[-f vm_flags]
Enter "help vm" for details.
crash> vm -M 0xdeadbeef
Usage:
vm [-p | -P vma | -M mm | -v | -m | -x | -d | [-R reference] [pid | task]]
[-f vm_flags]
Enter "help vm" for details.
It's fine to check here, but shall we say why it's wrong?
I'd prefer something like this.
if (!IS_KVADDR(pc->curcmd_private))
error(FATAL, "invalid mm_struct address: %s\n", optarg);
Thanks,
Kazu
9:35 p.m.
On Thu, Mar 23, 2023 at 8:30 AM HAGIO KAZUHITO(萩尾 一仁) <k-hagio-ab(a)nec.com>
wrote:
It's fine to check here, but shall we say why it's wrong?
Sure. I will add it to the patch log, and post v2 later.
I'd prefer something like this.
if (!IS_KVADDR(pc->curcmd_private))
error(FATAL, "invalid mm_struct address: %s\n", optarg);
Also fine to me.
Thanks.
Lianbo
466
days inactive
474
days old
devel@lists.crash-utility.osci.io
10 comments
4 participants
participants (4)
-
HAGIO KAZUHITO(萩尾 一仁) -
Lianbo Jiang -
lijiang
-
Philipp Rudo