Message-ID: <20210428045545.egqvhyulr4ybbad6@ast-mbp.dhcp.thefacebook.com>
Date:   Tue, 27 Apr 2021 21:55:45 -0700
From:   Alexei Starovoitov <alexei.starovoitov@...il.com>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Yonghong Song <yhs@...com>, Andrii Nakryiko <andrii@...nel.org>,
        bpf <bpf@...r.kernel.org>, Networking <netdev@...r.kernel.org>,
        Alexei Starovoitov <ast@...com>,
        Daniel Borkmann <daniel@...earbox.net>,
        Kernel Team <kernel-team@...com>
Subject: Re: [PATCH v2 bpf-next 2/6] libbpf: rename static variables during
 linking

On Tue, Apr 27, 2021 at 02:27:26PM -0700, Andrii Nakryiko wrote:
> 
> It's static for the purposes of BPF code, so no naming collisions in
> BPF code and no intentional or unintentional accesses beyond a single
> .bpf.c file. That's what we want. We don't want BPF library writers to
> worry about names colliding with some other library or user code. 

Who is 'we' ?
The skel gen by accident (probably) extended 'static' visibility
from .bpf.c into user space .c.
My point that it was a bad accident and I only realized it now.
The naming conflict in linking exposed this problem.
The selftests are relying on this 'feature' now.
Potentially some user could have used it as well.
Do I want to break that use case? Not really, but that's still an option.

I agree that library writers shouldn't worry about
naming conflicts in *.bpf.c space.
If they're exporting a static from .bpf.c into .c I think it's ok
to make them do extra steps.
Such 'static in .bpf.c' but 'extern into .c' should somehow work
without requiring people rename their vars.
I mean that the user of the library shouldn't need to do renames,
but the library author shouldn't rely on 'all statics are visible
in skel'.
In that sense what I proposed earlier to allow linking, but fail
skel gen is a step towards such development process.
Something like attr(alias()) or some other way hopefully can
help library authors create such library where static+something
is visible to library's skeleton, but users of the library
don't see its statics.
I think the static handling logic needs to be discussed
with your sub-skeleton idea.
If I got it correctly there will be something like this:
- lib.bpf.c compiled into lib.bpf.o
- main.bpf.c that links with lib.bpf.o
  It's all in *.bpf.c space and static has normal C scope.
  The static vars and maps in lib.bpf.c are not
  visible in main.bpf.c
- there is lib.c that works with lib.bpf.c via lib.skel.h
  that was generated out of lib.bpf.o
- main.c that links with lib.o
  main.c works with main.bpf.c via main.skel.h

I think lib.skel.h you were calling sub-skeleton.
pls correct me.
Since main.bpf.o was linked with lib.bpf.o
the main.skel.h will include the things from it.
But main.c shouldn't be accessing them, since that's the
point of the library.

At the same time lib.bpf.c and main.bpf.c could have been
just two files of the same project. If lib.bpf.o isn't a library
then main.skel.h should access it just fine.
So what is bpf library? How should it be defined?
And what is the scope and visibility of its vars/maps/funcs?
Unlike traditional C the bpf has two worlds .bpf.c and .c
So traditional 'static' doesn't cover these cases.

> And
> from the perspective of a user of two BPF libraries that have
> colliding names it's not great to have to somehow rename those
> libraries' internal variables through source code changes.

It's not great. That's why I'm trying to provoke a discussion
of more options and pick the best considering all + and -.

> 
> Omitting static variables from skeleton is a regression and will
> surprise existing users, we already went over this with you and
> Yonghong in previous emails.

Do I want to suffer this regression? No, but it could be the only option.

> Beyond that, it's not clear what exactly you are proposing. 

To discuss all options as a whole and hopefully you and others
can come up with more than what I proposed.

> For
> alias() seems like another variable with that "external_name" has to
> be already defined and you can't initialize var, it has to be just a
> declaration. And BTF doesn't capture attributes right now as well. And
> overall it sounds like an overly complicated approach both for users
> and for libbpf.

yes. supporting alias() would mean more work in clang, libbpf and
maybe new bits in BTF.

> As for the extra SEC() annotation. It's both not supported by libbpf
> right now, and it's not exactly clear how it helps with name conflicts
> (see example above with two libraries colliding). In that regard a
> prefix and ability to override it by user gives them an opportunity to
> resolve such naming conflicts. I know it's kind of ugly, but name
> overrides should hopefully be rarely needed.

Yes. it's not supported today. All I'm saying it's one of the options.

> What can I do to unblock BPF static linker work, though?

I believe that the way bpf toolchain interprets static is
a critical long term decision that shouldn't be done lightly.
There is no rush to define it quickly as an automatic prefix of filename
to all statics only because it's trivial to implement and sort-of works.
It's not something we can undo later. Today there are no libraries
and static definition of maps doesn't really work.
So the only regression (if we decide to change) would be the way skeleton
emits statics.

> I don't think we'll find an ideal solution that will satisfy everyone.

I think we didn't even start looking for that solution.
At least I'm only starting to grasp the complexity of the problem.

Powered by blists - more mailing lists