Message-ID: <ZdZOYnIkjqkyfo5P@x1>
Date: Wed, 6 Mar 2024 10:06:30 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
Cc: Mike Galbraith <efault@....de>, Peter Zijlstra <peterz@...radead.org>,
	Marco Elver <elver@...gle.com>, linux-rt-users@...r.kernel.org,
	Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
	Juri Lelli <juri.lelli@...hat.com>,
	Clark Williams <williams@...hat.com>,
	Alessandro Carminati <acarmina@...hat.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Artem Savkov <asavkov@...hat.com>
Subject: Re: 'perf test sigtrap' failing on PREEMPT_RT_FULL

> In Thu, 4 Jan 2024 19:35:57 -0300, Arnaldo Carvalho de Melo wrote:
> > +++ b/kernel/events/core.c
> > @@ -6801,7 +6801,7 @@ static void perf_pending_task(struct callback_head *head)
> >         * If we 'fail' here, that's OK, it means recursion is already disabled
> >         * and we won't recurse 'further'.
> >         */
> >-       preempt_disable_notrace();
> >+       migrate_disable();
> >        rctx = perf_swevent_get_recursion_context();
 
> Pardon my ignorance, is it safe to call preempt_count() with preemption
> enabled on PREEMPT_RT, or at least in the context being discussed here?
 
> Because:
 
> 	 perf_swevent_get_recursion_context()
> 	     get_recursion_context()
>                  interrupt_context_level()
>                      preempt_count()	
 
> And:
 
> int perf_swevent_get_recursion_context(void)
> {
>         struct swevent_htable *swhash = this_cpu_ptr(&swevent_htable);
> 
>         return get_recursion_context(swhash->recursion);
> }

Seems to be enough because perf_pending_task is a irq_work callback and
that is guaranteed not to reentry?

Artem's tests with a RHEL kernel seems to indicate that, ditto for my,
will test with upstream linux-6.8.y-rt.

But there is a lot more happening in perf_sigtrap and I'm not sure if
the irq_work callback gets preempted we would not race with something
else.

Marco, Mike, ideas?

- Arnaldo
 
> >         if (event->pending_work) {
> > @@ -6812,7 +6812,7 @@ static void perf_pending_task(struct callback_head *head)
>  
> >        if (rctx >= 0)
> >                 perf_swevent_put_recursion_context(rctx);
> > -       preempt_enable_notrace();
> > +       migrate_enable();
>  
> >         put_event(event);
> >  }
> > [acme@...e linux]$ uname -a
> > Linux nine 6.7.0-rc5-rt5.sigtrap-fix-dirty #2 SMP PREEMPT_RT Thu Jan  4 18:11:44 -03 2024 x86_64 x86_64 x86_64 GNU/Linux
> > [acme@...e linux]$ sudo su -
> > [sudo] password for acme: 
> > [root@...e ~]# 
> > [root@...e ~]# perf test sigtrap
> >  68: Sigtrap                                                         : Ok
> > [root@...e ~]# 
> > [root@...e ~]# perf probe -L perf_pending_task
> > <perf_pending_task@...me/acme/git/linux/kernel/events/core.c:0>
> >       0  static void perf_pending_task(struct callback_head *head)
> >          {
> >       2         struct perf_event *event = container_of(head, struct perf_event, pending_task);
> >       3         int rctx;
>        
> >                 /*
> >                  * If we 'fail' here, that's OK, it means recursion is already disabled
> >                  * and we won't recurse 'further'.
> >                  */
> >                 migrate_disable();
> >      10         rctx = perf_swevent_get_recursion_context();
> >         
> >      12         if (event->pending_work) {
> >      13                 event->pending_work = 0;
> >      14                 perf_sigtrap(event);
> >      15                 local_dec(&event->ctx->nr_pending);
> >                 }
> >         
> >      18         if (rctx >= 0)
> >      19                 perf_swevent_put_recursion_context(rctx);
> >      20         migrate_enable();
>      
> >      22         put_event(event);
> >          }
>          
> >          #ifdef CONFIG_GUEST_PERF_EVENTS
> 
> > [root@...e ~]# perf probe perf_pending_task
> > Added new event:
> >   probe:perf_pending_task (on perf_pending_task)
> 
> > You can now use it in all perf tools, such as:
> 
> > 	perf record -e probe:perf_pending_task -aR sleep 1
> 
> > [root@...e ~]# perf trace --max-events=1 -e probe:perf_pending_task/max-stack=6/ perf test sigtrap 
> >  68: Sigtrap                                                         : Ok
> >      0.000 :9608/9608 probe:perf_pending_task(__probe_ip: -2064408784)
> >                                        perf_pending_task ([kernel.kallsyms])
> >                                        task_work_run ([kernel.kallsyms])
> >                                        exit_to_user_mode_loop ([kernel.kallsyms])
> >                                        exit_to_user_mode_prepare ([kernel.kallsyms])
> >                                        irqentry_exit_to_user_mode ([kernel.kallsyms])
> >                                        asm_sysvec_irq_work ([kernel.kallsyms])
> > [root@...e ~]#
> 
> > [root@...e ~]# head -5 /etc/os-release
> > NAME="Red Hat Enterprise Linux"
> > VERSION="9.2 (Plow)"
> > ID="rhel"
> > ID_LIKE="fedora"
> > VERSION_ID="9.2"
> > [root@...e ~]#
> 
> > I did the test without the above patch and the original problem is
> > reproduced.
>  
> > > This is also used in perf_pending_irq() and on PREEMPT_RT this is
> > > invoked from softirq context which is preemptible.
> 
> Humm, and then when going thru perf_pending_irq() we don't hit that
> scheduling on atomic.
> 
> - Arnaldo

Powered by blists - more mailing lists