[Hexagon] [runtime] Manage RPC and runtime buffers separately

[janetsc]

Creates HexagonPageAllocator to manage allocations for the RPC server

Adds new Device APIs for RPC buffer management

Allocations are tracked by two separate buffer managers:

• rpc_hexbuffs is used exclusively for RPC buffers
• runtime_hexbuffs is used exclusively for runtime buffers

This will fix the throw on shutdown in the simulator.

cc: @csullivan, @kparzysz-quic, @adstraw

[Lunderberg]

Overall, looks good, just a few questions and locations where comments/errors could be improved.

[tvm-bot]

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

• Built docs for commit 9d3fddf can be found here.

_{Generated by tvm-bot}

[Lunderberg]

Changes look good to me, thank you! A couple additional nitpicks that can go either way.

[adstraw]

I apologize for the post-merge feedback. But ... having this method seems like a break in the abstraction of the HexagonBufferManager class, to me. If we have to do this it feels like something else is amiss.

[janetsc]

Thanks for the review.

I agree, this isn't ideal. However, I wanted to make sure that I don't give a "free pass" to all frees that come after ReleaseResources. The scenario I was trying to avoid was:

User allocates buffer B
Session ends, ReleaseResources is called and B is freed
User frees buffer B as objects are being torn down, which should not throw as it is somewhat expected
User frees buffer C, which SHOULD throw, as it was never allocated in the first place

I added this so that I can check if there's no runtime_hexbuffs, then I make sure that what is being attempted to be freed is something that we owned before ReleaseResources was called. In which case, that Free is a no-op. I also want to detect spurious calls to Free that wouldn't have been legit before ReleaseResources was called.

I'm happy to discuss more, and brainstorm other ways to accomplish this.

[Lunderberg]

As an alternative, what if the HexagonBufferManager allows three states for buffers instead of two.

• Valid to use, valid to free (Initial state after allocating memory)
• Invalid to use, valid to free (State after closing a session)
• Invalid to use, invalid to free (State of unknown pointer)

That way, instead of discarding the HexagonBufferManager runtime_hexbuffs altogether when the session closes, it instead moves all keys from the internal std::unordered_map<void*, std::unique_ptr<HexagonBuffer>> hexagon_buffer_map_; to an internal std::unordered_set<void*>. This avoids exposing any of the buffers to the outside, but does allow the HexagonBufferManager to identify a previously freed pointer.

[adstraw]

Thanks @Lunderberg, I think your suggestion would satisfy my concern. What do you think @janetsc? This would allow us to make valid frees a nop and to catch invalid frees as well. If I am understanding correctly runtime_hexbuffs would be instantiated by the Hexagon Device API in the same way that we instantiate the rpc_hexbuffs ... HexagonBufferManager runtime_hexbuffs;? If that is the case, we could potentially have just one HeagonBufferManager that implements the API and internal state that @Lunderberg suggests.

[janetsc]

There's also another state not captured - the buffer manager is in an invalid state to allocate more memory.

There needs to be two buffer managers, because on ReleaseResources how do we know which buffers need to be freed?

I'm thinking that a better model is to enforce that all frees should take place before ReleaseResources. That's cleaner. I will investigate which ones are outstanding. All objects should be cleanly torn down by the time the session ends. I will file a task to investigate.

[adstraw]

Not sure on the value-add for this else case. The only thing that happens is that we might fail in the CHECK if we free a buffer after we release resources that was not in the released_runtime_buffers. Do we want this behavior? What happens on-system if we just get rid of this else case? We can then get rid of released_runtime_buffers and the need for HexagonBufferManager::current_allocations. We can simply ...

  if (runtime_hexbuffs) {
    runtime_hexbuffs->FreeHexagonBuffer(ptr);
  }

With some comments to explain why the "free" may happen after the "release".

[janetsc]

My previous reply is related.

If there's a manager, I can just use it to call Free. If there isn't (it has been released, or maybe acquire was never called), I can check what I had allocated before Release. If that buffer is in there, I will not throw. But if there's a bug and the user is attempting to free some pointer we never knew about, I do want that to throw.

[adstraw]

Making sure all "temporal" frees occur before Release is a good idea and worthwhile endeavor. ✅

I am not convinced that two HexagonBufferManager objects are required. Instead, you could have separate interfaces in a single HexagonBufferManager class to track "permanent" (for RPC e.g.) versus "temporal" allocations. And, in this way, alleviate the need for the current_allocations method which exposes the internals of the HexagonBufferManager class to its users. Something to consider for a future cleanup, perhaps.