cgroup: __cpuset_node_allowed return bool

PATCH In review [email protected] 2 participants 8 messages Mar 24, 2018 – Mar 27, 2018

Subsystems (2)

cgroup Control group - cpuset

lore

Share & tools

lore

[email protected]

author

2018-03-24 04:56 UTC #0 lore raw

PATCH 1/1 cgroup: __cpuset_node_allowed return bool

as a bool, __cpuset_node_allowed(...) return should be bool.

Signed-off-by John Zhao <[email protected]>

cgroup/cpuset.c renamed from cgroup/cpuset.c.orig +1 -1

@@ -2552,7 +2552,7 @@ static struct cpuset *nearest_hardwall_a
 bool __cpuset_node_allowed(int node, gfp_t gfp_mask)
 {
 	struct cpuset *cs;		/* current cpuset ancestors */
-	int allowed;			/* is allocation in zone z allowed? */
+	bool allowed;			/* is allocation in zone z allowed? */
 	unsigned long flags;
 
 	if (in_interrupt())

From 304cec1cc42255fbd9e231a810f4eea20ab74b90 Mon Sep 17 00:00:00 2001
From: John Zhao <[email protected]>
Date: Sat, 24 Mar 2018 13:01:32 +0800
Subject: [PATCH] cgroup: __cpuset_node_allowed return bool

as a bool, __cpuset_node_allowed(...) return should be bool.

Signed-off-by John Zhao <[email protected]>

---
 kernel/cgroup/cpuset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/cgroup/cpuset.c +1 -1

index b42037e..42338b7 100644
@@ -2552,7 +2552,7 @@ static struct cpuset *nearest_hardwall_ancestor(struct cpuset *cs)
 bool __cpuset_node_allowed(int node, gfp_t gfp_mask)
 {
 	struct cpuset *cs;		/* current cpuset ancestors */
-	int allowed;			/* is allocation in zone z allowed? */
+	bool allowed;			/* is allocation in zone z allowed? */
 	unsigned long flags;
 
 	if (in_interrupt())

Signature · 1 line

2.7.4

Tejun Heo

maintainer

2018-03-26 14:12 UTC #2 lore raw

Hello, John.

Quoted 6 lines · On Sat, Mar 24, 2018 at 01:05:50PM +0800, [email protected] wrote:

From 304cec1cc42255fbd9e231a810f4eea20ab74b90 Mon Sep 17 00:00:00 2001
From: John Zhao <[email protected]>
Date: Sat, 24 Mar 2018 13:01:32 +0800
Subject: [PATCH] cgroup: __cpuset_node_allowed return bool
as a bool, __cpuset_node_allowed(...) return should be bool.

So, as a minor cleanup patch, this is fine but can you please soften the commit title / description a bit? It doesn't have to be bool. int is fine. bool may be marginally more readable but that's about it, so let's please make the commit match that.

Thanks.

Signature · 1 line

tejun

[email protected]

author

2018-03-26 14:20 UTC #3 lore raw

Hi Tejun,

inline.

Quoted 8 lines · Tejun Heo

Hello, John.
Quoted 5 lines · On Sat, Mar 24, 2018 at 01:05:50PM +0800, [email protected] wrote:
From 304cec1cc42255fbd9e231a810f4eea20ab74b90 Mon Sep 17 00:00:00 2001
From: John Zhao <[email protected]>
Date: Sat, 24 Mar 2018 13:01:32 +0800
Subject: [PATCH] cgroup: __cpuset_node_allowed return bool

Quoted 6 lines

Quoted 1 line
as a bool, __cpuset_node_allowed(...) return should be bool.
So, as a minor cleanup patch, this is fine but can you please soften the commit title / description a bit? It doesn't have to be bool. int is fine. bool may be marginally more readable but that's about it, so let's please make the commit match that.

[ZJ] In detail, Considering the conversion after it could be compiled
into asm such as: // cross compile it was done by "arm-linux-androideabi-gcc" on ubuntu
1) return int type variable in bool function:
bool enabled()
{
int ret = 1;
return ret;
}

/**
* ... ...

  *     mov     r3, #1
  *     str     r3, [fp, #-8]
  *     ldr     r3, [fp, #-8]
  *     cmp     r3, #0
  *     movne   r3, #1
  *     moveq   r3, #0
  *     uxtb    r3, r3

* ... ...
*/

2)
bool enabled()
{
bool ret = 1;
return ret;
}

/**
* ... ...

  *     mov     r3, #1
  *     strb    r3, [fp, #-5]
  *     ldrb    r3, [fp, #-5]   @ zero_extendqisi2

* ... ...
*/

so the #1) style function can generate significant instructions than
the #2).
While, this is happened only when "-On" is not used with *-gcc
together. Though, it is oftern there, it is best to provide this with decoupling of which option is used for optimization.

Situation is only nice to have this change as test_bit() is interpreted
in difference way in differece arch, which is "inline int" actually in arm-arch. Which makes the situation is not the like like the general case but needs to be checked and continued in the general include section.

So mark this change as nice to have to keep the thing as simple as
possible as this is what can be found under /linux/kernel related to this point.

Quoted 1 line

Thanks.

Thanks,
BR//Zhao

Tejun Heo

maintainer

2018-03-26 14:25 UTC #4 lore raw

Quoted 6 lines · On Mon, Mar 26, 2018 at 10:20:43PM +0800, [email protected] wrote:

1) return int type variable in bool function:
bool enabled()
{
(quoted diff) +0 -0
 	int ret = 1;
 	return ret;
 }

...

Quoted 6 lines

2)
bool enabled()
{
(quoted diff) +0 -0
 	bool ret = 1;
 	return ret;
 }

...

Quoted 2 lines

so the #1) style function can generate significant instructions
than the #2).

That is a problem for the compiler, not the code.

Quoted 3 lines

While, this is happened only when "-On" is not used with *-gcc
together. Though, it is oftern there, it is best to provide this with decoupling of which option is used for optimization.

We don't want to dictate minute coding styles to avoid things which are trivially optimized by compilers.

Thanks.

Signature · 1 line

tejun

[email protected]

author

2018-03-26 14:37 UTC #5 lore raw

Hi Tejun,

inline.

Quoted 18 lines · Tejun Heo

Quoted 6 lines · On Mon, Mar 26, 2018 at 10:20:43PM +0800, [email protected] wrote:
1) return int type variable in bool function:
bool enabled()
{
(quoted diff) +0 -0
 	int ret = 1;
 	return ret;
 }
...
Quoted 6 lines
2)
bool enabled()
{
(quoted diff) +0 -0
 	bool ret = 1;
 	return ret;
 }
...
Quoted 2 lines
so the #1) style function can generate significant instructions
than the #2).

Quoted 8 lines

That is a problem for the compiler, not the code.
Quoted 3 lines
While, this is happened only when "-On" is not used with *-gcc
together. Though, it is oftern there, it is best to provide this with decoupling of which option is used for optimization.
We don't want to dictate minute coding styles to avoid things which are trivially optimized by compilers.

[ZJ] Optimized by compiler is observed only. Such as it is not so big difference in x86-arch.

Quoted 2 lines

Thanks.

[email protected]

author

2018-03-26 14:41 UTC #6 lore raw

Hi Tejun,

Additionally,

Quoted 24 lines · On 2018-03-26 10:37 PM, [email protected] wrote:

Hi Tejun,
inline.
Quoted 19 lines · Tejun Heo
Quoted 6 lines · On Mon, Mar 26, 2018 at 10:20:43PM +0800, [email protected] wrote:
1) return int type variable in bool function:
bool enabled()
{
(quoted diff) +0 -0
 	int ret = 1;
 	return ret;
 }
...
Quoted 6 lines
2)
bool enabled()
{
(quoted diff) +0 -0
 	bool ret = 1;
 	return ret;
 }
...
Quoted 2 lines
so the #1) style function can generate significant instructions
than the #2).
That is a problem for the compiler, not the code.

[ZJ] Actually, it should be bool but not int. Without any optimization by compiler, it is the best if it is the same as returned.

Quoted 11 lines

Quoted 7 lines
Quoted 4 lines
While, this is happened only when "-On" is not used with *-gcc
together. Though, it is oftern there, it is best to provide this with decoupling of which option is used for optimization.
We don't want to dictate minute coding styles to avoid things which are trivially optimized by compilers.
[ZJ] Optimized by compiler is observed only. Such as it is not so big difference in x86-arch.
Quoted 1 line

Quoted 5 lines

Quoted 1 line
Thanks.
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Thanks,
BR//Zhao

[email protected]

author

2018-03-27 16:45 UTC #7 lore raw

In reply to Tejun Heo

Quoted 10 lines · Tejun Heo

Hello, John.
Quoted 1 line · On Sat, Mar 24, 2018 at 01:05:50PM +0800, [email protected] wrote:
as a bool, __cpuset_node_allowed(...) return should be bool.
So, as a minor cleanup patch, this is fine but can you please soften the commit title / description a bit? It doesn't have to be bool. int is fine. bool may be marginally more readable but that's about it, so let's please make the commit match that.

[ZJ] On summary, update it as below:

From 6fd65547d4c704d86fd98cac1d9b8c74c8bee879 Mon Sep 17 00:00:00 2001
From: John Zhao <[email protected]>
Date: Sat, 24 Mar 2018 13:01:32 +0800
Subject: [PATCH] cgroup: allowed of __cpuset_node_allowed to be bool

makes variable allowed returned by the __cpuset_node_allowed(...) to be bool as it is assigned by the test_bits which always return 0 or 1. In addition, it could save the size from typical 4 bytes to 1 byte only. In another side, it could save the potiential unnecessary instructionis during type conversion.

Signed-off-by John Zhao <[email protected]>

---
 kernel/cgroup/cpuset.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

kernel/cgroup/cpuset.c +1 -1

index b42037e..42338b7 100644
@@ -2552,7 +2552,7 @@ static struct cpuset *nearest_hardwall_ancestor(struct cpuset *cs)
 bool __cpuset_node_allowed(int node, gfp_t gfp_mask)
 {
 	struct cpuset *cs;		/* current cpuset ancestors */
-	int allowed;			/* is allocation in zone z allowed? */
+	bool allowed;			/* is allocation in zone z allowed? */
 	unsigned long flags;
 
 	if (in_interrupt())

Signature · 1 line

2.7.4