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 <[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).
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
> > -#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--;
> > }
> >
> >
> >
>
