Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc

["Drokin, Oleg" <oleg.drokin@intel.com>]

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 <james.a.nunez@intel.com>
>>>> 
>>>> 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.