From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 0C856C7EE2D for ; Tue, 23 May 2023 15:40:10 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S237588AbjEWPkI (ORCPT ); Tue, 23 May 2023 11:40:08 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:45156 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S233770AbjEWPkH (ORCPT ); Tue, 23 May 2023 11:40:07 -0400 Received: from out01.mta.xmission.com (out01.mta.xmission.com [166.70.13.231]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 5B781FA for ; Tue, 23 May 2023 08:40:06 -0700 (PDT) Received: from in01.mta.xmission.com ([166.70.13.51]:34724) by out01.mta.xmission.com with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1q1U7I-003qxd-SS; Tue, 23 May 2023 09:40:05 -0600 Received: from ip68-110-29-46.om.om.cox.net ([68.110.29.46]:41738 helo=email.froward.int.ebiederm.org.xmission.com) by in01.mta.xmission.com with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.93) (envelope-from ) id 1q1U7H-007qRy-L3; Tue, 23 May 2023 09:40:04 -0600 From: "Eric W. Biederman" To: "Michael S. Tsirkin" Cc: Mike Christie , oleg@redhat.com, linux@leemhuis.info, nicolas.dichtel@6wind.com, axboe@kernel.dk, torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, virtualization@lists.linux-foundation.org, sgarzare@redhat.com, jasowang@redhat.com, stefanha@redhat.com, brauner@kernel.org References: <20230522025124.5863-1-michael.christie@oracle.com> <20230522025124.5863-4-michael.christie@oracle.com> <20230522153852-mutt-send-email-mst@kernel.org> Date: Tue, 23 May 2023 10:39:56 -0500 In-Reply-To: <20230522153852-mutt-send-email-mst@kernel.org> (Michael S. Tsirkin's message of "Mon, 22 May 2023 15:40:48 -0400") Message-ID: <87cz2r83ar.fsf@email.froward.int.ebiederm.org> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.1 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain X-XM-SPF: eid=1q1U7H-007qRy-L3;;;mid=<87cz2r83ar.fsf@email.froward.int.ebiederm.org>;;;hst=in01.mta.xmission.com;;;ip=68.110.29.46;;;frm=ebiederm@xmission.com;;;spf=pass X-XM-AID: U2FsdGVkX19NgfMb/ImKg6q5N6Tu8xjjRvYEjWNx4d8= X-SA-Exim-Connect-IP: 68.110.29.46 X-SA-Exim-Mail-From: ebiederm@xmission.com Subject: Re: [PATCH 3/3] fork, vhost: Use CLONE_THREAD to fix freezer/ps regression X-SA-Exim-Version: 4.2.1 (built Sat, 08 Feb 2020 21:53:50 +0000) X-SA-Exim-Scanned: Yes (on in01.mta.xmission.com) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org "Michael S. Tsirkin" writes: > On Sun, May 21, 2023 at 09:51:24PM -0500, Mike Christie wrote: >> When switching from kthreads to vhost_tasks two bugs were added: >> 1. The vhost worker tasks's now show up as processes so scripts doing ps >> or ps a would not incorrectly detect the vhost task as another process. >> 2. kthreads disabled freeze by setting PF_NOFREEZE, but vhost tasks's >> didn't disable or add support for them. >> >> To fix both bugs, this switches the vhost task to be thread in the >> process that does the VHOST_SET_OWNER ioctl, and has vhost_worker call >> get_signal to support SIGKILL/SIGSTOP and freeze signals. Note that >> SIGKILL/STOP support is required because CLONE_THREAD requires >> CLONE_SIGHAND which requires those 2 signals to be suppported. >> >> This a modified version of patch originally written by Linus which >> handles his review comment to himself to rename ignore_signals to >> block_signals to better represent what it now does. And it includes a >> change to vhost_worker() to support SIGSTOP/KILL and freeze, and it >> drops the wait use per Oleg's review comment that it's no longer needed >> when using CLONE_THREAD. >> >> Fixes: 6e890c5d5021 ("vhost: use vhost_tasks for worker threads") >> Signed-off-by: Mike Christie >> --- >> drivers/vhost/vhost.c | 17 ++++++++++++++++- >> include/linux/sched/task.h | 2 +- >> kernel/fork.c | 12 +++--------- >> kernel/signal.c | 1 + >> kernel/vhost_task.c | 16 ++++------------ >> 5 files changed, 25 insertions(+), 23 deletions(-) >> >> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c >> index a92af08e7864..bf83e9340e40 100644 >> --- a/drivers/vhost/vhost.c >> +++ b/drivers/vhost/vhost.c >> @@ -338,6 +338,7 @@ static int vhost_worker(void *data) >> struct vhost_worker *worker = data; >> struct vhost_work *work, *work_next; >> struct llist_node *node; >> + bool dead = false; >> >> for (;;) { >> /* mb paired w/ kthread_stop */ >> @@ -349,8 +350,22 @@ static int vhost_worker(void *data) >> } >> >> node = llist_del_all(&worker->work_list); >> - if (!node) >> + if (!node) { >> schedule(); >> + /* >> + * When we get a SIGKILL our release function will >> + * be called. That will stop new IOs from being queued >> + * and check for outstanding cmd responses. It will then >> + * call vhost_task_stop to tell us to return and exit. >> + */ >> + if (!dead && signal_pending(current)) { >> + struct ksignal ksig; >> + >> + dead = get_signal(&ksig); >> + if (dead) >> + clear_thread_flag(TIF_SIGPENDING); > > > Does get_signal actually return true only on SIGKILL then? get_signal returns the signal that was dequeued, or 0 if no signal was dequeued. For these threads that block all but SIGSTOP and SIGKILL get_signal should properly never return any signal. As SIGSTOP and SIGKILL are handled internally to get_signal. However get_signal was modified to have a special case for io_uring and now the vhost code so that extra cleanup can be performed, before do_exit is called on the thread. That special case causes get_signal to return when with the value of SIGKILL when the process exits. The process can exit with do_group_exit aka exit(2) or any fatal signal not just SIGKILL, and get_signal on these threads will return. Eric