public inbox for [email protected]
 help / color / mirror / Atom feed
From: Oleg Nesterov <[email protected]>
To: Thomas Gleixner <[email protected]>
Cc: Jens Axboe <[email protected]>,
	[email protected], [email protected],
	[email protected]
Subject: Re: [PATCH 4/5] x86: wire up TIF_NOTIFY_SIGNAL
Date: Fri, 16 Oct 2020 12:54:16 +0200	[thread overview]
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>

On 10/16, Thomas Gleixner wrote:
>
> On Thu, Oct 15 2020 at 16:34, Oleg Nesterov wrote:
> > On 10/15, Thomas Gleixner wrote:
> >> Instead of adding this to every architectures signal magic, we can
> >> handle TIF_NOTIFY_SIGNAL in the core code:
> >>
> >> static void handle_singal_work(ti_work, regs)
> >> {
> >> 	if (ti_work & _TIF_NOTIFY_SIGNAL)
> >>         	tracehook_notify_signal();
> >>
> >>         arch_do_signal(ti_work, regs);
> >> }
> >>
> >>       loop {
> >>       		if (ti_work & (SIGPENDING | NOTIFY_SIGNAL))
> >>                 	handle_signal_work(ti_work, regs);
> >>       }
> >
> > To me this looks like unnecessary complication. We need to change
> > every architecture anyway, how can this helper help?
>
> This is about the generic entry code. For the users of that it makes
> absolutely no sense to have that in architecture code.
>
> Something which every architecture needs to do in the exactly same way
> goes into the common code. If not, you can spare the exercise of having
> common code in the first place.
>
> Also arch_do_signal() becomes a misnomer with this new magic.

Well, to me arch_do_signal() paths should handle the signal_pending() == T
case.

But I won't argue, this is subjective.

> static void handle_signal_work(ti_work, regs)
> {
> 	if (ti_work & _TIF_NOTIFY_SIGNAL)
>         	tracehook_notify_signal();
>
>         arch_do_signal_or_restart(ti_work, regs);
> }
>
> which makes it entirely clear what this is about.

In this case I'd prefer to pass the "(ti_work & _TIF_SIGPENDING)" boolen
to arch_do_signal_or_restart().

But again, I won't argue. And to remind, we do not really need to touch
arch_do_signal() at all. We can just add

	if (test_thread_flag(TIF_NOTIFY_SIGNAL))
		tracehook_notify_signal();

	if (!task_sigpending(current))
		return 0;

at the start of get_signal() and avoid the code duplication automatically.

Oleg.


  reply	other threads:[~2020-10-16 10:54 UTC|newest]

Thread overview: 46+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-10-15 13:16 [PATCHSET v5] Add support for TIF_NOTIFY_SIGNAL Jens Axboe
2020-10-15 13:16 ` [PATCH 1/5] tracehook: clear TIF_NOTIFY_RESUME in tracehook_notify_resume() Jens Axboe
2020-10-15 14:42   ` Oleg Nesterov
2020-10-15 14:43     ` Jens Axboe
2020-10-15 13:16 ` [PATCH 2/5] kernel: add task_sigpending() helper Jens Axboe
2020-10-15 14:42   ` Oleg Nesterov
2020-10-15 13:16 ` [PATCH 3/5] kernel: add support for TIF_NOTIFY_SIGNAL Jens Axboe
2020-10-15 14:31   ` Oleg Nesterov
2020-10-15 14:33     ` Jens Axboe
2020-10-15 14:37       ` Oleg Nesterov
2020-10-15 14:43         ` Jens Axboe
2020-10-15 14:47           ` Oleg Nesterov
2020-10-15 14:53             ` Oleg Nesterov
2020-10-15 14:56               ` Jens Axboe
2020-10-15 15:01         ` Thomas Gleixner
2020-10-15 15:27           ` Oleg Nesterov
2020-10-15 14:44   ` Oleg Nesterov
2020-10-15 13:17 ` [PATCH 4/5] x86: wire up TIF_NOTIFY_SIGNAL Jens Axboe
2020-10-15 14:11   ` Thomas Gleixner
2020-10-15 14:31     ` Jens Axboe
2020-10-15 14:34       ` Thomas Gleixner
2020-10-15 14:35         ` Jens Axboe
2020-10-15 14:36       ` Oleg Nesterov
2020-10-15 14:42         ` Jens Axboe
2020-10-15 14:34     ` Oleg Nesterov
2020-10-15 14:54       ` Thomas Gleixner
2020-10-15 15:17         ` Oleg Nesterov
2020-10-16  9:55       ` Thomas Gleixner
2020-10-16 10:54         ` Oleg Nesterov [this message]
2020-10-16 13:07           ` Thomas Gleixner
2020-10-15 14:44   ` Oleg Nesterov
2020-10-20 10:57   ` introduce asm-generic/thread_info.h ? Oleg Nesterov
2020-10-15 13:17 ` [PATCH 5/5] task_work: use TIF_NOTIFY_SIGNAL if available Jens Axboe
2020-10-15 15:49   ` Oleg Nesterov
2020-10-15 18:39     ` Jens Axboe
2020-10-16  9:00       ` Thomas Gleixner
2020-10-16  9:39         ` Thomas Gleixner
2020-10-16 13:35           ` Jens Axboe
2020-10-16 14:17             ` Thomas Gleixner
2020-10-16 14:51               ` Oleg Nesterov
2020-10-16 14:53                 ` Jens Axboe
2020-10-16 18:03                   ` Thomas Gleixner
2020-10-16 18:05                     ` Jens Axboe
2020-10-16 13:33         ` Jens Axboe
2020-10-16 14:11           ` Thomas Gleixner
2020-10-16 14:22             ` Jens Axboe

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 \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    [email protected] \
    /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