From: Alexander Bluhm Subject: Re: Copy if_data stuff to ifnet structure To: Vitaliy Makkoveev Cc: tech@openbsd.org Date: Mon, 20 Jan 2025 13:20:53 +0100 On Mon, Jan 20, 2025 at 12:16:22AM +0300, Vitaliy Makkoveev wrote: > 'if_data' structure contains interface data like type or baudrate mixed > with statistics counters. Each interface descriptor 'ifnet' contains the > instance of 'if_data' for deliver to the userland by request. It is not > clean which lock protects this data. Some old drivers rely on kernel > lock, some rely on net lock, ifioctl() relies on both. Moreover, network > interface could have per-cpu counters, but in such case both `if_data' > counters and per-cpu counters will be used by some pseudo drivers like > bridge(4). I want clean this mess. > > This diff copies 'if_data' stuff into 'ifnet' descriptor to separate > interface counters from the rest data. I want to introduce the wrappers > around interface statistics like below and use it in the net/* layer, so > the non per-cpu counters represented as array and accessed in the > per-cpu counters style. This will be done later. > > static inline void > if_counters_inc(struct ifnet *ifp, unsigned int c) > { > if (ifp->if_counters) > counters_inc(...); > else > ifp->if_data_counters[c]++; > } > > In the far future I want to start introduce the special mutex(9) to > protect this 'ifnet' side 'if_data' representation. > > ok? Splitting the values to have individual locking semantics makes sense. OK bluhm@ > Index: sys/net/if.c > =================================================================== > RCS file: /cvs/src/sys/net/if.c,v > retrieving revision 1.722 > diff -u -p -r1.722 if.c > --- sys/net/if.c 16 Jan 2025 17:20:23 -0000 1.722 > +++ sys/net/if.c 19 Jan 2025 20:19:56 -0000 > @@ -2798,7 +2798,29 @@ if_getdata(struct ifnet *ifp, struct if_ > { > unsigned int i; > > - *data = ifp->if_data; > + data->ifi_type = ifp->if_type; > + data->ifi_addrlen = ifp->if_addrlen; > + data->ifi_hdrlen = ifp->if_hdrlen; > + data->ifi_link_state = ifp->if_link_state; > + data->ifi_mtu = ifp->if_mtu; > + data->ifi_metric = ifp->if_metric; > + data->ifi_baudrate = ifp->if_baudrate; > + data->ifi_capabilities = ifp->if_capabilities; > + data->ifi_rdomain = ifp->if_rdomain; > + data->ifi_lastchange = ifp->if_lastchange; > + > + data->ifi_ipackets = ifp->if_data_counters[ifc_ipackets]; > + data->ifi_ierrors = ifp->if_data_counters[ifc_ierrors]; > + data->ifi_opackets = ifp->if_data_counters[ifc_opackets]; > + data->ifi_oerrors = ifp->if_data_counters[ifc_oerrors]; > + data->ifi_collisions = ifp->if_data_counters[ifc_collisions]; > + data->ifi_ibytes = ifp->if_data_counters[ifc_ibytes]; > + data->ifi_obytes = ifp->if_data_counters[ifc_obytes]; > + data->ifi_imcasts = ifp->if_data_counters[ifc_imcasts]; > + data->ifi_omcasts = ifp->if_data_counters[ifc_omcasts]; > + data->ifi_iqdrops = ifp->if_data_counters[ifc_iqdrops]; > + data->ifi_oqdrops = ifp->if_data_counters[ifc_oqdrops]; > + data->ifi_noproto = ifp->if_data_counters[ifc_noproto]; > > if (ifp->if_counters != NULL) { > uint64_t counters[ifc_ncounters]; > Index: sys/net/if_var.h > =================================================================== > RCS file: /cvs/src/sys/net/if_var.h,v > retrieving revision 1.133 > diff -u -p -r1.133 if_var.h > --- sys/net/if_var.h 12 Oct 2024 23:18:10 -0000 1.133 > +++ sys/net/if_var.h 19 Jan 2025 20:19:56 -0000 > @@ -110,6 +110,23 @@ struct if_clone { > .ifc_destroy = destroy, \ > } > > +enum if_counters { > + ifc_ipackets, /* packets received on interface */ > + ifc_ierrors, /* input errors on interface */ > + ifc_opackets, /* packets sent on interface */ > + ifc_oerrors, /* output errors on interface */ > + ifc_collisions, /* collisions on csma interfaces */ > + ifc_ibytes, /* total number of octets received */ > + ifc_obytes, /* total number of octets sent */ > + ifc_imcasts, /* packets received via multicast */ > + ifc_omcasts, /* packets sent via multicast */ > + ifc_iqdrops, /* dropped on input, this interface */ > + ifc_oqdrops, /* dropped on output, this interface */ > + ifc_noproto, /* destined for unsupported protocol */ > + > + ifc_ncounters > +}; > + > /* > * Structure defining a queue for a network interface. > * > @@ -147,7 +164,20 @@ struct ifnet { /* and the entries */ > short if_timer; /* time 'til if_watchdog called */ > unsigned short if_flags; /* [N] up/down, broadcast, etc. */ > int if_xflags; /* [N] extra softnet flags */ > - struct if_data if_data; /* stats and other data about if */ > + > + /* Stats and other data about if. Should be in sync with if_data. */ > + u_char if_type; > + u_char if_addrlen; > + u_char if_hdrlen; > + u_char if_link_state; > + uint32_t if_mtu; > + uint32_t if_metric; > + uint64_t if_baudrate; > + uint32_t if_capabilities; > + uint32_t if_rdomain; > + struct timeval if_lastchange; /* [c] last op. state change */ > + uint64_t if_data_counters[ifc_ncounters]; > + > struct cpumem *if_counters; /* per cpu stats */ > uint32_t if_hardmtu; /* [d] maximum MTU device supports */ > char if_description[IFDESCRSIZE]; /* [c] interface description */ > @@ -187,45 +217,19 @@ struct ifnet { /* and the entries */ > > struct nd_ifinfo *if_nd; /* [I] IPv6 Neighbor Discovery info */ > }; > -#define if_mtu if_data.ifi_mtu > -#define if_type if_data.ifi_type > -#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_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 /* [c] last op. state change */ > -#define if_capabilities if_data.ifi_capabilities > -#define if_rdomain if_data.ifi_rdomain > > -enum if_counters { > - ifc_ipackets, /* packets received on interface */ > - ifc_ierrors, /* input errors on interface */ > - ifc_opackets, /* packets sent on interface */ > - ifc_oerrors, /* output errors on interface */ > - ifc_collisions, /* collisions on csma interfaces */ > - ifc_ibytes, /* total number of octets received */ > - ifc_obytes, /* total number of octets sent */ > - ifc_imcasts, /* packets received via multicast */ > - ifc_omcasts, /* packets sent via multicast */ > - ifc_iqdrops, /* dropped on input, this interface */ > - ifc_oqdrops, /* dropped on output, this interface */ > - ifc_noproto, /* destined for unsupported protocol */ > - > - ifc_ncounters > -}; > +#define if_ipackets if_data_counters[ifc_ipackets] > +#define if_ierrors if_data_counters[ifc_ierrors] > +#define if_opackets if_data_counters[ifc_opackets] > +#define if_oerrors if_data_counters[ifc_oerrors] > +#define if_collisions if_data_counters[ifc_collisions] > +#define if_ibytes if_data_counters[ifc_ibytes] > +#define if_obytes if_data_counters[ifc_obytes] > +#define if_imcasts if_data_counters[ifc_imcasts] > +#define if_omcasts if_data_counters[ifc_omcasts] > +#define if_iqdrops if_data_counters[ifc_iqdrops] > +#define if_oqdrops if_data_counters[ifc_oqdrops] > +#define if_noproto if_data_counters[ifc_noproto] > > /* > * The ifaddr structure contains information about one address