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
-#define IFAFREE(ifa) \
-do { \
- if ((ifa)->ifa_refcnt <= 0) \
- ifafree(ifa); \
- else \
- (ifa)->ifa_refcnt--; \
-} while (/* CONSTCOND */0)
-
#ifdef ALTQ
#define IFQ_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
+++ sys/netinet/if_ether.c
@@ -298,7 +298,7 @@ arp_rtrequest(int req, struct rtentry *rt, struct
rt_addrinfo *info)
*/
ifa = &ia->ia_ifa;
if (ifa != rt->rt_ifa) {
- IFAFREE(rt->rt_ifa);
+ ifafree(rt->rt_ifa);
ifa->ifa_refcnt++;
rt->rt_ifa = ifa;
}
diff --git sys/netinet/in.c sys/netinet/in.c
index 9429a05..1fa93cf 100644
--- sys/netinet/in.c
+++ sys/netinet/in.c
@@ -427,7 +427,7 @@ cleanup:
}
/* remove backpointer, since ifp may die before ia */
ia->ia_ifp = NULL;
- IFAFREE((&ia->ia_ifa));
+ ifafree((&ia->ia_ifa));
if (!error)
dohooks(ifp->if_addrhooks, 0);
splx(s);
@@ -965,7 +965,7 @@ in_addmulti(struct in_addr *ap, struct ifnet *ifp)
if ((ifp->if_ioctl == NULL) ||
(*ifp->if_ioctl)(ifp, SIOCADDMULTI,(caddr_t)&ifr) != 0) {
LIST_REMOVE(inm, inm_list);
- IFAFREE(&inm->inm_ia->ia_ifa);
+ ifafree(&inm->inm_ia->ia_ifa);
free(inm, M_IPMADDR);
splx(s);
return (NULL);
@@ -1000,7 +1000,7 @@ in_delmulti(struct in_multi *inm)
*/
LIST_REMOVE(inm, inm_list);
ifp = inm->inm_ia->ia_ifp;
- IFAFREE(&inm->inm_ia->ia_ifa);
+ ifafree(&inm->inm_ia->ia_ifa);
if (ifp) {
/*
diff --git sys/netinet6/in6.c sys/netinet6/in6.c
index 4f827d7..fe4c708 100644
--- sys/netinet6/in6.c
+++ sys/netinet6/in6.c
@@ -195,7 +195,7 @@ in6_ifloop_request(int cmd, struct ifaddr *ifa)
* of the loopback address.
*/
if (cmd == RTM_ADD && nrt && ifa != nrt->rt_ifa) {
- IFAFREE(nrt->rt_ifa);
+ ifafree(nrt->rt_ifa);
ifa->ifa_refcnt++;
nrt->rt_ifa = ifa;
}
@@ -1277,7 +1277,7 @@ in6_unlink_ifa(struct in6_ifaddr *ia, struct ifnet *ifp)
* release another refcnt for the link from in6_ifaddr.
* Note that we should decrement the refcnt at least once for all *BSD.
*/
- IFAFREE(&oia->ia_ifa);
+ ifafree(&oia->ia_ifa);
splx(s);
}
@@ -1597,7 +1597,7 @@ in6_savemkludge(struct in6_ifaddr *oia)
for (in6m = LIST_FIRST(&oia->ia6_multiaddrs);
in6m != LIST_END(&oia->ia6_multiaddrs); in6m = next) {
next = LIST_NEXT(in6m, in6m_entry);
- IFAFREE(&in6m->in6m_ia->ia_ifa);
+ ifafree(&in6m->in6m_ia->ia_ifa);
ia->ia_ifa.ifa_refcnt++;
in6m->in6m_ia = ia;
LIST_INSERT_HEAD(&ia->ia6_multiaddrs, in6m, in6m_entry);
@@ -1615,7 +1615,7 @@ in6_savemkludge(struct in6_ifaddr *oia)
for (in6m = LIST_FIRST(&oia->ia6_multiaddrs);
in6m != LIST_END(&oia->ia6_multiaddrs); in6m = next) {
next = LIST_NEXT(in6m, in6m_entry);
- IFAFREE(&in6m->in6m_ia->ia_ifa); /* release reference */
+ ifafree(&in6m->in6m_ia->ia_ifa); /* release reference */
in6m->in6m_ia = NULL;
LIST_INSERT_HEAD(&mk->mk_head, in6m, in6m_entry);
}
@@ -1763,7 +1763,7 @@ in6_addmulti(struct in6_addr *maddr6, struct ifnet *ifp,
int *errorp)
if (*errorp) {
LIST_REMOVE(in6m, in6m_entry);
free(in6m, M_IPMADDR);
- IFAFREE(&ia->ia_ifa);
+ ifafree(&ia->ia_ifa);
splx(s);
return (NULL);
}
@@ -1798,7 +1798,7 @@ in6_delmulti(struct in6_multi *in6m)
*/
LIST_REMOVE(in6m, in6m_entry);
if (in6m->in6m_ia) {
- IFAFREE(&in6m->in6m_ia->ia_ifa); /* release reference */
+ ifafree(&in6m->in6m_ia->ia_ifa); /* release reference */
}
/*
diff --git sys/netinet6/in6_ifattach.c sys/netinet6/in6_ifattach.c
index 1c732d4..7661e04 100644
--- sys/netinet6/in6_ifattach.c
+++ sys/netinet6/in6_ifattach.c
@@ -719,7 +719,7 @@ in6_ifdetach(struct ifnet *ifp)
/* remove from the linked list */
ifa_del(ifp, &ia->ia_ifa);
- IFAFREE(&ia->ia_ifa);
+ ifafree(&ia->ia_ifa);
/* also remove from the IPv6 address chain(itojun&jinmei) */
oia = ia;
@@ -737,7 +737,7 @@ in6_ifdetach(struct ifnet *ifp)
}
}
- IFAFREE(&oia->ia_ifa);
+ ifafree(&oia->ia_ifa);
}
/* cleanup multicast address kludge table, if there is any */
diff --git sys/netinet6/nd6.c sys/netinet6/nd6.c
index efa4ed2..9689cc9 100644
--- sys/netinet6/nd6.c
+++ sys/netinet6/nd6.c
@@ -1198,7 +1198,7 @@ nd6_rtrequest(int req, struct rtentry *rt, struct
rt_addrinfo *info)
* of the loopback address.
*/
if (ifa != rt->rt_ifa) {
- IFAFREE(rt->rt_ifa);
+ ifafree(rt->rt_ifa);
ifa->ifa_refcnt++;
rt->rt_ifa = ifa;
}
diff --git sys/netinet6/nd6_nbr.c sys/netinet6/nd6_nbr.c
index d177cb0..b838f2a 100644
--- sys/netinet6/nd6_nbr.c
+++ sys/netinet6/nd6_nbr.c
@@ -1207,7 +1207,7 @@ nd6_dad_stop(struct ifaddr *ifa)
TAILQ_REMOVE(&dadq, (struct dadq *)dp, dad_list);
free(dp, M_IP6NDP);
dp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
ip6_dad_pending--;
}
@@ -1253,7 +1253,7 @@ nd6_dad_timer(struct ifaddr *ifa)
TAILQ_REMOVE(&dadq, (struct dadq *)dp, dad_list);
free(dp, M_IP6NDP);
dp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
ip6_dad_pending--;
goto done;
}
@@ -1307,7 +1307,7 @@ nd6_dad_timer(struct ifaddr *ifa)
TAILQ_REMOVE(&dadq, (struct dadq *)dp, dad_list);
free(dp, M_IP6NDP);
dp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
ip6_dad_pending--;
}
}
@@ -1347,7 +1347,7 @@ nd6_dad_duplicated(struct ifaddr *ifa)
TAILQ_REMOVE(&dadq, (struct dadq *)dp, dad_list);
free(dp, M_IP6NDP);
dp = NULL;
- IFAFREE(ifa);
+ ifafree(ifa);
ip6_dad_pending--;
}
