From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752429AbbJUKmB (ORCPT ); Wed, 21 Oct 2015 06:42:01 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:38391 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751545AbbJUKmA (ORCPT ); Wed, 21 Oct 2015 06:42:00 -0400 Message-ID: <56276B22.4050402@huawei.com> Date: Wed, 21 Oct 2015 18:38:26 +0800 From: "Wangnan (F)" User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Ingo Molnar , He Kuang CC: , , , , , , Subject: Re: [RFC PATCH] bpf: Add new bpf map type for timer References: <1445232889-155696-1-git-send-email-hekuang@huawei.com> <562762B8.5060706@huawei.com> <20151021102005.GA14510@gmail.com> In-Reply-To: <20151021102005.GA14510@gmail.com> Content-Type: text/plain; charset="utf-8"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.111.66.109] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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? > > Thanks, > > Ingo