Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1580109 > unrolled thread

linux-next: build failure after merge of the net tree

Back to article view | Back to linux.kernel

Contents

  linux-next: build failure after merge of the net tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-02-13 23:20 +0100
    Re: linux-next: build failure after merge of the net tree Ingo Molnar <mingo@kernel.org> - 2017-02-14 07:40 +0100
    Re: linux-next: build failure after merge of the net tree Ingo Molnar <mingo@kernel.org> - 2017-02-14 07:50 +0100
      Re: linux-next: build failure after merge of the net tree Jiri Olsa <jolsa@redhat.com> - 2017-02-14 10:20 +0100
        Re: linux-next: build failure after merge of the net tree Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-14 14:00 +0100
          Re: linux-next: build failure after merge of the net tree Jiri Olsa <jolsa@redhat.com> - 2017-02-14 14:30 +0100
            Re: linux-next: build failure after merge of the net tree Arnaldo Carvalho de Melo <acme@kernel.org> - 2017-02-14 15:00 +0100
    Re: linux-next: build failure after merge of the net tree Joe Stringer <joe@ovn.org> - 2017-02-14 09:10 +0100
    Re: linux-next: build failure after merge of the net tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-02-16 04:10 +0100
      Re: linux-next: build failure after merge of the net tree David Miller <davem@davemloft.net> - 2017-02-16 04:40 +0100
        Re: linux-next: build failure after merge of the net tree Stephen Rothwell <sfr@canb.auug.org.au> - 2017-02-16 04:50 +0100

#1580109 — linux-next: build failure after merge of the net tree

Hi all,

After merging the net tree, today's linux-next build (powerpc64le perf)
failed like this:

Warning: tools/include/uapi/linux/bpf.h differs from kernel
bpf.c: In function 'bpf_prog_attach':
bpf.c:180:6: error: 'union bpf_attr' has no member named 'attach_flags'; did you mean 'map_flags'?
  attr.attach_flags  = flags;
      ^

Caused by commit

  7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE flag")

Unfortunately, the perf header files are kept separate from the kernel
header files proper and are not automatically copied over :-(

I have applied the following build fix patch for today.

From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Tue, 14 Feb 2017 08:22:20 +1100
Subject: [PATCH] bpf: kernel header files need to be copied into the tools directory

Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
 tools/include/uapi/linux/bpf.h | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index 0eb0e87dbe9f..d2b0ac799d03 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -116,6 +116,12 @@ enum bpf_attach_type {
 
 #define MAX_BPF_ATTACH_TYPE __MAX_BPF_ATTACH_TYPE
 
+/* If BPF_F_ALLOW_OVERRIDE flag is used in BPF_PROG_ATTACH command
+ * to the given target_fd cgroup the descendent cgroup will be able to
+ * override effective bpf program that was inherited from this cgroup
+ */
+#define BPF_F_ALLOW_OVERRIDE	(1U << 0)
+
 #define BPF_PSEUDO_MAP_FD	1
 
 /* flags for BPF_MAP_UPDATE_ELEM command */
@@ -171,6 +177,7 @@ union bpf_attr {
 		__u32		target_fd;	/* container object to attach to */
 		__u32		attach_bpf_fd;	/* eBPF program to attach */
 		__u32		attach_type;
+		__u32		attach_flags;
 	};
 } __attribute__((aligned(8)));
 
-- 
2.10.2

-- 
Cheers,
Stephen Rothwell

[toc] | [next] | [standalone]

#1580326

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> --- a/tools/include/uapi/linux/bpf.h
> +++ b/tools/include/uapi/linux/bpf.h
> @@ -116,6 +116,12 @@ enum bpf_attach_type {
>  
>  #define MAX_BPF_ATTACH_TYPE __MAX_BPF_ATTACH_TYPE
>  
> +/* If BPF_F_ALLOW_OVERRIDE flag is used in BPF_PROG_ATTACH command
> + * to the given target_fd cgroup the descendent cgroup will be able to
> + * override effective bpf program that was inherited from this cgroup
> + */
> +#define BPF_F_ALLOW_OVERRIDE	(1U << 0)
> +

BTW., guys, for heaven's sake, please use the standard (multi-line) comment style:

  /*
   * Comment .....
   * ...... goes here.
   */

specified in Documentation/CodingStyle...

It's not that hard to create visually balanced patterns.

Thanks,

        Ingo

[toc] | [prev] | [next] | [standalone]

#1580334

* Stephen Rothwell <sfr@canb.auug.org.au> wrote:

> Hi all,
> 
> After merging the net tree, today's linux-next build (powerpc64le perf)
> failed like this:
> 
> Warning: tools/include/uapi/linux/bpf.h differs from kernel
> bpf.c: In function 'bpf_prog_attach':
> bpf.c:180:6: error: 'union bpf_attr' has no member named 'attach_flags'; did you mean 'map_flags'?
>   attr.attach_flags  = flags;
>       ^
> 
> Caused by commit
> 
>   7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE flag")
> 
> Unfortunately, the perf header files are kept separate from the kernel
> header files proper and are not automatically copied over :-(

No, that's wrong, the problem is not that headers were not shared, the problem is 
that a tooling interdependency was not properly tested *and* that the dependency 
was not properly implemented in the build system either.

Note that we had similar build breakages when include headers _were_ shared as 
well, so sharing the headers would only have worked around this particular bug and 
would have introduced fragility in other places...

The best, most robust solution in this particular case would be to fix the 
(tooling) build system to express the dependency, that would have shown the build 
failure right when the modification was done.

Thanks,

	Ingo

[toc] | [prev] | [next] | [standalone]

#1580419

On Tue, Feb 14, 2017 at 07:42:21AM +0100, Ingo Molnar wrote:
> 
> * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> 
> > Hi all,
> > 
> > After merging the net tree, today's linux-next build (powerpc64le perf)
> > failed like this:
> > 
> > Warning: tools/include/uapi/linux/bpf.h differs from kernel
> > bpf.c: In function 'bpf_prog_attach':
> > bpf.c:180:6: error: 'union bpf_attr' has no member named 'attach_flags'; did you mean 'map_flags'?
> >   attr.attach_flags  = flags;
> >       ^
> > 
> > Caused by commit
> > 
> >   7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE flag")
> > 
> > Unfortunately, the perf header files are kept separate from the kernel
> > header files proper and are not automatically copied over :-(
> 
> No, that's wrong, the problem is not that headers were not shared, the problem is 
> that a tooling interdependency was not properly tested *and* that the dependency 
> was not properly implemented in the build system either.
> 
> Note that we had similar build breakages when include headers _were_ shared as 
> well, so sharing the headers would only have worked around this particular bug and 
> would have introduced fragility in other places...
> 
> The best, most robust solution in this particular case would be to fix the 
> (tooling) build system to express the dependency, that would have shown the build 
> failure right when the modification was done.

so we have the warning now:
  Warning: tools/include/uapi/linux/bpf.h differs from kernel

do you want to change it into the build failure?

jirka

[toc] | [prev] | [next] | [standalone]

#1580524

Em Tue, Feb 14, 2017 at 10:19:37AM +0100, Jiri Olsa escreveu:
> On Tue, Feb 14, 2017 at 07:42:21AM +0100, Ingo Molnar wrote:
> > * Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > Unfortunately, the perf header files are kept separate from the kernel
> > > header files proper and are not automatically copied over :-(

> > No, that's wrong, the problem is not that headers were not shared, the problem is 
> > that a tooling interdependency was not properly tested *and* that the dependency 
> > was not properly implemented in the build system either.

> > Note that we had similar build breakages when include headers _were_ shared as 
> > well, so sharing the headers would only have worked around this particular bug and 
> > would have introduced fragility in other places...

> > The best, most robust solution in this particular case would be to fix the 
> > (tooling) build system to express the dependency, that would have shown the build 
> > failure right when the modification was done.
 
> so we have the warning now:
>   Warning: tools/include/uapi/linux/bpf.h differs from kernel
 
> do you want to change it into the build failure?

No. Differences in the copy are not always problematic, the problem here
lies elsewhere.

Please run:

  make -C tools all

To build all tools when you touch something in tools/include and/or
tools/lib/

- Arnaldo



Bored? Here is what I first wrote ;-)

Simply using the kernel original would require kernel hackers to build
all tools using that file, something we long decided not to do.

What I think Ingo meant with dependency at the build system level is to
somehow state that if file A gets changed, then tool B must be rebuilt.

Now that samples/bpf and tools/perf/ depend on tools/lib/bpf/ I _always_
build both, ditto for tools/objtool, that shares a different library
with tools/perf/, tools/lib/subcmd/:

ENTRYPOINT make -C /git/linux/tools/perf O=/tmp/build/perf && \
           rm -rf /tmp/build/perf/{.[^.]*,*} && \
           make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf && \
           make -C /git/linux/tools/objtool O=/tmp/build/objtool && \
           make -C /git/linux O=/tmp/build/linux allmodconfig && \
           make -C /git/linux O=/tmp/build/linux headers_install && \
           make -C /git/linux O=/tmp/build/linux samples/bpf/

This is the default action for my
docker.io/acmel/linux-perf-tools-build-fedora:rawhide container.

It is published, so a:

   docker pull docker.io/acmel/linux-perf-tools-build-fedora:rawhide

And then run it before pushing things upstream would catch these kinds
of errors.

But that would possibly disrupt too much people's workflow, that is why
using the Kbuild originated tools/build/ we have to somehow express that
when a change is made in a file then a tool that uses that file needs to
be rebuilt.

Makefile rules probably would be enough, but then it would have to be
done at the tools/build/ level and all tools using shared components
would have to use it to trigger the rebuild.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1580533

On Tue, Feb 14, 2017 at 09:50:20AM -0300, Arnaldo Carvalho de Melo wrote:

SNIP

> 
> What I think Ingo meant with dependency at the build system level is to
> somehow state that if file A gets changed, then tool B must be rebuilt.
> 
> Now that samples/bpf and tools/perf/ depend on tools/lib/bpf/ I _always_
> build both, ditto for tools/objtool, that shares a different library
> with tools/perf/, tools/lib/subcmd/:
> 
> ENTRYPOINT make -C /git/linux/tools/perf O=/tmp/build/perf && \
>            rm -rf /tmp/build/perf/{.[^.]*,*} && \
>            make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf && \
>            make -C /git/linux/tools/objtool O=/tmp/build/objtool && \
>            make -C /git/linux O=/tmp/build/linux allmodconfig && \
>            make -C /git/linux O=/tmp/build/linux headers_install && \
>            make -C /git/linux O=/tmp/build/linux samples/bpf/
> 
> This is the default action for my
> docker.io/acmel/linux-perf-tools-build-fedora:rawhide container.
> 
> It is published, so a:
> 
>    docker pull docker.io/acmel/linux-perf-tools-build-fedora:rawhide
> 
> And then run it before pushing things upstream would catch these kinds
> of errors.
> 
> But that would possibly disrupt too much people's workflow, that is why
> using the Kbuild originated tools/build/ we have to somehow express that
> when a change is made in a file then a tool that uses that file needs to
> be rebuilt.

we already have the check in the check-headers.sh script,
an AFAICS there's no 'rebuild' option here.. just warn or fail
because the headers update needs to be done manualy

> 
> Makefile rules probably would be enough, but then it would have to be
> done at the tools/build/ level and all tools using shared components
> would have to use it to trigger the rebuild.

we can move/invoke the check-headers.sh script in some upper dir

jirka

[toc] | [prev] | [next] | [standalone]

#1580561

Em Tue, Feb 14, 2017 at 02:23:26PM +0100, Jiri Olsa escreveu:
> On Tue, Feb 14, 2017 at 09:50:20AM -0300, Arnaldo Carvalho de Melo wrote:
> 
> SNIP
> 
> > 
> > What I think Ingo meant with dependency at the build system level is to
> > somehow state that if file A gets changed, then tool B must be rebuilt.
> > 
> > Now that samples/bpf and tools/perf/ depend on tools/lib/bpf/ I _always_
> > build both, ditto for tools/objtool, that shares a different library
> > with tools/perf/, tools/lib/subcmd/:
> > 
> > ENTRYPOINT make -C /git/linux/tools/perf O=/tmp/build/perf && \
> >            rm -rf /tmp/build/perf/{.[^.]*,*} && \
> >            make NO_LIBELF=1 -C /git/linux/tools/perf O=/tmp/build/perf && \
> >            make -C /git/linux/tools/objtool O=/tmp/build/objtool && \
> >            make -C /git/linux O=/tmp/build/linux allmodconfig && \
> >            make -C /git/linux O=/tmp/build/linux headers_install && \
> >            make -C /git/linux O=/tmp/build/linux samples/bpf/
> > 
> > This is the default action for my
> > docker.io/acmel/linux-perf-tools-build-fedora:rawhide container.
> > 
> > It is published, so a:
> > 
> >    docker pull docker.io/acmel/linux-perf-tools-build-fedora:rawhide
> > 
> > And then run it before pushing things upstream would catch these kinds
> > of errors.
> > 
> > But that would possibly disrupt too much people's workflow, that is why
> > using the Kbuild originated tools/build/ we have to somehow express that
> > when a change is made in a file then a tool that uses that file needs to
> > be rebuilt.
> 
> we already have the check in the check-headers.sh script,
> an AFAICS there's no 'rebuild' option here.. just warn or fail
> because the headers update needs to be done manualy

... when needed. And that will only be detected if you try to build
tools using what is in tools/include/linux/bpf.h

Tools using tools/lib/bpf/ _must_ use what is in tools/include/.

So lemme see if my reasoning is right:

tools/lib/bpf/bpf.c has:

  #include <linux/bpf.h>

Now, samples/bpf/ will build tools/lib/bpf/bpf.o:

# Libbpf dependencies
LIBBPF := ../../tools/lib/bpf/bpf.o

HOSTCFLAGS += -I$(objtree)/usr/include
HOSTCFLAGS += -I$(srctree)/tools/lib/
HOSTCFLAGS += -I$(srctree)/tools/testing/selftests/bpf/
HOSTCFLAGS += -I$(srctree)/tools/lib/ -I$(srctree)/tools/include
HOSTCFLAGS += -I$(srctree)/tools/perf

HOSTCFLAGS_bpf_load.o += -I$(objtree)/usr/include -Wno-unused-variable

So it will never include tools/include/uapi/linux/bpf.h, which it
should.

Because the workflow people working on sample/bpf/ is to first install
the new headers using a variation of:

  make headers_install

So they will get the new bpf.h, not use tools/include/uapi/linux/bpf.h,
b00m.

They should use tools/include/uapi/linux/bpf.h, which is the one we know
builds well with tools/lib/bpf/bpf.c, since we tested it last time we
made the copy.
 
> > Makefile rules probably would be enough, but then it would have to be
> > done at the tools/build/ level and all tools using shared components
> > would have to use it to trigger the rebuild.
 
> we can move/invoke the check-headers.sh script in some upper dir

Most of the time I just ignore that warning, only when I find spare time
I go look if the changes in the kernel copy, i.e. upstream, should
trigger changes in the tools using its copy in tools/include/.

- Arnaldo

[toc] | [prev] | [next] | [standalone]

#1580371

On 13 February 2017 at 15:31, Alexei Starovoitov <ast@fb.com> wrote:
> On 2/13/17 2:12 PM, Stephen Rothwell wrote:
>>
>> Hi all,
>>
>> After merging the net tree, today's linux-next build (powerpc64le perf)
>> failed like this:
>>
>> Warning: tools/include/uapi/linux/bpf.h differs from kernel
>> bpf.c: In function 'bpf_prog_attach':
>> bpf.c:180:6: error: 'union bpf_attr' has no member named 'attach_flags';
>> did you mean 'map_flags'?
>>    attr.attach_flags  = flags;
>>        ^
>>
>> Caused by commit
>>
>>    7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE flag")
>>
>> Unfortunately, the perf header files are kept separate from the kernel
>> header files proper and are not automatically copied over :-(
>>
>> I have applied the following build fix patch for today.
>
>
> Yes. Thanks for the fix. It's more than a merge conflict.
> I should have added it in the first place. Now we have both
> perf and samples/bpf depend on tools/lib/bpf and I simply
> forgot about this dependency, since building perf
> is not my typical workflow.
>
> Joe,
> can you think of a way to make tools/lib/bpf to
> use tools/include only?
> Right now we just pull tools/lib/bpf/bpf.o in samples/bpf/Makefile
> and that's a hack that caused this issue.
> samples/bpf/ needs to depend on libbpf.a properly.

Honestly the build system stuff is all black magic to me.

Originally I had a line in the samples/bpf makefile that changed
directory and ran make there, but it was limited in that it only
addressed the 'all' target so you could still hit issues when trying
to build specific samples directly via their own targets so I removed
it on request:

https://patchwork.kernel.org/patch/9472573/

[toc] | [prev] | [next] | [standalone]

#1582290

Hi all,

On Tue, 14 Feb 2017 09:12:50 +1100 Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> After merging the net tree, today's linux-next build (powerpc64le perf)
> failed like this:
> 
> Warning: tools/include/uapi/linux/bpf.h differs from kernel
> bpf.c: In function 'bpf_prog_attach':
> bpf.c:180:6: error: 'union bpf_attr' has no member named 'attach_flags'; did you mean 'map_flags'?
>   attr.attach_flags  = flags;
>       ^
> 
> Caused by commit
> 
>   7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE flag")
> 

So do we have a fix for this?  I am sure that Dave would like to send
his "net" tree to Linus sometime soonish ...

-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [next] | [standalone]

#1582298

From: Alexei Starovoitov <ast@fb.com>
Date: Wed, 15 Feb 2017 19:06:02 -0800

> On 2/15/17 7:02 PM, Stephen Rothwell wrote:
>> Hi all,
>>
>> On Tue, 14 Feb 2017 09:12:50 +1100 Stephen Rothwell
>> <sfr@canb.auug.org.au> wrote:
>>>
>>> After merging the net tree, today's linux-next build (powerpc64le
>>> perf)
>>> failed like this:
>>>
>>> Warning: tools/include/uapi/linux/bpf.h differs from kernel
>>> bpf.c: In function 'bpf_prog_attach':
>>> bpf.c:180:6: error: 'union bpf_attr' has no member named
>>> 'attach_flags'; did you mean 'map_flags'?
>>>    attr.attach_flags  = flags;
>>>        ^
>>>
>>> Caused by commit
>>>
>>>    7f677633379b ("bpf: introduce BPF_F_ALLOW_OVERRIDE flag")
>>>
>>
>> So do we have a fix for this?  I am sure that Dave would like to send
>> his "net" tree to Linus sometime soonish ...
> 
> Do you mind resending it to netdev with my Ack ?
> please mention [PATCH net] in subj, so it's get caught by Dave's
> scripts.

I applied the fix that synced the two header files already.

[toc] | [prev] | [next] | [standalone]

#1582302

Hi Dave,

On Wed, 15 Feb 2017 22:27:46 -0500 (EST) David Miller <davem@davemloft.net> wrote:
>
> I applied the fix that synced the two header files already.

Sorry about that, I missed it in checking your tree over breakfast :-)

-- 
Cheers,
Stephen Rothwell

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web