Re: [PATCH 4/4] chrt: don't rely on exact values of SCHED_* defines

[Povilas Kanapickas]

Hi,

On 14/01/2018 07:48, Kang-Che Sung wrote:
> On Sun, Jan 14, 2018 at 5:30 AM, Povilas Kanapickas  wrote:
>> --- a/util-linux/chrt.c
>> +++ b/util-linux/chrt.c
>> @@ -36,17 +36,20 @@
>>  #include 
>>  #include "libbb.h"
>>
>> -static const struct {
>> -   int policy;
>> -   char name[sizeof("SCHED_OTHER")];
>> -} policies[] = {
>> -   {SCHED_OTHER, "SCHED_OTHER"},
>> -   {SCHED_FIFO, "SCHED_FIFO"},
>> -   {SCHED_RR, "SCHED_RR"},
>> -   {SCHED_BATCH, "SCHED_BATCH"},
>> -   {0 /* unused */, ""},
>> -   {SCHED_IDLE, "SCHED_IDLE"}
>> -};
>> +static const char* get_policy_name(int pol)
>> +{
>> +   if (pol == SCHED_OTHER)
>> +   return "SCHED_OTHER";
>> +   if (pol == SCHED_FIFO)
>> +   return "SCHED_FIFO";
>> +   if (pol == SCHED_RR)
>> +   return "SCHED_RR";
>> +   if (pol == SCHED_BATCH)
>> +   return "SCHED_BATCH";
>> +   if (pol == SCHED_IDLE)
>> +   return "SCHED_IDLE";
>> +   return "";
>> +}
>>
>>  static void show_min_max(int pol)
>>  {
> 
> I don't like the chain of "if"s here. Have you checked the size changes on the
> compiled code?
> Since this SCHED policy constants are just enum values, what about using
> "switch-case" statement? The problem of if-chains is that some compilers are
> not smart at figuring out that the comparisons are just enums and can be
> optimized. The switch-case statement will make it more clear.
> 
> Sorry for nit-picking the code.
> 

Thanks a lot for suggestion. I've checked what impact it has on code
size and indeed switch statement results in smaller code size in all
architectures except x86.

Regards,
Povilas

___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

Re: [PATCH 4/4] chrt: don't rely on exact values of SCHED_* defines

[Kang-Che Sung]

On Sun, Jan 14, 2018 at 5:30 AM, Povilas Kanapickas  wrote:
> --- a/util-linux/chrt.c
> +++ b/util-linux/chrt.c
> @@ -36,17 +36,20 @@
>  #include 
>  #include "libbb.h"
>
> -static const struct {
> -   int policy;
> -   char name[sizeof("SCHED_OTHER")];
> -} policies[] = {
> -   {SCHED_OTHER, "SCHED_OTHER"},
> -   {SCHED_FIFO, "SCHED_FIFO"},
> -   {SCHED_RR, "SCHED_RR"},
> -   {SCHED_BATCH, "SCHED_BATCH"},
> -   {0 /* unused */, ""},
> -   {SCHED_IDLE, "SCHED_IDLE"}
> -};
> +static const char* get_policy_name(int pol)
> +{
> +   if (pol == SCHED_OTHER)
> +   return "SCHED_OTHER";
> +   if (pol == SCHED_FIFO)
> +   return "SCHED_FIFO";
> +   if (pol == SCHED_RR)
> +   return "SCHED_RR";
> +   if (pol == SCHED_BATCH)
> +   return "SCHED_BATCH";
> +   if (pol == SCHED_IDLE)
> +   return "SCHED_IDLE";
> +   return "";
> +}
>
>  static void show_min_max(int pol)
>  {

I don't like the chain of "if"s here. Have you checked the size changes on the
compiled code?
Since this SCHED policy constants are just enum values, what about using
"switch-case" statement? The problem of if-chains is that some compilers are
not smart at figuring out that the comparisons are just enums and can be
optimized. The switch-case statement will make it more clear.

Sorry for nit-picking the code.
___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox

[PATCH 4/4] chrt: don't rely on exact values of SCHED_* defines

[Povilas Kanapickas]

---
 util-linux/chrt.c | 29 -
 1 file changed, 16 insertions(+), 13 deletions(-)

diff --git a/util-linux/chrt.c b/util-linux/chrt.c
index 5c5513aeb..8fd4b7bea 100644
--- a/util-linux/chrt.c
+++ b/util-linux/chrt.c
@@ -36,17 +36,20 @@
 #include 
 #include "libbb.h"
 
-static const struct {
-   int policy;
-   char name[sizeof("SCHED_OTHER")];
-} policies[] = {
-   {SCHED_OTHER, "SCHED_OTHER"},
-   {SCHED_FIFO, "SCHED_FIFO"},
-   {SCHED_RR, "SCHED_RR"},
-   {SCHED_BATCH, "SCHED_BATCH"},
-   {0 /* unused */, ""},
-   {SCHED_IDLE, "SCHED_IDLE"}
-};
+static const char* get_policy_name(int pol)
+{
+   if (pol == SCHED_OTHER)
+   return "SCHED_OTHER";
+   if (pol == SCHED_FIFO)
+   return "SCHED_FIFO";
+   if (pol == SCHED_RR)
+   return "SCHED_RR";
+   if (pol == SCHED_BATCH)
+   return "SCHED_BATCH";
+   if (pol == SCHED_IDLE)
+   return "SCHED_IDLE";
+   return "";
+}
 
 static void show_min_max(int pol)
 {
@@ -57,7 +60,7 @@ static void show_min_max(int pol)
min = sched_get_priority_min(pol);
if ((max|min) < 0)
fmt = "%s not supported\n";
-   printf(fmt, policies[pol].name, min, max);
+   printf(fmt, get_policy_name(pol), min, max);
 }
 
 #define OPT_m (1<<0)
@@ -127,7 +130,7 @@ int chrt_main(int argc UNUSED_PARAM, char **argv)
if (pol < 0)
bb_perror_msg_and_die("can't %cet pid %d's policy", 
'g', (int)pid);
printf("pid %d's %s scheduling policy: %s\n",
-   pid, current_new, policies[pol].name);
+   pid, current_new, get_policy_name(pol));
if (sched_getparam(pid, ))
bb_perror_msg_and_die("can't get pid %d's attributes", 
(int)pid);
printf("pid %d's %s scheduling priority: %d\n",
-- 
2.14.1


___
busybox mailing list
busybox@busybox.net
http://lists.busybox.net/mailman/listinfo/busybox