Re: [PATCH gnumach] smp: Create AP processor set and put all APs inside it

[Almudena Garcia]

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 +0000, 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_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 {
> > >   long                    sched_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
> 
> -- 
> Samuel
> ---
> Pour une évaluation indépendante, transparente et rigoureuse !
> Je soutiens la Commission d'Évaluation de l'Inria.
>

-- 
Enviado desde mi dispositivo Sailfish