All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables
@ 2022-09-19 15:12 Xin Hao
  2022-09-19 15:12 ` [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes Xin Hao
  2022-09-19 17:03 ` [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables SeongJae Park
  0 siblings, 2 replies; 8+ messages in thread
From: Xin Hao @ 2022-09-19 15:12 UTC (permalink / raw)
  To: sj; +Cc: akpm, damon, linux-mm, linux-kernel, xhao

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

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes
  2022-09-19 15:12 [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables Xin Hao
@ 2022-09-19 15:12 ` Xin Hao
  2022-09-19 17:22   ` SeongJae Park
  2022-09-19 17:03 ` [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables SeongJae Park
  1 sibling, 1 reply; 8+ messages in thread
From: Xin Hao @ 2022-09-19 15:12 UTC (permalink / raw)
  To: sj; +Cc: akpm, damon, linux-mm, linux-kernel, xhao

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


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables
  2022-09-19 15:12 [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables Xin Hao
  2022-09-19 15:12 ` [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes Xin Hao
@ 2022-09-19 17:03 ` SeongJae Park
  2022-09-20  1:54   ` haoxin
  1 sibling, 1 reply; 8+ messages in thread
From: SeongJae Park @ 2022-09-19 17:03 UTC (permalink / raw)
  To: Xin Hao; +Cc: sj, akpm, damon, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes
  2022-09-19 15:12 ` [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes Xin Hao
@ 2022-09-19 17:22   ` SeongJae Park
  2022-09-20  1:58     ` haoxin
  0 siblings, 1 reply; 8+ messages in thread
From: SeongJae Park @ 2022-09-19 17:22 UTC (permalink / raw)
  To: Xin Hao; +Cc: sj, akpm, damon, linux-mm, linux-kernel

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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables
  2022-09-19 17:03 ` [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables SeongJae Park
@ 2022-09-20  1:54   ` haoxin
  0 siblings, 0 replies; 8+ messages in thread
From: haoxin @ 2022-09-20  1:54 UTC (permalink / raw)
  To: SeongJae Park; +Cc: akpm, damon, linux-mm, linux-kernel


在 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes
  2022-09-19 17:22   ` SeongJae Park
@ 2022-09-20  1:58     ` haoxin
  2022-09-20 16:25       ` SeongJae Park
  0 siblings, 1 reply; 8+ messages in thread
From: haoxin @ 2022-09-20  1:58 UTC (permalink / raw)
  To: SeongJae Park; +Cc: akpm, damon, linux-mm, linux-kernel


在 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

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes
  2022-09-20  1:58     ` haoxin
@ 2022-09-20 16:25       ` SeongJae Park
  2022-09-21  5:20         ` haoxin
  0 siblings, 1 reply; 8+ messages in thread
From: SeongJae Park @ 2022-09-20 16:25 UTC (permalink / raw)
  To: haoxin; +Cc: SeongJae Park, akpm, damon, linux-mm, linux-kernel

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

[...]

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes
  2022-09-20 16:25       ` SeongJae Park
@ 2022-09-21  5:20         ` haoxin
  0 siblings, 0 replies; 8+ messages in thread
From: haoxin @ 2022-09-21  5:20 UTC (permalink / raw)
  To: SeongJae Park; +Cc: akpm, damon, linux-mm, linux-kernel


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

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2022-09-21  5:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-19 15:12 [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables Xin Hao
2022-09-19 15:12 ` [PATCH v1 2/2] mm/damon/sysfs: use kzmalloc instead kmalloc to simplify codes Xin Hao
2022-09-19 17:22   ` SeongJae Park
2022-09-20  1:58     ` haoxin
2022-09-20 16:25       ` SeongJae Park
2022-09-21  5:20         ` haoxin
2022-09-19 17:03 ` [PATCH v1 1/2] mm/damon/sysfs: remove unnecessary variables SeongJae Park
2022-09-20  1:54   ` haoxin

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.