Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
From:   Song Liu <songliubraving@fb.com>
To:     Peter Zijlstra <peterz@infradead.org>
CC:     lkml <linux-kernel@vger.kernel.org>,
        "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "acme@kernel.org" <acme@kernel.org>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        Kernel Team <Kernel-team@fb.com>,
        Andi Kleen <andi@firstfloor.org>
Subject: Re: [PATCH v5 perf, bpf-next 3/7] perf, bpf: introduce
 PERF_RECORD_BPF_EVENT
Thread-Topic: [PATCH v5 perf, bpf-next 3/7] perf, bpf: introduce
 PERF_RECORD_BPF_EVENT
Thread-Index: AQHUmJHygB8CuOaEkU2QMQ4A40UscaWl0lUAgAAIHwCAAAktAIAARhkAgACuXQCAABTegIAAGEEAgAAzjIA=
Date:   Wed, 9 Jan 2019 16:04:08 +0000
Message-ID: <135D6A3B-45C8-42F3-ABD5-B0C8225BFAF2@fb.com>
References: <20181220182904.4193196-1-songliubraving@fb.com>
 <20181220182904.4193196-4-songliubraving@fb.com>
 <20190108184116.GC30894@hirez.programming.kicks-ass.net>
 <77A478D9-F36F-443A-BBFD-F0C1FFE0DD90@fb.com>
 <20190108194310.GD1900@hirez.programming.kicks-ass.net>
 <A0C713E9-6ACE-4235-87C2-2653A8F22F7B@fb.com>
 <20190109101808.GG1900@hirez.programming.kicks-ass.net>
 <924AE46C-B2B9-4E17-A6FC-C678FEADC03B@fb.com>
 <20190109125938.GJ1900@hirez.programming.kicks-ass.net>
In-Reply-To: <20190109125938.GJ1900@hirez.programming.kicks-ass.net>
Accept-Language: en-US
Content-Language: en-US
received-spf: None (protection.outlook.com: fb.com does not designate
 permitted sender hosts)
spamdiagnosticoutput: 1:99
spamdiagnosticmetadata: NSPM
Content-Type: text/plain; charset="us-ascii"
Content-ID: <8C9748F267FFE445B357C37907CC190F@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: 5a7bb1a5-ba01-481d-7470-08d6764c1671
X-MS-Exchange-CrossTenant-originalarrivaltime: 09 Jan 2019 16:04:08.3848
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1776
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2019-01-09_07:,,
 signatures=0
X-Proofpoint-Spam-Reason: safe
X-FB-Internal: Safe
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


> On Jan 9, 2019, at 4:59 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>=20
> 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,=
=20
>> we can use something like
>>=20
>> struct perf_record_text_modify {
>>    u64 addr;
>>    u_big_enough old_instr;
>>    u_big_enough new_instr;
>=20
> char[15] for x86 ;-)
>=20
> 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.
>=20
>>    timestamp ;
>=20
> that lives in struct sample_id.
>=20
>> };
>>=20
>> It is a fixed size record, and we don't need process it immediately=20
>> in user space. At the end of perf run, a series of these events will=20
>> help us reconstruct exact text at any time.=20
>=20
> That works for text_poke users, see also:
>=20
>  https://lkml.kernel.org/r/20190109103544.GH1900@hirez.programming.kicks-=
ass.net
>=20
> But is useless for module / bpf / ftrace dynamic text.

I think we will end up with RECORD_KSYMBOL + something else for all cases.=
=20
For bpf, it is RECORD_KSYMBOL + (optional) RECORD_BPF_EVENT. For text_poke,=
=20
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=20
record goes to a separate buffer, and requires extra processing.=20

>=20
>>> All we need is some means of ensuring the symbol is still there by the
>>> time we see the event and do the copy.
>>>=20
>>> I think we can do this with a new ioctl() on /proc/kcore itself:
>>>=20
>>> - when we have kcore open, we queue all text-free operations on list-1.
>>>=20
>>> - when we close kcore, we drain all (text-free) list-* and perform the
>>>  pending frees immediately.
>>>=20
>>> - 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.
>>>=20
>>> 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.
>>=20
>> Does this mean we need to scan every buffer before writing it to perf.da=
ta=20
>> during perf-record?=20
>=20
> 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.=20

>=20
>> Also, if we need ksym unreg here, I guess it is NOT really modifying tex=
t=20
>> in-place, but creating new version and swap? Then can we include somethi=
ng=20
>> like this in perf.data:
>>=20
>> 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 ;
>> };
>>=20
>> 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).=20
>> This would work for text modifying case, as modifying text is simply
>> old-text to new-text.
>>=20
>> Similar solution would not work for BPF case, as bpf_prog_info is=20
>> getting a lot more members in the near future.=20
>>=20
>> Does this make sense...?
>=20
> I don't think we actually need old_text here either. We're creating a
> new text mapping, there was nothing there before.
>=20
> 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=20
RECORD_TEXT_MODIFY.=20

Thanks,
Song=