diff mbox series

[v3] bpf: Remove custom build rule

Message ID 20240828181028.4166334-1-legion@kernel.org (mailing list archive)
State New
Headers show
Series [v3] bpf: Remove custom build rule | expand

Commit Message

Alexey Gladkov Aug. 28, 2024, 6:10 p.m. UTC 
According to the documentation, when building a kernel with the C=2
parameter, all source files should be checked. But this does not happen
for the kernel/bpf/ directory.

$ touch kernel/bpf/core.c
$ make C=2 CHECK=true kernel/bpf/core.o

Outputs:

  CHECK   scripts/mod/empty.c
  CALL    scripts/checksyscalls.sh
  DESCEND objtool
  INSTALL libsubcmd_headers
  CC      kernel/bpf/core.o

As can be seen the compilation is done, but CHECK is not executed. This
happens because kernel/bpf/Makefile has defined its own rule for
compilation and forgotten the macro that does the check.

There is no need to duplicate the build code, and this rule can be
removed to use generic rules.

Signed-off-by: Alexey Gladkov <legion@kernel.org>
---
 kernel/bpf/Makefile       | 6 ------
 kernel/bpf/btf_iter.c     | 2 ++
 kernel/bpf/btf_relocate.c | 2 ++
 kernel/bpf/relo_core.c    | 2 ++
 4 files changed, 6 insertions(+), 6 deletions(-)
 create mode 100644 kernel/bpf/btf_iter.c
 create mode 100644 kernel/bpf/btf_relocate.c
 create mode 100644 kernel/bpf/relo_core.c

Comments

Masahiro Yamada Aug. 28, 2024, 6:32 p.m. UTC | #1 
On Thu, Aug 29, 2024 at 3:11 AM Alexey Gladkov <legion@kernel.org> wrote:
>
> According to the documentation, when building a kernel with the C=2
> parameter, all source files should be checked. But this does not happen
> for the kernel/bpf/ directory.
>
> $ touch kernel/bpf/core.c
> $ make C=2 CHECK=true kernel/bpf/core.o
>
> Outputs:
>
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   CC      kernel/bpf/core.o
>
> As can be seen the compilation is done, but CHECK is not executed. This
> happens because kernel/bpf/Makefile has defined its own rule for
> compilation and forgotten the macro that does the check.
>
> There is no need to duplicate the build code, and this rule can be
> removed to use generic rules.
>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>


Acked-by: Masahiro Yamada <masahiroy@kernel.org>


> ---
>  kernel/bpf/Makefile       | 6 ------
>  kernel/bpf/btf_iter.c     | 2 ++
>  kernel/bpf/btf_relocate.c | 2 ++
>  kernel/bpf/relo_core.c    | 2 ++
>  4 files changed, 6 insertions(+), 6 deletions(-)
>  create mode 100644 kernel/bpf/btf_iter.c
>  create mode 100644 kernel/bpf/btf_relocate.c
>  create mode 100644 kernel/bpf/relo_core.c
>
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 0291eef9ce92..9b9c151b5c82 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -52,9 +52,3 @@ obj-$(CONFIG_BPF_PRELOAD) += preload/
>  obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
>  obj-$(CONFIG_BPF_SYSCALL) += btf_iter.o
>  obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o
> -
> -# Some source files are common to libbpf.
> -vpath %.c $(srctree)/kernel/bpf:$(srctree)/tools/lib/bpf
> -
> -$(obj)/%.o: %.c FORCE
> -       $(call if_changed_rule,cc_o_c)
> diff --git a/kernel/bpf/btf_iter.c b/kernel/bpf/btf_iter.c
> new file mode 100644
> index 000000000000..eab8493a1669
> --- /dev/null
> +++ b/kernel/bpf/btf_iter.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/btf_iter.c"
> diff --git a/kernel/bpf/btf_relocate.c b/kernel/bpf/btf_relocate.c
> new file mode 100644
> index 000000000000..8c89c7b59ef8
> --- /dev/null
> +++ b/kernel/bpf/btf_relocate.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/btf_relocate.c"
> diff --git a/kernel/bpf/relo_core.c b/kernel/bpf/relo_core.c
> new file mode 100644
> index 000000000000..6a36fbc0e5ab
> --- /dev/null
> +++ b/kernel/bpf/relo_core.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/relo_core.c"
> --
> 2.46.0
>
Oleg Nesterov Aug. 28, 2024, 7:14 p.m. UTC | #2 
I know nothing about Kbuild, I can only confirm that this patch fixes the
problem I encountered in practice.

On 08/28, Alexey Gladkov wrote:
>
> $ touch kernel/bpf/core.c
> $ make C=2 CHECK=true kernel/bpf/core.o
>
> Outputs:
>
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   CC      kernel/bpf/core.o
>
> As can be seen the compilation is done, but CHECK is not executed.

And after that

	$ make C=2 CHECK=true kernel/bpf/core.o
	  CHECK   scripts/mod/empty.c
	  CALL    scripts/checksyscalls.sh
	  DESCEND objtool
	  INSTALL libsubcmd_headers

CHECK is also not executed.

compare with, for example,

	$ touch kernel/trace/trace.c
	$ make C=2 CHECK=true kernel/trace/trace.o
	  CHECK   scripts/mod/empty.c
	  CALL    scripts/checksyscalls.sh
	  DESCEND objtool
	  INSTALL libsubcmd_headers
	  CC      kernel/trace/trace.o
	  CHECK   kernel/trace/trace.c
	$ make C=2 CHECK=true kernel/trace/trace.o
	  CHECK   scripts/mod/empty.c
	  CALL    scripts/checksyscalls.sh
	  DESCEND objtool
	  INSTALL libsubcmd_headers
	  CHECK   kernel/trace/trace.c

Tested-by: Oleg Nesterov <oleg@redhat.com>
Alan Maguire Aug. 29, 2024, 10:12 a.m. UTC | #3 
On 28/08/2024 19:10, Alexey Gladkov wrote:
> According to the documentation, when building a kernel with the C=2
> parameter, all source files should be checked. But this does not happen
> for the kernel/bpf/ directory.
> 
> $ touch kernel/bpf/core.c
> $ make C=2 CHECK=true kernel/bpf/core.o
> 
> Outputs:
> 
>   CHECK   scripts/mod/empty.c
>   CALL    scripts/checksyscalls.sh
>   DESCEND objtool
>   INSTALL libsubcmd_headers
>   CC      kernel/bpf/core.o
> 
> As can be seen the compilation is done, but CHECK is not executed. This
> happens because kernel/bpf/Makefile has defined its own rule for
> compilation and forgotten the macro that does the check.
> 
> There is no need to duplicate the build code, and this rule can be
> removed to use generic rules.
> 
> Signed-off-by: Alexey Gladkov <legion@kernel.org>

Tested-by: Alan Maguire <alan.maguire@oracle.com>

The aim from the BPF side is just to share the btf_iter.c,
btf_relocate.c and relo_core.c files for both libbpf and kernel build.
Those files need to live in tools/lib/bpf because the libbpf standalone
repo on github is built from tools/lib/bpf.

Since the approach in this patch continues to support that while it
doesn't break other things like check targets it's definitely preferred.

Thanks for the fix!

Alan

> ---
>  kernel/bpf/Makefile       | 6 ------
>  kernel/bpf/btf_iter.c     | 2 ++
>  kernel/bpf/btf_relocate.c | 2 ++
>  kernel/bpf/relo_core.c    | 2 ++
>  4 files changed, 6 insertions(+), 6 deletions(-)
>  create mode 100644 kernel/bpf/btf_iter.c
>  create mode 100644 kernel/bpf/btf_relocate.c
>  create mode 100644 kernel/bpf/relo_core.c
> 
> diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
> index 0291eef9ce92..9b9c151b5c82 100644
> --- a/kernel/bpf/Makefile
> +++ b/kernel/bpf/Makefile
> @@ -52,9 +52,3 @@ obj-$(CONFIG_BPF_PRELOAD) += preload/
>  obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
>  obj-$(CONFIG_BPF_SYSCALL) += btf_iter.o
>  obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o
> -
> -# Some source files are common to libbpf.
> -vpath %.c $(srctree)/kernel/bpf:$(srctree)/tools/lib/bpf
> -
> -$(obj)/%.o: %.c FORCE
> -	$(call if_changed_rule,cc_o_c)
> diff --git a/kernel/bpf/btf_iter.c b/kernel/bpf/btf_iter.c
> new file mode 100644
> index 000000000000..eab8493a1669
> --- /dev/null
> +++ b/kernel/bpf/btf_iter.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/btf_iter.c"
> diff --git a/kernel/bpf/btf_relocate.c b/kernel/bpf/btf_relocate.c
> new file mode 100644
> index 000000000000..8c89c7b59ef8
> --- /dev/null
> +++ b/kernel/bpf/btf_relocate.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/btf_relocate.c"
> diff --git a/kernel/bpf/relo_core.c b/kernel/bpf/relo_core.c
> new file mode 100644
> index 000000000000..6a36fbc0e5ab
> --- /dev/null
> +++ b/kernel/bpf/relo_core.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/relo_core.c"
Alexei Starovoitov Aug. 29, 2024, 7:24 p.m. UTC | #4 
On Wed, Aug 28, 2024 at 11:11 AM Alexey Gladkov <legion@kernel.org> wrote:
>
> --- /dev/null
> +++ b/kernel/bpf/btf_iter.c
> @@ -0,0 +1,2 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +#include "../../tools/lib/bpf/btf_iter.c"

These files are licensed as
// SPDX-License-Identifier: (LGPL-2.1 OR BSD-2-Clause)
in libbpf directory.
Pls use that.

Pls keep acks/reviews-bys/tested-by when you respin.

Thanks

pw-bot: cr
diff mbox series

Patch

diff --git a/kernel/bpf/Makefile b/kernel/bpf/Makefile
index 0291eef9ce92..9b9c151b5c82 100644
--- a/kernel/bpf/Makefile
+++ b/kernel/bpf/Makefile
@@ -52,9 +52,3 @@  obj-$(CONFIG_BPF_PRELOAD) += preload/
 obj-$(CONFIG_BPF_SYSCALL) += relo_core.o
 obj-$(CONFIG_BPF_SYSCALL) += btf_iter.o
 obj-$(CONFIG_BPF_SYSCALL) += btf_relocate.o
-
-# Some source files are common to libbpf.
-vpath %.c $(srctree)/kernel/bpf:$(srctree)/tools/lib/bpf
-
-$(obj)/%.o: %.c FORCE
-	$(call if_changed_rule,cc_o_c)
diff --git a/kernel/bpf/btf_iter.c b/kernel/bpf/btf_iter.c
new file mode 100644
index 000000000000..eab8493a1669
--- /dev/null
+++ b/kernel/bpf/btf_iter.c
@@ -0,0 +1,2 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+#include "../../tools/lib/bpf/btf_iter.c"
diff --git a/kernel/bpf/btf_relocate.c b/kernel/bpf/btf_relocate.c
new file mode 100644
index 000000000000..8c89c7b59ef8
--- /dev/null
+++ b/kernel/bpf/btf_relocate.c
@@ -0,0 +1,2 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+#include "../../tools/lib/bpf/btf_relocate.c"
diff --git a/kernel/bpf/relo_core.c b/kernel/bpf/relo_core.c
new file mode 100644
index 000000000000..6a36fbc0e5ab
--- /dev/null
+++ b/kernel/bpf/relo_core.c
@@ -0,0 +1,2 @@ 
+// SPDX-License-Identifier: GPL-2.0-only
+#include "../../tools/lib/bpf/relo_core.c"