Simple Forward with Token Bucket NF

[rohit-mp]

This PR adds a new example - simple_fwd_tb

The NF uses the advanced rings mode and simulates a queue with a token bucket. It takes in depth and rate as additional parameters and forwards/drops the packets based on the those values.

Summary:

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 :

• [x] PR is ready for review

Test Plan:

Trivial inputs have been tested. The rate was increased while keeping depth constant to observe a increase in tx_pps upto a limit. The depth was increased while keeping rate constant to observe a decrease in the number of dropped packets. Any input on how to test the same thoroughly is appreciated.

Testing the rate

R=100

R=1000

R=10000

Testing the depth

D=1000

D=10000

D=100000

Point to be noted is that the depth should be sufficiently high enough to enqueue atleast 32 packets at once onto the tx_ring for the NF stats to be updated properly. Related to #198

Review:

@twood02

Subscribers: << @-mention people who probably care about these changes >> @Shashwatha-Mitra @NishanthSubramanian @archit-p @mohittahiliani

[onvm]

In response to PR creation

CI Message

Your results will arrive shortly

[twood02]

@onvm test this please!

[onvm]

@onvm test this please!

CI Message

Your results will arrive shortly

[kkrama]

I wonder if this whole process of dequeuing a large burst of packets from the Rx ring and then processing it in the leaky bucket is the most appropriate way to design a leaky bucket AQM mechanism...

On Mon, Mar 30, 2020 at 9:45 AM Dennis Afanasev notifications@github.com wrote:

@dennisafa commented on this pull request.

In examples/simple_fwd_tb/forward.c https://github.com/sdnfv/openNetVM/pull/199#discussion_r400339245:

+#include +#include +#include <sys/queue.h> +#include + +#include +#include +#include +#include + +#include "onvm_nflib.h" +#include "onvm_pkt_helper.h" + +#define NF_TAG "simple_fwd_tb" + +#define PKT_READ_SIZE ((uint16_t) 100000000) // TODO: Find proper value for PKT_READ_SIZE

That value, RTEMP*_DESC_DEFAULT appears to be related to the port configuration. NF_QUEUE_RINGSIZE is the size of the RX/TX ring. You may either check the size and dequeue all packets up to that maximum, or just set the dequeue burst parameter to the ringsize. I think the latter approach is more efficient.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sdnfv/openNetVM/pull/199#discussion_r400339245, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6TUJJUYRJDVFWAB3ZOHVLRKDEDVANCNFSM4LVFHKTA .

-- K. K. Ramakrishnan

[rohit-mp]

I could think of 3 possible ways of implementing the token bucket:

1. Dequeue pkt by pkt based on availability of tokens - This way, we wouldn't be able to apply a dynamic depth constraint and it would be fixed to the size of the ring. Also, we'd have to preemptively dequeue a pkt to know it's size if we treat a token to be equivalent to a byte instead of a pkt.
2. Burst dequeue and process pkts based on availability of tokens - This way, we can apply depth constraints since we know how many pkts were in the queue (since we dequeue all available pkts). Although, the packets which would've been dropped as compared between this implementation and dequeuing one-by-one would be different.
3. Maintain a separate queue apart from the ring to simulate a traditional token bucket - This method would involve launching threads, one for processing rx and the other for tx since it would be hard for a single thread to dequeue from rx_ring at a constant rate while generating tokens and enqueuing to tx_ring based on availability of tokens. And maintaining two threads comes with the cost of having locks on the queue which could lead to poor performance.

Considering the above points, I went ahead with the second approach as mentioned above. Does that look fine? Thanks and Regards.

[kkrama]

I feel this is not the right way to implement it. It adds a lot of latency on the processing of the Rx; potentially drops packets unnecessarily on the Rx; creates lots of burstiness on the Tx, which in principle will be bad for downstream nodes processing the traffic. I really don't think you need to have these multiple threads the way you have structured it. Maybe Sameer might give you a better design on how to structure the token filling... K. K.

On Mon, Mar 30, 2020 at 11:46 AM Rohit M P notifications@github.com wrote:

I could think of 3 possible implementing the token bucket:

1. Dequeue pkt by pkt based on availability of tokens - This way, we wouldn't be able to apply a dynamic depth constraint and it would be fixed to the size of the ring. Also, we'd have to preemptively dequeue a pkt to know it's size if we treat a token to be equivalent to a byte instead of a pkt.
2. Burst dequeue and process pkts based on availability of tokens - This way, we can apply depth constraints since we know how many pkts were in the queue (since we dequeue all available pkts). Although, the packets which would've been dropped as compared between this implementation and dequeuing one-by-one would be different.
3. Maintain a separate queue apart from the ring to simulate a traditional token bucket - This method would involve launching threads, one for processing rx and the other for tx since it would be hard for a single thread to dequeue from rx_ring at a constant rate while generating tokens and enqueuing to tx_ring based on availability of tokens. And maintaining two threads comes with the cost of having locks on the queue which could lead to poor performance.

Considering the above points, I went ahead with the second approach as mentioned above. Does that look fine? Thanks and Regards.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sdnfv/openNetVM/pull/199#issuecomment-606174738, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6TUJML5HT5FDFI5TELJITRKDSG5ANCNFSM4LVFHKTA .

-- K. K. Ramakrishnan

[rohit-mp]

Thanks for the feedback Sir. I could remove the tx burst by modifying my implementation but I'm not quite sure how the rx latency is increasing. Should I send an email to Sameer?

[rohit-mp]

Thanks for the inputs. I've simplified the code to work with a batch size of 32. The excess packets are now stalled and not dropped if tokens are insufficient. Packets are dropped only when the ring becomes full.

[twood02]

Thanks @rohit-mp . I think this is a good solution which should reduce burstiness caused by large dequeues/enqueues.

How have you been testing this?

[rohit-mp]

I tested the rate by varying it to see a corresponding change in the packets processed per second. I'm not sure how to verify depth as that can be tested under bursty condition and speed-tester wasn't suitable for that.

Any input on how to test it thoroughly is appreciated.

[dennisafa]

I tested the rate by varying it to see a corresponding change in the packets processed per second. I'm not sure how to verify depth as that can be tested under bursty condition and speed-tester wasn't suitable for that.

Any input on how to test it thoroughly is appreciated.

You may try testing depth out by hooking up pktgen on a client node and sending it traffic from there, check out our Wiki page to see how to set it up. I think that pktgen lets you attach a script, you can try simulating bursty behavior through that.

[kevindweb]

Just like @dennisafa said, write a little Lua code that Pktgen will process and send. If you update pktgen Lua script you can create these kinds of advanced tests to pump packets through the network.

[rohit-mp]

Sorry for the delay! Thanks @dennisafa @kevindweb

This was run with a 10G NIC and Pktgen generating packets. The flow was Pktgen -> simple_forward -> simple_fwd_tb -> drop. The tb NF was run with a rate of 1000MBps and as seen in the screenshot, the rate of pkt generation from Pktgen was around 1200MBps (rx_pps of simple_forward) and due to the tb_rate, the output rate of tb NF (tx_pps of simple_fwd_tb) was reduced to 1000MBps and packets can be seen being dropped at the rx_ring of tb NF.

For verifying the tb_depth, I wrote a basic NF which enqueued pkts in burst of 10000pkts to it's tx_ring (since the max burst from Pktgen was 64pkts and that was not sufficient). The flow was Pktgen -> bursty_forward -> simple_fwd_tb -> drop. The burst was set to be more than the rate (Eg: tb_depth of 10MB and tb_rate of 1MBps). On starting the NF, the initial burst caused the bandwidth to reach 10MBps but that soon reduced to 1MBps (to match the tb_rate after the bucket became empty).

[Each pkt size was 1000bytes and the bandwidth was calculated by tx_pps or rx_pps * pkt_size]

[onvmstats]

@onvm test unauthorized

[onvm]

@onvm test unauthorized

CI Message

Your results will arrive shortly

[catherinemeadows]

@onvm test

[onvm]

@onvm test

CI Message

Your results will arrive shortly

[twood02]

@rohit-mp - can you share a link to your burst test NF? We'll take a look and do some tests with that too.

[rohit-mp]

https://github.com/rohit-mp/openNetVM/tree/bursty_forward_nf/examples/bursty_forward I've added the code I used here. I use two parameters:

• PKT_BURST_SIZE - how many pkts to enqueue at once, and
• SLEEP_TIME - after how many seconds do I start processing the pkts again after one enqueue operation This was done solely to let the token_bucket get filled again before receiving the next burst and thus, a large amount of pkts get dropped.

[catherinemeadows]

@onvm

[onvm]

@onvm

CI Message

Your results will arrive shortly

[rohit-mp]

Hello team, I recently realized that we don't actually need advanced rings mode to simulate the token bucket. But I think this is still a good basic example showing the usage. Would you prefer that I use default rings mode instead? Or would it be good to provide both modes as options as done in scaling NF?

[kkrama]

It would seem to me that if the default ring mode is sufficient, I would just restrict it to use that capability. Unless you are likely to use the advanced ring mode for the scaling to multiple instances. I would think that we may have to carefully think through all the ramifications before that.

On Sun, May 3, 2020 at 12:33 AM Rohit M P notifications@github.com wrote:

Hello team, I recently realized that we don't actually need advanced rings mode to simulate the token bucket. But I think this is still a good basic example showing the usage. Would you prefer that I use default rings mode instead? Or would it be good to provide both modes as options as done in scaling NF?

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/sdnfv/openNetVM/pull/199#issuecomment-623068881, or unsubscribe https://github.com/notifications/unsubscribe-auth/AC6TUJPLJZ7DHYYB5TPKCATRPUM5TANCNFSM4LVFHKTA .

-- K. K. Ramakrishnan

[rohit-mp]

Thank you for the suggestion. Yes, default ring mode is sufficient. But @twood02 mentioned that this would be a good example for helping people learn how to use the advanced rings interface (in the comments above on this PR).

[dennisafa]

Thank you for the suggestion. Yes, default ring mode is sufficient. But @twood02 mentioned that this would be a good example for helping people learn how to use the advanced rings interface (in the comments above on this PR).

In my opinion I think that it would be best to use the default mode unless necessary. Adv. rings mode should just be used when we need full control over the RX/TX rings.

[twood02]

Yes, I still think this is quite useful as an example of the advanced rings mode. I suggest we finalize this Pull Request and merge it with just the advanced rings mode. Later we can make a second version based on the normal packet handler function, which I'd prefer to keep as a separate directory so it is easier to see the differences in the two approaches. If @rohit-mp wants to add the standard packet handler version, that'd be great. Otherwise one of our students can do it this summer.

[rohit-mp]

Thanks, let me know if anything is needed to help in finalizing the PR.

I'd be happy to add the default ring mode as well soon.

[catherinemeadows]

Tested Rate

Tested rate by increasing rate parameter and keeping depth consistent and saw the expected increase in tx_pps.

R=100, D=100

R=1000, D=100

R=10000, D=100

R=20000, D=100

[rohit-mp]

This is the output of the C Linter which failed:

examples/simple_fwd_tb/forward_tb.c:57:  Include the directory when naming .h files  [build/include] [3]
examples/simple_fwd_tb/forward_tb.c:58:  Include the directory when naming .h files  [build/include] [3]
examples/simple_fwd_tb/forward_tb.c:90:  Odd number of spaces at line-start.  Are you using a 4/8-space indent?  [whitespace/indent] [3]
examples/simple_fwd_tb/forward_tb.c:182:  Odd number of spaces at line-start.  Are you using a 4/8-space indent?  [whitespace/indent] [3]
examples/simple_fwd_tb/forward_tb.c:191:  Odd number of spaces at line-start.  Are you using a 4/8-space indent?  [whitespace/indent] [3]

The first two lines would be an issue with every example in the repo. Should I fix that here? The next three lines are due to the indentation done by an extension for vs code which I personally think looks neater. Should I change that as well to make it conform to the start of the line everywhere? The same code didn't raise the error during the CI checks before.

[bdevierno1]

This is the output of the C Linter which failed:

examples/simple_fwd_tb/forward_tb.c:57:  Include the directory when naming .h files  [build/include] [3]
examples/simple_fwd_tb/forward_tb.c:58:  Include the directory when naming .h files  [build/include] [3]
examples/simple_fwd_tb/forward_tb.c:90:  Odd number of spaces at line-start.  Are you using a 4/8-space indent?  [whitespace/indent] [3]
examples/simple_fwd_tb/forward_tb.c:182:  Odd number of spaces at line-start.  Are you using a 4/8-space indent?  [whitespace/indent] [3]
examples/simple_fwd_tb/forward_tb.c:191:  Odd number of spaces at line-start.  Are you using a 4/8-space indent?  [whitespace/indent] [3]

The first two lines would be an issue with every example in the repo. Should I fix that here? The next three lines are due to the indentation done by an extension for vs code which I personally think looks neater. Should I change that as well to make it conform to the start of the line everywhere? The same code didn't raise the error during the CI checks before.

Hi @rohit-mp . Thanks for your comment. The reason is because we recently introduced a new linter using Github actions. The new linter updated the verbosity level --the amount of information returned by the linter-- which is why the lint failed. I felt this would provide more useful feedback to the user. However, you raised some important points and I will speak to the rest of the team and get back to you.

[rohit-mp]

In order to fix a bug for the case where the token bucket is larger than the packet size, on line 210 of forward_tb.c, you should have a condition:

 if (pkt->pkt_len > tb_tokens) {
                tb_tokens -= pkt->pkt_len;
 } else {
                tb_tokens = 0;
 }

Token Bucket being larger than the pkt size is the only valid case here. If size of Token Bucket is less than the pkt size, we would never be able to process any pkts. I think you meant you say tokens instead. These lines make sure that tokens are more than pkt size before reaching line 210: https://github.com/rohit-mp/openNetVM/blob/108f9ed196399a133de9620fcb47b19ddc1a26c3/examples/simple_fwd_tb/forward_tb.c#L187-L191 Also, I think you meant to use > in the if condition instead of <. In that case, the if condition wouldn't be required as just tb_tokens -= pkt->pkt_len would take care of the situation.

Also, in your README.md, when describing the -R flag, 'Mbps' should be changed to 'MBps' to specify Megabytes rather than Megabits.

Good catch. Will fix that in the usage as well.

[twood02]

@rohit-mp I think you are correct that your if statement up top helps prevent it in the normal case, but since tb_tokens is initialized to a large number, the bug can still arise as @catherinemeadows and @bdevierno1 point out.

To test this, run speed tester with -s 1024 (for 1024 byte packets) and your code with a depth of 1000. There will be no rate limiting because of the unsigned issue. @catherinemeadows‘s fix works, but maybe adjusting the default value will be simpler and trigger your existing check.

Your PR mentions depth should be set carefully, but I think that should be more explicit. At least print an obvious warning if depth is less than say 10,000.

[rohit-mp]

Ah nice catch! Thanks for that. I missed out the value of depth being less than pkt_len.

But @catherinemeadows fix would resolve the issue only partially (I'm assuming we've to interchange the if and else part in that) since we'd still be processing the pkt without sufficient tokens.

I will add a warning for when depth is set to less than 10,000. But along with that warning, should I drop pkts that arrive with a size of more than the depth? It seems like the better option to me since technically, that packet should never be processed and all the pkts arriving behind it would be stalled forever.

[twood02]

@rohit-mp if you have a chance to add the warning and fix soon that'd be great (sorry to bug you with all these little things, your contribution is greatly appreciated!). If you won't have time to make them soon, then we can push the edits ourselves. We'd like to merge this for our May release.

[rohit-mp]

@rohit-mp if you have a chance to add the warning and fix soon that'd be great (sorry to bug you with all these little things, your contribution is greatly appreciated!). If you won't have time to make them soon, then we can push the edits ourselves. We'd like to merge this for our May release.

Not an issue at all. The careful reviews are highly appreciated. I was waiting for confirmation on what to do when a pkt with length greater than depth arrives (as asked in my last message).

[twood02]

oops, sorry missed your question at the end.

should I drop pkts that arrive with a size of more than the depth? It seems like the better option to me since technically, that packet should never be processed and all the pkts arriving behind it would be stalled forever.

Yes, I agree that dropping packets that are larger than the depth is fine. I don't think this would be used much in practice (depth should always be much bigger), but I suppose this would be a way to apply a filter to remove large packets which might be useful in some cases.

[rohit-mp]

I've added a warning for when depth is less than 10,000 and an unlikely check in the packet handler for when pkt length is more than depth. Let me know if any changes are required