Re: arprequest() serialization

["Sepherosa Ziehau" <sepherosa@xxxxxxxxx>]

On Feb 10, 2008 10:03 AM, Nuno Antunes <nuno.antunes@gmail.com> wrote:
> 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(rt_key(rt))->sin_addr,
>                             &SIN(rt_key(rt))->sin_addr,
>                             LLADDR(SDL(gate)));
> +                       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;
>

Looks good to me.

>
> 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

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

> the serializer would have to be droped too (almost every other call to
> arprequest()).

Best Regards,
sephe

-- 
Live Free or Die