Re: arp mutex mbuf queue

[Alexander Bluhm]

On Mon, Apr 03, 2023 at 05:45:43PM +0300, Vitaliy Makkoveev wrote:
> On Mon, Apr 03, 2023 at 02:12:45PM +0200, Alexander Bluhm wrote:
> > Hi,
> > 
> > The arp mbuf queue la_mq has its own mutex, la_hold_total uses
> > atomic operations.  So they don't need the global arp mutex.
> > 
> > Pull them out of arp_mtx blocks to make clear what the scope of
> > arp_mtx protection is.
> > 
> > ok?
> > 
> 
> According `arp_mtx' mutex(9) description, "llinfo_arp live time,
> rt_llinfo and RTF_LLINFO are protected by arp_mtx". So why unlocked `la'
> dereference within arpinvalidate() is safe?

You are right.  Forget this diff.  It is not important.
Thanks for reminding me of my own comment :-)

bluhm

Re: arp mutex mbuf queue

[Vitaliy Makkoveev]

On Mon, Apr 03, 2023 at 02:12:45PM +0200, Alexander Bluhm wrote:
> Hi,
> 
> The arp mbuf queue la_mq has its own mutex, la_hold_total uses
> atomic operations.  So they don't need the global arp mutex.
> 
> Pull them out of arp_mtx blocks to make clear what the scope of
> arp_mtx protection is.
> 
> ok?
> 

According `arp_mtx' mutex(9) description, "llinfo_arp live time,
rt_llinfo and RTF_LLINFO are protected by arp_mtx". So why unlocked `la'
dereference within arpinvalidate() is safe?

The mq_init(>la_mq, ...) movement hunks are good to me.

> bluhm
> 
> Index: netinet/if_ether.c
> ===
> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v
> retrieving revision 1.258
> diff -u -p -r1.258 if_ether.c
> --- netinet/if_ether.c8 Mar 2023 04:43:08 -   1.258
> +++ netinet/if_ether.c3 Apr 2023 11:47:57 -
> @@ -203,6 +203,7 @@ arp_rtrequest(struct ifnet *ifp, int req
>   log(LOG_DEBUG, "%s: pool get failed\n", __func__);
>   break;
>   }
> + mq_init(>la_mq, LA_HOLD_QUEUE, IPL_SOFTNET);
>  
>   mtx_enter(_mtx);
>   if (rt->rt_llinfo != NULL) {
> @@ -211,7 +212,6 @@ arp_rtrequest(struct ifnet *ifp, int req
>   pool_put(_pool, la);
>   break;
>   }
> - mq_init(>la_mq, LA_HOLD_QUEUE, IPL_SOFTNET);
>   rt->rt_llinfo = (caddr_t)la;
>   la->la_rt = rt;
>   rt->rt_flags |= RTF_LLINFO;
> @@ -233,9 +233,9 @@ arp_rtrequest(struct ifnet *ifp, int req
>   LIST_REMOVE(la, la_list);
>   rt->rt_llinfo = NULL;
>   rt->rt_flags &= ~RTF_LLINFO;
> - atomic_sub_int(_hold_total, mq_purge(>la_mq));
>   mtx_leave(_mtx);
>  
> + atomic_sub_int(_hold_total, mq_purge(>la_mq));
>   pool_put(_pool, la);
>   break;
>  
> @@ -755,10 +755,11 @@ arpinvalidate(struct rtentry *rt)
>   mtx_leave(_mtx);
>   return;
>   }
> - atomic_sub_int(_hold_total, mq_purge(>la_mq));
>   sdl->sdl_alen = 0;
>   la->la_asked = 0;
>   mtx_leave(_mtx);
> +
> + atomic_sub_int(_hold_total, mq_purge(>la_mq));
>  }
>  
>  /*
>