Re: *_clone_create: leave default ifq_maxlen handling to ifq_init()
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 - 1.33 > +++ if_aggr.c 21 Aug 2020 20:33:36 - > @@ -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(>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 - 1.13 > +++ if_bpe.c 21 Aug 2020 20:33:36 - > @@ -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(>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 - 1.46 > +++ if_etherip.c 21 Aug 2020 20:33:36 - > @@ -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(>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 - 1.130 > +++ if_gif.c 21 Aug 2020 20:33:36 - > @@ -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(>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 - 1.158 > +++ if_gre.c 21 Aug 2020 20:33:36 - > @@ -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(>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(>if_snd, IFQ_MAXLEN); > ifp->if_flags = IFF_BROADCAST | IFF_SIMPLEX | IFF_MULTICAST; >
*_clone_create: leave default ifq_maxlen handling to ifq_init()
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? 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 - 1.33 +++ if_aggr.c 21 Aug 2020 20:33:36 - @@ -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(>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.c22 Jul 2020 08:38:51 - 1.13 +++ if_bpe.c21 Aug 2020 20:33:36 - @@ -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(>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.c10 Jul 2020 13:26:41 - 1.46 +++ if_etherip.c21 Aug 2020 20:33:36 - @@ -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(>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.c10 Jul 2020 13:26:41 - 1.130 +++ if_gif.c21 Aug 2020 20:33:36 - @@ -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(>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.c10 Jul 2020 13:26:41 - 1.158 +++ if_gre.c21 Aug 2020 20:33:36 - @@ -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(>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(>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(>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.c10 Jul 2020 13:26:41 - 1.96 +++ if_mpe.c21 Aug
