MIME-Version: 1.0
References: <20230202232517.8695-1-michael.christie@oracle.com> <20230202232517.8695-7-michael.christie@oracle.com>
In-Reply-To: <20230202232517.8695-7-michael.christie@oracle.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Thu, 2 Feb 2023 16:43:17 -0800
Message-ID: <CAHk-=wiEx+G7bwOj6MsHgpWavVkPAg6c1D1zcg4cgugg+ccrPQ@mail.gmail.com>
Subject: Re: [PATCH v11 6/8] vhost_task: Allow vhost layer to use copy_process
To:     Mike Christie <michael.christie@oracle.com>
Cc:     hch@infradead.org, stefanha@redhat.com, jasowang@redhat.com,
        mst@redhat.com, sgarzare@redhat.com,
        virtualization@lists.linux-foundation.org, brauner@kernel.org,
        ebiederm@xmission.com, konrad.wilk@oracle.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Feb 2, 2023 at 3:25 PM Mike Christie
<michael.christie@oracle.com> wrote:
>
> +/**
> + * vhost_task_start - start a vhost_task created with vhost_task_create
> + * @vtsk: vhost_task to wake up
> + * @namefmt: printf-style format string for the thread name
> + */
> +void vhost_task_start(struct vhost_task *vtsk, const char namefmt[], ...)
> +{
> +       char name[TASK_COMM_LEN];
> +       va_list args;
> +
> +       va_start(args, namefmt);
> +       vsnprintf(name, sizeof(name), namefmt, args);
> +       set_task_comm(vtsk->task, name);
> +       va_end(args);
> +
> +       wake_up_new_task(vtsk->task);
> +}

Ok, I like this more than what we do for the IO workers - they set
their own names themselves once they start running, rather than have
the creator do it like this.

At the same time, my reaction to this was "why do we need to go
through that temporary 'name[]' buffer at all?"

And I think this patch is very much correct to do so, because
"copy_thread()" has already exposed the new thread to the rest of the
world, even though it hasn't actually started running yet.

So I think this is all doing the right thing, and I like how it does
it better than what io_uring does, BUT...

It does make me think that maybe we should make that task name
handling part of copy_process(), and simply create the task name
before we need this careful set_task_comm() with a temporary buffer.

Because if we just did it in copy_process() before the new task has
been exposed anywhere,. we could just do it as

        if (args->name)
            vsnprintf(tsk->comm, TASK_COMM_LEN, "%s-%d", args->name, tsk->pid);

or something like that.

Not a big deal, it was just me reacting to this patch with "do we
really need set_task_comm() when we're creating the task?"

               Linus