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

[Song Liu]

> On Jan 9, 2019, at 4:59 AM, Peter Zijlstra <peterz@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx
> 
> 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