linux-kernel - Re: [RFC PATCH] bpf: Add new bpf map type for timer

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: <20151021102005.GA14510@gmail.com>
Date:	Wed, 21 Oct 2015 12:20:05 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	He Kuang <hekuang@...wei.com>
Cc:	ast@...nel.org, davem@...emloft.net, daniel@...earbox.net,
	rostedt@...dmis.org, xiakaixu@...wei.com, ast@...mgrid.com,
	wangnan0@...wei.com, linux-kernel@...r.kernel.org
Subject: Re: [RFC PATCH] bpf: Add new bpf map type for timer


* He Kuang <hekuang@...wei.com> wrote:

> ping and add ast@...mgrid.com, what's your opinion on this?

Firstly, two days isn't nearly enough for a 'review timeout', secondly, have you 
seen the kbuild test reports?

Thirdly, I suspect others will do a deeper review, but even stylistically the 
patch is a bit weird, for example these kinds of unstructured struct initializers 
are annoying:

> >   struct bpf_map_def SEC("maps") timer_map = {
> >   	.type = BPF_MAP_TYPE_TIMER_ARRAY,
> >   	.key_size = sizeof(int),
> >   	.value_size = sizeof(unsigned long long),
> >   	.max_entries = 4,
> >   };

> >  	.map_alloc = fd_array_map_alloc,
> >  	.map_free = fd_array_map_free,
> >  	.map_get_next_key = array_map_get_next_key,
> >-	.map_lookup_elem = fd_array_map_lookup_elem,
> >+	.map_lookup_elem = empty_array_map_lookup_elem,
> >  	.map_update_elem = fd_array_map_update_elem,
> >  	.map_delete_elem = fd_array_map_delete_elem,
> >  	.map_fd_get_ptr = prog_fd_array_get_ptr,
> >@@ -312,7 +318,7 @@ static const struct bpf_map_ops perf_event_array_ops = {
> >  	.map_alloc = fd_array_map_alloc,
> >  	.map_free = perf_event_array_map_free,
> >  	.map_get_next_key = array_map_get_next_key,
> >-	.map_lookup_elem = fd_array_map_lookup_elem,
> >+	.map_lookup_elem = empty_array_map_lookup_elem,
> >  	.map_update_elem = fd_array_map_update_elem,
> >  	.map_delete_elem = fd_array_map_delete_elem,
> >  	.map_fd_get_ptr = perf_event_fd_array_get_ptr,

> >+static const struct bpf_map_ops timer_array_ops = {
> >+	.map_alloc = timer_array_map_alloc,
> >+	.map_free = timer_array_map_free,
> >+	.map_get_next_key = array_map_get_next_key,
> >+	.map_lookup_elem = empty_array_map_lookup_elem,
> >+	.map_update_elem = timer_array_map_update_elem,
> >+	.map_delete_elem = timer_array_map_delete_elem,
> >+};
> >+
> >+static struct bpf_map_type_list timer_array_type __read_mostly = {
> >+	.ops = &timer_array_ops,
> >+	.type = BPF_MAP_TYPE_TIMER_ARRAY,
> >+};

Please align initializations vertically, so the second column becomes readable, 
patterns in them become easy to see and individual entries become easier to 
compare.

See for example kernel/sched/core.c:

struct cgroup_subsys cpu_cgrp_subsys = {
        .css_alloc      = cpu_cgroup_css_alloc,
        .css_free       = cpu_cgroup_css_free,
        .css_online     = cpu_cgroup_css_online,
        .css_offline    = cpu_cgroup_css_offline,
        .fork           = cpu_cgroup_fork,
        .can_attach     = cpu_cgroup_can_attach,
        .attach         = cpu_cgroup_attach,
        .exit           = cpu_cgroup_exit,
        .legacy_cftypes = cpu_files,
        .early_init     = 1,
};

That's a _lot_ more readable than:

struct cgroup_subsys cpu_cgrp_subsys = {
        .css_alloc = cpu_cgroup_css_alloc,
        .css_free = cpu_cgroup_css_free,
        .css_online = cpu_cgroup_css_online,
        .css_offline = cpu_cgroup_css_offline,
        .fork = cpu_cgroup_fork,
        .can_attach = cpu_cgroup_attach,
        .attach = cpu_cgroup_attach,
        .exit = cpu_cgroup_exit,
        .legacy_cftypes = cpu_files,
        .early_init = 1,
};

right? For example I've hidden a small initialization bug into the second variant, 
how much time does it take for you to notice it?

Thanks,

	Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/