| Message ID | 20181203180613.228133-10-james.morse@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | APEI in_nmi() rework and SDEI wire-up | expand |
On Mon, Dec 03, 2018 at 06:05:57PM +0000, James Morse wrote: > Refactor the estatus queue's pool notification routine from > NOTIFY_NMI's handlers. This will allow another notification > method to use the estatus queue without duplicating this code. > > This patch adds rcu_read_lock()/rcu_read_unlock() around the list s/This patch adds/Add/ > list_for_each_entry_rcu() walker. These aren't strictly necessary as > the whole nmi_enter/nmi_exit() window is a spooky RCU read-side > critical section. > > _in_nmi_notify_one() is separate from the rcu-list walker for a later > caller that doesn't need to walk a list. > > Signed-off-by: James Morse <james.morse@arm.com> > Reviewed-by: Punit Agrawal <punit.agrawal@arm.com> > Tested-by: Tyler Baicar <tbaicar@codeaurora.org> > > --- > Changes since v6: > * Removed pool grow/remove code as this is no longer necessary. > > Changes since v3: > * Removed duplicate or redundant paragraphs in commit message. > * Fixed the style of a zero check. > Changes since v1: > * Tidied up _in_nmi_notify_one(). > --- > drivers/acpi/apei/ghes.c | 63 ++++++++++++++++++++++++++-------------- > 1 file changed, 41 insertions(+), 22 deletions(-) ... > +static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) > +{ > + int ret = NMI_DONE; > + > + if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) > + return ret; > + > + if (!ghes_estatus_queue_notified(&ghes_nmi)) > + ret = NMI_HANDLED; So this reads kinda the other way around, at least to me: "if the queue was *not* notified, the NMI was handled." Maybe rename to this: err = process_queue(&ghes_nmi); if (!err) ret = NMI_HANDLED; to make it clearer... And yeah, all those static functions having "ghes_" prefix is just encumbering readability for no good reason. Thx.
Hi Boris, On 11/12/2018 17:44, Borislav Petkov wrote: > On Mon, Dec 03, 2018 at 06:05:57PM +0000, James Morse wrote: >> Refactor the estatus queue's pool notification routine from >> NOTIFY_NMI's handlers. This will allow another notification >> method to use the estatus queue without duplicating this code. >> >> This patch adds rcu_read_lock()/rcu_read_unlock() around the list > > s/This patch adds/Add/ > >> list_for_each_entry_rcu() walker. These aren't strictly necessary as >> the whole nmi_enter/nmi_exit() window is a spooky RCU read-side >> critical section. >> >> _in_nmi_notify_one() is separate from the rcu-list walker for a later >> caller that doesn't need to walk a list. >> +static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) >> +{ >> + int ret = NMI_DONE; >> + >> + if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) >> + return ret; >> + >> + if (!ghes_estatus_queue_notified(&ghes_nmi)) >> + ret = NMI_HANDLED; > > So this reads kinda the other way around, at least to me: > > "if the queue was *not* notified, the NMI was handled." > > Maybe rename to this: > > err = process_queue(&ghes_nmi); > if (!err) > ret = NMI_HANDLED; > > to make it clearer... (yup, that's clearer). But now we've opened pandora's box of naming-things: This thing isn't really processing anything, its walking a list of 'maybe it was one of these' and copying anything it finds into the estatus-queue to be handled later. I've evidently overloaded 'notified' to mean this. __process_error() doesn't process anything either, it does the add-to-queue. 'spool' is the word that best conveys what's going on here, I should probably use that 'in_nmi' prefix more to make it clear this has to be nmi safe. Something like: ghes_notify_nmi() -> in_nmi_spool_from_list(list) -> in_nmi_queue_one_entry(ghes). Thanks, James
On Thu, Jan 10, 2019 at 06:21:21PM +0000, James Morse wrote: > Something like: > ghes_notify_nmi() -> in_nmi_spool_from_list(list) -> in_nmi_queue_one_entry(ghes). Yah, but make that ghes_notify_nmi() -> ghes_nmi_spool_from_list(list) -> ghes_nmi_queue_one_entry(ghes). to denote it is the GHES NMI path. Thx.
diff --git a/drivers/acpi/apei/ghes.c b/drivers/acpi/apei/ghes.c index d06456e60318..366dbdd41ef3 100644 --- a/drivers/acpi/apei/ghes.c +++ b/drivers/acpi/apei/ghes.c @@ -907,37 +907,56 @@ static void __process_error(struct ghes *ghes) #endif } -static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) +static int _in_nmi_notify_one(struct ghes *ghes) { u64 buf_paddr; - struct ghes *ghes; - int sev, ret = NMI_DONE; + int sev; - if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) - return ret; + if (ghes_read_estatus(ghes, &buf_paddr)) { + ghes_clear_estatus(ghes, buf_paddr); + return -ENOENT; + } - list_for_each_entry_rcu(ghes, &ghes_nmi, list) { - if (ghes_read_estatus(ghes, &buf_paddr)) { - ghes_clear_estatus(ghes, buf_paddr); - continue; - } else { - ret = NMI_HANDLED; - } + sev = ghes_severity(ghes->estatus->error_severity); + if (sev >= GHES_SEV_PANIC) { + ghes_print_queued_estatus(); + __ghes_panic(ghes); + } - sev = ghes_severity(ghes->estatus->error_severity); - if (sev >= GHES_SEV_PANIC) { - ghes_print_queued_estatus(); - __ghes_panic(ghes); - } + __process_error(ghes); + ghes_clear_estatus(ghes, buf_paddr); - __process_error(ghes); - ghes_clear_estatus(ghes, buf_paddr); + return 0; +} + +static int ghes_estatus_queue_notified(struct list_head *rcu_list) +{ + int ret = -ENOENT; + struct ghes *ghes; + + rcu_read_lock(); + list_for_each_entry_rcu(ghes, rcu_list, list) { + if (!_in_nmi_notify_one(ghes)) + ret = 0; } + rcu_read_unlock(); -#ifdef CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG - if (ret == NMI_HANDLED) + if (IS_ENABLED(CONFIG_ARCH_HAVE_NMI_SAFE_CMPXCHG) && !ret) irq_work_queue(&ghes_proc_irq_work); -#endif + + return ret; +} + +static int ghes_notify_nmi(unsigned int cmd, struct pt_regs *regs) +{ + int ret = NMI_DONE; + + if (!atomic_add_unless(&ghes_in_nmi, 1, 1)) + return ret; + + if (!ghes_estatus_queue_notified(&ghes_nmi)) + ret = NMI_HANDLED; + atomic_dec(&ghes_in_nmi); return ret; }