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); >