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

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Costin Lupu <costin.lup@xxxxxxxxx> writes:

> 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.
Ok, v2 will have include all 3 patches from that series.

>
>>>
>>> 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.
Sorry I am not sure I understood. But I guess I am not getting any
follow up on this.

>
>>>
>>>> +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.
Ah, you are talking about newlines.. For me it does not make any
difference :). And since I don't care, I will do as you ask.
>
>
> Costin

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

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