by Song Liu

[permalink] [raw]

Subject: 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 <[email protected]> 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/[email protected]
>
> 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