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;
Date:   Wed, 9 Jan 2019 13:59:38 +0100
From:   Peter Zijlstra <peterz@infradead.org>
To:     Song Liu <songliubraving@fb.com>
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
Message-ID: <20190109125938.GJ1900@hirez.programming.kicks-ass.net>
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>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <924AE46C-B2B9-4E17-A6FC-C678FEADC03B@fb.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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@hirez.programming.kicks-ass.net

But is useless for module / bpf / ftrace dynamic text.

> > 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.

> 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).

Something like that could work, but I'm not sure it is actually better.
Some PT person would have to play with things I suppose.