[PATCH v2 0/2] Fixing error_interrupt()'s messages v2

[PATCH v2 0/2] Fixing error_interrupt()'s messages v2

[Elliott Mitchell]

Using a somewhat distinct strategy with the printk().  This is two
patches, since nominally the first can work with other strategies.  I
could see them being squashed during commit to the main repository.

Elliott Mitchell (2):
  x86/APIC: include full string with error_interrupt() error messages
  x86/APIC: modify error_interrupt() to output using single printk()

 xen/arch/x86/apic.c | 27 ++++++++++++++-------------
 1 file changed, 14 insertions(+), 13 deletions(-)

-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |       ehem+sigmsg@drgnwing.com PGP 87145445       |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[PATCH v2 1/2] x86/APIC: include full string with error_interrupt() error messages

[Elliott Mitchell]

Rather than adding ", " with each printf(), simply include them in the
string initially.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 xen/arch/x86/apic.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index f71474d47d..8cfb8cd71c 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1401,14 +1401,14 @@ static void cf_check spurious_interrupt(struct cpu_user_regs *regs)
 static void cf_check error_interrupt(struct cpu_user_regs *regs)
 {
     static const char *const esr_fields[] = {
-        "Send CS error",
-        "Receive CS error",
-        "Send accept error",
-        "Receive accept error",
-        "Redirectable IPI",
-        "Send illegal vector",
-        "Received illegal vector",
-        "Illegal register address",
+        ", Send CS error",
+        ", Receive CS error",
+        ", Send accept error",
+        ", Receive accept error",
+        ", Redirectable IPI",
+        ", Send illegal vector",
+        ", Received illegal vector",
+        ", Illegal register address",
     };
     unsigned int v, v1;
     int i;
@@ -1423,7 +1423,7 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs)
             smp_processor_id(), v , v1);
     for ( i = 7; i >= 0; --i )
         if ( v1 & (1 << i) )
-            printk(", %s", esr_fields[i]);
+            printk("%s", esr_fields[i]);
     printk("\n");
 }
 
-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |       ehem+sigmsg@drgnwing.com PGP 87145445       |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH v2 1/2] x86/APIC: include full string with error_interrupt() error messages

[Jan Beulich]

On 17.03.2023 20:45, Elliott Mitchell wrote:
> Rather than adding ", " with each printf(), simply include them in the
> string initially.

Why is this better? You're now using more space in .rodata. (I haven't
looked at patch 2 yet to see whether there's a possible reason there
for the change here, but if there was it would need saying here.)

Jan

Re: [PATCH v2 1/2] x86/APIC: include full string with error_interrupt() error messages

[Elliott Mitchell]

On Mon, Mar 20, 2023 at 09:49:14AM +0100, Jan Beulich wrote:
> On 17.03.2023 20:45, Elliott Mitchell wrote:
> > Rather than adding ", " with each printf(), simply include them in the
> > string initially.
> 
> Why is this better? You're now using more space in .rodata. (I haven't
> looked at patch 2 yet to see whether there's a possible reason there
> for the change here, but if there was it would need saying here.)

I would expect this to give trivially better performance.  Instead of
needing to needing copy some data from the format string, then strcat()
from the arguments this turns it into a single strcat().

Other item is this sort of change is very often a precursor to replacing
the use of a *printf()-type function with a str*cat()-type function.
Though in this case I doubt there is a strlcatk() function so that is
unlikely.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

[PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Elliott Mitchell]

This takes care of the issue of APIC errors tending to occur on multiple
cores at one.  In turn this tends to causes the error messages to be
merged together, making understanding them difficult.

Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>
---
 xen/arch/x86/apic.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/xen/arch/x86/apic.c b/xen/arch/x86/apic.c
index 8cfb8cd71c..89abd1ea51 100644
--- a/xen/arch/x86/apic.c
+++ b/xen/arch/x86/apic.c
@@ -1410,6 +1410,7 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs)
         ", Received illegal vector",
         ", Illegal register address",
     };
+    const char *entries[ARRAY_SIZE(esr_fields)];
     unsigned int v, v1;
     int i;
 
@@ -1419,12 +1420,12 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs)
     v1 = apic_read(APIC_ESR);
     ack_APIC_irq();
 
-    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)",
-            smp_processor_id(), v , v1);
     for ( i = 7; i >= 0; --i )
-        if ( v1 & (1 << i) )
-            printk("%s", esr_fields[i]);
-    printk("\n");
+        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
+    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)"
+            "%s%s%s%s%s%s%s%s" "\n",
+            smp_processor_id(), v , v1, entries[0], entries[1], entries[2],
+            entries[3], entries[4], entries[5], entries[6], entries[7]);
 }
 
 /*
-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |       ehem+sigmsg@drgnwing.com PGP 87145445       |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Jan Beulich]

On 17.03.2023 20:53, Elliott Mitchell wrote:
> This takes care of the issue of APIC errors tending to occur on multiple
> cores at one.  In turn this tends to causes the error messages to be

Nit: "at once"?

> merged together, making understanding them difficult.
> 
> Signed-off-by: Elliott Mitchell <ehem+xen@m5p.com>

Here it becomes clear why you're making the change in patch 1; as you say
in the cover letter these may better be folded (or else, as said there,
patch 1 needs better justification).

> @@ -1419,12 +1420,12 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs)
>      v1 = apic_read(APIC_ESR);
>      ack_APIC_irq();
>  
> -    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)",
> -            smp_processor_id(), v , v1);
>      for ( i = 7; i >= 0; --i )
> -        if ( v1 & (1 << i) )
> -            printk("%s", esr_fields[i]);
> -    printk("\n");
> +        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
> +    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)"
> +            "%s%s%s%s%s%s%s%s" "\n",
> +            smp_processor_id(), v , v1, entries[0], entries[1], entries[2],
> +            entries[3], entries[4], entries[5], entries[6], entries[7]);

Two style nits: Indentation wants fixing here (it was wrong in the original
code already), and the stray blank between v and the comma also wants
dropping at this occasion.

Jan

Re: [PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Elliott Mitchell]

On Mon, Mar 20, 2023 at 09:56:54AM +0100, Jan Beulich wrote:
> On 17.03.2023 20:53, Elliott Mitchell wrote:
> > This takes care of the issue of APIC errors tending to occur on multiple
> > cores at one.  In turn this tends to causes the error messages to be
> 
> Nit: "at once"?

https://en.wiktionary.org/wiki/at_once

Adverb #2, synonym of "simultaneously".


> > @@ -1419,12 +1420,12 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs)
> >      v1 = apic_read(APIC_ESR);
> >      ack_APIC_irq();
> >  
> > -    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)",
> > -            smp_processor_id(), v , v1);
> >      for ( i = 7; i >= 0; --i )
> > -        if ( v1 & (1 << i) )
> > -            printk("%s", esr_fields[i]);
> > -    printk("\n");
> > +        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
> > +    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)"
> > +            "%s%s%s%s%s%s%s%s" "\n",
> > +            smp_processor_id(), v , v1, entries[0], entries[1], entries[2],
> > +            entries[3], entries[4], entries[5], entries[6], entries[7]);
> 
> Two style nits: Indentation wants fixing here (it was wrong in the original
> code already), and the stray blank between v and the comma also wants
> dropping at this occasion.

There are several minor issues here which may be best handled during
commit as they're very small items about how precisely you want this to
look.

First, I later realized I goofed the argument order.  In order to match
the original implementation, it needs to be entries[7] ... entries[0]
(could though be the low-order bits should be reported first).

Second, the order of the for loop no longer matters.  Using
ARRAY_SIZE(esr_fields) and increment should now be more maintainable
(this would also allow i to be unsigned).

Third, I'm simply unsure how you would prefer to format the printk().
I imagine at some future point the register may have additional bits
allocated.  At such point esr_fields[] would grow, but this would also
require adding to the format string and adding an argument.

Seems like several fiddly bits which mostly effect look and don't really
effect the implementation.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Jan Beulich]

On 20.03.2023 15:29, Elliott Mitchell wrote:
> On Mon, Mar 20, 2023 at 09:56:54AM +0100, Jan Beulich wrote:
>> On 17.03.2023 20:53, Elliott Mitchell wrote:
>>> This takes care of the issue of APIC errors tending to occur on multiple
>>> cores at one.  In turn this tends to causes the error messages to be
>>
>> Nit: "at once"?
> 
> https://en.wiktionary.org/wiki/at_once
> 
> Adverb #2, synonym of "simultaneously".

And that's what you mean, I think? Not being a native speaker, I have no
idea what "at one" is meaning here.

>>> @@ -1419,12 +1420,12 @@ static void cf_check error_interrupt(struct cpu_user_regs *regs)
>>>      v1 = apic_read(APIC_ESR);
>>>      ack_APIC_irq();
>>>  
>>> -    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)",
>>> -            smp_processor_id(), v , v1);
>>>      for ( i = 7; i >= 0; --i )
>>> -        if ( v1 & (1 << i) )
>>> -            printk("%s", esr_fields[i]);
>>> -    printk("\n");
>>> +        entries[i] = v1 & (1 << i) ? esr_fields[i] : "";
>>> +    printk(XENLOG_DEBUG "APIC error on CPU%u: %02x(%02x)"
>>> +            "%s%s%s%s%s%s%s%s" "\n",
>>> +            smp_processor_id(), v , v1, entries[0], entries[1], entries[2],
>>> +            entries[3], entries[4], entries[5], entries[6], entries[7]);
>>
>> Two style nits: Indentation wants fixing here (it was wrong in the original
>> code already), and the stray blank between v and the comma also wants
>> dropping at this occasion.
> 
> There are several minor issues here which may be best handled during
> commit as they're very small items about how precisely you want this to
> look.
> 
> First, I later realized I goofed the argument order.  In order to match
> the original implementation, it needs to be entries[7] ... entries[0]
> (could though be the low-order bits should be reported first).

I'm not really concerned of the order. A change of order wants
mentioning in the description though.

> Second, the order of the for loop no longer matters.  Using
> ARRAY_SIZE(esr_fields) and increment should now be more maintainable
> (this would also allow i to be unsigned).

Indeed. But that would better done in a separate patch then anyway.

> Third, I'm simply unsure how you would prefer to format the printk().

About any way matching style guidelines is okay. There are two more
things to mention though (sorry for not noticing earlier): We aim at
keeping the entire format string on one line, for grep-ability. And
there's no need (and in fact no reason) to split the sequence of %s
from the \n. To summarize:

    printk(XENLOG_DEBUG
           "APIC error on CPU%u: %02x(%02x)%s%s%s%s%s%s%s%s\n",

(unless of course it all fits on one line, which it looks like it
does).

Jan

Re: [PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Elliott Mitchell]

On Mon, Mar 20, 2023 at 04:39:48PM +0100, Jan Beulich wrote:
> On 20.03.2023 15:29, Elliott Mitchell wrote:
> > 
> > There are several minor issues here which may be best handled during
> > commit as they're very small items about how precisely you want this to
> > look.
> > 
> > First, I later realized I goofed the argument order.  In order to match
> > the original implementation, it needs to be entries[7] ... entries[0]
> > (could though be the low-order bits should be reported first).
> 
> I'm not really concerned of the order. A change of order wants
> mentioning in the description though.

Seemed simple enough to fix on commit (simply switch the order of
numbers).

> > Second, the order of the for loop no longer matters.  Using
> > ARRAY_SIZE(esr_fields) and increment should now be more maintainable
> > (this would also allow i to be unsigned).
> 
> Indeed. But that would better done in a separate patch then anyway.

Feel free to split.

> > Third, I'm simply unsure how you would prefer to format the printk().
> 
> About any way matching style guidelines is okay. There are two more
> things to mention though (sorry for not noticing earlier): We aim at
> keeping the entire format string on one line, for grep-ability. And
> there's no need (and in fact no reason) to split the sequence of %s
> from the \n. To summarize:
> 
>     printk(XENLOG_DEBUG
>            "APIC error on CPU%u: %02x(%02x)%s%s%s%s%s%s%s%s\n",
> 
> (unless of course it all fits on one line, which it looks like it
> does).

I like keeping the "%s%s%s%s%s%s%s%s" section separated since it needs to
match the number of arguments.  In the future where more bits of the
register are defined, both sections will need to be modified together.


This seems to be a spot where there are large numbers of similarly
functional, but mildly different style variants.  As such I suspect this
is best left in your hands as this is a bog of trivial style
considerations which have no real functional effect.


-- 
(\___(\___(\______          --=> 8-) EHM <=--          ______/)___/)___/)
 \BS (    |         ehem+sigmsg@m5p.com  PGP 87145445         |    )   /
  \_CS\   |  _____  -O #include <stddisclaimer.h> O-   _____  |   /  _/
8A19\___\_|_/58D2 7E3D DDF4 7BA6 <-PGP-> 41D1 B375 37D0 8714\_|_/___/5445

Re: [PATCH v2 2/2] x86/APIC: modify error_interrupt() to output using single printk()

[Jan Beulich]

On 20.03.2023 16:54, Elliott Mitchell wrote:
> On Mon, Mar 20, 2023 at 04:39:48PM +0100, Jan Beulich wrote:
>> On 20.03.2023 15:29, Elliott Mitchell wrote:
>>>
>>> There are several minor issues here which may be best handled during
>>> commit as they're very small items about how precisely you want this to
>>> look.
>>>
>>> First, I later realized I goofed the argument order.  In order to match
>>> the original implementation, it needs to be entries[7] ... entries[0]
>>> (could though be the low-order bits should be reported first).
>>
>> I'm not really concerned of the order. A change of order wants
>> mentioning in the description though.
> 
> Seemed simple enough to fix on commit (simply switch the order of
> numbers).
> 
>>> Second, the order of the for loop no longer matters.  Using
>>> ARRAY_SIZE(esr_fields) and increment should now be more maintainable
>>> (this would also allow i to be unsigned).
>>
>> Indeed. But that would better done in a separate patch then anyway.
> 
> Feel free to split.
> 
>>> Third, I'm simply unsure how you would prefer to format the printk().
>>
>> About any way matching style guidelines is okay. There are two more
>> things to mention though (sorry for not noticing earlier): We aim at
>> keeping the entire format string on one line, for grep-ability. And
>> there's no need (and in fact no reason) to split the sequence of %s
>> from the \n. To summarize:
>>
>>     printk(XENLOG_DEBUG
>>            "APIC error on CPU%u: %02x(%02x)%s%s%s%s%s%s%s%s\n",
>>
>> (unless of course it all fits on one line, which it looks like it
>> does).
> 
> I like keeping the "%s%s%s%s%s%s%s%s" section separated since it needs to
> match the number of arguments.  In the future where more bits of the
> register are defined, both sections will need to be modified together.
> 
> 
> This seems to be a spot where there are large numbers of similarly
> functional, but mildly different style variants.  As such I suspect this
> is best left in your hands as this is a bog of trivial style
> considerations which have no real functional effect.

Just to clarify: What is or is not adjusted on commit is a decision of
the committer. A no longer as active committer was actually of the
opinion that it is a mistake to ever make any changes while committing.
In the case here you're asking for far more changes (including either
one to the description of patch 1, or the folding of both patches) than
I personally would be willing to do. I'm sorry for that.

Jan