Page MenuHomeFreeBSD

Use PFIL's rmlock instead of IPFW's static rules rmlock
ClosedPublic

Authored by ae on Mar 27 2017, 8:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 8, 7:59 PM
Unknown Object (File)
Thu, Mar 28, 6:44 PM
Unknown Object (File)
Jan 13 2024, 3:43 AM
Unknown Object (File)
Dec 20 2023, 2:54 AM
Unknown Object (File)
Sep 6 2023, 1:12 AM
Unknown Object (File)
Sep 6 2023, 1:09 AM
Unknown Object (File)
Sep 1 2023, 12:13 AM
Unknown Object (File)
Sep 1 2023, 12:00 AM

Details

Summary

This change reduce the number of locks required to hold for each transmitted packet when ipfw is loaded.

Test Plan

We use this patch more than 3 years at Yandex.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

Hi,

So this just changes the locking model to use a per-vnet lock rather than a per-chain lock for stuff?

So this just changes the locking model to use a per-vnet lock rather than a per-chain lock for stuff?

We have only one chain per-vnet. Thus the locking model is still the same.

Just curious, do you have any comparison/profiling data if this improves anything?

This is a change in good direction, though it would be better if some lockless model was applied to pfil.

In D10154#209953, @eri wrote:

Just curious, do you have any comparison/profiling data if this improves anything?

No, I have not, I'm just trying to sync our code with base :)
The difference was significant, when this lock was a rwlock, with rmlock I think it is less noticeable.

This is a change in good direction, though it would be better if some lockless model was applied to pfil.

With a simple ipfw.conf file:

ipfw add 3000 allow ip from any to any keep-state

I didn't see any difference (I'm building world&kernel using WITH_META_MODE, I hope it didn't impact build):

x head-r316325 (packets-per-second)
+ head-r316325 with D10154 patch (packets-per-second)
+--------------------------------------------------------------------------+
|x     + +                                              x xx  +   + +      |
|                    |________________________A_________M______________|   |
|          |______________________________A___________________M___________||
+--------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x   5       1784669       2015140       2005646     1964435.2     100604.15
+   5       1807598       2051231     2029335.5     1949548.3     126065.06
No difference proven at 95.0% confidence

I didn't see any difference (I'm building world&kernel using WITH_META_MODE, I hope it didn't impact build):

I think some performance increasing is possible with static rules.

In D10154#211425, @ae wrote:

I think some performance increasing is possible with static rules.

What do you mean by "static rules" ?

In D10154#211425, @ae wrote:

I think some performance increasing is possible with static rules.

What do you mean by "static rules" ?

rules, that do not create dynamic states. I.e. rules without keep-state and limit keyword.

Ok then with one stateless rules:

ipfw add 3000 allow ip from any to any

It seems there is a small improvement (but not enough for 95.0% confidence):

x r316325 (pps)
+ r316325 with D10154 (pps)
+--------------------------------------------------------------------------+
|x+                                           x x  x      +++ +            |
|                 |____________________A________M___________|              |
|                      |________________________A__________M______________||
+--------------------------------------------------------------------------+
    N           Min           Max        Median           Avg        Stddev
x   5       2253260       2583506       2564852     2503733.8     140517.37
+   5       2263084       2655258       2641113     2567190.2     170216.89
No difference proven at 95.0% confidence

graph.png (1×1 px, 38 KB)

This revision was automatically updated to reflect the committed changes.