diff mbox series

[02/14] xen/sched: Constify name and opt_name in struct scheduler

Message ID 20210405155713.29754-3-julien@xen.org (mailing list archive)
State New, archived
Headers show
Series Use const whether we point to literal strings (take 1) | expand

Commit Message

Julien Grall April 5, 2021, 3:57 p.m. UTC 
From: Julien Grall <jgrall@amazon.com>

Both name and opt_name are pointing to literal string. So mark both of
the fields as const.

Signed-off-by: Julien Grall <jgrall@amazon.com>
---
 xen/common/sched/private.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jan Beulich April 6, 2021, 8:07 a.m. UTC | #1 
On 05.04.2021 17:57, Julien Grall wrote:
> From: Julien Grall <jgrall@amazon.com>
> 
> Both name and opt_name are pointing to literal string. So mark both of
> the fields as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit ...

> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>  }
>  
>  struct scheduler {
> -    char *name;             /* full name for this scheduler      */
> -    char *opt_name;         /* option name for this scheduler    */
> +    const char *name;       /* full name for this scheduler      */
> +    const char *opt_name;   /* option name for this scheduler    */

... I'd like to suggest considering at least the latter to become
an array instead of a pointer - there's little point wasting 8
bytes of storage for the pointer when the strings pointed to are
all at most 9 bytes long (right now; I don't expect much longer
ones to appear).

Jan
George Dunlap April 6, 2021, 2:19 p.m. UTC | #2 
> On Apr 5, 2021, at 4:57 PM, Julien Grall <julien@xen.org> wrote:
> 
> From: Julien Grall <jgrall@amazon.com>
> 
> Both name and opt_name are pointing to literal string. So mark both of
> the fields as const.
> 
> Signed-off-by: Julien Grall <jgrall@amazon.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>
Julien Grall April 6, 2021, 6:24 p.m. UTC | #3 
Hi Jan,

On 06/04/2021 09:07, Jan Beulich wrote:
> On 05.04.2021 17:57, Julien Grall wrote:
>> From: Julien Grall <jgrall@amazon.com>
>>
>> Both name and opt_name are pointing to literal string. So mark both of
>> the fields as const.
>>
>> Signed-off-by: Julien Grall <jgrall@amazon.com>
> 
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit ...
> 
>> --- a/xen/common/sched/private.h
>> +++ b/xen/common/sched/private.h
>> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>>   }
>>   
>>   struct scheduler {
>> -    char *name;             /* full name for this scheduler      */
>> -    char *opt_name;         /* option name for this scheduler    */
>> +    const char *name;       /* full name for this scheduler      */
>> +    const char *opt_name;   /* option name for this scheduler    */
> 
> ... I'd like to suggest considering at least the latter to become
> an array instead of a pointer - there's little point wasting 8
> bytes of storage for the pointer when the strings pointed to are
> all at most 9 bytes long (right now; I don't expect much longer
> ones to appear).

I have tried this simple/dumb change on top of my patch:

diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index a870320146ef..ab2236874217 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -273,7 +273,7 @@ static inline spinlock_t 
*pcpu_schedule_trylock(unsigned int cpu)

  struct scheduler {
      const char *name;       /* full name for this scheduler      */
-    const char *opt_name;   /* option name for this scheduler    */
+    const char opt_name[9]; /* option name for this scheduler    */
      unsigned int sched_id;  /* ID for this scheduler             */
      void *sched_data;       /* global data pointer               */
      struct cpupool *cpupool;/* points to this scheduler's pool   */

GCC will throw an error:

core.c: In function ‘scheduler_init’:
core.c:2987:17: error: assignment of read-only variable ‘ops’
              ops = *schedulers[i];
                  ^
core.c:2997:21: error: assignment of read-only variable ‘ops’
                  ops = *schedulers[i];
                      ^

I don't particularly want to drop the const. So the code would probably 
need some rework.

My patch doesn't change the size of the structure, so I would prefer to 
keep this patch as-is.

Cheers,
Jan Beulich April 7, 2021, 8:22 a.m. UTC | #4 
On 06.04.2021 20:24, Julien Grall wrote:
> Hi Jan,
> 
> On 06/04/2021 09:07, Jan Beulich wrote:
>> On 05.04.2021 17:57, Julien Grall wrote:
>>> From: Julien Grall <jgrall@amazon.com>
>>>
>>> Both name and opt_name are pointing to literal string. So mark both of
>>> the fields as const.
>>>
>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>
>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>> albeit ...
>>
>>> --- a/xen/common/sched/private.h
>>> +++ b/xen/common/sched/private.h
>>> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>>>   }
>>>   
>>>   struct scheduler {
>>> -    char *name;             /* full name for this scheduler      */
>>> -    char *opt_name;         /* option name for this scheduler    */
>>> +    const char *name;       /* full name for this scheduler      */
>>> +    const char *opt_name;   /* option name for this scheduler    */
>>
>> ... I'd like to suggest considering at least the latter to become
>> an array instead of a pointer - there's little point wasting 8
>> bytes of storage for the pointer when the strings pointed to are
>> all at most 9 bytes long (right now; I don't expect much longer
>> ones to appear).
> 
> I have tried this simple/dumb change on top of my patch:
> 
> diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
> index a870320146ef..ab2236874217 100644
> --- a/xen/common/sched/private.h
> +++ b/xen/common/sched/private.h
> @@ -273,7 +273,7 @@ static inline spinlock_t 
> *pcpu_schedule_trylock(unsigned int cpu)
> 
>   struct scheduler {
>       const char *name;       /* full name for this scheduler      */
> -    const char *opt_name;   /* option name for this scheduler    */
> +    const char opt_name[9]; /* option name for this scheduler    */
>       unsigned int sched_id;  /* ID for this scheduler             */
>       void *sched_data;       /* global data pointer               */
>       struct cpupool *cpupool;/* points to this scheduler's pool   */
> 
> GCC will throw an error:
> 
> core.c: In function ‘scheduler_init’:
> core.c:2987:17: error: assignment of read-only variable ‘ops’
>               ops = *schedulers[i];
>                   ^
> core.c:2997:21: error: assignment of read-only variable ‘ops’
>                   ops = *schedulers[i];
>                       ^
> 
> I don't particularly want to drop the const. So the code would probably 
> need some rework.

What's wrong with not having the const when the field is an array?
The more that all original (build-time, i.e. contain in the binary)
instances of the struct are already const as a whole?

Jan
Julien Grall April 7, 2021, 9:06 a.m. UTC | #5 
Hi Jan,

On 07/04/2021 09:22, Jan Beulich wrote:
> On 06.04.2021 20:24, Julien Grall wrote:
>> Hi Jan,
>>
>> On 06/04/2021 09:07, Jan Beulich wrote:
>>> On 05.04.2021 17:57, Julien Grall wrote:
>>>> From: Julien Grall <jgrall@amazon.com>
>>>>
>>>> Both name and opt_name are pointing to literal string. So mark both of
>>>> the fields as const.
>>>>
>>>> Signed-off-by: Julien Grall <jgrall@amazon.com>
>>>
>>> Reviewed-by: Jan Beulich <jbeulich@suse.com>
>>> albeit ...
>>>
>>>> --- a/xen/common/sched/private.h
>>>> +++ b/xen/common/sched/private.h
>>>> @@ -272,8 +272,8 @@ static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
>>>>    }
>>>>    
>>>>    struct scheduler {
>>>> -    char *name;             /* full name for this scheduler      */
>>>> -    char *opt_name;         /* option name for this scheduler    */
>>>> +    const char *name;       /* full name for this scheduler      */
>>>> +    const char *opt_name;   /* option name for this scheduler    */
>>>
>>> ... I'd like to suggest considering at least the latter to become
>>> an array instead of a pointer - there's little point wasting 8
>>> bytes of storage for the pointer when the strings pointed to are
>>> all at most 9 bytes long (right now; I don't expect much longer
>>> ones to appear).
>>
>> I have tried this simple/dumb change on top of my patch:
>>
>> diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
>> index a870320146ef..ab2236874217 100644
>> --- a/xen/common/sched/private.h
>> +++ b/xen/common/sched/private.h
>> @@ -273,7 +273,7 @@ static inline spinlock_t
>> *pcpu_schedule_trylock(unsigned int cpu)
>>
>>    struct scheduler {
>>        const char *name;       /* full name for this scheduler      */
>> -    const char *opt_name;   /* option name for this scheduler    */
>> +    const char opt_name[9]; /* option name for this scheduler    */
>>        unsigned int sched_id;  /* ID for this scheduler             */
>>        void *sched_data;       /* global data pointer               */
>>        struct cpupool *cpupool;/* points to this scheduler's pool   */
>>
>> GCC will throw an error:
>>
>> core.c: In function ‘scheduler_init’:
>> core.c:2987:17: error: assignment of read-only variable ‘ops’
>>                ops = *schedulers[i];
>>                    ^
>> core.c:2997:21: error: assignment of read-only variable ‘ops’
>>                    ops = *schedulers[i];
>>                        ^
>>
>> I don't particularly want to drop the const. So the code would probably
>> need some rework.
> 
> What's wrong with not having the const when the field is an array?
> The more that all original (build-time, i.e. contain in the binary)
> instances of the struct are already const as a whole?

The scheduler will do a shallow copy of the structure that will be 
non-const. So opt_name can be modified afterwards (one can argue this is 
unlikely).

If I have to chose between saving overall 20 bytes in the binary and 
const. I would chose the latter.

Cheers,
diff mbox series

Patch

diff --git a/xen/common/sched/private.h b/xen/common/sched/private.h
index 92d0d4961063..a870320146ef 100644
--- a/xen/common/sched/private.h
+++ b/xen/common/sched/private.h
@@ -272,8 +272,8 @@  static inline spinlock_t *pcpu_schedule_trylock(unsigned int cpu)
 }
 
 struct scheduler {
-    char *name;             /* full name for this scheduler      */
-    char *opt_name;         /* option name for this scheduler    */
+    const char *name;       /* full name for this scheduler      */
+    const char *opt_name;   /* option name for this scheduler    */
     unsigned int sched_id;  /* ID for this scheduler             */
     void *sched_data;       /* global data pointer               */
     struct cpupool *cpupool;/* points to this scheduler's pool   */