Page MenuHomeFreeBSD

Update the tcp_ratelimit code to finally work right :)
ClosedPublic

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

Details

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

Repository
R10 FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

rrs requested review of this revision.Tue, Jan 26, 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.

?

@rrs: I'm fine with your proposal to commit everything except the mlx5en(4) parts, which we would like split out in an own commit.

Please consider MFC'ing to stable/13.

This revision is now accepted and ready to land.Thu, Jan 28, 1:35 PM