From owner-svn-src-all@FreeBSD.ORG Thu Oct 31 18:01:55 2013 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 ESMTP id 7407C9DC; Thu, 31 Oct 2013 18:01:55 +0000 (UTC) (envelope-from luigi@onelab2.iet.unipi.it) Received: from onelab2.iet.unipi.it (onelab2.iet.unipi.it [131.114.59.238]) by mx1.freebsd.org (Postfix) with ESMTP id DB8F6248C; Thu, 31 Oct 2013 18:01:54 +0000 (UTC) Received: by onelab2.iet.unipi.it (Postfix, from userid 275) id 3BA457300A; Thu, 31 Oct 2013 19:03:36 +0100 (CET) Date: Thu, 31 Oct 2013 19:03:36 +0100 From: Luigi Rizzo To: Andre Oppermann Subject: Re: svn commit: r257455 - head/sys/net Message-ID: <20131031180336.GA62132@onelab2.iet.unipi.it> References: <201310311546.r9VFkAIb049844@svn.freebsd.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <201310311546.r9VFkAIb049844@svn.freebsd.org> User-Agent: Mutt/1.5.20 (2009-06-14) Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.14 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: Thu, 31 Oct 2013 18:01:55 -0000 On Thu, Oct 31, 2013 at 03:46:10PM +0000, Andre Oppermann wrote: > Author: andre > Date: Thu Oct 31 15:46:10 2013 > New Revision: 257455 > URL: http://svnweb.freebsd.org/changeset/base/257455 > > Log: > Make struct ifnet readable and comprehensible again by grouping > and ordering related variables, fields and locks next to each > other. Add more comments to variables. > Over time 'ifnet' has accumlated a lot of additional pointers and > functionality in an unstructured way making it quite hard to read > and understand while obfuscating relationships between fields and > variables. > > Quantify the structure size and how bloated it has become. > > This is only a mechanical change in preparation for upcoming > work to make ifnet opaque to drivers and to separate out the > interface queuing. as you do the above I think it would make sense to replace all int/short/long with fixed-size fields as appropriate (and large enough) to make it easier to reason about things such as 'how many flags can i stuff into a field'. The "large enough" part refers to two things: - bitfields containing flags or capabilities have a tendency to overflow (not just in freebsd, linux has the same) requiring KBI changes. We should probably go for 64 bits unless there are compelling space reasons (not for ifnet). - it is useful if certain opaque fields (flow ids, cookies...) can store pointers. Once again, make them at least 64 bit helps cheers luigi > > Sponsored by: The FreeBSD Foundation > > Modified: > head/sys/net/if_var.h > > Modified: head/sys/net/if_var.h > ============================================================================== > --- head/sys/net/if_var.h Thu Oct 31 15:27:39 2013 (r257454) > +++ head/sys/net/if_var.h Thu Oct 31 15:46:10 2013 (r257455) > @@ -96,19 +96,42 @@ VNET_DECLARE(struct pfil_head, link_pfil > /* > * Structure defining a network interface. > * > - * (Would like to call this struct ``if'', but C isn't PL/1.) > + * Size ILP32: 592 (approx) > + * LP64: 1048 (approx) > */ > - > struct ifnet { > + /* General book keeping of interface lists. */ > + TAILQ_ENTRY(ifnet) if_link; /* all struct ifnets are chained */ > + LIST_ENTRY(ifnet) if_clones; /* interfaces of a cloner */ > + TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */ > + /* protected by if_addr_lock */ > + u_char if_alloctype; /* if_type at time of allocation */ > + > + /* Driver and protocol specific information that remains stable. */ > void *if_softc; /* pointer to driver state */ > + void *if_llsoftc; /* link layer softc */ > void *if_l2com; /* pointer to protocol bits */ > - struct vnet *if_vnet; /* pointer to network stack instance */ > - TAILQ_ENTRY(ifnet) if_link; /* all struct ifnets are chained */ > - char if_xname[IFNAMSIZ]; /* external name (name + unit) */ > const char *if_dname; /* driver name */ > int if_dunit; /* unit or IF_DUNIT_NONE */ > + u_short if_index; /* numeric abbreviation for this if */ > + short if_index_reserved; /* spare space to grow if_index */ > + char if_xname[IFNAMSIZ]; /* external name (name + unit) */ > + char *if_description; /* interface description */ > + > + /* Variable fields that are touched by the stack and drivers. */ > + int if_flags; /* up/down, broadcast, etc. */ > + int if_capabilities; /* interface features & capabilities */ > + int if_capenable; /* enabled features & capabilities */ > + void *if_linkmib; /* link-type-specific MIB data */ > + size_t if_linkmiblen; /* length of above data */ > + int if_drv_flags; /* driver-managed status flags */ > u_int if_refcount; /* reference count */ > - struct ifaddrhead if_addrhead; /* linked list of addresses per if */ > + 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. */ > + struct rwlock if_addr_lock; /* lock to protect address lists */ > /* > * if_addrhead is the list of all addresses associated to > * an interface. > @@ -119,21 +142,29 @@ struct ifnet { > * However, access to the AF_LINK address through this > * field is deprecated. Use if_addr or ifaddr_byindex() instead. > */ > - int if_pcount; /* number of promiscuous listeners */ > - struct carp_if *if_carp; /* carp interface structure */ > - struct bpf_if *if_bpf; /* packet filter structure */ > - u_short if_index; /* numeric abbreviation for this if */ > - short if_index_reserved; /* spare space to grow if_index */ > - struct ifvlantrunk *if_vlantrunk; /* pointer to 802.1q data */ > - int if_flags; /* up/down, broadcast, etc. */ > - int if_capabilities; /* interface features & capabilities */ > - int if_capenable; /* enabled features & capabilities */ > - void *if_linkmib; /* link-type-specific MIB data */ > - size_t if_linkmiblen; /* length of above data */ > - struct if_data if_data; > + struct ifaddrhead if_addrhead; /* linked list of addresses per if */ > struct ifmultihead if_multiaddrs; /* multicast addresses configured */ > int if_amcount; /* number of all-multicast requests */ > -/* procedure handles */ > + struct ifaddr *if_addr; /* pointer to link-level address */ > + const u_int8_t *if_broadcastaddr; /* linklevel broadcast bytestring */ > + struct rwlock if_afdata_lock; > + void *if_afdata[AF_MAX]; > + int if_afdata_initialized; > + > + /* Additional features hung off the interface. */ > + u_int if_fib; /* interface FIB */ > + struct vnet *if_vnet; /* pointer to network stack instance */ > + struct vnet *if_home_vnet; /* where this ifnet originates from */ > + struct ifvlantrunk *if_vlantrunk; /* pointer to 802.1q data */ > + struct bpf_if *if_bpf; /* packet filter structure */ > + int if_pcount; /* number of promiscuous listeners */ > + void *if_bridge; /* bridge glue */ > + void *if_lagg; /* lagg glue */ > + void *if_pf_kif; /* pf glue */ > + struct carp_if *if_carp; /* carp interface structure */ > + struct label *if_label; /* interface MAC label */ > + > + /* Various procedures of the layer2 encapsulation and drivers. */ > int (*if_output) /* output routine (enqueue) */ > (struct ifnet *, struct mbuf *, const struct sockaddr *, > struct route *); > @@ -153,39 +184,12 @@ struct ifnet { > (struct ifnet *, struct mbuf *); > void (*if_reassign) /* reassign to vnet routine */ > (struct ifnet *, struct vnet *, char *); > - struct vnet *if_home_vnet; /* where this ifnet originates from */ > - struct ifaddr *if_addr; /* pointer to link-level address */ > - void *if_llsoftc; /* link layer softc */ > - int if_drv_flags; /* driver-managed status flags */ > - struct ifaltq if_snd; /* output queue (includes altq) */ > - const u_int8_t *if_broadcastaddr; /* linklevel broadcast bytestring */ > - > - void *if_bridge; /* bridge glue */ > - > - struct label *if_label; /* interface MAC label */ > - > - /* these are only used by IPv6 */ > - void *if_unused[2]; > - void *if_afdata[AF_MAX]; > - int if_afdata_initialized; > - struct rwlock if_afdata_lock; > - struct task if_linktask; /* task for link change events */ > - struct rwlock if_addr_lock; /* lock to protect address lists */ > - > - LIST_ENTRY(ifnet) if_clones; /* interfaces of a cloner */ > - TAILQ_HEAD(, ifg_list) if_groups; /* linked list of groups per if */ > - /* protected by if_addr_lock */ > - void *if_pf_kif; > - void *if_lagg; /* lagg glue */ > - char *if_description; /* interface description */ > - u_int if_fib; /* interface FIB */ > - u_char if_alloctype; /* if_type at time of allocation */ > > + /* Stuff that's only temporary and doesn't belong here. */ > u_int if_hw_tsomax; /* tso burst length limit, the minimum > * is (IP_MAXPACKET / 8). > * 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 > @@ -193,6 +197,7 @@ struct ifnet { > */ > char if_cspare[3]; > int if_ispare[4]; > + void *if_unused[2]; > void *if_pspare[8]; /* 1 netmap, 7 TDB */ > }; > > @@ -521,5 +526,4 @@ void if_deregister_com_alloc(u_char type > LLADDR((struct sockaddr_dl *)((ifp)->if_addr->ifa_addr)) > > #endif /* _KERNEL */ > - > #endif /* !_NET_IF_VAR_H_ */