linux-kernel - Re: [PATCH bpf-next 2/9] kthread: allow vararg kthread_{create,run}_on

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <235dcd89-54a6-43ca-bbb3-45dfd6db97e6@intel.com>
Date: Tue, 3 Sep 2024 14:25:58 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Stanislav Fomichev <sdf@...ichev.me>
CC: Alexei Starovoitov <ast@...nel.org>, Daniel Borkmann
	<daniel@...earbox.net>, Andrii Nakryiko <andrii@...nel.org>, Lorenzo Bianconi
	<lorenzo@...nel.org>, Daniel Xu <dxu@...uu.xyz>, John Fastabend
	<john.fastabend@...il.com>, Jesper Dangaard Brouer <hawk@...nel.org>, "Martin
 KaFai Lau" <martin.lau@...ux.dev>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, "Paolo
 Abeni" <pabeni@...hat.com>, <bpf@...r.kernel.org>, <netdev@...r.kernel.org>,
	<linux-kernel@...r.kernel.org>
Subject: Re: [PATCH bpf-next 2/9] kthread: allow vararg
 kthread_{create,run}_on_cpu()

From: Stanislav Fomichev <sdf@...ichev.me>
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@...el.com>
>> ---
>>  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