On 10.03.20 18:02, Jan Beulich wrote:
On 10.03.2020 08:28, Juergen Gross wrote:
--- 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)
Why the separate bool? Can't you ...
@@ -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) )
... check !(ignore_mask & RCU_SOFTIRQ) here?
Good idea.
@@ -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) |
+ (1ul << RCU_SOFTIRQ), false);
I guess the comment here also wants to mention RCU?
Yes.
--- 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();
At the example of this - the property of holding an RCU lock is
entirely invisible here, as it's the generic
iommu_dump_p2m_table() which acquires it. This suggest to me that
going forward breaking this is going to be very likely. Couldn't
process_pending_softirqs() exclude RCU handling when finding
preempt_count() to return non-zero?
This can be done, but then the non-debug build would require to have
non-empty rcu lock functions.
An alternative would be to ASSERT() no rcu lock being held in
process_pending_softirqs() or rcu_check_callbacks() which would catch
the problematic cases in debug builds.
Juergen
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
