Re: [PATCH v26 07/13] mm/damon: Implement a debugfs-based user space interface
From: SeongJae Park On Tue, 30 Mar 2021 09:59:50 + SeongJae Park wrote: > From: SeongJae Park > > On Tue, 30 Mar 2021 11:22:45 +0200 Greg KH wrote: > > > On Tue, Mar 30, 2021 at 09:05:31AM +, sj38.p...@gmail.com wrote: > > > +static int __init __damon_dbgfs_init(void) > > > +{ > > > + struct dentry *dbgfs_root; > > > + const char * const file_names[] = {"monitor_on"}; > > > + const struct file_operations *fops[] = {_on_fops}; > > > + int i; > > > + > > > + dbgfs_root = debugfs_create_dir("damon", NULL); > > > + > > > + for (i = 0; i < ARRAY_SIZE(file_names); i++) > > > + debugfs_create_file(file_names[i], 0600, dbgfs_root, NULL, > > > + fops[i]); > > > + dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]); > > > + > > > + dbgfs_dirs = kmalloc_array(1, sizeof(dbgfs_root), GFP_KERNEL); > > > > No error checking for memory allocation failures? > > Oops, I will add the check in the next spin. > > > > > > > > + dbgfs_dirs[0] = dbgfs_root; > > > + > > > + return 0; > > > +} > > > + > > > +/* > > > + * Functions for the initialization > > > + */ > > > + > > > +static int __init damon_dbgfs_init(void) > > > +{ > > > + int rc; > > > + > > > + dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL); > > > > No error checking? > > Will add in the next spin. > > > > > > + dbgfs_ctxs[0] = dbgfs_new_ctx(); > > > + if (!dbgfs_ctxs[0]) > > > + return -ENOMEM; And, I found that I'm not printing the error for this failure. I guess this might made you to to think below error message should printed inside the callee. I will add 'pr_err()' here and above unchecked failure case, in the next version. BTW, I forgot saying thank you for your review. Appreciate! Thanks, SeongJae Park > > > + dbgfs_nr_ctxs = 1; > > > + > > > + rc = __damon_dbgfs_init(); > > > + if (rc) > > > + pr_err("%s: dbgfs init failed\n", __func__); > > > > Shouldn't the error be printed out in the function that failed, not in > > this one? > > I thought some other functions (in future) might want to use > '__damon_dbgfs_init()' but siletnly handles it's failure. Therefore I made > the > function fails silently but returns the error code explicitly. Am I missing > somthing? > > > Thanks, > SeongJae Park > > > > > thanks, > > > > greg k-h >
Re: [PATCH v26 07/13] mm/damon: Implement a debugfs-based user space interface
From: SeongJae Park On Tue, 30 Mar 2021 11:22:45 +0200 Greg KH wrote: > On Tue, Mar 30, 2021 at 09:05:31AM +, sj38.p...@gmail.com wrote: > > +static int __init __damon_dbgfs_init(void) > > +{ > > + struct dentry *dbgfs_root; > > + const char * const file_names[] = {"monitor_on"}; > > + const struct file_operations *fops[] = {_on_fops}; > > + int i; > > + > > + dbgfs_root = debugfs_create_dir("damon", NULL); > > + > > + for (i = 0; i < ARRAY_SIZE(file_names); i++) > > + debugfs_create_file(file_names[i], 0600, dbgfs_root, NULL, > > + fops[i]); > > + dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]); > > + > > + dbgfs_dirs = kmalloc_array(1, sizeof(dbgfs_root), GFP_KERNEL); > > No error checking for memory allocation failures? Oops, I will add the check in the next spin. > > > > + dbgfs_dirs[0] = dbgfs_root; > > + > > + return 0; > > +} > > + > > +/* > > + * Functions for the initialization > > + */ > > + > > +static int __init damon_dbgfs_init(void) > > +{ > > + int rc; > > + > > + dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL); > > No error checking? Will add in the next spin. > > > + dbgfs_ctxs[0] = dbgfs_new_ctx(); > > + if (!dbgfs_ctxs[0]) > > + return -ENOMEM; > > + dbgfs_nr_ctxs = 1; > > + > > + rc = __damon_dbgfs_init(); > > + if (rc) > > + pr_err("%s: dbgfs init failed\n", __func__); > > Shouldn't the error be printed out in the function that failed, not in > this one? I thought some other functions (in future) might want to use '__damon_dbgfs_init()' but siletnly handles it's failure. Therefore I made the function fails silently but returns the error code explicitly. Am I missing somthing? Thanks, SeongJae Park > > thanks, > > greg k-h
Re: [PATCH v26 07/13] mm/damon: Implement a debugfs-based user space interface
On Tue, Mar 30, 2021 at 09:05:31AM +, sj38.p...@gmail.com wrote: > +static int __init __damon_dbgfs_init(void) > +{ > + struct dentry *dbgfs_root; > + const char * const file_names[] = {"monitor_on"}; > + const struct file_operations *fops[] = {_on_fops}; > + int i; > + > + dbgfs_root = debugfs_create_dir("damon", NULL); > + > + for (i = 0; i < ARRAY_SIZE(file_names); i++) > + debugfs_create_file(file_names[i], 0600, dbgfs_root, NULL, > + fops[i]); > + dbgfs_fill_ctx_dir(dbgfs_root, dbgfs_ctxs[0]); > + > + dbgfs_dirs = kmalloc_array(1, sizeof(dbgfs_root), GFP_KERNEL); No error checking for memory allocation failures? > + dbgfs_dirs[0] = dbgfs_root; > + > + return 0; > +} > + > +/* > + * Functions for the initialization > + */ > + > +static int __init damon_dbgfs_init(void) > +{ > + int rc; > + > + dbgfs_ctxs = kmalloc(sizeof(*dbgfs_ctxs), GFP_KERNEL); No error checking? > + dbgfs_ctxs[0] = dbgfs_new_ctx(); > + if (!dbgfs_ctxs[0]) > + return -ENOMEM; > + dbgfs_nr_ctxs = 1; > + > + rc = __damon_dbgfs_init(); > + if (rc) > + pr_err("%s: dbgfs init failed\n", __func__); Shouldn't the error be printed out in the function that failed, not in this one? thanks, greg k-h