On Wed, Apr 28, 2021 at 12:46:31AM +0200, Alexandr Nedvedicky wrote:
> looks good in general. I have just two nits/questions.
mvs@ has asked the same questions.
> > struct llinfo_arp {
> > - LIST_ENTRY(llinfo_arp) la_list;
> > - struct rtentry *la_rt; /* backpointer to rtentry */
> > - struct mbuf_queue la_mq; /* packet hold queue */
> > + LIST_ENTRY(llinfo_arp) la_list; /* [mN] global arp_list */
> > + struct rtentry *la_rt; /* [I] backpointer to rtentry */
> > + struct mbuf_queue la_mq; /* [I] packet hold queue */
> ^^^
> I think la_mq is not immutable. packets
> get inserted and removed. so the queue changes. it does not seem
> to be immutable.
Yes, I was confused. I will remove this [I].
> > @@ -144,6 +155,8 @@ arp_rtrequest(struct ifnet *ifp, int req
> > struct sockaddr *gate = rt->rt_gateway;
> > struct llinfo_arp *la = (struct llinfo_arp *)rt->rt_llinfo;
> >
> > + NET_ASSERT_LOCKED();
> > +
>
> is there any case, where arp_rtrequest() is being called as a reader
> on netlock? Looks like arp_rtrequest() is always being called as
> a writer on netlock.
My multiple nettaskq diff to run forwarding in parallel will change
that to shared lock.
> I'm not sure we need to grab arp_mtx if we own netlock exclusively here.
>
> Also we should think of introducin NET_ASSERT_WLOCKED() I think.
I had actually implement it. It proved that this path will be
called shared.
> > @@ -194,13 +207,17 @@ arp_rtrequest(struct ifnet *ifp, int req
> > rt->rt_flags |= RTF_LLINFO;
> > if ((rt->rt_flags & RTF_LOCAL) == 0)
> > rt->rt_expire = getuptime();
> > + mtx_enter(&arp_mtx);
> > LIST_INSERT_HEAD(&arp_list, la, la_list);
> > + mtx_leave(&arp_mtx);
> > break;
> >
> > case RTM_DELETE:
> > if (la == NULL)
> > break;
> > + mtx_enter(&arp_mtx);
> > LIST_REMOVE(la, la_list);
> > + mtx_leave(&arp_mtx);
> > rt->rt_llinfo = NULL;
> > rt->rt_flags &= ~RTF_LLINFO;
> > atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq));
> >
These mutexes are needed if we want run IP output on multiple CPU.
The quick workaround was to put kernel lock around arpresolve().
But small mutex results in less contension.
bluhm
