Re: Kill IFAFREE()
On 05/03/13(Tue) 21:57, Claudio Jeker wrote:
On Tue, Mar 05, 2013 at 12:03:49PM +0100, Mike Belopuhov wrote:
On 5 March 2013 11:55, Mark Kettenis mark.kette...@xs4all.nl wrote:
Date: Tue, 5 Mar 2013 11:36:36 +0100
From: Martin Pieuchot mpieuc...@nolizard.org
The ifaddr structure contains a reference counter and two different way
to check it before freeing its memory: a macro IFAFREE(), and a function
ifafree().
Because the former calls the latter when the reference counter is null,
and then also check for the reference counter, I see no point in keeping
two ways to do the same thing.
Well, the point is probably that by doing the refcount check in the
macro you avoid a function call in most cases. It's very well
possible that this is a case of premature optimization. Almost
certainly the case unless the macro is called in a
performance-critical path. If it is called in a performance-critical
path, some benchmarking should probably be done to make sure this
doesn't impact something like packet forwarding performance in a
negative way.
to be fair, there are millions of these function calls. i highly
doubt one can measure any performance difference -- it'll all be
within error margin.
If we do IFAFREE() on a per packet basis then we do something wrong.
Glancing at the diff I see no hot pathes that would matter.
Exactly, we are unrefcounting address descriptors when detaching an
interface, removing addresses, modifying routes and obviously in some
ioctl() code paths.
So, ok?
Martin
diff --git sys/net/if.c sys/net/if.c
index 534d434..3edd0a7 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -599,7 +599,7 @@ do { \
continue;
ifa-ifa_ifp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
}
for (ifg = TAILQ_FIRST(ifp-if_groups); ifg;
@@ -609,7 +609,7 @@ do { \
if_free_sadl(ifp);
ifnet_addrs[ifp-if_index]-ifa_ifp = NULL;
- IFAFREE(ifnet_addrs[ifp-if_index]);
+ ifafree(ifnet_addrs[ifp-if_index]);
ifnet_addrs[ifp-if_index] = NULL;
free(ifp-if_addrhooks, M_TEMP);
@@ -1007,7 +1007,7 @@ link_rtrequest(int cmd, struct rtentry *rt, struct
rt_addrinfo *info)
return;
if ((ifa = ifaof_ifpforaddr(dst, ifp)) != NULL) {
ifa-ifa_refcnt++;
- IFAFREE(rt-rt_ifa);
+ ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
if (ifa-ifa_rtrequest ifa-ifa_rtrequest != link_rtrequest)
ifa-ifa_rtrequest(cmd, rt, info);
@@ -1515,7 +1515,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
(struct in_ifaddr *)ifa, ia_list);
ifa_del(ifp, ifa);
ifa-ifa_ifp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
}
#endif
splx(s);
diff --git sys/net/if.h sys/net/if.h
index 26ea6b1..27b209b 100644
--- sys/net/if.h
+++ sys/net/if.h
@@ -704,14 +704,6 @@ struct if_laddrreq {
#include net/if_arp.h
#ifdef _KERNEL
-#defineIFAFREE(ifa) \
-do { \
- if ((ifa)-ifa_refcnt = 0) \
- ifafree(ifa); \
- else \
- (ifa)-ifa_refcnt--; \
-} while (/* CONSTCOND */0)
-
#ifdef ALTQ
#defineIFQ_ENQUEUE(ifq, m, pattr, err)
\
diff --git sys/net/route.c sys/net/route.c
index 9ec8a47..a0dc710 100644
--- sys/net/route.c
+++ sys/net/route.c
@@ -401,7 +401,7 @@ rtfree(struct rtentry *rt)
rt_timer_remove_all(rt);
ifa = rt-rt_ifa;
if (ifa)
- IFAFREE(ifa);
+ ifafree(ifa);
rtlabel_unref(rt-rt_labelid);
#ifdef MPLS
if (rt-rt_flags RTF_MPLS)
@@ -926,7 +926,7 @@ rtrequest1(int req, struct rt_addrinfo *info, u_int8_t prio,
if ((*ret_nrt)-rt_ifa-ifa_rtrequest)
(*ret_nrt)-rt_ifa-ifa_rtrequest(
RTM_DELETE, *ret_nrt, NULL);
- IFAFREE((*ret_nrt)-rt_ifa);
+ ifafree((*ret_nrt)-rt_ifa);
(*ret_nrt)-rt_ifa = ifa;
(*ret_nrt)-rt_ifp = ifa-ifa_ifp;
ifa-ifa_refcnt++;
@@ -957,7 +957,7 @@ rtrequest1(int req, struct rt_addrinfo *info, u_int8_t prio,
RTFREE(crt);
}
if (rn == 0) {
- IFAFREE(ifa);
+ ifafree(ifa);
if ((rt-rt_flags RTF_CLONED) != 0 rt-rt_parent)
rtfree(rt-rt_parent);
if (rt-rt_gwroute)
@@ -1139,7 +1139,7 @@
Re: Kill IFAFREE()
On Wed, Mar 06, 2013 at 03:58:22PM +0100, Mark Kettenis wrote:
Date: Wed, 6 Mar 2013 15:25:34 +0100
From: Martin Pieuchot mpieuc...@nolizard.org
On 05/03/13(Tue) 21:57, Claudio Jeker wrote:
On Tue, Mar 05, 2013 at 12:03:49PM +0100, Mike Belopuhov wrote:
On 5 March 2013 11:55, Mark Kettenis mark.kette...@xs4all.nl wrote:
Date: Tue, 5 Mar 2013 11:36:36 +0100
From: Martin Pieuchot mpieuc...@nolizard.org
The ifaddr structure contains a reference counter and two different
way
to check it before freeing its memory: a macro IFAFREE(), and a
function
ifafree().
Because the former calls the latter when the reference counter is
null,
and then also check for the reference counter, I see no point in
keeping
two ways to do the same thing.
Well, the point is probably that by doing the refcount check in the
macro you avoid a function call in most cases. It's very well
possible that this is a case of premature optimization. Almost
certainly the case unless the macro is called in a
performance-critical path. If it is called in a performance-critical
path, some benchmarking should probably be done to make sure this
doesn't impact something like packet forwarding performance in a
negative way.
to be fair, there are millions of these function calls. i highly
doubt one can measure any performance difference -- it'll all be
within error margin.
If we do IFAFREE() on a per packet basis then we do something wrong.
Glancing at the diff I see no hot pathes that would matter.
Exactly, we are unrefcounting address descriptors when detaching an
interface, removing addresses, modifying routes and obviously in some
ioctl() code paths.
So, ok?
I'm satisfied with the explanation. Premature optimisation is the
verdict and we all know that that's evil ;).
so ok kettenis@ (but you probably should get an ok from a real
networking person as well).
ok krw@ with same condition. :-)
Ken
diff --git sys/net/if.c sys/net/if.c
index 534d434..3edd0a7 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -599,7 +599,7 @@ do { \
continue;
ifa-ifa_ifp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
}
for (ifg = TAILQ_FIRST(ifp-if_groups); ifg;
@@ -609,7 +609,7 @@ do { \
if_free_sadl(ifp);
ifnet_addrs[ifp-if_index]-ifa_ifp = NULL;
- IFAFREE(ifnet_addrs[ifp-if_index]);
+ ifafree(ifnet_addrs[ifp-if_index]);
ifnet_addrs[ifp-if_index] = NULL;
free(ifp-if_addrhooks, M_TEMP);
@@ -1007,7 +1007,7 @@ link_rtrequest(int cmd, struct rtentry *rt, struct
rt_addrinfo *info)
return;
if ((ifa = ifaof_ifpforaddr(dst, ifp)) != NULL) {
ifa-ifa_refcnt++;
- IFAFREE(rt-rt_ifa);
+ ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
if (ifa-ifa_rtrequest ifa-ifa_rtrequest != link_rtrequest)
ifa-ifa_rtrequest(cmd, rt, info);
@@ -1515,7 +1515,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
(struct in_ifaddr *)ifa, ia_list);
ifa_del(ifp, ifa);
ifa-ifa_ifp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
}
#endif
splx(s);
diff --git sys/net/if.h sys/net/if.h
index 26ea6b1..27b209b 100644
--- sys/net/if.h
+++ sys/net/if.h
@@ -704,14 +704,6 @@ struct if_laddrreq {
#include net/if_arp.h
#ifdef _KERNEL
-#defineIFAFREE(ifa) \
-do { \
- if ((ifa)-ifa_refcnt = 0) \
- ifafree(ifa); \
- else \
- (ifa)-ifa_refcnt--; \
-} while (/* CONSTCOND */0)
-
#ifdef ALTQ
#defineIFQ_ENQUEUE(ifq, m, pattr, err)
\
diff --git sys/net/route.c sys/net/route.c
index 9ec8a47..a0dc710 100644
--- sys/net/route.c
+++ sys/net/route.c
@@ -401,7 +401,7 @@ rtfree(struct rtentry *rt)
rt_timer_remove_all(rt);
ifa = rt-rt_ifa;
if (ifa)
- IFAFREE(ifa);
+ ifafree(ifa);
rtlabel_unref(rt-rt_labelid);
#ifdef MPLS
if (rt-rt_flags RTF_MPLS)
@@ -926,7 +926,7 @@ rtrequest1(int req, struct rt_addrinfo *info, u_int8_t
prio,
if ((*ret_nrt)-rt_ifa-ifa_rtrequest)
(*ret_nrt)-rt_ifa-ifa_rtrequest(
RTM_DELETE, *ret_nrt, NULL);
- IFAFREE((*ret_nrt)-rt_ifa);
+ ifafree((*ret_nrt)-rt_ifa);
(*ret_nrt)-rt_ifa = ifa;
(*ret_nrt)-rt_ifp = ifa-ifa_ifp;
Kill IFAFREE()
The ifaddr structure contains a reference counter and two different way
to check it before freeing its memory: a macro IFAFREE(), and a function
ifafree().
Because the former calls the latter when the reference counter is null,
and then also check for the reference counter, I see no point in keeping
two ways to do the same thing.
The diff below kills the IFAFREE() macro and replace it by the function.
ok?
diff --git sys/net/if.c sys/net/if.c
index 534d434..3edd0a7 100644
--- sys/net/if.c
+++ sys/net/if.c
@@ -599,7 +599,7 @@ do { \
continue;
ifa-ifa_ifp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
}
for (ifg = TAILQ_FIRST(ifp-if_groups); ifg;
@@ -609,7 +609,7 @@ do { \
if_free_sadl(ifp);
ifnet_addrs[ifp-if_index]-ifa_ifp = NULL;
- IFAFREE(ifnet_addrs[ifp-if_index]);
+ ifafree(ifnet_addrs[ifp-if_index]);
ifnet_addrs[ifp-if_index] = NULL;
free(ifp-if_addrhooks, M_TEMP);
@@ -1007,7 +1007,7 @@ link_rtrequest(int cmd, struct rtentry *rt, struct
rt_addrinfo *info)
return;
if ((ifa = ifaof_ifpforaddr(dst, ifp)) != NULL) {
ifa-ifa_refcnt++;
- IFAFREE(rt-rt_ifa);
+ ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
if (ifa-ifa_rtrequest ifa-ifa_rtrequest != link_rtrequest)
ifa-ifa_rtrequest(cmd, rt, info);
@@ -1515,7 +1515,7 @@ ifioctl(struct socket *so, u_long cmd, caddr_t data,
struct proc *p)
(struct in_ifaddr *)ifa, ia_list);
ifa_del(ifp, ifa);
ifa-ifa_ifp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
}
#endif
splx(s);
diff --git sys/net/if.h sys/net/if.h
index 26ea6b1..27b209b 100644
--- sys/net/if.h
+++ sys/net/if.h
@@ -704,14 +704,6 @@ struct if_laddrreq {
#include net/if_arp.h
#ifdef _KERNEL
-#defineIFAFREE(ifa) \
-do { \
- if ((ifa)-ifa_refcnt = 0) \
- ifafree(ifa); \
- else \
- (ifa)-ifa_refcnt--; \
-} while (/* CONSTCOND */0)
-
#ifdef ALTQ
#defineIFQ_ENQUEUE(ifq, m, pattr, err)
\
diff --git sys/net/route.c sys/net/route.c
index 9ec8a47..a0dc710 100644
--- sys/net/route.c
+++ sys/net/route.c
@@ -401,7 +401,7 @@ rtfree(struct rtentry *rt)
rt_timer_remove_all(rt);
ifa = rt-rt_ifa;
if (ifa)
- IFAFREE(ifa);
+ ifafree(ifa);
rtlabel_unref(rt-rt_labelid);
#ifdef MPLS
if (rt-rt_flags RTF_MPLS)
@@ -926,7 +926,7 @@ rtrequest1(int req, struct rt_addrinfo *info, u_int8_t prio,
if ((*ret_nrt)-rt_ifa-ifa_rtrequest)
(*ret_nrt)-rt_ifa-ifa_rtrequest(
RTM_DELETE, *ret_nrt, NULL);
- IFAFREE((*ret_nrt)-rt_ifa);
+ ifafree((*ret_nrt)-rt_ifa);
(*ret_nrt)-rt_ifa = ifa;
(*ret_nrt)-rt_ifp = ifa-ifa_ifp;
ifa-ifa_refcnt++;
@@ -957,7 +957,7 @@ rtrequest1(int req, struct rt_addrinfo *info, u_int8_t prio,
RTFREE(crt);
}
if (rn == 0) {
- IFAFREE(ifa);
+ ifafree(ifa);
if ((rt-rt_flags RTF_CLONED) != 0 rt-rt_parent)
rtfree(rt-rt_parent);
if (rt-rt_gwroute)
@@ -1139,7 +1139,7 @@ rtinit(struct ifaddr *ifa, int cmd, int flags)
ifa, rt-rt_ifa);
if (rt-rt_ifa-ifa_rtrequest)
rt-rt_ifa-ifa_rtrequest(RTM_DELETE, rt, NULL);
- IFAFREE(rt-rt_ifa);
+ ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
rt-rt_ifp = ifa-ifa_ifp;
ifa-ifa_refcnt++;
diff --git sys/net/rtsock.c sys/net/rtsock.c
index 091ec53..e8784ea 100644
--- sys/net/rtsock.c
+++ sys/net/rtsock.c
@@ -785,7 +785,7 @@ report:
if (oifa oifa-ifa_rtrequest)
oifa-ifa_rtrequest(RTM_DELETE, rt,
info);
- IFAFREE(rt-rt_ifa);
+ ifafree(rt-rt_ifa);
rt-rt_ifa = ifa;
ifa-ifa_refcnt++;
rt-rt_ifp = ifp;
diff --git sys/netinet/if_ether.c sys/netinet/if_ether.c
index 83e4204..09c2f5a 100644
--- sys/netinet/if_ether.c
+++
Re: Kill IFAFREE()
On 5 March 2013 11:55, Mark Kettenis mark.kette...@xs4all.nl wrote: Date: Tue, 5 Mar 2013 11:36:36 +0100 From: Martin Pieuchot mpieuc...@nolizard.org The ifaddr structure contains a reference counter and two different way to check it before freeing its memory: a macro IFAFREE(), and a function ifafree(). Because the former calls the latter when the reference counter is null, and then also check for the reference counter, I see no point in keeping two ways to do the same thing. Well, the point is probably that by doing the refcount check in the macro you avoid a function call in most cases. It's very well possible that this is a case of premature optimization. Almost certainly the case unless the macro is called in a performance-critical path. If it is called in a performance-critical path, some benchmarking should probably be done to make sure this doesn't impact something like packet forwarding performance in a negative way. to be fair, there are millions of these function calls. i highly doubt one can measure any performance difference -- it'll all be within error margin.
