On Wed, Nov 23, 2022 at 09:49:17AM +0100, Claudio Jeker wrote:
> On Wed, Nov 23, 2022 at 08:24:11AM +0000, Klemens Nanni wrote:
> > *if_afdata[] and struct domain's dom_if{at,de}tach() are only used with
> > IPv6 Neighbour Discovery in6_dom{at,de}tach(), which allocate/init and
> > free single struct nd_ifinfo.
> >
> > Set up a new ND-specific *if_nd member directly to avoid yet another
> > layer of indirection and thus make the generic domain API obsolete.
> >
> > The per-interface data is only accessed in nd6.c and nd6_nbr.c through
> > the ND_IFINFO() macro; it is allocated and freed exactly once during
> > interface at/detach, so document it as [I]mmutable.
> >
> > Next up as separate commits:
> > - remove *if_afdata[] struct domain's dom_if{at,de}tach()
> > - nd6_if{at,de}tach() return/argument type cleanup
> > - inline ND_IFINFO()
> >
> > Feedback? Objection? OK?
>
> I like this but one comment below.
>
> > diff --git a/sys/net/if.c b/sys/net/if.c
> > index f3fba33de3f..bdaa41aea9f 100644
> > --- a/sys/net/if.c
> > +++ b/sys/net/if.c
> > @@ -453,16 +453,25 @@ if_idxmap_remove(struct ifnet *ifp)
> > */
> > void
> > if_attachsetup(struct ifnet *ifp)
> > {
> > unsigned long ifidx;
> > +#ifdef INET6
> > + int s;
> > +#endif
> >
> > NET_ASSERT_LOCKED();
> >
> > if_addgroup(ifp, IFG_ALL);
> >
> > if_attachdomain(ifp);
> > +#ifdef INET6
> > + s = splnet();
> > + ifp->if_nd = nd6_ifattach(ifp);
> > + splx(s);
>
> There is no need to do this splnet() dance here.
> nd6_ifattach calls malloc() and then sets some values of that memory and
> returns the pointer. There is no need for splnet() around that code
> nothing can see the returned value ahead of time.
if_attachdomain(), where nd6_ifattach() is currently called, does this,
so it just remained.
I'll drop it, thanks, just didn't want to do too much stuff at once.
> Shouldn't we also remove if_afdata in the same commit? You killed the last
> user of this and I don't like to alter somewhat common structs over and
> over. Removeing this part of the code as well wont make this diff much
> larger.
I've removed it together with the dead domain API code in net/if.c
as well as the two struct domain members.
Moving stuff out of the API in one commit (this one) and then removing
dead code afterwards seems clearer to me, but I can send one big diff if
that's preferred.
>
> > +#endif
> > +
> > #if NPF > 0
> > pfi_attach_ifnet(ifp);
> > #endif
> >
> > timeout_set(&ifp->if_slowtimo, if_slowtimo, ifp);
> > @@ -1125,10 +1134,13 @@ if_detach(struct ifnet *ifp)
> > for (i = 0; (dp = domains[i]) != NULL; i++) {
> > if (dp->dom_ifdetach && ifp->if_afdata[dp->dom_family])
> > (*dp->dom_ifdetach)(ifp,
> > ifp->if_afdata[dp->dom_family]);
> > }
> > +#ifdef INET6
> > + nd6_ifdetach(ifp->if_nd);
> > +#endif
> >
> > /* Announce that the interface is gone. */
> > rtm_ifannounce(ifp, IFAN_DEPARTURE);
> > splx(s);
> > NET_UNLOCK();
> > diff --git a/sys/net/if_var.h b/sys/net/if_var.h
> > index 3a418bf0547..3bf07ed3071 100644
> > --- a/sys/net/if_var.h
> > +++ b/sys/net/if_var.h
> > @@ -185,10 +185,11 @@ struct ifnet { /* and
> > the entries */
> > unsigned int if_niqs; /* [I] number of input queues */
> >
> > struct sockaddr_dl *if_sadl; /* [N] pointer to our sockaddr_dl */
> >
> > void *if_afdata[AF_MAX];
> > + struct nd_ifinfo *if_nd; /* [I] IPv6 Neighour 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
> > diff --git a/sys/netinet6/in6.c b/sys/netinet6/in6.c
> > index 7971f79f44c..3fe9386f8af 100644
> > --- a/sys/netinet6/in6.c
> > +++ b/sys/netinet6/in6.c
> > @@ -1595,17 +1595,5 @@ in6if_do_dad(struct ifnet *ifp)
> > return (0);
> >
> > return (1);
> > }
> > }
> > -
> > -void *
> > -in6_domifattach(struct ifnet *ifp)
> > -{
> > - return nd6_ifattach(ifp);
> > -}
> > -
> > -void
> > -in6_domifdetach(struct ifnet *ifp, void *aux)
> > -{
> > - nd6_ifdetach(aux);
> > -}
> > diff --git a/sys/netinet6/in6_proto.c b/sys/netinet6/in6_proto.c
> > index 808422f6eab..e302f39cddf 100644
> > --- a/sys/netinet6/in6_proto.c
> > +++ b/sys/netinet6/in6_proto.c
> > @@ -336,12 +336,10 @@ const struct domain inet6domain = {
> > .dom_protosw = inet6sw,
> > .dom_protoswNPROTOSW = &inet6sw[nitems(inet6sw)],
> > .dom_sasize = sizeof(struct sockaddr_in6),
> > .dom_rtoffset = offsetof(struct sockaddr_in6, sin6_addr),
> > .dom_maxplen = 128,
> > - .dom_ifattach = in6_domifattach,
> > - .dom_ifdetach = in6_domifdetach
> > };
> >
> > /*
> > * Internet configuration info
> > */
> > diff --git a/sys/netinet6/nd6.h b/sys/netinet6/nd6.h
> > index f6920137cea..1bbbc5f9e23 100644
> > --- a/sys/netinet6/nd6.h
> > +++ b/sys/netinet6/nd6.h
> > @@ -75,11 +75,11 @@ struct in6_ndireq {
> > #ifdef _KERNEL
> >
> > #include <sys/queue.h>
> >
> > #define ND_IFINFO(ifp) \
> > - ((struct nd_ifinfo *)(ifp)->if_afdata[AF_INET6])
> > + ((ifp)->if_nd)
> >
> > struct llinfo_nd6 {
> > TAILQ_ENTRY(llinfo_nd6) ln_list;
> > struct rtentry *ln_rt;
> > struct mbuf *ln_hold; /* last packet until resolved/timeout */
>
> --
> :wq Claudio
>
