From owner-freebsd-net@FreeBSD.ORG Thu Jun 5 16:53:22 2014 Return-Path: Delivered-To: net@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 5C7107FE for ; Thu, 5 Jun 2014 16:53:22 +0000 (UTC) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id 1A9D722CF for ; Thu, 5 Jun 2014 16:53:21 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 3E09A7300A; Thu, 5 Jun 2014 18:58:06 +0200 (CEST) Date: Thu, 5 Jun 2014 18:58:06 +0200 From: 'Luigi Rizzo' To: bycn82 Subject: Re: [CFT]: ipfw named tables / different tabletypes Message-ID: <20140605165806.GC81905@onelab2.iet.unipi.it> 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <000401cf80d8$ad1bb840$075328c0$@gmail.com> User-Agent: Mutt/1.5.20 (2009-06-14) Cc: "'Alexander V. Chernikov'" , 'FreeBSD Net' X-BeenThere: freebsd-net@freebsd.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Networking and TCP/IP with FreeBSD List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 05 Jun 2014 16:53:22 -0000 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; > + }