On Fri, Aug 21, 2020 at 11:05:56PM +0200, Klemens Nanni wrote:
> Creating a cloned interface requires attaching it in the end, that's how
> it works.
>
> All clonable interfaces start with a fresh softc structure that all
> zeros after allocation due to malloc(9)'s M_ZERO flag.
>
> After driver dependent setup, all drivers call if_attach() to present
> the new interface to the stack.
>
> if_attach() starts with calling if_attach_common() which starts with
> preparing the interface queues and therefore calling ifq_init() on the
> send queue.
>
> ifq_init() eventually checks the queue's maximum length and defaults to
> IFQ_MAXLEN if it is zero, which it always is during this create/attach
> path:
>
> if (ifq->ifq_maxlen == 0)
> ifq_set_maxlen(ifq, IFQ_MAXLEN);
>
> Now, most clonable interface drivers (except bridge, enc, loop, pppx,
> switch, trunk and vlan) initialise the send queue's length to IFQ_MAXLEN
> the same way, which seems entirely redundant to me.
>
> The queue API does this in a central place already and it bothered me
> why not all drivers did the same in this regard, until I concluded this.
>
> Is my analysis correct?
> If so, I'd like to remove the redundant init code and unify drivers a
> tiny bit.
>
> Feedback? Objections? OK?
>
I have no objections. Also fgsch@ already did the same in 2001 [1]:
Don't set up ifq_maxlen manually for drivers that uses IFQ_MAXLEN
(or ifqmaxlen); it's done in if_attach() now.
No future drivers needs to set up this anymore unless they want to
use something else.
ok mvs@
1.
https://github.com/openbsd/src/commit/b59942f79e8c9a7102417b8713ad3ffe9adecf05
>
> Index: if_aggr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_aggr.c,v
> retrieving revision 1.33
> diff -u -p -r1.33 if_aggr.c
> --- if_aggr.c 22 Jul 2020 02:16:01 -0000 1.33
> +++ if_aggr.c 21 Aug 2020 20:33:36 -0000
> @@ -561,7 +561,6 @@ aggr_clone_create(struct if_clone *ifc,
> ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST | IFF_SIMPLEX;
> ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
> ifp->if_link_state = LINK_STATE_DOWN;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ether_fakeaddr(ifp);
>
> if_counters_alloc(ifp);
> Index: if_bpe.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_bpe.c,v
> retrieving revision 1.13
> diff -u -p -r1.13 if_bpe.c
> --- if_bpe.c 22 Jul 2020 08:38:51 -0000 1.13
> +++ if_bpe.c 21 Aug 2020 20:33:36 -0000
> @@ -189,7 +189,6 @@ bpe_clone_create(struct if_clone *ifc, i
> ifp->if_start = bpe_start;
> ifp->if_flags = IFF_BROADCAST | IFF_MULTICAST;
> ifp->if_xflags = IFXF_CLONED;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ether_fakeaddr(ifp);
>
> if_counters_alloc(ifp);
> Index: if_etherip.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_etherip.c,v
> retrieving revision 1.46
> diff -u -p -r1.46 if_etherip.c
> --- if_etherip.c 10 Jul 2020 13:26:41 -0000 1.46
> +++ if_etherip.c 21 Aug 2020 20:33:36 -0000
> @@ -150,7 +150,6 @@ etherip_clone_create(struct if_clone *if
> ifp->if_start = etherip_start;
> ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> ifp->if_xflags = IFXF_CLONED;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_capabilities = IFCAP_VLAN_MTU;
> ether_fakeaddr(ifp);
>
> Index: if_gif.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_gif.c,v
> retrieving revision 1.130
> diff -u -p -r1.130 if_gif.c
> --- if_gif.c 10 Jul 2020 13:26:41 -0000 1.130
> +++ if_gif.c 21 Aug 2020 20:33:36 -0000
> @@ -170,7 +170,6 @@ gif_clone_create(struct if_clone *ifc, i
> ifp->if_output = gif_output;
> ifp->if_rtrequest = p2p_rtrequest;
> ifp->if_type = IFT_GIF;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_softc = sc;
>
> if_attach(ifp);
> Index: if_gre.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_gre.c,v
> retrieving revision 1.158
> diff -u -p -r1.158 if_gre.c
> --- if_gre.c 10 Jul 2020 13:26:41 -0000 1.158
> +++ if_gre.c 21 Aug 2020 20:33:36 -0000
> @@ -715,7 +715,6 @@ egre_clone_create(struct if_clone *ifc,
> ifp->if_ioctl = egre_ioctl;
> ifp->if_start = egre_start;
> ifp->if_xflags = IFXF_CLONED;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> ether_fakeaddr(ifp);
>
> @@ -777,7 +776,6 @@ nvgre_clone_create(struct if_clone *ifc,
> ifp->if_ioctl = nvgre_ioctl;
> ifp->if_start = nvgre_start;
> ifp->if_xflags = IFXF_CLONED;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> ether_fakeaddr(ifp);
>
> @@ -849,7 +847,6 @@ eoip_clone_create(struct if_clone *ifc,
> ifp->if_ioctl = eoip_ioctl;
> ifp->if_start = eoip_start;
> ifp->if_xflags = IFXF_CLONED;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST;
> ether_fakeaddr(ifp);
>
> Index: if_mpe.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_mpe.c,v
> retrieving revision 1.96
> diff -u -p -r1.96 if_mpe.c
> --- if_mpe.c 10 Jul 2020 13:26:41 -0000 1.96
> +++ if_mpe.c 21 Aug 2020 20:33:36 -0000
> @@ -112,7 +112,6 @@ mpe_clone_create(struct if_clone *ifc, i
> ifp->if_start = mpe_start;
> ifp->if_type = IFT_MPLS;
> ifp->if_hdrlen = MPE_HDRLEN;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> sc->sc_dead = 0;
>
> Index: if_mpip.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_mpip.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 if_mpip.c
> --- if_mpip.c 10 Jul 2020 13:26:41 -0000 1.11
> +++ if_mpip.c 21 Aug 2020 20:33:36 -0000
> @@ -117,7 +117,6 @@ mpip_clone_create(struct if_clone *ifc,
> ifp->if_rtrequest = p2p_rtrequest;
> ifp->if_mtu = 1500;
> ifp->if_hardmtu = 65535;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> if_attach(ifp);
> if_counters_alloc(ifp);
> Index: if_mpw.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_mpw.c,v
> retrieving revision 1.58
> diff -u -p -r1.58 if_mpw.c
> --- if_mpw.c 10 Jul 2020 13:26:41 -0000 1.58
> +++ if_mpw.c 21 Aug 2020 20:33:36 -0000
> @@ -111,7 +111,6 @@ mpw_clone_create(struct if_clone *ifc, i
> ifp->if_output = mpw_output;
> ifp->if_start = mpw_start;
> ifp->if_hardmtu = ETHER_MAX_HARDMTU_LEN;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ether_fakeaddr(ifp);
>
> sc->sc_dead = 0;
> Index: if_pair.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pair.c,v
> retrieving revision 1.15
> diff -u -p -r1.15 if_pair.c
> --- if_pair.c 10 Jul 2020 13:26:41 -0000 1.15
> +++ if_pair.c 21 Aug 2020 20:33:36 -0000
> @@ -118,7 +118,6 @@ pair_clone_create(struct if_clone *ifc,
> ifp->if_ioctl = pairioctl;
> ifp->if_start = pairstart;
> ifp->if_xflags = IFXF_CLONED;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> ifp->if_hardmtu = ETHER_MAX_HARDMTU_LEN;
> ifp->if_capabilities = IFCAP_VLAN_MTU;
> Index: if_pflog.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pflog.c,v
> retrieving revision 1.89
> diff -u -p -r1.89 if_pflog.c
> --- if_pflog.c 30 Jul 2020 03:30:04 -0000 1.89
> +++ if_pflog.c 21 Aug 2020 20:33:36 -0000
> @@ -140,7 +140,6 @@ pflog_clone_create(struct if_clone *ifc,
> ifp->if_start = pflogstart;
> ifp->if_xflags = IFXF_CLONED;
> ifp->if_type = IFT_PFLOG;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_hdrlen = PFLOG_HDRLEN;
> if_attach(ifp);
> if_alloc_sadl(ifp);
> Index: if_pflow.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pflow.c,v
> retrieving revision 1.92
> diff -u -p -r1.92 if_pflow.c
> --- if_pflow.c 10 Jul 2020 13:26:42 -0000 1.92
> +++ if_pflow.c 21 Aug 2020 20:33:36 -0000
> @@ -248,7 +248,6 @@ pflow_clone_create(struct if_clone *ifc,
> ifp->if_start = NULL;
> ifp->if_xflags = IFXF_CLONED;
> ifp->if_type = IFT_PFLOW;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_hdrlen = PFLOW_HDRLEN;
> ifp->if_flags = IFF_UP;
> ifp->if_flags &= ~IFF_RUNNING; /* not running, need receiver */
> Index: if_pfsync.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pfsync.c,v
> retrieving revision 1.276
> diff -u -p -r1.276 if_pfsync.c
> --- if_pfsync.c 11 Aug 2020 23:40:54 -0000 1.276
> +++ if_pfsync.c 21 Aug 2020 20:33:36 -0000
> @@ -341,7 +341,6 @@ pfsync_clone_create(struct if_clone *ifc
> ifp->if_output = pfsyncoutput;
> ifp->if_qstart = pfsyncstart;
> ifp->if_type = IFT_PFSYNC;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
> ifp->if_hdrlen = sizeof(struct pfsync_header);
> ifp->if_mtu = ETHERMTU;
> ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
> Index: if_ppp.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_ppp.c,v
> retrieving revision 1.116
> diff -u -p -r1.116 if_ppp.c
> --- if_ppp.c 10 Jul 2020 13:26:42 -0000 1.116
> +++ if_ppp.c 21 Aug 2020 20:33:36 -0000
> @@ -220,7 +220,6 @@ ppp_clone_create(struct if_clone *ifc, i
> sc->sc_if.if_output = pppoutput;
> sc->sc_if.if_start = ppp_ifstart;
> sc->sc_if.if_rtrequest = p2p_rtrequest;
> - ifq_set_maxlen(&sc->sc_if.if_snd, IFQ_MAXLEN);
> mq_init(&sc->sc_inq, IFQ_MAXLEN, IPL_NET);
> ppp_pkt_list_init(&sc->sc_rawq, IFQ_MAXLEN);
> if_attach(&sc->sc_if);
> Index: if_pppoe.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_pppoe.c,v
> retrieving revision 1.71
> diff -u -p -r1.71 if_pppoe.c
> --- if_pppoe.c 21 Aug 2020 01:17:33 -0000 1.71
> +++ if_pppoe.c 21 Aug 2020 20:33:37 -0000
> @@ -213,7 +213,6 @@ pppoe_clone_create(struct if_clone *ifc,
> sc->sc_sppp.pp_if.if_xflags = IFXF_CLONED;
> sc->sc_sppp.pp_tls = pppoe_tls;
> sc->sc_sppp.pp_tlf = pppoe_tlf;
> - ifq_set_maxlen(&sc->sc_sppp.pp_if.if_snd, IFQ_MAXLEN);
>
> /* changed to real address later */
> memcpy(&sc->sc_dest, etherbroadcastaddr, sizeof(sc->sc_dest));
> Index: if_tpmr.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_tpmr.c,v
> retrieving revision 1.19
> diff -u -p -r1.19 if_tpmr.c
> --- if_tpmr.c 29 Jul 2020 12:07:58 -0000 1.19
> +++ if_tpmr.c 21 Aug 2020 20:33:37 -0000
> @@ -169,7 +169,6 @@ tpmr_clone_create(struct if_clone *ifc,
> ifp->if_flags = IFF_POINTOPOINT;
> ifp->if_xflags = IFXF_CLONED | IFXF_MPSAFE;
> ifp->if_link_state = LINK_STATE_DOWN;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> if_counters_alloc(ifp);
> if_attach(ifp);
> Index: if_tun.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_tun.c,v
> retrieving revision 1.225
> diff -u -p -r1.225 if_tun.c
> --- if_tun.c 22 Jul 2020 02:16:02 -0000 1.225
> +++ if_tun.c 21 Aug 2020 20:33:37 -0000
> @@ -235,7 +235,6 @@ tun_create(struct if_clone *ifc, int uni
> ifp->if_start = tun_start;
> ifp->if_hardmtu = TUNMRU;
> ifp->if_link_state = LINK_STATE_DOWN;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> if_counters_alloc(ifp);
>
> Index: if_vether.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vether.c,v
> retrieving revision 1.34
> diff -u -p -r1.34 if_vether.c
> --- if_vether.c 9 Aug 2020 14:33:49 -0000 1.34
> +++ if_vether.c 21 Aug 2020 20:33:37 -0000
> @@ -84,7 +84,6 @@ vether_clone_create(struct if_clone *ifc
> ifp->if_softc = sc;
> ifp->if_ioctl = vetherioctl;
> ifp->if_qstart = vetherqstart;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> ifp->if_hardmtu = ETHER_MAX_HARDMTU_LEN;
> ifp->if_capabilities = IFCAP_VLAN_MTU;
> Index: if_vxlan.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_vxlan.c,v
> retrieving revision 1.80
> diff -u -p -r1.80 if_vxlan.c
> --- if_vxlan.c 28 Jul 2020 09:52:32 -0000 1.80
> +++ if_vxlan.c 21 Aug 2020 20:33:37 -0000
> @@ -151,7 +151,6 @@ vxlan_clone_create(struct if_clone *ifc,
> ifp->if_softc = sc;
> ifp->if_ioctl = vxlanioctl;
> ifp->if_start = vxlanstart;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> ifp->if_hardmtu = ETHER_MAX_HARDMTU_LEN;
> ifp->if_capabilities = IFCAP_VLAN_MTU;
> Index: if_wg.c
> ===================================================================
> RCS file: /cvs/src/sys/net/if_wg.c,v
> retrieving revision 1.11
> diff -u -p -r1.11 if_wg.c
> --- if_wg.c 13 Jul 2020 08:29:34 -0000 1.11
> +++ if_wg.c 21 Aug 2020 20:33:37 -0000
> @@ -2655,7 +2655,6 @@ wg_clone_create(struct if_clone *ifc, in
> ifp->if_output = wg_output;
>
> ifp->if_type = IFT_WIREGUARD;
> - ifq_set_maxlen(&ifp->if_snd, IFQ_MAXLEN);
>
> if_attach(ifp);
> if_alloc_sadl(ifp);
>
