> Date: Wed, 6 Mar 2013 15:25:34 +0100
> From: Martin Pieuchot <[email protected]>
>
> 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 <[email protected]> wrote:
> > > >> Date: Tue, 5 Mar 2013 11:36:36 +0100
> > > >> From: Martin Pieuchot <[email protected]>
> > > >>
> > > >> 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).
> 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--;
> }
>
>
>
