Re: [PATCH] uprobes: Improve scalability by reducing the contention on siglock
From: Oleg Nesterov
Date: Tue Aug 06 2024 - 13:26:14 EST
On 08/06, Liao, Chang wrote:
>
> You're absolutely right. handle_signlestep() has chance to handle _DENY_SIGANL
> unless it followed by setting TIF_UPROBE in uprobe_deny_signal(). This means
> _DENY_SIGNAL is likey replaced during next uprobe single-stepping.
>
> I believe introducing _DENY_SIGNAL as the immediate state between UTASK_SSTEP
> and UTASK_SSTEP_ACK is still necessary. This allow uprobe_post_sstep_notifier()
> to correctly restore TIF_SIGPENDING upon the completion of single-step.
>
> A revised implementation would look like this:
Still looks "obviously wrong" to me... even the approach itself.
Perhaps I am wrong, yet another day when I can't even read emails on lkml
carefully, sorry.
But can you please send the patch which I could actually apply? This one
looks white-space damaged...
I'll try to reply with more details as soon I convince myself I fully
understand what does your patch actually do, but most probably not tomorrow.
Thanks,
Oleg.
> ------------------%<------------------
> --- a/kernel/events/uprobes.c
> +++ b/kernel/events/uprobes.c
> @@ -1980,6 +1980,7 @@ bool uprobe_deny_signal(void)
>
> if (task_sigpending(t)) {
> clear_tsk_thread_flag(t, TIF_SIGPENDING);
> + utask->state = UTASK_SSTEP_DENY_SIGNAL;
>
> if (__fatal_signal_pending(t) || arch_uprobe_xol_was_trapped(t)) {
> utask->state = UTASK_SSTEP_TRAPPED;
> @@ -2276,22 +2277,23 @@ static void handle_singlestep(struct uprobe_task *utask, struct pt_regs *regs)
> int err = 0;
>
> uprobe = utask->active_uprobe;
> - if (utask->state == UTASK_SSTEP_ACK)
> + switch (utask->state) {
> + case UTASK_SSTEP_ACK:
> err = arch_uprobe_post_xol(&uprobe->arch, regs);
> - else if (utask->state == UTASK_SSTEP_TRAPPED)
> + break;
> + case UTASK_SSTEP_TRAPPED:
> arch_uprobe_abort_xol(&uprobe->arch, regs);
> - else
> + set_thread_flag(TIF_SIGPENDING);
> + break;
> + default:
> WARN_ON_ONCE(1);
> + }
>
> put_uprobe(uprobe);
> utask->active_uprobe = NULL;
> utask->state = UTASK_RUNNING;
> xol_free_insn_slot(current);
>
> - spin_lock_irq(¤t->sighand->siglock);
> - recalc_sigpending(); /* see uprobe_deny_signal() */
> - spin_unlock_irq(¤t->sighand->siglock);
> -
> if (unlikely(err)) {
> uprobe_warn(current, "execute the probed insn, sending SIGILL.");
> force_sig(SIGILL);
> @@ -2351,6 +2353,8 @@ int uprobe_post_sstep_notifier(struct pt_regs *regs)
> /* task is currently not uprobed */
> return 0;
>
> + if (utask->state == UTASK_SSTEP_DENY_SIGNAL)
> + set_thread_flag(TIF_SIGPENDING);
> utask->state = UTASK_SSTEP_ACK;
> set_thread_flag(TIF_UPROBE);
> return 1;
>
> ------------------>%------------------
>
> >
> > Oleg.
> >
> >
>
> --
> BR
> Liao, Chang
>