[RFC,05/12] opal-prd: implement basic heuristic for hservice_puts loglevels

Message ID OF8270EB2F.B681BF4F-ON86258134.0009D902-86258134.0009E63A@notes.na.collabserv.com
State New
Headers show

Commit Message

Daniel M Crowell June 3, 2017, 1:48 a.m.
So what will the actual result of these changes be?  Is this going to 
change where our messages show up?

--
Dan Crowell
Senior Software Engineer - Power Systems Enablement Firmware
IBM Rochester: t/l 553-2987
dcrowell@us.ibm.com



From:   Jeremy Kerr <jk@ozlabs.org>
To:     skiboot@lists.ozlabs.org
Cc:     Dan Crowell <dcrowell@us.ibm.com>, Jeremy Kerr <jk@ozlabs.org>
Date:   05/25/2017 02:06 AM
Subject:        [PATCH RFC 05/12] opal-prd: implement basic heuristic for 
hservice_puts loglevels



Signed-off-by: Jeremy Kerr <jk@ozlabs.org>
---
 external/opal-prd/opal-prd.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

 void hservice_assert(void)

Comments

Jeremy Kerr June 3, 2017, 3:15 a.m. | #1
Hi Dan,

> So what will the actual result of these changes be?  Is this going to
> change where our messages show up?

This only changes the priority of the messages logged. Currently, we log
all HBRT output at LOG_INFO priority. After this change, we will log
HBRT messages that contain the string "error" as LOG_ERROR and those
that contain "warn" as LOG_WARNING (both are higher priorities than
LOG_INFO).

So, nothing will change in terms of log locations at this stage.

However, once this is done, I'd like to propose a second change, to the
distributions' packing of opal-prd. The idea would be that:

  - all log output will go to a new log file, /var/log/opal-prd.log

  - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard
    syslog paths (generally /var/log/syslog).

This means that we get all the HBRT output stored, but only include
higher-priority messages in the standard system logs.

How does that sound?

Cheers,


Jeremy
Daniel M Crowell June 3, 2017, 3:56 a.m. | #2
>  - all log output will go to a new log file, /var/log/opal-prd.log
>
>  - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard
>    syslog paths (generally /var/log/syslog).

I'm good with that.  We just need to get PE to update their FFDC 
collection scripts to grab the new file once it shows up.


I would suggest you add a couple more cases in your compares.  We've got 
these constant in HB that are used a lot (though not all over) as the 
first 2 characters in our traces. 
#define ERR_MRK   "E>"
#define FAIL_MRK  "F>"
#define WARN_MRK  "W>"
#define INFO_MRK  "I>"


--
Dan Crowell
Senior Software Engineer - Power Systems Enablement Firmware
IBM Rochester: t/l 553-2987
dcrowell@us.ibm.com



From:   Jeremy Kerr <jk@ozlabs.org>
To:     Daniel M Crowell <dcrowell@us.ibm.com>
Cc:     skiboot@lists.ozlabs.org, Frederic Bonnard <FREDERIC@fr.ibm.com>
Date:   06/02/2017 10:15 PM
Subject:        Re: [PATCH RFC 05/12] opal-prd: implement basic heuristic 
for hservice_puts loglevels



Hi Dan,

> So what will the actual result of these changes be?  Is this going to
> change where our messages show up?

This only changes the priority of the messages logged. Currently, we log
all HBRT output at LOG_INFO priority. After this change, we will log
HBRT messages that contain the string "error" as LOG_ERROR and those
that contain "warn" as LOG_WARNING (both are higher priorities than
LOG_INFO).

So, nothing will change in terms of log locations at this stage.

However, once this is done, I'd like to propose a second change, to the
distributions' packing of opal-prd. The idea would be that:

  - all log output will go to a new log file, /var/log/opal-prd.log

  - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard
    syslog paths (generally /var/log/syslog).

This means that we get all the HBRT output stored, but only include
higher-priority messages in the standard system logs.

How does that sound?

Cheers,


Jeremy
Jeremy Kerr June 3, 2017, 5:20 a.m. | #3
Hi Dan,

>>  - all log output will go to a new log file, /var/log/opal-prd.log
>>
>>  - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard
>>    syslog paths (generally /var/log/syslog).
> 
> I'm good with that.  We just need to get PE to update their FFDC
> collection scripts to grab the new file once it shows up.
> 
> 
> I would suggest you add a couple more cases in your compares.  We've got
> these constant in HB that are used a lot (though not all over) as the
> first 2 characters in our traces.  
> #define ERR_MRK   "E>"
> #define FAIL_MRK  "F>"
> #define WARN_MRK  "W>"
> #define INFO_MRK  "I>"

Oh, that's great then. If one of these is present, is it going to
be at the beginning of the puts() string?

Would it be worthwhile removing the strcasestr(.., "error"/"warning"),
and *just* using these instead? That would prevent the potential for
false classification.

We'd probably map both FAIL_MRK and ERR_MRK to LOG_ERROR. Or is FAIL_MRK
more of a warning? Or a notice ("normal but significant condition")?

Cheers,


Jeremy
Daniel M Crowell June 3, 2017, 6:35 p.m. | #4
> be at the beginning of the puts() string?
Yes, those are always at the beginning.

> Would it be worthwhile removing the strcasestr(.., "error"/"warning"),
Yeah, not sure what you'll get with those.

> Or is FAIL_MRK more of a warning? Or a notice ("normal but significant 
condition")?
I've honestly never seen a F> in any trace I've looked at.  I suspect we 
copied these in from the FSP and just never used it.  I would equate FAIL 
with ERR

--
Dan Crowell
Senior Software Engineer - Power Systems Enablement Firmware
IBM Rochester: t/l 553-2987
dcrowell@us.ibm.com



From:   Jeremy Kerr <jk@ozlabs.org>
To:     Daniel M Crowell <dcrowell@us.ibm.com>
Cc:     Frederic Bonnard <FREDERIC@fr.ibm.com>, skiboot@lists.ozlabs.org
Date:   06/03/2017 12:20 AM
Subject:        Re: [PATCH RFC 05/12] opal-prd: implement basic heuristic 
for hservice_puts loglevels



Hi Dan,

>>  - all log output will go to a new log file, /var/log/opal-prd.log
>>
>>  - log messages of LOG_WARNING and LOG_ERROR *also* go to the standard
>>    syslog paths (generally /var/log/syslog).
> 
> I'm good with that.  We just need to get PE to update their FFDC
> collection scripts to grab the new file once it shows up.
> 
> 
> I would suggest you add a couple more cases in your compares.  We've got
> these constant in HB that are used a lot (though not all over) as the
> first 2 characters in our traces. 
> #define ERR_MRK   "E>"
> #define FAIL_MRK  "F>"
> #define WARN_MRK  "W>"
> #define INFO_MRK  "I>"

Oh, that's great then. If one of these is present, is it going to
be at the beginning of the puts() string?

Would it be worthwhile removing the strcasestr(.., "error"/"warning"),
and *just* using these instead? That would prevent the potential for
false classification.

We'd probably map both FAIL_MRK and ERR_MRK to LOG_ERROR. Or is FAIL_MRK
more of a warning? Or a notice ("normal but significant condition")?

Cheers,


Jeremy

Patch

diff --git a/external/opal-prd/opal-prd.c b/external/opal-prd/opal-prd.c
index 6ac8a20..c10803d 100644
--- a/external/opal-prd/opal-prd.c
+++ b/external/opal-prd/opal-prd.c
@@ -267,7 +267,14 @@  extern int call_run_command(int argc, const char 
**argv, char **o_outString);
 
 void hservice_puts(const char *str)
 {
-                pr_log(LOG_INFO, "HBRT: %s", str);
+                int priority = LOG_INFO;
+
+                if (strcasestr(str, "error"))
+                                priority = LOG_ERR;
+                else if (strcasestr(str, "warn"))
+                                priority = LOG_WARNING;
+
+                pr_log(priority, "HBRT: %s", str);
 }