Re: remove pf_check_congestion()

2014-03-07 Thread Henning Brauer 
* Ted Unangst t...@tedunangst.com [2014-03-07 07:40]:
 On Thu, Mar 06, 2014 at 23:56, Lawrence Teo wrote:
  pf_check_congestion() simply checks if ifq-ifq_congestion is non-zero,
  and returns 1 or 0 accordingly.  It is only called by pf_test_rule().
  
  Since what pf_check_congestion() does is very trivial and pf_test_rule()
  is its only user, would it make sense to remove it and let
  pf_test_rule() check ifq-ifq_congestion directly to save a function
  call?
 
 I think function calls are not so very expensive, and it makes it
 easier to read imo.

exactly.

making in static inline would be the max I'd find acceptable - but I'm
certain you won't be able to demonstrate any performance benefit
(previous profiling is pretty clear on that).

-- 
Henning Brauer, h...@bsws.de, henn...@openbsd.org
BS Web Services GmbH, http://bsws.de, Full-Service ISP
Secure Hosting, Mail and DNS Services. Dedicated Servers, Root to Fully Managed
Henning Brauer Consulting, http://henningbrauer.com/

Re: remove pf_check_congestion()

2014-03-07 Thread Theo de Raadt 
 * Ted Unangst t...@tedunangst.com [2014-03-07 07:40]:
  On Thu, Mar 06, 2014 at 23:56, Lawrence Teo wrote:
   pf_check_congestion() simply checks if ifq-ifq_congestion is non-zero,
   and returns 1 or 0 accordingly.  It is only called by pf_test_rule().
   
   Since what pf_check_congestion() does is very trivial and pf_test_rule()
   is its only user, would it make sense to remove it and let
   pf_test_rule() check ifq-ifq_congestion directly to save a function
   call?
  
  I think function calls are not so very expensive, and it makes it
  easier to read imo.
 
 exactly.
 
 making in static inline would be the max I'd find acceptable - but I'm
 certain you won't be able to demonstrate any performance benefit
 (previous profiling is pretty clear on that).

I have checked my old mail.  The original reason for making this a
function is that we thought there might be other conditions, rather
than just a variable.  That might still happen in the future.

Re: remove pf_check_congestion()

2014-03-07 Thread Lawrence Teo 
On Fri, Mar 07, 2014 at 10:22:59AM -0700, Theo de Raadt wrote:
  * Ted Unangst t...@tedunangst.com [2014-03-07 07:40]:
   On Thu, Mar 06, 2014 at 23:56, Lawrence Teo wrote:
pf_check_congestion() simply checks if ifq-ifq_congestion is non-zero,
and returns 1 or 0 accordingly.  It is only called by pf_test_rule().

Since what pf_check_congestion() does is very trivial and pf_test_rule()
is its only user, would it make sense to remove it and let
pf_test_rule() check ifq-ifq_congestion directly to save a function
call?
   
   I think function calls are not so very expensive, and it makes it
   easier to read imo.
  
  exactly.
  
  making in static inline would be the max I'd find acceptable - but I'm
  certain you won't be able to demonstrate any performance benefit
  (previous profiling is pretty clear on that).
 
 I have checked my old mail.  The original reason for making this a
 function is that we thought there might be other conditions, rather
 than just a variable.  That might still happen in the future.

Ah, makes sense now.  Thanks for clarifying.

remove pf_check_congestion()

2014-03-06 Thread Lawrence Teo 
pf_check_congestion() simply checks if ifq-ifq_congestion is non-zero,
and returns 1 or 0 accordingly.  It is only called by pf_test_rule().

Since what pf_check_congestion() does is very trivial and pf_test_rule()
is its only user, would it make sense to remove it and let
pf_test_rule() check ifq-ifq_congestion directly to save a function
call?


Index: pf.c
===
RCS file: /cvs/src/sys/net/pf.c,v
retrieving revision 1.868
diff -u -p -r1.868 pf.c
--- pf.c25 Jan 2014 03:39:00 -  1.868
+++ pf.c18 Feb 2014 17:57:42 -
@@ -230,7 +230,6 @@ int  pf_compare_state_keys(struct pf_s
 struct pf_state*pf_find_state(struct pfi_kif *,
struct pf_state_key_cmp *, u_int, struct mbuf *);
 int pf_src_connlimit(struct pf_state **);
-int pf_check_congestion(struct ifqueue *);
 int pf_match_rcvif(struct mbuf *, struct pf_rule *);
 voidpf_step_into_anchor(int *, struct pf_ruleset **,
struct pf_rule **, struct pf_rule **);
@@ -3159,7 +3158,7 @@ pf_test_rule(struct pf_pdesc *pd, struct
ifq = ip6intrq;
 #endif
 
-   if (pd-dir == PF_IN  pf_check_congestion(ifq)) {
+   if (pd-dir == PF_IN  ifq-ifq_congestion) {
REASON_SET(reason, PFRES_CONGEST);
return (PF_DROP);
}
@@ -6745,15 +6744,6 @@ done:
}
 
return (action);
-}
-
-int
-pf_check_congestion(struct ifqueue *ifq)
-{
-   if (ifq-ifq_congestion)
-   return (1);
-   else
-   return (0);
 }
 
 void