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_ */
_______________________________________________
svn-src-head@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-head
To unsubscribe, send any mail to "svn-src-head-unsubscr...@freebsd.org"
