Re: [PATCH] printk: add cpu id information to printk() output

[Petr Mladek]

On Fri 2023-09-22 15:20:37, Enlin Mu wrote:
> Petr Mladek <pmladek@xxxxxxxx> 于2023年9月16日周六 00:34写道：
> >
> > On Fri 2023-09-15 11:53:13, Greg KH wrote:
> > > On Fri, Sep 15, 2023 at 04:46:02PM +0800, Enlin Mu wrote:
> > > > John Ogness <john.ogness@xxxxxxxxxxxxx> 于2023年9月15日周五 16:34写道：
> > > > >
> > > > > On 2023-09-15, Enlin Mu <enlin.mu@xxxxxxxxxxx> wrote:
> > > > > > Sometimes we want to print cpu id of printk() messages to consoles
> > > > > >
> > > > > > diff --git a/include/linux/threads.h b/include/linux/threads.h
> > > > > > index c34173e6c5f1..6700bd9a174f 100644
> > > > > > --- a/include/linux/threads.h
> > > > > > +++ b/include/linux/threads.h
> > > > > > @@ -34,6 +34,9 @@
> > > > > >  #define PID_MAX_LIMIT (CONFIG_BASE_SMALL ? PAGE_SIZE * 8 : \
> > > > > >       (sizeof(long) > 4 ? 4 * 1024 * 1024 : PID_MAX_DEFAULT))
> > > > > >
> > > > > > +#define CPU_ID_SHIFT 23
> > > > > > +#define CPU_ID_MASK  0xff800000
> > > > >
> > > > > This only supports 256 CPUs. I think it doesn't make sense to try to
> > > > > squish CPU and Task IDs into 32 bits.
> > > > Yes, it is not good way,
> > > > >
> > > > > What about introducing a caller_id option to always only print the CPU
> > > > > ID? Or do you really need Task _and_ CPU?
> > > >    Yes, I need it.Because I need to know which CPU is printing the
> > > > log, so that I can identify the current system operation, such as load
> > > > situation and CPU busy/idle status
> > >
> > > The cpu that is printing the log isn't the one that added the log
> > > message, so I think you will have incorrect data here, right?
> >
> > We already store some metadata about the caller:
> >
> >  * All fields are set by the printk code except for @seq, which is
> >  * set by the ringbuffer code.
> >  */
> > struct printk_info {
> >         u64     seq;            /* sequence number */
> >         u64     ts_nsec;        /* timestamp in nanoseconds */
> >         u16     text_len;       /* length of text message */
> >         u8      facility;       /* syslog facility */
> >         u8      flags:5;        /* internal record flags */
> >         u8      level:3;        /* syslog level */
> >         u32     caller_id;      /* thread id or processor id */
> >
> >         struct dev_printk_info  dev_info;
> > };
> >
> > The 32-bit caller ID is generated using:
> >
> > static inline u32 printk_caller_id(void)
> > {
> >         return in_task() ? task_pid_nr(current) :
> >                 0x80000000 + smp_processor_id();
> > }
> >
> > We could add more metadata and always store the CPU ID and something
> > like:
> >
> >    [CTXT][ Tpid][  Ccpu]
> >
> > for example
> >
> >    [TASK][  T234][    C4]
> >    [ IRQ][ T4567][   C17]
> >    [SIRQ][    T5][    C0]
> >    [ NMI][  T356][  C128]
> >
> Greate!
> Do you have a plan to push it to linus?

No. It was just a POC. It would require much more effort to make
it ready for upstream, see below.

> > The biggest problem is that it would change the format of the
> > ringbuffer so that it would require updating external tools,
> > working with crashdump, especially crash but there are also
> > alternative python extensions for gdb.

It would require patches for the crash tool,
./scripts/gdb/linux/dmesg.py,
Documentation/admin-guide/kdump/gdbmacros.txt

> > See below POC of the kernel part. It is not even compile tested. The size
> > of the buffers is updated by a guess. Comments are not updated, ...

And of course, make the POC working, update comments, ...

I am sorry but I do not have enough time and motivation to do so.
But I could answer questions, review the patches, ... when any
interested person start working on it.

Best Regards,
Petr