Re: [PATCH] Binder: Add timestamp and async from pid/tid to transaction record

[Greg KH <gregkh@linuxfoundation.org>]

On Thu, Apr 13, 2023 at 07:33:26PM +0800, Chuang Zhang wrote:
> On Thu, Apr 13, 2023 at 6:51 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > >       spin_lock(&t->lock);
> > >       to_proc = t->to_proc;
> > >       seq_printf(m,
> > > -                "%s %d: %pK from %d:%d to %d:%d code %x flags %x pri
> > %ld r%d",
> > > +                "%s %d: %pK from %d:%d to %d:%d code %x elapsed %lldms
> > flags %x pri %ld r%d",
> >
> > Why not add this at the end of the line instead of in the middle?  That
> > way any existing code that looks at this line isn't as broken as it
> > might be with your change :)
> >
> >> [chuang] Ok, so move it after t->need_reply or buffer->user_data?

After need_reply?

> > > --- a/drivers/android/binder_internal.h
> > > +++ b/drivers/android/binder_internal.h
> > > @@ -528,6 +528,9 @@ struct binder_transaction {
> > >       long    priority;
> > >       long    saved_priority;
> > >       kuid_t  sender_euid;
> > > +     int async_from_pid;
> > > +     int async_from_tid;
> >
> > As you are just using 0/1 for these variables, why not add them to the
> > bitfield location in this structure instead of wasting a whole int for
> > both of them?
> >
> >> [chuang] They are used to store the caller's process ID and thread ID,
> not just 1 or 0, so I understand that integers are still needed to save
> them. thanks!

Ah, I missed that these really were the pid itself.  Then an int will
not work at all, it has to be pid_t, right?

And how are pid namespaces being handled here?  I know it's just
debugging data but showing a pid from the wrong namespace probably would
not be a good idea as it might get confusing quickly.

thanks,

greg k-h