__proc_give_cores() no longer makes a callback
authorBarret Rhoden <brho@cs.berkeley.edu>
Fri, 9 Mar 2012 21:26:42 +0000 (13:26 -0800)
committerBarret Rhoden <brho@cs.berkeley.edu>
Fri, 9 Mar 2012 21:26:42 +0000 (13:26 -0800)
The put_idle_cores() callback could deadlock if kscheds ever did smart
things in that function.

Kernel schedulers can do more advanced things than in core_request().
For instance, one could attempt to give some cores to one proc, and if
that fails, just give the same pc_arr to the next in line (assuming it
needs them).  Right now, we don't support partial allocations, mostly
because it's the ksched'd job to know what you want and give no more
than that amount.

kern/include/process.h
kern/src/process.c
kern/src/schedule.c

index de4724f..59c77db 100644 (file)
@@ -100,7 +100,7 @@ struct vcore *vcoreid2vcore(struct proc *p, uint32_t vcoreid);
  *
  * WARNING: YOU MUST HOLD THE PROC_LOCK BEFORE CALLING THESE! */
 /* Gives process p the additional num cores listed in corelist */
  *
  * WARNING: YOU MUST HOLD THE PROC_LOCK BEFORE CALLING THESE! */
 /* Gives process p the additional num cores listed in corelist */
-void __proc_give_cores(struct proc *p, uint32_t *pc_arr, uint32_t num);
+int __proc_give_cores(struct proc *p, uint32_t *pc_arr, uint32_t num);
 /* Takes from process p the num cores listed in pc_arr */
 void __proc_take_corelist(struct proc *p, uint32_t *pc_arr, uint32_t num,
                           bool preempt);
 /* Takes from process p the num cores listed in pc_arr */
 void __proc_take_corelist(struct proc *p, uint32_t *pc_arr, uint32_t num,
                           bool preempt);
index 499117f..d9a4b90 100644 (file)
@@ -425,7 +425,7 @@ void proc_run_s(struct proc *p)
        switch (p->state) {
                case (PROC_DYING):
                        spin_unlock(&p->proc_lock);
        switch (p->state) {
                case (PROC_DYING):
                        spin_unlock(&p->proc_lock);
-                       printk("Process %d not starting due to async death\n", p->pid);
+                       printk("[kernel] _S %d not starting due to async death\n", p->pid);
                        return;
                case (PROC_RUNNABLE_S):
                        assert(current != p);
                        return;
                case (PROC_RUNNABLE_S):
                        assert(current != p);
@@ -502,8 +502,10 @@ void __proc_run_m(struct proc *p)
 {
        struct vcore *vc_i;
        switch (p->state) {
 {
        struct vcore *vc_i;
        switch (p->state) {
+               case (PROC_WAITING):
                case (PROC_DYING):
                case (PROC_DYING):
-                       printk("Process %d not starting due to async death\n", p->pid);
+                       warn("ksched tried to run proc %d in state %s\n", p->pid,
+                            procstate2str(p->state));
                        return;
                case (PROC_RUNNABLE_M):
                        /* vcoremap[i] holds the coreid of the physical core allocated to
                        return;
                case (PROC_RUNNABLE_M):
                        /* vcoremap[i] holds the coreid of the physical core allocated to
@@ -535,7 +537,6 @@ void __proc_run_m(struct proc *p)
                         *   it may not get the message for a while... */
                        return;
                case (PROC_RUNNING_M):
                         *   it may not get the message for a while... */
                        return;
                case (PROC_RUNNING_M):
-               case (PROC_WAITING):
                        return;
                default:
                        /* unlock just so the monitor can call something that might lock*/
                        return;
                default:
                        /* unlock just so the monitor can call something that might lock*/
@@ -1253,11 +1254,12 @@ static void __proc_give_cores_running(struct proc *p, uint32_t *pc_arr,
        __seq_end_write(&p->procinfo->coremap_seqctr);
 }
 
        __seq_end_write(&p->procinfo->coremap_seqctr);
 }
 
-/* Gives process p the additional num cores listed in pcorelist.  You must be
- * RUNNABLE_M or RUNNING_M before calling this.  If you're RUNNING_M, this will
- * startup your new cores at the entry point with their virtual IDs (or restore
- * a preemption).  If you're RUNNABLE_M, you should call __proc_run_m after this
- * so that the process can start to use its cores.
+/* Gives process p the additional num cores listed in pcorelist.  If the proc is
+ * not RUNNABLE_M or RUNNING_M, this will fail and allocate none of the core
+ * (and return -1).  If you're RUNNING_M, this will startup your new cores at
+ * the entry point with their virtual IDs (or restore a preemption).  If you're
+ * RUNNABLE_M, you should call __proc_run_m after this so that the process can
+ * start to use its cores.  In either case, this returns 0.
  *
  * If you're *_S, make sure your core0's TF is set (which is done when coming in
  * via arch/trap.c and we are RUNNING_S), change your state, then call this.
  *
  * If you're *_S, make sure your core0's TF is set (which is done when coming in
  * via arch/trap.c and we are RUNNING_S), change your state, then call this.
@@ -1268,21 +1270,19 @@ static void __proc_give_cores_running(struct proc *p, uint32_t *pc_arr,
  * this is called from another core, and to avoid the _S -> _M transition.
  *
  * WARNING: You must hold the proc_lock before calling this! */
  * this is called from another core, and to avoid the _S -> _M transition.
  *
  * WARNING: You must hold the proc_lock before calling this! */
-void __proc_give_cores(struct proc *p, uint32_t *pc_arr, uint32_t num)
+int __proc_give_cores(struct proc *p, uint32_t *pc_arr, uint32_t num)
 {
        /* should never happen: */
        assert(num + p->procinfo->num_vcores <= MAX_NUM_CPUS);
        switch (p->state) {
                case (PROC_RUNNABLE_S):
                case (PROC_RUNNING_S):
 {
        /* should never happen: */
        assert(num + p->procinfo->num_vcores <= MAX_NUM_CPUS);
        switch (p->state) {
                case (PROC_RUNNABLE_S):
                case (PROC_RUNNING_S):
-                       panic("Don't give cores to a process in a *_S state!\n");
-                       break;
+                       warn("Don't give cores to a process in a *_S state!\n");
+                       return -1;
                case (PROC_DYING):
                case (PROC_WAITING):
                case (PROC_DYING):
                case (PROC_WAITING):
-                       /* can't accept, give the cores back to the ksched and return */
-                       for (int i = 0; i < num; i++)
-                               put_idle_core(pc_arr[i]);
-                       return;
+                       /* can't accept, just fail */
+                       return -1;
                case (PROC_RUNNABLE_M):
                        __proc_give_cores_runnable(p, pc_arr, num);
                        break;
                case (PROC_RUNNABLE_M):
                        __proc_give_cores_runnable(p, pc_arr, num);
                        break;
@@ -1293,8 +1293,9 @@ void __proc_give_cores(struct proc *p, uint32_t *pc_arr, uint32_t num)
                        panic("Weird state(%s) in %s()", procstate2str(p->state),
                              __FUNCTION__);
        }
                        panic("Weird state(%s) in %s()", procstate2str(p->state),
                              __FUNCTION__);
        }
-       /* TODO: move me to the ksched */
+       /* TODO: considering moving to the ksched (hard, due to yield) */
        p->procinfo->res_grant[RES_CORES] += num;
        p->procinfo->res_grant[RES_CORES] += num;
+       return 0;
 }
 
 /********** Core revocation (bulk and single) ***********/
 }
 
 /********** Core revocation (bulk and single) ***********/
index 903fd77..e6d137b 100644 (file)
@@ -39,6 +39,7 @@ uint32_t num_mgmtcores = 1;
 
 /* Helper, defined below */
 static void __core_request(struct proc *p);
 
 /* Helper, defined below */
 static void __core_request(struct proc *p);
+static void __put_idle_cores(uint32_t *pc_arr, uint32_t num);
 
 /* Alarm struct, for our example 'timer tick' */
 struct alarm_waiter ksched_waiter;
 
 /* Alarm struct, for our example 'timer tick' */
 struct alarm_waiter ksched_waiter;
@@ -250,8 +251,8 @@ void put_idle_core(uint32_t coreid)
        spin_unlock(&idle_lock);
 }
 
        spin_unlock(&idle_lock);
 }
 
-/* Bulk interface for put_idle */
-void put_idle_cores(uint32_t *pc_arr, uint32_t num)
+/* Helper for put_idle and core_req. */
+static void __put_idle_cores(uint32_t *pc_arr, uint32_t num)
 {
        spin_lock(&idle_lock);
        for (int i = 0; i < num; i++)
 {
        spin_lock(&idle_lock);
        for (int i = 0; i < num; i++)
@@ -259,6 +260,13 @@ void put_idle_cores(uint32_t *pc_arr, uint32_t num)
        spin_unlock(&idle_lock);
 }
 
        spin_unlock(&idle_lock);
 }
 
+/* Bulk interface for put_idle */
+void put_idle_cores(uint32_t *pc_arr, uint32_t num)
+{
+       /* could trigger a sched decision here */
+       __put_idle_cores(pc_arr, num);
+}
+
 /* Available resources changed (plus or minus).  Some parts of the kernel may
  * call this if a particular resource that is 'quantity-based' changes.  Things
  * like available RAM to processes, bandwidth, etc.  Cores would probably be
 /* Available resources changed (plus or minus).  Some parts of the kernel may
  * call this if a particular resource that is 'quantity-based' changes.  Things
  * like available RAM to processes, bandwidth, etc.  Cores would probably be
@@ -324,12 +332,20 @@ static void __core_request(struct proc *p)
        if (num_granted) {
                /* give them the cores.  this will start up the extras if RUNNING_M. */
                spin_lock(&p->proc_lock);
        if (num_granted) {
                /* give them the cores.  this will start up the extras if RUNNING_M. */
                spin_lock(&p->proc_lock);
-               __proc_give_cores(p, corelist, num_granted);
-               /* at some point after giving cores, call proc_run_m() (harmless on
-                * RUNNING_Ms).  You can give small groups of cores, then run them
-                * (which is more efficient than interleaving runs with the gives for
-                * bulk preempted processes). */
-               __proc_run_m(p); /* harmless to call this on RUNNING_Ms */
+               /* if they fail, it is because they are WAITING or DYING.  we could give
+                * the cores to another proc or whatever.  for the current type of
+                * ksched, we'll just put them back on the pile and return.  Note, the
+                * ksched could check the states after locking, but it isn't necessary:
+                * just need to check at some point in the ksched loop. */
+               if (__proc_give_cores(p, corelist, num_granted)) {
+                       __put_idle_cores(corelist, num_granted);
+               } else {
+                       /* at some point after giving cores, call proc_run_m() (harmless on
+                        * RUNNING_Ms).  You can give small groups of cores, then run them
+                        * (which is more efficient than interleaving runs with the gives
+                        * for bulk preempted processes). */
+                       __proc_run_m(p);
+               }
                spin_unlock(&p->proc_lock);
        }
 }
                spin_unlock(&p->proc_lock);
        }
 }