Re: [Minios-devel] [UNIKRAFT PATCH 6/8] support/scripts: implement trace buffer parcing

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Costin Lupu <costin.lup@xxxxxxxxx> writes:

> On 5/29/19 4:42 PM, Yuri Volchkov wrote:
>> Hi,
>> 
>> Costin Lupu <costin.lup@xxxxxxxxx> writes:
>> 
>>> Hi Yuri,
>>>
>>> In the subject: s/parcing/parsing .
>> Ack
>> 
>>>
>>> Please see my comments inline.
>>>
>>> On 5/10/19 9:29 PM, Yuri Volchkov wrote:
>>>> With this patch trace.py can print the data collected in the
>>>> tracefile. And two more use cases are covered for convince:
>>>
>>> s/for convince/for convenience/
>> Ack
>> 
>>>
>>>> +
>>>> +class tp_sample:
>>>
>>> It can get confusing on why there are 2 classes for tracepoints,
>>> tracepoint and tp_sample. I think their names should reflect that the
>>> former are the tracepoint definitions and latter are the tracepoint values.
>> Hm, I thought that tracepoint sample is a lot more clear that is a
>> _sample_ of a tracepoint, than "tracepoint value". From merriam-webster
>> dictionary (https://www.merriam-webster.com/dictionary/sample):
>> 
>>     1) a representative part or a single item from a larger whole or
>>        group especially when presented for inspection or shown as
>>        evidence of quality
>>     
>>     2) finite part of a statistical population whose properties are
>>        studied to gain information about the whole
>
> Oh, and I thought that one of the purposes of reviewing was to help the
> author with suggestions in order to make the code even more clearer, you
> know, for other people than the author.
You thought very right. I did not mean to upset you. I really described
my thought process. I really thought (and actually still do) that
'tp_sample' is a better name to a sample of tracepoint then 'value'. If
you are still not happy with the name, I can offer you 2 other names:
'tp_sample_data' 'tp_event_data'. However it makes it a little too
long. What do you think?
>
>> 
>> 
>>>
>>>> +    def __init__(self, tp, time, args):
>>>> +        self.tp = tp
>>>> +        self.args = args
>>>> +        self.time = time
>>>> +    def __str__(self):
>>>> +        return (("%016d %s: " % (self.time, self.tp.name)) +
>>>> +                 (self.tp.fmt % self.args))
>>>> +    def tabulate_fmt(self):
>>>> +        return [self.time, self.tp.name, (self.tp.fmt % self.args)]
>>>> +
>>>> +class EndOfBuffer(Exception):
>>>> +    pass
>>>> +
>>>> +# Parsing of trace buffer is designed to be used without gdb. If ever
>>>> +# Unikraft will have a bare metal port, it would be complicated to use
>>>> +# gdb for fetching and parsing runtime data.
>>>> +#
>>>> +# However, it is possible anyways to get an address of the statically
>>>> +# allocated trace buffer using gdb/nm/objdump. And we can use some
>>>> +# different mechanism to copy memory from the running instance of
>>>> +# Unikraft (which is yet to be designed).
>>>> +#
>>>> +# Similar considerations are applied to parsing trace point
>>>> +# definitions. We can do that disregarding if it is possible to attach
>>>> +# gdb to a running instance or not
>>>> +class sample_parser:
>>>> +    def __init__(self, keyvals, tp_defs_data, trace_buff, ptr_size):
>>>> +        if (int(keyvals['format_version']) > FORMAT_VERSION):
>>>> +            print("Warning: Version of trace format is more recent",
>>>> +                  file=sys.stderr)
>>>> +        self.data = unpacker(trace_buff)
>>>> +        self.tps = get_tp_definitions(tp_defs_data, ptr_size)
>>>> +    def __iter__(self):
>>>> +        self.data.pos = 0
>>>> +        return self
>>>> +    def __next__(self):
>>>> +        try:
>>>> +            # TODO: generate format. Cookie can be 4 bytes long on other
>>>> +            # platforms
>>>> +            magic,size,time,cookie = self.data.unpack('4sLQQ')
>>>> +        except EndOfBuffer:
>>>> +            raise StopIteration
>>>> +
>>>> +        magic = magic.decode()
>>>> +        if (magic != TP_HEADER_MAGIC):
>>>> +            raise StopIteration
>>>> +
>>>> +        tp = self.tps[cookie]
>>>> +        args = []
>>>> +        for i in range(tp.args_nr):
>>>> +            if tp.types[i] == UK_TRACE_ARG_STRING:
>>>> +                args += [self.data.unpack_string()]
>>>> +            else:
>>>> +                args += [self.data.unpack_int(tp.sizes[i])]
>>>> +
>>>> +        return tp_sample(tp, time, tuple(args))
>>>> +
>>>
>>> I think it would have been clearer if the parsing alone would have
>>> stayed in its own module. Or at least delimit the sections in the module
>>> with (block) comments.
>> What do you mean in it's own module? It is in it's own module?
>> Everything in this file is about parsing.
>> 
>
> The changes in 'support/scripts: reorganise tracing scripts'
Now I see what you mean. Great, than it is already fixed :)

>(btw, that title has a type) would clarify that.
Oops. Will correct that.
Off-topic: Funny that word 'typo' has a typo :)

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