Re: [PATCH v24 07/14] mm/damon: Implement a debugfs-based user space interface

[SeongJae Park]

On Fri, 5 Feb 2021 16:29:41 +0100 Greg KH <greg@xxxxxxxxx> wrote:

> On Thu, Feb 04, 2021 at 04:31:43PM +0100, SeongJae Park wrote:
> > From: SeongJae Park <sjpark@xxxxxxxxx>
> > 
> > DAMON is designed to be used by kernel space code such as the memory
> > management subsystems, and therefore it provides only kernel space API.
> > That said, letting the user space control DAMON could provide some
> > benefits to them.  For example, it will allow user space to analyze
> > their specific workloads and make their own special optimizations.
> > 
> > For such cases, this commit implements a simple DAMON application kernel
> > module, namely 'damon-dbgfs', which merely wraps the DAMON api and
> > exports those to the user space via the debugfs.
> > 
> > 'damon-dbgfs' exports three files, ``attrs``, ``target_ids``, and
> > ``monitor_on`` under its debugfs directory, ``<debugfs>/damon/``.
[...]
> > ---
> >  include/linux/damon.h |   3 +
> >  mm/damon/Kconfig      |   9 +
> >  mm/damon/Makefile     |   1 +
> >  mm/damon/core.c       |  47 +++++
> >  mm/damon/dbgfs.c      | 387 ++++++++++++++++++++++++++++++++++++++++++
> >  5 files changed, 447 insertions(+)
> >  create mode 100644 mm/damon/dbgfs.c
[...]
> > diff --git a/mm/damon/dbgfs.c b/mm/damon/dbgfs.c
> > new file mode 100644
> > index 000000000000..db15380737d1
> > --- /dev/null
> > +++ b/mm/damon/dbgfs.c
[...]
> > +
> > +static int dbgfs_fill_ctx_dir(struct dentry *dir, struct damon_ctx *ctx)
> > +{
> > +	const char * const file_names[] = {"attrs", "target_ids"};
> > +	const struct file_operations *fops[] = {&attrs_fops, &target_ids_fops};
> > +	int i;
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, dir,
> > +					ctx, fops[i])) {
> > +			pr_err("failed to create %s file\n", file_names[i]);
> > +			return -ENOMEM;
> 
> No need to check the return value of this function, just keep going and
> ignore it as there's nothing to do and kernel code should not do
> different things based on the output of any debugfs calls.
> 
> Also, this check is totally wrong and doesn't do what you think it is
> doing...

Ok, I will drop the check.

> 
> > +static int __init __damon_dbgfs_init(void)
> > +{
> > +	struct dentry *dbgfs_root;
> > +	const char * const file_names[] = {"monitor_on"};
> > +	const struct file_operations *fops[] = {&monitor_on_fops};
> > +	int i;
> > +
> > +	dbgfs_root = debugfs_create_dir("damon", NULL);
> > +	if (IS_ERR(dbgfs_root)) {
> > +		pr_err("failed to create the dbgfs dir\n");
> > +		return PTR_ERR(dbgfs_root);
> 
> Again, no need to check anything, just pass the result of a debugfs call
> back into another one just fine.

Ok.

> 
> > +	}
> > +
> > +	for (i = 0; i < ARRAY_SIZE(file_names); i++) {
> > +		if (!debugfs_create_file(file_names[i], 0600, dbgfs_root,
> > +					NULL, fops[i])) {
> 
> Again, this isn't checking what you think it is, so please don't do it.

Got it.

I will fix those as you suggested in the next version.


Thanks,
SeongJae Park

> 
> thanks,
> 
> greg k-h