Skip to content
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

[PoC]: Yield-WaitFor-NoCallback #345

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from
Draft

Conversation

ppannuto
Copy link
Member

@ppannuto ppannuto commented Sep 8, 2023

This is the companion PR for tock/tock#3577.

I just did console to start. The only thing using the {put|get}nstr_async methods were the test programs testing the async. Everything else used the synchronous interface, so I just removed the async.

The code complexity reduction is significant, which is also reflected in the size savings.

For tests/console, size before:

Application size report for arch family cortex-m:
   text	   data	    bss	    dec	    hex	filename
   6132	    292	   2400	   8824	   2278	build/cortex-m0/cortex-m0.elf
   5616	    292	   2400	   8308	   2074	build/cortex-m3/cortex-m3.elf
   5616	    292	   2400	   8308	   2074	build/cortex-m4/cortex-m4.elf
   5616	    292	   2400	   8308	   2074	build/cortex-m7/cortex-m7.elf
  22980	   1168	   9600	  33748	   83d4	(TOTALS)

tests/console size after:
Application size report for arch family cortex-m:

   text	   data	    bss	    dec	    hex	filename
   5628	    264	   2392	   8284	   205c	build/cortex-m0/cortex-m0.elf
   5160	    264	   2392	   7816	   1e88	build/cortex-m3/cortex-m3.elf
   5160	    264	   2392	   7816	   1e88	build/cortex-m4/cortex-m4.elf
   5160	    264	   2392	   7816	   1e88	build/cortex-m7/cortex-m7.elf
  21108	   1056	   9568	  31732	   7bf4	(TOTALS)

Removing the async console implementation saves 400~500 bytes.


Caveat: Compile-tested only.

@bradjc bradjc force-pushed the yield-waitfor-nocallback branch from 76ae950 to 48d5105 Compare June 14, 2024 21:08
@bradjc
Copy link
Contributor

bradjc commented Jun 16, 2024

Unfortunately we did not include this when working on the libtock-c rewrite. If anyone has ideas on how this support should be structured please suggest.

@alevy
Copy link
Member

alevy commented Jun 16, 2024

For integrating this once tock/tock#3577 is merged, I think a good first step would be to simply use WaitFor always in contexts where we are confident it's virtually always the best thing to do. printf, in particular seems to be that.

In general, perhaps migrating entirely away from userspace yield_for to kernel-mediated WaitFor might be the best default bet, and that we now have a nice separate of the _sync library, that might be pretty straight forward.

@alevy
Copy link
Member

alevy commented Jun 18, 2024

Can we convert this to a regular PR for using yield_wait_for in console?

@bradjc
Copy link
Contributor

bradjc commented Jun 19, 2024

Can we convert this to a regular PR for using yield_wait_for in console?

I'm concerned about repeating the original libtock-c mistakes, ie everyone implements drivers with yield_wait_for in whatever way they see fit and it varies driver to driver. My patch for console uses the #defs in libtock/syscalls directly in libtock-sync, plus just a constant. Additionally, this duplicates the logic for determining what the three upcall values actually mean. I think we should at least try to think of a consistent way to use yield_wait_for we can add to the programming guide.

@alevy
Copy link
Member

alevy commented Jun 19, 2024

For alarm, I just did (I believe) the same thing. I also had to duplicate the ms-to-ticks conversion, which is suboptimal (probably just renaming it to match the module naming scheme and exposing it from libtock makes sense).

I think using the DRIVER definition in libtock + a constant for the subscribe number is OK, though might be better to name subscribes in libtock and use those constants in libtock_sync.

I agree that duplicating the logic to determine what each return/argument value means is unfortunately. You could imagine having a three-register-size struct that is driver specific and used in both the return type of yield_wait_for and as the first argument for a subscribe callback, though we need each driver to be careful that the mapping of that struct to registers is actually correct.

The final, and maybe most important question (to me) is which things should and should not use yield_wait_for. One version is that everything in libtock_sync should use yield_wait_for, in which case it basically cannot co-oexist with any asynchronous code (e.g. you wouldn't be able to have both many outstanding alarms and do a synchronous sleep).

@bradjc
Copy link
Contributor

bradjc commented Jun 20, 2024

e.g. you wouldn't be able to have both many outstanding alarms and do a synchronous sleep

Wait, what happens if you try this? It seems like things would break pretty significantly if the async alarms fire before the delay_ms alarm fires. That seems...bad.

I think in general all of libtock-sync should use yield-waitfor. The benefit of libtock-sync working as users expect outweighs the disadvantage of not being able to do a sync and async call for the same driver, in my opinion.

Is there a way to special case alarm? As in, if libtock is written with the expectation that we want to support mixing async and sync, can we support both? The "easier" fix would be to add another upcall somehow (at least easier for userspace).

Or, what about a sort of "best effort" delay_ms()? Either, if an alarm is outstanding when delay_ms() is called, delay_ms() uses yield() like today, or delay_ms() can just return early if another alarm fires before delay_ms() finishes. Or maybe delay_ms() is a special case where it guarantees no other drivers will run, but other alarms might fire.

@alevy
Copy link
Member

alevy commented Jun 20, 2024

e.g. you wouldn't be able to have both many outstanding alarms and do a synchronous sleep

Wait, what happens if you try this?

yield_wait_for will pick up whatever the next event for that subscribe is, and drop any callbacks on the ground. So you'd basically miss the asynchronous alarm and delay_ms would return after a shorter period of time, and yeah, all sorts of hilarity would ensue.

This is the kind of thing i was hoping to be able to address with the proposal to run a callback if one is registered for that subscribe number.

I suppose it's worth thinking through whether/how this could actually happen unintentionally, given that our only form of concurrency is through some sort of yield.

@bradjc
Copy link
Contributor

bradjc commented Jun 24, 2024

It seems like realistically this is only going to be an issue with anything where there is a long-running outstanding operation (essentially an interrupt). Examples that come to mind:

  • alarm
  • button press / gpio
  • accelerometer movement detection
  • other detection sensors (eg proximity)
  • perhaps console input

Perhaps we need libtock-sync to handle mixing async and sync for drivers where that makes sense. If libtock-sync using yield_wait_for first unsubscribes the upcall, it can check if an upcall function was registered. If so, it calls the function after yield_wait_for. This repeats until the bool that yield_for is waiting for today is set.

@ppannuto
Copy link
Member Author

I feel like 'special casing alarm' starts to look a lot like posix signals--which have their faults but are a known design point; though I think this would come with some concurrency rules that are famously easy to error around.

Another option would be something akin to a select interface (yield-for-these[]), though mildly disheartening to already be pitching a new syscall...

@alevy
Copy link
Member

alevy commented Jun 29, 2024

Re pitching a new syscall, it can easily be an extension to this new experimental syscall. The reason to have it in the kernel is so we can start trying to actually use it and see if it need tweaking .

I think something like a select style interface is probably desirable and inevitable.

@alevy
Copy link
Member

alevy commented Jun 29, 2024

I don't understand the relationship between special casing alarm and POSIX signals. Can you explain what you mean by this and whether you are implying this is a good or bad thing (and why?).

@ppannuto
Copy link
Member Author

ppannuto commented Jul 8, 2024

IIUC, special-casing alarm would translate to allowing the alarm callback to run when userspace would otherwise not expect any random callbacks to be able to run. This means that the alarm callback function now has all the responsibilities of being careful to not touch any shared state, including implicitly (e.g., the picolib-c and micro-newlib we compile that omits reentrancy support on libc methods...). Indeed, I think the 'safe operations' for such a special-cased alarm method would be the exact same as those of a signal handler function in POSIX.

Then, once something else comes along that is 'worth' special-casing, now we've fully re-created signals.

Now, I'm not staking a claim about whether that's a good thing or a bad thing. I know @phil-levis has great disdain for the signal interface, but the more abstract notion of 'user-space interrupts' probably is very useful, and if signal really is the wrong interface, it'd be an interesting (sosp paper? :) ) question to design the/a RightOne(TM).

@bradjc
Copy link
Contributor

bradjc commented Jul 9, 2024

I think the expectation for delay_ms() is that everything stops and blocks until that many ms have elapsed. To implement this, I think we need to re-implement delay_ms() to not use any of the existing libtock-c virtualized code and instead to take over the alarm syscall driver. Then when yield_waitfor returns we know that ms have elapsed. Then we can restore the upcall and call into the virtualized stack in case an alarm did occur during the delay_ms().

@ppannuto ppannuto force-pushed the yield-waitfor-nocallback branch from 61f3669 to 3763057 Compare October 30, 2024 21:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants