Messages in this thread | | | From | Song Liu <> | Subject | Re: [PATCH v5 perf, bpf-next 3/7] perf, bpf: introduce PERF_RECORD_BPF_EVENT | Date | Wed, 9 Jan 2019 16:04:08 +0000 |
| |
> On Jan 9, 2019, at 4:59 AM, Peter Zijlstra <peterz@infradead.org> wrote: > > On Wed, Jan 09, 2019 at 11:32:50AM +0000, Song Liu wrote: >> I was thinking about modifying the text in-place scenario. In this case, >> we can use something like >> >> struct perf_record_text_modify { >> u64 addr; >> u_big_enough old_instr; >> u_big_enough new_instr; > > char[15] for x86 ;-) > > Also, I don't think we need old, we should already have the old text, > either from a previous event or from the initial kcore snapshot. > >> timestamp ; > > that lives in struct sample_id. > >> }; >> >> It is a fixed size record, and we don't need process it immediately >> in user space. At the end of perf run, a series of these events will >> help us reconstruct exact text at any time. > > That works for text_poke users, see also: > > https://lkml.kernel.org/r/20190109103544.GH1900@hirez.programming.kicks-ass.net > > But is useless for module / bpf / ftrace dynamic text.
I think we will end up with RECORD_KSYMBOL + something else for all cases. For bpf, it is RECORD_KSYMBOL + (optional) RECORD_BPF_EVENT. For text_poke, it will be RECORD_KSYMBOL + RECORD_TEXT_POKE. In all cases, RECORD_KSYMBOL goes to regular buffer and gets saved directly to perf.data. The other record goes to a separate buffer, and requires extra processing.
> >>> All we need is some means of ensuring the symbol is still there by the >>> time we see the event and do the copy. >>> >>> I think we can do this with a new ioctl() on /proc/kcore itself: >>> >>> - when we have kcore open, we queue all text-free operations on list-1. >>> >>> - when we close kcore, we drain all (text-free) list-* and perform the >>> pending frees immediately. >>> >>> - on ioctl(KCORE_QC) we perform the pending free of list-3 and advance >>> list-2 to list-3 and list-1 to list-2. >>> >>> Perf would then open kcore at the start of the record, make a complete >>> copy and keep the FD open. At the end of every buffer process, we issue >>> KCORE_QC IFF we observed a ksym unreg in that buffer. >> >> Does this mean we need to scan every buffer before writing it to perf.data >> during perf-record? > > Just like the BPF events, yes. Now for PT most of the actual data is not > in the regular buffer, so it shouldn't be too horrible, but just like > the BPF event, it can get its own buffer if it does become a problem.
I see. Separate buffer does make it better.
> >> Also, if we need ksym unreg here, I guess it is NOT really modifying text >> in-place, but creating new version and swap? Then can we include something >> like this in perf.data: >> >> struct perf_record_text_modify { >> u64 old_addr; >> u64 new_addr; >> u32 old_len; /* up to MAX_SIZE */ >> u32 new_len; /* up to MAX_SIZE */ >> u8 old_text[MAX_SIZE]; >> u8 new_text[MAX_SIZE]; >> timestamp ; >> }; >> >> In this way, this record is embedded in perf.data, and doesn't require >> extra processing during perf-record (only at the end of perf-record). >> This would work for text modifying case, as modifying text is simply >> old-text to new-text. >> >> Similar solution would not work for BPF case, as bpf_prog_info is >> getting a lot more members in the near future. >> >> Does this make sense...? > > I don't think we actually need old_text here either. We're creating a > new text mapping, there was nothing there before. > > But still, perf events are limited to 64k, so that means we cannot > support symbols larger than that (although I suppose that would be > fairly rare).
For larger symbols, I guess we can do one RECORD_KSYMBOL and multiple RECORD_TEXT_MODIFY.
Thanks, Song
| |