Re: [Crash-utility] [PATCH 2/2] tree: add an option to dump the tree sorted

[Daniel Vacek]

On Thu, Apr 12, 2018 at 4:51 PM, Daniel Vacek  wrote:
> On Thu, Apr 12, 2018 at 3:57 PM, Dave Anderson  wrote:
>>
>> Daniel,
>>
>> Hmmm, when looking at your example, I just noticed it didn't use the -t 
>> argument,
>> but then looking at the code realized that the command defaults to rbtrees.  
>> I
>> actually don't think that was the original intent, but maybe that fact should
>> should be indicated in the help page?  Although I can't come up with a 
>> compelling
>> argument why one should be the default.  Either that, or -t should be 
>> enforced.
>
> Wow. I didn't know this was not intended. I thought that red-black is
> default as radix trees are only used in address_space of mapped files
> (page cache), AFAIK. I usually do not need to specify the type as all
> the major trees are red-black - being it cgroups, scheduler entities,
> memory mappings, inodes, you name it.
>
> Again, I didn't change anything with regards to this behavior.
>
>> Anyway, you probably didn't notice that all help page output is constrained
>> to 80 columns.  Your example command alone is 162 bytes long, making it way
>> too confusing to figure out what the command is doing.  Can you just show
>> the command output without piping it to an external command, or make a much
>> simpler example?  And also keep it to 80 columns or less,
>
> I was thinking about wrapping the command to a second line but that's
> what terminal will do anyways if narrower so I left it that way. The
> rest of the pipe is just formatting/pretty printing the table.
> Otherwise the output would be wy t lng and not really
> human readable. I do not think it can get any simpler than that. I can
> get rid of one of the columns though, it does not need to contain both
> the start and the end of the VMA range.

Or even get rid of the position? Something as simple as this:

crash> tree -ls vm_area_struct.vm_start -o vm_area_struct.vm_rb -r
mm_struct.mm_rb 0x880074b5be80 | paste - -
88001f2c50e0  vm_start = 0x40
88001f2c5290  vm_start = 0xceb000
880074bfc6c0  vm_start = 0xcec000
88001f2c4bd0  vm_start = 0xd1
880074bfc948  vm_start = 0x1fe9000
880036e54510  vm_start = 0x7ff6aa296000
88001f2c5bd8  vm_start = 0x7ff6aa298000
880036e54af8  vm_start = 0x7ff6aa497000
880036e54f30  vm_start = 0x7ff6aa498000
88000e06aa20  vm_start = 0x7ff6aa499000
88000e06b368  vm_start = 0x7ff6ab95f000
...
88001f2c5e60  vm_start = 0x7ff6bc1af000
88001f2c4ca8  vm_start = 0x7ff6bc1b6000
88001f2c5008  vm_start = 0x7ff6bc20
88001f2c5d88  vm_start = 0x7ff6bc205000
880074bfd6c8  vm_start = 0x7ff6bc206000
88001f2c4288  vm_start = 0x7ff6bc207000
88001f2c4510  vm_start = 0x7ffc7a5fc000
88001f2c5b00  vm_start = 0x7ffc7a6d1000

vs.

crash> tree -s vm_area_struct.vm_start -o vm_area_struct.vm_rb -r
mm_struct.mm_rb 0x880074b5be80 | paste - -
88001f2c5a28  vm_start = 0x7ff69000
88001f2c55f0  vm_start = 0x7ff6bb252000
88000e06a360  vm_start = 0x7ff6ac6c3000
88001f2c4bd0  vm_start = 0xd1
88001f2c5290  vm_start = 0xceb000
88001f2c50e0  vm_start = 0x40
880074bfc6c0  vm_start = 0xcec000
88000e06b368  vm_start = 0x7ff6ab95f000
88001f2c5bd8  vm_start = 0x7ff6aa298000
880074bfc948  vm_start = 0x1fe9000
880036e54510  vm_start = 0x7ff6aa296000
880036e54f30  vm_start = 0x7ff6aa498000
880036e54af8  vm_start = 0x7ff6aa497000
88000e06aa20  vm_start = 0x7ff6aa499000
88000e06ae58  vm_start = 0x7ff6ac1df000
88000e06ba28  vm_start = 0x7ff6abefc000
88000e06a6c0  vm_start = 0x7ff6ac41b000
88001f2c4000  vm_start = 0x7ff6bac75000
88000e06bd88  vm_start = 0x7ff6b2d0
88000e06b440  vm_start = 0x7ff6b28de000
...
880074bfd6c8  vm_start = 0x7ff6bc206000
88001f2c4510  vm_start = 0x7ffc7a5fc000
88001f2c5b00  vm_start = 0x7ffc7a6d1000

That would work, right? Though even with the position it may fit to
80... Still I do not see the point.

--nX

> As an added value it also teaches eventual readers some advanced usage
> techniques in general so I believe it can be useful. There is enough
> simple examples already. Would you rather prefer something like this?
>
> crash> task -R mm
> PID: 28682  TASK: 880036d4af10  CPU: 0   COMMAND: "crash"
>   mm = 0x880074b5be80,
>
> crash> tree -lp -o vm_area_struct.vm_rb -r mm_struct.mm_rb 0x880074b5be80
> 88001f2c50e0
>   position: root/l/l/l/l/l
> 88001f2c5290
>   position: root/l/l/l/l
> 880074bfc6c0
>   position: root/l/l/l/l/r
> 88001f2c4bd0
>   position: root/l/l/l
> 880074bfc948
>   position: root/l/l/l/r/l/l
> 880036e54510
>   position: root/l/l/l/r/l/l/r
> 88001f2c5bd8
>   position: root/l/l/l/r/l
> 880036e54af8
>   position: 

Re: [Crash-utility] [PATCH 2/2] tree: add an option to dump the tree sorted

[Dave Anderson]

- Original Message -
> On Thu, Apr 12, 2018 at 3:57 PM, Dave Anderson  wrote:
> >
> > Daniel,
> >
> > Hmmm, when looking at your example, I just noticed it didn't use the -t
> > argument,
> > but then looking at the code realized that the command defaults to rbtrees.
> > I
> > actually don't think that was the original intent, but maybe that fact
> > should
> > should be indicated in the help page?  Although I can't come up with a
> > compelling
> > argument why one should be the default.  Either that, or -t should be
> > enforced.
> 
> Wow. I didn't know this was not intended. I thought that red-black is
> default as radix trees are only used in address_space of mapped files
> (page cache), AFAIK. I usually do not need to specify the type as all
> the major trees are red-black - being it cgroups, scheduler entities,
> memory mappings, inodes, you name it.
> 
> Again, I didn't change anything with regards to this behavior.

OK, that's a good point.  So let's declare the default at top of the help
page "-t type" segment.

> 
> > Anyway, you probably didn't notice that all help page output is constrained
> > to 80 columns.  Your example command alone is 162 bytes long, making it way
> > too confusing to figure out what the command is doing.  Can you just show
> > the command output without piping it to an external command, or make a much
> > simpler example?  And also keep it to 80 columns or less,
> 
> I was thinking about wrapping the command to a second line but that's
> what terminal will do anyways if narrower so I left it that way. The
> rest of the pipe is just formatting/pretty printing the table.
> Otherwise the output would be wy t lng and not really
> human readable. I do not think it can get any simpler than that. I can
> get rid of one of the columns though, it does not need to contain both
> the start and the end of the VMA range.
> 
> As an added value it also teaches eventual readers some advanced usage
> techniques in general so I believe it can be useful. There is enough
> simple examples already. Would you rather prefer something like this?
>
> crash> task -R mm
> PID: 28682  TASK: 880036d4af10  CPU: 0   COMMAND: "crash" 
> mm = 0x880074b5be80,
> 
> crash> tree -lp -o vm_area_struct.vm_rb -r mm_struct.mm_rb 0x880074b5be80
> 88001f2c50e0
>   position: root/l/l/l/l/l
> 88001f2c5290
>   position: root/l/l/l/l
> 880074bfc6c0
>   position: root/l/l/l/l/r
> 88001f2c4bd0
>   position: root/l/l/l
> 880074bfc948
>   position: root/l/l/l/r/l/l
> 880036e54510
>   position: root/l/l/l/r/l/l/r
> 88001f2c5bd8
>   position: root/l/l/l/r/l
> 880036e54af8
>   position: root/l/l/l/r/l/r/l
> 880036e54f30
>   position: root/l/l/l/r/l/r
> 88000e06aa20
>   position: root/l/l/l/r/l/r/r
> 88000e06b368
>   position: root/l/l/l/r
> 88000e06ba28
>   position: root/l/l/l/r/r/l
> 88000e06ae58
>   position: root/l/l/l/r/r
> 88000e06a6c0
>   position: root/l/l/l/r/r/r
> ...
> 88001f2c51b8
>  position: root/l/r/r/r/l
> 88001f2c4d80
>   position: root/l/r/r/r
> 880074bfd878
>   position: root/l/r/r/r/r
> 88001f2c5a28
>   position: root
> 88001f2c4a20
>   position: root/r/l/l/l
> 88001f2c4360
>   position: root/r/l/l
> 880074bfcaf8
>   position: root/r/l/l/r
> ...
> 88001f2c5e60
>   position: root/r/r/l/l/l
> 88001f2c4ca8
>   position: root/r/r/l/l
> 88001f2c5008
>   position: root/r/r/l
> 88001f2c5d88
>   position: root/r/r/l/r
> 880074bfd6c8
>   position: root/r/r/l/r/r
> 88001f2c4288
>   position: root/r/r
> 88001f2c4510
>   position: root/r/r/r
> 88001f2c5b00
>   position: root/r/r/r/r
> 
> This is not as readable in my opinion. And also it does not show why
> would one care about the order in the first place. See?

Yes, that's what I'm looking for.  You can also remove the "task" command,
and just say something in the leading description like, "Given an mm_struct
address of 0x880074b5be80, ..." 

As far as caring about the order, you could add the "-s vm_area_struct.vm_start"
option, and indicate that the -l option would display the vm_start addresses 
in an ascending value.  That would obviously overflow the command line, but you
could use a numeric argument for the -o argument as is done in a few other
examples above that.

> I do not believe a hard limit of 80 columns is useful for any serious
> kernel debugging. For developing and maintaining a source code, yes.
> For dumping a lot of debugging data in a human readable form, no.

I don't care about the length of any command output, I'm just talking about
the output examples in the help page data.

Thanks,
  Dave


--
Crash-utility mailing list
Crash-utility@redhat.com
https://www.redhat.com/mailman/listinfo/crash-utility

Re: [Crash-utility] [PATCH 2/2] tree: add an option to dump the tree sorted

[Daniel Vacek]

On Thu, Apr 12, 2018 at 3:57 PM, Dave Anderson  wrote:
>
> Daniel,
>
> Hmmm, when looking at your example, I just noticed it didn't use the -t 
> argument,
> but then looking at the code realized that the command defaults to rbtrees.  I
> actually don't think that was the original intent, but maybe that fact should
> should be indicated in the help page?  Although I can't come up with a 
> compelling
> argument why one should be the default.  Either that, or -t should be 
> enforced.

Wow. I didn't know this was not intended. I thought that red-black is
default as radix trees are only used in address_space of mapped files
(page cache), AFAIK. I usually do not need to specify the type as all
the major trees are red-black - being it cgroups, scheduler entities,
memory mappings, inodes, you name it.

Again, I didn't change anything with regards to this behavior.

> Anyway, you probably didn't notice that all help page output is constrained
> to 80 columns.  Your example command alone is 162 bytes long, making it way
> too confusing to figure out what the command is doing.  Can you just show
> the command output without piping it to an external command, or make a much
> simpler example?  And also keep it to 80 columns or less,

I was thinking about wrapping the command to a second line but that's
what terminal will do anyways if narrower so I left it that way. The
rest of the pipe is just formatting/pretty printing the table.
Otherwise the output would be wy t lng and not really
human readable. I do not think it can get any simpler than that. I can
get rid of one of the columns though, it does not need to contain both
the start and the end of the VMA range.

As an added value it also teaches eventual readers some advanced usage
techniques in general so I believe it can be useful. There is enough
simple examples already. Would you rather prefer something like this?

crash> task -R mm
PID: 28682  TASK: 880036d4af10  CPU: 0   COMMAND: "crash"
  mm = 0x880074b5be80,

crash> tree -lp -o vm_area_struct.vm_rb -r mm_struct.mm_rb 0x880074b5be80
88001f2c50e0
  position: root/l/l/l/l/l
88001f2c5290
  position: root/l/l/l/l
880074bfc6c0
  position: root/l/l/l/l/r
88001f2c4bd0
  position: root/l/l/l
880074bfc948
  position: root/l/l/l/r/l/l
880036e54510
  position: root/l/l/l/r/l/l/r
88001f2c5bd8
  position: root/l/l/l/r/l
880036e54af8
  position: root/l/l/l/r/l/r/l
880036e54f30
  position: root/l/l/l/r/l/r
88000e06aa20
  position: root/l/l/l/r/l/r/r
88000e06b368
  position: root/l/l/l/r
88000e06ba28
  position: root/l/l/l/r/r/l
88000e06ae58
  position: root/l/l/l/r/r
88000e06a6c0
  position: root/l/l/l/r/r/r
...
88001f2c51b8
 position: root/l/r/r/r/l
88001f2c4d80
  position: root/l/r/r/r
880074bfd878
  position: root/l/r/r/r/r
88001f2c5a28
  position: root
88001f2c4a20
  position: root/r/l/l/l
88001f2c4360
  position: root/r/l/l
880074bfcaf8
  position: root/r/l/l/r
...
88001f2c5e60
  position: root/r/r/l/l/l
88001f2c4ca8
  position: root/r/r/l/l
88001f2c5008
  position: root/r/r/l
88001f2c5d88
  position: root/r/r/l/r
880074bfd6c8
  position: root/r/r/l/r/r
88001f2c4288
  position: root/r/r
88001f2c4510
  position: root/r/r/r
88001f2c5b00
  position: root/r/r/r/r

This is not as readable in my opinion. And also it does not show why
would one care about the order in the first place. See?

I do not believe a hard limit of 80 columns is useful for any serious
kernel debugging. For developing and maintaining a source code, yes.
For dumping a lot of debugging data in a human readable form, no.

For example if I type 'timer -r' I already get an output which is over
eighty. How would you copy/paste this to help? Or how do you feel
about the output itself?

  CLOCK: 1  HRTIMER_CLOCK_BASE: 8805ed82d9a0  [ktime_get_real]
CURRENT
  1522436617507394900
  SOFTEXPIRESEXPIRESHRTIMER   FUNCTION
  1522436624101007000  1522436624101057000  8805e08ebd60
810a95c0  
  1522436628203647900  1522436628203697900  8805e33fbd60
810a95c0  
  1522436635368413000  1522436635368463000  8805e07cfd60
810a95c0  
  1522436652248574300  1522436652248624300  8805e1acfd60
810a95c0  
  1522436652257417000  1522436652257467000  8805e07a3d60
810a95c0  
  1522436664228364000  1522436664228414000  8805e1347d60
810a95c0  

That's about the same width as my example.
What do you suggest?

> and put the "l" argument in the synopsis line at the top.

Nah. I missed that one. Will fix it. Or you can just amend it yourself.

--nX

> Thanks,
>   Dave
>
>
>
> - Original Message -
>> ---
>>  defs.h  |  1 +
>>  help.c  | 80
>>  +
>>  tools.c | 16 +++--
>>  3 files changed, 95 insertions(+), 2 deletions(-)
>>
>> diff 

Re: [Crash-utility] [PATCH 2/2] tree: add an option to dump the tree sorted

[Dave Anderson]

Daniel,

Hmmm, when looking at your example, I just noticed it didn't use the -t 
argument,
but then looking at the code realized that the command defaults to rbtrees.  I 
actually don't think that was the original intent, but maybe that fact should
should be indicated in the help page?  Although I can't come up with a 
compelling
argument why one should be the default.  Either that, or -t should be enforced.

Anyway, you probably didn't notice that all help page output is constrained
to 80 columns.  Your example command alone is 162 bytes long, making it way
too confusing to figure out what the command is doing.  Can you just show
the command output without piping it to an external command, or make a much
simpler example?  And also keep it to 80 columns or less, and put the "l"
argument in the synopsis line at the top.

Thanks,
  Dave



- Original Message -
> ---
>  defs.h  |  1 +
>  help.c  | 80
>  +
>  tools.c | 16 +++--
>  3 files changed, 95 insertions(+), 2 deletions(-)
> 
> diff --git a/defs.h b/defs.h
> index adddb9f2748d..ec298cbd70be 100644
> --- a/defs.h
> +++ b/defs.h
> @@ -2480,6 +2480,7 @@ struct tree_data {
>  #define TREE_STRUCT_RADIX_16  (VERBOSE << 6)
>  #define TREE_PARSE_MEMBER (VERBOSE << 7)
>  #define TREE_READ_MEMBER  (VERBOSE << 8)
> +#define TREE_LINEAR_ORDER (VERBOSE << 9)
>  
>  #define ALIAS_RUNTIME  (1)
>  #define ALIAS_RCLOCAL  (2)
> diff --git a/help.c b/help.c
> index c5cec5365962..e7df50fa2ef4 100644
> --- a/help.c
> +++ b/help.c
> @@ -5698,6 +5698,8 @@ char *help_tree[] = {
>  " indicates \"root/l/r\" means that the node is the right
>  child",
>  " of the left child of the root node.  For radix trees, the
>  height",
>  " and slot index values are shown with respect to the root.",
> +" -l  Dump the tree sorted in linear order starting with the
> leftmost",
> +" node and progressing to the right.",
>  " ",
>  "  The meaning of the \"start\" argument, which can be expressed either in",
>  "  hexadecimal format or symbolically, depends upon whether the -N option",
> @@ -5823,6 +5825,84 @@ char *help_tree[] = {
>  "ea000407de58",
>  "  position: root/3/28",
>  "",
> +"  List the tree in linear order from the leftmost node progressing",
> +"  to the right using the -l option:\n",
> +"%s> tree -lps vm_area_struct.vm_start,vm_end -o vm_area_struct.vm_rb -r
> mm_struct.mm_rb 0x8805dee5e400 | sed 's/^  //' | paste - - - - | column
> -ts'  '",
> +"8805e108f008  position: root/l/l/l/l/l/l/l/l/l  vm_start =
> 0x7f6f9ca5f000  vm_end = 0x7f6f9d44c000",
> +"880540d35d88  position: root/l/l/l/l/l/l/l/lvm_start =
> 0x7f6f9d44c000  vm_end = 0x7f6f9d454000",
> +"8805def64d80  position: root/l/l/l/l/l/l/l  vm_start =
> 0x7f6f9d454000  vm_end = 0x7f6f9d653000",
> +"8805e0b46510  position: root/l/l/l/l/l/l/l/r/l  vm_start =
> 0x7f6f9d653000  vm_end = 0x7f6f9d654000",
> +"8805e108e288  position: root/l/l/l/l/l/l/l/rvm_start =
> 0x7f6f9d654000  vm_end = 0x7f6f9d655000",
> +"8805def65bd8  position: root/l/l/l/l/l/l/l/r/r  vm_start =
> 0x7f6f9d655000  vm_end = 0x7f6f9d661000",
> +"8805def64ca8  position: root/l/l/l/l/l/lvm_start =
> 0x7f6f9d661000  vm_end = 0x7f6f9d86",
> +"8805def641b0  position: root/l/l/l/l/l/l/r/lvm_start =
> 0x7f6f9d86  vm_end = 0x7f6f9d861000",
> +"8805def64438  position: root/l/l/l/l/l/l/r/l/r  vm_start =
> 0x7f6f9d861000  vm_end = 0x7f6f9d862000",
> +"8805def65368  position: root/l/l/l/l/l/l/r  vm_start =
> 0x7f6f9d862000  vm_end = 0x7f6f9d868000",
> +"880540ec2e58  position: root/l/l/l/l/l/l/r/rvm_start =
> 0x7f6f9d868000  vm_end = 0x7f6f9d88c000",
> +"...",
> +"8805dfc08e58  position: root/l/r/r/l/r/lvm_start =
> 0x7f6fa020f000  vm_end = 0x7f6fa0216000",
> +"880540ec35f0  position: root/l/r/r/l/r  vm_start =
> 0x7f6fa0216000  vm_end = 0x7f6fa0217000",
> +"8805dfc09cb0  position: root/l/r/r/l/r/rvm_start =
> 0x7f6fa0217000  vm_end = 0x7f6fa0338000",
> +"8805dfc08360  position: root/l/r/r  vm_start =
> 0x7f6fa0338000  vm_end = 0x7f6fa0538000",
> +"8805dfc08bd0  position: rootvm_start =
> 0x7f6fa07af000  vm_end = 0x7f6fa09ae000",
> +"8805dfc08288  position: root/r/l/l/l/l/lvm_start =
> 0x7f6fa09ae000  vm_end = 0x7f6fa09b2000",
> +"880540ec31b8  position: root/r/l/l/l/l/l/r  vm_start =
> 0x7f6fa09b2000  vm_end = 0x7f6fa09b3000",
> +"8805dfc08ca8  position: root/r/l/l/l/l  vm_start =
> 0x7f6fa09b3000  vm_end = 0x7f6fa09b4000",
> +"8800f78c15f0  position: root/r/l/l/l/l/rvm_start =
> 0x7f6fa09b4000  vm_end = 0x7f6fa0b6c000",
> +"...",
> +"8805def651b8  position: root/r/r/r/r/l/lvm_start =
>