On Tue, Sep 3, 2024 at 8:15 AM Petr Pavlu <petr.pavlu@xxxxxxxx> wrote: > > On 8/15/24 19:39, Sami Tolvanen wrote: > > Expand each structure type only once per exported symbol. This > > is necessary to support self-referential structures, which would > > otherwise result in infinite recursion, but is still sufficient for > > catching ABI changes. > > > > For pointers to structure types, limit type expansion inside the > > pointer to two levels. This should be plenty for detecting ABI > > differences, but it stops us from pulling in half the kernel for > > types that contain pointers to large kernel data structures, like > > task_struct, for example. > > I'm quite worried about this optimization for pointer types. It could > result in some kABI changes not being recognized. > > I assume the goal of the optimization is to speed up the tool's runtime. > How much does it improve the processing time and is there any other way > how it could be done? It’s mostly a matter of how deep it makes sense to go. For example, queue_delayed_work_on accepts a pointer to s#workqueue_struct, which points to s#worker, which points to s#task_struct, which points to s#mm_struct etc. Does a change to an internal kernel data structure several references deep change the ABI of the function? If we traverse through the DWARF without limits, during a defconfig build the highest pointer expansion depth I see is 70 levels (!), with ~5k symbols going 30+ levels deep. We would end up pulling in a lot of major internal data structures at that point, and a change to any of them would change thousands of symbol versions, which feels undesirable. I'm fine with increasing the limit to something more reasonable though, the impact on performance doesn't seem to be significant in parallel builds. Of course, this might impact vmlinux.o processing more, if we end up doing that, since the DWARF at that point contains information about all the data structures. I do wonder if there's a better way to figure out where to stop than a hard limit. Any thoughts? > > diff --git a/scripts/gendwarfksyms/dwarf.c b/scripts/gendwarfksyms/dwarf.c > > index 92b6ca4c5c91..2f1601015c4e 100644 > > --- a/scripts/gendwarfksyms/dwarf.c > > +++ b/scripts/gendwarfksyms/dwarf.c > > [...] > > @@ -651,6 +742,7 @@ static int process_exported_symbols(struct state *state, struct die *cache, > > else > > check(process_variable(state, &state->die)); > > > > + cache_clear_expanded(&state->expansion_cache); > > return 0; > > default: > > return 0; > > I wonder if it would make sense to share the cache between processing > individual exported symbols. The actual DIE caching happens in die_map, which is already shared between symbols. The expansion cache keeps track of which DIEs we have processed per symbol, so we don't process the same thing twice and end up in a loop, for example. Sami
