Date: Thu, 5 Jun 2014 18:58:06 +0200 From: 'Luigi Rizzo' <rizzo@iet.unipi.it> To: bycn82 <bycn82@gmail.com> Cc: "'Alexander V. Chernikov'" <melifaro@ipfw.ru>, 'FreeBSD Net' <net@FreeBSD.org> Subject: Re: [CFT]: ipfw named tables / different tabletypes Message-ID: <20140605165806.GC81905@onelab2.iet.unipi.it> In-Reply-To: <000401cf80d8$ad1bb840$075328c0$@gmail.com> References: <20140522154740.GA76448@onelab2.iet.unipi.it> <537E2153.1040005@FreeBSD.org> <20140522163812.GA77634@onelab2.iet.unipi.it> <538B2FE5.6070407@FreeBSD.org> <539044E4.1020904@ipfw.ru> <000c01cf80be$41194370$c34bca50$@gmail.com> <20140605134256.GA81234@onelab2.iet.unipi.it> <000001cf80cd$5dc1d9b0$19458d10$@gmail.com> <20140605155402.GA81905@onelab2.iet.unipi.it> <000401cf80d8$ad1bb840$075328c0$@gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Fri, Jun 06, 2014 at 12:10:26AM +0800, bycn82 wrote:
> Sorry for waste you time to explain it again, I will read the code first.
>
> And the latest patch of `PPS` should be OK, I checked the logic carefully this time. I sent it out last weekend.
>
> logic as below, PPS actually will be fulfilled using `PPT`,(N packets per M ticks).
Bill,
sorry but even this version has multiple issues -- performance,
correctness, style.
performance:
duration_in_ticks and limit do not need to be recomputed
on every packet, just once when you install the rule.
correctness
limit should be not lower than the user-specified value,
so if the equation has the form \lceil{a}{b} \rceil,
the correct computation using integer arithmetic is
(a + b - 1)/b and not (a + b/2) / b
You should also check for corner cases, overflows etc.
I haven't actually checked that the equation you use is
correct, so a comment explaining why it is would be appropriate.
BTW you have no checks on the input arguments, so you can
have duration =0 and then you have a division by 0 in the kernel.
style
boring as it might be, we use spaces around keywords and { }
I am also unhappy on the ppt option, because the duration of a tick
can change across reboots, so this option would give variable
behaviour. With all sources of uncertainty on packet arrivals
and when the timer fires to advance 'ticks', there is really
no point in having PPT to avoid the rounding error with some
strange values of HZ.
Now before you think i am too picky:
some of the above (style) are trivial issues, but they require
manual editing of the patch before applying and this is the reason
many project and people usually bounce patches just because of style
(the process does not scale otherwise).
If it were just for that, i would not mind.
But many of the other problems are more serious, and the fact they
keep coming out after so many iterations suggest me that _you_
should spend more time reviewing your code before submitting it
and asking others to find and fix problems.
cheers
luigi
>
> + case O_PPT:
> + case O_PPS:{
> + ipfw_insn_pps *pps = (ipfw_insn_pps *)cmd;
> + if(pps->limit==0){
> + int limit,duration_in_ticks;
> + if(1000/hz > pps->duration){
> + duration_in_ticks=1;
> + }else{
> + duration_in_ticks=(pps->duration * hz +500)/1000;
> + }
> + limit=(cmd->arg1 *1000 * duration_in_ticks + hz * pps->duration/2)/(hz * pps->duration);
> + pps->limit=limit;
> + pps->duration_in_ticks=duration_in_ticks;
> + }
> + if(pps->start_time+pps->duration_in_ticks>= ticks){
> + if(pps->count < pps->limit){
> + retval = IP_FW_PASS;
> + }else{
> + retval = IP_FW_DENY;
> + }
> + pps->count++;
> + }else{
> + pps->start_time=ticks;
> + pps->count=1;
> + retval = IP_FW_PASS;
> + }
> + l = 0;
> + done = 1;
> + break;
> + }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20140605165806.GC81905>