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

[Ingo Molnar <mingo@kernel.org>]

* Wangnan (F) <wangnan0@huawei.com> wrote:

> 
> 
> On 2015/10/21 18:20, Ingo Molnar wrote:
> >* He Kuang <hekuang@huawei.com> wrote:
> >
> >>ping and add ast@plumgrid.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,
> 
> Here :)
> 
> >         .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?

Correct, so that was 18 minutes ;-)

The bug should be easier to ffind in this form:

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,
};

as there's a visual anomaly at a glance already, if you look carefully enough.

Agreed? These kinds of visual clues get hidden if the vertical alignment is 
missing.

Thanks,

	Ingo