| Message ID | 4db49e192b6829e520cf497481a51deef227b23b.1679084101.git.ehem+xen@m5p.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Fixing error_interrupt()'s messages v2 | expand |
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
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.
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
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.
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
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]); } /*
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(-)