Messages in this thread | | | Date | Wed, 21 Oct 2015 12:47:08 +0200 | From | Ingo Molnar <> | Subject | Re: [RFC PATCH] bpf: Add new bpf map type for timer |
| |
* 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
| |