From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jakub Kicinski Subject: Re: [PATCH net-next v2 03/15] bpf: report offload info to user space Date: Sat, 4 Nov 2017 03:32:46 -0700 Message-ID: <20171104033246.0f65f853@cakuba.lan> References: <20171103205630.1083-1-jakub.kicinski@netronome.com> <20171103205630.1083-4-jakub.kicinski@netronome.com> <20171104094528.fbrhgkqwsvre2t3u@ast-mbp> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Cc: netdev@vger.kernel.org, oss-drivers@netronome.com, daniel@iogearbox.net To: Alexei Starovoitov Return-path: Received: from mx3.wp.pl ([212.77.101.9]:19095 "EHLO mx3.wp.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750750AbdKDKc4 (ORCPT ); Sat, 4 Nov 2017 06:32:56 -0400 In-Reply-To: <20171104094528.fbrhgkqwsvre2t3u@ast-mbp> Sender: netdev-owner@vger.kernel.org List-ID: On Sat, 4 Nov 2017 18:45:31 +0900, Alexei Starovoitov wrote: > On Fri, Nov 03, 2017 at 01:56:18PM -0700, Jakub Kicinski wrote: > > Extend struct bpf_prog_info to contain information about program > > being bound to a device. Since the netdev may get destroyed while > > program still exists we need a flag to indicate the program is > > loaded for a device, even if the device is gone. > > > > Signed-off-by: Jakub Kicinski > > Reviewed-by: Simon Horman > > Reviewed-by: Quentin Monnet > > --- > > include/linux/bpf.h | 1 + > > include/uapi/linux/bpf.h | 6 ++++++ > > kernel/bpf/offload.c | 12 ++++++++++++ > > kernel/bpf/syscall.c | 5 +++++ > > 4 files changed, 24 insertions(+) > > > > diff --git a/include/linux/bpf.h b/include/linux/bpf.h > > index e45d43f9ec92..98bacd0fa5cc 100644 > > --- a/include/linux/bpf.h > > +++ b/include/linux/bpf.h > > @@ -506,6 +506,7 @@ static inline int cpu_map_enqueue(struct bpf_cpu_map_entry *rcpu, > > > > int bpf_prog_offload_compile(struct bpf_prog *prog); > > void bpf_prog_offload_destroy(struct bpf_prog *prog); > > +u32 bpf_prog_offload_ifindex(struct bpf_prog *prog); > > > > #if defined(CONFIG_NET) && defined(CONFIG_BPF_SYSCALL) > > int bpf_prog_offload_init(struct bpf_prog *prog, union bpf_attr *attr); > > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h > > index 727a3dba13e6..e92f62cf933a 100644 > > --- a/include/uapi/linux/bpf.h > > +++ b/include/uapi/linux/bpf.h > > @@ -894,6 +894,10 @@ enum sk_action { > > > > #define BPF_TAG_SIZE 8 > > > > +enum bpf_prog_status { > > + BPF_PROG_STATUS_DEV_BOUND = (1 << 0), > > +}; > > + > > struct bpf_prog_info { > > __u32 type; > > __u32 id; > > @@ -907,6 +911,8 @@ struct bpf_prog_info { > > __u32 nr_map_ids; > > __aligned_u64 map_ids; > > char name[BPF_OBJ_NAME_LEN]; > > + __u32 ifindex; > > + __u32 status; > > why status is needed? > ifindex cannot be zero, so if it's set > 0 would mean > that the program is bound. Devices may come and go, independently from the lifetime of the program, therefore there is a notion of a program which has been loaded for a particular device but the device is gone (and therefore its ifindex is meaningless). I tried to explain this in the commit message. > Also would be good to have consistent name with prog_load. > imo prog_target_ifindex is too long. > May be call it 'ifindex' both in bpf_attr and in bpf_prog_info ? Perhaps I'm missing something, but bpf_attr is a huge union of (mostly) unnamed anonymous structs. I foresee that we will have to add an ifindex member for a map command as well, therefore the prog_* prefix seems prudent. Should I go back to prog_ifindex in bpf_attr? Or perhaps should I duplicate the struct for BPF_PROG_LOAD but this time give it a member name so we can extend it without worrying about member name conflicts?