Virtualization - May 2023 - [PATCH v11 8/8] vhost: use vhost_tasks for worker threads

On 5/15/23 9:44?AM, Linus Torvalds wrote:
> On Mon, May 15, 2023 at 7:23?AM Christian Brauner <brauner at
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

On Mon, May 15, 2023 at 8:52?AM Jens Axboe <axboe at kernel.dk>
wrote:
>
> On 5/15/23 9:44?AM, Linus Torvalds wrote:
> >
> > 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...
Blush. I decided to built-test it, and then forgot to attach it. Here.

                  Linus
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patch.diff
Type: text/x-patch
Size: 1712 bytes
Desc: not available
URL:
<http://lists.linuxfoundation.org/pipermail/virtualization/attachments/20230515/ab843347/attachment.bin>

On Mon, May 15, 2023 at 8:52?AM Jens Axboe <axboe at kernel.dk>
wrote:
>
> 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?
Would the vhost user worker user processes ever be otherwise single-threaded?

I'd *assume* that a vhost user is already doing its own threads. But
maybe that's a completely bogus assumption. I don't actually use any
of this, so...

Because you are obviously 100% right that if you're otherwise
single-threaded, then a CLONE_FILES kernel helper thread will cause
the extra cost for file descriptor lookup/free due to all the race
prevention.

                 Linus