Bug 189720 - [ipfw] [patch] pps action for ipfw
Summary: [ipfw] [patch] pps action for ipfw
Status: Closed Feedback Timeout
Alias: None
Product: Base System
Classification: Unclassified
Component: kern (show other bugs)
Version: Unspecified
Hardware: Any Any
: Normal Affects Only Me
Assignee: freebsd-ipfw (Nobody)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-05-12 16:40 UTC by bycn82
Modified: 2021-06-10 08:20 UTC (History)
4 users (show)

See Also:


Attachments
file.diff (4.37 KB, patch)
2014-05-12 16:40 UTC, bycn82
no flags Details | Diff
pps.patch2.txt (4.03 KB, patch)
2014-05-13 03:54 UTC, bycn82
no flags Details | Diff
pps.patch (4.86 KB, patch)
2014-05-30 17:53 UTC, bycn82
no flags Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description bycn82 2014-05-12 16:40:00 UTC
pps action for ipfw
pps: packet for second (millisecond)

usage:
    ipfw add pps 1 50 tcp from any to any


man page

 pps limit duration
             Rule with the pps keyword will allow the first limit packets in
             each duration milliseconds

Fix: Patch attached with submission follows:
Comment 1 Mark Linimon freebsd_committer freebsd_triage 2014-05-13 05:51:40 UTC
Responsible Changed
From-To: freebsd-bugs->freebsd-ipfw

Over to maintainer(s).
Comment 2 rizzo 2014-05-29 15:12:16 UTC
Hi,
I have looked at the update from May 13th but it is not ready yet,
the code assumes HZ=1000 so 1 tick=1ms.

The translation can be done in userspace or in the kernel.
I would prefer the latter.

Please note that the count might need to be adjusted accordingly
if 1/HZ > duration. I covered most of these things in the email
exchange before the patch was submitted.

cheers
luigi
Comment 3 bycn82 2014-05-29 16:06:27 UTC
-----Original Message-----
From: Luigi Rizzo [mailto:rizzo@iet.unipi.it]=20
Sent: 29 May, 2014 22:12
To: bug-followup@FreeBSD.org; bycn82@gmail.com
Subject: kern/189720: [ipfw] [patch] pps action for ipfw

Hi,
I have looked at the update from May 13th but it is not ready yet, the =
code assumes HZ=3D1000 so 1 tick=3D1ms.

The translation can be done in userspace or in the kernel.
I would prefer the latter.
I see,=20
If the HZ=3D3, that means every tick=3D333ms
And if the user wants to =E2=80=9C 1 packet per 500ms=E2=80=9D, then in =
the backend will not do the exactly the same as what user expect.

Actually the implementation should be =E2=80=9Cpackets per =
ticks=E2=80=9D, so how about this? Instead of translate it in codes. Why =
not update the document, and explain it to the user in the document ?

Please note that the count might need to be adjusted accordingly if 1/HZ =
> duration. I covered most of these things in the email exchange before =
the patch was submitted.

cheers
luigi
Comment 4 rizzo 2014-05-29 16:17:59 UTC
On Thu, May 29, 2014 at 11:06:27PM +0800, bycn82 wrote:
> 
> 
> -----Original Message-----
> From: Luigi Rizzo [mailto:rizzo@iet.unipi.it] 
> Sent: 29 May, 2014 22:12
> To: bug-followup@FreeBSD.org; bycn82@gmail.com
> Subject: kern/189720: [ipfw] [patch] pps action for ipfw
> 
> Hi,
> I have looked at the update from May 13th but it is not ready yet, the code assumes HZ=1000 so 1 tick=1ms.
> 
> The translation can be done in userspace or in the kernel.
> I would prefer the latter.
> I see, 
> If the HZ=3, that means every tick=333ms
> And if the user wants to ??? 1 packet per 500ms???, then in the backend will not do the exactly the same as what user expect.
> 
> Actually the implementation should be ???packets per ticks???, so how about this? Instead of translate it in codes. Why not update the document, and explain it to the user in the document ?

'Packets per tick' this is not a useful specification
since the tick's duration is unknown to the user.
Depending on the platform you can have HZ ranging from 15-20 (on windows)
to 10000 or even more. Normal values are 100, 250, 1000 but
you just cannot know what you are going to get.

Yes there are rounding issues, and yes it is boring to write
code to handle them.

luigi
Comment 5 rizzo 2014-05-30 18:16:10 UTC
On Sat, May 31, 2014 at 12:53:56AM +0800, bycn82 wrote:
> 1.       Add static int to store the value of kern.hz

this is unnecessary. There is already a global variable
called "hz" which contains the correct information

> 2.       Convert the duration into number of ticks based on  kern.hz

this is done incorrectly.

First, hz does not change at runtime so it is unnecessary to recompute
the duration on every instace, even more since this is costing you
one division.  You should adjust the value when the rule is injected
in the kernel, perhaps adding a couple of fields in the rule
so you can store the adjusted duration and threshold (see below).

Second, you are still not doing the rounding correctly.
When the requested interval is shorter than a tick, you adjust the
interval but leave the limit unchanged, which means you are reducing
the limit below what the user wants.
Instead you should correct the limit so that it approximates
the desired rate; one above or one below is not a problem,
but your code might be off by a very large factor.

BTW even in the other case you are always adding 1 tick unconditionally.
A correct way to do the rounding is (pps->duration * hz + 999)/1000
(and then again adjust the count according to the actual duration)

cheers
luigi
Comment 6 ArchyCho 2016-01-19 12:08:39 UTC
Hello , I try to recompile the IPFW and kernel with the patch , but no luck to enable the pps limitation of IPFW .

10.2-RELEASE-p10 AMD64

file.diff , patch could be applied but fail to compile ipfw.
pps.patch2.txt , patch could be applied but fail to compile ipfw.
pps.patch , patch could not be applied.

Do anybody send me advise how could I enable `pps` limit for any version of Freebsd ?

Thanks for all and sorry for my poor eng.
Comment 7 Eitan Adler freebsd_committer freebsd_triage 2018-05-28 19:43:31 UTC
batch change:

For bugs that match the following
-  Status Is In progress 
AND
- Untouched since 2018-01-01.
AND
- Affects Base System OR Documentation

DO:

Reset to open status.


Note:
I did a quick pass but if you are getting this email it might be worthwhile to double check to see if this bug ought to be closed.
Comment 8 Lutz Donnerhacke freebsd_committer 2021-05-10 16:13:09 UTC
Because there is no further action in this ticket, please allow me to ask what the purpose of this proposal is.

As far as I understand the desired "Packets per Second" is an unoptimized version of any of the dummynet shapers.  If you aim to add an option for "packets" instead of "bytes" per timeslot, that please state this.