Re: [PATCH gnumach] smp: Rearrange IPI sending mechanism
Damien Zammit, le dim. 11 févr. 2024 07:08:48 +, a ecrit:
> Wait for ICR then just assert the signal. No de-assert.
Please mention in the commit log why.
> ---
> i386/i386/smp.c | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/i386/i386/smp.c b/i386/i386/smp.c
> index 05e9de67..a758eea3 100644
> --- a/i386/i386/smp.c
> +++ b/i386/i386/smp.c
> @@ -54,17 +54,11 @@ static void smp_send_ipi(unsigned apic_id, unsigned
> vector)
>
> cpu_intr_save(&flags);
>
> -apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, ASSERT, EDGE, vector,
> apic_id);
> -
> do {
> cpu_pause();
> } while(lapic->icr_low.delivery_status == SEND_PENDING);
>
> -apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, DE_ASSERT, EDGE, vector,
> apic_id);
> -
> -do {
> -cpu_pause();
> -} while(lapic->icr_low.delivery_status == SEND_PENDING);
> +apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, ASSERT, EDGE, vector,
> apic_id);
>
> cpu_intr_restore(flags);
> }
> --
> 2.43.0
Re: [PATCH gnumach] smp: Fix unable to enter kdb during boot
Damien Zammit, le dim. 11 févr. 2024 07:09:03 +, a ecrit: > --- > i386/i386at/kd.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/i386/i386at/kd.c b/i386/i386at/kd.c > index 984d62e3..486dea67 100644 > --- a/i386/i386at/kd.c > +++ b/i386/i386at/kd.c > @@ -458,6 +458,7 @@ kdopen( > kdinit(); > } > tp->t_state |= TS_CARR_ON; > + unmask_irq(KBD_IRQ); > simple_unlock_irq(o_pri, &tp->t_lock); > return (char_open(dev, tp, flag, ior)); > } > @@ -485,6 +486,7 @@ kdclose(dev_t dev, int flag) > spl_t s; > s = simple_lock_irq(&tp->t_lock); > ttyclose(tp); > + mask_irq(KBD_IRQ); > simple_unlock_irq(s, &tp->t_lock); > } Then we'll have two places that mask/unmask the irq without coordination, that can only bring issues. I'd say that the unmask should rather be done by the place which actually sets up the irq, kdinit. Concerning the mask, since kd_initialized is never set to false, I'd say we don't need any, and we can thus drop it from kbdclose. Samuel
Re: [PATCH gnumach] smp: Fix parenthesis around logic expression value
Applied, thanks!
Damien Zammit, le dim. 11 févr. 2024 07:09:20 +, a ecrit:
> ---
> kern/thread.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kern/thread.c b/kern/thread.c
> index 38287581..de9d1982 100644
> --- a/kern/thread.c
> +++ b/kern/thread.c
> @@ -1921,7 +1921,7 @@ Restart:
>* Reset policy and priorities if needed.
>*/
> #if MACH_FIXPRI
> - if (thread->policy & new_pset->policies == 0) {
> + if ((thread->policy & new_pset->policies) == 0) {
> thread->policy = POLICY_TIMESHARE;
> recompute_pri = TRUE;
> }
> --
> 2.43.0
>
>
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields
Damien Zammit, le dim. 11 févr. 2024 07:09:48 +, a ecrit:
> ---
> configfrag.ac | 2 +-
> kern/task.h | 10 +-
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/configfrag.ac b/configfrag.ac
> index f9285c9d..d059c7b9 100644
> --- a/configfrag.ac
> +++ b/configfrag.ac
> @@ -71,7 +71,7 @@ AC_DEFINE([MACH_DEBUG], [1], [MACH_DEBUG])
> AC_DEFINE([MACH_FIXPRI], [1], [MACH_FIXPRI])
>
> # Mach host (resource alloc.).
> -AC_DEFINE([MACH_HOST], [0], [MACH_HOST])
> +AC_DEFINE([MACH_HOST], [1], [MACH_HOST])
That's enabling a significant part of code. Better only enable it only
for NCPUS > 1
> # IPC debugging calls.
> AC_DEFINE([MACH_IPC_DEBUG], [1], [MACH_IPC_DEBUG])
> 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 intactive: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.
Samuel
>
> /* Miscellaneous */
> vm_map_tmap;/* Address space description */
> --
> 2.43.0
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
I don't understand which is the objective of this. In an SMP system, all
processor are equal in the scheduler. Why do you add this difference?
El domingo 11 de febrero de 2024, Damien Zammit escribió:
> This has the effect of running with one cpu only with smp,
> but has the ability to enable APs in userspace with the right
> processor set RPCs.
> ---
> ddb/db_print.c | 4 +++-
> kern/machine.c | 7 ++-
> kern/processor.c | 30 +++---
> kern/processor.h | 1 +
> 4 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/ddb/db_print.c b/ddb/db_print.c
> index 028cb887..b5b562fd 100644
> --- a/ddb/db_print.c
> +++ b/ddb/db_print.c
> @@ -347,8 +347,10 @@ db_show_all_runqs(
> {
> int i;
>
> - db_printf("Processor set runq:\t");
> + db_printf("Processor set def runq:\t");
> showrq(&default_pset.runq);
> + db_printf("Processor set APs runq:\t");
> + showrq(&ap_pset->runq);
> for (i = 0; i < smp_get_numcpus(); i++) {
> db_printf("Processor #%d runq:\t", i);
> showrq(&cpu_to_processor(i)->runq);
> diff --git a/kern/machine.c b/kern/machine.c
> index 87fbc4d1..9fbc3871 100644
> --- a/kern/machine.c
> +++ b/kern/machine.c
> @@ -84,6 +84,7 @@ void cpu_up(int cpu)
>
> processor = cpu_to_processor(cpu);
> pset_lock(&default_pset);
> + pset_lock(ap_pset);
> s = splsched();
> processor_lock(processor);
> #if NCPUS > 1
> @@ -92,10 +93,14 @@ void cpu_up(int cpu)
> ms = &machine_slot[cpu];
> ms->running = TRUE;
> machine_info.avail_cpus++;
> - pset_add_processor(&default_pset, processor);
> + if (cpu == 0)
> + pset_add_processor(&default_pset, processor);
> + else
> + pset_add_processor(ap_pset, processor);
> processor->state = PROCESSOR_RUNNING;
> processor_unlock(processor);
> splx(s);
> + pset_unlock(ap_pset);
> pset_unlock(&default_pset);
> }
>
> diff --git a/kern/processor.c b/kern/processor.c
> index 76735381..d151f497 100644
> --- a/kern/processor.c
> +++ b/kern/processor.c
> @@ -60,6 +60,7 @@ struct kmem_cache pset_cache;
> int master_cpu;
>
> struct processor_set default_pset;
> +struct processor_set *ap_pset;
>
> queue_head_t all_psets;
> int all_psets_count;
> @@ -67,16 +68,13 @@ def_simple_lock_data(, all_psets_lock);
>
> processor_t master_processor;
>
> -/*
> - * Bootstrap the processor/pset system so the scheduler can run.
> - */
> -void pset_sys_bootstrap(void)
> +static void pset_sys_bootstrap_real(struct processor_set *pset, int start,
> int count)
> {
> int i;
>
> - pset_init(&default_pset);
> - default_pset.empty = FALSE;
> - for (i = 0; i < NCPUS; i++) {
> + pset_init(pset);
> + pset->empty = FALSE;
> + for (i = start; i < start + count; i++) {
> /*
>* Initialize processor data structures.
>* Note that cpu_to_processor is processor_ptr.
> @@ -86,10 +84,10 @@ void pset_sys_bootstrap(void)
> master_processor = cpu_to_processor(master_cpu);
> queue_init(&all_psets);
> simple_lock_init(&all_psets_lock);
> - queue_enter(&all_psets, &default_pset, processor_set_t, all_psets);
> - all_psets_count = 1;
> - default_pset.active = TRUE;
> - default_pset.empty = FALSE;
> + queue_enter(&all_psets, pset, processor_set_t, all_psets);
> + all_psets_count++;
> + pset->active = TRUE;
> + pset->empty = FALSE;
>
> /*
>* Note: the default_pset has a max_priority of BASEPRI_USER.
> @@ -97,6 +95,14 @@ void pset_sys_bootstrap(void)
>*/
> }
>
> +/*
> + * Bootstrap the processor/pset system so the scheduler can run.
> + */
> +void pset_sys_bootstrap(void)
> +{
> + pset_sys_bootstrap_real(&default_pset, 0, NCPUS);
> +}
> +
> #if MACH_HOST
> /*
> * Rest of pset system initializations.
> @@ -124,6 +130,8 @@ void pset_sys_init(void)
> ipc_processor_init(processor);
> }
> }
> +
> + processor_set_create(&realhost, &ap_pset, &ap_pset);
> }
> #endif /* MACH_HOST */
>
> diff --git a/kern/processor.h b/kern/processor.h
> index fc204ffa..29abc99a 100644
> --- a/kern/processor.h
> +++ b/kern/processor.h
> @@ -85,6 +85,7 @@ struct processor_set {
> longsched_load; /* load avg for scheduler */
> };
> extern struct processor_set default_pset;
> +extern struct processor_set *ap_pset;
>
> struct processor {
> struct run_queue runq; /* local runq for this processor */
> --
> 2.43.0
>
>
>
>
--
Enviado desde mi dispositivo Sailfish
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
Damien Zammit, le dim. 11 févr. 2024 07:10:24 +, a ecrit:
> This has the effect of running with one cpu only with smp,
> but has the ability to enable APs in userspace with the right
> processor set RPCs.
> ---
> ddb/db_print.c | 4 +++-
> kern/machine.c | 7 ++-
> kern/processor.c | 30 +++---
> kern/processor.h | 1 +
> 4 files changed, 29 insertions(+), 13 deletions(-)
>
> diff --git a/ddb/db_print.c b/ddb/db_print.c
> index 028cb887..b5b562fd 100644
> --- a/ddb/db_print.c
> +++ b/ddb/db_print.c
> @@ -347,8 +347,10 @@ db_show_all_runqs(
> {
> int i;
>
> - db_printf("Processor set runq:\t");
> + db_printf("Processor set def runq:\t");
> showrq(&default_pset.runq);
> + db_printf("Processor set APs runq:\t");
> + showrq(&ap_pset->runq);
Since userland can create psets as well, better iterate over all_psets.
> for (i = 0; i < smp_get_numcpus(); i++) {
> db_printf("Processor #%d runq:\t", i);
> showrq(&cpu_to_processor(i)->runq);
> diff --git a/kern/machine.c b/kern/machine.c
> index 87fbc4d1..9fbc3871 100644
> --- a/kern/machine.c
> +++ b/kern/machine.c
> @@ -84,6 +84,7 @@ void cpu_up(int cpu)
>
> processor = cpu_to_processor(cpu);
> pset_lock(&default_pset);
> + pset_lock(ap_pset);
Only #if MACH_HOST
> s = splsched();
> processor_lock(processor);
> #if NCPUS > 1
> @@ -92,10 +93,14 @@ void cpu_up(int cpu)
> ms = &machine_slot[cpu];
> ms->running = TRUE;
> machine_info.avail_cpus++;
> - pset_add_processor(&default_pset, processor);
> + if (cpu == 0)
> + pset_add_processor(&default_pset, processor);
> + else
> + pset_add_processor(ap_pset, processor);
Only #if MACH_HOST
> processor->state = PROCESSOR_RUNNING;
> processor_unlock(processor);
> splx(s);
> + pset_unlock(ap_pset);
> pset_unlock(&default_pset);
> }
>
> diff --git a/kern/processor.c b/kern/processor.c
> index 76735381..d151f497 100644
> --- a/kern/processor.c
> +++ b/kern/processor.c
> @@ -60,6 +60,7 @@ struct kmem_cache pset_cache;
> int master_cpu;
>
> struct processor_set default_pset;
> +struct processor_set *ap_pset;
"AP" is an intel name that is mostly unknown otherwise. Since the BSP is
called master processor, we can call the additional pset slaves_pset.
Also only #if MACH_HOST
> queue_head_t all_psets;
> int all_psets_count;
> @@ -67,16 +68,13 @@ def_simple_lock_data(, all_psets_lock);
>
> processor_t master_processor;
>
> -/*
> - * Bootstrap the processor/pset system so the scheduler can run.
> - */
> -void pset_sys_bootstrap(void)
> +static void pset_sys_bootstrap_real(struct processor_set *pset, int start,
> int count)
This refactoring doesn't seem to be used?
> @@ -124,6 +130,8 @@ void pset_sys_init(void)
> ipc_processor_init(processor);
> }
> }
> +
> + processor_set_create(&realhost, &ap_pset, &ap_pset);
> }
> #endif /* MACH_HOST */
>
> diff --git a/kern/processor.h b/kern/processor.h
> index fc204ffa..29abc99a 100644
> --- a/kern/processor.h
> +++ b/kern/processor.h
> @@ -85,6 +85,7 @@ struct processor_set {
> longsched_load; /* load avg for scheduler */
> };
> extern struct processor_set default_pset;
> +extern struct processor_set *ap_pset;
>
> struct processor {
> struct run_queue runq; /* local runq for this processor */
> --
> 2.43.0
>
>
>
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
Almudena Garcia, le dim. 11 févr. 2024 10:48:23 +, a ecrit:
> I don't understand which is the objective of this. In an SMP system, all
> processor are equal in the scheduler. Why do you add this difference?
That's what we had already discussed at the time you introduced
the initial SMP code: there ought to be races in various parts of
translators etc. So for a first, better keep all translators etc. on the
BSP and only progressively try to put stuff on the APs.
Samuel
> El domingo 11 de febrero de 2024, Damien Zammit escribió:
> > This has the effect of running with one cpu only with smp,
> > but has the ability to enable APs in userspace with the right
> > processor set RPCs.
> > ---
> > ddb/db_print.c | 4 +++-
> > kern/machine.c | 7 ++-
> > kern/processor.c | 30 +++---
> > kern/processor.h | 1 +
> > 4 files changed, 29 insertions(+), 13 deletions(-)
> >
> > diff --git a/ddb/db_print.c b/ddb/db_print.c
> > index 028cb887..b5b562fd 100644
> > --- a/ddb/db_print.c
> > +++ b/ddb/db_print.c
> > @@ -347,8 +347,10 @@ db_show_all_runqs(
> > {
> > int i;
> >
> > - db_printf("Processor set runq:\t");
> > + db_printf("Processor set def runq:\t");
> > showrq(&default_pset.runq);
> > + db_printf("Processor set APs runq:\t");
> > + showrq(&ap_pset->runq);
> > for (i = 0; i < smp_get_numcpus(); i++) {
> > db_printf("Processor #%d runq:\t", i);
> > showrq(&cpu_to_processor(i)->runq);
> > diff --git a/kern/machine.c b/kern/machine.c
> > index 87fbc4d1..9fbc3871 100644
> > --- a/kern/machine.c
> > +++ b/kern/machine.c
> > @@ -84,6 +84,7 @@ void cpu_up(int cpu)
> >
> > processor = cpu_to_processor(cpu);
> > pset_lock(&default_pset);
> > + pset_lock(ap_pset);
> > s = splsched();
> > processor_lock(processor);
> > #ifNCPUS > 1
> > @@ -92,10 +93,14 @@ void cpu_up(int cpu)
> > ms = &machine_slot[cpu];
> > ms->running = TRUE;
> > machine_info.avail_cpus++;
> > - pset_add_processor(&default_pset, processor);
> > + if (cpu == 0)
> > + pset_add_processor(&default_pset, processor);
> > + else
> > + pset_add_processor(ap_pset, processor);
> > processor->state = PROCESSOR_RUNNING;
> > processor_unlock(processor);
> > splx(s);
> > + pset_unlock(ap_pset);
> > pset_unlock(&default_pset);
> > }
> >
> > diff --git a/kern/processor.c b/kern/processor.c
> > index 76735381..d151f497 100644
> > --- a/kern/processor.c
> > +++ b/kern/processor.c
> > @@ -60,6 +60,7 @@ struct kmem_cache pset_cache;
> > intmaster_cpu;
> >
> > struct processor_set default_pset;
> > +struct processor_set *ap_pset;
> >
> > queue_head_t all_psets;
> > intall_psets_count;
> > @@ -67,16 +68,13 @@ def_simple_lock_data(, all_psets_lock);
> >
> > processor_tmaster_processor;
> >
> > -/*
> > - * Bootstrap the processor/pset system so the scheduler can run.
> > - */
> > -void pset_sys_bootstrap(void)
> > +static void pset_sys_bootstrap_real(struct processor_set *pset, int start,
> > int count)
> > {
> > int i;
> >
> > - pset_init(&default_pset);
> > - default_pset.empty = FALSE;
> > - for (i = 0; i < NCPUS; i++) {
> > + pset_init(pset);
> > + pset->empty = FALSE;
> > + for (i = start; i < start + count; i++) {
> > /*
> > * Initialize processor data structures.
> > * Note that cpu_to_processor is processor_ptr.
> > @@ -86,10 +84,10 @@ void pset_sys_bootstrap(void)
> > master_processor = cpu_to_processor(master_cpu);
> > queue_init(&all_psets);
> > simple_lock_init(&all_psets_lock);
> > - queue_enter(&all_psets, &default_pset, processor_set_t, all_psets);
> > - all_psets_count = 1;
> > - default_pset.active = TRUE;
> > - default_pset.empty = FALSE;
> > + queue_enter(&all_psets, pset, processor_set_t, all_psets);
> > + all_psets_count++;
> > + pset->active = TRUE;
> > + pset->empty = FALSE;
> >
> > /*
> > * Note: the default_pset has a max_priority of BASEPRI_USER.
> > @@ -97,6 +95,14 @@ void pset_sys_bootstrap(void)
> > */
> > }
> >
> > +/*
> > + * Bootstrap the processor/pset system so the scheduler can run.
> > + */
> > +void pset_sys_bootstrap(void)
> > +{
> > + pset_sys_bootstrap_real(&default_pset, 0, NCPUS);
> > +}
> > +
> > #ifMACH_HOST
> > /*
> > * Rest of pset system initializations.
> > @@ -124,6 +130,8 @@ void pset_sys_init(void)
> > ipc_processor_init(processor);
> > }
> > }
> > +
> > + processor_set_create(&realhost, &ap_pset, &ap_pset);
> > }
> > #endif /* MACH_HOST */
> >
> > diff --git a/kern/processor.h b/kern/processor.h
> > index fc204ffa..29abc99a 100644
> > --- a/kern/processor.h
> > +++ b/kern/processor.h
> > @@ -85,6 +85,7 @@ struct processor_set {
> > longsched_load; /* load avg for scheduler */
> > };
> > extern st
Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields
Hi,
On 2/11/24 9:43 PM, Samuel Thibault wrote:
> Damien Zammit, le dim. 11 févr. 2024 07:09:48 +, a ecrit:
>> diff --git a/configfrag.ac b/configfrag.ac
>> index f9285c9d..d059c7b9 100644
>> --- a/configfrag.ac
>> +++ b/configfrag.ac
>> @@ -71,7 +71,7 @@ AC_DEFINE([MACH_DEBUG], [1], [MACH_DEBUG])
>> AC_DEFINE([MACH_FIXPRI], [1], [MACH_FIXPRI])
>>
>> # Mach host (resource alloc.).
>> -AC_DEFINE([MACH_HOST], [0], [MACH_HOST])
>> +AC_DEFINE([MACH_HOST], [1], [MACH_HOST])
> That's enabling a significant part of code. Better only enable it only
> for NCPUS > 1
OK that is fair.
>> 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 intactive: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.
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?
Damien
Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields
Damien Zammit, le dim. 11 févr. 2024 10:55:26 +, 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 intactive: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?
> 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"?
Samuel
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
With this old patch I got to boot and executive a bunch of threads splitting in
multiple cpus
https://git.zammit.org/gnumach-sv.git/commit/?h=fixes&id=0fe92b6b52726bcd2976863d344117dad8d19694
So I prefer to check which is the block of this that really cause the problem.
With the patch I quote and latest modifications, we got success booting 4/5
times.
Once booted, it usually only crash when i do high load harddisk operations, so
I think there are a condition race in rumpdisk.
But we prefer doesn't put this patch to upstream, or only apply for booting
process, to i can be able to investigate the scheduler and harddisk issues
without needing to enable all AP manually each time I boot the system in smp
mode.
El domingo 11 de febrero de 2024, Samuel Thibault escribió:
> Almudena Garcia, le dim. 11 févr. 2024 10:48:23 +, a ecrit:
> > I don't understand which is the objective of this. In an SMP system, all
> > processor are equal in the scheduler. Why do you add this difference?
>
> That's what we had already discussed at the time you introduced
> the initial SMP code: there ought to be races in various parts of
> translators etc. So for a first, better keep all translators etc. on the
> BSP and only progressively try to put stuff on the APs.
>
> Samuel
>
> > El domingo 11 de febrero de 2024, Damien Zammit escribió:
> > > This has the effect of running with one cpu only with smp,
> > > but has the ability to enable APs in userspace with the right
> > > processor set RPCs.
> > > ---
> > > ddb/db_print.c | 4 +++-
> > > kern/machine.c | 7 ++-
> > > kern/processor.c | 30 +++---
> > > kern/processor.h | 1 +
> > > 4 files changed, 29 insertions(+), 13 deletions(-)
> > >
> > > diff --git a/ddb/db_print.c b/ddb/db_print.c
> > > index 028cb887..b5b562fd 100644
> > > --- a/ddb/db_print.c
> > > +++ b/ddb/db_print.c
> > > @@ -347,8 +347,10 @@ db_show_all_runqs(
> > > {
> > > int i;
> > >
> > > - db_printf("Processor set runq:\t");
> > > + db_printf("Processor set def runq:\t");
> > > showrq(&default_pset.runq);
> > > + db_printf("Processor set APs runq:\t");
> > > + showrq(&ap_pset->runq);
> > > for (i = 0; i < smp_get_numcpus(); i++) {
> > > db_printf("Processor #%d runq:\t", i);
> > > showrq(&cpu_to_processor(i)->runq);
> > > diff --git a/kern/machine.c b/kern/machine.c
> > > index 87fbc4d1..9fbc3871 100644
> > > --- a/kern/machine.c
> > > +++ b/kern/machine.c
> > > @@ -84,6 +84,7 @@ void cpu_up(int cpu)
> > >
> > > processor = cpu_to_processor(cpu);
> > > pset_lock(&default_pset);
> > > + pset_lock(ap_pset);
> > > s = splsched();
> > > processor_lock(processor);
> > > #if NCPUS > 1
> > > @@ -92,10 +93,14 @@ void cpu_up(int cpu)
> > > ms = &machine_slot[cpu];
> > > ms->running = TRUE;
> > > machine_info.avail_cpus++;
> > > - pset_add_processor(&default_pset, processor);
> > > + if (cpu == 0)
> > > + pset_add_processor(&default_pset, processor);
> > > + else
> > > + pset_add_processor(ap_pset, processor);
> > > processor->state = PROCESSOR_RUNNING;
> > > processor_unlock(processor);
> > > splx(s);
> > > + pset_unlock(ap_pset);
> > > pset_unlock(&default_pset);
> > > }
> > >
> > > diff --git a/kern/processor.c b/kern/processor.c
> > > index 76735381..d151f497 100644
> > > --- a/kern/processor.c
> > > +++ b/kern/processor.c
> > > @@ -60,6 +60,7 @@ struct kmem_cache pset_cache;
> > > int master_cpu;
> > >
> > > struct processor_set default_pset;
> > > +struct processor_set *ap_pset;
> > >
> > > queue_head_t all_psets;
> > > int all_psets_count;
> > > @@ -67,16 +68,13 @@ def_simple_lock_data(, all_psets_lock);
> > >
> > > processor_t master_processor;
> > >
> > > -/*
> > > - * Bootstrap the processor/pset system so the scheduler can run.
> > > - */
> > > -void pset_sys_bootstrap(void)
> > > +static void pset_sys_bootstrap_real(struct processor_set *pset, int
> > > start, int count)
> > > {
> > > int i;
> > >
> > > - pset_init(&default_pset);
> > > - default_pset.empty = FALSE;
> > > - for (i = 0; i < NCPUS; i++) {
> > > + pset_init(pset);
> > > + pset->empty = FALSE;
> > > + for (i = start; i < start + count; i++) {
> > > /*
> > >* Initialize processor data structures.
> > >* Note that cpu_to_processor is processor_ptr.
> > > @@ -86,10 +84,10 @@ void pset_sys_bootstrap(void)
> > > master_processor = cpu_to_processor(master_cpu);
> > > queue_init(&all_psets);
> > > simple_lock_init(&all_psets_lock);
> > > - queue_enter(&all_psets, &default_pset, processor_set_t, all_psets);
> > > - all_psets_count = 1;
> > > - default_pset.active = TRUE;
> > > - default_pset.empty = FALSE;
> > > + queue_enter(&all_psets, pset, processor_set_t, all_psets);
> > > + all_psets_count++;
> > > + pset->active = TRUE;
> > > + pset->empty = FALSE;
> > >
> > > /*
> > >* Note: the default_
Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields
Hi,
On 2/11/24 10:07 PM, Samuel Thibault wrote:
> Damien Zammit, le dim. 11 févr. 2024 10:55:26 +, 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 intactive: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
>> 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.
Therefore, my original code change here preserves most of the layout and
sizeof(struct task).
Damien
[PATCH v2 gnumach] smp: Fix unable to enter kdb during boot
---
i386/i386at/kd.c | 1 +
i386/i386at/kd_event.c | 2 --
2 files changed, 1 insertion(+), 2 deletions(-)
diff --git a/i386/i386at/kd.c b/i386/i386at/kd.c
index 984d62e3..2bea3c8c 100644
--- a/i386/i386at/kd.c
+++ b/i386/i386at/kd.c
@@ -1127,6 +1127,7 @@ kdinit(void)
k_comm |= K_CB_ENBLIRQ; /* enable interrupt */
kd_sendcmd(KC_CMD_WRITE); /* write new ctlr command byte */
kd_senddata(k_comm);
+ unmask_irq(KBD_IRQ);
kd_initialized = TRUE;
#ifENABLE_IMMEDIATE_CONSOLE
diff --git a/i386/i386at/kd_event.c b/i386/i386at/kd_event.c
index d4809084..247d95b1 100644
--- a/i386/i386at/kd_event.c
+++ b/i386/i386at/kd_event.c
@@ -121,7 +121,6 @@ kbdopen(dev_t dev, int flags, io_req_t ior)
kdinit();
splx(o_pri);
kbdinit();
- unmask_irq(KBD_IRQ);
return(0);
}
@@ -140,7 +139,6 @@ kbdclose(
{
spl_t s = SPLKD();
- mask_irq(KBD_IRQ);
kb_mode = KB_ASCII;
kdq_reset(&kbd_queue);
splx(s);
--
2.43.0
[PATCH v2 gnumach] smp: Rearrange IPI sending mechanism
Wait for ICR then just assert the signal.
No need for deassert. This is how Linux and NetBSD does it.
I couldn't find documentation on the correct method, however.
---
i386/i386/smp.c | 8 +---
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/i386/i386/smp.c b/i386/i386/smp.c
index 05e9de67..a758eea3 100644
--- a/i386/i386/smp.c
+++ b/i386/i386/smp.c
@@ -54,17 +54,11 @@ static void smp_send_ipi(unsigned apic_id, unsigned vector)
cpu_intr_save(&flags);
-apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, ASSERT, EDGE, vector,
apic_id);
-
do {
cpu_pause();
} while(lapic->icr_low.delivery_status == SEND_PENDING);
-apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, DE_ASSERT, EDGE, vector,
apic_id);
-
-do {
-cpu_pause();
-} while(lapic->icr_low.delivery_status == SEND_PENDING);
+apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, ASSERT, EDGE, vector,
apic_id);
cpu_intr_restore(flags);
}
--
2.43.0
[PATCH v2 gnumach] Enable MACH_HOST and fix non-addressable bitfields
This is only enabled when NCPUS > 1.
Enables some older code paths that allows userspace
to manage cpu resources via processor set RPCs.
Size of struct task is preserved while making 4 bitfields
addressable as 4 single byte fields.
---
configfrag.ac | 8 ++--
kern/task.h | 10 +-
2 files changed, 11 insertions(+), 7 deletions(-)
diff --git a/configfrag.ac b/configfrag.ac
index f9285c9d..b8b41261 100644
--- a/configfrag.ac
+++ b/configfrag.ac
@@ -70,8 +70,12 @@ AC_DEFINE([MACH_DEBUG], [1], [MACH_DEBUG])
# Fixed priority threads.
AC_DEFINE([MACH_FIXPRI], [1], [MACH_FIXPRI])
-# Mach host (resource alloc.).
-AC_DEFINE([MACH_HOST], [0], [MACH_HOST])
+# Mach host (cpu resource alloc.).
+[if [ $mach_ncpus -gt 1 ]; then]
+ AC_DEFINE([MACH_HOST], [1], [MACH_HOST])
+[else]
+ AC_DEFINE([MACH_HOST], [0], [MACH_HOST])
+[fi]
# IPC debugging calls.
AC_DEFINE([MACH_IPC_DEBUG], [1], [MACH_IPC_DEBUG])
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 intactive: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? */
/* Miscellaneous */
vm_map_tmap;/* Address space description */
--
2.43.0
[PATCH v2 gnumach] smp: Create AP processor set and put all APs inside it
This has the effect of running with one cpu only with smp,
but has the ability to enable APs in userspace with the right
processor set RPCs.
---
ddb/db_print.c | 10 +++---
kern/machine.c | 13 +
kern/processor.c | 3 +++
kern/processor.h | 3 +++
4 files changed, 26 insertions(+), 3 deletions(-)
diff --git a/ddb/db_print.c b/ddb/db_print.c
index 028cb887..c8d85d26 100644
--- a/ddb/db_print.c
+++ b/ddb/db_print.c
@@ -345,10 +345,14 @@ db_show_all_runqs(
db_expr_t count,
const char *modif)
{
- int i;
+ int i = 0;
+ processor_set_t pset;
- db_printf("Processor set runq:\t");
- showrq(&default_pset.runq);
+ queue_iterate(&all_psets, pset, processor_set_t, all_psets) {
+ db_printf("Processor set #%d runq:\t", i);
+ showrq(&pset->runq);
+ i++;
+ }
for (i = 0; i < smp_get_numcpus(); i++) {
db_printf("Processor #%d runq:\t", i);
showrq(&cpu_to_processor(i)->runq);
diff --git a/kern/machine.c b/kern/machine.c
index 87fbc4d1..7fed1246 100644
--- a/kern/machine.c
+++ b/kern/machine.c
@@ -84,6 +84,9 @@ void cpu_up(int cpu)
processor = cpu_to_processor(cpu);
pset_lock(&default_pset);
+#ifMACH_HOST
+ pset_lock(slave_pset);
+#endif
s = splsched();
processor_lock(processor);
#ifNCPUS > 1
@@ -92,10 +95,20 @@ void cpu_up(int cpu)
ms = &machine_slot[cpu];
ms->running = TRUE;
machine_info.avail_cpus++;
+#ifMACH_HOST
+ if (cpu == 0)
+ pset_add_processor(&default_pset, processor);
+ else
+ pset_add_processor(slave_pset, processor);
+#else
pset_add_processor(&default_pset, processor);
+#endif
processor->state = PROCESSOR_RUNNING;
processor_unlock(processor);
splx(s);
+#ifMACH_HOST
+ pset_unlock(slave_pset);
+#endif
pset_unlock(&default_pset);
}
diff --git a/kern/processor.c b/kern/processor.c
index 76735381..f06b5d62 100644
--- a/kern/processor.c
+++ b/kern/processor.c
@@ -51,6 +51,7 @@
#ifMACH_HOST
#include
struct kmem_cache pset_cache;
+struct processor_set *slave_pset;
#endif /* MACH_HOST */
@@ -124,6 +125,8 @@ void pset_sys_init(void)
ipc_processor_init(processor);
}
}
+
+ processor_set_create(&realhost, &slave_pset, &slave_pset);
}
#endif /* MACH_HOST */
diff --git a/kern/processor.h b/kern/processor.h
index fc204ffa..747badf2 100644
--- a/kern/processor.h
+++ b/kern/processor.h
@@ -85,6 +85,9 @@ struct processor_set {
longsched_load; /* load avg for scheduler */
};
extern struct processor_setdefault_pset;
+#ifMACH_HOST
+extern struct processor_set*slave_pset;
+#endif
struct processor {
struct run_queue runq; /* local runq for this processor */
--
2.43.0
Re: [PATCH v2 gnumach] smp: Rearrange IPI sending mechanism
The ASSERT and DEASSERT, if I remember well, if a part of SIPI algorithm. I
think that I extracted it from the Intel manuals. Or maybe from OSDev.
El domingo 11 de febrero de 2024, Damien Zammit escribió:
> Wait for ICR then just assert the signal.
> No need for deassert. This is how Linux and NetBSD does it.
> I couldn't find documentation on the correct method, however.
> ---
> i386/i386/smp.c | 8 +---
> 1 file changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/i386/i386/smp.c b/i386/i386/smp.c
> index 05e9de67..a758eea3 100644
> --- a/i386/i386/smp.c
> +++ b/i386/i386/smp.c
> @@ -54,17 +54,11 @@ static void smp_send_ipi(unsigned apic_id, unsigned
> vector)
>
> cpu_intr_save(&flags);
>
> -apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, ASSERT, EDGE, vector,
> apic_id);
> -
> do {
> cpu_pause();
> } while(lapic->icr_low.delivery_status == SEND_PENDING);
>
> -apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, DE_ASSERT, EDGE, vector,
> apic_id);
> -
> -do {
> -cpu_pause();
> -} while(lapic->icr_low.delivery_status == SEND_PENDING);
> +apic_send_ipi(NO_SHORTHAND, FIXED, PHYSICAL, ASSERT, EDGE, vector,
> apic_id);
>
> cpu_intr_restore(flags);
> }
> --
> 2.43.0
>
>
>
>
--
Enviado desde mi dispositivo Sailfish
Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields
Damien Zammit, le dim. 11 févr. 2024 11:30:31 +, a ecrit:
> On 2/11/24 10:07 PM, Samuel Thibault wrote:
> > Damien Zammit, le dim. 11 févr. 2024 10:55:26 +, 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 intactive: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 intactive:1, /* Task has not been terminated */
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
Re: [PATCH v2 gnumach] smp: Fix unable to enter kdb during boot
Applied, thanks!
Damien Zammit, le dim. 11 févr. 2024 12:00:04 +, a ecrit:
> ---
> i386/i386at/kd.c | 1 +
> i386/i386at/kd_event.c | 2 --
> 2 files changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/i386/i386at/kd.c b/i386/i386at/kd.c
> index 984d62e3..2bea3c8c 100644
> --- a/i386/i386at/kd.c
> +++ b/i386/i386at/kd.c
> @@ -1127,6 +1127,7 @@ kdinit(void)
> k_comm |= K_CB_ENBLIRQ; /* enable interrupt */
> kd_sendcmd(KC_CMD_WRITE); /* write new ctlr command byte */
> kd_senddata(k_comm);
> + unmask_irq(KBD_IRQ);
> kd_initialized = TRUE;
>
> #if ENABLE_IMMEDIATE_CONSOLE
> diff --git a/i386/i386at/kd_event.c b/i386/i386at/kd_event.c
> index d4809084..247d95b1 100644
> --- a/i386/i386at/kd_event.c
> +++ b/i386/i386at/kd_event.c
> @@ -121,7 +121,6 @@ kbdopen(dev_t dev, int flags, io_req_t ior)
> kdinit();
> splx(o_pri);
> kbdinit();
> - unmask_irq(KBD_IRQ);
>
> return(0);
> }
> @@ -140,7 +139,6 @@ kbdclose(
> {
> spl_t s = SPLKD();
>
> - mask_irq(KBD_IRQ);
> kb_mode = KB_ASCII;
> kdq_reset(&kbd_queue);
> splx(s);
> --
> 2.43.0
>
>
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
Re: [PATCH v2 gnumach] smp: Create AP processor set and put all APs inside it
Applied, thanks!
Damien Zammit, le dim. 11 févr. 2024 12:00:57 +, a ecrit:
> This has the effect of running with one cpu only with smp,
> but has the ability to enable APs in userspace with the right
> processor set RPCs.
> ---
> ddb/db_print.c | 10 +++---
> kern/machine.c | 13 +
> kern/processor.c | 3 +++
> kern/processor.h | 3 +++
> 4 files changed, 26 insertions(+), 3 deletions(-)
>
> diff --git a/ddb/db_print.c b/ddb/db_print.c
> index 028cb887..c8d85d26 100644
> --- a/ddb/db_print.c
> +++ b/ddb/db_print.c
> @@ -345,10 +345,14 @@ db_show_all_runqs(
> db_expr_t count,
> const char *modif)
> {
> - int i;
> + int i = 0;
> + processor_set_t pset;
>
> - db_printf("Processor set runq:\t");
> - showrq(&default_pset.runq);
> + queue_iterate(&all_psets, pset, processor_set_t, all_psets) {
> + db_printf("Processor set #%d runq:\t", i);
> + showrq(&pset->runq);
> + i++;
> + }
> for (i = 0; i < smp_get_numcpus(); i++) {
> db_printf("Processor #%d runq:\t", i);
> showrq(&cpu_to_processor(i)->runq);
> diff --git a/kern/machine.c b/kern/machine.c
> index 87fbc4d1..7fed1246 100644
> --- a/kern/machine.c
> +++ b/kern/machine.c
> @@ -84,6 +84,9 @@ void cpu_up(int cpu)
>
> processor = cpu_to_processor(cpu);
> pset_lock(&default_pset);
> +#if MACH_HOST
> + pset_lock(slave_pset);
> +#endif
> s = splsched();
> processor_lock(processor);
> #if NCPUS > 1
> @@ -92,10 +95,20 @@ void cpu_up(int cpu)
> ms = &machine_slot[cpu];
> ms->running = TRUE;
> machine_info.avail_cpus++;
> +#if MACH_HOST
> + if (cpu == 0)
> + pset_add_processor(&default_pset, processor);
> + else
> + pset_add_processor(slave_pset, processor);
> +#else
> pset_add_processor(&default_pset, processor);
> +#endif
> processor->state = PROCESSOR_RUNNING;
> processor_unlock(processor);
> splx(s);
> +#if MACH_HOST
> + pset_unlock(slave_pset);
> +#endif
> pset_unlock(&default_pset);
> }
>
> diff --git a/kern/processor.c b/kern/processor.c
> index 76735381..f06b5d62 100644
> --- a/kern/processor.c
> +++ b/kern/processor.c
> @@ -51,6 +51,7 @@
> #if MACH_HOST
> #include
> struct kmem_cache pset_cache;
> +struct processor_set *slave_pset;
> #endif /* MACH_HOST */
>
>
> @@ -124,6 +125,8 @@ void pset_sys_init(void)
> ipc_processor_init(processor);
> }
> }
> +
> + processor_set_create(&realhost, &slave_pset, &slave_pset);
> }
> #endif /* MACH_HOST */
>
> diff --git a/kern/processor.h b/kern/processor.h
> index fc204ffa..747badf2 100644
> --- a/kern/processor.h
> +++ b/kern/processor.h
> @@ -85,6 +85,9 @@ struct processor_set {
> longsched_load; /* load avg for scheduler */
> };
> extern struct processor_set default_pset;
> +#if MACH_HOST
> +extern struct processor_set *slave_pset;
> +#endif
>
> struct processor {
> struct run_queue runq; /* local runq for this processor */
> --
> 2.43.0
>
>
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
Re: [PATCH v2 gnumach] Enable MACH_HOST and fix non-addressable bitfields
fixed and applied, thanks!
Damien Zammit, le dim. 11 févr. 2024 12:00:29 +, a ecrit:
> This is only enabled when NCPUS > 1.
> Enables some older code paths that allows userspace
> to manage cpu resources via processor set RPCs.
>
> Size of struct task is preserved while making 4 bitfields
> addressable as 4 single byte fields.
>
> ---
> configfrag.ac | 8 ++--
> kern/task.h | 10 +-
> 2 files changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/configfrag.ac b/configfrag.ac
> index f9285c9d..b8b41261 100644
> --- a/configfrag.ac
> +++ b/configfrag.ac
> @@ -70,8 +70,12 @@ AC_DEFINE([MACH_DEBUG], [1], [MACH_DEBUG])
> # Fixed priority threads.
> AC_DEFINE([MACH_FIXPRI], [1], [MACH_FIXPRI])
>
> -# Mach host (resource alloc.).
> -AC_DEFINE([MACH_HOST], [0], [MACH_HOST])
> +# Mach host (cpu resource alloc.).
> +[if [ $mach_ncpus -gt 1 ]; then]
> + AC_DEFINE([MACH_HOST], [1], [MACH_HOST])
> +[else]
> + AC_DEFINE([MACH_HOST], [0], [MACH_HOST])
> +[fi]
>
> # IPC debugging calls.
> AC_DEFINE([MACH_IPC_DEBUG], [1], [MACH_IPC_DEBUG])
> 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 intactive: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? */
>
> /* Miscellaneous */
> vm_map_tmap;/* Address space description */
> --
> 2.43.0
>
>
>
--
Samuel
---
Pour une évaluation indépendante, transparente et rigoureuse !
Je soutiens la Commission d'Évaluation de l'Inria.
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
Almudena Garcia, le dim. 11 févr. 2024 11:17:01 +, a ecrit: > With this old patch I got to boot and executive a bunch of threads splitting > in multiple cpus > > https://git.zammit.org/gnumach-sv.git/commit/?h=fixes&id=0fe92b6b52726bcd2976863d344117dad8d19694 > > With the patch I quote and latest modifications, we got success booting 4/5 > times. Ok, but this patch is only making races less probable. I.e. it is making the problems even harder to spot. We don't want that, we rather want to corner problems so that we can more easily fix them. Using psets is a way to corner problems: avoid them for most of the system, and get exposed to the for selected pieces of the system. > So I prefer to check which is the block of this that really cause the > problem. By making the races less probable it will be even harder to identify what is posing the problem. By being able to select which block gets exposed to parallelism, you can selectively check which blocks work and which don't. > Once booted, it usually only crash when i do high load harddisk operations, > so I think there are a condition race in rumpdisk. Not necessarily rumpdisk, it can also be libpager, ext2fs, etc. > But we prefer doesn't put this patch to upstream, or only apply for booting > process, to i can be able to investigate the scheduler and harddisk issues > without needing to enable all AP manually each time I boot the system in smp > mode. That's the point of the patch: now you can use psets to choose what processes/translators you put in the slaves pset. Samuel
Re: [PATCH gnumach] Enable MACH_HOST and fix non-addressable bitfields
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 +, a ecrit:
> > On 2/11/24 10:07 PM, Samuel Thibault wrote:
> > > Damien Zammit, le dim. 11 févr. 2024 10:55:26 +, 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 intactive: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 intactive: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.
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
Samuel Thibault, le dim. 11 févr. 2024 15:13:51 +0100, a ecrit: > By being able to select which block gets exposed to parallelism, you can > selectively check which blocks work and which don't. (while also being already useful to be able to run make in parallel ;) Samuel
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
But, how can I add the processors to the pset from userspace? I want to test the parallel environment from Hurd, although gnumach boots with 1 processor El dom, 11 feb 2024 a las 15:36, Samuel Thibault () escribió: > Samuel Thibault, le dim. 11 févr. 2024 15:13:51 +0100, a ecrit: > > By being able to select which block gets exposed to parallelism, you can > > selectively check which blocks work and which don't. > > (while also being already useful to be able to run make in parallel ;) > > Samuel >
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
Almudena Garcia, le dim. 11 févr. 2024 16:23:17 +0100, a ecrit: > But, how can I add the processors to the pset from userspace? Using processor set RPCs Probably we are missing an RPC to get the pset containing APs, but that's not hard to add. Samuel
Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it
Samuel Thibault, le dim. 11 févr. 2024 16:50:51 +0100, a ecrit: > Almudena Garcia, le dim. 11 févr. 2024 16:23:17 +0100, a ecrit: > > But, how can I add the processors to the pset from userspace? > > Using processor set RPCs > > Probably we are missing an RPC to get the pset containing APs, but > that's not hard to add. Or you can probably create a new pset and add the APs to it. Samuel
[PATCH gnumach] smp: Set processor set to non-empty when adding a processor
This allows the slave_pset to be used for actual tasks with the processor_set RPCs. --- kern/processor.c | 1 + 1 file changed, 1 insertion(+) diff --git a/kern/processor.c b/kern/processor.c index f06b5d62..71bbb757 100644 --- a/kern/processor.c +++ b/kern/processor.c @@ -245,6 +245,7 @@ void pset_add_processor( queue_enter(&pset->processors, processor, processor_t, processors); processor->processor_set = pset; pset->processor_count++; + pset->empty = FALSE; quantum_set(pset); } -- 2.43.0
Test program for running task on slave_pset
Hi,
This program requires master branch PLUS
a one line patch I just mailed in for gnumach.
Here is a simple test program to enable
a new task to run on slave_pset processor set
containing the rest of the processors in SMP kernel.
You can compile it as a sutil in hurd/sutils:
usage must be run as root eg:
$ sudo /path/to/smp /bin/bash
# stress -c 7
Damien
>
$ cat smp.c
/* Run a task on slave_pset
Copyright (C) 2024 Free Software Foundation, Inc.
This file is part of the GNU Hurd.
The GNU Hurd is free software; you can redistribute it and/or
modify it under the terms of the GNU General Public License as
published by the Free Software Foundation; either version 2, or (at
your option) any later version.
The GNU Hurd is distributed in the hope that it will be useful, but
WITHOUT ANY WARRANTY; without even the implied warranty of
MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
General Public License for more details.
You should have received a copy of the GNU General Public License
along with this program; if not, write to the Free Software
Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA. */
#include
#include
#include
#include
#include
#include
#include
#include
static void
smp(char * const argv[])
{
int err;
unsigned int pset_count;
mach_port_t hostpriv;
processor_set_name_array_t psets = {0};
processor_set_t slave_pset = {0};
if (get_privileged_ports (&hostpriv, NULL))
error (1, 0, "Must be run as root for privileged cpu control");
err = host_processor_sets (hostpriv, &psets, &pset_count);
if (err)
error (1, 0, "Cannot get list of host processor sets");
if (pset_count < 2)
error (1, 0, "gnumach does not have the expected processor sets,
are you running smp kernel?");
err = host_processor_set_priv (hostpriv, psets[1], &slave_pset);
if (err)
error (1, 0, "Cannot get access to slave processor set");
err = task_assign(mach_task_self(), slave_pset, FALSE);
if (err)
error (1, 0, "Cannot assign task self to slave processor set");
execve(argv[1], &argv[1], NULL);
/* NOT REACHED */
}
int
main (int argc, char **argv)
{
smp(argv);
return 0;
}
[PATCH 1/2] Replace kernel header includes in include/mach/mach_types.h with forward declarations.
I was trying to reuse TASK_NAME_SIZE in kern/thread.h but it was
impossible because files included from kern/task.h end up requiring
kern/thread.h (through percpu.h), creating a recursive dependency.
With this change, mach_types.h only defines forward declarations and
modules have to explicitly include the appropriate header file if they
want to be able touch those structures. Most of the other includes are
required because we no longer grab many different includes through
mach_types.h.
---
ddb/db_examine.c | 1 +
device/io_req.h | 1 +
i386/i386/machine_task.c | 1 +
i386/i386/percpu.h| 2 +-
i386/i386/trap.h | 1 +
i386/i386at/int_init.c| 1 +
include/mach/mach_types.h | 13 ++---
include/mach/std_types.h | 4
ipc/ipc_space.h | 1 +
kern/eventcount.h | 2 ++
kern/ipc_mig.h| 1 +
kern/thread.c | 4
12 files changed, 20 insertions(+), 12 deletions(-)
diff --git a/ddb/db_examine.c b/ddb/db_examine.c
index 88d7a57..1941fc3 100644
--- a/ddb/db_examine.c
+++ b/ddb/db_examine.c
@@ -47,6 +47,7 @@
#include
#include
#include
+#include
#define db_thread_to_task(thread) ((thread)? thread->task: TASK_NULL)
diff --git a/device/io_req.h b/device/io_req.h
index e66e080..fb63696 100644
--- a/device/io_req.h
+++ b/device/io_req.h
@@ -38,6 +38,7 @@
#include
#include
#include
+#include
#include
#include
#include
diff --git a/i386/i386/machine_task.c b/i386/i386/machine_task.c
index d592838..8bebf36 100644
--- a/i386/i386/machine_task.c
+++ b/i386/i386/machine_task.c
@@ -23,6 +23,7 @@
#include
#include
#include
+#include
#include
#include
diff --git a/i386/i386/percpu.h b/i386/i386/percpu.h
index 86b0a31..637d2ca 100644
--- a/i386/i386/percpu.h
+++ b/i386/i386/percpu.h
@@ -66,7 +66,7 @@ MACRO_END
#endif
#include
-#include
+#include
struct percpu {
struct percpu *self;
diff --git a/i386/i386/trap.h b/i386/i386/trap.h
index e82164d..db22273 100644
--- a/i386/i386/trap.h
+++ b/i386/i386/trap.h
@@ -30,6 +30,7 @@
#include
#ifndef __ASSEMBLER__
+#include
#include
char *trap_name(unsigned int trapnum);
diff --git a/i386/i386at/int_init.c b/i386/i386at/int_init.c
index 262bef1..5c8fce6 100644
--- a/i386/i386at/int_init.c
+++ b/i386/i386at/int_init.c
@@ -23,6 +23,7 @@
#include
#include
+#include
#include
#include
#ifdef APIC
diff --git a/include/mach/mach_types.h b/include/mach/mach_types.h
index 57f8f22..5ecd686 100644
--- a/include/mach/mach_types.h
+++ b/include/mach/mach_types.h
@@ -57,13 +57,12 @@
#include
#ifdef MACH_KERNEL
-#include /* for task_array_t */
-#include/* for thread_array_t */
-#include /* for processor_array_t,
- processor_set_array_t,
- processor_set_name_array_t */
-#include
- /* for emulation_vector_t */
+
+typedef struct task*task_t;
+typedef struct thread *thread_t;
+typedef struct processor *processor_t;
+typedef struct processor_set *processor_set_t;
+
#else /* MACH_KERNEL */
typedefmach_port_t task_t;
typedef task_t *task_array_t;
diff --git a/include/mach/std_types.h b/include/mach/std_types.h
index f78e236..0d5db0a 100644
--- a/include/mach/std_types.h
+++ b/include/mach/std_types.h
@@ -41,8 +41,4 @@
typedefvm_offset_t pointer_t;
typedefvm_offset_t vm_address_t;
-#ifdef MACH_KERNEL
-#include
-#endif /* MACH_KERNEL */
-
#endif /* _MACH_STD_TYPES_H_ */
diff --git a/ipc/ipc_space.h b/ipc/ipc_space.h
index 3f0eaa0..96d5894 100644
--- a/ipc/ipc_space.h
+++ b/ipc/ipc_space.h
@@ -49,6 +49,7 @@
#include
#include
#include
+#include
#include
/*
diff --git a/kern/eventcount.h b/kern/eventcount.h
index 7cc8220..598d7e0 100644
--- a/kern/eventcount.h
+++ b/kern/eventcount.h
@@ -35,6 +35,8 @@
#ifndef_KERN_EVENTCOUNT_H_
#define_KERN_EVENTCOUNT_H_ 1
+#include
+
/* kernel visible only */
typedef struct evc {
diff --git a/kern/ipc_mig.h b/kern/ipc_mig.h
index a8ee786..422e8d8 100644
--- a/kern/ipc_mig.h
+++ b/kern/ipc_mig.h
@@ -28,6 +28,7 @@
#include
#include
+#include
/*
* Routine:mach_msg_send_from_kernel
diff --git a/kern/thread.c b/kern/thread.c
index de9d198..23ee8b0 100644
--- a/kern/thread.c
+++ b/kern/thread.c
@@ -32,12 +32,15 @@
*/
#include
+#include
#include
#include
#include
#include
#include
#include
+#include
+#include
#include
#include
#include
@@ -59,6 +62,7 @@
#include
#include
#include
+#include
#include
#include
#include
--
2.39.2
[PATCH 2/2] Add thread_set_name RPC.
Like task_set_name, we use the same size as the task name and will
inherit the task name, whenever it exists. This will be used to
implement pthread_setname_np.
---
ddb/db_print.c| 9 +
include/mach/gnumach.defs | 8
kern/thread.c | 20
kern/thread.h | 8
4 files changed, 41 insertions(+), 4 deletions(-)
diff --git a/ddb/db_print.c b/ddb/db_print.c
index c8d85d2..f08dd6c 100644
--- a/ddb/db_print.c
+++ b/ddb/db_print.c
@@ -222,10 +222,11 @@ db_print_thread(
}
} else {
if (flag & OPTION_INDENT)
- db_printf("%3d (%0*X) ", thread_id,
- 2*sizeof(vm_offset_t), thread);
- else
- db_printf("(%0*X) ", 2*sizeof(vm_offset_t), thread);
+ db_printf("%3d ", thread_id);
+ if (thread->name[0] &&
+ strncmp (thread->name, thread->task->name, THREAD_NAME_SIZE))
+ db_printf("%s ", thread->name);
+ db_printf("(%0*X) ", 2*sizeof(vm_offset_t), thread);
char status[8];
db_printf("%s", db_thread_stat(thread, status));
if (thread->state & TH_SWAPPED) {
diff --git a/include/mach/gnumach.defs b/include/mach/gnumach.defs
index 6252de9..7ecf74d 100644
--- a/include/mach/gnumach.defs
+++ b/include/mach/gnumach.defs
@@ -207,3 +207,11 @@ routine vm_pages_phys(
vaddr : vm_address_t;
size: vm_size_t;
out pages : rpc_phys_addr_array_t);
+
+/*
+ * Set the name of thread THREAD to NAME. This is a debugging aid.
+ * NAME will be used in error messages printed by the kernel.
+ */
+simpleroutine thread_set_name(
+ thread : thread_t;
+ name: kernel_debug_name_t);
diff --git a/kern/thread.c b/kern/thread.c
index 23ee8b0..2eab1ca 100644
--- a/kern/thread.c
+++ b/kern/thread.c
@@ -46,6 +46,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -551,6 +552,10 @@ kern_return_t thread_create(
#endif /* MACH_PCSAMPLE */
new_thread->pc_sample.buffer = 0;
+
+ /* Inherit the task name as the thread name. */
+ memcpy (new_thread->name, parent_task->name, THREAD_NAME_SIZE);
+
/*
* Add the thread to the task`s list of threads.
* The new thread holds another reference to the task.
@@ -2624,3 +2629,18 @@ thread_stats(void)
printf("%d using rpc_reply.\n", rpcreply);
}
#endif /* MACH_DEBUG */
+
+/*
+ * thread_set_name
+ *
+ * Set the name of thread THREAD to NAME.
+ */
+kern_return_t
+thread_set_name(
+ thread_tthread,
+ const_kernel_debug_name_t name)
+{
+ strncpy(thread->name, name, sizeof thread->name - 1);
+ thread->name[sizeof thread->name - 1] = '\0';
+ return KERN_SUCCESS;
+}
diff --git a/kern/thread.h b/kern/thread.h
index 7bfe2e8..21b2503 100644
--- a/kern/thread.h
+++ b/kern/thread.h
@@ -54,6 +54,12 @@
#include
#include
+/*
+ * Thread name buffer size. Use the same size as the task so
+ * the thread can inherit the task's name.
+ */
+#define THREAD_NAME_SIZE TASK_NAME_SIZE
+
struct thread {
/* Run queues */
queue_chain_t links; /* current run queue links */
@@ -232,6 +238,8 @@ struct thread {
#ifMACH_LOCK_MON
unsigned lock_stack;
#endif
+
+ charname[THREAD_NAME_SIZE];
};
#include
--
2.39.2
