Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1356509 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH 1/6] staging: lustre: Correct missing newline for CERROR call in sfw_handle_server_rpc James Simmons <jsimmons@infradead.org> - 2016-03-12 19:10 +0100
    Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR  call in sfw_handle_server_rpc Joe Perches <joe@perches.com> - 2016-03-12 19:30 +0100
      Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR  call in sfw_handle_server_rpc "Drokin, Oleg" <oleg.drokin@intel.com> - 2016-03-12 19:40 +0100
        Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR  call in sfw_handle_server_rpc Joe Perches <joe@perches.com> - 2016-03-12 20:00 +0100
          Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR  call in sfw_handle_server_rpc "Drokin, Oleg" <oleg.drokin@intel.com> - 2016-03-12 20:20 +0100
            Re: [PATCH 1/6] staging: lustre: Correct missing newline for CERROR  call in sfw_handle_server_rpc Joe Perches <joe@perches.com> - 2016-03-12 20:30 +0100

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

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.

Signed-off-by: James Nunez <james.a.nunez@intel.com>
Intel-bug-id: https://jira.hpdd.intel.com/browse/LU-4871
Reviewed-on: http://review.whamcloud.com/10000
Reviewed-by: Andreas Dilger <andreas.dilger@intel.com>
Reviewed-by: John L. Hammond <john.hammond@intel.com>
Reviewed-by: Cliff White <cliff.white@intel.com>
---
 drivers/staging/lustre/lnet/selftest/framework.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/staging/lustre/lnet/selftest/framework.c b/drivers/staging/lustre/lnet/selftest/framework.c
index 926c397..0f32f0b 100644
--- a/drivers/staging/lustre/lnet/selftest/framework.c
+++ b/drivers/staging/lustre/lnet/selftest/framework.c
@@ -1244,7 +1244,7 @@ sfw_handle_server_rpc(struct srpc_server_rpc *rpc)
 
 	/* Remove timer to avoid racing with it or expiring active session */
 	if (sfw_del_session_timer()) {
-		CERROR("Dropping RPC (%s) from %s: racing with expiry timer.",
+		CERROR("dropping RPC %s from %s: racing with expiry timer\n",
 		       sv->sv_name, libcfs_id2str(rpc->srpc_peer));
 		spin_unlock(&sfw_data.fw_lock);
 		return -EAGAIN;
-- 
1.7.1

[toc] | [next] | [standalone]

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

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?

[toc] | [prev] | [next] | [standalone]

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

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. or do you mean to have a common
structure for every callsite (but instantiated separately)?

This used to be a local structure in the past, but that
caused considerable stack growth for some functions, that added
up along the call chain, and that was the solution we came up with
that did help.

[toc] | [prev] | [next] | [standalone]

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

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?

net_ratelimit() has similar parallelization and it doesn't
seem to matter there.

>  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
o __FILE__, __func__ and __LINE__ fields have marginal value
o .msg_subsys seems set only to DEBUG_SUBSYSTEM.

> This used to be a local structure in the past, but that
> caused considerable stack growth for some functions, that added
> up along the call chain, and that was the solution we came up with
> that did help.

[toc] | [prev] | [next] | [standalone]

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

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.

[toc] | [prev] | [next] | [standalone]

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

On Sat, 2016-03-12 at 19:17 +0000, Drokin, Oleg wrote:
> 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.

Sure.  It's up to you to control your output and
I don't know if it matters or not.  You do.

> > > 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?

Doesn't msg_cdls just point to the other structure?
Combining the 2 into one might be useful.

> > 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:

Thanks.  I didn't look hard.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web