Re: [Xen-devel] [PATCH v4 3/6] xen: add process_pending_softirqs_norcu() for keyhandlers

[Jan Beulich <jbeulich@xxxxxxxx>]

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?

> @@ -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?

> --- 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?

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel