kern/165863

[Gleb Smirnoff]

The following reply was made to PR kern/165863; it has been noted by GNATS.

From: Gleb Smirnoff <glebius at FreeBSD.org>
To: Eric van Gyzen <eric_van_gyzen at dell.com>,
        Eric van Gyzen <eric at vangyzen.net>, emaste at FreeBSD.org
Cc: bug-followup at FreeBSD.org
Subject: kern/165863
Date: Fri, 9 Mar 2012 13:20:56 +0400

 --BXVAT5kNtrzKuDFl
 Content-Type: text/plain; charset=koi8-r
 Content-Disposition: inline
 
   Hello, Eric and Ed.
 
   Can you look at this patch? I decided to utilize newer callout API,
 that allows to delegate lock retrieval to the callout subsystem, and
 this makes things simplier. Hope that should work.
 
   Patch is against head.
 
   Eric, can you please send me your test programs, that you use to
 reproduce the bug?
 
 -- 
 Totus tuus, Glebius.
 
 --BXVAT5kNtrzKuDFl
 Content-Type: text/x-diff; charset=koi8-r
 Content-Disposition: attachment; filename="kern165863.diff"
 
 Index: in.c
 ===================================================================
 --- in.c	(revision 232689)
 +++ in.c	(working copy)
 @@ -1279,11 +1279,12 @@
  {
  	struct in_llentry *lle;
  
 -	lle = malloc(sizeof(struct in_llentry), M_LLTABLE, M_DONTWAIT | M_ZERO);
 +	lle = malloc(sizeof(struct in_llentry), M_LLTABLE, M_NOWAIT | M_ZERO);
  	if (lle == NULL)		/* NB: caller generates msg */
  		return NULL;
  
 -	callout_init(&lle->base.la_timer, CALLOUT_MPSAFE);
 +	LLE_LOCK_INIT(&lle->base);
 +	callout_init_rw(&lle->base.la_timer, &lle->base.lle_lock, 0);
  	/*
  	 * For IPv4 this will trigger "arpresolve" to generate
  	 * an ARP request.
 @@ -1292,46 +1293,44 @@
  	lle->l3_addr4 = *(const struct sockaddr_in *)l3addr;
  	lle->base.lle_refcnt = 1;
  	lle->base.lle_free = in_lltable_free;
 -	LLE_LOCK_INIT(&lle->base);
 -	return &lle->base;
 +
 +	return (&lle->base);
  }
  
  #define IN_ARE_MASKED_ADDR_EQUAL(d, a, m)	(			\
  	    (((ntohl((d)->sin_addr.s_addr) ^ (a)->sin_addr.s_addr) & (m)->sin_addr.s_addr)) == 0 )
  
  static void
 -in_lltable_prefix_free(struct lltable *llt, 
 -		       const struct sockaddr *prefix,
 -		       const struct sockaddr *mask,
 -		       u_int flags)
 +in_lltable_prefix_free(struct lltable *llt, const struct sockaddr *prefix,
 +    const struct sockaddr *mask, u_int flags)
  {
  	const struct sockaddr_in *pfx = (const struct sockaddr_in *)prefix;
  	const struct sockaddr_in *msk = (const struct sockaddr_in *)mask;
  	struct llentry *lle, *next;
 -	register int i;
 +	int i;
  	size_t pkts_dropped;
  
 -	for (i=0; i < LLTBL_HASHTBL_SIZE; i++) {
 +	IF_AFDATA_WLOCK(llt->llt_ifp);
 +	for (i = 0; i < LLTBL_HASHTBL_SIZE; i++) {
  		LIST_FOREACH_SAFE(lle, &llt->lle_head[i], lle_next, next) {
  
  		        /* 
  			 * (flags & LLE_STATIC) means deleting all entries
 -			 * including static ARP entries
 +			 * including static ARP entries.
  			 */
 -			if (IN_ARE_MASKED_ADDR_EQUAL((struct sockaddr_in *)L3_ADDR(lle), 
 -						     pfx, msk) &&
 -			    ((flags & LLE_STATIC) || !(lle->la_flags & LLE_STATIC))) {
 -				int canceled;
 +			if (IN_ARE_MASKED_ADDR_EQUAL(satosin(L3_ADDR(lle)), 
 +			    pfx, msk) && ((flags & LLE_STATIC) ||
 +			    !(lle->la_flags & LLE_STATIC))) {
  
 -				canceled = callout_drain(&lle->la_timer);
  				LLE_WLOCK(lle);
 -				if (canceled)
 +				if (callout_stop(&lle->la_timer))
  					LLE_REMREF(lle);
  				pkts_dropped = llentry_free(lle);
  				ARPSTAT_ADD(dropped, pkts_dropped);
  			}
  		}
  	}
 +	IF_AFDATA_WUNLOCK(llt->llt_ifp);
  }
  
  
 Index: if_ether.c
 ===================================================================
 --- if_ether.c	(revision 232689)
 +++ if_ether.c	(working copy)
 @@ -163,38 +163,23 @@
  static void
  arptimer(void *arg)
  {
 +	struct llentry *lle = (struct llentry *)arg;
  	struct ifnet *ifp;
 -	struct llentry   *lle;
 -	int pkts_dropped;
 +	size_t pkts_dropped;
  
 -	KASSERT(arg != NULL, ("%s: arg NULL", __func__));
 -	lle = (struct llentry *)arg;
 +	LLE_WLOCK_ASSERT(lle);
 +
 +	if (lle->la_flags & LLE_STATIC)
 +		return;
 +
  	ifp = lle->lle_tbl->llt_ifp;
  	CURVNET_SET(ifp->if_vnet);
 -	IF_AFDATA_LOCK(ifp);
 -	LLE_WLOCK(lle);
 -	if (lle->la_flags & LLE_STATIC)
 -		LLE_WUNLOCK(lle);
 -	else {
 -		if (!callout_pending(&lle->la_timer) &&
 -		    callout_active(&lle->la_timer)) {
 -			callout_stop(&lle->la_timer);
 -			LLE_REMREF(lle);
 -			pkts_dropped = llentry_free(lle);
 -			ARPSTAT_ADD(dropped, pkts_dropped);
 -			ARPSTAT_INC(timeouts);
 -		} else {
 -#ifdef DIAGNOSTIC
 -			struct sockaddr *l3addr = L3_ADDR(lle);
 -			log(LOG_INFO, 
 -			    "arptimer issue: %p, IPv4 address: \"%s\"\n", lle,
 -			    inet_ntoa(
 -			        ((const struct sockaddr_in *)l3addr)->sin_addr));
 -#endif
 -			LLE_WUNLOCK(lle);
 -		}
 -	}
 -	IF_AFDATA_UNLOCK(ifp);
 +
 +	LLE_REMREF(lle);
 +	pkts_dropped = llentry_free(lle);
 +	ARPSTAT_ADD(dropped, pkts_dropped);
 +	ARPSTAT_INC(timeouts);
 +
  	CURVNET_RESTORE();
  }
  
 
 --BXVAT5kNtrzKuDFl--