On 11/13/24 9:33 AM, Alan Maguire wrote:
On 12/11/2024 19:10, Arnaldo Carvalho de Melo wrote:
On Tue, Nov 12, 2024 at 06:33:38PM +0000, Alan Maguire wrote:
On 12/11/2024 17:07, Yonghong Song wrote:
On 11/12/24 8:56 AM, Alan Maguire wrote:
On 12/11/2024 01:51, Yonghong Song wrote:
On 11/11/24 7:39 AM, Alan Maguire wrote:
"for one of internal 6.11 kernel, there are 62498 functions in BTF and
perf_event_read() is not there. With this patch, there are 61552
functions in BTF and perf_event_read() is included."
These numbers suggest you lost nearly 1000 functions when building
vmlinux BTF with pahole using this series. That's the part I don't
understand - we should just see a gain in numbers of functions in
vmlinux BTF, right? Did you mean 62552 functions rather than 61552
perhaps?tion
Sorry, really embarrassing. it is typo. Indeed it should be 62552 functions
in BTF instead.
No problem, makes perfect sense now, thanks! I'm trying to reproduce the
core dumps Eduard saw now with this setup; I'll report back if I manage
to do so and see if locks as Jiri and Arnaldo suggested help. If so a v2
along the lines of Eduard's suggested change plus locking might be the
best approach, what do you think? Thanks!
So the idea is to try to see what are the data structures that are
being corrupted in the features we use from elfutils libraries and check
how they are being protected via their non-default enabled experimental
thread safety locks and then use it before calling their functions that
would use those locks.
At some point we need to do some feature check to see if the lock is
enabled there and avoid adding it from pahole's side.
I.e. a transitional strategy to keep pahole -j feature that works with
older elfutils versions as well as with modern, thread safe ones.
This was used with the existing libdw__lock we have in the pahole
codebase with, AFAIK, good results.
Thanks for the additional info! From Eduard's analysis, it seems like it
is safer to take the libdw__lock around dwarf_getlocation(s), since
multiple threads can access the CU location cache. I've tried tweaking
Eduard's modification of Yonghong's original patch and adding a second
patch to add locking; with these two patches applied
- we see the desired behaviour where perf_event_read() is present in
BTF; and
- we don't see any segmentation faults after ~700 iterations where I saw
one every 200 or so before
Yonghong, Eduard - do these changes look okay from your side? Feel free
to resubmit if so (fixing up attributions as you see fit if they look
wrong of course). Thanks!
Thanks Alan for working on this. The following are some suggestions for patch one:
1. rename __dwarf_getlocations() to __parameter__locations()?
2. rename param_reg_at_entry to parameter__locations()?
3. You missed the following:
static int param_reg_at_entry(Dwarf_Attribute *attr, int expected_reg)
{
...
if (first_expr) // this line
return first_expr->atom; // this line
return -1;
}
Patch 2 needs adjustment as well due to the above point #3.
Otherwise, LGTM. Since you are already preparing the patch,
please go ahead to pose v2 after you fixing the above things.
Alan
