Attention is currently required from: Tarun Tuli, Martin L Roth, Jakub Czapiga, Tim Wawrzynczak, Reka Norman.
Jan Dabros has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/65606 )
Change subject: profiler: Add basic profiler with cbmem support ......................................................................
Patch Set 10:
(15 comments)
File Makefile.inc:
https://review.coreboot.org/c/coreboot/+/65606/comment/6f97e96e_ece6ad24 PS10, Line 515: CFLAGS_common += -Os For CONFIG_PROFILER-enabled build we will get both -Os & -Og. Are we OK with this?
File src/arch/arm64/armv8/exception.c:
https://review.coreboot.org/c/coreboot/+/65606/comment/eb58b671_87474337 PS10, Line 160: profiler_handle_exit_scope(); `profiler_handle_exit/entry_scope` pair functionality is not clear at first glance (although you did a great job with comments in `/src/commonlib/bsd/include/commonlib/bsd/profiler.h`. What about some more descriptive naming, e.g. `non-profiled-section_entry()` and `non-profiled-section-exit()`?
File src/arch/arm64/transition.c:
https://review.coreboot.org/c/coreboot/+/65606/comment/b25d4e14_f96f45ed PS10, Line 22: profiler_handle_entry_scope(); Why we cannot profile exception handlers? Add a comment in commit msg.
File src/commonlib/bsd/include/commonlib/bsd/profiler.h:
https://review.coreboot.org/c/coreboot/+/65606/comment/53ae9c85_e4818ef9 PS10, Line 8: uint32_t function_addr; /*< Address of function related to entry */ Don't we need 64bits for address? This will increase memory footprint, but wonder if there are cases where coreboot code is placed higher in memory map.
https://review.coreboot.org/c/coreboot/+/65606/comment/8cc7a370_3ff4740f PS10, Line 16: uint32_t entries_num; Should we introduce "version" field in case of some format changes in the future? In this case footprint is small, but we can avoid some problems. Actually with version field being there, we may keep `function_addr` as 32bit for now and modify (and bump version) if needed.
https://review.coreboot.org/c/coreboot/+/65606/comment/c4c2c45c_82848e12 PS10, Line 34: __attribute__((no_instrument_function)) void profiler_handle_entry_scope(void); I wrote a comment about naming above.
File src/lib/profiler.c:
https://review.coreboot.org/c/coreboot/+/65606/comment/895b775a_bce62a98 PS10, Line 26: = {0}; Not necessary.
https://review.coreboot.org/c/coreboot/+/65606/comment/e99a8323_056b8151 PS10, Line 37: printk, vprintk, vtxprintf, die, do_putchar Can't we use `-finstrument-functions-exclude-function-list` to handle these at compilation level? If I got the logic correct, we just don't want to profile functions from `exclude_scope[]`, right?
https://review.coreboot.org/c/coreboot/+/65606/comment/e46df929_af47e094 PS10, Line 37: profiler_mock_exclude_symbol See my comment about using `profiler_enabled` below. If this is the case, we can completely get rid of `exclude_scope[]` table.
https://review.coreboot.org/c/coreboot/+/65606/comment/501bda80_95cb96c0 PS10, Line 37: profiler_mock_exclude_symbol Even if `profiler_enabled` cannot be used, we should rewrite the algorithm to simply check for value of `profiler_mock_exclude_symbol`.
https://review.coreboot.org/c/coreboot/+/65606/comment/5ec5c93e_a6ff550e PS10, Line 59: const uint32_t fn_addr = (uint32_t)(this_fn - (void *)_text); : : struct profiler_entry *const current = : &profiler_header->entries[profiler_header->entries_num] Put variables definition at the top of function.
https://review.coreboot.org/c/coreboot/+/65606/comment/6907926b_50d22123 PS10, Line 86: profiler_mock_exclude_symbol Can't we just use `profiler_enabled` variable?
https://review.coreboot.org/c/coreboot/+/65606/comment/ec8174d8_dad58c85 PS10, Line 101: return !initial_lapicid(); I propose to add is_main_cpu() in per-arch C files. This way, we can get rid of #ifdefs.
https://review.coreboot.org/c/coreboot/+/65606/comment/3b264e6d_61001cf3 PS10, Line 114: atomic_read(&profiler_enabled) Let's split this if and first check for `if(atomic_read..)` and get out immediately if profiler is disabled.
https://review.coreboot.org/c/coreboot/+/65606/comment/61d523c0_b94d48bf PS10, Line 115: handle_scope(this_fn, 1); I have many comments requiring your answer, so probably it will be easier for me to add this suggestion in v2/after your reply - but if we can simplify algorithm, we may get rid of `handle_scope()` completely and just check if `this_fn` is within text secion here.