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

[Jürgen Groß <jgross@xxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel