[PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Luming Yu]

Before we have powerpc to use the generic entry infrastructure,
the call to fire user return notifier is made temporarily in powerpc
entry code.

Signed-off-by: Luming Yu <luming.yu@shingroup.cn>
---
 arch/powerpc/kernel/interrupt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
index c4f6d3c69ba9..7fe704946e96 100644
--- a/arch/powerpc/kernel/interrupt.c
+++ b/arch/powerpc/kernel/interrupt.c
@@ -19,6 +19,7 @@
 #include <asm/time.h>
 #include <asm/tm.h>
 #include <asm/unistd.h>
+#include <asm/entry-common.h>
 
 #if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
 unsigned long global_dbcr0[NR_CPUS];
@@ -245,6 +246,8 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)
 	/* Restore user access locks last */
 	kuap_user_restore(regs);
 
+	arch_exit_to_user_mode_prepare(regs, ti_flags);
+
 	return ret;
 }
 
-- 
2.42.0.windows.2

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Aneesh Kumar K.V]

Luming Yu <luming.yu@shingroup.cn> writes:

> Before we have powerpc to use the generic entry infrastructure,
> the call to fire user return notifier is made temporarily in powerpc
> entry code.
>

It is still not clear what will be registered as user return notifier.
Can you summarize that here?

>
> Signed-off-by: Luming Yu <luming.yu@shingroup.cn>
> ---
>  arch/powerpc/kernel/interrupt.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/arch/powerpc/kernel/interrupt.c b/arch/powerpc/kernel/interrupt.c
> index c4f6d3c69ba9..7fe704946e96 100644
> --- a/arch/powerpc/kernel/interrupt.c
> +++ b/arch/powerpc/kernel/interrupt.c
> @@ -19,6 +19,7 @@
>  #include <asm/time.h>
>  #include <asm/tm.h>
>  #include <asm/unistd.h>
> +#include <asm/entry-common.h>
>  
>  #if defined(CONFIG_PPC_ADV_DEBUG_REGS) && defined(CONFIG_PPC32)
>  unsigned long global_dbcr0[NR_CPUS];
> @@ -245,6 +246,8 @@ interrupt_exit_user_prepare_main(unsigned long ret, struct pt_regs *regs)
>  	/* Restore user access locks last */
>  	kuap_user_restore(regs);
>  
> +	arch_exit_to_user_mode_prepare(regs, ti_flags);
> +
>

That will run the notifier with user AMR/IAMR values. 

>  	return ret;
>  }
>  
> -- 
> 2.42.0.windows.2

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Michael Ellerman]

Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
> Luming Yu <luming.yu@shingroup.cn> writes:
>
>> Before we have powerpc to use the generic entry infrastructure,
>> the call to fire user return notifier is made temporarily in powerpc
>> entry code.
>>
>
> It is still not clear what will be registered as user return notifier.
> Can you summarize that here?

fire_user_return_notifiers() is defined in kernel/user-return-notifier.c

That's built when CONFIG_USER_RETURN_NOTIFIER=y.

That is not user selectable, it's only enabled by:

arch/x86/kvm/Kconfig:        select USER_RETURN_NOTIFIER

So it looks to me like (currently) it's always a nop and does nothing.

Which makes me wonder what the point of wiring this feature up is :)
Maybe it's needed for some other feature I don't know about?

Arguably we could just enable it because we can, and it currently does
nothing so it's unlikely to break anything. But that also makes it
impossible to test the implementation is correct, and runs the risk that
one day in the future when it does get enabled only then do we discover
it doesn't work.

cheers

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Christophe Leroy]

Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
>> Luming Yu <luming.yu@shingroup.cn> writes:
>>
>>> Before we have powerpc to use the generic entry infrastructure,
>>> the call to fire user return notifier is made temporarily in powerpc
>>> entry code.
>>>
>>
>> It is still not clear what will be registered as user return notifier.
>> Can you summarize that here?
> 
> fire_user_return_notifiers() is defined in kernel/user-return-notifier.c
> 
> That's built when CONFIG_USER_RETURN_NOTIFIER=y.
> 
> That is not user selectable, it's only enabled by:
> 
> arch/x86/kvm/Kconfig:        select USER_RETURN_NOTIFIER
> 
> So it looks to me like (currently) it's always a nop and does nothing.
> 
> Which makes me wonder what the point of wiring this feature up is :)
> Maybe it's needed for some other feature I don't know about?
> 
> Arguably we could just enable it because we can, and it currently does
> nothing so it's unlikely to break anything. But that also makes it
> impossible to test the implementation is correct, and runs the risk that
> one day in the future when it does get enabled only then do we discover
> it doesn't work.

Opened an "issue" for the day we need it: 
https://github.com/KSPP/linux/issues/348

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Christophe Leroy]

Le 20/02/2024 à 09:51, Christophe Leroy a écrit :
> 
> 
> Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
>> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
>>> Luming Yu <luming.yu@shingroup.cn> writes:
>>>
>>>> Before we have powerpc to use the generic entry infrastructure,
>>>> the call to fire user return notifier is made temporarily in powerpc
>>>> entry code.
>>>>
>>>
>>> It is still not clear what will be registered as user return notifier.
>>> Can you summarize that here?
>>
>> fire_user_return_notifiers() is defined in kernel/user-return-notifier.c
>>
>> That's built when CONFIG_USER_RETURN_NOTIFIER=y.
>>
>> That is not user selectable, it's only enabled by:
>>
>> arch/x86/kvm/Kconfig:        select USER_RETURN_NOTIFIER
>>
>> So it looks to me like (currently) it's always a nop and does nothing.
>>
>> Which makes me wonder what the point of wiring this feature up is :)
>> Maybe it's needed for some other feature I don't know about?
>>
>> Arguably we could just enable it because we can, and it currently does
>> nothing so it's unlikely to break anything. But that also makes it
>> impossible to test the implementation is correct, and runs the risk that
>> one day in the future when it does get enabled only then do we discover
>> it doesn't work.
> 
> Opened an "issue" for the day we need it:
> https://github.com/KSPP/linux/issues/348

Correct one is https://github.com/linuxppc/issues/issues/477

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[虞陆铭]

Hi,

it appears the little feature might require a little bit more work to find its value of the patch.

Using the following debug module ,  some debugging shows the TIF_USER_RETURN_NOTIFY
bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to 
be dropped somewhere on somone who carries the bit return to user space.
side notes:
there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
which should be sovled first to make it easier for further debuggig. 

[root@localhost linux]# cat lib/user-return-test.c
#include <linux/module.h>
#include <linux/kernel.h>
#include <linux/init.h>
#include <linux/container_of.h>
#include <linux/user-return-notifier.h>
#include <linux/delay.h>
#include <linux/kthread.h>
#include <linux/sched.h>

MODULE_LICENSE("GPL");


struct test_user_return {
        struct user_return_notifier urn;
        bool registered;
        int urn_value_changed;
        struct task_struct *worker;
};

static struct test_user_return __percpu *user_return_test;

static void test_user_return_cb(struct user_return_notifier *urn)
{
        struct test_user_return *tur =
                container_of(urn, struct test_user_return, urn);
        unsigned long flags;

        local_irq_save(flags);
        tur->urn_value_changed++;
        local_irq_restore(flags);
        return;
}

static int test_user_return_worker(void *tur)
{
        struct test_user_return *t;
        t = (struct test_user_return *) tur;
        preempt_disable();
        user_return_notifier_register(&t->urn);
        preempt_enable();
        t->registered = true;
        while (!kthread_should_stop()) {
                static int err_rate = 0;

                msleep (1000);
                if (!test_thread_flag(TIF_USER_RETURN_NOTIFY) && (err_rate == 0)) {
                        pr_err("TIF_USER_RETURN_NOTIFY is lost");
                        err_rate++;
                }
        }
        return 0;
}
static int init_test_user_return(void)
{
        int r = 0;

        user_return_test = alloc_percpu(struct test_user_return);
        if (!user_return_test) {
                pr_err("failed to allocate percpu test_user_return\n");
                r = -ENOMEM;
                goto exit;
        }
        {
                unsigned int cpu;
                struct task_struct *task;
                struct test_user_return *tur;

                for_each_online_cpu(cpu) {
                        tur = per_cpu_ptr(user_return_test, cpu);
                        if (!tur->registered) {
                                tur->urn.on_user_return = test_user_return_cb;
                                task = kthread_create(test_user_return_worker,
                                        tur, "test_user_return");
                                if (IS_ERR(task))
                                        pr_err("no test_user_return kthread created for cpu %d",cpu);
                                else {
                                        tur->worker = task;
                                        wake_up_process(task);
                                }
                        }
                }
        }

exit:
        return r;
}
static void exit_test_user_return(void)
{
        struct test_user_return *tur;
        int i,ret=0;

        for_each_online_cpu(i) {
                tur = per_cpu_ptr(user_return_test, i);
                if (tur->registered) {
                        pr_info("[cpu=%d, %d] ", i, tur->urn_value_changed);
                        user_return_notifier_unregister(&tur->urn);
                        tur->registered = false;
                }
                if (tur->worker) {
                        ret = kthread_stop(tur->worker);
                        if (ret)
                                pr_err("can't stop test_user_return kthread for cpu %d", i);
                }
        }
        free_percpu(user_return_test);
        return;
}

module_init(init_test_user_return);
module_exit(exit_test_user_return);

 
------------------ Original ------------------
From:  "Christophe Leroy"<christophe.leroy@csgroup.eu>;
Date:  Tue, Feb 20, 2024 05:02 PM
To:  "mpe"<mpe@ellerman.id.au>; "Aneesh Kumar K.V"<aneesh.kumar@kernel.org>; "虞陆铭"<luming.yu@shingroup.cn>; "linuxppc-dev"<linuxppc-dev@lists.ozlabs.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "npiggin"<npiggin@gmail.com>; 
Cc:  "shenghui.qu@shingroup.cn"<shenghui.qu@shingroup.cn>; "dawei.li@shingroup.cn"<dawei.li@shingroup.cn>; "ke.zhao@shingroup.cn"<ke.zhao@shingroup.cn>; "luming.yu"<luming.yu@gmail.com>; 
Subject:  Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

 



Le 20/02/2024 à 09:51, Christophe Leroy a écrit :
> 
> 
> Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
>> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
>>> Luming Yu <luming.yu@shingroup.cn> writes:
>>>
>>>> Before we have powerpc to use the generic entry infrastructure,
>>>> the call to fire user return notifier is made temporarily in powerpc
>>>> entry code.
>>>>
>>>
>>> It is still not clear what will be registered as user return notifier.
>>> Can you summarize that here?
>>
>> fire_user_return_notifiers() is defined in kernel/user-return-notifier.c
>>
>> That's built when CONFIG_USER_RETURN_NOTIFIER=y.
>>
>> That is not user selectable, it's only enabled by:
>>
>> arch/x86/kvm/Kconfig:        select USER_RETURN_NOTIFIER
>>
>> So it looks to me like (currently) it's always a nop and does nothing.
>>
>> Which makes me wonder what the point of wiring this feature up is :)
>> Maybe it's needed for some other feature I don't know about?
>>
>> Arguably we could just enable it because we can, and it currently does
>> nothing so it's unlikely to break anything. But that also makes it
>> impossible to test the implementation is correct, and runs the risk that
>> one day in the future when it does get enabled only then do we discover
>> it doesn't work.
> 
> Opened an "issue" for the day we need it:
> https://github.com/KSPP/linux/issues/348

Correct one is https://github.com/linuxppc/issues/issues/477

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Christophe Leroy]

Hi,

Le 28/08/2024 à 05:17, 虞陆铭 a écrit :
> Hi,
> 
> it appears the little feature might require a little bit more work to find its value of the patch.
> 
> Using the following debug module ,  some debugging shows the TIF_USER_RETURN_NOTIFY
> bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to
> be dropped somewhere on somone who carries the bit return to user space.
> side notes:
> there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
> which should be sovled first to make it easier for further debuggig.

As far as I can see, user return notifier infrastructure was implemented 
in 2009 for KVM on x86, see 
https://lore.kernel.org/all/1253105134-8862-1-git-send-email-avi@redhat.com/

Can you explain what is your usage of that infrastructure with your 
patch ? You are talking about debug, what's the added value, what is it 
used for ?

Thanks
Christophe

> 
> [root@localhost linux]# cat lib/user-return-test.c
> #include <linux/module.h>
> #include <linux/kernel.h>
> #include <linux/init.h>
> #include <linux/container_of.h>
> #include <linux/user-return-notifier.h>
> #include <linux/delay.h>
> #include <linux/kthread.h>
> #include <linux/sched.h>
> 
> MODULE_LICENSE("GPL");
> 
> 
> struct test_user_return {
>          struct user_return_notifier urn;
>          bool registered;
>          int urn_value_changed;
>          struct task_struct *worker;
> };
> 
> static struct test_user_return __percpu *user_return_test;
> 
> static void test_user_return_cb(struct user_return_notifier *urn)
> {
>          struct test_user_return *tur =
>                  container_of(urn, struct test_user_return, urn);
>          unsigned long flags;
> 
>          local_irq_save(flags);
>          tur->urn_value_changed++;
>          local_irq_restore(flags);
>          return;
> }
> 
> static int test_user_return_worker(void *tur)
> {
>          struct test_user_return *t;
>          t = (struct test_user_return *) tur;
>          preempt_disable();
>          user_return_notifier_register(&t->urn);
>          preempt_enable();
>          t->registered = true;
>          while (!kthread_should_stop()) {
>                  static int err_rate = 0;
> 
>                  msleep (1000);
>                  if (!test_thread_flag(TIF_USER_RETURN_NOTIFY) && (err_rate == 0)) {
>                          pr_err("TIF_USER_RETURN_NOTIFY is lost");
>                          err_rate++;
>                  }
>          }
>          return 0;
> }
> static int init_test_user_return(void)
> {
>          int r = 0;
> 
>          user_return_test = alloc_percpu(struct test_user_return);
>          if (!user_return_test) {
>                  pr_err("failed to allocate percpu test_user_return\n");
>                  r = -ENOMEM;
>                  goto exit;
>          }
>          {
>                  unsigned int cpu;
>                  struct task_struct *task;
>                  struct test_user_return *tur;
> 
>                  for_each_online_cpu(cpu) {
>                          tur = per_cpu_ptr(user_return_test, cpu);
>                          if (!tur->registered) {
>                                  tur->urn.on_user_return = test_user_return_cb;
>                                  task = kthread_create(test_user_return_worker,
>                                          tur, "test_user_return");
>                                  if (IS_ERR(task))
>                                          pr_err("no test_user_return kthread created for cpu %d",cpu);
>                                  else {
>                                          tur->worker = task;
>                                          wake_up_process(task);
>                                  }
>                          }
>                  }
>          }
> 
> exit:
>          return r;
> }
> static void exit_test_user_return(void)
> {
>          struct test_user_return *tur;
>          int i,ret=0;
> 
>          for_each_online_cpu(i) {
>                  tur = per_cpu_ptr(user_return_test, i);
>                  if (tur->registered) {
>                          pr_info("[cpu=%d, %d] ", i, tur->urn_value_changed);
>                          user_return_notifier_unregister(&tur->urn);
>                          tur->registered = false;
>                  }
>                  if (tur->worker) {
>                          ret = kthread_stop(tur->worker);
>                          if (ret)
>                                  pr_err("can't stop test_user_return kthread for cpu %d", i);
>                  }
>          }
>          free_percpu(user_return_test);
>          return;
> }
> 
> module_init(init_test_user_return);
> module_exit(exit_test_user_return);
> 
>   
> ------------------ Original ------------------
> From:  "Christophe Leroy"<christophe.leroy@csgroup.eu>;
> Date:  Tue, Feb 20, 2024 05:02 PM
> To:  "mpe"<mpe@ellerman.id.au>; "Aneesh Kumar K.V"<aneesh.kumar@kernel.org>; "虞陆铭"<luming.yu@shingroup.cn>; "linuxppc-dev"<linuxppc-dev@lists.ozlabs.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "npiggin"<npiggin@gmail.com>;
> Cc:  "shenghui.qu@shingroup.cn"<shenghui.qu@shingroup.cn>; "dawei.li@shingroup.cn"<dawei.li@shingroup.cn>; "ke.zhao@shingroup.cn"<ke.zhao@shingroup.cn>; "luming.yu"<luming.yu@gmail.com>;
> Subject:  Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure
> 
>   
> 
> 
> 
> Le 20/02/2024 à 09:51, Christophe Leroy a écrit :
>>
>>
>> Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
>>> Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
>>>> Luming Yu <luming.yu@shingroup.cn> writes:
>>>>
>>>>> Before we have powerpc to use the generic entry infrastructure,
>>>>> the call to fire user return notifier is made temporarily in powerpc
>>>>> entry code.
>>>>>
>>>>
>>>> It is still not clear what will be registered as user return notifier.
>>>> Can you summarize that here?
>>>
>>> fire_user_return_notifiers() is defined in kernel/user-return-notifier.c
>>>
>>> That's built when CONFIG_USER_RETURN_NOTIFIER=y.
>>>
>>> That is not user selectable, it's only enabled by:
>>>
>>> arch/x86/kvm/Kconfig:        select USER_RETURN_NOTIFIER
>>>
>>> So it looks to me like (currently) it's always a nop and does nothing.
>>>
>>> Which makes me wonder what the point of wiring this feature up is :)
>>> Maybe it's needed for some other feature I don't know about?
>>>
>>> Arguably we could just enable it because we can, and it currently does
>>> nothing so it's unlikely to break anything. But that also makes it
>>> impossible to test the implementation is correct, and runs the risk that
>>> one day in the future when it does get enabled only then do we discover
>>> it doesn't work.
>>
>> Opened an "issue" for the day we need it:
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F348&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cd9e9b6315413430cfba108dcc7100633%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604118862628419%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=iIYQTodb9zrGdfmzvnZrIVZ%2BKh2qZjMjT29ddkUpGIw%3D&reserved=0
> 
> Correct one is https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinuxppc%2Fissues%2Fissues%2F477&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cd9e9b6315413430cfba108dcc7100633%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604118862637095%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=aJRVdRWu%2F7NQ13jQ6yFLBynXIIfPPPQ3nS4FxiXGNyw%3D&reserved=0

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Luming Yu]

On Wed, Aug 28, 2024 at 07:46:52AM +0200, Christophe Leroy wrote:
> Hi,
> 
> Le 28/08/2024 à 05:17, 虞陆铭 a écrit :
> > Hi,
> > 
> > it appears the little feature might require a little bit more work to find its value of the patch.
> > 
> > Using the following debug module ,  some debugging shows the TIF_USER_RETURN_NOTIFY
> > bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to
> > be dropped somewhere on somone who carries the bit return to user space.
> > side notes:
> > there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
> > which should be sovled first to make it easier for further debuggig.
> 
> As far as I can see, user return notifier infrastructure was implemented in
> 2009 for KVM on x86, see
> https://lore.kernel.org/all/1253105134-8862-1-git-send-email-avi@redhat.com/
> 
> Can you explain what is your usage of that infrastructure with your patch ?
> You are talking about debug, what's the added value, what is it used for ?
one example: I was thinking to live patch kernel at the moment that all cpus are
either returning to user space or going into idle. But I'm not sure if it is truly
valuable. secondly, it can help us get more accurate user/system time 
accounting via tracing rather than through sampling technique. 
The third: it could have similar usages in kvm for ppc as x86 for tsc_aux. 
etc :-)
> 
> Thanks
> Christophe
> 
> > 
> > [root@localhost linux]# cat lib/user-return-test.c
> > #include <linux/module.h>
> > #include <linux/kernel.h>
> > #include <linux/init.h>
> > #include <linux/container_of.h>
> > #include <linux/user-return-notifier.h>
> > #include <linux/delay.h>
> > #include <linux/kthread.h>
> > #include <linux/sched.h>
> > 
> > MODULE_LICENSE("GPL");
> > 
> > 
> > struct test_user_return {
> >          struct user_return_notifier urn;
> >          bool registered;
> >          int urn_value_changed;
> >          struct task_struct *worker;
> > };
> > 
> > static struct test_user_return __percpu *user_return_test;
> > 
> > static void test_user_return_cb(struct user_return_notifier *urn)
> > {
> >          struct test_user_return *tur =
> >                  container_of(urn, struct test_user_return, urn);
> >          unsigned long flags;
> > 
> >          local_irq_save(flags);
> >          tur->urn_value_changed++;
> >          local_irq_restore(flags);
> >          return;
> > }
> > 
> > static int test_user_return_worker(void *tur)
> > {
> >          struct test_user_return *t;
> >          t = (struct test_user_return *) tur;
> >          preempt_disable();
> >          user_return_notifier_register(&t->urn);
> >          preempt_enable();
> >          t->registered = true;
> >          while (!kthread_should_stop()) {
> >                  static int err_rate = 0;
> > 
> >                  msleep (1000);
> >                  if (!test_thread_flag(TIF_USER_RETURN_NOTIFY) && (err_rate == 0)) {
> >                          pr_err("TIF_USER_RETURN_NOTIFY is lost");
> >                          err_rate++;
> >                  }
> >          }
> >          return 0;
> > }
> > static int init_test_user_return(void)
> > {
> >          int r = 0;
> > 
> >          user_return_test = alloc_percpu(struct test_user_return);
> >          if (!user_return_test) {
> >                  pr_err("failed to allocate percpu test_user_return\n");
> >                  r = -ENOMEM;
> >                  goto exit;
> >          }
> >          {
> >                  unsigned int cpu;
> >                  struct task_struct *task;
> >                  struct test_user_return *tur;
> > 
> >                  for_each_online_cpu(cpu) {
> >                          tur = per_cpu_ptr(user_return_test, cpu);
> >                          if (!tur->registered) {
> >                                  tur->urn.on_user_return = test_user_return_cb;
> >                                  task = kthread_create(test_user_return_worker,
> >                                          tur, "test_user_return");
> >                                  if (IS_ERR(task))
> >                                          pr_err("no test_user_return kthread created for cpu %d",cpu);
> >                                  else {
> >                                          tur->worker = task;
> >                                          wake_up_process(task);
> >                                  }
> >                          }
> >                  }
> >          }
> > 
> > exit:
> >          return r;
> > }
> > static void exit_test_user_return(void)
> > {
> >          struct test_user_return *tur;
> >          int i,ret=0;
> > 
> >          for_each_online_cpu(i) {
> >                  tur = per_cpu_ptr(user_return_test, i);
> >                  if (tur->registered) {
> >                          pr_info("[cpu=%d, %d] ", i, tur->urn_value_changed);
> >                          user_return_notifier_unregister(&tur->urn);
> >                          tur->registered = false;
> >                  }
> >                  if (tur->worker) {
> >                          ret = kthread_stop(tur->worker);
> >                          if (ret)
> >                                  pr_err("can't stop test_user_return kthread for cpu %d", i);
> >                  }
> >          }
> >          free_percpu(user_return_test);
> >          return;
> > }
> > 
> > module_init(init_test_user_return);
> > module_exit(exit_test_user_return);
> > 
> > ------------------ Original ------------------
> > From:  "Christophe Leroy"<christophe.leroy@csgroup.eu>;
> > Date:  Tue, Feb 20, 2024 05:02 PM
> > To:  "mpe"<mpe@ellerman.id.au>; "Aneesh Kumar K.V"<aneesh.kumar@kernel.org>; "虞陆铭"<luming.yu@shingroup.cn>; "linuxppc-dev"<linuxppc-dev@lists.ozlabs.org>; "linux-kernel"<linux-kernel@vger.kernel.org>; "npiggin"<npiggin@gmail.com>;
> > Cc:  "shenghui.qu@shingroup.cn"<shenghui.qu@shingroup.cn>; "dawei.li@shingroup.cn"<dawei.li@shingroup.cn>; "ke.zhao@shingroup.cn"<ke.zhao@shingroup.cn>; "luming.yu"<luming.yu@gmail.com>;
> > Subject:  Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure
> > 
> > 
> > 
> > 
> > Le 20/02/2024 à 09:51, Christophe Leroy a écrit :
> > > 
> > > 
> > > Le 19/12/2023 à 07:33, Michael Ellerman a écrit :
> > > > Aneesh Kumar K.V <aneesh.kumar@kernel.org> writes:
> > > > > Luming Yu <luming.yu@shingroup.cn> writes:
> > > > > 
> > > > > > Before we have powerpc to use the generic entry infrastructure,
> > > > > > the call to fire user return notifier is made temporarily in powerpc
> > > > > > entry code.
> > > > > > 
> > > > > 
> > > > > It is still not clear what will be registered as user return notifier.
> > > > > Can you summarize that here?
> > > > 
> > > > fire_user_return_notifiers() is defined in kernel/user-return-notifier.c
> > > > 
> > > > That's built when CONFIG_USER_RETURN_NOTIFIER=y.
> > > > 
> > > > That is not user selectable, it's only enabled by:
> > > > 
> > > > arch/x86/kvm/Kconfig:        select USER_RETURN_NOTIFIER
> > > > 
> > > > So it looks to me like (currently) it's always a nop and does nothing.
> > > > 
> > > > Which makes me wonder what the point of wiring this feature up is :)
> > > > Maybe it's needed for some other feature I don't know about?
> > > > 
> > > > Arguably we could just enable it because we can, and it currently does
> > > > nothing so it's unlikely to break anything. But that also makes it
> > > > impossible to test the implementation is correct, and runs the risk that
> > > > one day in the future when it does get enabled only then do we discover
> > > > it doesn't work.
> > > 
> > > Opened an "issue" for the day we need it:
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FKSPP%2Flinux%2Fissues%2F348&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cd9e9b6315413430cfba108dcc7100633%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604118862628419%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=iIYQTodb9zrGdfmzvnZrIVZ%2BKh2qZjMjT29ddkUpGIw%3D&reserved=0
> > 
> > Correct one is https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Flinuxppc%2Fissues%2Fissues%2F477&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7Cd9e9b6315413430cfba108dcc7100633%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604118862637095%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=aJRVdRWu%2F7NQ13jQ6yFLBynXIIfPPPQ3nS4FxiXGNyw%3D&reserved=0
> 

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Christophe Leroy]

Le 28/08/2024 à 08:50, Luming Yu a écrit :
> On Wed, Aug 28, 2024 at 07:46:52AM +0200, Christophe Leroy wrote:
>> Hi,
>>
>> Le 28/08/2024 à 05:17, 虞陆铭 a écrit :
>>> Hi,
>>>
>>> it appears the little feature might require a little bit more work to find its value of the patch.
>>>
>>> Using the following debug module ,  some debugging shows the TIF_USER_RETURN_NOTIFY
>>> bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to
>>> be dropped somewhere on somone who carries the bit return to user space.
>>> side notes:
>>> there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
>>> which should be sovled first to make it easier for further debuggig.
>>
>> As far as I can see, user return notifier infrastructure was implemented in
>> 2009 for KVM on x86, see
>> https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F1253105134-8862-1-git-send-email-avi%40redhat.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C260e5ecf10764312459c08dcc72dc2f5%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604246584044745%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=3hjAzcVu3xOq0QNK5WINQ8trLd9Xp7XCiQjw2htabpQ%3D&reserved=0
>>
>> Can you explain what is your usage of that infrastructure with your patch ?
>> You are talking about debug, what's the added value, what is it used for ?
> one example: I was thinking to live patch kernel at the moment that all cpus are
> either returning to user space or going into idle. But I'm not sure if it is truly
> valuable. secondly, it can help us get more accurate user/system time
> accounting via tracing rather than through sampling technique.
> The third: it could have similar usages in kvm for ppc as x86 for tsc_aux.
> etc :-)

Thanks.

Don't we already have a very accurate user/system time accounting with 
CONFIG_VIRT_CPU_ACCOUNTING_NATIVE ?

Christophe

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Luming Yu]

On Wed, Aug 28, 2024 at 09:27:23AM +0200, Christophe Leroy wrote:
> 
> 
> Le 28/08/2024 à 08:50, Luming Yu a écrit :
> > On Wed, Aug 28, 2024 at 07:46:52AM +0200, Christophe Leroy wrote:
> > > Hi,
> > > 
> > > Le 28/08/2024 à 05:17, 虞陆铭 a écrit :
> > > > Hi,
> > > > 
> > > > it appears the little feature might require a little bit more work to find its value of the patch.
> > > > 
> > > > Using the following debug module ,  some debugging shows the TIF_USER_RETURN_NOTIFY
> > > > bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to
> > > > be dropped somewhere on somone who carries the bit return to user space.
> > > > side notes:
> > > > there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
> > > > which should be sovled first to make it easier for further debuggig.
> > > 
> > > As far as I can see, user return notifier infrastructure was implemented in
> > > 2009 for KVM on x86, see
> > > https://eur01.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fall%2F1253105134-8862-1-git-send-email-avi%40redhat.com%2F&data=05%7C02%7Cchristophe.leroy%40csgroup.eu%7C260e5ecf10764312459c08dcc72dc2f5%7C8b87af7d86474dc78df45f69a2011bb5%7C0%7C0%7C638604246584044745%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C0%7C%7C%7C&sdata=3hjAzcVu3xOq0QNK5WINQ8trLd9Xp7XCiQjw2htabpQ%3D&reserved=0
> > > 
> > > Can you explain what is your usage of that infrastructure with your patch ?
> > > You are talking about debug, what's the added value, what is it used for ?
> > one example: I was thinking to live patch kernel at the moment that all cpus are
> > either returning to user space or going into idle. But I'm not sure if it is truly
> > valuable. secondly, it can help us get more accurate user/system time
> > accounting via tracing rather than through sampling technique.
> > The third: it could have similar usages in kvm for ppc as x86 for tsc_aux.
> > etc :-)
> 
> Thanks.
> 
> Don't we already have a very accurate user/system time accounting with
> CONFIG_VIRT_CPU_ACCOUNTING_NATIVE ?
Yes, there are many such instrumented code on various entry/exit code paths 
that could be removed in future, if we could find that the common call back 
mechanism like user notifier could be more clear and better implementation 
for such needs. :-)
> 
> Christophe
> 
> 

Re: [PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure

[Luming Yu]

Wed, Aug 28, 2024 at 09:27:23AM +0200, Christophe Leroy wrote:
> 
> 
> Le 28/08/2024 à 08:50, Luming Yu a écrit :
> > On Wed, Aug 28, 2024 at 07:46:52AM +0200, Christophe Leroy wrote:
> > > Hi,
> > > 
> > > Le 28/08/2024 à 05:17, 虞陆铭 a écrit :
> > > > Hi,
> > > > 
> > > > it appears the little feature might require a little bit more work to find its value of the patch.
> > > > 
> > > > Using the following debug module ,  some debugging shows the TIF_USER_RETURN_NOTIFY
> > > > bit is propagated in __switch_to among tasks , but USER_RETURN_NOTIFY call back seems to
> > > > be dropped somewhere on somone who carries the bit return to user space.
> > > > side notes:
> > > > there is an issue that the module symbols is not appended to /sys/kernel/debug/tracing/available_filter_functions
this is not a problem as I just noticed that lib/Makefile carries this magic
ccflags-remove-$(CONFIG_FUNCTION_TRACER) += $(CC_FLAGS_FTRACE)