From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932375AbbJUKUM (ORCPT ); Wed, 21 Oct 2015 06:20:12 -0400 Received: from mail-wi0-f179.google.com ([209.85.212.179]:35504 "EHLO mail-wi0-f179.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753708AbbJUKUK (ORCPT ); Wed, 21 Oct 2015 06:20:10 -0400 Date: Wed, 21 Oct 2015 12:20:05 +0200 From: Ingo Molnar To: He Kuang Cc: ast@kernel.org, davem@davemloft.net, daniel@iogearbox.net, rostedt@goodmis.org, xiakaixu@huawei.com, ast@plumgrid.com, wangnan0@huawei.com, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH] bpf: Add new bpf map type for timer Message-ID: <20151021102005.GA14510@gmail.com> References: <1445232889-155696-1-git-send-email-hekuang@huawei.com> <562762B8.5060706@huawei.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <562762B8.5060706@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 * 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, .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