On 2015/10/21 18:20, Ingo Molnar wrote:
* He Kuang <[email protected]> wrote:
ping and add [email protected], 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?
Thanks,
Ingo
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
