Restoring NVM contexts require kilobytes of stack and is unnecessarily complicated

[andysan]

The current implementation of NvmCtxMgmtRestore allocates many kilobytes memory on the stack stack. This could be avoided if the semantics of MIB_NVM_CTXS were cleaned up.

The current semantics seem to be that a read of MIB_NVM_CTXS returns a buffer pointer that should be assumed to be read-only and a size for every context. Setting MIB_NVM_CTXS copies data from a caller-allocated buffer to an internal context.

As a consequence, the current implementation of NvmCtxMgmtRestore looks something like this:

1. Get MIB_NVM_CTXS to get the sizes of all of the contexts.
2. Allocate enough space to store the contexts on the stack (this is generally considered bad practice).
3. Read contexts from NVM to temporary on the stack.
4. Set MIB_NVM_CTXS to apply the restored contexts.
5. Each module performs a memcpy1 from the temporary buffer to its internal NVM context buffer.

The semantics could be changed to mean that getting MIB_NVM_CTXS returns non-constant pointers and sizes to the internal state of the stack and setting MIB_NVM_CTXS notifies the stack that the data pointed to by those pointers has been updated. This would significantly simplify the code for the caller and avoid dynamic memory allocation. In this case, NvmCtxMgmtRestore would look something like this:

1. Get MIB_NVM_CTXS to get the sizes of all of the contexts.
2. Read contexts from NVM to pointers received in step 1.
3. Set MIB_NVM_CTXS to apply the restored contexts.

This would remove the need for a heap allocation or large stack allocation in NvmCtxMgmtRestore and remove one buffer copy. The current use of large stack allocations makes the code unpredictable in resource constrained environments.

In terms of code changes, the only change necessary would be to update NvmCtxMgmtRestore to read straight into the context buffers and to remove the memcpy1 calls in the context handlers.

[mluis1]

Thanks for the review.

It is in our roadmap to improve the contexts management. This improvement work will start as soon as other higher priority tasks are finalized.

Please note that, I also find the way it is currently implemented to be too complex and should be improved.

[mluis1]

@andysan

We have re-worked the NVM context management keeping in mind your recommendations.

The new implementation does not use anymore the memcpy1 calls.

Concerning the used amount of memory to be stored, we have tried to reduce it as much as possible. Although, the full context is still around 1 KB.

If you are OK with the proposed implementation, could you please close this issue?

[andysan]

I was mostly concerned about the amount of dynamic memory (i.e., stack or heap) used by the previous implementation which would lead to unpredictable out-of-memory conditions at runtime. The new version seems to solve this problem.