MIME-Version: 1.0
References: <20230213010020.1813-1-michael.christie@oracle.com> <20230213010020.1813-5-michael.christie@oracle.com>
In-Reply-To: <20230213010020.1813-5-michael.christie@oracle.com>
From:   Linus Torvalds <torvalds@linux-foundation.org>
Date:   Mon, 13 Feb 2023 11:54:23 -0800
Message-ID: <CAHk-=wj0oA0VV77eAgMYFSJm7SBQ15vDLFjcFnNvWrbCRXXSpA@mail.gmail.com>
Subject: Re: [PATCH 4/5] kernel: Prepare set_kthread_struct to be used for setup_thread_fn
To:     Mike Christie <michael.christie@oracle.com>
Cc:     brauner@kernel.org, ebiederm@xmission.com,
        linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Feb 12, 2023 at 5:00 PM Mike Christie
<michael.christie@oracle.com> wrote:
>
> This preps set_kthread_struct to be used for a setup_thread_fn callback
> by having it set the task's comm and also returning an int instead of a
> bool.

Ok, so I like the concept, but this patch is just too ugly for words,
and very very confused.

Now, some of it is pre-exisging nasty code just moved around:

> +       mutex_lock(&create->name_mutex);
> +       if (!create->name_args) {
> +               mutex_unlock(&create->name_mutex);
> +               return -EINTR;
> +       }
> +
> +       va_copy(name_args, *create->name_args);
> +       len = vsnprintf(tsk->comm, TASK_COMM_LEN, create->name_fmt, name_args);
> +       va_end(name_args);
> +       if (len >= TASK_COMM_LEN) {
> +               /* leave it truncated when out of memory. */
> +               kthread->full_name = kvasprintf(GFP_KERNEL, create->name_fmt,
> +                                               *create->name_args);
> +       }
> +       mutex_unlock(&create->name_mutex);

The *whole* point of my suggestion was to stop having silly pointless
locking on the name, because this all should be local to that one
thread creation, so that "name_mutex" kind of makes this all
pointless,

But what the heck is this:

> +       mutex_init(&create->name_mutex);
> +       create->name_fmt = namefmt;
> +       va_copy(name_args, args);
> +       create->name_args = &name_args;

That's just crazy talk.

Please just create the name once.

And please don't think that just because it was using a "va_list", you
need to keep it in that format.

Just make it create the name in __kthread_create_on_node() and be done
with it. That code already does a

        struct kthread_create_info *create = kmalloc(sizeof(*create), ..

and you can just make a sufficiently large buffer there. Don't worry
about "kthread->fuil_name" being huge, it should just be bigger than
16. Make it be 32 or something. Nobody wants a larger "full name"
anyway.

No name_mutex, no va_list that is bigger than the buffer we'd need for
the name anyway. Just "create the name once".

IOW, this patch is just being much too complicated for no good reason.
The point was to make it _simpler_ to do thread setup, not more
complicated.

        Linus