Re: [PATCH v5 perf, bpf-next 3/7] perf, bpf: introduce PERF_RECORD_BPF_EVENT

On Jan 9, 2019, at 4:59 AM, Peter Zijlstra [off-list ref] wrote:

On Wed, Jan 09, 2019 at 11:32:50AM +0000, Song Liu wrote:

quoted

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.

quoted

   timestamp ;

that lives in struct sample_id.

quoted

};

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. 

quoted

quoted

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. 

quoted

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