Memory #216

bdevierno1 · 2020-05-31T09:23:43Z

Related to issues #200 and #201
Removed a few memory leaks. Replaced some libc function calls with the corresponding recommended dpdk function calls. Created new function to free allocated memory upon manager termination. Added if statements to error check after every allocation.

Summary:

Run manager as per usual. To test with valgrind, this is highlighted in issue #200 Note that it may take up to 20 mins to finish. Possibly much longer than that if you compile with line numbers.

Here are DPDK's recommendations
DPDK highlights that replacing the libc functions will provide a performance benefit. In the tests that I ran, I did not see any significant changes in the packet speed. This is probably due to the function calls only being made once in the NF setup and not continuously being called in the NFs packet handler.

There are some libc allocations remaining. Noticeably in onvm config. These functions are called to parse the json config upon booting with a json. They are called before the initialization of dpdk in onvm_nflib_init() therefore using rte_malloc will not be possible nor will it have an impact on performance.

Usage:

This PR includes
Resolves issues
Breaking API changes
Internal API changes
Usability improvements
Bug fixes	👍
New functionality
New NF/onvm_mgr args
Changes to starting NFs
Dependency updates
Web stats updates

Merging notes:

Dependencies: None

TODO before merging :

PR is ready for review
Test whether changes to onvm/onvm_nflib/onvm_nflib.c by using rte_calloc is causing possible packet rate loss.

Test Plan:

Ran the manager with speed tester as well by creating a NF chain.
Tested with the different flags the manager outlined in the readme.
Booted NFs from json config file.
Tested NFs such as firewall still ran smoothly after changes to json.h
Ran with cppcheck to ensure all previous errors were no longer highlighted.
Ran with valgrind to ensure no memory leaks.
DPDK Valgrind is not perfect, so went through code to look for any obvious memory leaks that can be resolved.

After: Memory definitely lost now zero.

Before:

Note that I dont believe valgrind is optimized very well for dpdk. Many of the existing 'errors' picked up by valgrind are caused during the Initialize the Environment Abstraction Layer (EAL) by DPDK function calls such as rte_eal_init(). I believe these can be ignored. Note that DPDK recommends using their cleanup function. However doing so caused errors in compile time: "is deprecated: Symbol is not yet part of stable ABI [-Werror=deprecated-declarations]"

Review:

@kevindweb

onvm · 2020-05-31T09:23:45Z

In response to PR creation

CI Message

Your results will arrive shortly

onvm

In response to PR creation

CI Message

Run successful see results:
Test took: 5 minutes, 29 seconds
✔️ Pktgen performance check passed
❌ PR drops Speed Test performance below minimum requirement

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 40347308
Performance rating - 96.07% (compared to 42000000 average)

bdevierno1 · 2020-05-31T10:21:30Z

@onvm Test please.

onvm · 2020-05-31T10:21:32Z

@onvm Test please.

CI Message

Your results will arrive shortly

onvm

@onvm Test please.

CI Message

Run successful see results:
Test took: 5 minutes, 26 seconds
✔️ Pktgen performance check passed
❌ PR drops Speed Test performance below minimum requirement

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 40462067
Performance rating - 96.34% (compared to 42000000 average)

bdevierno1 · 2020-05-31T11:04:41Z

@onvm Test again please.

onvm · 2020-05-31T11:04:43Z

@onvm Test again please.

CI Message

Your results will arrive shortly

onvm

@onvm Test again please.

CI Message

Run successful see results:
Test took: 5 minutes, 27 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 42008679
Performance rating - 100.02% (compared to 42000000 average)

bdevierno1 · 2020-05-31T12:54:24Z

@onvm Test whether rte_calloc caused rate drop.

onvm · 2020-05-31T12:54:25Z

@onvm Test whether rte_calloc caused rate drop.

CI Message

Your results will arrive shortly

onvm

@onvm Test whether rte_calloc caused rate drop.

CI Message

Run successful see results:
Test took: 5 minutes, 25 seconds
✔️ Pktgen performance check passed
❌ PR drops Speed Test performance below minimum requirement

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 40410498
Performance rating - 96.22% (compared to 42000000 average)

bdevierno1 · 2020-05-31T14:29:31Z

@onvm Test with first solution without rte_calloc please.

onvm · 2020-05-31T14:29:33Z

@onvm Test with first solution without rte_calloc please.

CI Message

Your results will arrive shortly

onvm

@onvm Test with first solution without rte_calloc please.

CI Message

Run successful see results:
Test took: 5 minutes, 28 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 41983113
Performance rating - 99.96% (compared to 42000000 average)

kevindweb · 2020-05-31T15:19:02Z

@bdevierno1 This is awesome! What did CI not like about your first commit? I haven't seen it that slow in a while. Also sorry I didn't see this before, but I don't think we should be changing lib/cJSON.c as we don't write anything in there. Every few months/to a year we pull from a different repository for that code and probably don't want to change memcpy->rte_memcpy as a result.

bdevierno1 · 2020-05-31T15:21:42Z

@bdevierno1 This is awesome! What did CI not like about your first commit? I haven't seen it that slow in a while. Also sorry I didn't see this before, but I don't think we should be changing lib/cJSON.c as we don't write anything in there. Every few months/to a year we pull from a different repository for that code and probably don't want to change memcpy->rte_memcpy as a result.

Cool that's fine. It took a few tries to figure out but it really does not like rte_calloc in main.c I'm going to test if there is any impact on performance when I call the function in other areas.

bdevierno1 · 2020-05-31T15:35:24Z

@onvm Test whether removing rte_calloc on libc as well impacts performance.

onvm · 2020-05-31T15:35:25Z

@onvm Test whether removing rte_calloc on libc as well impacts performance.

CI Message

Your results will arrive shortly

onvm

@onvm Test whether removing rte_calloc on libc as well impacts performance.

CI Message

Run successful see results:
Test took: 5 minutes, 25 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 42220573
Performance rating - 100.53% (compared to 42000000 average)

onvm · 2020-06-09T03:48:25Z

@onvm Try again please.

CI Message

Your results will arrive shortly

onvm · 2020-06-09T03:54:55Z

@onvm Try again please.

CI Message

Error: ERROR: Failed to post results to GitHub

bdevierno1 · 2020-06-09T03:58:55Z

@onvm Please post results.

onvm · 2020-06-09T03:58:57Z

@onvm Please post results.

CI Message

Your results will arrive shortly

onvm · 2020-06-09T04:04:26Z

@onvm Please post results.

CI Message

Error: ERROR: Failed to post results to GitHub

bdevierno1 · 2020-06-09T23:33:38Z

@onvm

onvm · 2020-06-09T23:33:40Z

@onvm

CI Message

Your results will arrive shortly

onvm

@onvm

CI Message

Run successful see results:
Test took: 5 minutes, 23 seconds
✔️ Pktgen performance check passed
✔️ Speed Test performance check passed

[Results from nimbnode23]

Median TX pps for Pktgen: 7721721
Performance rating - 100.28% (compared to 7700000 average)
Median TX pps for Speed Tester: 42266681
Performance rating - 100.63% (compared to 42000000 average)

bdevierno1 · 2020-06-10T00:13:28Z

Okay looks like using rte_zmalloc made a difference. Ready for review @catherinemeadows @dennisafa

dennisafa · 2020-06-12T20:44:37Z

onvm/onvm_mgr/main.c

+                tx_mgr[i]->nf_rx_bufs = rte_calloc(NULL, MAX_NFS, sizeof(struct packet_buf), RTE_CACHE_LINE_SIZE);
+                if (tx_mgr[i]->nf_rx_bufs == NULL) {
+                        RTE_LOG(ERR, APP, "Can't allocate packet_buf struct\n");
+                        onvm_main_free(tx_lcores,rx_lcores, tx_mgr, rx_mgr, wakeup_ctx);


I think that calling onvm_main_free after every check may be redundant, could we maybe set a flag and move that call out of the loop so that it's called only once? We should fail on any unsuccessful allocation anyway.

I think calling onvm_main_free followed by RTE_EXIT would be best? If not I could also see using a flag or a goto label as another option.

@bdevierno1 you probably want to just set a flag and break out of the loop. Then just log that we couldn't allocate the structs required to continue.

I just made a change as per your suggestions. I ended up using a goto flag as using a break would exit the for loop. If I used a break in the first loop it will still enter the next loop.

kevindweb · 2020-06-14T19:29:28Z

Can someone explain the difference between zmalloc and calloc? The dpdk doc entries looked quite similar so idk why zmalloc wouldn’t cause the problems calloc did

dennisafa · 2020-06-14T19:34:13Z

Can someone explain the difference between zmalloc and calloc? The dpdk doc entries looked quite similar so idk why zmalloc wouldn’t cause the problems calloc did

They're equivalent, but rte_calloc allows you to specify a num_items to allocate variable while rte_zmalloc does not. I don't think they should cause performance fluctuations either, but we've been having those with speed tester lately

onvm/onvm_mgr/main.c

dennisafa

Good work. We can also add a doc that maybe guides devs on using DPDK correctly in the system. Maybe in our wiki
Also I don't think that this has dependencies on the other DPDK pr (refactoring to the new version) but we should double check

twood02 · 2020-07-22T13:27:49Z

@catherinemeadows will mark this for approval since she reviewed it previously

Benjamin De Vierno added 2 commits May 31, 2020 07:47

Removed memory leaks and updated with libc functions.

76fb73c

Conform to linter.

0f48bd0

onvm suggested changes May 31, 2020

View reviewed changes

Testing if not using rte_memcpy will increase pkt speed.

8ab055a

onvm suggested changes May 31, 2020

View reviewed changes

Revert changes to main.c to check if for pkt rate loss.

ec7c832

onvm previously approved these changes May 31, 2020

View reviewed changes

Added dpdk calloc functions to mgr/main.c

1dc03a8

bdevierno1 dismissed onvm’s stale review via 1dc03a8 May 31, 2020 12:52

onvm suggested changes May 31, 2020

View reviewed changes

Benjamin De Vierno added 2 commits May 31, 2020 14:22

Reverted to earlier solution with rte_calloc removed

31eb834

Reverted to earlier solution with rte_calloc removed

92861ca

onvm previously approved these changes May 31, 2020

View reviewed changes

Test whether other rte_calloc has same impact on performance

e91a209

bdevierno1 dismissed onvm’s stale review via e91a209 May 31, 2020 15:18

onvm previously approved these changes May 31, 2020

View reviewed changes

Revert changes to JSON.c

cb51345

twood02 assigned catherinemeadows and dennisafa Jun 9, 2020

onvm previously approved these changes Jun 9, 2020

View reviewed changes

dennisafa reviewed Jun 12, 2020

View reviewed changes

Use goto flag instead of repeatedly calling onvm_free

c1a8be0

bdevierno1 dismissed onvm’s stale review via c1a8be0 June 15, 2020 15:00

Benjamin De Vierno added 2 commits June 15, 2020 15:07

Edit err msg

2429e4f

Fixed pointer err

6cb46a2

catherinemeadows mentioned this pull request Jun 16, 2020

Unable to re-launch NF with same instance ID #233

Closed

twood02 added this to the ONVM 20.07 milestone Jun 18, 2020

dennisafa requested changes Jun 19, 2020

View reviewed changes

onvm/onvm_mgr/main.c Outdated Show resolved Hide resolved

onvm/onvm_mgr/main.c Show resolved Hide resolved

onvm/onvm_mgr/main.c Show resolved Hide resolved

Benjamin De Vierno added 2 commits June 19, 2020 15:03

Merge remote-tracking branch 'upstream/develop' into Memory

ef9da0e

Move check to start of for loop

f184475

dennisafa approved these changes Jul 22, 2020

View reviewed changes

twood02 added the ready-for-gatekeeper 🚪 PRs that have been approved and are ready for a final review/merge label Jul 22, 2020

This was linked to issues Jul 22, 2020

Replacing libc functions with DPDK functions #201

Closed

Memory Leaks #200

Closed

catherinemeadows approved these changes Jul 22, 2020

View reviewed changes

dennisafa merged commit c2b92b5 into sdnfv:develop Jul 22, 2020

Memory #216

Memory #216

Conversation

bdevierno1 commented May 31, 2020 • edited Loading

Summary:

Merging notes:

Test Plan:

Review:

onvm commented May 31, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

bdevierno1 commented May 31, 2020

onvm commented May 31, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

bdevierno1 commented May 31, 2020

onvm commented May 31, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

bdevierno1 commented May 31, 2020

onvm commented May 31, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

bdevierno1 commented May 31, 2020

onvm commented May 31, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

kevindweb commented May 31, 2020

bdevierno1 commented May 31, 2020

bdevierno1 commented May 31, 2020

onvm commented May 31, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

onvm commented Jun 9, 2020

CI Message

onvm commented Jun 9, 2020

CI Message

bdevierno1 commented Jun 9, 2020

onvm commented Jun 9, 2020

CI Message

onvm commented Jun 9, 2020

CI Message

bdevierno1 commented Jun 9, 2020

onvm commented Jun 9, 2020

CI Message

onvm left a comment

Choose a reason for hiding this comment

CI Message

bdevierno1 commented Jun 10, 2020

dennisafa Jun 12, 2020

Choose a reason for hiding this comment

bdevierno1 Jun 13, 2020

Choose a reason for hiding this comment

kevindweb Jun 14, 2020

Choose a reason for hiding this comment

dennisafa Jun 14, 2020

Choose a reason for hiding this comment

bdevierno1 Jun 15, 2020

Choose a reason for hiding this comment

kevindweb commented Jun 14, 2020

dennisafa commented Jun 14, 2020

dennisafa left a comment • edited Loading

Choose a reason for hiding this comment

twood02 commented Jul 22, 2020

bdevierno1 commented May 31, 2020 •

edited

Loading

dennisafa left a comment •

edited

Loading