On Tue, Aug 27, 2019 at 01:41:50PM -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 <hri...@google.com> > --- > drivers/android/binder.c | 15 ++-- > drivers/android/binder_internal.h | 8 ++ > drivers/android/binderfs.c | 137 +++++++++++++++++++++++++++++- > 3 files changed, 150 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 d95d179aec58..d542f9b8d8ab 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 (!device || S_ISREG(inode->i_mode))
Hm, remind me why we need the S_ISREG again? Also we probably should do: if (S_ISREG(inode->i_mode) || !device) should this maybe be: if (!S_ISCHR(inode->i_mode) || !device) ? > return; > > mutex_lock(&binderfs_minors_mutex); > @@ -504,6 +504,138 @@ 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 *dir, > + const char *name) > +{ > + struct dentry *dentry; > + > + dentry = lookup_one_len(name, dir, 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 *dir, const char > *name, > + const struct file_operations *fops, > + void *data) > +{ > + struct dentry *dentry; > + struct inode *new_inode, *dir_inode; > + struct super_block *sb; > + > + dir_inode = dir->d_inode; > + inode_lock(dir_inode); > + > + dentry = binderfs_create_dentry(dir, name); > + if (IS_ERR(dentry)) > + goto out; > + > + sb = dir_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(dir_inode, dentry); > + > +out: > + inode_unlock(dir_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); For consistency, could you use the same variable name for the directory in which you create a new dentry? I don't care if its "dir_inode" like above or "parent_inode". > + 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); > + d_instantiate(dentry, new_inode); > + inc_nlink(parent_inode); > + fsnotify_mkdir(parent_inode, dentry); > +out: For consistency please leave a \n after fsnotify_mkdir and the goto label like you did in the function above. > + inode_unlock(parent_inode); > + return dentry; > +} > + > +static int init_binder_logs(struct super_block *sb) > +{ > + struct dentry *binder_logs_root_dir, *file_dentry; Why "file_dentry" and not just simply "dentry" like everywhere else? > + 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; > + } > + > + file_dentry = binderfs_create_file(binder_logs_root_dir, "stats", > + &binder_stats_fops, NULL); > + if (IS_ERR(file_dentry)) { > + ret = PTR_ERR(file_dentry); > + goto out; > + } > + > + file_dentry = binderfs_create_file(binder_logs_root_dir, "state", > + &binder_state_fops, NULL); > + if (IS_ERR(file_dentry)) { > + ret = PTR_ERR(file_dentry); > + goto out; > + } > + > + file_dentry = binderfs_create_file(binder_logs_root_dir, "transactions", > + &binder_transactions_fops, NULL); > + if (IS_ERR(file_dentry)) > + ret = PTR_ERR(file_dentry); > +out: > + return ret; > +} > + > static int binderfs_fill_super(struct super_block *sb, void *data, int > silent) > { > int ret; > @@ -582,6 +714,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 >