Bug #944
closedarprequest() serialization
0%
Description
Hi all,
Please review/comment the following.
arprequest() calls ifp->if_output() without locally grabbing the
respective serializer, so ASSERT_SERIALIZED at the beginning of the
function.
Grab the serializer at arp_rtrequest() when it calls arprequest().
diff --git a/sys/netinet/if_ether.c b/sys/netinet/if_ether.c
index b85cb03..dcc11b8 100644
--- a/sys/netinet/if_ether.c
++ b/sys/netinet/if_ether.c@ -223,11 +223,14
@ arp_rtrequest(int req, struct rtentry rt,
struct rt_addrinfo *info)
break;
}
/ Announce a new entry if requested. /
- if (rt->rt_flags & RTF_ANNOUNCE)
if (rt->rt_flags & RTF_ANNOUNCE) {
+ lwkt_serialize_enter(rt->rt_ifp->if_serializer);
arprequest(rt->rt_ifp,
&SIN)->sin_addr,
&SIN)->sin_addr,
LLADDR));
+ lwkt_serialize_exit(rt->rt_ifp->if_serializer);
+ }
/*FALLTHROUGH/
case RTM_RESOLVE:
if (gate->sa_family != AF_LINK ||@ -324,6 +327,8
@ arprequest(struct ifnet *ifp, struct in_addr *sip,
struct in_addr *tip,
struct sockaddr sa;
u_short ar_hrd;
+ ASSERT_SERIALIZED(ifp->if_serializer);
+
if ((m = m_gethdr(MB_DONTWAIT, MT_DATA)) == NULL)
return;
m->m_pkthdr.rcvif = (struct ifnet *)NULL;
I thought about grabbing the serializer locally at arprequest()
instead, but this function is also called by ether_output() which
already holds the serializer. That would cause a recursive
serialization. Would it be better to do this and drop the serializer
in ether_output() around the call to arprequest() instead of the
change proposed in the patch above? There would be other places where
the serializer would have to be droped too (almost every other call to
arprequest()).
Thanks,
Nuno
Updated by sepherosa almost 17 years ago
Looks good to me.
Nah, I'd say let the caller do the serialization. This function
actually does not have many callers; I would like to avoid frequent
serializer enter->exit->enter sequence
Best Regards,
sephe
Updated by dillon almost 17 years ago
This looks like something we want to throw into the release, I am going
to commit it now before the branch, we will have two weeks to test it
before the release.
-Matt
Updated by dillon almost 17 years ago
Never mind, you committed it already today! Great!
cvs update is my friend :-)
-Matt
Updated by nant almost 17 years ago
Heh :) I actually committed it just a few minutes before your email.
Cheers,
Nuno