Re: [PATCH net-next v4 1/5] printk: Add execution context (task name/CPU) to printk_info

[Breno Leitao <leitao@debian.org>]

Hello John,

On Tue, Jan 27, 2026 at 09:49:25PM +0106, John Ogness wrote:
> On 2026-01-23, Breno Leitao <leitao@debian.org> wrote:
> > diff --git a/kernel/printk/nbcon.c b/kernel/printk/nbcon.c
> > index 32fc12e536752..391a58be0c5b3 100644
> > --- a/kernel/printk/nbcon.c
> > +++ b/kernel/printk/nbcon.c
> > @@ -946,6 +946,19 @@ void nbcon_reacquire_nobuf(struct nbcon_write_context *wctxt)
> >  }
> >  EXPORT_SYMBOL_GPL(nbcon_reacquire_nobuf);
> >  
> > +#ifdef CONFIG_PRINTK_EXECUTION_CTX
> > +static void wctxt_load_execution_ctx(struct nbcon_write_context *wctxt,
> > +				     struct printk_message *pmsg)
> > +{
> > +	wctxt->cpu = pmsg->cpu;
> > +	wctxt->pid = pmsg->pid;
> > +	memcpy(wctxt->comm, pmsg->comm, TASK_COMM_LEN);
> 
> Perhaps using sizeof() instead?
> 
> 	memcpy(wctxt->comm, pmsg->comm, sizeof(wctxt->comm));
> 
> And adding a static assert that the sizes match?
> 
> 	static_assert(sizeof(wctxt->comm) == sizeof(pmsg->comm));

Sure, I move the size from TASK_COMM_LEN to sizeof(wctxt->comm) and also
adding the build time assert.

> > diff --git a/kernel/printk/printk.c b/kernel/printk/printk.c
> > index 1d765ad242b82..7daaa27705339 100644
> > --- a/kernel/printk/printk.c
> > +++ b/kernel/printk/printk.c
> > @@ -2131,12 +2131,37 @@ static inline void printk_delay(int level)
> >  	}
> >  }
> >  
> > +#define CALLER_ID_MASK 0x80000000
> > +
> >  static inline u32 printk_caller_id(void)
> >  {
> >  	return in_task() ? task_pid_nr(current) :
> > -		0x80000000 + smp_processor_id();
> > +		CALLER_ID_MASK + smp_processor_id();
> > +}
> > +
> > +#ifdef CONFIG_PRINTK_EXECUTION_CTX
> > +/* Store the opposite info than caller_id. */
> > +static u32 printk_caller_id2(void)
> > +{
> > +	return !in_task() ? task_pid_nr(current) :
> > +		CALLER_ID_MASK + smp_processor_id();
> >  }
> >  
> > +static pid_t printk_info_get_pid(const struct printk_info *info)
> > +{
> > +	u32 caller_id = info->caller_id;
> > +	u32 caller_id2 = info->caller_id2;
> > +
> > +	return caller_id & CALLER_ID_MASK ? caller_id2 : caller_id;
> > +}
> > +
> > +static int printk_info_get_cpu(const struct printk_info *info)
> > +{
> > +	return ((info->caller_id & CALLER_ID_MASK ?
> > +		 info->caller_id : info->caller_id2) & ~CALLER_ID_MASK);
> > +}
> 
> It is a bit odd that printk_info_get_pid() uses local variables and
> printk_info_get_cpu() does not. I could understand if things evolve that
> way over time, but it is odd to use the two different styles from the
> beginning.
>
> I would prefer the local variable variant. But mostly I would prefer
> that they are the same style.

They evolve different, I will get both of them using local variables and
in the same style.

> > +
> > +static void pmsg_load_execution_ctx(struct printk_message *pmsg,
> > +				    const struct printk_info *info)
> > +{
> > +	pmsg->cpu = printk_info_get_cpu(info);
> > +	pmsg->pid = printk_info_get_pid(info);
> > +	memcpy(pmsg->comm, info->comm, TASK_COMM_LEN);
> 
> Here I also suggest using sizeof() and static_assert():
> 
> 	memcpy(pmsg->comm, info->comm, sizeof(pmsg->comm));
> 	static_assert(sizeof(pmsg->comm) == sizeof(info->comm));

Ack! I will respin it.

Thanks for the review,
--breno