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

[Julien Grall <julien@xen.org>]

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,

-- 
Julien Grall