From owner-svn-src-all@FreeBSD.ORG Sun Aug 31 08:20:22 2014 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) (using TLSv1 with cipher ADH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id D261E444; Sun, 31 Aug 2014 08:20:22 +0000 (UTC) Received: from mail.ipfw.ru (mail.ipfw.ru [IPv6:2a01:4f8:120:6141::2]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by mx1.freebsd.org (Postfix) with ESMTPS id 54ED71870; Sun, 31 Aug 2014 08:20:22 +0000 (UTC) Received: from [2a02:6b8:0:401:222:4dff:fe50:cd2f] (helo=ptichko.yndx.net) by mail.ipfw.ru with esmtpsa (TLSv1:DHE-RSA-AES128-SHA:128) (Exim 4.82 (FreeBSD)) (envelope-from ) id 1XNwOv-000NN1-VE; Sun, 31 Aug 2014 08:06:02 +0400 Message-ID: <5402DAB7.2040806@FreeBSD.org> Date: Sun, 31 Aug 2014 12:20:07 +0400 From: "Alexander V. Chernikov" User-Agent: Mozilla/5.0 (X11; FreeBSD amd64; rv:24.0) Gecko/20100101 Thunderbird/24.6.0 MIME-Version: 1.0 To: Gleb Smirnoff , src-committers@freebsd.org, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r270870 - head/sys/net References: <201408310646.s7V6kM1X050678@svn.freebsd.org> In-Reply-To: <201408310646.s7V6kM1X050678@svn.freebsd.org> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sun, 31 Aug 2014 08:20:22 -0000 On 31.08.2014 10:46, Gleb Smirnoff wrote: > Author: glebius > Date: Sun Aug 31 06:46:21 2014 > New Revision: 270870 > URL: http://svnweb.freebsd.org/changeset/base/270870 > > Log: > o Remove struct if_data from struct ifnet. Now it is merely API structure > for route(4) socket and ifmib(4) sysctl. > o Move fields from if_data to ifnet, but keep all statistic counters > separate, since they should disappear later. > o Provide function if_data_copy() to fill if_data, utilize it in routing > socket and ifmib handler. > o Provide overridable ifnet(9) method to fetch counters. If no provided, > if_get_counters_compat() would be used, that returns old counters. Thanks! > > Sponsored by: Netflix > Sponsored by: Nginx, Inc. > > Modified: > head/sys/net/if.c > head/sys/net/if_mib.c > head/sys/net/if_var.h > head/sys/net/rtsock.c > > Modified: head/sys/net/if.c > ============================================================================== > --- head/sys/net/if.c Sun Aug 31 06:30:50 2014 (r270869) > +++ head/sys/net/if.c Sun Aug 31 06:46:21 2014 (r270870) > @@ -605,8 +605,7 @@ if_attach_internal(struct ifnet *ifp, in > if_addgroup(ifp, IFG_ALL); > > getmicrotime(&ifp->if_lastchange); > - ifp->if_data.ifi_epoch = time_uptime; > - ifp->if_data.ifi_datalen = sizeof(struct if_data); > + ifp->if_epoch = time_uptime; > > KASSERT((ifp->if_transmit == NULL && ifp->if_qflush == NULL) || > (ifp->if_transmit != NULL && ifp->if_qflush != NULL), > @@ -615,7 +614,10 @@ if_attach_internal(struct ifnet *ifp, in > ifp->if_transmit = if_transmit; > ifp->if_qflush = if_qflush; > } > - > + > + if (ifp->if_get_counter == NULL) > + ifp->if_get_counter = if_get_counter_compat; > + > if (!vmove) { > #ifdef MAC > mac_ifnet_create(ifp); > @@ -1384,6 +1386,77 @@ if_rtdel(struct radix_node *rn, void *ar > } > > /* > + * Return counter values from old racy non-pcpu counters. > + */ > +uint64_t > +if_get_counter_compat(struct ifnet *ifp, ifnet_counter cnt) > +{ > + > + switch (cnt) { > + case IFCOUNTER_IPACKETS: > + return (ifp->if_ipackets); > + case IFCOUNTER_IERRORS: > + return (ifp->if_ierrors); > + case IFCOUNTER_OPACKETS: > + return (ifp->if_opackets); > + case IFCOUNTER_OERRORS: > + return (ifp->if_oerrors); > + case IFCOUNTER_COLLISIONS: > + return (ifp->if_collisions); > + case IFCOUNTER_IBYTES: > + return (ifp->if_ibytes); > + case IFCOUNTER_OBYTES: > + return (ifp->if_obytes); > + case IFCOUNTER_IMCASTS: > + return (ifp->if_imcasts); > + case IFCOUNTER_OMCASTS: > + return (ifp->if_omcasts); > + case IFCOUNTER_IQDROPS: > + return (ifp->if_iqdrops); > + case IFCOUNTER_OQDROPS: > + return (ifp->if_oqdrops); > + case IFCOUNTER_NOPROTO: > + return (ifp->if_noproto); > + } > + panic("%s: unknown counter %d", __func__, cnt); > +} > + > +/* > + * Copy data from ifnet to userland API structure if_data. > + */ > +void > +if_data_copy(struct ifnet *ifp, struct if_data *ifd) > +{ > + > + ifd->ifi_type = ifp->if_type; > + ifd->ifi_physical = 0; > + ifd->ifi_addrlen = ifp->if_addrlen; > + ifd->ifi_hdrlen = ifp->if_hdrlen; > + ifd->ifi_link_state = ifp->if_link_state; > + ifd->ifi_vhid = 0; > + ifd->ifi_datalen = sizeof(struct if_data); > + ifd->ifi_mtu = ifp->if_mtu; > + ifd->ifi_metric = ifp->if_metric; > + ifd->ifi_baudrate = ifp->if_baudrate; > + ifd->ifi_hwassist = ifp->if_hwassist; > + ifd->ifi_epoch = ifp->if_epoch; > + ifd->ifi_lastchange = ifp->if_lastchange; > + > + ifd->ifi_ipackets = ifp->if_get_counter(ifp, IFCOUNTER_IPACKETS); > + ifd->ifi_ierrors = ifp->if_get_counter(ifp, IFCOUNTER_IERRORS); > + ifd->ifi_opackets = ifp->if_get_counter(ifp, IFCOUNTER_OPACKETS); > + ifd->ifi_oerrors = ifp->if_get_counter(ifp, IFCOUNTER_OERRORS); > + ifd->ifi_collisions = ifp->if_get_counter(ifp, IFCOUNTER_COLLISIONS); > + ifd->ifi_ibytes = ifp->if_get_counter(ifp, IFCOUNTER_IBYTES); > + ifd->ifi_obytes = ifp->if_get_counter(ifp, IFCOUNTER_OBYTES); > + ifd->ifi_imcasts = ifp->if_get_counter(ifp, IFCOUNTER_IMCASTS); > + ifd->ifi_omcasts = ifp->if_get_counter(ifp, IFCOUNTER_OMCASTS); > + ifd->ifi_iqdrops = ifp->if_get_counter(ifp, IFCOUNTER_IQDROPS); > + ifd->ifi_oqdrops = ifp->if_get_counter(ifp, IFCOUNTER_OQDROPS); > + ifd->ifi_noproto = ifp->if_get_counter(ifp, IFCOUNTER_NOPROTO); > +} > + > +/* > * Wrapper functions for struct ifnet address list locking macros. These are > * used by kernel modules to avoid encoding programming interface or binary > * interface assumptions that may be violated when kernel-internal locking > @@ -2167,7 +2240,8 @@ ifhwioctl(u_long cmd, struct ifnet *ifp, > break; > > case SIOCGIFPHYS: > - ifr->ifr_phys = ifp->if_physical; > + /* XXXGL: did this ever worked? */ > + ifr->ifr_phys = 0; > break; > > case SIOCGIFDESCR: > @@ -3915,7 +3989,7 @@ if_sendq_prepend(if_t ifp, struct mbuf * > int > if_setifheaderlen(if_t ifp, int len) > { > - ((struct ifnet *)ifp)->if_data.ifi_hdrlen = len; > + ((struct ifnet *)ifp)->if_hdrlen = len; > return (0); > } > > > Modified: head/sys/net/if_mib.c > ============================================================================== > --- head/sys/net/if_mib.c Sun Aug 31 06:30:50 2014 (r270869) > +++ head/sys/net/if_mib.c Sun Aug 31 06:46:21 2014 (r270870) > @@ -99,10 +99,9 @@ sysctl_ifdata(SYSCTL_HANDLER_ARGS) /* XX > bzero(&ifmd, sizeof(ifmd)); > strlcpy(ifmd.ifmd_name, ifp->if_xname, sizeof(ifmd.ifmd_name)); > > -#define COPY(fld) ifmd.ifmd_##fld = ifp->if_##fld > - COPY(pcount); > - COPY(data); > -#undef COPY > + ifmd.ifmd_pcount = ifp->if_pcount; > + if_data_copy(ifp, &ifmd.ifmd_data); > + > ifmd.ifmd_flags = ifp->if_flags | ifp->if_drv_flags; > ifmd.ifmd_snd_len = ifp->if_snd.ifq_len; > ifmd.ifmd_snd_maxlen = ifp->if_snd.ifq_maxlen; > > Modified: head/sys/net/if_var.h > ============================================================================== > --- head/sys/net/if_var.h Sun Aug 31 06:30:50 2014 (r270869) > +++ head/sys/net/if_var.h Sun Aug 31 06:46:21 2014 (r270870) > @@ -94,11 +94,27 @@ VNET_DECLARE(struct pfil_head, link_pfil > #define V_link_pfil_hook VNET(link_pfil_hook) > #endif /* _KERNEL */ > > +typedef enum { > + IFCOUNTER_IPACKETS = 1, > + IFCOUNTER_IERRORS, > + IFCOUNTER_OPACKETS, > + IFCOUNTER_OERRORS, > + IFCOUNTER_COLLISIONS, > + IFCOUNTER_IBYTES, > + IFCOUNTER_OBYTES, > + IFCOUNTER_IMCASTS, > + IFCOUNTER_OMCASTS, > + IFCOUNTER_IQDROPS, > + IFCOUNTER_OQDROPS, > + IFCOUNTER_NOPROTO, > +} ifnet_counter; > + > typedef void (*if_start_fn_t)(struct ifnet *); > typedef int (*if_ioctl_fn_t)(struct ifnet *, u_long, caddr_t); > typedef void (*if_init_fn_t)(void *); > typedef void (*if_qflush_fn_t)(struct ifnet *); > typedef int (*if_transmit_fn_t)(struct ifnet *, struct mbuf *); > +typedef uint64_t (*if_get_counter_t)(struct ifnet *, ifnet_counter); > > /* Opaque object pointing to interface structure (ifnet) */ > typedef void *if_t; > @@ -136,8 +152,21 @@ struct ifnet { > size_t if_linkmiblen; /* length of above data */ > int if_drv_flags; /* driver-managed status flags */ > u_int if_refcount; /* reference count */ > + > + /* These fields are shared with struct if_data. */ > + uint8_t if_type; /* ethernet, tokenring, etc */ > + uint8_t if_addrlen; /* media address length */ > + uint8_t if_hdrlen; /* media header length */ > + uint8_t if_link_state; /* current link state */ > + uint32_t if_spare32; > + uint32_t if_mtu; /* maximum transmission unit */ > + uint32_t if_metric; /* routing metric (external only) */ > + uint64_t if_baudrate; /* linespeed */ > + uint64_t if_hwassist; /* HW offload capabilities, see IFCAP */ > + time_t if_epoch; /* uptime at attach or stat reset */ > + struct timeval if_lastchange; /* time of last administrative change */ > + > struct ifaltq if_snd; /* output queue (includes altq) */ > - struct if_data if_data; /* type information and statistics */ > struct task if_linktask; /* task for link change events */ > > /* Addresses of different protocol families assigned to this if. */ > @@ -190,6 +219,7 @@ struct ifnet { > > void (*if_reassign) /* reassign to vnet routine */ > (struct ifnet *, struct vnet *, char *); > + if_get_counter_t if_get_counter; /* get counter values */ > > /* Stuff that's only temporary and doesn't belong here. */ > u_int if_hw_tsomax; /* tso burst length limit, the minimum > @@ -197,45 +227,31 @@ struct ifnet { > * XXXAO: Have to find a better place > * for it eventually. */ > /* > - * Spare fields are added so that we can modify sensitive data > - * structures without changing the kernel binary interface, and must > - * be used with care where binary compatibility is required. > + * Old, racy and expensive statistics, should not be used in > + * new drivers. > + */ > + uint64_t if_ipackets; /* packets received on interface */ > + uint64_t if_ierrors; /* input errors on interface */ > + uint64_t if_opackets; /* packets sent on interface */ > + uint64_t if_oerrors; /* output errors on interface */ > + uint64_t if_collisions; /* collisions on csma interfaces */ > + uint64_t if_ibytes; /* total number of octets received */ > + uint64_t if_obytes; /* total number of octets sent */ > + uint64_t if_imcasts; /* packets received via multicast */ > + uint64_t if_omcasts; /* packets sent via multicast */ > + uint64_t if_iqdrops; /* dropped on input */ > + uint64_t if_oqdrops; /* dropped on output */ > + uint64_t if_noproto; /* destined for unsupported protocol */ > + > + /* > + * Spare fields to be added before branching a stable branch, so > + * that structure can be enhanced without changing the kernel > + * binary interface. > */ > - char if_cspare[3]; > - int if_ispare[4]; > - void *if_unused[2]; > - void *if_pspare[8]; /* 1 netmap, 7 TDB */ > }; > > #include /* XXXAO: temporary unconditional include */ > > -/* > - * XXX These aliases are terribly dangerous because they could apply > - * to anything. > - */ > -#define if_mtu if_data.ifi_mtu > -#define if_type if_data.ifi_type > -#define if_physical if_data.ifi_physical > -#define if_addrlen if_data.ifi_addrlen > -#define if_hdrlen if_data.ifi_hdrlen > -#define if_metric if_data.ifi_metric > -#define if_link_state if_data.ifi_link_state > -#define if_baudrate if_data.ifi_baudrate > -#define if_hwassist if_data.ifi_hwassist > -#define if_ipackets if_data.ifi_ipackets > -#define if_ierrors if_data.ifi_ierrors > -#define if_opackets if_data.ifi_opackets > -#define if_oerrors if_data.ifi_oerrors > -#define if_collisions if_data.ifi_collisions > -#define if_ibytes if_data.ifi_ibytes > -#define if_obytes if_data.ifi_obytes > -#define if_imcasts if_data.ifi_imcasts > -#define if_omcasts if_data.ifi_omcasts > -#define if_iqdrops if_data.ifi_iqdrops > -#define if_oqdrops if_data.ifi_oqdrops > -#define if_noproto if_data.ifi_noproto > -#define if_lastchange if_data.ifi_lastchange > - > /* for compatibility with other BSDs */ > #define if_addrlist if_addrhead > #define if_list if_link > @@ -513,6 +529,8 @@ typedef void *if_com_alloc_t(u_char type > typedef void if_com_free_t(void *com, u_char type); > void if_register_com_alloc(u_char type, if_com_alloc_t *a, if_com_free_t *f); > void if_deregister_com_alloc(u_char type); > +void if_data_copy(struct ifnet *, struct if_data *); > +uint64_t if_get_counter_compat(struct ifnet *, ifnet_counter); > > #define IF_LLADDR(ifp) \ > LLADDR((struct sockaddr_dl *)((ifp)->if_addr->ifa_addr)) > > Modified: head/sys/net/rtsock.c > ============================================================================== > --- head/sys/net/rtsock.c Sun Aug 31 06:30:50 2014 (r270869) > +++ head/sys/net/rtsock.c Sun Aug 31 06:46:21 2014 (r270870) > @@ -1252,7 +1252,7 @@ rt_ifmsg(struct ifnet *ifp) > ifm = mtod(m, struct if_msghdr *); > ifm->ifm_index = ifp->if_index; > ifm->ifm_flags = ifp->if_flags | ifp->if_drv_flags; > - ifm->ifm_data = ifp->if_data; > + if_data_copy(ifp, &ifm->ifm_data); > ifm->ifm_addrs = 0; > rt_dispatch(m, AF_UNSPEC); > } > @@ -1574,7 +1574,7 @@ sysctl_iflist_ifml(struct ifnet *ifp, st > ifd = &ifm->ifm_data; > } > > - *ifd = ifp->if_data; > + if_data_copy(ifp, ifd); > > /* Some drivers still use ifqueue(9), add its stats. */ > ifd->ifi_oqdrops += ifp->if_snd.ifq_drops; > @@ -1609,7 +1609,7 @@ sysctl_iflist_ifm(struct ifnet *ifp, str > ifd = &ifm->ifm_data; > } > > - *ifd = ifp->if_data; > + if_data_copy(ifp, ifd); > > /* Some drivers still use ifqueue(9), add its stats. */ > ifd->ifi_oqdrops += ifp->if_snd.ifq_drops; > >