From: Jens Axboe <axboe@kernel.dk> To: Linus Torvalds <torvalds@linux-foundation.org>, Christian Brauner <brauner@kernel.org> Cc: Thorsten Leemhuis <linux@leemhuis.info>, Mike Christie <michael.christie@oracle.com>, nicolas.dichtel@6wind.com, Linux kernel regressions list <regressions@lists.linux.dev>, hch@infradead.org, stefanha@redhat.com, jasowang@redhat.com, mst@redhat.com, sgarzare@redhat.com, virtualization@lists.linux-foundation.org, ebiederm@xmission.com, konrad.wilk@oracle.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 8/8] vhost: use vhost_tasks for worker threads Date: Mon, 15 May 2023 09:52:11 -0600 [thread overview] Message-ID: <33b84605-1d0c-1b0e-7927-7ffa96b3c308@kernel.dk> (raw) In-Reply-To: <CAHk-=wgXJ5VS1iBkfsG=HDjsyhn5XYDKt5xhQcNuz-e7VKyg8A@mail.gmail.com> On 5/15/23 9:44?AM, Linus Torvalds wrote: > On Mon, May 15, 2023 at 7:23?AM Christian Brauner <brauner@kernel.org> wrote: >> >> So I think we will be able to address (1) and (2) by making vhost tasks >> proper threads and blocking every signal except for SIGKILL and SIGSTOP >> and then having vhost handle get_signal() - as you mentioned - the same >> way io uring already does. We should also remove the ingore_signals >> thing completely imho. I don't think we ever want to do this with user >> workers. > > Right. That's what IO_URING does: > > if (args->io_thread) { > /* > * Mark us an IO worker, and block any signal that isn't > * fatal or STOP > */ > p->flags |= PF_IO_WORKER; > siginitsetinv(&p->blocked, sigmask(SIGKILL)|sigmask(SIGSTOP)); > } > > and I really think that vhost should basically do exactly what io_uring does. > > Not because io_uring fundamentally got this right - but simply because > io_uring had almost all the same bugs (and then some), and what the > io_uring worker threads ended up doing was to basically zoom in on > "this works". > > And it zoomed in on it largely by just going for "make it look as much > as possible as a real user thread", because every time the kernel > thread did something different, it just caused problems. This is exactly what I told Christian in a private chat too - we went through all of that, and this is what works. KISS. > So I think the patch should just look something like the attached. > Mike, can you test this on whatever vhost test-suite? Seems like that didn't get attached... > I did consider getting rid of ".ignore_signals" entirely, and instead > just keying the "block signals" behavior off the ".user_worker" flag. > But this approach doesn't seem wrong either, and I don't think it's > wrong to make the create_io_thread() function say that > ".ignore_signals = 1" thing explicitly, rather than key it off the > ".io_thread" flag. > > Jens/Christian - comments? > > Slightly related to this all: I think vhost should also do > CLONE_FILES, and get rid of the whole ".no_files" thing. Again, if > vhost doesn't use any files, it shouldn't matter, and looking > different just to be different is wrong. But if vhost doesn't use any > files, the current situation shouldn't be a bug either. Only potential downside is that it does make file references more expensive for other syscalls, since you now have a shared file table. But probably not something to worry about here? -- Jens Axboe
WARNING: multiple messages have this Message-ID (diff)
From: Jens Axboe <axboe@kernel.dk> To: Linus Torvalds <torvalds@linux-foundation.org>, Christian Brauner <brauner@kernel.org> Cc: Linux kernel regressions list <regressions@lists.linux.dev>, mst@redhat.com, linux-kernel@vger.kernel.org, konrad.wilk@oracle.com, Thorsten Leemhuis <linux@leemhuis.info>, virtualization@lists.linux-foundation.org, hch@infradead.org, ebiederm@xmission.com, stefanha@redhat.com, nicolas.dichtel@6wind.com Subject: Re: [PATCH v11 8/8] vhost: use vhost_tasks for worker threads Date: Mon, 15 May 2023 09:52:11 -0600 [thread overview] Message-ID: <33b84605-1d0c-1b0e-7927-7ffa96b3c308@kernel.dk> (raw) In-Reply-To: <CAHk-=wgXJ5VS1iBkfsG=HDjsyhn5XYDKt5xhQcNuz-e7VKyg8A@mail.gmail.com> On 5/15/23 9:44?AM, Linus Torvalds wrote: > On Mon, May 15, 2023 at 7:23?AM Christian Brauner <brauner@kernel.org> wrote: >> >> So I think we will be able to address (1) and (2) by making vhost tasks >> proper threads and blocking every signal except for SIGKILL and SIGSTOP >> and then having vhost handle get_signal() - as you mentioned - the same >> way io uring already does. We should also remove the ingore_signals >> thing completely imho. I don't think we ever want to do this with user >> workers. > > Right. That's what IO_URING does: > > if (args->io_thread) { > /* > * Mark us an IO worker, and block any signal that isn't > * fatal or STOP > */ > p->flags |= PF_IO_WORKER; > siginitsetinv(&p->blocked, sigmask(SIGKILL)|sigmask(SIGSTOP)); > } > > and I really think that vhost should basically do exactly what io_uring does. > > Not because io_uring fundamentally got this right - but simply because > io_uring had almost all the same bugs (and then some), and what the > io_uring worker threads ended up doing was to basically zoom in on > "this works". > > And it zoomed in on it largely by just going for "make it look as much > as possible as a real user thread", because every time the kernel > thread did something different, it just caused problems. This is exactly what I told Christian in a private chat too - we went through all of that, and this is what works. KISS. > So I think the patch should just look something like the attached. > Mike, can you test this on whatever vhost test-suite? Seems like that didn't get attached... > I did consider getting rid of ".ignore_signals" entirely, and instead > just keying the "block signals" behavior off the ".user_worker" flag. > But this approach doesn't seem wrong either, and I don't think it's > wrong to make the create_io_thread() function say that > ".ignore_signals = 1" thing explicitly, rather than key it off the > ".io_thread" flag. > > Jens/Christian - comments? > > Slightly related to this all: I think vhost should also do > CLONE_FILES, and get rid of the whole ".no_files" thing. Again, if > vhost doesn't use any files, it shouldn't matter, and looking > different just to be different is wrong. But if vhost doesn't use any > files, the current situation shouldn't be a bug either. Only potential downside is that it does make file references more expensive for other syscalls, since you now have a shared file table. But probably not something to worry about here? -- Jens Axboe _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization
next prev parent reply other threads:[~2023-05-15 15:52 UTC|newest] Thread overview: 176+ messages / expand[flat|nested] mbox.gz Atom feed top 2023-02-02 23:25 [PATCH v11 0/8] Use copy_process in vhost layer Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-02 23:25 ` [PATCH v11 1/8] fork: Make IO worker options flag based Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-03 0:14 ` Linus Torvalds 2023-02-03 0:14 ` Linus Torvalds 2023-02-02 23:25 ` [PATCH v11 2/8] fork/vm: Move common PF_IO_WORKER behavior to new flag Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-02 23:25 ` [PATCH v11 3/8] fork: add USER_WORKER flag to not dup/clone files Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-03 0:16 ` Linus Torvalds 2023-02-03 0:16 ` Linus Torvalds 2023-02-02 23:25 ` [PATCH v11 4/8] fork: Add USER_WORKER flag to ignore signals Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-03 0:19 ` Linus Torvalds 2023-02-03 0:19 ` Linus Torvalds 2023-02-05 16:06 ` Mike Christie 2023-02-05 16:06 ` Mike Christie 2023-02-02 23:25 ` [PATCH v11 5/8] fork: allow kernel code to call copy_process Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-02 23:25 ` [PATCH v11 6/8] vhost_task: Allow vhost layer to use copy_process Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-03 0:43 ` Linus Torvalds 2023-02-03 0:43 ` Linus Torvalds 2023-02-02 23:25 ` [PATCH v11 7/8] vhost: move worker thread fields to new struct Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-02-02 23:25 ` [PATCH v11 8/8] vhost: use vhost_tasks for worker threads Mike Christie 2023-02-02 23:25 ` Mike Christie 2023-05-05 13:40 ` Nicolas Dichtel 2023-05-05 18:22 ` Linus Torvalds 2023-05-05 18:22 ` Linus Torvalds 2023-05-05 22:37 ` Mike Christie 2023-05-05 22:37 ` Mike Christie 2023-05-06 1:53 ` Linus Torvalds 2023-05-06 1:53 ` Linus Torvalds 2023-05-08 17:13 ` Christian Brauner 2023-05-09 8:09 ` Nicolas Dichtel 2023-05-09 8:17 ` Nicolas Dichtel 2023-05-13 12:39 ` Thorsten Leemhuis 2023-05-13 12:39 ` Thorsten Leemhuis 2023-05-13 15:08 ` Linus Torvalds 2023-05-13 15:08 ` Linus Torvalds 2023-05-15 14:23 ` Christian Brauner 2023-05-15 15:44 ` Linus Torvalds 2023-05-15 15:44 ` Linus Torvalds 2023-05-15 15:52 ` Jens Axboe [this message] 2023-05-15 15:52 ` Jens Axboe 2023-05-15 15:54 ` Linus Torvalds 2023-05-15 15:54 ` Linus Torvalds 2023-05-15 17:23 ` Linus Torvalds 2023-05-15 17:23 ` Linus Torvalds 2023-05-15 15:56 ` Linus Torvalds 2023-05-15 15:56 ` Linus Torvalds 2023-05-15 22:23 ` Mike Christie 2023-05-15 22:23 ` Mike Christie 2023-05-15 22:54 ` Linus Torvalds 2023-05-15 22:54 ` Linus Torvalds 2023-05-16 3:53 ` Mike Christie 2023-05-16 3:53 ` Mike Christie 2023-05-16 13:18 ` Oleg Nesterov 2023-05-16 13:18 ` Oleg Nesterov 2023-05-16 13:40 ` Oleg Nesterov 2023-05-16 13:40 ` Oleg Nesterov 2023-05-16 15:56 ` Eric W. Biederman 2023-05-16 15:56 ` Eric W. Biederman 2023-05-16 18:37 ` Oleg Nesterov 2023-05-16 18:37 ` Oleg Nesterov 2023-05-16 20:12 ` Eric W. Biederman 2023-05-16 20:12 ` Eric W. Biederman 2023-05-17 17:09 ` Oleg Nesterov 2023-05-17 17:09 ` Oleg Nesterov 2023-05-17 18:22 ` Mike Christie 2023-05-17 18:22 ` Mike Christie 2023-05-16 8:39 ` Christian Brauner 2023-05-16 16:24 ` Mike Christie 2023-05-16 16:24 ` Mike Christie 2023-05-16 16:44 ` Christian Brauner 2023-05-19 12:15 ` [RFC PATCH 0/8] vhost_tasks: Use CLONE_THREAD/SIGHAND Christian Brauner 2023-06-01 7:58 ` Thorsten Leemhuis 2023-06-01 7:58 ` Thorsten Leemhuis 2023-06-01 10:18 ` Nicolas Dichtel 2023-06-01 10:47 ` Christian Brauner 2023-06-01 11:29 ` Thorsten Leemhuis 2023-06-01 11:29 ` Thorsten Leemhuis 2023-06-01 12:26 ` Linus Torvalds 2023-06-01 12:26 ` Linus Torvalds 2023-06-01 16:10 ` Mike Christie 2023-06-01 16:10 ` Mike Christie 2023-05-16 14:06 ` [PATCH v11 8/8] vhost: use vhost_tasks for worker threads Linux regression tracking #adding (Thorsten Leemhuis) 2023-05-26 9:03 ` Linux regression tracking #update (Thorsten Leemhuis) 2023-06-02 11:38 ` Thorsten Leemhuis 2023-07-20 13:06 ` Michael S. Tsirkin 2023-07-20 13:06 ` Michael S. Tsirkin 2023-07-23 4:03 ` michael.christie 2023-07-23 4:03 ` michael.christie 2023-07-23 9:31 ` Michael S. Tsirkin 2023-07-23 9:31 ` Michael S. Tsirkin 2023-08-10 18:57 ` Michael S. Tsirkin 2023-08-10 18:57 ` Michael S. Tsirkin 2023-08-11 18:51 ` Mike Christie 2023-08-11 18:51 ` Mike Christie 2023-08-13 19:01 ` Michael S. Tsirkin 2023-08-13 19:01 ` Michael S. Tsirkin 2023-08-14 3:13 ` michael.christie 2023-08-14 3:13 ` michael.christie 2023-02-07 8:19 ` [PATCH v11 0/8] Use copy_process in vhost layer Christian Brauner 2023-05-18 0:09 [RFC PATCH 0/8] vhost_tasks: Use CLONE_THREAD/SIGHAND Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 0:09 ` [RFC PATCH 1/8] signal: Dequeue SIGKILL even if SIGNAL_GROUP_EXIT/group_exec_task is set Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 2:34 ` Eric W. Biederman 2023-05-18 2:34 ` Eric W. Biederman 2023-05-18 3:49 ` Eric W. Biederman 2023-05-18 3:49 ` Eric W. Biederman 2023-05-18 15:21 ` Mike Christie 2023-05-18 15:21 ` Mike Christie 2023-05-18 16:25 ` Oleg Nesterov 2023-05-18 16:25 ` Oleg Nesterov 2023-05-18 16:42 ` Mike Christie 2023-05-18 16:42 ` Mike Christie 2023-05-18 17:04 ` Oleg Nesterov 2023-05-18 17:04 ` Oleg Nesterov 2023-05-18 18:28 ` Eric W. Biederman 2023-05-18 18:28 ` Eric W. Biederman 2023-05-18 22:57 ` Mike Christie 2023-05-18 22:57 ` Mike Christie 2023-05-19 4:16 ` Eric W. Biederman 2023-05-19 4:16 ` Eric W. Biederman 2023-05-19 23:24 ` Mike Christie 2023-05-19 23:24 ` Mike Christie 2023-05-22 13:30 ` Oleg Nesterov 2023-05-22 13:30 ` Oleg Nesterov 2023-05-18 8:08 ` Christian Brauner 2023-05-18 15:27 ` Mike Christie 2023-05-18 15:27 ` Mike Christie 2023-05-18 17:07 ` Christian Brauner 2023-05-18 18:08 ` Oleg Nesterov 2023-05-18 18:08 ` Oleg Nesterov 2023-05-18 18:12 ` Christian Brauner 2023-05-18 18:23 ` Oleg Nesterov 2023-05-18 18:23 ` Oleg Nesterov 2023-05-18 0:09 ` [RFC PATCH 2/8] vhost/vhost_task: Hook vhost layer into signal handler Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 0:16 ` Linus Torvalds 2023-05-18 0:16 ` Linus Torvalds 2023-05-18 1:01 ` Mike Christie 2023-05-18 1:01 ` Mike Christie 2023-05-18 8:16 ` Christian Brauner 2023-05-18 0:09 ` [RFC PATCH 3/8] fork/vhost_task: Switch to CLONE_THREAD and CLONE_SIGHAND Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 8:18 ` Christian Brauner 2023-05-18 0:09 ` [RFC PATCH 4/8] vhost-net: Move vhost_net_open Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 0:09 ` [RFC PATCH 5/8] vhost: Add callback that stops new work and waits on running ones Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 14:18 ` Christian Brauner 2023-05-18 15:03 ` Mike Christie 2023-05-18 15:03 ` Mike Christie 2023-05-18 15:09 ` Christian Brauner 2023-05-18 18:38 ` Eric W. Biederman 2023-05-18 18:38 ` Eric W. Biederman 2023-05-18 0:09 ` [RFC PATCH 6/8] vhost-scsi: Add callback to stop and wait on works Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 0:09 ` [RFC PATCH 7/8] vhost-net: " Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 0:09 ` [RFC PATCH 8/8] fork/vhost_task: remove no_files Mike Christie 2023-05-18 0:09 ` Mike Christie 2023-05-18 1:04 ` Mike Christie 2023-05-18 1:04 ` Mike Christie 2023-05-18 12:31 ` kernel test robot 2023-05-18 15:30 ` kernel test robot 2023-05-18 23:14 ` kernel test robot 2023-05-19 7:26 ` kernel test robot 2023-05-18 8:25 ` [RFC PATCH 0/8] vhost_tasks: Use CLONE_THREAD/SIGHAND Christian Brauner 2023-05-18 8:40 ` Christian Brauner 2023-05-18 14:30 ` Christian Brauner
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=33b84605-1d0c-1b0e-7927-7ffa96b3c308@kernel.dk \ --to=axboe@kernel.dk \ --cc=brauner@kernel.org \ --cc=ebiederm@xmission.com \ --cc=hch@infradead.org \ --cc=jasowang@redhat.com \ --cc=konrad.wilk@oracle.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@leemhuis.info \ --cc=michael.christie@oracle.com \ --cc=mst@redhat.com \ --cc=nicolas.dichtel@6wind.com \ --cc=regressions@lists.linux.dev \ --cc=sgarzare@redhat.com \ --cc=stefanha@redhat.com \ --cc=torvalds@linux-foundation.org \ --cc=virtualization@lists.linux-foundation.org \ /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: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.