Index | Thread | Search

From:
Alexander Bluhm <bluhm@openbsd.org>
Subject:
Re: Copy if_data stuff to ifnet structure
To:
Vitaliy Makkoveev <mvs@openbsd.org>
Cc:
tech@openbsd.org
Date:
Mon, 20 Jan 2025 13:20:53 +0100

Download raw body.

Thread
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