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

[Julien Grall <julien@xxxxxxx>]

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@xxxxxxxxxx>
> > > >
> > > > Both name and opt_name are pointing to literal string. So mark both of
> > > > the fields as const.
> > > >
> > > > Signed-off-by: Julien Grall <jgrall@xxxxxxxxxx>
> > > Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>
> > > 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,

--
Julien Grall