[PATCH v1 2/2] powerpc/debug: hook to user return notifier infrastructure
Luming Yu
luming.yu at shingroup.cn
Wed Aug 28 16:50:19 AEST 2024
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 at 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 at csgroup.eu>;
> > Date: Tue, Feb 20, 2024 05:02 PM
> > To: "mpe"<mpe at ellerman.id.au>; "Aneesh Kumar K.V"<aneesh.kumar at kernel.org>; "虞陆铭"<luming.yu at shingroup.cn>; "linuxppc-dev"<linuxppc-dev at lists.ozlabs.org>; "linux-kernel"<linux-kernel at vger.kernel.org>; "npiggin"<npiggin at gmail.com>;
> > Cc: "shenghui.qu at shingroup.cn"<shenghui.qu at shingroup.cn>; "dawei.li at shingroup.cn"<dawei.li at shingroup.cn>; "ke.zhao at shingroup.cn"<ke.zhao at shingroup.cn>; "luming.yu"<luming.yu at 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 at kernel.org> writes:
> > > > > Luming Yu <luming.yu at 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
>
More information about the Linuxppc-dev
mailing list