[PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[Nicholas Krause]

This makes the function alloc_mem_cgroup_per_zone_info have a
return type of bool now due to this particular function always
returning either one or zero as its return value.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 mm/memcontrol.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index acb93c5..35d86d2 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4425,7 +4425,7 @@ static struct cftype mem_cgroup_legacy_files[] = {
 	{ },	/* terminate */
 };
 
-static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
+static bool alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
 {
 	struct mem_cgroup_per_node *pn;
 	struct mem_cgroup_per_zone *mz;
@@ -4442,7 +4442,7 @@ static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
 		tmp = -1;
 	pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp);
 	if (!pn)
-		return 1;
+		return true;
 
 	for (zone = 0; zone < MAX_NR_ZONES; zone++) {
 		mz = &pn->zoneinfo[zone];
@@ -4452,7 +4452,7 @@ static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
 		mz->memcg = memcg;
 	}
 	memcg->nodeinfo[node] = pn;
-	return 0;
+	return false;
 }
 
 static void free_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[Michal Hocko]

On Mon 29-06-15 10:13:53, Nicholas Krause wrote:
[...]
> -static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
> +static bool alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>  {
>  	struct mem_cgroup_per_node *pn;
>  	struct mem_cgroup_per_zone *mz;
> @@ -4442,7 +4442,7 @@ static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>  		tmp = -1;
>  	pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp);
>  	if (!pn)
> -		return 1;
> +		return true;

Have you tried to think about the semantic of the function? The function
has returned 0 to signal the success which is pretty common. It could have
returned -ENOMEM for the allocation failure which would be much more
nicer than 1.

After your change we have bool semantic where the success is reported by
false while failure is true. Doest this make any sense to you? Because
it doesn't make to me and it only shows that this is a mechanical
conversion without deeper thinking about consequences.

Nacked-by: Michal Hocko <mhocko@suse.cz>

Btw. I can see your other patches which trying to do similar. I would
strongly discourage you from this path. Try to understand the code and
focus on changes which would actually make any improvements to the code
base. Doing stylist changes which do not help readability and neither
help compiler to generate a better code is simply waste of your and
reviewers time.

>  	for (zone = 0; zone < MAX_NR_ZONES; zone++) {
>  		mz = &pn->zoneinfo[zone];
> @@ -4452,7 +4452,7 @@ static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>  		mz->memcg = memcg;
>  	}
>  	memcg->nodeinfo[node] = pn;
> -	return 0;
> +	return false;
>  }
>  
>  static void free_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
> -- 
> 2.1.4
> 

-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[Nicholas Krause]

On June 29, 2015 11:03:11 AM EDT, Michal Hocko <mhocko@suse.cz> wrote:
>On Mon 29-06-15 10:13:53, Nicholas Krause wrote:
>[...]
>> -static int alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg,
>int node)
>> +static bool alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg,
>int node)
>>  {
>>  	struct mem_cgroup_per_node *pn;
>>  	struct mem_cgroup_per_zone *mz;
>> @@ -4442,7 +4442,7 @@ static int
>alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>>  		tmp = -1;
>>  	pn = kzalloc_node(sizeof(*pn), GFP_KERNEL, tmp);
>>  	if (!pn)
>> -		return 1;
>> +		return true;
>
>Have you tried to think about the semantic of the function? The
>function
>has returned 0 to signal the success which is pretty common. It could
>have
>returned -ENOMEM for the allocation failure which would be much more
>nicer than 1.
>
>After your change we have bool semantic where the success is reported
>by
>false while failure is true. Doest this make any sense to you? Because
>it doesn't make to me and it only shows that this is a mechanical
>conversion without deeper thinking about consequences.
>
>Nacked-by: Michal Hocko <mhocko@suse.cz>
>
>Btw. I can see your other patches which trying to do similar. I would
>strongly discourage you from this path. Try to understand the code and
>focus on changes which would actually make any improvements to the code
>base. Doing stylist changes which do not help readability and neither
>help compiler to generate a better code is simply waste of your and
>reviewers time.
>
>>  	for (zone = 0; zone < MAX_NR_ZONES; zone++) {
>>  		mz = &pn->zoneinfo[zone];
>> @@ -4452,7 +4452,7 @@ static int
>alloc_mem_cgroup_per_zone_info(struct mem_cgroup *memcg, int node)
>>  		mz->memcg = memcg;
>>  	}
>>  	memcg->nodeinfo[node] = pn;
>> -	return 0;
>> +	return false;
>>  }
>>  
>>  static void free_mem_cgroup_per_zone_info(struct mem_cgroup *memcg,
>int node)
>> -- 
>> 2.1.4
>> 
I agree with and looked into the callers about this wasn't sure if you you wanted me to return - ENOMEM.  I will rewrite this patch the other way.  Furthermore I apologize about this and do have actual useful patches but will my rep it's hard to get replies from maintainers. If you would like to take a look at them please let know. 
Nick 
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[Michal Hocko]

On Mon 29-06-15 11:23:08, Nicholas Krause wrote:
[...]
> I agree with and looked into the callers about this wasn't sure if you
> you wanted me to return - ENOMEM.  I will rewrite this patch the other
> way. 

I am not sure this path really needs a cleanup.

> Furthermore I apologize about this and do have actual useful
> patches but will my rep it's hard to get replies from maintainers.

You can hardly expect somebody will be thrilled about your patches when
their fault rate is close to 100%. Reviewing each patch takes time and
that is a scarce resource. If you want people to follow your patches
make sure you are offering something that might be interesting or
useful. Cleanups like these usually are not interesting without
either building something bigger on top of them or when they improve
readability considerably.

[...]
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[nick]

On 2015-06-29 11:36 AM, Michal Hocko wrote:
> On Mon 29-06-15 11:23:08, Nicholas Krause wrote:
> [...]
>> I agree with and looked into the callers about this wasn't sure if you
>> you wanted me to return - ENOMEM.  I will rewrite this patch the other
>> way. 
> 
> I am not sure this path really needs a cleanup.
> 
>> Furthermore I apologize about this and do have actual useful
>> patches but will my rep it's hard to get replies from maintainers.
> 
> You can hardly expect somebody will be thrilled about your patches when
> their fault rate is close to 100%. Reviewing each patch takes time and
> that is a scarce resource. If you want people to follow your patches
> make sure you are offering something that might be interesting or
> useful. Cleanups like these usually are not interesting without
> either building something bigger on top of them or when they improve
> readability considerably.
> 
> [...]
> 
Actually my patch record is much better now it's at the worst case 60% are correct and 40 % are not
and this based on the few that have been merged. Here is a patch series I have been trying to merge
for a bug in the gma500 other the last few patches. There are other patches I have like this lying
around.
Nick 

From 2d2ddb5d9a2c4fcbae45339d4f775fcde49f36e1 Mon Sep 17 00:00:00 2001
From: Nicholas Krause <xerofoify@gmail.com>
Date: Wed, 13 May 2015 21:36:44 -0400
Subject: [PATCH 1/2] gma500:Add proper use of the variable ret for the
 function, psb_mmu_inset_pfn_sequence

This adds proper use of the variable ret by returning it
at the end of the function, psb_mmu_inset_pfn_sequence for
indicating to callers when an error has occurred. Further
more remove the unneeded double setting of ret to the error
code, -ENOMEM after checking if a call to the function,
psb_mmu_pt_alloc_map_lock is successful.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/gpu/drm/gma500/mmu.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
index 0eaf11c..d2c4bac 100644
--- a/drivers/gpu/drm/gma500/mmu.c
+++ b/drivers/gpu/drm/gma500/mmu.c
@@ -677,10 +677,9 @@ int psb_mmu_insert_pfn_sequence(struct psb_mmu_pd *pd, uint32_t start_pfn,
 	do {
 		next = psb_pd_addr_end(addr, end);
 		pt = psb_mmu_pt_alloc_map_lock(pd, addr);
-		if (!pt) {
-			ret = -ENOMEM;
+		if (!pt)
 			goto out;
-		}
+
 		do {
 			pte = psb_mmu_mask_pte(start_pfn++, type);
 			psb_mmu_set_pte(pt, addr, pte);
@@ -700,7 +699,7 @@ out:
 	if (pd->hw_context != -1)
 		psb_mmu_flush(pd->driver);
 
-	return 0;
+	return ret;
 }
 
 int psb_mmu_insert_pages(struct psb_mmu_pd *pd, struct page **pages,
-- 
2.1.4From e0bb93a1752af6092dc3bad647aca8730cb7817d Mon Sep 17 00:00:00 2001
From: Nicholas Krause <xerofoify@gmail.com>
Date: Wed, 13 May 2015 20:52:21 -0400
Subject: [PATCH RESEND] drm:Make the function, psb_mmu_pt_alloc_map_lock static

This makes the function psb_mmu_pt_alloc_map_lock static
now due to having no callers outside of its declaration
in the file, mmu.c.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/gpu/drm/gma500/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
index 0eaf11c..3ccb09a 100644
--- a/drivers/gpu/drm/gma500/mmu.c
+++ b/drivers/gpu/drm/gma500/mmu.c
@@ -323,7 +323,7 @@ static struct psb_mmu_pt *psb_mmu_alloc_pt(struct psb_mmu_pd *pd)
 	return pt;
 }
 
-struct psb_mmu_pt *psb_mmu_pt_alloc_map_lock(struct psb_mmu_pd *pd,
+static struct psb_mmu_pt *psb_mmu_pt_alloc_map_lock(struct psb_mmu_pd *pd,
 					     unsigned long addr)
 {
 	uint32_t index = psb_mmu_pd_index(addr);
-- 
2.1.4

This another patch for removing a 100 line unused function from  the wireless core.

From e0bb93a1752af6092dc3bad647aca8730cb7817d Mon Sep 17 00:00:00 2001
From: Nicholas Krause <xerofoify@gmail.com>
Date: Wed, 13 May 2015 20:52:21 -0400
Subject: [PATCH RESEND] drm:Make the function, psb_mmu_pt_alloc_map_lock static

This makes the function psb_mmu_pt_alloc_map_lock static
now due to having no callers outside of its declaration
in the file, mmu.c.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/gpu/drm/gma500/mmu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
index 0eaf11c..3ccb09a 100644
--- a/drivers/gpu/drm/gma500/mmu.c
+++ b/drivers/gpu/drm/gma500/mmu.c
@@ -323,7 +323,7 @@ static struct psb_mmu_pt *psb_mmu_alloc_pt(struct psb_mmu_pd *pd)
 	return pt;
 }
 
-struct psb_mmu_pt *psb_mmu_pt_alloc_map_lock(struct psb_mmu_pd *pd,
+static struct psb_mmu_pt *psb_mmu_pt_alloc_map_lock(struct psb_mmu_pd *pd,
 					     unsigned long addr)
 {
 	uint32_t index = psb_mmu_pd_index(addr);
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[nick]

On 2015-06-29 11:44 AM, nick wrote:
> 
> 
> On 2015-06-29 11:36 AM, Michal Hocko wrote:
>> On Mon 29-06-15 11:23:08, Nicholas Krause wrote:
>> [...]
>>> I agree with and looked into the callers about this wasn't sure if you
>>> you wanted me to return - ENOMEM.  I will rewrite this patch the other
>>> way. 
>>
>> I am not sure this path really needs a cleanup.
>>
>>> Furthermore I apologize about this and do have actual useful
>>> patches but will my rep it's hard to get replies from maintainers.
>>
>> You can hardly expect somebody will be thrilled about your patches when
>> their fault rate is close to 100%. Reviewing each patch takes time and
>> that is a scarce resource. If you want people to follow your patches
>> make sure you are offering something that might be interesting or
>> useful. Cleanups like these usually are not interesting without
>> either building something bigger on top of them or when they improve
>> readability considerably.
>>
>> [...]
>>
> Actually my patch record is much better now it's at the worst case 60% are correct and 40 % are not
> and this based on the few that have been merged. Here is a patch series I have been trying to merge
> for a bug in the gma500 other the last few patches. There are other patches I have like this lying
> around.
> Nick 
> 
> From 2d2ddb5d9a2c4fcbae45339d4f775fcde49f36e1 Mon Sep 17 00:00:00 2001
> From: Nicholas Krause <xerofoify@gmail.com>
> Date: Wed, 13 May 2015 21:36:44 -0400
> Subject: [PATCH 1/2] gma500:Add proper use of the variable ret for the
>  function, psb_mmu_inset_pfn_sequence
> 
> This adds proper use of the variable ret by returning it
> at the end of the function, psb_mmu_inset_pfn_sequence for
> indicating to callers when an error has occurred. Further
> more remove the unneeded double setting of ret to the error
> code, -ENOMEM after checking if a call to the function,
> psb_mmu_pt_alloc_map_lock is successful.
> 
> Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
> ---
>  drivers/gpu/drm/gma500/mmu.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gma500/mmu.c b/drivers/gpu/drm/gma500/mmu.c
> index 0eaf11c..d2c4bac 100644
> --- a/drivers/gpu/drm/gma500/mmu.c
> +++ b/drivers/gpu/drm/gma500/mmu.c
> @@ -677,10 +677,9 @@ int psb_mmu_insert_pfn_sequence(struct psb_mmu_pd *pd, uint32_t start_pfn,
>  	do {
>  		next = psb_pd_addr_end(addr, end);
>  		pt = psb_mmu_pt_alloc_map_lock(pd, addr);
> -		if (!pt) {
> -			ret = -ENOMEM;
> +		if (!pt)
>  			goto out;
> -		}
> +
>  		do {
>  			pte = psb_mmu_mask_pte(start_pfn++, type);
>  			psb_mmu_set_pte(pt, addr, pte);
> @@ -700,7 +699,7 @@ out:
>  	if (pd->hw_context != -1)
>  		psb_mmu_flush(pd->driver);
>  
> -	return 0;
> +	return ret;
>  }
>  
>  int psb_mmu_insert_pages(struct psb_mmu_pd *pd, struct page **pages,
> 
Sorry the second patch in the drm was the wrong one this was another patch I am lying around.
Below is the actual second patch for the bug fix.
Nick
From f1802ff61ef69ff9ecaaeadb941d4ed725a0255a Mon Sep 17 00:00:00 2001
From: Nicholas Krause <xerofoify@gmail.com>
Date: Fri, 22 May 2015 23:10:49 -0400
Subject: [PATCH 2/2] gma500: Add new error checking for the function, psb_driver_load

This adds new error checking to the function, psb_driver_load
for when the function, psb_mmu_inset_pfn_sequence fails to grab
memory for the internal function call to psb_pd_addr_end. In
addition we now implement this by checking for a error code
return  value from the internal call to the function,
psb_mmu_inset_pfn_sequence for the function, psb_driver_load.
If a error code is returned we must jump to a new label,
unlock_err in order to deallocate our usage count by
one for the usage of the semaphore, sem before unloading
the drm_device structure and returning a error code.

Signed-off-by: Nicholas Krause <xerofoify@gmail.com>
---
 drivers/gpu/drm/gma500/psb_drv.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/gma500/psb_drv.c b/drivers/gpu/drm/gma500/psb_drv.c
index 92e7e57..4cef2c9 100644
--- a/drivers/gpu/drm/gma500/psb_drv.c
+++ b/drivers/gpu/drm/gma500/psb_drv.c
@@ -343,6 +343,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 					  dev_priv->stolen_base >> PAGE_SHIFT,
 					  pg->gatt_start,
 					  pg->stolen_size >> PAGE_SHIFT, 0);
+	if (ret)
+		goto unlock_err;
 	up_read(&pg->sem);
 
 	psb_mmu_set_pd_context(psb_mmu_get_default_pd(dev_priv->mmu), 0);
@@ -405,6 +407,8 @@ static int psb_driver_load(struct drm_device *dev, unsigned long flags)
 #endif
 	/* Intel drm driver load is done, continue doing pvr load */
 	return 0;
+unlock_err:
+	up_read(&pg->sem);
 out_err:
 	psb_driver_unload(dev);
 	return ret;
-- 
2.1.4

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] mm:Make the function alloc_mem_cgroup_per_zone_info bool

[Michal Hocko]

On Mon 29-06-15 11:44:24, nick wrote:
[...]
> Here is a patch series I have been trying to merge for a bug in the
> gma500 other the last few patches. There are other patches I have like
> this lying around.

I am not interested in looking at random and unrelated patches. I am
also not thrilled about random cleanups which do not have an additional
value. I will not repeat that again and will start ignoring patches like
that. I have tried to help you but this seems pointless investment of my
time.
-- 
Michal Hocko
SUSE Labs

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>