Re: [PATCH v5 00/17] Reference count checker and related fixes

[Adrian Hunter]

On 4/04/23 21:54, Arnaldo Carvalho de Melo wrote:
> Em Tue, Apr 04, 2023 at 03:41:38PM -0300, Arnaldo Carvalho de Melo escreveu:
>> Em Tue, Apr 04, 2023 at 08:25:41PM +0300, Adrian Hunter escreveu:
>>> On 4/04/23 18:58, Ian Rogers wrote:
>>>> Ping. It would be nice to have this landed or at least the first 10
>>>> patches that refactor the map API and are the bulk of the
>>>> lines-of-code changed. Having those landed would make it easier to
>>>> rebase in the future, but I also think the whole series is ready to
>>>> go.
>>>
>>> I was wondering if the handling of dynamic data like struct map makes
>>> any sense at present.  Perhaps someone can reassure me.
>>>
>>> A struct map can be updated when an MMAP event is processed.  So it
>>
>> Yes, it can, and the update is made via a new PERF_RECORD_MMAP, right?
>>
>> So:
>>
>> 	perf_event__process_mmap()
>> 	  machine__process_mmap2_event()
>> 	    map__new() + thread__insert_map(thread, map)
>> 	    	maps__fixup_overlappings()
>> 			maps__insert(thread->maps, map);
>>
>> Ok, from this point on new samples on ] map->start .. map->end ] will
>> grab a refcount to this new map in its hist_entry, right?
>>
>> When we want to sort by dso we will look at hist_entry->map->dso, etc.
> 
> And in 'perf top' we go decaying hist entries, when we delete the
> hist_entry, drop the reference count to things it holds, that will then
> be finally deleted when no more hist_entries point to it.
> 
>>> seems like anything racing with event processing is already broken, and
>>> reference counting / locking cannot help - unless there is also
>>> copy-on-write (which there isn't at present)?

So I checked, and struct map *is* copy-on-write in
maps__fixup_overlappings(), so that should not be a problem.

>>
>>> For struct maps, referencing it while simultaneously processing
>>> events seems to make even less sense?
>>
>> Can you elaborate some more?

Only that the maps are not necessarily stable e.g. the map that you
need has been replaced in the meantime.

But upon investigation, the only user at the moment is
maps__find_ams().  If we kept the removed maps (we used to),
it might be possible to make maps__find_ams() work correctly
in any case.