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 [off-list ref] wrote:

quoted

This patch adds BPFMAP_TYPE_PERCPU_HASH map type and its
htab_map_ops implementation.

Signed-off-by: Martin KaFai Lau <redacted>
---
 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?

quoted

 {
        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