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@xxxxxxxxx> >>>> >>>> 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. _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
