| Message ID | 20200218122114.17596-4-jgross@suse.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/rcu: let rcu work better with core scheduling | expand |
On Tue, Feb 18, 2020 at 01:21:13PM +0100, Juergen Gross wrote: > Some keyhandlers are calling process_pending_softirqs() while holding > a rcu_read_lock(). This is wrong, as process_pending_softirqs() might > activate rcu calls which should not happen inside a rcu_read_lock(). It might be helpful to turn the ASSERT in process_pending_softirqs into ASSERT_NOT_IN_ATOMIC also, as it would catch such missuses AFAICT. > > For that purpose add process_pending_softirqs_norcu() which will not > do any rcu activity and use this for keyhandlers. I wonder if for keyhandlers it might be easier to just disable the watchdog in handle_keypress and remove the softirq processing from the handlers. At the end of day we want the keyhanders to run as fast as possible in order to get the data out, and we only care about the watchdog not triggering? (maybe I'm missing something here) > +void process_pending_softirqs_norcu(void) > +{ > + ASSERT(!in_irq() && local_irq_is_enabled()); > + /* Do not enter scheduler as it can preempt the calling context. */ > + __do_softirq((1ul << SCHEDULE_SOFTIRQ) | (1ul << SCHED_SLAVE_SOFTIRQ), Don't you also need to pass RCU_SOFTIRQ to the ignore mask in order to avoid any RCU work happening? Thanks, Roger.
On 24.02.20 12:25, Roger Pau Monné wrote: > On Tue, Feb 18, 2020 at 01:21:13PM +0100, Juergen Gross wrote: >> Some keyhandlers are calling process_pending_softirqs() while holding >> a rcu_read_lock(). This is wrong, as process_pending_softirqs() might >> activate rcu calls which should not happen inside a rcu_read_lock(). > > It might be helpful to turn the ASSERT in process_pending_softirqs > into ASSERT_NOT_IN_ATOMIC also, as it would catch such missuses > AFAICT. No, this would be triggering in __cpu_up() at system boot. > >> >> For that purpose add process_pending_softirqs_norcu() which will not >> do any rcu activity and use this for keyhandlers. > > I wonder if for keyhandlers it might be easier to just disable the > watchdog in handle_keypress and remove the softirq processing from the > handlers. > > At the end of day we want the keyhanders to run as fast as possible in > order to get the data out, and we only care about the watchdog not > triggering? (maybe I'm missing something here) It is not that simple, I believe. You'd need to be very careful that other functionality wouldn't suffer. I'm e.g. not sure time_calibration won't lead to a hanging system then. > >> +void process_pending_softirqs_norcu(void) >> +{ >> + ASSERT(!in_irq() && local_irq_is_enabled()); >> + /* Do not enter scheduler as it can preempt the calling context. */ >> + __do_softirq((1ul << SCHEDULE_SOFTIRQ) | (1ul << SCHED_SLAVE_SOFTIRQ), > > Don't you also need to pass RCU_SOFTIRQ to the ignore mask in order to > avoid any RCU work happening? Yes, that's probably a good idea. Juergen
On Mon, Feb 24, 2020 at 12:44:48PM +0100, Jürgen Groß wrote: > On 24.02.20 12:25, Roger Pau Monné wrote: > > On Tue, Feb 18, 2020 at 01:21:13PM +0100, Juergen Gross wrote: > > > Some keyhandlers are calling process_pending_softirqs() while holding > > > a rcu_read_lock(). This is wrong, as process_pending_softirqs() might > > > activate rcu calls which should not happen inside a rcu_read_lock(). > > > > It might be helpful to turn the ASSERT in process_pending_softirqs > > into ASSERT_NOT_IN_ATOMIC also, as it would catch such missuses > > AFAICT. > > No, this would be triggering in __cpu_up() at system boot. Yes, saw that in the next patch. > > > > > > > > For that purpose add process_pending_softirqs_norcu() which will not > > > do any rcu activity and use this for keyhandlers. > > > > I wonder if for keyhandlers it might be easier to just disable the > > watchdog in handle_keypress and remove the softirq processing from the > > handlers. > > > > At the end of day we want the keyhanders to run as fast as possible in > > order to get the data out, and we only care about the watchdog not > > triggering? (maybe I'm missing something here) > > It is not that simple, I believe. > > You'd need to be very careful that other functionality wouldn't suffer. > I'm e.g. not sure time_calibration won't lead to a hanging system then. AFAICT time_calibration is used to sync the timestamps of the various CPUs so that they don't drift too much, but I don't think not executing it could lead to a hang, it would lead to (bigger) skews between CPUs, but such skews happen anyway. Thanks, Roger.
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c index d4defa01c2..af2b012144 100644 --- a/xen/arch/x86/mm/p2m-ept.c +++ b/xen/arch/x86/mm/p2m-ept.c @@ -1342,7 +1342,7 @@ static void ept_dump_p2m_table(unsigned char key) c ?: ept_entry->ipat ? '!' : ' '); if ( !(record_counter++ % 100) ) - process_pending_softirqs(); + process_pending_softirqs_norcu(); } unmap_domain_page(table); } diff --git a/xen/arch/x86/numa.c b/xen/arch/x86/numa.c index f1066c59c7..cf6fcc9966 100644 --- a/xen/arch/x86/numa.c +++ b/xen/arch/x86/numa.c @@ -418,7 +418,7 @@ static void dump_numa(unsigned char key) printk("Memory location of each domain:\n"); for_each_domain ( d ) { - process_pending_softirqs(); + process_pending_softirqs_norcu(); printk("Domain %u (total: %u):\n", d->domain_id, domain_tot_pages(d)); @@ -462,7 +462,7 @@ static void dump_numa(unsigned char key) for ( j = 0; j < d->max_vcpus; j++ ) { if ( !(j & 0x3f) ) - process_pending_softirqs(); + process_pending_softirqs_norcu(); if ( vnuma->vcpu_to_vnode[j] == i ) { diff --git a/xen/common/keyhandler.c b/xen/common/keyhandler.c index 87bd145374..0d32bc4e2a 100644 --- a/xen/common/keyhandler.c +++ b/xen/common/keyhandler.c @@ -263,7 +263,7 @@ static void dump_domains(unsigned char key) { unsigned int i; - process_pending_softirqs(); + process_pending_softirqs_norcu(); printk("General information for domain %u:\n", d->domain_id); printk(" refcnt=%d dying=%d pause_count=%d\n", @@ -307,7 +307,7 @@ static void dump_domains(unsigned char key) for_each_sched_unit_vcpu ( unit, v ) { if ( !(v->vcpu_id & 0x3f) ) - process_pending_softirqs(); + process_pending_softirqs_norcu(); printk(" VCPU%d: CPU%d [has=%c] poll=%d " "upcall_pend=%02x upcall_mask=%02x ", @@ -337,7 +337,7 @@ static void dump_domains(unsigned char key) for_each_vcpu ( d, v ) { if ( !(v->vcpu_id & 0x3f) ) - process_pending_softirqs(); + process_pending_softirqs_norcu(); printk("Notifying guest %d:%d (virq %d, port %d)\n", d->domain_id, v->vcpu_id, diff --git a/xen/common/softirq.c b/xen/common/softirq.c index b83ad96d6c..3fe75ca3e8 100644 --- a/xen/common/softirq.c +++ b/xen/common/softirq.c @@ -25,7 +25,7 @@ static softirq_handler softirq_handlers[NR_SOFTIRQS]; static DEFINE_PER_CPU(cpumask_t, batch_mask); static DEFINE_PER_CPU(unsigned int, batching); -static void __do_softirq(unsigned long ignore_mask) +static void __do_softirq(unsigned long ignore_mask, bool rcu_allowed) { unsigned int i, cpu; unsigned long pending; @@ -38,7 +38,7 @@ static void __do_softirq(unsigned long ignore_mask) */ cpu = smp_processor_id(); - if ( rcu_pending(cpu) ) + if ( rcu_allowed && rcu_pending(cpu) ) rcu_check_callbacks(cpu); if ( ((pending = (softirq_pending(cpu) & ~ignore_mask)) == 0) @@ -55,13 +55,22 @@ void process_pending_softirqs(void) { ASSERT(!in_irq() && local_irq_is_enabled()); /* Do not enter scheduler as it can preempt the calling context. */ - __do_softirq((1ul << SCHEDULE_SOFTIRQ) | (1ul << SCHED_SLAVE_SOFTIRQ)); + __do_softirq((1ul << SCHEDULE_SOFTIRQ) | (1ul << SCHED_SLAVE_SOFTIRQ), + true); +} + +void process_pending_softirqs_norcu(void) +{ + ASSERT(!in_irq() && local_irq_is_enabled()); + /* Do not enter scheduler as it can preempt the calling context. */ + __do_softirq((1ul << SCHEDULE_SOFTIRQ) | (1ul << SCHED_SLAVE_SOFTIRQ), + false); } void do_softirq(void) { ASSERT_NOT_IN_ATOMIC(); - __do_softirq(0); + __do_softirq(0, true); } void open_softirq(int nr, softirq_handler handler) diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 3112653960..880d64c748 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -587,7 +587,7 @@ static void amd_dump_p2m_table_level(struct page_info* pg, int level, struct amd_iommu_pte *pde = &table_vaddr[index]; if ( !(index % 2) ) - process_pending_softirqs(); + process_pending_softirqs_norcu(); if ( !pde->pr ) continue; diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index 3d60976dd5..c7bd8d4ada 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -2646,7 +2646,7 @@ static void vtd_dump_p2m_table_level(paddr_t pt_maddr, int level, paddr_t gpa, for ( i = 0; i < PTE_NUM; i++ ) { if ( !(i % 2) ) - process_pending_softirqs(); + process_pending_softirqs_norcu(); pte = &pt_vaddr[i]; if ( !dma_pte_present(*pte) ) diff --git a/xen/drivers/vpci/msi.c b/xen/drivers/vpci/msi.c index 75010762ed..1d337604cc 100644 --- a/xen/drivers/vpci/msi.c +++ b/xen/drivers/vpci/msi.c @@ -321,13 +321,13 @@ void vpci_dump_msi(void) * holding the lock. */ printk("unable to print all MSI-X entries: %d\n", rc); - process_pending_softirqs(); + process_pending_softirqs_norcu(); continue; } } spin_unlock(&pdev->vpci->lock); - process_pending_softirqs(); + process_pending_softirqs_norcu(); } } rcu_read_unlock(&domlist_read_lock); diff --git a/xen/include/xen/softirq.h b/xen/include/xen/softirq.h index b4724f5c8b..b5bf3b83b1 100644 --- a/xen/include/xen/softirq.h +++ b/xen/include/xen/softirq.h @@ -37,7 +37,9 @@ void cpu_raise_softirq_batch_finish(void); * Process pending softirqs on this CPU. This should be called periodically * when performing work that prevents softirqs from running in a timely manner. * Use this instead of do_softirq() when you do not want to be preempted. + * The norcu variant is to be used while holding a read_rcu_lock(). */ void process_pending_softirqs(void); +void process_pending_softirqs_norcu(void); #endif /* __XEN_SOFTIRQ_H__ */
Some keyhandlers are calling process_pending_softirqs() while holding a rcu_read_lock(). This is wrong, as process_pending_softirqs() might activate rcu calls which should not happen inside a rcu_read_lock(). For that purpose add process_pending_softirqs_norcu() which will not do any rcu activity and use this for keyhandlers. Signed-off-by: Juergen Gross <jgross@suse.com> --- xen/arch/x86/mm/p2m-ept.c | 2 +- xen/arch/x86/numa.c | 4 ++-- xen/common/keyhandler.c | 6 +++--- xen/common/softirq.c | 17 +++++++++++++---- xen/drivers/passthrough/amd/pci_amd_iommu.c | 2 +- xen/drivers/passthrough/vtd/iommu.c | 2 +- xen/drivers/vpci/msi.c | 4 ++-- xen/include/xen/softirq.h | 2 ++ 8 files changed, 25 insertions(+), 14 deletions(-)