netdev - Re: [PATCH net-next 2/4] bpf: bpf_htab: Add BPF_MAP_TYPE_PERCPU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160112031148.GA13416@kafai-mba.local>
Date:	Mon, 11 Jan 2016 19:11:48 -0800
From:	Martin KaFai Lau <kafai@...com>
To:	Ming Lei <tom.leiming@...il.com>
CC:	Network Development <netdev@...r.kernel.org>,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	FB Kernel Team <kernel-team@...com>,
	Alexei Starovoitov <alexei.starovoitov@...il.com>
Subject: Re: [PATCH net-next 2/4] bpf: bpf_htab: Add BPF_MAP_TYPE_PERCPU_HASH

On Sat, Jan 09, 2016 at 06:06:15PM +0800, Ming Lei wrote:
> On Fri, Jan 8, 2016 at 6:35 AM, Martin KaFai Lau <kafai@...com> wrote:
> > This patch adds BPFMAP_TYPE_PERCPU_HASH map type and its
> > htab_map_ops implementation.
> >
> > Signed-off-by: Martin KaFai Lau <kafai@...com>
> > ---
> >  include/uapi/linux/bpf.h |   1 +
> >  kernel/bpf/hashtab.c     | 201 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  2 files changed, 201 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> > index 8bed7f1..e4f8060 100644
> > --- a/include/uapi/linux/bpf.h
> > +++ b/include/uapi/linux/bpf.h
> > @@ -81,6 +81,7 @@ enum bpf_map_type {
> >         BPF_MAP_TYPE_ARRAY,
> >         BPF_MAP_TYPE_PROG_ARRAY,
> >         BPF_MAP_TYPE_PERF_EVENT_ARRAY,
> > +       BPF_MAP_TYPE_PERCPU_HASH,
> >  };
> >
> >  enum bpf_prog_type {
> > diff --git a/kernel/bpf/hashtab.c b/kernel/bpf/hashtab.c
> > index d55df8c..63f2945 100644
> > --- a/kernel/bpf/hashtab.c
> > +++ b/kernel/bpf/hashtab.c
> > @@ -278,7 +278,7 @@ find_first_elem:
> >  }
> >
> >  static struct htab_elem_common *htab_elem_common_alloc(struct bpf_htab *htab,
> > -                                                       void *key)
> > +                                                      void *key)
>
> better to not introduce the above change.
What is the concern?

>
> >  {
> >         struct htab_elem_common *l;
> >
> > @@ -451,9 +451,208 @@ static struct bpf_map_type_list htab_type __read_mostly = {
> >         .type = BPF_MAP_TYPE_HASH,
> >  };
> >
> > +/* each htab_percpu_elem is struct htab_percpu_elem + key  */
> > +struct htab_percpu_elem {
> > +       struct htab_elem_common common;
> > +       void * __percpu value;
> > +       char key[0] __aligned(8);
> > +};
> > +
> > +static struct htab_percpu_elem *htab_percpu_elem(struct htab_elem_common *l)
> > +{
> > +       return (struct htab_percpu_elem *)l;
> > +}
> > +
> > +static void htab_percpu_elem_free(struct htab_percpu_elem *l)
> > +{
> > +       free_percpu(l->value);
> > +       kfree(l);
> > +}
> > +
> > +static void htab_percpu_elem_rcu_free(struct rcu_head *head)
> > +{
> > +       struct htab_elem_common *l = container_of(head,
> > +                                                 struct htab_elem_common,
> > +                                                 rcu);
> > +
> > +       htab_percpu_elem_free(htab_percpu_elem(l));
> > +}
> > +
> > +static void htab_percpu_map_flush(struct bpf_htab *htab)
> > +{
> > +       int i;
> > +
> > +       for (i = 0; i < htab->n_buckets; i++) {
> > +               struct hlist_head *head = select_bucket(htab, i);
> > +               struct hlist_node *n;
> > +               struct htab_elem_common *l;
> > +
> > +               hlist_for_each_entry_safe(l, n, head, hash_node) {
> > +                       hlist_del_rcu(&l->hash_node);
> > +                       atomic_dec(&htab->count);
> > +                       htab_percpu_elem_free(htab_percpu_elem(l));
> > +               }
> > +       }
> > +}
>
> The above helper should have been saved by introduce percpu_map
> flag in bpf_htab.
There is no need to introduce a new flag. Is it the same as checking
htab->map.map_type == BPF_MAP_TYPE_PERCPU_HASH?

The current 'struct bpf_map_ops' setup has already made a clean function
dispatch based on different 'enum bpf_map_type'.  I have been refraining
to make another map_type check else where again.

I will make another attempt to further remove duplicate code first and
will post it shortly.

Thanks,
-- Martin