From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754627AbbJUKrR (ORCPT ); Wed, 21 Oct 2015 06:47:17 -0400 Received: from mail-wi0-f178.google.com ([209.85.212.178]:32967 "EHLO mail-wi0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752480AbbJUKrN (ORCPT ); Wed, 21 Oct 2015 06:47:13 -0400 Date: Wed, 21 Oct 2015 12:47:08 +0200 From: Ingo Molnar To: "Wangnan (F)" Cc: He Kuang , ast@kernel.org, davem@davemloft.net, daniel@iogearbox.net, rostedt@goodmis.org, xiakaixu@huawei.com, ast@plumgrid.com, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH] bpf: Add new bpf map type for timer Message-ID: <20151021104708.GA16402@gmail.com> References: <1445232889-155696-1-git-send-email-hekuang@huawei.com> <562762B8.5060706@huawei.com> <20151021102005.GA14510@gmail.com> <56276B22.4050402@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56276B22.4050402@huawei.com> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org * Wangnan (F) wrote: > > > On 2015/10/21 18:20, Ingo Molnar wrote: > >* He Kuang 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