Re: [PATCH bpf-next 2/9] kthread: allow vararg kthread_{create,run}_on_cpu()

[Stanislav Fomichev]

On 09/03, Alexander Lobakin wrote:
> 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.

Makes sense, thanks! That's why, presumably, kthread_run uses __k name
to avoid the warning..