Attention is currently required from: Tim Wawrzynczak, Paul Menzel, Alex Levin, Julius Werner, Jan Dabros. Jakub Czapiga has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/62474 )
Change subject: util/cbmem: Add FlameGraph-compatible timestamps output ......................................................................
Patch Set 13:
(6 comments)
File src/commonlib/include/commonlib/timestamp_serialized.h:
https://review.coreboot.org/c/coreboot/+/62474/comment/bdd1ab8c_23e195a8 PS7, Line 454: uint32_t id_end;
Hmm... okay, that's an odd case. Is that the only one? Even though the code is written like that I don't think there's actually any board that has an RW VPD but no RO VPD in practice.
That simplifies code. Thanks
I think having them all in one macro has the benefit that when people add new timestamps, they are more explicitly prompted to add a possible end symbol by the structure of the table. If you have the ranges hidden somewhere else in another table, I think it's a lot more likely people will forget to add new entries down there.
Makes perfect sense. If there is no need for defining multiple ends for single start, then it will be easier this way.
Please, take a look at new implementation. Zeros indicate that there is no real end for selected timestamp. I'm wondering, if it would be nicer to define separate macro for entries like that, e.g.: TS_NAME_DEF(id, desc) for entries without end, and TS_NAME_RANGE_DEF(id, id_end, desc) for entries with specified end. What do you think about it?
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62474/comment/86546f20_b21fa156 PS8, Line 629: ::
Oh, I thought this was given by the flame graph tool? Or is this how it will look like in the flame […]
I had some problems with spaces before, but I tested it again and it seems to work correctly. It even is correctly parsed by FlameGraph script. Example: ``` TS_ROMSTAGE_START <-> TS_ROMSTAGE_END 364598 TS_ROMSTAGE_START <-> TS_ROMSTAGE_END;TS_EC_SYNC_START <-> TS_EC_SYNC_END 6548 TS_ROMSTAGE_START <-> TS_ROMSTAGE_END;TS_EC_SYNC_START <-> TS_EC_SYNC_END;TS_EC_HASH_READY 1869 ```
FlameGraph will take this input and will create blocks with e.g.: ``` TS_EC_SYNC_START <-> TS_EC_SYNC_END ``` or ``` TS_EC_HASH_READY ```
FlameChart seems to be pretty good when it comes to parsing input like this. However it does not parse quoted strings, but just includes quote characters in the output, so I think we can use spaces and '<->' for now :)
File util/cbmem/cbmem.c:
https://review.coreboot.org/c/coreboot/+/62474/comment/0c8debf5_2ed11fc9 PS11, Line 630: if (i < stacklvl)
nit: I think if you add ` || last_part` here...
Done
https://review.coreboot.org/c/coreboot/+/62474/comment/82e0a152_31e61389 PS11, Line 634: if (stacklvl > 0)
...you can take this out.
Done
https://review.coreboot.org/c/coreboot/+/62474/comment/e3ed7ab0_94e0530a PS11, Line 727: sorted_tst_p->entries[match].entry_stamp;
nit: I think adding base_time here would make it easier to understand than subtracting it below (bec […]
Done
https://review.coreboot.org/c/coreboot/+/62474/comment/64535b79_d1d41129 PS11, Line 1384: enum timestamps_print_type timestamp_type = TIMESTAMPS_PRINT_NORMAL;
If you have an enum anyway maybe also roll print_timestamps into that (i.e. […]
Done
-
Jakub Czapiga (Code Review)