LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Thomas Gleixner <tglx@linutronix.de>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: Borislav Petkov <bp@alien8.de>,
	Peter Zijlstra <peterz@infradead.org>, Yonghong Song <yhs@fb.com>,
	Ingo Molnar <mingo@kernel.org>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Alexei Starovoitov <ast@fb.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	LKML <linux-kernel@vger.kernel.org>, X86 ML <x86@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	Kernel Team <kernel-team@fb.com>
Subject: Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto
Date: Sun, 13 May 2018 20:02:09 +0200 (CEST)	[thread overview]
Message-ID: <alpine.DEB.2.21.1805131953410.1582@nanos.tec.linutronix.de> (raw)
In-Reply-To: <20180513174348.eoh2lrhzqbmqb5nc@ast-mbp>

On Sun, 13 May 2018, Alexei Starovoitov wrote:
> On Sat, May 12, 2018 at 10:30:02PM +0200, Thomas Gleixner wrote:
> > But yes, the situation is slightly different here because tools which
> > create trace event magic _HAVE_ to pull in kernel headers. At the same time
> > these tools depend on a compiler which failed to implement asm_goto for
> > fricking 8 years.
> 
> As a maintainer of a piece of llvm codebase I have to say that
> this bullying tactic has the opposite effect.

I'm not bullying at all. Its a fact that the discussion about asm goto is
dragging out 8 years now. We've stayed away from mandating it for quite
some time, but at some point it just doesn't make sense anymore.

> The inline asm is processed by gcc and llvm very differently.  gcc is
> leaking internal backend implementation details into inline asm
> syntax. It makes little sense for llvm to do the same, since compiler
> codegen is completely different. gcc doesn't have integrated assembler
> whereas llvm not only can parse asm, but can potentially optimize it as
> well.  Instead of demanding asm-goto that matches gcc one to one it's
> better to work with the community to define the syntax that works for
> both kernel and llvm.

Come on, we surely are open for discussions, but what I've seen so far is
just 'oh we can't do this because' instead of a sane proposal how it can be
done w/o rewriting the whole ASM GOTO stuff in the kernel or even
duplicating it.

> > + * Workaround for the sake of BPF compilation which utilizes kernel
> > + * headers, but clang does not support ASM GOTO and fails the build.
> > + */
> > +#ifndef __BPF__
> > +#warning "Compiler lacks ASM_GOTO support. Add -D __BPF__ to your compiler arguments"
> > +#endif
> 
> Agree.
> The warning makes sense to me, but it has to be different macro name.
> How about -D__BPF_TRACING__ or -D__BPF_KPROBES__ or something similar ?

Fair enough.

> Such name will also make it clear that only tracing bpf programs
> need this. Networking programs shouldn't be including kernel headers.
> There was never a need, but since the tracing progs are often used
> as an example people copy paste makefiles too.
> We tried to document it as much as possible, but people still use
> 'clang -target native -I/kernel/includes bpf_prog.c -emit-llvm | llc -march=bpf'
> in their builds.
> (sometimes as a workaround for setups where clang is older version,
> but llc/llvm is new)
> Now they will see this warning and it will force them to think whether
> they actually need the kernel headers.

Makes sense.

> > +
> > +#define static_cpu_has(bit)		boot_cpu_has(bit)
> > +
> > +#else
> > +
> >  /*
> >   * Static testing of CPU features.  Used the same as boot_cpu_has().
> >   * These will statically patch the target code for additional
> > @@ -195,6 +209,7 @@ static __always_inline __pure bool _stat
> >  		boot_cpu_has(bit) :				\
> >  		_static_cpu_has(bit)				\
> >  )
> > +#endif
> >  
> >  #define cpu_has_bug(c, bit)		cpu_has(c, (bit))
> >  #define set_cpu_bug(c, bit)		set_cpu_cap(c, (bit))
> > --- a/samples/bpf/Makefile
> > +++ b/samples/bpf/Makefile
> > @@ -255,7 +255,7 @@ verify_target_bpf: verify_cmds
> >  $(obj)/%.o: $(src)/%.c
> >  	$(CLANG) $(NOSTDINC_FLAGS) $(LINUXINCLUDE) $(EXTRA_CFLAGS) -I$(obj) \
> >  		-I$(srctree)/tools/testing/selftests/bpf/ \
> > -		-D__KERNEL__ -Wno-unused-value -Wno-pointer-sign \
> > +		-D__KERNEL__ -D__BPF__ -Wno-unused-value -Wno-pointer-sign \
> 
> Yep. In samples/bpf and libbcc we can selectively add -D__BPF_TRACING__
> I think sysdig and other folks can live with that as well.
> Agree?

Sure. Care to send an updated patch?

Thanks,

	tglx

  reply	other threads:[~2018-05-13 18:02 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-05-12 16:03 Alexei Starovoitov
2018-05-12 20:30 ` Thomas Gleixner
2018-05-13 17:43   ` Alexei Starovoitov
2018-05-13 18:02     ` Thomas Gleixner [this message]
  -- strict thread matches above, loose matches on Subject: below --
2018-05-04  3:31 Yonghong Song
2018-05-10 10:06 ` Peter Zijlstra
2018-05-10 15:52   ` Alexei Starovoitov
2018-05-10 16:20     ` Borislav Petkov
2018-05-10 17:57       ` Gianluca Borello
2018-05-10 17:58       ` Alexei Starovoitov
2018-05-10 18:16         ` Borislav Petkov
2018-05-10 10:49 ` Borislav Petkov

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=alpine.DEB.2.21.1805131953410.1582@nanos.tec.linutronix.de \
    --to=tglx@linutronix.de \
    --cc=alexei.starovoitov@gmail.com \
    --cc=ast@fb.com \
    --cc=bp@alien8.de \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=peterz@infradead.org \
    --cc=torvalds@linux-foundation.org \
    --cc=x86@kernel.org \
    --cc=yhs@fb.com \
    --subject='Re: [PATCH bpf v3] x86/cpufeature: bpf hack for clang not supporting asm goto' \
    /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

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