Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields

[Samuel Thibault]

Samuel Thibault, le dim. 11 févr. 2024 15:01:43 +0100, a ecrit:
> Damien Zammit, le dim. 11 févr. 2024 11:30:31 +0000, a ecrit:
> > On 2/11/24 10:07 PM, Samuel Thibault wrote:
> > > Damien Zammit, le dim. 11 févr. 2024 10:55:26 +0000, a ecrit:
> > >>>> diff --git a/kern/task.h b/kern/task.h
> > >>>> index dec3a530..27970620 100644
> > >>>> --- a/kern/task.h
> > >>>> +++ b/kern/task.h
> > >>>> @@ -61,11 +61,11 @@ struct task {
> > >>>>        decl_simple_lock_data(,lock)    /* Task's lock */
> > >>>>        int             ref_count;      /* Number of references to me */
> > >>>>
> > >>>> -      /* Flags */
> > >>>> -      unsigned int    active:1,       /* Task has not been terminated 
> > >>>> */
> > >>>> -      /* boolean_t */ may_assign:1,   /* can assigned pset be 
> > >>>> changed? */
> > >>>> -                      assign_active:1,        /* waiting for 
> > >>>> may_assign */
> > >>>> -                      essential:1;    /* Is this task essential for 
> > >>>> the system? */
> > >>>> +      /* Addressable flags */
> > >>>> +      unsigned char   active;         /* Task has not been terminated 
> > >>>> */
> > >>>> +      unsigned char   may_assign;     /* can assigned pset be 
> > >>>> changed? */
> > >>>> +      unsigned char   assign_active;  /* waiting for may_assign */
> > >>>> +      unsigned char   essential;      /* Is this task essential for 
> > >>>> the system? */
> > >>> AIUI only assign_active need to be adressable? Better make only that one
> > >>> addressable.
> > >> Looking at the existing flag types, it needs to fit into part of a 
> > >> cacheline.
> > > Why would it need to fit in a cacheline?
> > 
> > See comment above struct task:
> > 
> >   /*
> >    * Task name buffer size.  The size is chosen so that struct task fits
> >    * into three cache lines.  The size of a cache line on a typical CPU
> >    * is 64 bytes.
> >    */
> >   #define TASK_NAME_SIZE 32
> 
> Ok, but that's only for optimization.
> 
> > >> The way I have done it, I rearranged the existing 4 byte integer to be 4 
> > >> separate single byte flags.
> > >> If you want me to put only one of the values into an addressable field,
> > >> how will I retain the same size for the rest of the fields?
> > > What do you mean by "the same size"?
> > 
> > I think the actual sizeof(struct task) and its layout matters.
> 
> Ok
> 
> > Therefore, my original code change here preserves most of the layout and 
> > sizeof(struct task).
> 
> The original code uses 4 bits, so a byte. Your proposed change uses 4
> bytes, so 3 more bytes. By using e.g.
> 
>       unsigned char may_assign,       /* can assigned pset be changed? */
>       unsigned int    active:1,       /* Task has not been terminated */

I meant unsigned char here of course.

>                       assign_active:1,        /* waiting for may_assign */
>                       essential:1;    /* Is this task essential for the 
> system? */
> 
> That'll be 2 bytes, so even better for cache line size.
> 
> Samuel
> 

-- 
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.