Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753541AbcCLTRM (ORCPT ); Sat, 12 Mar 2016 14:17:12 -0500 Received: from mga14.intel.com ([192.55.52.115]:1246 "EHLO mga14.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751254AbcCLTRF convert rfc822-to-8bit (ORCPT ); Sat, 12 Mar 2016 14:17:05 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.24,326,1455004800"; d="scan'208";a="65036951" From: "Drokin, Oleg" To: Joe Perches CC: James Simmons , Greg Kroah-Hartman , "" , "Dilger, Andreas" , Linux Kernel Mailing List , "Lustre Development List" , "Nunez, James A" Subject: Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc Thread-Topic: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc Thread-Index: AQHRfJDnyXRUsed9oUmno3ElPng1wJ9WtH0A Date: Sat, 12 Mar 2016 19:17:02 +0000 Message-ID: References: <1457805636-23859-1-git-send-email-jsimmons@infradead.org> <1457805636-23859-2-git-send-email-jsimmons@infradead.org> <1457807005.11972.9.camel@perches.com> <5A0A38B8-B387-436D-B72A-7D8247B6A00D@intel.com> <1457808989.11972.22.camel@perches.com> In-Reply-To: <1457808989.11972.22.camel@perches.com> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-originating-ip: [10.254.1.152] Content-Type: text/plain; charset="Windows-1252" Content-ID: <2C9BE8CBA505934FA009A7A7E75C6668@intel.com> Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3012 Lines: 82 On Mar 12, 2016, at 1:56 PM, Joe Perches wrote: > On Sat, 2016-03-12 at 18:32 +0000, Drokin, Oleg wrote: >> On Mar 12, 2016, at 1:23 PM, Joe Perches wrote: >>> On Sat, 2016-03-12 at 13:00 -0500, James Simmons wrote: >>>> From: James Nunez >>>> >>>> This is one of the fixes broken out of patch 10000 that was >>>> missed in the merger. With this fix the CERROR called in >>>> sfw_handle_server_rpc will print out correctly. >>> Speaking of CERROR and logging, it it really useful >>> for each CERROR use to have 2 static structs? >>> >>> In CERROR -> CDEBUG_LIMIT there is a: >>> static struct cfs_debug_limit_state cdls; >>> (12 or 16 bytes depending on 32/64 bit arch) >>> >>> and in CDEBUG_LIMIT -> _CDEBUG >>> static struct libcfs_debug_msg_data msgdata; >>> (24 or 36 bytes depending on 32/64 bit arch) >>> >>> That seems a largish bit of data and code to initialize >>> these structs for over a thousand call sites. >>> >>> Wouldn't a single static suffice? >> Single static would not work because the code is parallel so it'll >> stomp over each other. > > Sure, but would that matter in practice? Well. The bits about the callsite would certainly matter since we need to know where the message is coming from. Overwriting them in a racy way would make the messages unreliable. > net_ratelimit() has similar parallelization and it doesn't > seem to matter there. That one seems to rate limit all prints together. We are trying limit each individual one. So if you have a bunch of print1 and a bunch of print2, but a few of print3, you see the print3, but ratelimit the first two and get something like this in the logs: print1 print2 print3 print2 condensed: the message was repeated a gazillion times print3 print1 condensed: the message was repeated two gazillion times. > >> or do you mean to have a common >> structure for every callsite (but instantiated separately)? > > That might help a tiny bit. > > Some possibly unnecessary bits: > > o .msg_cdls How are we going to rate-limit this stuff without remembering some information between the calls? > o __FILE__, __func__ and __LINE__ fields have marginal value Probably not as important in the kernel indeed, but on the other hand if the message has moved compared to the source developer has then there is evidence some patches were applied and that could be asked about. > o .msg_subsys seems set only to DEBUG_SUBSYSTEM. This is redefined in every source file: drivers/staging/lustre/lustre/fid/fid_lib.c:#define DEBUG_SUBSYSTEM S_FID drivers/staging/lustre/lustre/fid/fid_request.c:#define DEBUG_SUBSYSTEM S_FID drivers/staging/lustre/lustre/fid/lproc_fid.c:#define DEBUG_SUBSYSTEM S_FID drivers/staging/lustre/lustre/fld/fld_cache.c:#define DEBUG_SUBSYSTEM S_FLD drivers/staging/lustre/lustre/fld/fld_request.c:#define DEBUG_SUBSYSTEM S_FLD drivers/staging/lustre/lustre/fld/lproc_fld.c:#define DEBUG_SUBSYSTEM S_FLD ? Allows you to filter out messages by subsystem in addition to by level.