Page MenuHomeFreeBSD

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

Authored by ae on Mar 27 2017, 8:45 PM.

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
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ae created this revision.Mar 27 2017, 8:45 PM
adrian added a subscriber: adrian.Mar 27 2017, 10:14 PM

Hi,

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

ae added a comment.Mar 27 2017, 10:56 PM

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.

eri added a subscriber: eri.Mar 28 2017, 12:51 AM

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.

ae added a comment.Mar 28 2017, 9:03 AM
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.

ae added a subscriber: olivier.Mar 28 2017, 9:04 AM

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
ae added a comment.Mar 31 2017, 6:35 PM

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" ?

ae added a comment.Apr 3 2017, 6:14 AM
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.

olivier added a comment.EditedApr 3 2017, 7:55 AM

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

This revision was automatically updated to reflect the committed changes.