[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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



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?

>
> 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.

>
>> +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?

-- 
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

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.