LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: Yonghong Song <yhs@fb.com>
To: Peter Zijlstra <peterz@infradead.org>
Cc: <mingo@kernel.org>, <ast@fb.com>, <daniel@iogearbox.net>,
	<linux-kernel@vger.kernel.org>, <x86@kernel.org>,
	<kernel-team@fb.com>
Subject: Re: [PATCH v2] x86/cpufeature: guard asm_volatile_goto usage with NO_BPF_WORKAROUND
Date: Mon, 23 Apr 2018 09:50:59 -0700	[thread overview]
Message-ID: <3a6e6348-fb23-c2f9-5bd8-928b056b4722@fb.com> (raw)
In-Reply-To: <20180423105209.GX4064@hirez.programming.kicks-ass.net>

Hi, Peter,

Please see comments below.

On 4/23/18 3:52 AM, Peter Zijlstra wrote:
> On Fri, Apr 20, 2018 at 11:06:03AM -0700, Yonghong Song wrote:
>> On 4/20/18 1:19 AM, Peter Zijlstra wrote:
> 
>>> Hurm, so adding __BPF__ for BPF compiles isn't an option? It seems to me
>>> having a CPP flag to identify BPF compile context might be useful in
>>> general.
>>
>> With "clang -target bpf", we already have __BPF__ defined.
>> For tracing, esp. ptrace.h is included, "clang -target <native_arch>" where
>> "-target <native_arch>" can be omitted, is typically used.
> 
>> The reason is the native architecture header files typically
>> include a lot of various asm related stuff where "-target bpf" cannot
>> really handle. We relay on native clang to flush out all these
>> asm constructs and only bpf program needed stuff survives
>> reach to backend compiler.
> 
> So because 'clang -target bpf' is 'broken', you do a work-around using

'clang -target bpf' is 'broken' in this case because the x86 arch has
a lot of inline asm's in the header file where bpf target cannot handle.
For most networking related bpf programs where `asm` is rarely involved,
`clang -target bpf` works fine most of time.

> 'clang -target <native_arch>'. But because that doesn't set __BPF__ you

`clang -target <native_arch>` should work, regardless of whether __BPF__
macro is setup or not. The reason it doesn't work now is due to its 
lacking asm-goto support. So to use `clang -target <native_arch>` is not
really a workaround for `target bpf`. It by itself should work.

> want to add NO_BPF_WORKAROUND to the kernel instead of adding __BPF__ to
> your build rules to better mimick -target bpf, which you should be
> using.
> 
> How is that sane? Why not use 'clang -target <native_arch> -D__BPF__'

To workaround the asm-goto issue, the suggested macro __BPF__ can be 
added to user space and kernel. But note that `clang -target 
<native_arch>` will not define the macro __BPF__, so this requires
user space change.

Also, to make sure people understand that this is a WORKAROUND for
asm-goto issue and is not a lasting thing we want to support. I have
the following change for cpufeature.h:

diff --git a/arch/x86/include/asm/cpufeature.h 
b/arch/x86/include/asm/cpufeature.h
index b27da9602a6d..c832118defa1 100644
--- a/arch/x86/include/asm/cpufeature.h
+++ b/arch/x86/include/asm/cpufeature.h
@@ -140,6 +140,7 @@ extern void clear_cpu_cap(struct cpuinfo_x86 *c, 
unsigned int bit);

  #define setup_force_cpu_bug(bit) setup_force_cpu_cap(bit)

+#ifndef __BPF_WORKAROUND__
  /*
   * Static testing of CPU features.  Used the same as boot_cpu_has().
   * These will statically patch the target code for additional
@@ -195,6 +196,9 @@ static __always_inline __pure bool 
_static_cpu_has(u16 bit)
                 boot_cpu_has(bit) :                             \
                 _static_cpu_has(bit)                            \
  )
+#else
+#define static_cpu_has(bit)            boot_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))
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 4d6a6edd4bf6..b229e5090e4a 100644

As mentioned above, user space needs to add this new macro definition.
Specifically for kernel/samples/bpf:
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 4d6a6edd4bf6..b229e5090e4a 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -255,7 +255,7 @@ $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
  $(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_WORKAROUND__ -Wno-unused-value 
-Wno-pointer-sign \
                 -D__TARGET_ARCH_$(ARCH) 
-Wno-compare-distinct-pointer-types \
                 -Wno-gnu-variable-sized-type-not-at-end \
                 -Wno-address-of-packed-member -Wno-tautological-compare \

Please let me know whether this approach is okay to you or not,
whether the name __BPF_WORKAROUND__ is better than __BPF__ or not, or we 
could use the earlier approach which does not require user space change.

Thanks!

  reply	other threads:[~2018-04-23 16:51 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-04-15  4:27 Yonghong Song
2018-04-19 16:36 ` Yonghong Song
2018-04-20  8:19 ` Peter Zijlstra
2018-04-20 18:06   ` Yonghong Song
2018-04-23 10:52     ` Peter Zijlstra
2018-04-23 16:50       ` Yonghong Song [this message]
2018-04-27 16:34         ` Yonghong Song
2018-05-02 14:01           ` Yonghong Song

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=3a6e6348-fb23-c2f9-5bd8-928b056b4722@fb.com \
    --to=yhs@fb.com \
    --cc=ast@fb.com \
    --cc=daniel@iogearbox.net \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@kernel.org \
    --cc=peterz@infradead.org \
    --cc=x86@kernel.org \
    --subject='Re: [PATCH v2] x86/cpufeature: guard asm_volatile_goto usage with NO_BPF_WORKAROUND' \
    /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).