Re: [PATCH v11 8/8] vhost: use vhost_tasks for worker threads

From: Linus Torvalds <torvalds@linux-foundation.org>
To: Christian Brauner <brauner@kernel.org>
Cc: Jens Axboe <axboe@kernel.dk>,
	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 08:44:52 -0700	[thread overview]
Message-ID: <CAHk-=wgXJ5VS1iBkfsG=HDjsyhn5XYDKt5xhQcNuz-e7VKyg8A@mail.gmail.com> (raw)
In-Reply-To: <20230515-vollrausch-liebgeworden-2765f3ca3540@brauner>

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.

So I think the patch should just look something like the attached.
Mike, can you test this on whatever vhost test-suite?

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.

                     Linus
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization