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

Memory leak when sending messages between NFs with full rings #293

Closed
twood02 opened this issue Jun 15, 2021 · 3 comments
Closed

Memory leak when sending messages between NFs with full rings #293

twood02 opened this issue Jun 15, 2021 · 3 comments
Labels
bug 🐛 good first issue Simple issues for new ONVM developers to help with! memleak 🚰

Comments

@twood02
Copy link
Member

twood02 commented Jun 15, 2021

Bug Report

Current Behavior
If an NF tries to send a message to another NF using the onvm_nflib_send_msg_to_nf() function it will allocate memory to store the message and then enqueue the message for the other NF. If the enqueue fails (because the destination NF's ring is full), then the allocated memory will never be freed.

onvm_nflib_send_msg_to_nf(uint16_t dest, void *msg_data) {
int ret;
struct onvm_nf_msg *msg;
ret = rte_mempool_get(nf_msg_pool, (void**)(&msg));
if (ret != 0) {
RTE_LOG(INFO, APP, "Oh the huge manatee! Unable to allocate msg from pool :(\n");
return ret;
}
msg->msg_type = MSG_FROM_NF;
msg->msg_data = msg_data;
return rte_ring_enqueue(nfs[dest].msg_q, (void*)msg);
}

Expected behavior/code
If the enqueue fails, we should free the allocated message. So line 730 should be split into multiple lines that check if the enqueue succeeds or fails, calls rte_mempool_put on failure, and then returns the success/failure flag.

Steps to reproduce
I don't think any of our example NFs use this function (it has mainly been used in our research projects). We should create a simple test NF that demonstrates this functionality and helps us verify that the bug is fixed.

@twood02 twood02 added bug 🐛 good first issue Simple issues for new ONVM developers to help with! memleak 🚰 labels Jun 15, 2021
@twood02
Copy link
Member Author

twood02 commented Jun 15, 2021

@NoahChinitzGWU - this can be a good first issue for you

@NoahChinitz
Copy link
Contributor

@twood02 - will definitely work on this

dennisafa pushed a commit that referenced this issue Aug 31, 2021
…essage Memory Leak (#296)

This PR includes a new NF called Basic Message Test that tests the functionality of an NF sending messages to itself. This NF was created in response to Issue #293 where a memory leak was found. The memory leak occurred when a message was trying to be sent and the message queue had been filled; the message was never freed and put back into the shared memory pool. Three tests are ran: the first making sure that one message (with the correct data) can be sent to an NF, next it tests if 10 messages can be sent to an NF, and finally it tests to make sure that even if the ring buffer is overflowed that the appropriate messages are received by the NF. Moreover, there were changes made to onvm_sc_common.c in which if there was no packet sent to the function then we would return the first instance ID associated with the given service ID.

Commit log:

* [Update] Fixed Issue #293

* [Update] Deleted dead code

* [Update] Abstracted SID so that it works with whatever SID given

* [Update] Changed name

* [Update] Changed dir name

* [Update] Changed code in regards to PR #296 comments

* [Update] Drop packets

* [Update] Sends more than one msg

* [Update] Working on making output cleaner as well as creating a struct to hold 'global data'

* [Update] Added checking of memory

* [Update] Added working struct. Working on cleaner output and automation of test

* [Update] Added first Test

* [Update] Working on Unit Tests

* [Update] Verson 1 of Tests. Looking at messaging code now

* [Update] Testing fixed. Changing Verbosity

* Delete Makefile

* Delete README.md

* Delete go.sh

* Delete test_messaging.c

* [Update] Ready for Review.

* debug files

* [Update] Test Version 3.0

* restored old go.sh file

* [Update] Version 3.1 of Tests. Added destroy funciton

* [Update] Fixed destroy function. Added NF to examples/Makefile. Still working on hanging execution.

* Hanging occurs when IID != SID

* Update launch.json

* - Added GDB configuration for VSCode.
 - Convert from service ID to instance ID when sending message in /examples/test_messaging
 - Add support to look up instance ID of a message by passing NULL for the packet

* Added function comments

* [Update] fixed file according to PR comments

* Delete launch.json

* Delete tasks.json

* Delete gdb.sh

* [Update] Deleted unnecessary files

* Changed test making sure all messages have been dequeued

* [Update] cleaned up the tests

* [Update] Ran Linter and finalized tests

* [Update] Print out socket ID information for manager and network functions

* Revert "[Update] Print out socket ID information for manager and network functions"

This reverts commit 8ac16b8.

* [Update] Print out socket ID information for manager and network functions

* [Updated] Fixed tests

* [Update] added 4th test phase

* Revert "[Update] Print out socket ID information for manager and network func…"

* [Update] Changing Workflow

* [Update] Fixed Workflow to be cleaner and more organized

* Update onvm/onvm_nflib/onvm_nflib.c
@twood02
Copy link
Member Author

twood02 commented Nov 19, 2021

resolved by #296

@twood02 twood02 closed this as completed Nov 19, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug 🐛 good first issue Simple issues for new ONVM developers to help with! memleak 🚰
Projects
None yet
Development

No branches or pull requests

2 participants