| Message ID | 20220919151201.66696-2-xhao@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v1,1/2] mm/damon/sysfs: remove unnecessary variables | expand |
On Mon, 19 Sep 2022 23:12:01 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > In damon_sysfs_access_pattern_alloc() adn damon_sysfs_attrs_alloc(), > we can use kzmalloc to alloc instance of the related structs, This makes > the function code much cleaner. This definitely makes the code cleaner, thank you. But, the initial intent of the code is to initialize the fiedls that really need to be initialized at the point, for the efficiency and also for making it clear which field is needed to be initialized to what value here. It's also intended to make readers wonder about where and how the remaining fields are initialized. So, to my humble eyes, this change looks like making the code a little bit inefficient and unclear, sorry. Thanks, SJ > > Signed-off-by: Xin Hao <xhao@linux.alibaba.com> > --- > mm/damon/sysfs.c | 15 ++------------- > 1 file changed, 2 insertions(+), 13 deletions(-) > > diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c > index b852a75b9f39..06154ece7960 100644 > --- a/mm/damon/sysfs.c > +++ b/mm/damon/sysfs.c > @@ -657,13 +657,7 @@ struct damon_sysfs_access_pattern { > static > struct damon_sysfs_access_pattern *damon_sysfs_access_pattern_alloc(void) > { > - struct damon_sysfs_access_pattern *access_pattern = > - kmalloc(sizeof(*access_pattern), GFP_KERNEL); > - > - if (!access_pattern) > - return NULL; > - access_pattern->kobj = (struct kobject){}; > - return access_pattern; > + return kzalloc(sizeof(struct damon_sysfs_access_pattern), GFP_KERNEL); > } > > static int damon_sysfs_access_pattern_add_range_dir( > @@ -1620,12 +1614,7 @@ struct damon_sysfs_attrs { > > static struct damon_sysfs_attrs *damon_sysfs_attrs_alloc(void) > { > - struct damon_sysfs_attrs *attrs = kmalloc(sizeof(*attrs), GFP_KERNEL); > - > - if (!attrs) > - return NULL; > - attrs->kobj = (struct kobject){}; > - return attrs; > + return kzalloc(sizeof(struct damon_sysfs_attrs), GFP_KERNEL); > } > > static int damon_sysfs_attrs_add_dirs(struct damon_sysfs_attrs *attrs) > -- > 2.31.0
在 2022/9/20 上午1:22, SeongJae Park 写道: > On Mon, 19 Sep 2022 23:12:01 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > >> In damon_sysfs_access_pattern_alloc() adn damon_sysfs_attrs_alloc(), >> we can use kzmalloc to alloc instance of the related structs, This makes >> the function code much cleaner. > This definitely makes the code cleaner, thank you. But, the initial intent of > the code is to initialize the fiedls that really need to be initialized at the > point, for the efficiency and also for making it clear which field is needed to > be initialized to what value here. It's also intended to make readers wonder > about where and how the remaining fields are initialized. Maybe the other func like damon_sysfs_kdamonds_alloc() also need to do like this, you can see it return directly by kzalloc. static struct damon_sysfs_kdamonds *damon_sysfs_kdamonds_alloc(void) { return kzalloc(sizeof(struct damon_sysfs_kdamonds), GFP_KERNEL); } > So, to my humble eyes, this change looks like making the code a little bit > inefficient and unclear, sorry. > > > Thanks, > SJ > >> Signed-off-by: Xin Hao <xhao@linux.alibaba.com> >> --- >> mm/damon/sysfs.c | 15 ++------------- >> 1 file changed, 2 insertions(+), 13 deletions(-) >> >> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c >> index b852a75b9f39..06154ece7960 100644 >> --- a/mm/damon/sysfs.c >> +++ b/mm/damon/sysfs.c >> @@ -657,13 +657,7 @@ struct damon_sysfs_access_pattern { >> static >> struct damon_sysfs_access_pattern *damon_sysfs_access_pattern_alloc(void) >> { >> - struct damon_sysfs_access_pattern *access_pattern = >> - kmalloc(sizeof(*access_pattern), GFP_KERNEL); >> - >> - if (!access_pattern) >> - return NULL; >> - access_pattern->kobj = (struct kobject){}; >> - return access_pattern; >> + return kzalloc(sizeof(struct damon_sysfs_access_pattern), GFP_KERNEL); >> } >> >> static int damon_sysfs_access_pattern_add_range_dir( >> @@ -1620,12 +1614,7 @@ struct damon_sysfs_attrs { >> >> static struct damon_sysfs_attrs *damon_sysfs_attrs_alloc(void) >> { >> - struct damon_sysfs_attrs *attrs = kmalloc(sizeof(*attrs), GFP_KERNEL); >> - >> - if (!attrs) >> - return NULL; >> - attrs->kobj = (struct kobject){}; >> - return attrs; >> + return kzalloc(sizeof(struct damon_sysfs_attrs), GFP_KERNEL); >> } >> >> static int damon_sysfs_attrs_add_dirs(struct damon_sysfs_attrs *attrs) >> -- >> 2.31.0
Hi Xin, On Tue, 20 Sep 2022 09:58:41 +0800 haoxin <xhao@linux.alibaba.com> wrote: > > 在 2022/9/20 上午1:22, SeongJae Park 写道: > > On Mon, 19 Sep 2022 23:12:01 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: > > > >> In damon_sysfs_access_pattern_alloc() adn damon_sysfs_attrs_alloc(), > >> we can use kzmalloc to alloc instance of the related structs, This makes > >> the function code much cleaner. > > This definitely makes the code cleaner, thank you. But, the initial intent of > > the code is to initialize the fiedls that really need to be initialized at the > > point, for the efficiency and also for making it clear which field is needed to > > be initialized to what value here. It's also intended to make readers wonder > > about where and how the remaining fields are initialized. > > Maybe the other func like damon_sysfs_kdamonds_alloc() also need to do > like this, you can see it return directly by > > kzalloc. > > static struct damon_sysfs_kdamonds *damon_sysfs_kdamonds_alloc(void) > { > return kzalloc(sizeof(struct damon_sysfs_kdamonds), > GFP_KERNEL); > } In this case, all the fields of the struct need to be initialized as zero. That's why we use kzalloc() there. Of course, my opinion is not a static and concrete rule, but changing mind. And obviously everyone makes many mistakes and DAMON code has many rooms for improvement. I really appreciate for your greatful efforts for that. But, at least in this case, I think it doesn't really need the change at the moment, IMHO. Thanks, SJ [...]
在 2022/9/21 上午12:25, SeongJae Park 写道: > Hi Xin, > > On Tue, 20 Sep 2022 09:58:41 +0800 haoxin <xhao@linux.alibaba.com> wrote: > >> 在 2022/9/20 上午1:22, SeongJae Park 写道: >>> On Mon, 19 Sep 2022 23:12:01 +0800 Xin Hao <xhao@linux.alibaba.com> wrote: >>> >>>> In damon_sysfs_access_pattern_alloc() adn damon_sysfs_attrs_alloc(), >>>> we can use kzmalloc to alloc instance of the related structs, This makes >>>> the function code much cleaner. >>> This definitely makes the code cleaner, thank you. But, the initial intent of >>> the code is to initialize the fiedls that really need to be initialized at the >>> point, for the efficiency and also for making it clear which field is needed to >>> be initialized to what value here. It's also intended to make readers wonder >>> about where and how the remaining fields are initialized. >> Maybe the other func like damon_sysfs_kdamonds_alloc() also need to do >> like this, you can see it return directly by >> >> kzalloc. >> >> static struct damon_sysfs_kdamonds *damon_sysfs_kdamonds_alloc(void) >> { >> return kzalloc(sizeof(struct damon_sysfs_kdamonds), >> GFP_KERNEL); >> } > In this case, all the fields of the struct need to be initialized as zero. > That's why we use kzalloc() there. > > Of course, my opinion is not a static and concrete rule, but changing mind. > And obviously everyone makes many mistakes and DAMON code has many rooms for > improvement. Understand your concern, at least, there is no point in simplifying the code too mush if we are ignoring its readability. > I really appreciate for your greatful efforts for that. But, at > least in this case, I think it doesn't really need the change at the moment, > IMHO. > > > Thanks, > SJ > > [...]
diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c index b852a75b9f39..06154ece7960 100644 --- a/mm/damon/sysfs.c +++ b/mm/damon/sysfs.c @@ -657,13 +657,7 @@ struct damon_sysfs_access_pattern { static struct damon_sysfs_access_pattern *damon_sysfs_access_pattern_alloc(void) { - struct damon_sysfs_access_pattern *access_pattern = - kmalloc(sizeof(*access_pattern), GFP_KERNEL); - - if (!access_pattern) - return NULL; - access_pattern->kobj = (struct kobject){}; - return access_pattern; + return kzalloc(sizeof(struct damon_sysfs_access_pattern), GFP_KERNEL); } static int damon_sysfs_access_pattern_add_range_dir( @@ -1620,12 +1614,7 @@ struct damon_sysfs_attrs { static struct damon_sysfs_attrs *damon_sysfs_attrs_alloc(void) { - struct damon_sysfs_attrs *attrs = kmalloc(sizeof(*attrs), GFP_KERNEL); - - if (!attrs) - return NULL; - attrs->kobj = (struct kobject){}; - return attrs; + return kzalloc(sizeof(struct damon_sysfs_attrs), GFP_KERNEL); } static int damon_sysfs_attrs_add_dirs(struct damon_sysfs_attrs *attrs)
In damon_sysfs_access_pattern_alloc() adn damon_sysfs_attrs_alloc(), we can use kzmalloc to alloc instance of the related structs, This makes the function code much cleaner. Signed-off-by: Xin Hao <xhao@linux.alibaba.com> --- mm/damon/sysfs.c | 15 ++------------- 1 file changed, 2 insertions(+), 13 deletions(-)