Hello,
On Wed, Apr 28, 2021 at 01:49:27AM +0200, Alexander Bluhm wrote:
> 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].
thanks
>
> > > @@ -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.
>
thanks for clarification.
I have no further objections/comments. I'm OK with your diff with
fixed comment at la_mq llinfo_arp member.
regards
sashan
