-
Notifications
You must be signed in to change notification settings - Fork 167
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Improving spd
's worst case latency
#599
Comments
Over in #601 I've reworked the H7 I2C target code to be more robust, and it seems like it even worked. So I'm looking into the SPD EEPROM side of this proposal now. Currently, the spd task reads the EEPROMs once, at startup, using this routine. (The routine is called in a loop, but the loop exits once it succeeds; the comments mention that this is to handle racing the sequencer turning power on. That problem will be conveniently fixed by this proposal!) Two 256-byte pages are read and proxied, though they're read in different ways -- the first as a sequence of 1+255 byte reads, the second as a pair of 128+128 byte reads. From my previous experience with this style of EEPROMs that difference is probably not important. The output from the read phase consists of three pieces of information:
The remainder of the code is the actual proxy implementation, which shouldn't need to change that much. An issue I can foresee, though: the I2C target code assumes that it has complete control of the task and provides its own main loop. This makes it incompatible with tasks exposing an IPC interface. For the SPD task to receive EEPROM data from the sequencer as I've proposed, we will have to make two concessions:
This isn't great, in large part because any task so unfortunate as to send to Another option is that I rework the I2C target code to remove the "inversion of control" structure. This would be ... involved, because it's using that inversion-of-control thing to good effect: it implements a multi-part state machine using code, rather than an explicit data-driven state machine, which is nice. Inverting that would require moving to an explicit data-driven state machine built out of match statements, and that would be harder to follow. A final option is that I look into (ab)using the I2C target code's |
This will provide a channel for feeding EEPROM data into the cache, incrementally, per the design laid out in #599.
The sequencer task now reads the EEPROMs once it's confident they're available, and pushes the info into the SPD proxy before starting the host. This eliminates the last blocking I2C transfer from the SPD proxy, which should improve its liveness. It also eliminates the priority inversion between SPD (high) and `i2c_driver` (lower). Fixes #599.
This got fixed as a side effect of #1242. |
Currently we have a priority inversion in the
gimlet
images betweenspd
(the DIMM SPD proxy task) andi2c_driver
. This is becausespd
issues i2c transactions to read information from DIMMs, but also has strict latency requirements as a result of implementing an SMBus device, due to how the ST I2C controller works. We movedspd
above thei2c_driver
task because, sincei2c_driver
is monolithic (serves all controllers one at a time), putting it higher priority thanspd
meant that any i2c traffic anywhere on the system would blockspd
from running, making it very, very easy to miss its deadlines.The priority inversion creates a very real availability bug:
spd
starts a blocking call toi2c_driver
.i2c_driver
andspd
starts runningspd
is now out to lunch for an arbitrarily long period of time despite being higher priorityThe system does not currently enforce the uphill send rule that prevents these priority inversions, which is why you can build
gimlet
. But we intend to start, to avoid bugs like the one I just described.So, let's fix
spd
.I worked through a number of options on paper, and the one that seems like the best compromise between "reasonably low amount of work" and "not a total hack that will prevent me from sleeping" can be summarized as:
thermal
task (lower priority). Post the results to thesensors
task (high priority) like all other thermal sensors.gimlet_seq
task (lower priority), because it knows exactly when the window between "DIMM SPD power available" (A2) and "potentially racing the host" (A0) is, since it's responsible for sequencing it.spd
task via IPC.spd
to serve cached EEPROM contents (pre-prepared bygimlet_seq
before leaving A2), plus live temperature data taken from thesensors
task. This meansspd
never makes a blocking call to any task exceptsensors
, which is high priority and responds promptly.The relative priority of the affected tasks, and their IPC relationships, are shown in this diagram:
![PXL_20220610_175113553](https://user-images.githubusercontent.com/45247/173127300-e792be4f-8ae9-466b-954d-7b9ffd9239d6.PNG)
In descending order from highest to lowest priority we have
sensors
spd
i2c
thermal
/gimlet_seq
- my assumption is that, for other reasons,gimlet_seq
will wind up being lower priority thanthermal
, but for the purposes of fixingspd
they are partially ordered and their relative priority doesn't matter.This proposal would fix #562, #545, and the thing people have run into (but for which I cannot find a filed issue) where the host will lock up on boot intermittently despite known bad actors like
humility dashboard
not being used.The text was updated successfully, but these errors were encountered: