* Re: [PATCH v2 2/4] binder: Add stats, state and transactions files
[not found] ` <20190830113215.eaa6dfvlhxkmhqc3@wittgenstein>
@ 2019-08-30 18:47 ` Hridya Valsaraju
0 siblings, 0 replies; 2+ messages in thread
From: Hridya Valsaraju @ 2019-08-30 18:47 UTC (permalink / raw)
To: Christian Brauner
Cc: devel, kernel-team, Todd Kjos, Greg Kroah-Hartman, linux-kernel,
Arve Hjønnevåg, Joel Fernandes, Martijn Coenen
On Fri, Aug 30, 2019 at 4:32 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Aug 29, 2019 at 02:18:10PM -0700, Hridya Valsaraju wrote:
> > The following binder stat files currently live in debugfs.
> >
> > /sys/kernel/debug/binder/state
> > /sys/kernel/debug/binder/stats
> > /sys/kernel/debug/binder/transactions
> >
> > This patch makes these files available in a binderfs instance
> > mounted with the mount option 'stats=global'. For example, if a binderfs
> > instance is mounted at path /dev/binderfs, the above files will be
> > available at the following locations:
> >
> > /dev/binderfs/binder_logs/state
> > /dev/binderfs/binder_logs/stats
> > /dev/binderfs/binder_logs/transactions
> >
> > This provides a way to access them even when debugfs is not mounted.
> >
> > Signed-off-by: Hridya Valsaraju <hridya@google.com>
>
> Just two comments below. If you have addressed them you can add my:
>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
Thank you for taking another look Christian, will address both
comments and send out v3 soon :)
>
> > ---
> >
> > Changes in v2:
> > - Consistently name variables across functions as per Christian
> > Brauner.
> > - Improve check for binderfs device in binderfs_evict_inode()
> > as per Christian Brauner.
> >
> > drivers/android/binder.c | 15 ++--
> > drivers/android/binder_internal.h | 8 ++
> > drivers/android/binderfs.c | 140 +++++++++++++++++++++++++++++-
> > 3 files changed, 153 insertions(+), 10 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index ca6b21a53321..de795bd229c4 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -6055,7 +6055,7 @@ static void print_binder_proc_stats(struct seq_file *m,
> > }
> >
> >
> > -static int state_show(struct seq_file *m, void *unused)
> > +int binder_state_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> > struct binder_node *node;
> > @@ -6094,7 +6094,7 @@ static int state_show(struct seq_file *m, void *unused)
> > return 0;
> > }
> >
> > -static int stats_show(struct seq_file *m, void *unused)
> > +int binder_stats_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6110,7 +6110,7 @@ static int stats_show(struct seq_file *m, void *unused)
> > return 0;
> > }
> >
> > -static int transactions_show(struct seq_file *m, void *unused)
> > +int binder_transactions_show(struct seq_file *m, void *unused)
> > {
> > struct binder_proc *proc;
> >
> > @@ -6198,9 +6198,6 @@ const struct file_operations binder_fops = {
> > .release = binder_release,
> > };
> >
> > -DEFINE_SHOW_ATTRIBUTE(state);
> > -DEFINE_SHOW_ATTRIBUTE(stats);
> > -DEFINE_SHOW_ATTRIBUTE(transactions);
> > DEFINE_SHOW_ATTRIBUTE(transaction_log);
> >
> > static int __init init_binder_device(const char *name)
> > @@ -6256,17 +6253,17 @@ static int __init binder_init(void)
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - &state_fops);
> > + &binder_state_fops);
> > debugfs_create_file("stats",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - &stats_fops);
> > + &binder_stats_fops);
> > debugfs_create_file("transactions",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > NULL,
> > - &transactions_fops);
> > + &binder_transactions_fops);
> > debugfs_create_file("transaction_log",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > index fe8c745dc8e0..12ef96f256c6 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -57,4 +57,12 @@ static inline int __init init_binderfs(void)
> > }
> > #endif
> >
> > +int binder_stats_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_stats);
> > +
> > +int binder_state_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_state);
> > +
> > +int binder_transactions_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_transactions);
> > #endif /* _LINUX_BINDER_INTERNAL_H */
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 7045bfe5b52b..0e1e7c87cd33 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -280,7 +280,7 @@ static void binderfs_evict_inode(struct inode *inode)
> >
> > clear_inode(inode);
> >
> > - if (!device)
> > + if (!S_ISCHR(inode->i_mode) || !device)
> > return;
> >
> > mutex_lock(&binderfs_minors_mutex);
> > @@ -502,6 +502,141 @@ static const struct inode_operations binderfs_dir_inode_operations = {
> > .unlink = binderfs_unlink,
> > };
> >
> > +static struct inode *binderfs_make_inode(struct super_block *sb, int mode)
> > +{
> > + struct inode *ret;
> > +
> > + ret = new_inode(sb);
> > + if (ret) {
> > + ret->i_ino = iunique(sb, BINDERFS_MAX_MINOR + INODE_OFFSET);
> > + ret->i_mode = mode;
> > + ret->i_atime = ret->i_mtime = ret->i_ctime = current_time(ret);
> > + }
> > + return ret;
> > +}
> > +
> > +static struct dentry *binderfs_create_dentry(struct dentry *parent,
> > + const char *name)
> > +{
> > + struct dentry *dentry;
> > +
> > + dentry = lookup_one_len(name, parent, strlen(name));
> > + if (IS_ERR(dentry))
> > + return dentry;
> > +
> > + /* Return error if the file/dir already exists. */
> > + if (d_really_is_positive(dentry)) {
> > + dput(dentry);
> > + return ERR_PTR(-EEXIST);
> > + }
> > +
> > + return dentry;
> > +}
> > +
> > +static struct dentry *binderfs_create_file(struct dentry *parent,
> > + const char *name,
> > + const struct file_operations *fops,
> > + void *data)
> > +{
> > + struct dentry *dentry;
> > + struct inode *new_inode, *parent_inode;
> > + struct super_block *sb;
> > +
> > + parent_inode = parent->d_inode;
>
> Note that you're using d_inode(parent) below but parent->d_inode here. :)
>
> > + inode_lock(parent_inode);
> > +
> > + dentry = binderfs_create_dentry(parent, name);
> > + if (IS_ERR(dentry))
> > + goto out;
> > +
> > + sb = parent_inode->i_sb;
> > + new_inode = binderfs_make_inode(sb, S_IFREG | 0444);
> > + if (!new_inode) {
> > + dput(dentry);
> > + dentry = ERR_PTR(-ENOMEM);
> > + goto out;
> > + }
> > +
> > + new_inode->i_fop = fops;
> > + new_inode->i_private = data;
> > + d_instantiate(dentry, new_inode);
> > + fsnotify_create(parent_inode, dentry);
> > +
> > +out:
> > + inode_unlock(parent_inode);
> > + return dentry;
> > +}
> > +
> > +static struct dentry *binderfs_create_dir(struct dentry *parent,
> > + const char *name)
> > +{
> > + struct dentry *dentry;
> > + struct inode *new_inode, *parent_inode;
> > + struct super_block *sb;
> > +
> > + parent_inode = d_inode(parent);
> > + inode_lock(parent_inode);
> > +
> > + dentry = binderfs_create_dentry(parent, name);
> > + if (IS_ERR(dentry))
> > + goto out;
> > +
> > + sb = parent_inode->i_sb;
> > + new_inode = binderfs_make_inode(sb, S_IFDIR | 0755);
> > + if (!new_inode) {
> > + dput(dentry);
> > + dentry = ERR_PTR(-ENOMEM);
> > + goto out;
> > + }
> > +
> > + new_inode->i_fop = &simple_dir_operations;
> > + new_inode->i_op = &simple_dir_inode_operations;
> > +
> > + inc_nlink(new_inode);
>
> This should be set_nlink(new_inode, 2) since noboby can modify it and
> it's also clearer what's happening and what the expected count is.
>
> > + d_instantiate(dentry, new_inode);
> > + inc_nlink(parent_inode);
> > + fsnotify_mkdir(parent_inode, dentry);
> > +
> > +out:
> > + inode_unlock(parent_inode);
> > + return dentry;
> > +}
> > +
> > +static int init_binder_logs(struct super_block *sb)
> > +{
> > + struct dentry *binder_logs_root_dir, *dentry;
> > + int ret = 0;
> > +
> > + binder_logs_root_dir = binderfs_create_dir(sb->s_root,
> > + "binder_logs");
> > + if (IS_ERR(binder_logs_root_dir)) {
> > + ret = PTR_ERR(binder_logs_root_dir);
> > + goto out;
> > + }
> > +
> > + dentry = binderfs_create_file(binder_logs_root_dir, "stats",
> > + &binder_stats_fops, NULL);
> > + if (IS_ERR(dentry)) {
> > + ret = PTR_ERR(dentry);
> > + goto out;
> > + }
> > +
> > + dentry = binderfs_create_file(binder_logs_root_dir, "state",
> > + &binder_state_fops, NULL);
> > + if (IS_ERR(dentry)) {
> > + ret = PTR_ERR(dentry);
> > + goto out;
> > + }
> > +
> > + dentry = binderfs_create_file(binder_logs_root_dir, "transactions",
> > + &binder_transactions_fops, NULL);
> > + if (IS_ERR(dentry))
> > + ret = PTR_ERR(dentry);
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> > {
> > int ret;
> > @@ -580,6 +715,9 @@ static int binderfs_fill_super(struct super_block *sb, void *data, int silent)
> >
> > }
> >
> > + if (info->mount_opts.stats_mode == STATS_GLOBAL)
> > + return init_binder_logs(sb);
> > +
> > return 0;
> > }
> >
> > --
> > 2.23.0.187.g17f5b7556c-goog
> >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [PATCH v2 3/4] binder: Make transaction_log available in binderfs
[not found] ` <20190830113420.ikbjj37cn5qvlhmk@wittgenstein>
@ 2019-08-30 18:54 ` Hridya Valsaraju
0 siblings, 0 replies; 2+ messages in thread
From: Hridya Valsaraju @ 2019-08-30 18:54 UTC (permalink / raw)
To: Christian Brauner
Cc: open list:ANDROID DRIVERS, Android Kernel Team, Todd Kjos,
Greg Kroah-Hartman, LKML, Arve Hjønnevåg,
Joel Fernandes, Martijn Coenen
On Fri, Aug 30, 2019 at 4:34 AM Christian Brauner
<christian.brauner@ubuntu.com> wrote:
>
> On Thu, Aug 29, 2019 at 02:18:11PM -0700, Hridya Valsaraju wrote:
> > Currently, the binder transaction log files 'transaction_log'
> > and 'failed_transaction_log' live in debugfs at the following locations:
> >
> > /sys/kernel/debug/binder/failed_transaction_log
> > /sys/kernel/debug/binder/transaction_log
> >
> > This patch makes these files also available in a binderfs instance
> > mounted with the mount option "stats=global".
> > It does not affect the presence of these files in debugfs.
> > If a binderfs instance is mounted at path /dev/binderfs, the location of
> > these files will be as follows:
> >
> > /dev/binderfs/binder_logs/failed_transaction_log
> > /dev/binderfs/binder_logs/transaction_log
> >
> > This change provides an alternate option to access these files when
> > debugfs is not mounted.
> >
> > Signed-off-by: Hridya Valsaraju <hridya@google.com>
>
> (If you don't change this patch in the next version, please just keep my:
>
> Acked-by: Christian Brauner <christian.brauner@ubuntu.com>
>
> when sending it out. :)
Will do! Thank you Christian!
>
> > ---
> >
> > Changes in v2:
> > -Consistent variable naming accross functions as per Christian Brauner.
> >
> > drivers/android/binder.c | 34 +++++--------------------------
> > drivers/android/binder_internal.h | 30 +++++++++++++++++++++++++++
> > drivers/android/binderfs.c | 18 ++++++++++++++++
> > 3 files changed, 53 insertions(+), 29 deletions(-)
> >
> > diff --git a/drivers/android/binder.c b/drivers/android/binder.c
> > index de795bd229c4..bed217310197 100644
> > --- a/drivers/android/binder.c
> > +++ b/drivers/android/binder.c
> > @@ -197,30 +197,8 @@ static inline void binder_stats_created(enum binder_stat_types type)
> > atomic_inc(&binder_stats.obj_created[type]);
> > }
> >
> > -struct binder_transaction_log_entry {
> > - int debug_id;
> > - int debug_id_done;
> > - int call_type;
> > - int from_proc;
> > - int from_thread;
> > - int target_handle;
> > - int to_proc;
> > - int to_thread;
> > - int to_node;
> > - int data_size;
> > - int offsets_size;
> > - int return_error_line;
> > - uint32_t return_error;
> > - uint32_t return_error_param;
> > - const char *context_name;
> > -};
> > -struct binder_transaction_log {
> > - atomic_t cur;
> > - bool full;
> > - struct binder_transaction_log_entry entry[32];
> > -};
> > -static struct binder_transaction_log binder_transaction_log;
> > -static struct binder_transaction_log binder_transaction_log_failed;
> > +struct binder_transaction_log binder_transaction_log;
> > +struct binder_transaction_log binder_transaction_log_failed;
> >
> > static struct binder_transaction_log_entry *binder_transaction_log_add(
> > struct binder_transaction_log *log)
> > @@ -6166,7 +6144,7 @@ static void print_binder_transaction_log_entry(struct seq_file *m,
> > "\n" : " (incomplete)\n");
> > }
> >
> > -static int transaction_log_show(struct seq_file *m, void *unused)
> > +int binder_transaction_log_show(struct seq_file *m, void *unused)
> > {
> > struct binder_transaction_log *log = m->private;
> > unsigned int log_cur = atomic_read(&log->cur);
> > @@ -6198,8 +6176,6 @@ const struct file_operations binder_fops = {
> > .release = binder_release,
> > };
> >
> > -DEFINE_SHOW_ATTRIBUTE(transaction_log);
> > -
> > static int __init init_binder_device(const char *name)
> > {
> > int ret;
> > @@ -6268,12 +6244,12 @@ static int __init binder_init(void)
> > 0444,
> > binder_debugfs_dir_entry_root,
> > &binder_transaction_log,
> > - &transaction_log_fops);
> > + &binder_transaction_log_fops);
> > debugfs_create_file("failed_transaction_log",
> > 0444,
> > binder_debugfs_dir_entry_root,
> > &binder_transaction_log_failed,
> > - &transaction_log_fops);
> > + &binder_transaction_log_fops);
> > }
> >
> > if (!IS_ENABLED(CONFIG_ANDROID_BINDERFS) &&
> > diff --git a/drivers/android/binder_internal.h b/drivers/android/binder_internal.h
> > index 12ef96f256c6..b9be42d9464c 100644
> > --- a/drivers/android/binder_internal.h
> > +++ b/drivers/android/binder_internal.h
> > @@ -65,4 +65,34 @@ DEFINE_SHOW_ATTRIBUTE(binder_state);
> >
> > int binder_transactions_show(struct seq_file *m, void *unused);
> > DEFINE_SHOW_ATTRIBUTE(binder_transactions);
> > +
> > +int binder_transaction_log_show(struct seq_file *m, void *unused);
> > +DEFINE_SHOW_ATTRIBUTE(binder_transaction_log);
> > +
> > +struct binder_transaction_log_entry {
> > + int debug_id;
> > + int debug_id_done;
> > + int call_type;
> > + int from_proc;
> > + int from_thread;
> > + int target_handle;
> > + int to_proc;
> > + int to_thread;
> > + int to_node;
> > + int data_size;
> > + int offsets_size;
> > + int return_error_line;
> > + uint32_t return_error;
> > + uint32_t return_error_param;
> > + const char *context_name;
> > +};
> > +
> > +struct binder_transaction_log {
> > + atomic_t cur;
> > + bool full;
> > + struct binder_transaction_log_entry entry[32];
> > +};
> > +
> > +extern struct binder_transaction_log binder_transaction_log;
> > +extern struct binder_transaction_log binder_transaction_log_failed;
> > #endif /* _LINUX_BINDER_INTERNAL_H */
> > diff --git a/drivers/android/binderfs.c b/drivers/android/binderfs.c
> > index 0e1e7c87cd33..1715e72ce9c7 100644
> > --- a/drivers/android/binderfs.c
> > +++ b/drivers/android/binderfs.c
> > @@ -630,6 +630,24 @@ static int init_binder_logs(struct super_block *sb)
> >
> > dentry = binderfs_create_file(binder_logs_root_dir, "transactions",
> > &binder_transactions_fops, NULL);
> > + if (IS_ERR(dentry)) {
> > + ret = PTR_ERR(dentry);
> > + goto out;
> > + }
> > +
> > + dentry = binderfs_create_file(binder_logs_root_dir,
> > + "transaction_log",
> > + &binder_transaction_log_fops,
> > + &binder_transaction_log);
> > + if (IS_ERR(dentry)) {
> > + ret = PTR_ERR(dentry);
> > + goto out;
> > + }
> > +
> > + dentry = binderfs_create_file(binder_logs_root_dir,
> > + "failed_transaction_log",
> > + &binder_transaction_log_fops,
> > + &binder_transaction_log_failed);
> > if (IS_ERR(dentry))
> > ret = PTR_ERR(dentry);
> >
> > --
> > 2.23.0.187.g17f5b7556c-goog
> >
_______________________________________________
devel mailing list
devel@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2019-08-30 18:55 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <20190829211812.32520-1-hridya@google.com>
[not found] ` <20190829211812.32520-3-hridya@google.com>
[not found] ` <20190830113215.eaa6dfvlhxkmhqc3@wittgenstein>
2019-08-30 18:47 ` [PATCH v2 2/4] binder: Add stats, state and transactions files Hridya Valsaraju
[not found] ` <20190829211812.32520-4-hridya@google.com>
[not found] ` <20190830113420.ikbjj37cn5qvlhmk@wittgenstein>
2019-08-30 18:54 ` [PATCH v2 3/4] binder: Make transaction_log available in binderfs Hridya Valsaraju
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).