Re: [Minios-devel] [UNIKRAFT PATCH 5/8] support/scripts: fetch trace data from running unikraft

[Costin Lupu <costin.lup@xxxxxxxxx>]

On 5/29/19 4:28 PM, Yuri Volchkov wrote:
> Hi,
> 
> Costin Lupu <costin.lup@xxxxxxxxx> writes:
> 
>> Hi Yuri,
>>
>> As a general comment, I don't get this split: putting scripts in
>> support/scripts/ and support/scripts/unikraft/. What's the rationale
>> behind it?
>>
>> Why aren't they all in support/scripts/unikraft/ ? Also,
>> having two scripts with the same name, trace.py, is misleading.
> The logic here was the following: The support/scripts/trace.py is a
> tool. That is the only thing user needs to parse the buffer. He does not
> need to know the second trace.py even exists.
> 
> The support/scripts/unikraft/trace.py is a module. It contains the
> shared functionality used from the parsing tool and from gdb. You never
> run it directly.
>>
>> Moreover, I see that in another series (patch 'support/scripts:
>> reorganise tracing scripts') you rename these scripts, why didn't it
>> happen in the current patch series?
> Because I came up with this idea way later. And that series was mostly
> RFC. While this one is stable (I thought so).
> 
> Would you mind to keep them separate? Merging might be potentially very
> time consuming. In normal conditions I would not object to merging, but
> as you know we are quite short on time. If that is not critical, could
> we make an exception and go with a follow-up?
> 

Even if you don't have time for merging them, that 3rd patch should come
right after these 2 script patches. It makes more sense using that
structure.

>>
>> Please see my other comments inline.
>>
>> On 5/10/19 9:29 PM, Yuri Volchkov wrote:
>>> Fetch tracebuffer, key-values and tracepoint definitions using objcopy
>>> and gdb.
>>>
>>> This patch introduces uk-gdb.py script, which is a gdb-extension
>>> providing handy helpers. Currently it is only tracepoints related
>>> staff, but possibilities are going beyond that.
>>
>> s/staff/stuff
> Noted
> 
>>
>>>
>>> The trace.py is a tool for analyzing traces. Currently it does only
>>> the basic staff: fetches tracing data from a running instance (can run
>>
>> s/staff/stuff
> Noted
>>
>>> gdb for you) and prints out parsed information (in the next
>>> patch). But in potentially it will be able to do more. For example
>>
>> "But it will potentially/eventually be able..."
> Noted
>>
>>> statistics of most frequently occurred events.
>>>
>>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>>> +
>>> +import click
>>
>> For the record, I'd prefer using argparse instead, it avoids adding
>> extra dependencies. The series introducing installation support make
>> things even worse, I prefer simplicity. But I know you really want this
>> so I'm not going to insist on this one.
> Ok then, I then. I just describe my position here for the record too :)
> 
> In my experience, argparse-based code gets messy very
> quickly. Especially for the git-style cli. The trace.py is a developer's
> tool. And if you are developer the chances are that you already using
> git-pw, which is using click as well.
> 
> Anyways, it is not a KDE dependency, which forces you to install a
> gigabyte or two of packages.
>>
>>> +def save_traces(out):
>>> +    elf = gdb.current_progspace().filename
>>> +
>>> +    pickler = pickle.Pickler(out)
>>> +
>>> +    # keyvals need to go first, because they have format_version
>>> +    # key. Even if the format is changed we must guarantee that at
>>> +    # least keyvals are always stored first. However, ideally, next
>>> +    # versions should just have modifications at the very end to keep
>>> +    # compatibility with previously collected data.
>>> +    pickler.dump(trace.get_keyvals(elf))
>>> +    pickler.dump(elf)
>>> +    pickler.dump(PTR_SIZE)
>>> +    # We are saving raw trace buffer here. Another option is to pickle
>>> +    # already parsed samples. But in the chosen case it is a lot
>>> +    # easier to debug the parser, because python in gdb is not very
>>> +    # convenient for development.
>>> +    pickler.dump(trace.get_tp_sections(elf))
>>> +    pickler.dump(get_trace_buffer())
>>> +
>>
>> I think this is not the best design. The serialization for traces is
>> here, while the deserialization is in support/scripts/trace.py . It's
>> very easy to make a mistake when adding more tracing information. I'd
>> prefer the serialization and deserialization to stay together.
> This is two parts of the protocol. Think client-server approach. You do
> not keep xenbus server and client in the same files. Same here. The
> uk_gdb.py contains only the code needed to be run from gdb-side. The
> support/scripts/trace.py is a reader of what is generated.
> 
> More then that, I do not like to put serialization code into
> support/scripts/trace.py, where it will never be used by design.
> 

Serialization/deserialization are operations performed on data, it has
to do with data representation, not with the protocol. Separating data
from protocol is an old trick in software engineering. And that's the
last thing I'm going to say about it, you can do whatever you like with it.

>>
>>> +class uk(gdb.Command):
>>> +    def __init__(self):
>>> +        gdb.Command.__init__(self, 'uk',
>>> +                             gdb.COMMAND_USER, gdb.COMPLETE_COMMAND, True)
>>> +
>>> +class uk_trace(gdb.Command):
>>> +    def __init__(self):
>>> +        gdb.Command.__init__(self, 'uk trace', gdb.COMMAND_USER,
>>> +                             gdb.COMPLETE_COMMAND, True)
>>> +    def invoke(self, arg, from_tty):
>>> +        # TODO
>>> +        pass
>>> +
>>> +class uk_trace_save(gdb.Command):
>>> +    def __init__(self):
>>> +        gdb.Command.__init__(self, 'uk trace save', gdb.COMMAND_USER,
>>> +                             gdb.COMPLETE_COMMAND)
>>
>> If there will be a new version, I prefer these initializers (I mean
>> __init__ calls) to have the same style (e.g. like the first one). It
>> improves the readability and it eases my OCD.
> Could you be more specific on this please?
> 

You have 3 __init__ calls here. Making their splitting consistent
improves readability. E.g. first line of __init__ should contain only
the command string for all 3 of them, just like in the 1st case.


Costin

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel