Re: [PATCH RFC v4 bpf-next 00/11] Add support for XDP in egress path

From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
To: "Toke Høiland-Jørgensen" <toke@redhat.com>
Cc: David Ahern <dsahern@kernel.org>,
	Network Development <netdev@vger.kernel.org>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Prashant Bhole <prashantbhole.linux@gmail.com>,
	Jason Wang <jasowang@redhat.com>,
	Jesper Dangaard Brouer <brouer@redhat.com>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Toshiaki Makita <toshiaki.makita1@gmail.com>,
	Daniel Borkmann <daniel@iogearbox.net>,
	John Fastabend <john.fastabend@gmail.com>,
	Alexei Starovoitov <ast@kernel.org>,
	Martin KaFai Lau <kafai@fb.com>, Song Liu <songliubraving@fb.com>,
	Yonghong Song <yhs@fb.com>, Andrii Nakryiko <andriin@fb.com>,
	David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH RFC v4 bpf-next 00/11] Add support for XDP in egress path
Date: Thu, 27 Feb 2020 10:37:03 -0800	[thread overview]
Message-ID: <20200227183702.5aaprwthg3tvuvyu@ast-mbp> (raw)
In-Reply-To: <87r1ygufgu.fsf@toke.dk>

On Thu, Feb 27, 2020 at 06:06:57PM +0100, Toke Høiland-Jørgensen wrote:
> Alexei Starovoitov <alexei.starovoitov@gmail.com> writes:
> 
> > On Thu, Feb 27, 2020 at 3:55 AM Toke Høiland-Jørgensen <toke@redhat.com> wrote:
> >>
> >> However, my issue with this encoding is that it is write-only: You can't
> >> inspect a BPF program already loaded into the kernel and tell which type
> >> it is. So my proposal would be to make it explicit: Expose the
> >> expected_attach_type as a new field in bpf_prog_info so userspace can
> >> query it, and clearly document it as, essentially, a program subtype
> >> that can significantly affect how a program is treated by the kernel.
> >
> > You had the same request for "freplace" target prog.
> 
> Yes, and for the same reason; I'm being consistent here :)
> 
> > My answer to both is still the same:
> > Please take a look at drgn and the script that Andrey posted.
> > All this information is trivial to extract from the kernel
> > without introducing new uapi.
> 
> I'm sorry, but having this kind of write-only data structure is a
> horrible API design; and saying "you can just parse the internal kernel
> data structures to see what is going on" is a cop-out. The whole point
> of having a stable UAPI is to make it possible for people to write
> applications that make use of kernel features with an expectation that
> those applications will keep working. XDP is a networking feature;
> people building networking applications shouldn't have to chase internal
> kernel APIs just to keep their packet processing programs working.

You're mistaking human needs for tooling needs.
Humans want to see all sorts of internal things and they can tweak drgn
script to look at them on the live kernel and examine kdump's vmcore
with the same drgn script.
Tooling needs uapi to work on a live system.
There are 26 attach_types that are used by various cgroup based progs,
sockmap, lirc, flow_dissector and tracing. Many of these attach types
are used in production and not a single time _tools_ had a need to
retrieve that field from the kernel.
Show me the tool that needs to read expected_attach_type back and
then we can discuss how to expose it in uapi.

There is another side of this. struct bpf_prog->pages field currently
is not exposed in uapi and I've seen tools doing
cat /proc/pid/fdinfo/fd|grep "memlock:"
just to retrive 'pages' field. That's the case where 'struct bpf_prog_info'
should be extended.