From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx0a-00082601.pphosted.com ([67.231.145.42]:48060 "EHLO mx0a-00082601.pphosted.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751714AbeCVPwP (ORCPT ); Thu, 22 Mar 2018 11:52:15 -0400 Subject: Re: [PATCH v2 bpf-next 4/8] tracepoint: compute num_args at build time To: Steven Rostedt References: <20180321185448.2806324-1-ast@fb.com> <20180321185448.2806324-5-ast@fb.com> <2559d7cb-ec60-1200-2362-04fa34fd02bb@fb.com> <20180322093605.51b067fe@gandalf.local.home> CC: Linus Torvalds , David Miller , Daniel Borkmann , Peter Zijlstra , Network Development , kernel-team , Linux API From: Alexei Starovoitov Message-ID: Date: Thu, 22 Mar 2018 08:51:16 -0700 MIME-Version: 1.0 In-Reply-To: <20180322093605.51b067fe@gandalf.local.home> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit Sender: netdev-owner@vger.kernel.org List-ID: On 3/22/18 6:36 AM, Steven Rostedt wrote: > On Wed, 21 Mar 2018 15:05:46 -0700 > Alexei Starovoitov wrote: > >> Like the only reason my patch is counting till 17 is because of >> trace_iwlwifi_dev_ucode_error(). >> The next offenders are using 12 arguments: >> trace_mc_event() >> trace_mm_vmscan_lru_shrink_inactive() >> >> Clearly not every efficient usage of it: >> trace_mm_vmscan_lru_shrink_inactive(pgdat->node_id, >> nr_scanned, nr_reclaimed, >> stat.nr_dirty, stat.nr_writeback, >> stat.nr_congested, stat.nr_immediate, >> stat.nr_activate, stat.nr_ref_keep, >> stat.nr_unmap_fail, >> sc->priority, file); >> could have passed &stat instead. > > Yes they should have, and if I was on the Cc for that patch, I would > have yelled at them and told them that's exactly what they needed to do. > > Perhaps I should add something to keep any tracepoint from having more > than 6 arguments. That should force a clean up quickly. I was hesitant to do anything about iwlwifi_dev_ucode_error's 17 args, because when the code is in such shape there are likely more skeletons in the closet. Turned out 'struct iwl_error_event_table' is defined twice with subtle different field names and layout. While the same trace_iwlwifi_dev_ucode_error() is used in what looks like two different cases. I think I managed to refactor it from 17 args to 4 while keeping all bugs in place, but it really should be a job of the author of the code to deal with such oddities. Will send the 'fix' in the next respin. So I definitely support the idea of build time warn for large number of args.