Hello,
looks good in general. I have just two nits/questions.
</snip>
> ok?
>
> bluhm
>
> Index: netinet/if_ether.c
> ===================================================================
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.246
> diff -u -p -r1.246 if_ether.c
> --- netinet/if_ether.c 26 Apr 2021 07:55:16 -0000 1.246
> +++ netinet/if_ether.c 27 Apr 2021 17:23:17 -0000
> @@ -65,10 +65,19 @@
> #include <netinet/ip_carp.h>
> #endif
>
> +/*
> + * Locks used to protect struct members in this file:
> + * a atomic operations
> + * I immutable after creation
> + * K kernel lock
> + * m arp mutex, needed when net lock is shared
> + * N net lock
> + */
> +
> 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.
> time_t la_refreshed; /* when was refresh sent */
> int la_asked; /* number of queries sent */
> };
</snip>
> @@ -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.
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.
> if (ISSET(rt->rt_flags,
> RTF_GATEWAY|RTF_BROADCAST|RTF_MULTICAST|RTF_MPLS))
> return;
> @@ -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));
>
thanks and
regards
sashan
