From: Stanislav Fomichev <sdf@xxxxxxxxxxx>
Date: Fri, 30 Aug 2024 15:56:12 -0700
> On 08/30, Alexander Lobakin wrote:
>> Currently, kthread_{create,run}_on_cpu() doesn't support varargs like
>> kthread_create{,_on_node}() do, which makes them less convenient to
>> use.
>> Convert them to take varargs as the last argument. The only difference
>> is that they always append the CPU ID at the end and require the format
>> string to have an excess '%u' at the end due to that. That's still true;
>> meanwhile, the compiler will correctly point out to that if missing.
>> One more nice side effect is that you can now use the underscored
>> __kthread_create_on_cpu() if you want to override that rule and not
>> have CPU ID at the end of the name.
>> The current callers are not anyhow affected.
>>
>> Signed-off-by: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
>> ---
>> include/linux/kthread.h | 51 ++++++++++++++++++++++++++---------------
>> kernel/kthread.c | 22 ++++++++++--------
>> 2 files changed, 45 insertions(+), 28 deletions(-)
>>
>> diff --git a/include/linux/kthread.h b/include/linux/kthread.h
>> index b11f53c1ba2e..27a94e691948 100644
>> --- a/include/linux/kthread.h
>> +++ b/include/linux/kthread.h
>> @@ -27,11 +27,21 @@ struct task_struct *kthread_create_on_node(int (*threadfn)(void *data),
>> #define kthread_create(threadfn, data, namefmt, arg...) \
>> kthread_create_on_node(threadfn, data, NUMA_NO_NODE, namefmt, ##arg)
>>
>> -
>> -struct task_struct *kthread_create_on_cpu(int (*threadfn)(void *data),
>> - void *data,
>> - unsigned int cpu,
>> - const char *namefmt);
>> +__printf(4, 5)
>> +struct task_struct *__kthread_create_on_cpu(int (*threadfn)(void *data),
>> + void *data, unsigned int cpu,
>> + const char *namefmt, ...);
>> +
>> +#define kthread_create_on_cpu(threadfn, data, cpu, namefmt, ...) \
>> + _kthread_create_on_cpu(threadfn, data, cpu, __UNIQUE_ID(cpu_), \
>> + namefmt, ##__VA_ARGS__)
>> +
>> +#define _kthread_create_on_cpu(threadfn, data, cpu, uc, namefmt, ...) ({ \
>> + u32 uc = (cpu); \
>> + \
>> + __kthread_create_on_cpu(threadfn, data, uc, namefmt, \
>> + ##__VA_ARGS__, uc); \
>> +})
>>
>> void get_kthread_comm(char *buf, size_t buf_size, struct task_struct *tsk);
>> bool set_kthread_struct(struct task_struct *p);
>> @@ -62,25 +72,28 @@ bool kthread_is_per_cpu(struct task_struct *k);
>> * @threadfn: the function to run until signal_pending(current).
>> * @data: data ptr for @threadfn.
>> * @cpu: The cpu on which the thread should be bound,
>> - * @namefmt: printf-style name for the thread. Format is restricted
>> - * to "name.*%u". Code fills in cpu number.
>> + * @namefmt: printf-style name for the thread. Must have an excess '%u'
>> + * at the end as kthread_create_on_cpu() fills in CPU number.
>> *
>> * Description: Convenient wrapper for kthread_create_on_cpu()
>> * followed by wake_up_process(). Returns the kthread or
>> * ERR_PTR(-ENOMEM).
>> */
>> -static inline struct task_struct *
>> -kthread_run_on_cpu(int (*threadfn)(void *data), void *data,
>> - unsigned int cpu, const char *namefmt)
>> -{
>> - struct task_struct *p;
>> -
>> - p = kthread_create_on_cpu(threadfn, data, cpu, namefmt);
>> - if (!IS_ERR(p))
>> - wake_up_process(p);
>> -
>> - return p;
>> -}
>> +#define kthread_run_on_cpu(threadfn, data, cpu, namefmt, ...) \
>> + _kthread_run_on_cpu(threadfn, data, cpu, __UNIQUE_ID(task_), \
>> + namefmt, ##__VA_ARGS__)
>> +
>> +#define _kthread_run_on_cpu(threadfn, data, cpu, ut, namefmt, ...) \
>> +({ \
>> + struct task_struct *ut; \
>> + \
>> + ut = kthread_create_on_cpu(threadfn, data, cpu, namefmt, \
>> + ##__VA_ARGS__); \
>> + if (!IS_ERR(ut)) \
>> + wake_up_process(ut); \
>> + \
>> + ut; \
>> +})
>
> Why do you need to use __UNIQUE_ID here? Presumably ({}) in _kthread_run_on_cpu
It will still be a -Wshadow warning if the caller has a variable with
the same name. I know it's enabled only on W=2, but anyway I feel like
we shouldn't introduce any new warnings when possible.
> should be enough to avoid the issue of non unique variable in the parent
> scope. (and similar kthread_run isn't using any __UNIQUE_IDs)
>
> The rest of the patches look good.
Thanks,
Olek
