[PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables

[PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables

[Xin Hao]

Just do a little change here, the 'err' variable really no need to stay
here.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 mm/damon/sysfs.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
index 0cca1909bf67..b852a75b9f39 100644
--- a/mm/damon/sysfs.c
+++ b/mm/damon/sysfs.c
@@ -1109,9 +1109,8 @@ static ssize_t start_store(struct kobject *kobj, struct kobj_attribute *attr,
 {
 	struct damon_sysfs_region *region = container_of(kobj,
 			struct damon_sysfs_region, kobj);
-	int err = kstrtoul(buf, 0, &region->start);

-	if (err)
+	if (kstrtoul(buf, 0, &region->start))
 		return -EINVAL;
 	return count;
 }
@@ -1130,9 +1129,8 @@ static ssize_t end_store(struct kobject *kobj, struct kobj_attribute *attr,
 {
 	struct damon_sysfs_region *region = container_of(kobj,
 			struct damon_sysfs_region, kobj);
-	int err = kstrtoul(buf, 0, &region->end);

-	if (err)
+	if (kstrtoul(buf, 0, &region->end))
 		return -EINVAL;
 	return count;
 }
--
2.31.0

[PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes

[Xin Hao]

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(-)

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

Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes

[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.

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

Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes

[haoxin]

在 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

Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes

[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.  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

[...]

Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes

[haoxin]

在 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
>
> [...]

Re: [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables

[SeongJae Park]

On Mon, 19 Sep 2022 23:12:00 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:

> Just do a little change here, the 'err' variable really no need to stay
> here.
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  mm/damon/sysfs.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
> index 0cca1909bf67..b852a75b9f39 100644
> --- a/mm/damon/sysfs.c
> +++ b/mm/damon/sysfs.c
> @@ -1109,9 +1109,8 @@ static ssize_t start_store(struct kobject *kobj, struct kobj_attribute *attr,
>  {
>  	struct damon_sysfs_region *region = container_of(kobj,
>  			struct damon_sysfs_region, kobj);
> -	int err = kstrtoul(buf, 0, &region->start);
> 
> -	if (err)
> +	if (kstrtoul(buf, 0, &region->start))
>  		return -EINVAL;

Good finding.  But, I'd like to let the user know why it really fails by giving
them the error code that returned by 'kstrtoul()' here.  Let's keep the 'err'
but return 'err' here.

>  	return count;
>  }
> @@ -1130,9 +1129,8 @@ static ssize_t end_store(struct kobject *kobj, struct kobj_attribute *attr,
>  {
>  	struct damon_sysfs_region *region = container_of(kobj,
>  			struct damon_sysfs_region, kobj);
> -	int err = kstrtoul(buf, 0, &region->end);
> 
> -	if (err)
> +	if (kstrtoul(buf, 0, &region->end))
>  		return -EINVAL;

ditto.

>  	return count;
>  }
> --
> 2.31.0


Thanks,
SJ

Re: [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables

[haoxin]

在 2022/9/20 上午1:03, SeongJae Park 写道:
> On Mon, 19 Sep 2022 23:12:00 +0800 Xin Hao <xhao@linux.alibaba.com> wrote:
>
>> Just do a little change here, the 'err' variable really no need to stay
>> here.
>>
>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>> ---
>>   mm/damon/sysfs.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/mm/damon/sysfs.c b/mm/damon/sysfs.c
>> index 0cca1909bf67..b852a75b9f39 100644
>> --- a/mm/damon/sysfs.c
>> +++ b/mm/damon/sysfs.c
>> @@ -1109,9 +1109,8 @@ static ssize_t start_store(struct kobject *kobj, struct kobj_attribute *attr,
>>   {
>>   	struct damon_sysfs_region *region = container_of(kobj,
>>   			struct damon_sysfs_region, kobj);
>> -	int err = kstrtoul(buf, 0, &region->start);
>>
>> -	if (err)
>> +	if (kstrtoul(buf, 0, &region->start))
>>   		return -EINVAL;
> Good finding.  But, I'd like to let the user know why it really fails by giving
> them the error code that returned by 'kstrtoul()' here.  Let's keep the 'err'
> but return 'err' here.
Ok,  it make sense.
>
>>   	return count;
>>   }
>> @@ -1130,9 +1129,8 @@ static ssize_t end_store(struct kobject *kobj, struct kobj_attribute *attr,
>>   {
>>   	struct damon_sysfs_region *region = container_of(kobj,
>>   			struct damon_sysfs_region, kobj);
>> -	int err = kstrtoul(buf, 0, &region->end);
>>
>> -	if (err)
>> +	if (kstrtoul(buf, 0, &region->end))
>>   		return -EINVAL;
> ditto.
>
>>   	return count;
>>   }
>> --
>> 2.31.0
>
> Thanks,
> SJ