M-Labs/artiq-zynq
10
1
Fork 19
Code Issues 12 Pull Requests Releases Wiki Activity

idle kernel changes: run on startup, flashing new kernel after startup #276

Merged
sb10q merged 2 commits from srenblad/artiq-zynq:idle_kernel_rework_simple into master 2024-08-05 19:28:10 +08:00
Conversation 5 Commits 2 Files Changed 1 +44 -17
srenblad commented 2023-11-01 10:41:36 +08:00
Contributor
Copy Link

idle kernel changes: run on startup, flashing new kernel after startup

Relevant issue: https://github.com/m-labs/artiq/issues/2246

The idle kernel does not run automatically after booting after startup kernel has exited.

Proposed fix:

  • Start an additional task before entering main loop, terminate on first experiment connection.

Testing

  • Smoke tested on Kasli-SoC v1.1 standalone variant
  1. Flashed and power-cycled board.
  2. Idle kernel starts after startup kernel finishes (no startup kernel in this test)
  3. Run experiment.
  4. Idle kernel runs after experiment.
### idle kernel changes: run on startup, flashing new kernel after startup Relevant issue: https://github.com/m-labs/artiq/issues/2246 > The idle kernel does not run automatically after booting after startup kernel has exited. Proposed fix: + Start an additional task before entering main loop, terminate on first experiment connection. #### Testing + [x] Smoke tested on Kasli-SoC v1.1 standalone variant 1. Flashed and power-cycled board. 2. Idle kernel starts after startup kernel finishes (no startup kernel in this test) 3. Run experiment. 4. Idle kernel runs after experiment.
esavkin reviewed 2023-11-01 15:56:58 +08:00
src/runtime/src/comms.rs Outdated
@ -727,0 +733,4 @@
let up_destinations = up_destinations.clone();
let aux_mutex = aux_mutex.clone();
let routing_table = drtio_routing_table.clone();
async move {
esavkin commented 2023-11-01 15:56:58 +08:00
Owner
Copy Link

isn't there a way to reuse the idle kernel start-up code?

isn't there a way to reuse the idle kernel start-up code?
srenblad commented 2023-11-02 10:12:52 +08:00
Author
Contributor
Copy Link

there might be, let me have a look!

there might be, let me have a look!
srenblad marked this conversation as resolved
srenblad force-pushed idle_kernel_rework_simple from d77f141c3c to 1ab13f5799 2023-11-02 12:21:35 +08:00 Compare
srenblad commented 2023-11-02 12:32:07 +08:00
Author
Contributor
Copy Link

@esavkin see latest push reusing startup and idle kernel code.

@esavkin see latest push reusing startup and idle kernel code.
esavkin commented 2023-11-02 12:38:37 +08:00
Owner
Copy Link

@esavkin see latest push reusing startup and idle kernel code.

Looks fine to me, except the wording. The new function is not exactly flashing. I would say it better be some verb, that would represent something like starting the kernel from config.
Let's wait for other reviews.

> @esavkin see latest push reusing startup and idle kernel code. Looks fine to me, except the wording. The new function is not exactly flashing. I would say it better be some verb, that would represent something like `starting the kernel from config`. Let's wait for other reviews.
srenblad changed title from runtime.comms.rs: run idle kernel on start-up to idle kernel changes: run on startup, flashing new kernel after startup 2023-11-02 13:01:31 +08:00
srenblad commented 2023-11-02 13:07:46 +08:00
Author
Contributor
Copy Link

Looks fine to me, except the wording. The new function is not exactly flashing.

I don't think the flash_kernel_worker in session.rs does either. If I was to guess, I would say the original naming is based on the fact that it loads from the flash (ie config) not that it flashes. host_kernel_worker does the same thing but for experiment runs.

> Looks fine to me, except the wording. The new function is not exactly flashing. I don't think the `flash_kernel_worker` in session.rs does either. If I was to guess, I would say the original naming is based on the fact that it loads from the flash (ie config) not that it flashes. `host_kernel_worker` does the same thing but for experiment runs.
sb10q reviewed 2023-11-07 13:50:07 +08:00
src/runtime/src/comms.rs Outdated
@ -641,0 +652,4 @@
info!("Loading {}", config_key);
let _ = load_kernel(&buffer, &control, None)
.await.map_err(|_| warn!("Error loading {}", config_key));
info!("Running idle kernel");
sb10q commented 2023-11-07 13:50:02 +08:00
Owner
Copy Link

It's not always the idle.

It's not always the idle.
👍 1
srenblad marked this conversation as resolved
srenblad changed title from idle kernel changes: run on startup, flashing new kernel after startup to WIP: idle kernel changes: run on startup, flashing new kernel after startup 2024-03-01 14:44:34 +08:00
srenblad force-pushed idle_kernel_rework_simple from 1ab13f5799 to ff2e95c35e 2024-03-06 11:10:44 +08:00 Compare
srenblad force-pushed idle_kernel_rework_simple from ff2e95c35e to 16b0d258c5 2024-03-06 12:03:39 +08:00 Compare
srenblad changed title from WIP: idle kernel changes: run on startup, flashing new kernel after startup to idle kernel changes: run on startup, flashing new kernel after startup 2024-03-06 15:11:37 +08:00
sb10q reviewed 2024-07-31 17:29:05 +08:00
src/runtime/src/comms.rs Outdated
@ -721,0 +727,4 @@
timer: GlobalTimer,
) {
info!("Loading idle kernel");
let res = handle_flash_kernel(buffer, control, up_destinations, aux_mutex, routing_table, timer)
sb10q commented 2024-07-31 17:29:04 +08:00
Owner
Copy Link

formatting

formatting
srenblad marked this conversation as resolved
sb10q reviewed 2024-07-31 17:29:37 +08:00
src/runtime/src/comms.rs Outdated
@ -721,0 +730,4 @@
let res = handle_flash_kernel(buffer, control, up_destinations, aux_mutex, routing_table, timer)
.await;
match res {
#[cfg(has_drtio)]
sb10q commented 2024-07-31 17:29:35 +08:00
Owner
Copy Link

Why does this need to be aware of DRTIO?

DRTIO link status is to be handled manually by the user in idle kernels

Why does this need to be aware of DRTIO? DRTIO link status is to be handled manually by the user in idle kernels
srenblad commented 2024-07-31 17:54:47 +08:00
Author
Contributor
Copy Link

Added in #279. Inspecting handle_flash_kernel reveals this Error indeed does not occur for idle kernels.

@mwojcik safe to remove?

Added in #279. Inspecting `handle_flash_kernel` reveals this Error indeed does not occur for idle kernels. @mwojcik safe to remove?
mwojcik commented 2024-07-31 17:57:25 +08:00
Owner
Copy Link

It was there to ensure that DRTIO was fully set up before a startup kernel would run. It could take a considerable while for all required destinations to start. With an idle kernel that can run on startup as well, I would recommend keeping it.

It was there to ensure that DRTIO was fully set up before a startup kernel would run. It could take a considerable while for all required destinations to start. With an idle kernel that can run on startup as well, I would recommend keeping it.
sb10q commented 2024-07-31 17:58:12 +08:00
Owner
Copy Link

This is for the user to handle. That's why we have an API for it. The behavior hardcoded here of waiting for 1 second is messy and not applicable to all use cases.

This is for the user to handle. That's why we have an API for it. The behavior hardcoded here of waiting for 1 second is messy and not applicable to all use cases.
👍 1
mwojcik commented 2024-07-31 18:03:22 +08:00
Owner
Copy Link

Since all subkernels are distributed before the kernel starts (to reduce load and latency when the kernels are actually running), that doesn't give the user an opportunity to act accordingly. However, since idle kernels will be retried and restarted until they can finally run, this check could be removed here.

Since all subkernels are distributed before the kernel starts (to reduce load and latency when the kernels are actually running), that doesn't give the user an opportunity to act accordingly. However, since idle kernels will be retried and restarted until they can finally run, this check could be removed here.
sb10q commented 2024-08-01 07:08:41 +08:00
Owner
Copy Link

Idle subkernels and startup subkernels need to be flashed into their respective satellites. I think I mentioned this already.

Idle subkernels and startup subkernels need to be flashed into their respective satellites. I think I mentioned this already.
mwojcik commented 2024-08-01 14:01:53 +08:00
Owner
Copy Link

Oh then it would make perfect sense, but that's feasible only after artiq#2498 is implemented. So to get this PR merged before that, let's remove that part.

Oh then it would make perfect sense, but that's feasible only after [artiq#2498](https://github.com/m-labs/artiq/issues/2498) is implemented. So to get this PR merged before that, let's remove that part.
srenblad force-pushed idle_kernel_rework_simple from 16b0d258c5 to 456935c9aa 2024-08-01 17:06:18 +08:00 Compare
srenblad changed title from idle kernel changes: run on startup, flashing new kernel after startup to WIP: idle kernel changes: run on startup, flashing new kernel after startup 2024-08-01 17:28:05 +08:00
srenblad commented 2024-08-01 17:28:31 +08:00
Author
Contributor
Copy Link

Did not pass testing after changes, marking as WIP until resolved.

Did not pass testing after changes, marking as WIP until resolved.
srenblad changed title from WIP: idle kernel changes: run on startup, flashing new kernel after startup to idle kernel changes: run on startup, flashing new kernel after startup 2024-08-02 11:15:44 +08:00
srenblad commented 2024-08-02 11:15:47 +08:00
Author
Contributor
Copy Link

Did not pass testing after changes, marking as WIP until resolved.

It was a user issue on my end and not a fault of firmware. Successfully tested blinky and idle kernel runs before and after experiment kernels. Ready to merge, assuming no further comments.

> Did not pass testing after changes, marking as WIP until resolved. It was a user issue on my end and not a fault of firmware. Successfully tested blinky and idle kernel runs before and after experiment kernels. Ready to merge, assuming no further comments.
sb10q merged commit fad1db9796 into master 2024-08-05 19:28:10 +08:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
priority:critical

   
priority:high

   
priority:low

   
priority:medium
No Label
priority:critical
priority:high
priority:low
priority:medium
Milestone
Clear milestone
No items
No Milestone
Assignees
Clear assignees
No Assignees
4 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: M-Labs/artiq-zynq#276
No description provided.
Delete Branch "srenblad/artiq-zynq:idle_kernel_rework_simple"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?