Update the tcp_ratelimit code to finally work right :)
ClosedPublic
Actions

Authored by rrs on Jan 26 2021, 5:35 PM.

Details

Reviewers

tuexen
• hselasky
gallatin

Group Reviewers

transport

Commits

rG39b87f051506: Fix LINT kernel builds after 1a714ff20419 .
rGdb46c0d0cb3d: Fix LINT kernel builds after 1a714ff20419 .

Summary

The tcp_ratelimit code is a bit behind, we have found many issues in
working with Mellanox cards. Some of them are due to the fact
that TLS and Ratelimit uses tags, this makes it so we have to be able
to wind down to the right tag and thus the new methods in the if_var.

There were other issues with the epoch handling in the tcp_ratelimit.c code
that could cause us to have issues when we used it. The interface also needs
tweaking to clear the pathway for the widespread use of ratelimiting in rack (which
will be a follow on commit).

Note to Hans, I have all the commits here but if you would like me to pull out the
changes in the mlx driver and commit them separately that would be fine, just let
me know.

Test Plan

Make sure basic rack can use the new ratelimit code by enabling it and running
on a machine that uses a mlx interface. The BB logging should show us the
rate limit code being used.

Diff Detail

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

rrs created this revision.Jan 26 2021, 5:35 PM

Herald added 1 blocking reviewer(s): transport. · View Herald TranscriptJan 26 2021, 5:35 PM

Herald added subscribers: melifaro, ae. · View Herald Transcript

rrs requested review of this revision.Jan 26 2021, 5:35 PM

It looks like the Mellanox bits can be committed separately and before the other patches.

Then you can just rebase this patch.

I'm currently trying to get some allocation for testing internally first.

--HPS

Hans:

Thats fine if you want to do it that way but note that this patch adds the new
function if_next_snd_tag() to if_var.h. You won't be able to add that function unless
we commit this one (without the mlx changes) first.

Or we can have you commit your changes, then rebase this to those, followed by
adding the mlx version of the if_next_snd_tag() as shown here too..

I am happy with any why you want to approach it!

Take out the mlx bits so that Hans can commit them separately

Lets go ahead and not put the netflix_stats directive in front of the sysctl's.
Turns out we already had these defined if RATELIMIT is defined so lets
just add the two new ones.

Can if_next_snd_tag() be committed first?

So Hans:

Are you asking me to pull out the add of the snd_tag_nxt functions and then update the ratelimit code?
I can do that, but I would think just putting this in would be good enough. We don't need to have
the changes in mlx to commit this here.. there were some big changes here due to issues with
the locking/epoch's so the ratelimit code, as is in FreeBSD can cause panic's if used.

I would prefer to just commit this, but if you feel strongly that the if_var.h and nxt_snd_tag changes
should go in separately, I am willing to do that.