RFC: Remove AppPtr::Drop() and Owned::Drop()

[hudson-ayers]

Pull Request Overview

This PR proposes removing AppPtr::Drop() and Owned::Drop(), as well as the ProcessType::free() method that they each call. Currently, Process::free() is implemented with an empty body, merely dropping the passed raw pointer. Therefore, as far as I can tell, calls to Drop() on each type have no useful effect. Despite this, the presence of these implementations have a significant cost -- the implementations of Drop call kernel.process_map_or(), which takes a trait object reference (e.g. &dyn ProcessType), so the compiler cannot know that the concrete free() function that will be called is empty, and optimize the call out.

Currently, anytime an AppPtr or Owned type goes out of scope -- including anytime a struct containing such a type goes out of scope -- drop() is called on each type until the underlying drop() implementation is called. This happens frequently: a single iteration of the loop in the Imix app calls AppPtr::drop() 45 times; a single iteration of the hello_loop app calls AppPtr::drop() 4 times and Owned::drop() 3 times. Further, each call has some cost:

Cycles per call, measured using cortex-m DWT cycle counter peripheral:

AppSlice::drop() - 139 cycles (increases to 145 if the board is configured for 8 processes instead of 4)

Owned::drop() - 84-150 cycles, depending on context.

Removing the Drop implementations for AppPtr and Owned reduces the measured cycle count in each case to 1 cycle.

AppSlice::drop() is called regularly (inserted by the compiler) within the body of allow() system call implementations, Owned::drop() is called within the body of Grant::enter() and Grant::each().

Keeping in mind recent attempts to reduce Tock syscall overhead, I think that we should not pay all of this overhead for no benefit as Tock is implemented today.

Alternatively, if there is reason to believe these implementations will be useful in the future, I think we should document these functions (and ProcessType::free()) as to why they are present.

Testing Strategy

This pull request was tested by running several apps with the drop implementations removed.

TODO or Help Wanted

The Tock SOSP paper talks a bit about the Owned::drop() implementation, but as far as I can tell it does not actually do anything in Tock today. Is there reason to believe we will need these implementations for some future process type? Am I totally missing something here?

Documentation Updated

• No updates are required.

Formatting

• Ran make prepush.

[gendx]

Isn't it a bug that free doesn't actually free any memory? Doesn't this open the way for a malicious capsule dynamically allocating memory over and over again until there's no more memory available?

[hudson-ayers]

Isn't it a bug that free doesn't actually free any memory? Doesn't this open the way for a malicious capsule dynamically allocating memory over and over again until there's no more memory available?

I think that even if free did free memory, a capsule could do this by simply not dropping the types it has allocated. Currently, I don't think that any capsules in Tock actually make use of the allocation API that is exposed by the core kernel (though a use has been proposed for the rubble BLE stack).

I am pretty confused why free() is being called if the kernel is not actually dynamically allocating anything, however, so that seems like a bug to me. But perhaps I misunderstood how this is supposed to work, in which case adding documentation to these functions would be useful.

[jrvanwhy]

Doesn't this open the way for a malicious capsule dynamically allocating memory over and over again until there's no more memory available?

Tock does not try to prevent malicious capsules from denying service to the rest of the system. The short explanation is that doing so is not practical in a single-threaded, cooperatively-scheduled system like Tock's kernel.

[ppannuto]

I do think that free will someday be important, as we start to write richer and more complex apps, memory pressure is increasing, and giving apps the opportunity to manage that by freeing kernel resources will likely be necessary. That said, I'm also sympathetic to the notion that it hasn't been an issue yet nor is there any immediate demand.

Nonetheless, removing the ability to free resources feels like a very big hammer.

I am pretty confused why free() is being called if the kernel is not actually dynamically allocating anything, however, so that seems like a bug to me.

I think this is really the crux of the issue. Nothing has really exercised this code path before. It's old code too. Now that this capability is starting to be used, I think I'd rather see free fixed rather than removed if possible.

[hudson-ayers]

It seems like there must be some logic error in calling free() in the Drop impl for Owned, as an instance of Owned is created in every call to apps.enter() or apps.each(), despite the fact that neither of those calls should be doing any dynamic allocation, as I understand it.

Similarly, every call to allow() creates an AppSlice which contains an AppPtr, and thus an AppSlice is dropped whenever a replacement allow() is made, but allow() is not dynamically allocating memory either, so I am not sure why a call to free() is needed.

Perhaps we should keep free() as part of the ProcessType trait, but remove the Drop implementations? I just think that with free() being unimplemented it is pretty hard to reason about whether it is being called in the correct places.

[phil-levis]

Now that this capability is starting to be used, I think I'd rather see free fixed rather than removed if possible.

Where is it being used?

[ppannuto]

Where is it being used?

We talked about this on the call, but for posterity: Not in use in merged code yet, but ideally will be used by @daboross's BLE work.

[hudson-ayers]

Updated to keep free() but remove the Drop impls.

[ppannuto]

As discussed, there is more work to do here, but this isn't doing the right thing presently and is causing a non-trivial performance hit, so this feels like a good change for now.

[bradjc]

bors r+

[bors]

Build succeeded:

• ci-build (macos-latest)
• ci-build (ubuntu-latest)
• ci-format (macos-latest)
• ci-format (ubuntu-latest)
• ci-qemu
• ci-tests (macos-latest)
• ci-tests (ubuntu-latest)