Re: ARP mbuf queues

2015-03-21 Thread David Gwynne 

 On 19 Mar 2015, at 11:11 pm, Martin Pieuchot m...@openbsd.org wrote:
 
 When a host want to send packets to a destination whose Ethernet address
 that has not been resolved yet, it puts such packet on a mbuf queue.
 
 Right now this queue, linked to the corresponding ARP data structure, is
 hand rolled.  I wrote the diff below during s2k15 to make use of the
 mq_enqueue(9) API instead.
 
 I verified that the queue is correctly dropped when an infinite recursion
 in in_arpinput() is triggered.
 
 Comments, Ok?

the global count is annoying to read, but i get it.

why mbuf_queues instead of mbuf_lists? just to get the drop on mq_enqueue?

 
 Index: netinet/if_ether.c
 ===
 RCS file: /cvs/src/sys/netinet/if_ether.c,v
 retrieving revision 1.148
 diff -u -p -r1.148 if_ether.c
 --- netinet/if_ether.c14 Mar 2015 17:13:44 -  1.148
 +++ netinet/if_ether.c19 Mar 2015 13:04:28 -
 @@ -69,7 +69,6 @@
 #endif
 
 #define SDL(s) ((struct sockaddr_dl *)s)
 -#define SRP(s) ((struct sockaddr_inarp *)s)
 
 /*
  * ARP trailer negotiation.  Trailer protocol is not IP specific,
 @@ -77,6 +76,15 @@
  */
 #define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL
 
 +struct llinfo_arp {
 + LIST_ENTRY(llinfo_arp)   la_list;
 + struct rtentry  *la_rt; /* backpointer to rtentry */
 + long la_asked;  /* last time we QUERIED */
 + struct mbuf_queuela_mq; /* packet hold queue */
 +};
 +#define LA_HOLD_QUEUE 10
 +#define LA_HOLD_TOTAL 100
 +
 /* timer values */
 int   arpt_prune = (5*60*1);  /* walk list every 5 minutes */
 int   arpt_keep = (20*60);/* once resolved, good for 20 more minutes */
 @@ -220,6 +228,7 @@ arp_rtrequest(int req, struct rtentry *r
* add with a LL address.
*/
   la = pool_get(arp_pool, PR_NOWAIT | PR_ZERO);
 + mq_init(la-la_mq, LA_HOLD_QUEUE, IPL_NONE);
   rt-rt_llinfo = (caddr_t)la;
   if (la == NULL) {
   log(LOG_DEBUG, %s: malloc failed\n, __func__);
 @@ -282,8 +291,7 @@ arp_rtrequest(int req, struct rtentry *r
   LIST_REMOVE(la, la_list);
   rt-rt_llinfo = 0;
   rt-rt_flags = ~RTF_LLINFO;
 - while ((m = la-la_hold_head) != NULL) {
 - la-la_hold_head = la-la_hold_head-m_nextpkt;
 + while ((m = mq_dequeue(la-la_mq)) != NULL) {
   la_hold_total--;
   m_freem(m);
   }
 @@ -425,32 +433,14 @@ arpresolve(struct arpcom *ac, struct rte
* response yet. Insert mbuf in hold queue if below limit
* if above the limit free the queue without queuing the new packet.
*/
 - if (la_hold_total  MAX_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
 - if (la-la_hold_count = MAX_HOLD_QUEUE) {
 - mh = la-la_hold_head;
 - la-la_hold_head = la-la_hold_head-m_nextpkt;
 - if (mh == la-la_hold_tail)
 - la-la_hold_tail = NULL;
 - la-la_hold_count--;
 - la_hold_total--;
 - m_freem(mh);
 - }
 - if (la-la_hold_tail == NULL)
 - la-la_hold_head = m;
 - else
 - la-la_hold_tail-m_nextpkt = m;
 - la-la_hold_tail = m;
 - la-la_hold_count++;
 - la_hold_total++;
 + if (la_hold_total  LA_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
 + if (mq_enqueue(la-la_mq, m) == 0)
 + la_hold_total++;
   } else {
 - while ((mh = la-la_hold_head) != NULL) {
 - la-la_hold_head =
 - la-la_hold_head-m_nextpkt;
 + while ((mh = mq_dequeue(la-la_mq)) != NULL) {
   la_hold_total--;
   m_freem(mh);
   }
 - la-la_hold_tail = NULL;
 - la-la_hold_count = 0;
   m_freem(m);
   }
 
 @@ -483,14 +473,10 @@ arpresolve(struct arpcom *ac, struct rte
   rt-rt_flags |= RTF_REJECT;
   rt-rt_expire += arpt_down;
   la-la_asked = 0;
 - while ((mh = la-la_hold_head) != NULL) {
 - la-la_hold_head =
 - la-la_hold_head-m_nextpkt;
 + while ((mh = mq_dequeue(la-la_mq)) != NULL) {
   la_hold_total--;
   m_freem(mh);
   }
 - la-la_hold_tail = NULL;
 - la-la_hold_count = 0;
   }
   }
   }
 @@ -570,13 +556,14 @@ in_arpinput(struct mbuf

Re: ARP mbuf queues

2015-03-21 Thread Martin Pieuchot 
On 21/03/15(Sat) 17:48, David Gwynne wrote:
 
  On 19 Mar 2015, at 11:11 pm, Martin Pieuchot m...@openbsd.org wrote:
  
  When a host want to send packets to a destination whose Ethernet address
  that has not been resolved yet, it puts such packet on a mbuf queue.
  
  Right now this queue, linked to the corresponding ARP data structure, is
  hand rolled.  I wrote the diff below during s2k15 to make use of the
  mq_enqueue(9) API instead.
  
  I verified that the queue is correctly dropped when an infinite recursion
  in in_arpinput() is triggered.
  
  Comments, Ok?
 
 the global count is annoying to read, but i get it.
 
 why mbuf_queues instead of mbuf_lists? just to get the drop on mq_enqueue?

Yep...  That might be overkill since we do not really use the mutex.  Do
you prefer the version below using a mbuf_list?

As a bonus this diff only call ml_init() if the pool allocation succeed.

Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.148
diff -u -p -r1.148 if_ether.c
--- netinet/if_ether.c  14 Mar 2015 17:13:44 -  1.148
+++ netinet/if_ether.c  21 Mar 2015 13:59:55 -
@@ -69,7 +69,6 @@
 #endif
 
 #define SDL(s) ((struct sockaddr_dl *)s)
-#define SRP(s) ((struct sockaddr_inarp *)s)
 
 /*
  * ARP trailer negotiation.  Trailer protocol is not IP specific,
@@ -77,6 +76,15 @@
  */
 #define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL
 
+struct llinfo_arp {
+   LIST_ENTRY(llinfo_arp)   la_list;
+   struct rtentry  *la_rt; /* backpointer to rtentry */
+   long la_asked;  /* last time we QUERIED */
+   struct mbuf_list la_ml; /* packet hold queue */
+};
+#define LA_HOLD_QUEUE 10
+#define LA_HOLD_TOTAL 100
+
 /* timer values */
 intarpt_prune = (5*60*1);  /* walk list every 5 minutes */
 intarpt_keep = (20*60);/* once resolved, good for 20 more minutes */
@@ -227,6 +235,7 @@ arp_rtrequest(int req, struct rtentry *r
}
arp_inuse++;
arp_allocated++;
+   ml_init(la-la_ml);
la-la_rt = rt;
rt-rt_flags |= RTF_LLINFO;
LIST_INSERT_HEAD(llinfo_arp, la, la_list);
@@ -282,8 +291,7 @@ arp_rtrequest(int req, struct rtentry *r
LIST_REMOVE(la, la_list);
rt-rt_llinfo = 0;
rt-rt_flags = ~RTF_LLINFO;
-   while ((m = la-la_hold_head) != NULL) {
-   la-la_hold_head = la-la_hold_head-m_nextpkt;
+   while ((m = ml_dequeue(la-la_ml)) != NULL) {
la_hold_total--;
m_freem(m);
}
@@ -425,32 +433,19 @@ arpresolve(struct arpcom *ac, struct rte
 * response yet. Insert mbuf in hold queue if below limit
 * if above the limit free the queue without queuing the new packet.
 */
-   if (la_hold_total  MAX_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
-   if (la-la_hold_count = MAX_HOLD_QUEUE) {
-   mh = la-la_hold_head;
-   la-la_hold_head = la-la_hold_head-m_nextpkt;
-   if (mh == la-la_hold_tail)
-   la-la_hold_tail = NULL;
-   la-la_hold_count--;
+   if (la_hold_total  LA_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
+   if (ml_len(la-la_ml) = LA_HOLD_QUEUE) {
+   mh = ml_dequeue(la-la_ml);
la_hold_total--;
m_freem(mh);
}
-   if (la-la_hold_tail == NULL)
-   la-la_hold_head = m;
-   else
-   la-la_hold_tail-m_nextpkt = m;
-   la-la_hold_tail = m;
-   la-la_hold_count++;
+   ml_enqueue(la-la_ml, m);
la_hold_total++;
} else {
-   while ((mh = la-la_hold_head) != NULL) {
-   la-la_hold_head =
-   la-la_hold_head-m_nextpkt;
+   while ((mh = ml_dequeue(la-la_ml)) != NULL) {
la_hold_total--;
m_freem(mh);
}
-   la-la_hold_tail = NULL;
-   la-la_hold_count = 0;
m_freem(m);
}
 
@@ -483,14 +478,10 @@ arpresolve(struct arpcom *ac, struct rte
rt-rt_flags |= RTF_REJECT;
rt-rt_expire += arpt_down;
la-la_asked = 0;
-   while ((mh = la-la_hold_head) != NULL) {
-   la-la_hold_head =
-   la-la_hold_head-m_nextpkt;
+   while ((mh = ml_dequeue(la-la_ml)) != NULL) {
la_hold_total--;

Re: ARP mbuf queues

2015-03-19 Thread Martin Pieuchot 
On 19/03/15(Thu) 14:11, Martin Pieuchot wrote:
 When a host want to send packets to a destination whose Ethernet address
 that has not been resolved yet, it puts such packet on a mbuf queue.
 
 Right now this queue, linked to the corresponding ARP data structure, is
 hand rolled.  I wrote the diff below during s2k15 to make use of the
 mq_enqueue(9) API instead.
 
 I verified that the queue is correctly dropped when an infinite recursion
 in in_arpinput() is triggered.
 
 Comments, Ok?

Oops, kspillner@ pointed that I forgot a chunk, here's the full
diff:

Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.148
diff -u -p -r1.148 if_ether.c
--- netinet/if_ether.c  14 Mar 2015 17:13:44 -  1.148
+++ netinet/if_ether.c  19 Mar 2015 13:04:28 -
@@ -69,7 +69,6 @@
 #endif
 
 #define SDL(s) ((struct sockaddr_dl *)s)
-#define SRP(s) ((struct sockaddr_inarp *)s)
 
 /*
  * ARP trailer negotiation.  Trailer protocol is not IP specific,
@@ -77,6 +76,15 @@
  */
 #define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL
 
+struct llinfo_arp {
+   LIST_ENTRY(llinfo_arp)   la_list;
+   struct rtentry  *la_rt; /* backpointer to rtentry */
+   long la_asked;  /* last time we QUERIED */
+   struct mbuf_queuela_mq; /* packet hold queue */
+};
+#define LA_HOLD_QUEUE 10
+#define LA_HOLD_TOTAL 100
+
 /* timer values */
 intarpt_prune = (5*60*1);  /* walk list every 5 minutes */
 intarpt_keep = (20*60);/* once resolved, good for 20 more minutes */
@@ -220,6 +228,7 @@ arp_rtrequest(int req, struct rtentry *r
 * add with a LL address.
 */
la = pool_get(arp_pool, PR_NOWAIT | PR_ZERO);
+   mq_init(la-la_mq, LA_HOLD_QUEUE, IPL_NONE);
rt-rt_llinfo = (caddr_t)la;
if (la == NULL) {
log(LOG_DEBUG, %s: malloc failed\n, __func__);
@@ -282,8 +291,7 @@ arp_rtrequest(int req, struct rtentry *r
LIST_REMOVE(la, la_list);
rt-rt_llinfo = 0;
rt-rt_flags = ~RTF_LLINFO;
-   while ((m = la-la_hold_head) != NULL) {
-   la-la_hold_head = la-la_hold_head-m_nextpkt;
+   while ((m = mq_dequeue(la-la_mq)) != NULL) {
la_hold_total--;
m_freem(m);
}
@@ -425,32 +433,14 @@ arpresolve(struct arpcom *ac, struct rte
 * response yet. Insert mbuf in hold queue if below limit
 * if above the limit free the queue without queuing the new packet.
 */
-   if (la_hold_total  MAX_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
-   if (la-la_hold_count = MAX_HOLD_QUEUE) {
-   mh = la-la_hold_head;
-   la-la_hold_head = la-la_hold_head-m_nextpkt;
-   if (mh == la-la_hold_tail)
-   la-la_hold_tail = NULL;
-   la-la_hold_count--;
-   la_hold_total--;
-   m_freem(mh);
-   }
-   if (la-la_hold_tail == NULL)
-   la-la_hold_head = m;
-   else
-   la-la_hold_tail-m_nextpkt = m;
-   la-la_hold_tail = m;
-   la-la_hold_count++;
-   la_hold_total++;
+   if (la_hold_total  LA_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
+   if (mq_enqueue(la-la_mq, m) == 0)
+   la_hold_total++;
} else {
-   while ((mh = la-la_hold_head) != NULL) {
-   la-la_hold_head =
-   la-la_hold_head-m_nextpkt;
+   while ((mh = mq_dequeue(la-la_mq)) != NULL) {
la_hold_total--;
m_freem(mh);
}
-   la-la_hold_tail = NULL;
-   la-la_hold_count = 0;
m_freem(m);
}
 
@@ -483,14 +473,10 @@ arpresolve(struct arpcom *ac, struct rte
rt-rt_flags |= RTF_REJECT;
rt-rt_expire += arpt_down;
la-la_asked = 0;
-   while ((mh = la-la_hold_head) != NULL) {
-   la-la_hold_head =
-   la-la_hold_head-m_nextpkt;
+   while ((mh = mq_dequeue(la-la_mq)) != NULL) {
la_hold_total--;
m_freem(mh);
}
-   la-la_hold_tail = NULL;
-   la-la_hold_count = 0;
}
}
}
@@ -570,13 +556,14 @@ in_arpinput(struct mbuf *m)
struct sockaddr_dl *sdl;

ARP mbuf queues

2015-03-19 Thread Martin Pieuchot 
When a host want to send packets to a destination whose Ethernet address
that has not been resolved yet, it puts such packet on a mbuf queue.

Right now this queue, linked to the corresponding ARP data structure, is
hand rolled.  I wrote the diff below during s2k15 to make use of the
mq_enqueue(9) API instead.

I verified that the queue is correctly dropped when an infinite recursion
in in_arpinput() is triggered.

Comments, Ok?

Index: netinet/if_ether.c
===
RCS file: /cvs/src/sys/netinet/if_ether.c,v
retrieving revision 1.148
diff -u -p -r1.148 if_ether.c
--- netinet/if_ether.c  14 Mar 2015 17:13:44 -  1.148
+++ netinet/if_ether.c  19 Mar 2015 13:04:28 -
@@ -69,7 +69,6 @@
 #endif
 
 #define SDL(s) ((struct sockaddr_dl *)s)
-#define SRP(s) ((struct sockaddr_inarp *)s)
 
 /*
  * ARP trailer negotiation.  Trailer protocol is not IP specific,
@@ -77,6 +76,15 @@
  */
 #define ETHERTYPE_IPTRAILERS ETHERTYPE_TRAIL
 
+struct llinfo_arp {
+   LIST_ENTRY(llinfo_arp)   la_list;
+   struct rtentry  *la_rt; /* backpointer to rtentry */
+   long la_asked;  /* last time we QUERIED */
+   struct mbuf_queuela_mq; /* packet hold queue */
+};
+#define LA_HOLD_QUEUE 10
+#define LA_HOLD_TOTAL 100
+
 /* timer values */
 intarpt_prune = (5*60*1);  /* walk list every 5 minutes */
 intarpt_keep = (20*60);/* once resolved, good for 20 more minutes */
@@ -220,6 +228,7 @@ arp_rtrequest(int req, struct rtentry *r
 * add with a LL address.
 */
la = pool_get(arp_pool, PR_NOWAIT | PR_ZERO);
+   mq_init(la-la_mq, LA_HOLD_QUEUE, IPL_NONE);
rt-rt_llinfo = (caddr_t)la;
if (la == NULL) {
log(LOG_DEBUG, %s: malloc failed\n, __func__);
@@ -282,8 +291,7 @@ arp_rtrequest(int req, struct rtentry *r
LIST_REMOVE(la, la_list);
rt-rt_llinfo = 0;
rt-rt_flags = ~RTF_LLINFO;
-   while ((m = la-la_hold_head) != NULL) {
-   la-la_hold_head = la-la_hold_head-m_nextpkt;
+   while ((m = mq_dequeue(la-la_mq)) != NULL) {
la_hold_total--;
m_freem(m);
}
@@ -425,32 +433,14 @@ arpresolve(struct arpcom *ac, struct rte
 * response yet. Insert mbuf in hold queue if below limit
 * if above the limit free the queue without queuing the new packet.
 */
-   if (la_hold_total  MAX_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
-   if (la-la_hold_count = MAX_HOLD_QUEUE) {
-   mh = la-la_hold_head;
-   la-la_hold_head = la-la_hold_head-m_nextpkt;
-   if (mh == la-la_hold_tail)
-   la-la_hold_tail = NULL;
-   la-la_hold_count--;
-   la_hold_total--;
-   m_freem(mh);
-   }
-   if (la-la_hold_tail == NULL)
-   la-la_hold_head = m;
-   else
-   la-la_hold_tail-m_nextpkt = m;
-   la-la_hold_tail = m;
-   la-la_hold_count++;
-   la_hold_total++;
+   if (la_hold_total  LA_HOLD_TOTAL  la_hold_total  nmbclust / 64) {
+   if (mq_enqueue(la-la_mq, m) == 0)
+   la_hold_total++;
} else {
-   while ((mh = la-la_hold_head) != NULL) {
-   la-la_hold_head =
-   la-la_hold_head-m_nextpkt;
+   while ((mh = mq_dequeue(la-la_mq)) != NULL) {
la_hold_total--;
m_freem(mh);
}
-   la-la_hold_tail = NULL;
-   la-la_hold_count = 0;
m_freem(m);
}
 
@@ -483,14 +473,10 @@ arpresolve(struct arpcom *ac, struct rte
rt-rt_flags |= RTF_REJECT;
rt-rt_expire += arpt_down;
la-la_asked = 0;
-   while ((mh = la-la_hold_head) != NULL) {
-   la-la_hold_head =
-   la-la_hold_head-m_nextpkt;
+   while ((mh = mq_dequeue(la-la_mq)) != NULL) {
la_hold_total--;
m_freem(mh);
}
-   la-la_hold_tail = NULL;
-   la-la_hold_count = 0;
}
}
}
@@ -570,13 +556,14 @@ in_arpinput(struct mbuf *m)
struct sockaddr_dl *sdl;
struct sockaddr sa;
struct in_addr isaddr, itaddr, myaddr;
-   struct mbuf *mh, *mt;
+   struct mbuf *mh;