Discussion on interrupts during startup

[Mellvik]

@ghaerr, this issue is opened to continue the discussion from ghaerr/elks#2441 (comment)

If interrupts are supposed to be off during kernel initialization, why keep the irq off/on sequences in the driver drv_init code?

Yes, exactly. The problem was that before PR ghaerr/elks#2355 to ensure correctness in kernel startup, one could not be absolutely sure of the CPU interrupt state, both when they were required to be off, and then later, when they are required to be on (for instance, in the IDE probe routine where precision timing requires an operating jiffies timer).

PR ghaerr/elks#2355 ensures startup correctness, with changes so that interrupts remain disabled until after all character device initialization, and then enabled before any block device initialization:

void INITPROC device_init(void)
{
    chr_dev_init();
    
    set_irq();          /* interrupts enabled for possible disk I/O or timers */

    blk_dev_init();
...

Given that we now know that interrupts are off during char device initialization, why not remove all the save_flags/clr_irq/restore_flags sequences from the chr driver initialization code?

Also, why not keep interrupts off during block device initialization too. As far as I can tell there are no block device drivers that need interrupts enabled during initialization. An ISA probe doesn't need a precision timer.

After kernel initialization, there are a few rules that can be followed to ensure the system will operate correctly when writing device drivers or interrupt handlers:

Interrupt handlers are always called with interrupts enabled (although the PIC may have less-priority hardware interrupts disabled). Critical sections that require the use of disabled interrupts should be very short. All interrupt handlers should save the original interrupt state using save_flags() / restore_flags, since the handler may itself be interrupted by another interrupt handler.

OK, I'm trying to envision this.

• Interrupt handler A is running, and has not touched the IRQ enable bit so interrupts are on.
• Interrupt handler B is interrupting A because it has higher priority.
• B has a critical section and turns off interrupts. All interrupts are blocked except NMI which does not affect the IRQ enable bit.
• When B's critical section ends, it can safely call set_irq since it knows interrupts were enabled when it got control and it knows no one could have interrupted I for the duration of the critical section.
• If A was in a critical section (had called clr_irq) at the time B's interrupt came, B's interrupt would be blocked until A called set_irq.

So what I'm missing here is at what point or in what situation the state of the interrupt enable bit is unknown in an interrupt handler, qualifying the insurance provided by save_flags/restore_flags.

Non-interrupt handler kernel routines can disable interrupts for short periods, but the interrupt state should be handled the same way, since although the kernel is not reentrant, hardware interrupts can interrupt kernel code execution, and certain kernel routines can be called from others with interrupts required to be disabled the entire time.

Yes, this is a different setting since the IRQ state is unknown at the time of entry.

Upon any return by the kernel to user mode code, or interrupt handler entry, it is guaranteed that interrupts will be enabled. This means that a mistake saving a previous interrupt state above by leaving interrupt disabled will only cost a max (possibly recurring) 10ms until the next clock interrupt. Enabling interrupts too early will just defeat the purpose of the critical section, since interrupt handlers all run with interrupts enabled.

Agreed.

Therefore, the quick rule now is that all character and network device driver init routines can assume interrupts are disabled, whereas block device drivers and all other critical sections must save/restore interrupt state.

Like I said above, I'm not seeing the need for interrupts in block device init either... It would be simpler (and save code) to just leave them off.

NOTE however that shared routines, (such as dma_read in NE2K driver) should save/restore interrupt state since they're used at both init and run time. Always saving and restoring state ensures that the routine can be reimagined elsewhere without having to be concerned about interrupt status issues, and costs very little (see below).

A very pertinent example, @ghaerr. I want to take a close look at whether/which of these 'critical sections' are actually 'critical'. My suspicion is that some parts are overly protected (experiments that never ended) and could safely run with interrupts enabled. I'll be back on that.

Possibly a chance to do some cleanup, which I intend to do.
Further, there are experimental IRQ disable/enable pairs in most drivers. It's time to find out what's required and what's not.

Great, no problem - happy to wait while you do the cleanup in TLVC. If in doubt, it can never hurt to save/restore interrupts rather than cli/sti, the extra cost is only a single PUSHF for ASM routines and a PUSHF/POPF n(%bp) for C routines using save_flags().

Right! Good.

In order to check that all NIC drivers leave interrupts disabled through the init sequence, a quick check just in front of set_irq() can be made after chr_dev_init():

    flag_t flags;
    save_flags(flags);
    if (flags & 0x0200) printk("ERROR INTS ENABLED\n");
    set_irq();          /* interrupts enabled for possible disk I/O or timers */

Thanks, yes, this is good!

I compared the entire drivers/net and elkscmd/ktcp directories yesterday between TLVC and ELKS, and there's quite a few mods. In addition, there was one ELKS ktcp corruption fix ghaerr/elks#2267 and duplicate assignment ghaerr/elks@bc325ce that I haven't yet checked whether TLVC has fixed. There are some cases of lots of commented out debug statements in some files - not sure whether you plan on leaving those in longer term or not.

Appreciated.

[ghaerr]

why not keep interrupts off during block device initialization too. As far as I can tell there are no block device drivers that need interrupts enabled during initialization.

I have deep thoughts on this, bear with me.

I like to think of this as "evolution", not "revolution". At its core, it is the responsibility of an OS to correctly execute the intentions of its user. The way this is usually done at the micro level is to extremely precisely control the order of execution of machine instructions. In a real sense, that's the whole point of the multi-tasking - executing lots of wildly different tasks in an extremely orderly manner. To be correct, the code must first calculate correctly, and then code must be forced to execute in a correct order, using context switches, stacks, etc to help. Also, after a programmer writes code, the order of actual execution of that code may not in fact be the order that programmer was imagining. This is an important point too.

What I'm doing with ELKS with regards to the above is cleanup (for five years!!): it must be known what context any and all kernel code may be executed in - until just recently, it wasn't in fact entirely clear what state interrupts were in, nor what state they should be in, during kernel startup. In general, the idea isn't to force interrupts to be disabled for any longer a time than necessary (by requiring all drivers to execute with interrupts off, for example), as this prohibits what programmers can accomplish - see below.

As far as I can tell there are no block device drivers that need interrupts enabled during initialization.

[EDIT: the comment below refers to execution that actually occurs at open time, not init time. A block device open routine may be called within mount_root(). But the ELKS SSD and ATA drivers use jiffies timers that need to be operational at init time. So you're correct - but only for TLVC. Also the direct floppy driver may (need to check) perform a probe at init time in order to determine disc format type. This requires the interrupt system enabled. All-in-all, one should not have to worry about restricting normal device driver operations too much within device driver init routines, since this ultimate decreases system flexibility for the user, and complicates the startup ordering of device drivers. None of this needs to be that way.]

That's incorrect, and is a good example of a assuming a condition that comes back to bite... I had originally envisioned the simple plan of disabling interrupts throughout the entire boot process - only to find out that the partition reading code relies on fully-running system buffers and any possibly-associated block device, just to read the MBR. That won't work with interrupts disabled.

I then came to realize that interrupt had to be on during block device initialization, since the BIOSHD, ATA, (and your direct HD) driver depend on the ability to read the MBR and possibly requiring a fully interrupt-driven environment. So the "kernel startup" essentially has to be pretty much over by the time block device initialization comes around. [EDIT: while this last comment refers to execution that actually occurs after "open" instead of "init", all the same drivers now use jiffies timers for correctness, which has been proved a necessity. So we're back to splitting hairs on what can be done in which driver function, rather than just designing a system where any driver can depend on a fully functional interrupt and buffer subsystem and use any kernel facility at init, open or later time.]

I then dove in and saw that in fact various char drivers were both disabling and enabling interrupts without regards to saved state, etc, and fixed all that. Now, ELKS has evolved so that we're absolutely assured that interrupts are off and have remained off up until the block device init.

Given that we now know that interrupts are off during char device initialization, why not remove all the save_flags/clr_irq/restore_flags sequences from the chr driver initialization code?

Because the next evolution may be to allow interrupts in char device initialization, and we don't want to go having to figure out those critical sections all over again. The point is that programmers should not depend on whether interrupts are permanently on or off in device drivers - their requirements change too rapidly, compared to the kernel proper.

Currently, ELKS has interrupts disabled though char device init, but I plan on looking further into that, to explore the option of giving device driver writers more flexibility when they need it. See below.

An ISA probe doesn't need a precision timer.

Completely disagree here. Writing anything other than a jiffies-based timer introduces the possibility of incorrectness. Sure, we think, oh, we don't need that now - until a super high speed emulator comes along and executes at 1B instructions/second and the crappy busy-loop timers fail. Why not just write it correctly in the first place, we have an accurate 10ms timer, why not use it?

So that's exactly what I did - all timers were rewritten to use jiffies - the serial drivers, IDE probing, ATA delays, etc. BTW, this was another reason why the interrupt subsystem had to be fully functional and interrupts enabled before block device init - who wants to go rewrite all the block driver to use crap busy loops just because we thought it would be better for the whole system to have interrupts disabled throughout its entire startup?

[EDIT: I'm so glad we're having this discussion. I now see from my own comment that the serial driver which depends on jiffies for timing out on TBE won't work (this occurs for PC98 serial driver only) since interrupts are disabled! I've added this to my bug list and now consider it higher priority to determining whether interrupts can be enabled during char device initialization. This should be possible since the timer system is operational, but the drivers will still be restricted on being able to use other potential kernel resources. A list needs to be made.]

[EDIT: BTW, and I mention this because I know you're planning on it - adding interrupt driven transmit to the serial driver is going to complicate the problem of displaying post-early but pre-block-init kernel boot messages, because the serial driver won't have interrupts enabled until block device init time. So it's definitely worth getting even more organized around allowing char device initialization to operate without having to have interrupts disabled. For the NIC drivers, I would suggest worrying less about whether cli/sti should be replaced by save_flags/restore_flags, and more about which routines might be affected should the NIC driver enable interrupts on the hardware with CPU interrupts enabled, and what happens when a packet is received before driver open(). If this can't be sorted out, the interrupts will have to remain disabled and interrupt-driven serial output will likely have to wait.]

So that's where I'm coming from. All code should maintain interrupt state. No code should use busy loops (they're really only needed on certain SMP multiprocessor systems anyways). In this manner, I try to write correct code once, the first time, and have it run as long as possible without modification.

An ISA probe doesn't need a precision timer.

Another benefit of using a "standard" approach (whatever that might look like) of copying jiffies(), then using time_after(jiffies(), ...) is that it's easy to spot timer loops in all kernel code, and then update them all at once should another method be better suited. ideally, I'd like all of them to be encapsulated in a single function call. I haven't looked into that yet, but I've just added that to my list.

I'll comment on the remainder of your post separately. Thanks!

[ghaerr]

So what I'm missing here is at what point or in what situation the state of the interrupt enable bit is unknown in an interrupt handler,

When B's critical section ends, it can safely call set_irq since it knows interrupts were enabled when it got control and it knows no one could have interrupted I for the duration of the critical section.

Well yes - B could just call set_irq(). But that changes if/when the routine is reused. When the system is coded such that the programmer assumes the routine will ONLY be called with interrupts enabled, then that code fail when the same routine is called, (usually much later by another programmer not thinking about it), calls the routine from another driver, situation, etc.

Even the "assumption" that interrupt handlers are ALWAYS called with interrupts enabled is risky - because that means all such code has to be rewritten should some case arise where that is now longer true. For instance, sharing a driver with a real-time enhancement in ELKS. That handler is now called with interrupts disabled in order to force real-time constraints, etc.

So, yes - one could write all code in simple fashion cli/sti around everything, and then just be "on the lookout" to change it to be correct when a system assumption is made years later. IMO, except for the most important kernel time-sensitive routines, the +2 instruction overhead to write code such that it always works is superior.

What I'm doing in ELKS is to take the high road, especially in device drivers, since they're subject to change by more programmers with other things on their mind. For the cost of +2 instructions, it's a no-brainer for me.

BTW, the business of estimating time "lost" for executing +2 instructions in a driver brings up our conversation about the cost of LCALL vs CALL for driver function calls. I reason that, at least for ethernet and block device drivers, (and filesystem drivers), the cost of a couple CPU cycles is FAR less than the hugely larger time spent copying 1024 bytes into a buffer, or receiving 1500 bytes from a NIC. That time greatly outweighs any speed losses from FAR functions, except in the most time-critical situations. And with both our kernels basically completely out of near code space, that's also a no-brainer for me.

I want to take a close look at whether/which of these 'critical sections' are actually 'critical'. My suspicion is that some parts are overly protected (experiments that never ended) and could safely run with interrupts enabled. I'll be back on that.

Great idea! It's well worth considering where critical sections are truly required. I don't use them for insurance, instead take a hard deep-dive about how the device functions and then code to that. With heavy testing, lack of critical sections will show up, just like it did with the amazing results of your jiftest program showed!

[ghaerr]

After taking a close look at my comments above and the later edits, I'm going to look hard at what needs to be done to enable interrupts on most, if not all, char device drivers. I got bitten by the same bug I was talking about - writing code making assumptions about certain kernel resource availability only to find it false - for the PC98 serial driver!! (Looking back, the reason this was added was that if the PC98 is booted with serial console but no serial card, the non-existant UART TBE never becomes ready, and thus the kernel hangs. Probably the wrong solution for this problem. You may recall that ELKS and TLVC have a capability to perform serial output before the serial_open routine, to allow display of startup progress. It's all complicated, isn't it?).

I think allowing interrupts through both char and block device init routines, but creating a list of disallowed functions (e.g. certainly wait/schedule routines cannot be called) would prove very helpful. I'm starting a vacation Wednesday but will be thinking more about this when I can.

[ghaerr]

More thoughts and information on enabling interrupts before char device init routines are called.

The chr_dev_init() is called just before interrupts are enabled, and blk_dev_init() is called in device_init():

void INITPROC chr_dev_init(void)
{
#ifdef CONFIG_CHAR_DEV_LP
    lp_init();
#endif

#ifdef CONFIG_CHAR_DEV_CGATEXT
    cgatext_init();
#endif

#ifdef CONFIG_CHAR_DEV_MEM
    mem_dev_init();
#endif

#ifdef CONFIG_INET
    tcpdev_init();
#endif

#ifdef CONFIG_ETH
    eth_init();
#endif

#ifdef CONFIG_PSEUDO_TTY
    pty_init();
#endif
}

None of these routines now, other than probably eth_init() need interrupts disabled. And eth_init() could be moved to the first function called, and then interrupts enabled. But I'm thinking a better approach would be to enable interrupts first thing in device_init():

void INITPROC device_init(void)
{
    chr_dev_init();

    set_irq(); /* interrupts enabled for possible disk I/O or timers */ // <--move up one line or into main.c

    blk_dev_init();
...

Then go through all NIC init routines assuming interrupts are enabled, and only disabling them when absolutely required, not using cli/sti, but save/restore. Those init routines would, (somewhat contradicting an earlier statement) NOT enable hardware interrupts on the NIC until open time. In this way, there would never be any packet reception or interrupts during init, all real NIC activity would only be enabled at open time. Would this be possible? I don't know the NIC drivers well enough to answer that.

At eth_init() time, the only job should be NIC identification and probing, and that only for the purposes of displaying device information to the user on the boot screen. Any actual operation can fully wait until the device is opened. Hopefully no probing requires interrupt activity (after all, they're disabled now anyways), and assuming interrupts are on would necessitate save/restore around critical sections, which I've recommended anyways. This way, we could allow jiffies timers etc in NIC probe routines but still protect the system against unwanted ethernet packet activity since NIC interrupts would always be controlled by the open/close routines.

If this sounds possible, then you could easily try it out by enabling interrupts prior to eth_init and testing your drivers, which need to be done anyways for your planned critical section research.

BTW, with this design, a programmer could use cli/sti and it would work, since interrupts were previously enabled, even though it could be called bad programming practice because the routines are less portable when shared. So overall, this design improves stability, allowing device driver programmers to make mistakes that don't actually affect system operation (at least at first lol).

That leaves the problem of serial I/O and interrupts. Serial_init() is called very early in kernel boot, separately from all other char device drivers. This was likely done because of the previously mentioned problem of rs_conout() being called by printk when serial console is enabled, way before device rs_open may be called. Serial_init() is called prior to inode_init and buffer_init, so we're very early to be running jiffies timers and interrupts enabled. I think serial_init is also called to set the baud rate before any console out is attempted, and that's then when kputc is redirected with the set_console() call.

I will have to think about the special case of serial char devices separately, while also avoiding a non-jiffies timer kluge that I've been pushing so far to avoid. The good news is the issue is only present in PC98, and could be easily handled by dropping all character output until rs_open is called. Its messy because rs_open is never called even when serial console is configured, so we're really dealing with a potential bad design issue that needs to be rectified... but lets let that wait for another day for now.

What do you think? Does the idea of enabling interrupts before eth_init() sound plausible? Are there any routines that really need interrupts disabled that can't be protected with save/restore critical section code?

[ghaerr]

Off-topic, perhaps another issue should be opened to talk about specific NIC driver issues; let me know.

I code-reviewed the wd.c driver from the standpoint of cleaning up interrupts, init, and open code - it looks pretty good, also much simpler/cleaner than some other NIC drivers, it seems. I came across the following code which I don't understand:

/* Using this wrapper saves 44 bytes of RAM */
/* Using word transfers when possible improves transfer time ~10%
 * on large packets (measured @ 1200 bytes) */
static void NICPROC fmemcpy(void *dst_off, seg_t dst_seg, void *src_off, seg_t src_seg,
        size_t count, int type) {

        if (type == is_8bit)
                fmemcpyb(dst_off, dst_seg, src_off, src_seg, count);
        else    
                fmemcpyw(dst_off, dst_seg, src_off, src_seg, (count+1)>>1);
}

The issue is the if (type = is_8bit) statement in this static function. In every case where fmemcpy is called within this file, it's passed the is_8bit global, and then, inside this function, that parameter is then compared again to is_8bit. It would seem to me that that the if statement would always be true, and that fmemcpyw could never get executed. Am I missing something here?

Another point would be, if a global is being passed with every call as a parameter, and that parameter is always compared to a global, then why pass the global as a parameter in the first place?

Another issue: there seems to me no reason to increment count in fmemcpyw, wouldn't it always work with just count >> 1? That is, is there ever a case where the count value is odd? And if so, wouldn't the fmemcpyw in that case copy one byte of uninitialized data into the destination buffer, thus introducing undefined behavior or worst-case, buffer overrun?

[Mellvik]

@ghaerr, you certainly go all in when you do something, I appreciate that. Your rundown on interrupts and related issues above appear more like a strategy than anything else, which makes it even better for a serious and fruitful (not to mention useful) discussion. It will take some time for me to digest that, it forces me to finally organize and add missing pieces to my strategy for TLVC. Again, appreciated.

Off-topic, perhaps another issue should be opened to talk about specific NIC driver issues; let me know.

I code-reviewed the wd.c driver from the standpoint of cleaning up interrupts, init, and open code - it looks pretty good, also much simpler/cleaner than some other NIC drivers, it seems. I came across the following code which I don't understand:

/* Using this wrapper saves 44 bytes of RAM */
/* Using word transfers when possible improves transfer time ~10%
 * on large packets (measured @ 1200 bytes) */
static void NICPROC fmemcpy(void *dst_off, seg_t dst_seg, void *src_off, seg_t src_seg,
        size_t count, int type) {

        if (type == is_8bit)
                fmemcpyb(dst_off, dst_seg, src_off, src_seg, count);
        else    
                fmemcpyw(dst_off, dst_seg, src_off, src_seg, (count+1)>>1);
}

The issue is the if (type = is_8bit) statement in this static function. In every case where fmemcpy is called within this file, it's passed the is_8bit global, and then, inside this function, that parameter is then compared again to is_8bit. It would seem to me that that the if statement would always be true, and that fmemcpyw could never get executed. Am I missing something here?

Thanks for the code review - and the one missing something here is me. It's a leftover from before is_8bit was changed into a global and obviously a bug. That requires me to do a full regression test with my 4 wd NICs, which with the amount of daily screentime these days will take some time. That said, just as your message arrived I was actually looking at that same code, the probe section, that is (again). As I - for good measure - tested the latest addition to my NIC collection on the XT, I noticed that while working fine, the boot-message was not what I expected. And it turns out that the test for system bus size, a remnant from the big linux version of the driver, does not work with this NIC. I suspect that this has actually never been tested on big Linux as it never ran on 8bit ISA anyway. So I'm reverting to checking processor type to establish bus size, which seems to be the only reliable way. I'm also experimenting with using word moves regardless of bus, which should work - and thus eliminate this ordeal.

Another issue: there seems to me no reason to increment count in fmemcpyw, wouldn't it always work with just count >> 1? That is, is there ever a case where the count value is odd? And if so, wouldn't the fmemcpyw in that case copy one byte of uninitialized data into the destination buffer, thus introducing undefined behavior or worst-case, buffer overrun?

This is insurance really. Theoretically a packet may be odd sized, and the extra byte will be ignored since processing down the line will use the packet size.

[ghaerr]

Sounds good @Mellvik. You don't need to do full regression tests, I'm just pointing out potential issues I ran into looking through the code. Go ahead and put in INITPROC with a NIC you have installed, that'll help TLVC and ELKS. I'm happy to continue any code reviews on your posted PRs if you would like. I'm looking forward to getting your enhanced drivers added to ELKS to complement all the ktcp/ftp/ftpd code that is now added to ELKS from TLVC.

[Mellvik]

Thanks @ghaerr, your eyes in on the code are always useful. Appreciated.

Regression test: The bug you discovered is quite serious in that is has forced all memory transfers to be byte sized. This means that the testing - since the bug was introduced - involving mixing system bus width and card bus width are invalid and results possibly wrong. Given that different NICs - from experience - behave differently in their handling of 8 vs 16 bits, giving the fix as full round seems required.

PR pending.

[Mellvik]

[Still on the wd driver off-topic] This seemingly small endeavor sent me stumbling into some new territory @ghaerr . Like suddenly discovering that the ISA bus cannot reliably handle a mix of 8 and 16 bit cards using shared memory if that shared memory is within the same 128k segment. Took me a while to figure that one out - and it opened up a couple of possibly related issues, one being a seemingly random boot problem (code 4, wrong kernel signature).
I'm mentioning this because until I figure it out, delaying a potential port of the unlimited bootopts is a good idea. Not likely related, but not impossible either.

[ghaerr]

Like suddenly discovering that the ISA bus cannot reliably handle a mix of 8 and 16 bit cards using shared memory if that shared memory is within the same 128k segment.

Geez, but nice catch! Sounds like this is a hardware limiting situation that can't be worked around by software, correct? So one must watch which type cards are installed and where their memory addresses are set to?

one being a seemingly random boot problem (code 4, wrong kernel signature).

delaying a potential port of the unlimited bootopts is a good idea.

I remember having some thoughts about potential issues with the TLVC unlimited /bootopts design, but can't remember exactly what they were. I don't this was likely the issue, but will have more time to look into this after I finish with my current traveling. Thanks for the heads up!

[Mellvik]

@ghaerr, while still having limited screen time I'm closely following your evolving and very interesting work on interrupt structure and usage in ELKS. As pointed out in a different thread, I think this is very important in every way. I'll be back on that in different threads as I catch up.

The purpose of this thread seems to end naturally with your Early Interrupts PR (ghaerr/elks#2501 (comment)) which I'm catching ups with as we speak. There are open questions from this thread - both ways, and they will pop up as we progress further.

The wd driver issues were eventually resolved if not really solved (PR coming - with more info). It turns out that modern day BIOSes (modern being the late 90s systems still supporting ISA) prevent old ISA8 shared memory cards from working. There is just too much 'helpful functionality' in these BIOSes - shadow memory mapping, UMB availability, PnP probing/mapping etc etc. My long hunt for the root cause ended when I pulled out the Compaq 286 and everything worked just fine. No big deal - NICs that old don't support twisted pair/RJ45 Ethernet anyway. So adding a line in the man page is probably the right thing to do.