From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: X86 ML <x86@kernel.org>, Josh Poimboeuf <jpoimboe@redhat.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
LKML <linux-kernel@vger.kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>,
Daniel Borkmann <daniel@iogearbox.net>, bpf <bpf@vger.kernel.org>,
Andrii Nakryiko <andrii@kernel.org>
Subject: Re: [PATCH v2 14/14] bpf,x86: Respect X86_FEATURE_RETPOLINE*
Date: Thu, 21 Oct 2021 11:03:33 -0700 [thread overview]
Message-ID: <CAADnVQKD6=HwmnTw=Shup7Rav-+OTWJERRYSAn-as6iikqoHEA@mail.gmail.com> (raw)
In-Reply-To: <YXEpBKxUICIPVj14@hirez.programming.kicks-ass.net>
On Thu, Oct 21, 2021 at 1:47 AM Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Oct 20, 2021 at 05:05:02PM -0700, Alexei Starovoitov wrote:
> > On Wed, Oct 20, 2021 at 01:09:51PM +0200, Peter Zijlstra wrote:
>
> > > @@ -446,25 +440,8 @@ static void emit_bpf_tail_call_indirect(
> > > {
> > > int tcc_off = -4 - round_up(stack_depth, 8);
> > > u8 *prog = *pprog, *start = *pprog;
> > > - int pop_bytes = 0;
> > > - int off1 = 42;
> > > - int off2 = 31;
> > > - int off3 = 9;
> > > -
> > > - /* count the additional bytes used for popping callee regs from stack
> > > - * that need to be taken into account for each of the offsets that
> > > - * are used for bailing out of the tail call
> > > - */
> > > - pop_bytes = get_pop_bytes(callee_regs_used);
> > > - off1 += pop_bytes;
> > > - off2 += pop_bytes;
> > > - off3 += pop_bytes;
> > > -
> > > - if (stack_depth) {
> > > - off1 += 7;
> > > - off2 += 7;
> > > - off3 += 7;
> > > - }
> > > + static int out_label = -1;
> >
> > Interesting idea!
>
> I nicked it from emit_bpf_tail_call() in the 32bit jit :-) It seemed a
> lot more robust than the 64bit one and I couldn't figure out why the
> difference.
Interesting. Daniel will recognize that trick then :)
> > All insn emits trying to do the right thing from the start.
> > Here the logic assumes that there will be at least two passes over image.
> > I think that is correct, but we never had such assumption.
>
> That's not exactly true; I think image is NULL on every first run, so
> all insn that depend on it will be wrong to start with. Equally there's
> a number of insn that seem to depend on addrs[i], that also requires at
> least two passes.
Right. The image will be allocated after size converges and
addrs[] is inited with 64.
So there is certainly more than one pass.
I was saying that emit* helpers didn't have that assumption.
Looks like 32-bit JIT had.
> > A comment is certainly must have.
>
> I can certainly add one, although I think we'll disagree on the comment
> style :-)
I think we're on the same page actually.
> > The race is possible too. Not sure whether READ_ONCE/WRITE_ONCE
> > are really warranted though. Might be overkill.
>
> Is there concurrency on the jit?
The JIT of different progs can happen in parallel.
> > Once you have a git branch with all the changes I can give it a go.
>
> Ok, I'll go polish this thing and stick it in the tree mentioned in the
> cover letter.
>
> > Also you can rely on our BPF CI.
> > Just cc your patchset to bpf@vger and add [PATCH bpf-next] to a subject.
> > In patchwork there will be "bpf/vmtest-bpf-next" link that
> > builds kernel, selftests and runs everything.
>
> What's a patchwork and where do I find it?
https://patchwork.kernel.org/project/netdevbpf/list/?delegate=121173
Click on any patch, search for 'bpf/vmtest-bpf-next' and follow the
'VM_Test' link.
The summary of the test run is available without logging in into github.
To see detailed logs you need to be logged in with your github account.
It's a silly limitation they have.
They even have a button 'Sign in to view logs'. Oh well.
> > It's pretty much the same as selftests/bpf/vmtest.sh, but with the latest
> > clang nightly and other deps like pahole.
>
> nice.
One more thing. There is test_bpf.ko.
Just insmod it and it will run a ton of JIT tests that we cannot do
from user space.
Please use bpf-next tree though. Few weeks ago Johan Almbladh added
a lot more tests to it.
next prev parent reply other threads:[~2021-10-21 18:03 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20211020104442.021802560@infradead.org>
[not found] ` <20211020105843.345016338@infradead.org>
[not found] ` <YW/4/7MjUf3hWfjz@hirez.programming.kicks-ass.net>
2021-10-21 0:05 ` [PATCH v2 14/14] bpf,x86: Respect X86_FEATURE_RETPOLINE* Alexei Starovoitov
2021-10-21 8:47 ` Peter Zijlstra
2021-10-21 18:03 ` Alexei Starovoitov [this message]
2021-10-21 22:37 ` Peter Zijlstra
2021-10-21 23:24 ` Alexei Starovoitov
2021-10-21 23:38 ` Josh Poimboeuf
2021-10-21 23:42 ` Alexei Starovoitov
2021-10-22 11:31 ` Peter Zijlstra
2021-10-22 15:22 ` Alexei Starovoitov
2021-10-25 13:44 ` Maciej Fijalkowski
2021-10-25 12:42 ` Peter Zijlstra
2021-10-21 23:51 ` Zvi Effron
2021-10-22 8:33 ` Peter Zijlstra
2021-10-22 21:06 ` Zvi Effron
2021-10-21 0:07 ` Alexei Starovoitov
2021-10-21 0:18 ` Josh Poimboeuf
2021-10-21 8:53 ` Peter Zijlstra
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAADnVQKD6=HwmnTw=Shup7Rav-+OTWJERRYSAn-as6iikqoHEA@mail.gmail.com' \
--to=alexei.starovoitov@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=andrii@kernel.org \
--cc=bpf@vger.kernel.org \
--cc=daniel@iogearbox.net \
--cc=jpoimboe@redhat.com \
--cc=linux-kernel@vger.kernel.org \
--cc=ndesaulniers@google.com \
--cc=peterz@infradead.org \
--cc=x86@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).