Re: [PATCH bpf-next V1] bpf: devmap dynamic map-value area based on BTF

From: Jesper Dangaard Brouer <brouer@redhat.com>
To: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Cc: David Ahern <dsahern@gmail.com>,
	bpf@vger.kernel.org, netdev@vger.kernel.org,
	Daniel Borkmann <borkmann@iogearbox.net>,
	Andrii Nakryiko <andrii.nakryiko@gmail.com>,
	Lorenzo Bianconi <lorenzo@kernel.org>,
	brouer@redhat.com, David Miller <davem@davemloft.net>
Subject: Re: [PATCH bpf-next V1] bpf: devmap dynamic map-value area based on BTF
Date: Thu, 4 Jun 2020 17:48:06 +0200	[thread overview]
Message-ID: <20200604174806.29130b81@carbon> (raw)
In-Reply-To: <20200603162257.nxgultkidnb7yb6q@ast-mbp.dhcp.thefacebook.com>

On Wed, 3 Jun 2020 09:22:57 -0700
Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote:

> On Wed, Jun 03, 2020 at 05:44:43PM +0200, Jesper Dangaard Brouer wrote:
> > The recent commit fbee97feed9b ("bpf: Add support to attach bpf program to a
> > devmap entry"), introduced ability to attach (and run) a separate XDP
> > bpf_prog for each devmap entry. A bpf_prog is added via a file-descriptor,
> > thus not using the feature requires using value minus-1. The UAPI is
> > extended via tail-extending struct bpf_devmap_val and using map->value_size
> > to determine the feature set.
> > 
> > There is a specific problem with dev_map_can_have_prog() check, which is
> > called from net/core/dev.c in generic_xdp_install() to refuse usage of
> > devmap's from generic-XDP that support these bpf_prog's. The check is size
> > based. This means that all newer features will be blocked from being use by
> > generic-XDP.
> > 
> > This patch allows userspace to skip handling of 'bpf_prog' on map-inserts.
> > The feature can be skipped, via not including the member 'bpf_prog' in the
> > map-value struct, which is propagated/described via BTF.
> > 
> > Fixes: fbee97feed9b ("bpf: Add support to attach bpf program to a devmap entry")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com  
> 
> The patch makes no sense to me.

Hmm, that is not a very constructive answer, and it doesn't help me
to improve and move forward with the code.  I interpret that you think
my approach is completely wrong, but it would have been nice to
understand why.  I will give up on this approach, also given bpf-next
is closed now.

> please expose 'struct struct bpf_devmap_val' in uapi/bpf.h
> That's what it is whether you want to acknowledge that or not.

I will NOT send a patch that expose this in uapi/bpf.h.  As I explained
before, this caused the issues for my userspace application, that
automatically picked-up struct bpf_devmap_val, and started to fail
(with no code changes), because it needed minus-1 as input.  I fear
that this will cause more work for me later, when I have to helpout and
support end-users on e.g. xdp-newbies list, as it will not be obvious
to end-users why their programs map-insert start to fail.  I have given
up, so I will not NACK anyone sending such a patch.

Why is it we need to support file-descriptor zero as a valid
file-descriptor for a bpf-prog?

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer