Modifying Minix 3 Scheduler

Question

I recently purchased this book to get a better understanding of how operating systems work. I am on the second chapter and I'm stuck on this problem and my OS does not boot with code I have added. The code below was added to proc.c at the start of the pic_proc function to try and modify the scheduler.

Modification.

int realtime;
clock_t recent_time[NR_TASKS + NR_PROCS];
clock_t stopwatch;

realtime = getuptime();
recent_time[proc_ptr->p_nr + NR_TASKS] = realtime - stopwatch;
stopwatch = realtime;

Original Code:

    PRIVATE void pick_proc()
    {
    register struct proc *rp;                     /* process to run */
    int q;                                        /* iterate over queues */
    
    /* Modified Code here */

     for (q=0; q < NR_SCHED_QUEUES; q++) {
        if ( (rp = rdy_head[q]) != NIL_PROC) {
              next_ptr = rp;                      
              if (priv(rp)->s_flags & BILLABLE)
                  bill_ptr = rp; 
     return;
      }
}
}

Book Operating Systems Design and Implementation. 3rd, Edition The Minix Book, P219, #45. Modify the Minix 3 scheduler to keep track of how much CPU time each user process has had recently. When no task or server wants to run, pick the user process that has had the smallest share of the CPU.

_How_ does it "not boot"? Segfault, protection exception, etc.? Is `proc_ptr` _valid_ when your code is called? It might be `NULL`. Is `proc_ptr->p_nr` in the range 0 to `NR_PROC - 1`? If not, you've got UB [undefined behavior] and you're corrupting the stack [I assume `recent_time` is on the stack] because you're indexing beyond the end of `recent_time`. You should add `if` statement checks for validity and add [minux equiv of] `printk` to show the values. Maybe `realtime` should be `unsigned` [and/or `clock_t` as your setting `stopwatch` to its value]. Can `recent_time` fit on the stack? — Craig Estey, Mar 23 '21 at 21:24
Ive added the pic_proc function that i modified if that helps. After doing the make fresh install It will compile the new image. When I try to boot from that image it deadlocks and I have to shut it down and return to the previous image file. — Warfujjii, Mar 23 '21 at 21:50

Craig Estey · Accepted Answer · 2021-03-24T02:10:49.080

Based on your new code [and the pick_proc function body], the only possible failures are the ones I've described above in my top comments.

That is, proc_ptr must be valid (i.e. non-NULL) and proc_ptr->p_nr must be in range to prevent UB.

What are the NR_TASKS and NR_PROCS values? Can recent_time fit on the stack? Stack size in a kernel is usually very limited. For example, under linux, you can only have about 4KB of stack.

So, if NR_PROCS was (e.g.) 32767, then recent_time could not be placed on the stack.

And, I don't see a reason the have an independent array for this on the stack as it won't persist past the call.

So, to aid in debug, I'd add static to the recent_time definition

You'll need to add some debug code to see what the issue is.

Here's a sample. Adjust the printf et. al. to suit the minux kernel's print mechanism [assuming you can print at the state your code is called in]:

#ifdef DEBUG
#define dbgprt(_fmt...) \
    printf(_fmt)
#else
#define dbgprt(_fmt...) \
    do { } while (0)
#endif

PRIVATE void
pick_proc()
{
    register struct proc *rp;           /* process to run */
    int q;                              /* iterate over queues */

    /* Modified Code here */
    int realtime;
    static clock_t recent_time[NR_TASKS + NR_PROCS];
    clock_t stopwatch;
    do {
        // bad pointer
        dbgprt("pick_proc: proc_ptr=%p\n",proc_ptr);
        if (proc_ptr == NULL)
            break;

        // out of range process number
        dbgprt("pick_proc: p_nr=%u\n",proc_ptr->p_nr);
        if (proc_ptr->p_nr >= NR_PROCS)
            break;

        realtime = getuptime();
        dbgprt("pick_proc: realtime=%d\n",realtime);
        recent_time[proc_ptr->p_nr + NR_TASKS] = realtime - stopwatch;
        stopwatch = realtime;
    } while (0);

    for (q = 0; q < NR_SCHED_QUEUES; q++) {
        if ((rp = rdy_head[q]) != NIL_PROC) {
            next_ptr = rp;
            if (priv(rp)->s_flags & BILLABLE)
                bill_ptr = rp;
            return;
        }
    }
}

UPDATE:

I can't emphasize enough the importance of assert and/or if statements to pre-validate anything you do to prevent UB. And, debug printf, such as:

TRACE(VF_PICKPROC,printf("hello world\n"););

BTW, I pulled the minix source code from github:

git clone https://github.com/Stichting-MINIX-Research-Foundation/minix.git

It appears that you may have outdated source because the pick_proc in that repo has more SMP related code.

The NR_TASKS + NR_PROCS should be the time quanta taken from the process table. I orginally declared the Array and stopwatch in proc.h but the pic_proc function did not have access to them.

You really want to either use the existing fields in struct proc or add some of your own vs. creating a separate array [indexed by process number].

I added some screen shots of code that calls pic_proc. I think i need to modify sched and leave pic_proc alone. The array in pic_proc should be getting the time quanta from each process from the process table

Yes, hopefully elapsed CPU time for each process is in struct proc. If not, you'll have to add a field. I suspect that p_user_time [and/or p_sys_time] may be what you want. And, maybe, p_cpu_time_left. Other possibilities: p_accounting.time_in_queue. Or, p_cycles

pick_proc just looks at the head of a given run queue. So, you may need to change the code that actually inserts a given process into that queue. That might be enqueue and/or dequeue. They appear to honor process priority [p_priority] and preemption.

I've skimmed the kernel code, and at a guess, sched_proc may be of interest.

But, I really think you'll need to examine the kernel code more closely to see what functions actually add a process to a given run queue. And, how they select a process and from which queue.

When adding a process, it may just append to the tail. That code would need to scan the queue and [assuming the priority is the same], insert based on lowest CPU usage.

NB: In 2021 most linux systems have 8kb of default stack. (Which can be raised) — Antonin GAVREL, Mar 23 '21 at 23:06
@AntoninGAVREL Yes. I was doing it from memory [when it was 4KB]. But, I wanted to emphasize the limited nature of the resource. And, the fact OP's kernel "won't boot" would seem to indicate the code is called early in the boot process. — Craig Estey, Mar 23 '21 at 23:11
The NR_TASKS + NR_PROCS should be the time quanta taken from the process table. I orginally declared the Array and stopwatch in proc.h but the pic_proc function did not have access to them. — Warfujjii, Mar 24 '21 at 00:38
I added some screen shots of code that calls pic_proc. I think i need to modify sched and leave pic_proc alone. The array in pic_proc should be getting the time quanta from each process from the process table. — Warfujjii, Mar 24 '21 at 00:48

Modifying Minix 3 Scheduler

1 Answers1