| Message ID | 20220603154037.103733-3-imbrenda@linux.ibm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | better smp interrupt checks | expand |
On 6/3/22 17:40, Claudio Imbrenda wrote: > Use per-CPU flags and callbacks for Program, Extern, and I/O interrupts > instead of global variables. > > This allows for more accurate error handling; a CPU waiting for an > interrupt will not have it "stolen" by a different CPU that was not > supposed to wait for one, and now two CPUs can wait for interrupts at > the same time. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > lib/s390x/asm/arch_def.h | 7 ++++++- > lib/s390x/interrupt.c | 38 ++++++++++++++++---------------------- > 2 files changed, 22 insertions(+), 23 deletions(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index 72553819..3a0d9c43 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -124,7 +124,12 @@ struct lowcore { > uint8_t pad_0x0280[0x0308 - 0x0280]; /* 0x0280 */ > uint64_t sw_int_crs[16]; /* 0x0308 */ > struct psw sw_int_psw; /* 0x0388 */ > - uint8_t pad_0x0310[0x11b0 - 0x0398]; /* 0x0398 */ > + uint32_t pgm_int_expected; /* 0x0398 */ > + uint32_t ext_int_expected; /* 0x039c */ > + void (*pgm_cleanup_func)(void); /* 0x03a0 */ > + void (*ext_cleanup_func)(void); /* 0x03a8 */ > + void (*io_int_func)(void); /* 0x03b0 */ If you switch the function pointers and the *_expected around, you can use bools for the latter, right? I think, since they're names suggest that they're bools, they should be. Additionally I prefer true/false over 1/0, since the latter raises the questions if other values are also used. > + uint8_t pad_0x03b8[0x11b0 - 0x03b8]; /* 0x03b8 */ > uint64_t mcck_ext_sa_addr; /* 0x11b0 */ > uint8_t pad_0x11b8[0x1200 - 0x11b8]; /* 0x11b8 */ > uint64_t fprs_sa[16]; /* 0x1200 */ > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c > index 27d3b767..e57946f0 100644 > --- a/lib/s390x/interrupt.c > +++ b/lib/s390x/interrupt.c > @@ -15,14 +15,11 @@ > #include <fault.h> > #include <asm/page.h> > > -static bool pgm_int_expected; > -static bool ext_int_expected; > -static void (*pgm_cleanup_func)(void); > static struct lowcore *lc; > > void expect_pgm_int(void) > { > - pgm_int_expected = true; > + lc->pgm_int_expected = 1; > lc->pgm_int_code = 0; > lc->trans_exc_id = 0; > mb(); [...] > void handle_pgm_int(struct stack_frame_int *stack) > { > - if (!pgm_int_expected) { > + if (!lc->pgm_int_expected) { > /* Force sclp_busy to false, otherwise we will loop forever */ > sclp_handle_ext(); > print_pgm_info(stack); > } > > - pgm_int_expected = false; > + lc->pgm_int_expected = 0; > > - if (pgm_cleanup_func) > - (*pgm_cleanup_func)(); > + if (lc->pgm_cleanup_func) > + (*lc->pgm_cleanup_func)(); [...] > + if (lc->io_int_func) > + return lc->io_int_func(); Why is a difference between the function pointer usages here?
On Tue, 7 Jun 2022 12:01:11 +0200 Janis Schoetterl-Glausch <scgl@linux.ibm.com> wrote: > On 6/3/22 17:40, Claudio Imbrenda wrote: > > Use per-CPU flags and callbacks for Program, Extern, and I/O interrupts > > instead of global variables. > > > > This allows for more accurate error handling; a CPU waiting for an > > interrupt will not have it "stolen" by a different CPU that was not > > supposed to wait for one, and now two CPUs can wait for interrupts at > > the same time. > > > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > > --- > > lib/s390x/asm/arch_def.h | 7 ++++++- > > lib/s390x/interrupt.c | 38 ++++++++++++++++---------------------- > > 2 files changed, 22 insertions(+), 23 deletions(-) > > > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > > index 72553819..3a0d9c43 100644 > > --- a/lib/s390x/asm/arch_def.h > > +++ b/lib/s390x/asm/arch_def.h > > @@ -124,7 +124,12 @@ struct lowcore { > > uint8_t pad_0x0280[0x0308 - 0x0280]; /* 0x0280 */ > > uint64_t sw_int_crs[16]; /* 0x0308 */ > > struct psw sw_int_psw; /* 0x0388 */ > > - uint8_t pad_0x0310[0x11b0 - 0x0398]; /* 0x0398 */ > > + uint32_t pgm_int_expected; /* 0x0398 */ > > + uint32_t ext_int_expected; /* 0x039c */ > > + void (*pgm_cleanup_func)(void); /* 0x03a0 */ > > + void (*ext_cleanup_func)(void); /* 0x03a8 */ > > + void (*io_int_func)(void); /* 0x03b0 */ > > If you switch the function pointers and the *_expected around, > you can use bools for the latter, right? > I think, since they're names suggest that they're bools, they should > be. Additionally I prefer true/false over 1/0, since the latter raises > the questions if other values are also used. that's exactly what I wanted to avoid. uint32_t can easily be accessed atomically and/or compare-and-swapped if needed. I don't like using true/false for things that are not bools > > > + uint8_t pad_0x03b8[0x11b0 - 0x03b8]; /* 0x03b8 */ > > uint64_t mcck_ext_sa_addr; /* 0x11b0 */ > > uint8_t pad_0x11b8[0x1200 - 0x11b8]; /* 0x11b8 */ > > uint64_t fprs_sa[16]; /* 0x1200 */ > > diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c > > index 27d3b767..e57946f0 100644 > > --- a/lib/s390x/interrupt.c > > +++ b/lib/s390x/interrupt.c > > @@ -15,14 +15,11 @@ > > #include <fault.h> > > #include <asm/page.h> > > > > -static bool pgm_int_expected; > > -static bool ext_int_expected; > > -static void (*pgm_cleanup_func)(void); > > static struct lowcore *lc; > > > > void expect_pgm_int(void) > > { > > - pgm_int_expected = true; > > + lc->pgm_int_expected = 1; > > lc->pgm_int_code = 0; > > lc->trans_exc_id = 0; > > mb(); > > [...] > > > void handle_pgm_int(struct stack_frame_int *stack) > > { > > - if (!pgm_int_expected) { > > + if (!lc->pgm_int_expected) { > > /* Force sclp_busy to false, otherwise we will loop forever */ > > sclp_handle_ext(); > > print_pgm_info(stack); > > } > > > > - pgm_int_expected = false; > > + lc->pgm_int_expected = 0; > > > > - if (pgm_cleanup_func) > > - (*pgm_cleanup_func)(); > > + if (lc->pgm_cleanup_func) > > + (*lc->pgm_cleanup_func)(); > > [...] > > > + if (lc->io_int_func) > > + return lc->io_int_func(); > Why is a difference between the function pointer usages here? > because that is how it was before; both have the same semantics anyway
On Fri, 3 Jun 2022 17:40:37 +0200 Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > 0x1200 */ diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c > index 27d3b767..e57946f0 100644 [...] > @@ -30,7 +27,7 @@ void expect_pgm_int(void) > > void expect_ext_int(void) > { > - ext_int_expected = true; > + lc->ext_int_expected = 1; External Interrupts can be floating; so if I am not mistaken it could be delivered to a different CPU which didn't expect it. [...] > @@ -237,17 +231,17 @@ void handle_io_int(void) > > int register_io_int_func(void (*f)(void)) > { > - if (io_int_func) > + if (lc->io_int_func) > return -1; IO interrupts also are floating. Same concern as for the external interrupts.
On Tue, 7 Jun 2022 16:23:09 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > On Fri, 3 Jun 2022 17:40:37 +0200 > Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > > > 0x1200 */ diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c > > index 27d3b767..e57946f0 100644 > [...] > > @@ -30,7 +27,7 @@ void expect_pgm_int(void) > > > > void expect_ext_int(void) > > { > > - ext_int_expected = true; > > + lc->ext_int_expected = 1; > > External Interrupts can be floating; so if I am not mistaken it could be delivered to a different CPU which didn't expect it. yes I have considered that (maybe I should add this in the patch description) for floating interrupts, the testcase should take care to mask the interrupt on the CPUs where it should not be received. by default all interrupts are masked anyway and CPUs should only enable them on a case by case basis > > [...] > > @@ -237,17 +231,17 @@ void handle_io_int(void) > > > > int register_io_int_func(void (*f)(void)) > > { > > - if (io_int_func) > > + if (lc->io_int_func) > > return -1; > > IO interrupts also are floating. Same concern as for the external interrupts. same here the alternative is to have a separate handling of floating vs non-floating interrupts, which maybe would get a little out of hand
On Tue, 7 Jun 2022 16:41:13 +0200 Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > yes I have considered that (maybe I should add this in the patch > description) Yes, and not just that; maybe rename expect_ext_int to expect_ext_int_on_this_cpu, same for register_io_int_func.
On Tue, 7 Jun 2022 16:48:57 +0200 Nico Boehr <nrb@linux.ibm.com> wrote: > On Tue, 7 Jun 2022 16:41:13 +0200 > Claudio Imbrenda <imbrenda@linux.ibm.com> wrote: > > > yes I have considered that (maybe I should add this in the patch > > description) > > Yes, and not just that; maybe rename expect_ext_int to expect_ext_int_on_this_cpu, same for register_io_int_func. fair enough
On 6/3/22 17:40, Claudio Imbrenda wrote: > Use per-CPU flags and callbacks for Program, Extern, and I/O interrupts > instead of global variables. > > This allows for more accurate error handling; a CPU waiting for an > interrupt will not have it "stolen" by a different CPU that was not > supposed to wait for one, and now two CPUs can wait for interrupts at > the same time. > > Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> > --- > lib/s390x/asm/arch_def.h | 7 ++++++- > lib/s390x/interrupt.c | 38 ++++++++++++++++---------------------- > 2 files changed, 22 insertions(+), 23 deletions(-) > > diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h > index 72553819..3a0d9c43 100644 > --- a/lib/s390x/asm/arch_def.h > +++ b/lib/s390x/asm/arch_def.h > @@ -124,7 +124,12 @@ struct lowcore { > uint8_t pad_0x0280[0x0308 - 0x0280]; /* 0x0280 */ > uint64_t sw_int_crs[16]; /* 0x0308 */ > struct psw sw_int_psw; /* 0x0388 */ > - uint8_t pad_0x0310[0x11b0 - 0x0398]; /* 0x0398 */ > + uint32_t pgm_int_expected; /* 0x0398 */ > + uint32_t ext_int_expected; /* 0x039c */ > + void (*pgm_cleanup_func)(void); /* 0x03a0 */ > + void (*ext_cleanup_func)(void); /* 0x03a8 */ > + void (*io_int_func)(void); /* 0x03b0 */ > + uint8_t pad_0x03b8[0x11b0 - 0x03b8]; /* 0x03b8 */ Before we directly pollute the lowcore I'd much rather have a pointer to a struct. We could then either use any area of the lowcore by adding a union or we extend the SMP lib per-cpu structs. I don't want to have to review offset calculations for every change of per-cpu data. They are just way too easy to get wrong.
diff --git a/lib/s390x/asm/arch_def.h b/lib/s390x/asm/arch_def.h index 72553819..3a0d9c43 100644 --- a/lib/s390x/asm/arch_def.h +++ b/lib/s390x/asm/arch_def.h @@ -124,7 +124,12 @@ struct lowcore { uint8_t pad_0x0280[0x0308 - 0x0280]; /* 0x0280 */ uint64_t sw_int_crs[16]; /* 0x0308 */ struct psw sw_int_psw; /* 0x0388 */ - uint8_t pad_0x0310[0x11b0 - 0x0398]; /* 0x0398 */ + uint32_t pgm_int_expected; /* 0x0398 */ + uint32_t ext_int_expected; /* 0x039c */ + void (*pgm_cleanup_func)(void); /* 0x03a0 */ + void (*ext_cleanup_func)(void); /* 0x03a8 */ + void (*io_int_func)(void); /* 0x03b0 */ + uint8_t pad_0x03b8[0x11b0 - 0x03b8]; /* 0x03b8 */ uint64_t mcck_ext_sa_addr; /* 0x11b0 */ uint8_t pad_0x11b8[0x1200 - 0x11b8]; /* 0x11b8 */ uint64_t fprs_sa[16]; /* 0x1200 */ diff --git a/lib/s390x/interrupt.c b/lib/s390x/interrupt.c index 27d3b767..e57946f0 100644 --- a/lib/s390x/interrupt.c +++ b/lib/s390x/interrupt.c @@ -15,14 +15,11 @@ #include <fault.h> #include <asm/page.h> -static bool pgm_int_expected; -static bool ext_int_expected; -static void (*pgm_cleanup_func)(void); static struct lowcore *lc; void expect_pgm_int(void) { - pgm_int_expected = true; + lc->pgm_int_expected = 1; lc->pgm_int_code = 0; lc->trans_exc_id = 0; mb(); @@ -30,7 +27,7 @@ void expect_pgm_int(void) void expect_ext_int(void) { - ext_int_expected = true; + lc->ext_int_expected = 1; lc->ext_int_code = 0; mb(); } @@ -43,7 +40,7 @@ uint16_t clear_pgm_int(void) code = lc->pgm_int_code; lc->pgm_int_code = 0; lc->trans_exc_id = 0; - pgm_int_expected = false; + lc->pgm_int_expected = 0; return code; } @@ -57,7 +54,7 @@ void check_pgm_int_code(uint16_t code) void register_pgm_cleanup_func(void (*f)(void)) { - pgm_cleanup_func = f; + lc->pgm_cleanup_func = f; } static void fixup_pgm_int(struct stack_frame_int *stack) @@ -184,24 +181,23 @@ static void print_pgm_info(struct stack_frame_int *stack) void handle_pgm_int(struct stack_frame_int *stack) { - if (!pgm_int_expected) { + if (!lc->pgm_int_expected) { /* Force sclp_busy to false, otherwise we will loop forever */ sclp_handle_ext(); print_pgm_info(stack); } - pgm_int_expected = false; + lc->pgm_int_expected = 0; - if (pgm_cleanup_func) - (*pgm_cleanup_func)(); + if (lc->pgm_cleanup_func) + (*lc->pgm_cleanup_func)(); else fixup_pgm_int(stack); } void handle_ext_int(struct stack_frame_int *stack) { - if (!ext_int_expected && - lc->ext_int_code != EXT_IRQ_SERVICE_SIG) { + if (!lc->ext_int_expected && lc->ext_int_code != EXT_IRQ_SERVICE_SIG) { report_abort("Unexpected external call interrupt (code %#x): on cpu %d at %#lx", lc->ext_int_code, stap(), lc->ext_old_psw.addr); return; @@ -211,7 +207,7 @@ void handle_ext_int(struct stack_frame_int *stack) stack->crs[0] &= ~(1UL << 9); sclp_handle_ext(); } else { - ext_int_expected = false; + lc->ext_int_expected = 0; } if (!(stack->crs[0] & CR0_EXTM_MASK)) @@ -224,12 +220,10 @@ void handle_mcck_int(void) stap(), lc->mcck_old_psw.addr); } -static void (*io_int_func)(void); - void handle_io_int(void) { - if (io_int_func) - return io_int_func(); + if (lc->io_int_func) + return lc->io_int_func(); report_abort("Unexpected io interrupt: on cpu %d at %#lx", stap(), lc->io_old_psw.addr); @@ -237,17 +231,17 @@ void handle_io_int(void) int register_io_int_func(void (*f)(void)) { - if (io_int_func) + if (lc->io_int_func) return -1; - io_int_func = f; + lc->io_int_func = f; return 0; } int unregister_io_int_func(void (*f)(void)) { - if (io_int_func != f) + if (lc->io_int_func != f) return -1; - io_int_func = NULL; + lc->io_int_func = NULL; return 0; }
Use per-CPU flags and callbacks for Program, Extern, and I/O interrupts instead of global variables. This allows for more accurate error handling; a CPU waiting for an interrupt will not have it "stolen" by a different CPU that was not supposed to wait for one, and now two CPUs can wait for interrupts at the same time. Signed-off-by: Claudio Imbrenda <imbrenda@linux.ibm.com> --- lib/s390x/asm/arch_def.h | 7 ++++++- lib/s390x/interrupt.c | 38 ++++++++++++++++---------------------- 2 files changed, 22 insertions(+), 23 deletions(-)