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

[Daniel Vacek]

On Thu, Apr 12, 2018 at 5:11 PM, Dave Anderson <anderson at redhat.com> wrote:
>
> ----- Original Message -----
>> On Thu, Apr 12, 2018 at 3:57 PM, Dave Anderson <anderson at redhat.com> 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 waaaay toooo loooong 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: ffff880036d4af10  CPU: 0   COMMAND: "crash"
>> mm = 0xffff880074b5be80,
>>
>> crash> tree -lp -o vm_area_struct.vm_rb -r mm_struct.mm_rb 0xffff880074b5be80
>> ffff88001f2c50e0
>>   position: root/l/l/l/l/l
>> ffff88001f2c5290
>>   position: root/l/l/l/l
>> ffff880074bfc6c0
>>   position: root/l/l/l/l/r
>> ffff88001f2c4bd0
>>   position: root/l/l/l
>> ffff880074bfc948
>>   position: root/l/l/l/r/l/l
>> ffff880036e54510
>>   position: root/l/l/l/r/l/l/r
>> ffff88001f2c5bd8
>>   position: root/l/l/l/r/l
>> ffff880036e54af8
>>   position: root/l/l/l/r/l/r/l
>> ffff880036e54f30
>>   position: root/l/l/l/r/l/r
>> ffff88000e06aa20
>>   position: root/l/l/l/r/l/r/r
>> ffff88000e06b368
>>   position: root/l/l/l/r
>> ffff88000e06ba28
>>   position: root/l/l/l/r/r/l
>> ffff88000e06ae58
>>   position: root/l/l/l/r/r
>> ffff88000e06a6c0
>>   position: root/l/l/l/r/r/r
>> ...
>> ffff88001f2c51b8
>>  position: root/l/r/r/r/l
>> ffff88001f2c4d80
>>   position: root/l/r/r/r
>> ffff880074bfd878
>>   position: root/l/r/r/r/r
>> ffff88001f2c5a28
>>   position: root
>> ffff88001f2c4a20
>>   position: root/r/l/l/l
>> ffff88001f2c4360
>>   position: root/r/l/l
>> ffff880074bfcaf8
>>   position: root/r/l/l/r
>> ...
>> ffff88001f2c5e60
>>   position: root/r/r/l/l/l
>> ffff88001f2c4ca8
>>   position: root/r/r/l/l
>> ffff88001f2c5008
>>   position: root/r/r/l
>> ffff88001f2c5d88
>>   position: root/r/r/l/r
>> ffff880074bfd6c8
>>   position: root/r/r/l/r/r
>> ffff88001f2c4288
>>   position: root/r/r
>> ffff88001f2c4510
>>   position: root/r/r/r
>> ffff88001f2c5b00
>>   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 0xffff880074b5be80, ..."
> 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.

I see. I'm gonna use the example from my last email. That's simple
enough and clean.

Will send v2 tomorrow.

--nX

>
> Thanks,
>   Dave
>
>