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:     "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "ast@kernel.org" <ast@kernel.org>,
        "daniel@iogearbox.net" <daniel@iogearbox.net>,
        "acme@kernel.org" <acme@kernel.org>,
        Kernel Team <Kernel-team@fb.com>
Subject: Re: [PATCH perf,bpf 0/5] reveal invisible bpf programs
Thread-Topic: [PATCH perf,bpf 0/5] reveal invisible bpf programs
Thread-Index: AQHUgdQ11TRJMN8CIkSD/uQZUfyfEaVbiN6AgACRn4CABhB7AIAAVs6AgAGCpIA=
Date:   Tue, 27 Nov 2018 19:04:35 +0000
Message-ID: <B1C5F28C-5359-4E03-88B6-726FA9C73BB4@fb.com>
References: <20181121195502.3259930-1-songliubraving@fb.com>
 <20181122093219.GK2131@hirez.programming.kicks-ass.net>
 <71189F83-A09F-4A03-95EC-694D37FD7675@fb.com>
 <20181126145004.GO2113@hirez.programming.kicks-ass.net>
 <83B32093-DD4B-4A64-8476-07471015D72F@fb.com>
In-Reply-To: <83B32093-DD4B-4A64-8476-07471015D72F@fb.com>
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: <0ED587AAC44D02429CF982E8839F5095@namprd15.prod.outlook.com>
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-Network-Message-Id: c192e229-662b-4635-c567-08d6549b2c5f
X-MS-Exchange-CrossTenant-originalarrivaltime: 27 Nov 2018 19:04:35.8923
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MWHPR15MB1359
X-OriginatorOrg: fb.com
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10434:,, definitions=2018-11-27_14:,,
 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 Nov 26, 2018, at 12:00 PM, Song Liu <songliubraving@fb.com> wrote:
>=20
>=20
>=20
>> On Nov 26, 2018, at 6:50 AM, Peter Zijlstra <peterz@infradead.org> wrote=
:
>>=20
>> On Thu, Nov 22, 2018 at 06:13:32PM +0000, Song Liu wrote:
>>> Hi Peter,
>>>=20
>>>> On Nov 22, 2018, at 1:32 AM, Peter Zijlstra <peterz@infradead.org> wro=
te:
>>>>=20
>>>> On Wed, Nov 21, 2018 at 11:54:57AM -0800, Song Liu wrote:
>>>>> Changes RFC -> PATCH v1:
>>>>>=20
>>>>> 1. In perf-record, poll vip events in a separate thread;
>>>>> 2. Add tag to bpf prog name;
>>>>> 3. Small refactorings.
>>>>>=20
>>>>> Original cover letter (with minor revisions):
>>>>>=20
>>>>> This is to follow up Alexei's early effort to show bpf programs
>>=20
>>>>> In this version, PERF_RECORD_BPF_EVENT is introduced to send real tim=
e BPF
>>>>> load/unload events to user space. In user space, perf-record is modif=
ied
>>>>> to listen to these events (through a dedicated ring buffer) and gener=
ate
>>>>> detailed information about the program (struct bpf_prog_info_event). =
Then,
>>>>> perf-report translates these events into proper symbols.
>>>>>=20
>>>>> With this set, perf-report will show bpf program as:
>>>>>=20
>>>>> 18.49%     0.16%  test  [kernel.vmlinux]    [k] ksys_write
>>>>> 18.01%     0.47%  test  [kernel.vmlinux]    [k] vfs_write
>>>>> 17.02%     0.40%  test  bpf_prog            [k] bpf_prog_07367f7ba80d=
f72b_
>>>>> 16.97%     0.10%  test  [kernel.vmlinux]    [k] __vfs_write
>>>>> 16.86%     0.12%  test  [kernel.vmlinux]    [k] comm_write
>>>>> 16.67%     0.39%  test  [kernel.vmlinux]    [k] bpf_probe_read
>>>>>=20
>>>>> Note that, the program name is still work in progress, it will be cle=
aner
>>>>> with function types in BTF.
>>>>>=20
>>>>> Please share your comments on this.
>>>>=20
>>>> So I see:
>>>>=20
>>>> kernel/bpf/core.c:void bpf_prog_kallsyms_add(struct bpf_prog *fp)
>>>>=20
>>>> which should already provide basic symbol information for extant eBPF
>>>> programs, right?
>>>=20
>>> Right, if the BPF program is still loaded when perf-report runs, symbol=
s=20
>>> are available.=20
>>=20
>> Good, that is not something that was clear. The Changelog seems to imply
>> we need this new stuff in order to observe symbols.
>=20
> I will clarify this in next version.=20
>=20
>>=20
>>>> And (AFAIK) perf uses /proc/kcore for annotate on the current running
>>>> kernel (if not, it really should, given alternatives, jump_labels and
>>>> all other other self-modifying code).
>>>>=20
>>>> So this fancy new stuff is only for the case where your profile spans
>>>> eBPF load/unload events (which should be relatively rare in the normal
>>>> case, right), or when you want source annotated asm output (I normally
>>>> don't bother with that).
>>>=20
>>> This patch set adds two pieces of information:
>>> 1. At the beginning of perf-record, save info of existing BPF programs;
>>> 2. Gather information of BPF programs load/unload during perf-record.=20
>>>=20
>>> (1) is all in user space. It is necessary to show symbols of BPF progra=
m
>>> that are unloaded _after_ perf-record. (2) needs PERF_RECORD_BPF_EVENT=
=20
>>> from the ring buffer. It covers BPF program loaded during perf-record=20
>>> (perf record -- bpf_test).=20
>>=20
>> I'm saying that if you given them symbols; most people won't need any of
>> that ever.
>>=20
>> And just tracking kallsyms is _much_ cheaper than either 1 or 2. Alexei
>> was talking fairly big amounts of data per BPF prog. Dumping and saving
>> that sounds like pointless overhead for 99% of the users.
>=20
> Due to the kernel-module-like natural of BPF program, I think it is still
> necessary to cover cases that BPF programs are unloaded when perf-record
> runs. How about we add another step that scans all bpf_prog_XXXX from
> kallsyms, and synthesizes symbols for them?
>=20
>>=20
>>>> That is; I would really like this fancy stuff to be an optional extra
>>>> that is typically not needed.
>>>>=20
>>>> Does that make sense?
>>>=20
>>> (1) above is always enabled with this set. I added option no-bpf-events=
=20
>>> to disable (2). I guess you prefer the (2) is disabled by default, and=
=20
>>> enabled with an option?
>>=20
>> I'm saying neither should be default enabled. Instead we should do
>> recording and tracking by default.
>>=20
>> That gets people symbol information on BPF stuff, which is likely all
>> they ever need.
>=20
> How about we extend PERF_RECORD_BPF_EVENT with basic symbol information
> (name, addr, length)? By default, we just record these events in the ring
> buffer, just like mmap events. This (plus scanning kallsyms before record=
)
> will enable symbols for all BPF programs.=20
>=20
> For more information, we add an option to enable more information=20
> (annotated asm, etc.), with dedicated dummy event, thread, and ring buffe=
r.=20
>=20

After syncing with Alexei offline, I realized this won't work cleanly.=20

My initial idea is to extend PERF_RECORD_BPF_EVENT like:

struct bpf_event {
        struct perf_event_header header;
        u16 type;
        u16 flags;
        u32 id;          /* bpf prog_id */

        u64 start_addr;
        u32 length;   =20
        char name[KSYM_NAME_LEN];
};

This is a structure with variable length, which is OK.=20

However, I missed the fact that each bpf program could have up to 256 sub
programs. To fit that into single bpf_event need some pretty ugly hack:

struct bpf_sub_prog {
        u64 start_addr;
        u32 length;    /* or length */
        u32 name_len;  /* length of name, struct bpf_event need it */
        char name[KSYM_NAME_LEN];
};

struct bpf_event {
        struct perf_event_header header;
        u16 type;
        u16 flags;
        u32 id;          /* bpf prog_id */
        u64 num_of_sub_prog;
       =20
	struct bpf_sub_prog sub_progs[];
};

In this case, bpf_sub_prog has variable length, thus struct bpf_event has
variable number of variable length members. This not impossible to handle
these, but it will be ugly for sure.=20

One alternative to this approach is to keep one sub program per bpf_event,=
=20
but generate multiple bpf_event (one for each sub program). Another solutio=
n
is to generate separate records (same or different PERF_RECORD_*) for=20
bpf_event and bpf_sub_prog.=20

However, I don't think any of these yield as clean perf ABI as current=20
version.=20

In summary, I can think of the following directions:

1. Current design.=20
   Pros: simple perf ABI;=20
   Cons: more work in perf tool.
2. One bpf_event per BPF prog, variable number of variable length variable.
   Pros: less work for perf tool, if annotation is not needed;=20
   Cons: ugly perf ABI.=20
3. Multiple bpf_event per BPF prog (one per sub prog).=20
   Pros: less work for perf tool, if annotation is not needed;=20
   Cons: need coordinate multiple records (same prog_id, different sub prog=
).
4. Separate bpf_event and bpf_sub_prog_event.=20
   Pros: less work for perf tool, if annotation is not needed;=20
   Cons: more complicated perf ABI (two PERF_RECORD_* or more complicated=20
         encoding for the same PERF_RECORD_).=20

Overall, I think current design is the best for simplest ABI. And it gives=
=20
flexibility to tune what information perf tool get from bpf syscalls.=20

Peter and Arnaldo, what's your recommendation on these directions (or other
better alternatives)?

Thanks,
Song