From: Guo Ren <guoren@kernel.org>
To: Jisheng Zhang <jszhang@kernel.org>
Cc: Paul Walmsley <paul.walmsley@sifive.com>,
Palmer Dabbelt <palmer@dabbelt.com>,
Albert Ou <aou@eecs.berkeley.edu>,
Nathan Chancellor <nathan@kernel.org>,
Nick Desaulniers <ndesaulniers@google.com>,
linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
llvm@lists.linux.dev
Subject: Re: [PATCH 2/4] riscv: consolidate ret_from_kernel_thread into ret_from_fork
Date: Tue, 27 Sep 2022 07:55:27 +0800 [thread overview]
Message-ID: <CAJF2gTRbvvMcdm7ZrAeow=Dm7=HXx59+4g-3mBqb2-SN5mOpZg@mail.gmail.com> (raw)
In-Reply-To: <YzHNr13U+SdwJHo1@xhacker>
On Tue, Sep 27, 2022 at 12:14 AM Jisheng Zhang <jszhang@kernel.org> wrote:
>
> On Mon, Sep 26, 2022 at 07:25:30AM +0800, Guo Ren wrote:
> > On Mon, Sep 26, 2022 at 2:03 AM Jisheng Zhang <jszhang@kernel.org> wrote:
> > >
> > > The ret_from_kernel_thread() behaves similarly with ret_from_fork(),
> > > the only difference is whether call the fn(arg) or not, this can be
> > > acchieved by testing fn is NULL or not, I.E s0 is 0 or not.
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@kernel.org>
> > > ---
> > > arch/riscv/kernel/entry.S | 11 +++--------
> > > arch/riscv/kernel/process.c | 5 ++---
> > > 2 files changed, 5 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/arch/riscv/kernel/entry.S b/arch/riscv/kernel/entry.S
> > > index 2207cf44a3bc..a3e1ed2fa2ac 100644
> > > --- a/arch/riscv/kernel/entry.S
> > > +++ b/arch/riscv/kernel/entry.S
> > > @@ -323,20 +323,15 @@ END(handle_kernel_stack_overflow)
> > >
> > > ENTRY(ret_from_fork)
> > > call schedule_tail
> > > - move a0, sp /* pt_regs */
> > > - la ra, ret_from_exception
> > > - tail syscall_exit_to_user_mode
> > > -ENDPROC(ret_from_fork)
> > > -
> > > -ENTRY(ret_from_kernel_thread)
> > > - call schedule_tail
> > > + beqz s0, 1f /* not from kernel thread */
>
> Hi Guo,
>
> > We can't use s0 as condition for ret_from_fork/ret_from_kernel_thread.
> > The s0=0 is also okay for ret_from_fork.
>
> IIUC, in ret_from_fork, the s0 comes p->thread.s[0] rather than s0 in
> pt_regs.
Yes, you are correct.
>
> >
> > /* p->thread holds context to be restored by __switch_to() */
> > if (unlikely(args->fn)) {
> > /* Kernel thread */
> > memset(childregs, 0, sizeof(struct pt_regs));
> > childregs->gp = gp_in_global;
> > /* Supervisor/Machine, irqs on: */
> > childregs->status = SR_PP | SR_PIE;
> >
> > p->thread.ra = (unsigned long)ret_from_kernel_thread;
> > p->thread.s[0] = (unsigned long)args->fn;
> > p->thread.s[1] = (unsigned long)args->fn_arg;
> > } else {
> > *childregs = *(current_pt_regs());
> > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
Oh, I'm wrong, It's switch_to_restore -> exception_restore.
> > if (usp) /* User fork */
> > childregs->sp = usp;
> > if (clone_flags & CLONE_SETTLS)
> > childregs->tp = tls;
> > childregs->a0 = 0; /* Return value of fork() */
> > p->thread.ra = (unsigned long)ret_from_fork;
> > }
> > p->thread.sp = (unsigned long)childregs; /* kernel sp */
> >
>
> <snip>
>
> > > @@ -182,8 +180,9 @@ int copy_thread(struct task_struct *p, const struct kernel_clone_args *args)
> > > if (clone_flags & CLONE_SETTLS)
> > > childregs->tp = tls;
> > > childregs->a0 = 0; /* Return value of fork() */
> > > - p->thread.ra = (unsigned long)ret_from_fork;
> > > + p->thread.s[0] = 0;
>
> Here we assign 0 to p->thread.s[0]
I missed that.
Merge thread & fork is not a good idea, and using fp as the flag is so implicit.
➜ linux git:(rv64sv32) grep ret_from_fork arch -r | grep entry.S
arch/arc/kernel/entry.S:ENTRY(ret_from_fork)
arch/arc/kernel/entry.S:END(ret_from_fork)
arch/csky/kernel/entry.S:ENTRY(ret_from_fork)
arch/x86/kernel/process_32.c: * the task-switch, and shows up in
ret_from_fork in entry.S,
arch/alpha/kernel/entry.S: .globl ret_from_fork
arch/alpha/kernel/entry.S: .ent ret_from_fork
arch/alpha/kernel/entry.S:ret_from_fork:
arch/alpha/kernel/entry.S:.end ret_from_fork
arch/loongarch/kernel/entry.S:SYM_CODE_START(ret_from_fork)
arch/loongarch/kernel/entry.S:SYM_CODE_END(ret_from_fork)
arch/hexagon/kernel/vm_entry.S: .globl ret_from_fork
arch/hexagon/kernel/vm_entry.S:ret_from_fork:
arch/microblaze/kernel/entry.S: (copy_thread makes ret_from_fork the
return address in each new thread's
arch/microblaze/kernel/entry.S:C_ENTRY(ret_from_fork):
arch/m68k/kernel/entry.S:ENTRY(ret_from_fork)
arch/arm64/kernel/entry.S:SYM_CODE_START(ret_from_fork)
arch/arm64/kernel/entry.S:SYM_CODE_END(ret_from_fork)
arch/arm64/kernel/entry.S:NOKPROBE(ret_from_fork)
arch/riscv/kernel/entry.S:ENTRY(ret_from_fork)
arch/riscv/kernel/entry.S:ENDPROC(ret_from_fork)
arch/s390/kernel/entry.S:# a new process exits the kernel with ret_from_fork
arch/s390/kernel/entry.S:ENTRY(ret_from_fork)
arch/s390/kernel/entry.S: brasl %r14,__ret_from_fork
arch/s390/kernel/entry.S:ENDPROC(ret_from_fork)
arch/mips/kernel/entry.S:FEXPORT(ret_from_fork)
arch/openrisc/kernel/entry.S: /* All syscalls return here... just
pay attention to ret_from_fork
arch/openrisc/kernel/entry.S:ENTRY(ret_from_fork)
arch/openrisc/kernel/entry.S: * that may be either schedule(),
ret_from_fork(), or
arch/nios2/kernel/entry.S:ENTRY(ret_from_fork)
arch/xtensa/kernel/entry.S:ENTRY(ret_from_fork)
arch/xtensa/kernel/entry.S:ENDPROC(ret_from_fork)
arch/sparc/kernel/entry.S: .globl ret_from_fork
arch/sparc/kernel/entry.S:ret_from_fork:
➜ linux git:(rv64sv32) grep ret_from_kernel_thread arch -r | grep entry.S
arch/csky/kernel/entry.S:ENTRY(ret_from_kernel_thread)
arch/alpha/kernel/entry.S: .globl ret_from_kernel_thread
arch/alpha/kernel/entry.S: .ent ret_from_kernel_thread
arch/alpha/kernel/entry.S:ret_from_kernel_thread:
arch/alpha/kernel/entry.S:.end ret_from_kernel_thread
arch/parisc/kernel/entry.S:ENTRY(ret_from_kernel_thread)
arch/parisc/kernel/entry.S:END(ret_from_kernel_thread)
arch/loongarch/kernel/entry.S:SYM_CODE_START(ret_from_kernel_thread)
arch/loongarch/kernel/entry.S:SYM_CODE_END(ret_from_kernel_thread)
arch/microblaze/kernel/entry.S:C_ENTRY(ret_from_kernel_thread):
arch/m68k/kernel/entry.S:ENTRY(ret_from_kernel_thread)
arch/riscv/kernel/entry.S:ENTRY(ret_from_kernel_thread)
arch/riscv/kernel/entry.S:ENDPROC(ret_from_kernel_thread)
arch/mips/kernel/entry.S:FEXPORT(ret_from_kernel_thread)
arch/openrisc/kernel/entry.S: * ret_from_kernel_thread(). If we
are returning to a new thread,
arch/nios2/kernel/entry.S:ENTRY(ret_from_kernel_thread)
arch/xtensa/kernel/entry.S:ENTRY(ret_from_kernel_thread)
arch/xtensa/kernel/entry.S:ENDPROC(ret_from_kernel_thread)
arch/sparc/kernel/entry.S: .globl ret_from_kernel_thread
arch/sparc/kernel/entry.S:ret_from_kernel_thread:
Many architectures use a similar style. If you want to continue the
patch, I think you should first rename ret_from_fork properly, and
give an explicit flag definition, not just setting fp = 0.
>
> Thanks
--
Best Regards
Guo Ren
next prev parent reply other threads:[~2022-09-26 23:55 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-09-25 17:53 [PATCH 0/4] riscv: entry: further clean up and VMAP_STACK fix Jisheng Zhang
2022-09-25 17:53 ` [PATCH 1/4] riscv: remove extra level wrappers of trace_hardirqs_{on,off} Jisheng Zhang
2022-09-25 23:18 ` Guo Ren
2022-09-25 17:53 ` [PATCH 2/4] riscv: consolidate ret_from_kernel_thread into ret_from_fork Jisheng Zhang
2022-09-25 23:25 ` Guo Ren
2022-09-26 16:05 ` Jisheng Zhang
2022-09-26 23:55 ` Guo Ren [this message]
2022-09-28 16:40 ` Jisheng Zhang
2022-09-30 11:42 ` Guo Ren
2022-09-25 17:53 ` [PATCH 3/4] riscv: fix race when vmap stack overflow and remove shadow_stack Jisheng Zhang
2022-09-25 23:46 ` Guo Ren
2022-09-27 0:18 ` Jisheng Zhang
2022-09-27 2:00 ` Guo Ren
2022-11-23 17:02 ` Deepak Gupta
2022-09-25 17:53 ` [PATCH 4/4] riscv: entry: consolidate general regs saving into save_gp Jisheng Zhang
2022-09-25 23:29 ` Guo Ren
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAJF2gTRbvvMcdm7ZrAeow=Dm7=HXx59+4g-3mBqb2-SN5mOpZg@mail.gmail.com' \
--to=guoren@kernel.org \
--cc=aou@eecs.berkeley.edu \
--cc=jszhang@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-riscv@lists.infradead.org \
--cc=llvm@lists.linux.dev \
--cc=nathan@kernel.org \
--cc=ndesaulniers@google.com \
--cc=palmer@dabbelt.com \
--cc=paul.walmsley@sifive.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).