Re: [External] : arp mbuf delist
> On 28 Apr 2021, at 23:40, Alexander Bluhm wrote: > > On Wed, Apr 28, 2021 at 11:13:05PM +0300, Vitaliy Makkoveev wrote: >> Also we have two cases where `la_mq??? is not empty: >> 1. This thread put it back while performed ifp->if_output >> 2. Concurrent thread put it???s own packet because ARP resolution failed. >> >> So I doubt ???XXXSMP??? and following ???mbuf is back in queue..??? comments >> are >> correct as is. May be it???s better to combine them? > > I want to delete this code in next commit and want to replace it > with an assertion. Or do you object this idea? > > Which comment should I insert here to delete in next commit? > > This diff is about replacing mq_dequeue with mq_delist as sashan@ > suggested. The comments are unrelated and are just moved around. > I have no objections agains to replace this code by assertion. Go ahead, this diff is ok by me with braces removed. > bluhm > >>> Index: netinet/if_ether.c >>> === >>> RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v >>> retrieving revision 1.247 >>> diff -u -p -r1.247 if_ether.c >>> --- netinet/if_ether.c 28 Apr 2021 10:33:34 - 1.247 >>> +++ netinet/if_ether.c 28 Apr 2021 19:35:19 - >>> @@ -611,7 +611,9 @@ arpcache(struct ifnet *ifp, struct ether >>> struct in_addr *spa = (struct in_addr *)ea->arp_spa; >>> char addr[INET_ADDRSTRLEN]; >>> struct ifnet *rifp; >>> + struct mbuf_list ml; >>> struct mbuf *m; >>> + unsigned int len; >>> int changed = 0; >>> >>> KERNEL_ASSERT_LOCKED(); >>> @@ -683,21 +685,17 @@ arpcache(struct ifnet *ifp, struct ether >>> >>> la->la_asked = 0; >>> la->la_refreshed = 0; >>> - while ((m = mq_dequeue(&la->la_mq)) != NULL) { >>> - unsigned int len; >>> - >>> - atomic_dec_int(&la_hold_total); >>> - len = mq_len(&la->la_mq); >>> - >>> + mq_delist(&la->la_mq, &ml); >>> + len = ml_len(&ml); >>> + while ((m = ml_dequeue(&ml)) != NULL) { >>> ifp->if_output(ifp, m, rt_key(rt), rt); >>> - >>> - /* XXXSMP we discard if other CPU enqueues */ >>> - if (mq_len(&la->la_mq) > len) { >>> - /* mbuf is back in queue. Discard. */ >>> - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); >>> - break; >>> - } >>> } >>> + /* XXXSMP we discard if other CPU enqueues */ >>> + if (mq_len(&la->la_mq) > 0) { >>> + /* mbuf is back in queue. Discard. */ >>> + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); >>> + } else >>> + atomic_sub_int(&la_hold_total, len); >>> >>> return (0); >>> } >>> >> >
Re: [External] : arp mbuf delist
On Wed, Apr 28, 2021 at 11:13:05PM +0300, Vitaliy Makkoveev wrote: > Also we have two cases where `la_mq??? is not empty: > 1. This thread put it back while performed ifp->if_output > 2. Concurrent thread put it???s own packet because ARP resolution failed. > > So I doubt ???XXXSMP??? and following ???mbuf is back in queue..??? comments > are > correct as is. May be it???s better to combine them? I want to delete this code in next commit and want to replace it with an assertion. Or do you object this idea? Which comment should I insert here to delete in next commit? This diff is about replacing mq_dequeue with mq_delist as sashan@ suggested. The comments are unrelated and are just moved around. bluhm > > Index: netinet/if_ether.c > > === > > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v > > retrieving revision 1.247 > > diff -u -p -r1.247 if_ether.c > > --- netinet/if_ether.c 28 Apr 2021 10:33:34 - 1.247 > > +++ netinet/if_ether.c 28 Apr 2021 19:35:19 - > > @@ -611,7 +611,9 @@ arpcache(struct ifnet *ifp, struct ether > > struct in_addr *spa = (struct in_addr *)ea->arp_spa; > > char addr[INET_ADDRSTRLEN]; > > struct ifnet *rifp; > > + struct mbuf_list ml; > > struct mbuf *m; > > + unsigned int len; > > int changed = 0; > > > > KERNEL_ASSERT_LOCKED(); > > @@ -683,21 +685,17 @@ arpcache(struct ifnet *ifp, struct ether > > > > la->la_asked = 0; > > la->la_refreshed = 0; > > - while ((m = mq_dequeue(&la->la_mq)) != NULL) { > > - unsigned int len; > > - > > - atomic_dec_int(&la_hold_total); > > - len = mq_len(&la->la_mq); > > - > > + mq_delist(&la->la_mq, &ml); > > + len = ml_len(&ml); > > + while ((m = ml_dequeue(&ml)) != NULL) { > > ifp->if_output(ifp, m, rt_key(rt), rt); > > - > > - /* XXXSMP we discard if other CPU enqueues */ > > - if (mq_len(&la->la_mq) > len) { > > - /* mbuf is back in queue. Discard. */ > > - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); > > - break; > > - } > > } > > + /* XXXSMP we discard if other CPU enqueues */ > > + if (mq_len(&la->la_mq) > 0) { > > + /* mbuf is back in queue. Discard. */ > > + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); > > + } else > > + atomic_sub_int(&la_hold_total, len); > > > > return (0); > > } > > >
Re: [External] : arp mbuf delist
> On 28 Apr 2021, at 22:40, Alexander Bluhm wrote: > > On Wed, Apr 28, 2021 at 04:01:42PM +0200, Alexandr Nedvedicky wrote: >> On Wed, Apr 28, 2021 at 03:31:41PM +0200, Claudio Jeker wrote: >>> Another option is to assert on the condition. It is an error case >>> that should not exist. >>> >> >>I think assert won't hurt here as it will enable us to learn something. > > So let's commit my mq_delist() diff and after that commit the assert. > Then the latter can be easily tested and reverted if needed. > > ok? > Diff looks ok for me except two places: sashan@ already pointed about braces with while() loop. Also we have two cases where `la_mq’ is not empty: 1. This thread put it back while performed ifp->if_output 2. Concurrent thread put it’s own packet because ARP resolution failed. So I doubt “XXXSMP” and following “mbuf is back in queue..” comments are correct as is. May be it’s better to combine them? > bluhm > > Index: netinet/if_ether.c > === > RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v > retrieving revision 1.247 > diff -u -p -r1.247 if_ether.c > --- netinet/if_ether.c28 Apr 2021 10:33:34 - 1.247 > +++ netinet/if_ether.c28 Apr 2021 19:35:19 - > @@ -611,7 +611,9 @@ arpcache(struct ifnet *ifp, struct ether > struct in_addr *spa = (struct in_addr *)ea->arp_spa; > char addr[INET_ADDRSTRLEN]; > struct ifnet *rifp; > + struct mbuf_list ml; > struct mbuf *m; > + unsigned int len; > int changed = 0; > > KERNEL_ASSERT_LOCKED(); > @@ -683,21 +685,17 @@ arpcache(struct ifnet *ifp, struct ether > > la->la_asked = 0; > la->la_refreshed = 0; > - while ((m = mq_dequeue(&la->la_mq)) != NULL) { > - unsigned int len; > - > - atomic_dec_int(&la_hold_total); > - len = mq_len(&la->la_mq); > - > + mq_delist(&la->la_mq, &ml); > + len = ml_len(&ml); > + while ((m = ml_dequeue(&ml)) != NULL) { > ifp->if_output(ifp, m, rt_key(rt), rt); > - > - /* XXXSMP we discard if other CPU enqueues */ > - if (mq_len(&la->la_mq) > len) { > - /* mbuf is back in queue. Discard. */ > - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); > - break; > - } > } > + /* XXXSMP we discard if other CPU enqueues */ > + if (mq_len(&la->la_mq) > 0) { > + /* mbuf is back in queue. Discard. */ > + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); > + } else > + atomic_sub_int(&la_hold_total, len); > > return (0); > } >
Re: [External] : arp mbuf delist
Hello, diff looks good to me, I've just found one cosmetic nit. > @@ -683,21 +685,17 @@ arpcache(struct ifnet *ifp, struct ether > > la->la_asked = 0; > la->la_refreshed = 0; > - while ((m = mq_dequeue(&la->la_mq)) != NULL) { > - unsigned int len; > - > - atomic_dec_int(&la_hold_total); > - len = mq_len(&la->la_mq); > - > + mq_delist(&la->la_mq, &ml); > + len = ml_len(&ml); > + while ((m = ml_dequeue(&ml)) != NULL) { > ifp->if_output(ifp, m, rt_key(rt), rt); > - > - /* XXXSMP we discard if other CPU enqueues */ > - if (mq_len(&la->la_mq) > len) { > - /* mbuf is back in queue. Discard. */ > - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); > - break; > - } > } do we want to keep curly brackets for while() loop? the loop body is a one-liner now. > + /* XXXSMP we discard if other CPU enqueues */ > + if (mq_len(&la->la_mq) > 0) { > + /* mbuf is back in queue. Discard. */ > + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); > + } else > + atomic_sub_int(&la_hold_total, len); > > return (0); > } otherwise looks OK to me. thanks and regards sashan
Re: [External] : arp mbuf delist
On Wed, Apr 28, 2021 at 04:01:42PM +0200, Alexandr Nedvedicky wrote: > On Wed, Apr 28, 2021 at 03:31:41PM +0200, Claudio Jeker wrote: > > Another option is to assert on the condition. It is an error case > > that should not exist. > > > > I think assert won't hurt here as it will enable us to learn something. So let's commit my mq_delist() diff and after that commit the assert. Then the latter can be easily tested and reverted if needed. ok? bluhm Index: netinet/if_ether.c === RCS file: /data/mirror/openbsd/cvs/src/sys/netinet/if_ether.c,v retrieving revision 1.247 diff -u -p -r1.247 if_ether.c --- netinet/if_ether.c 28 Apr 2021 10:33:34 - 1.247 +++ netinet/if_ether.c 28 Apr 2021 19:35:19 - @@ -611,7 +611,9 @@ arpcache(struct ifnet *ifp, struct ether struct in_addr *spa = (struct in_addr *)ea->arp_spa; char addr[INET_ADDRSTRLEN]; struct ifnet *rifp; + struct mbuf_list ml; struct mbuf *m; + unsigned int len; int changed = 0; KERNEL_ASSERT_LOCKED(); @@ -683,21 +685,17 @@ arpcache(struct ifnet *ifp, struct ether la->la_asked = 0; la->la_refreshed = 0; - while ((m = mq_dequeue(&la->la_mq)) != NULL) { - unsigned int len; - - atomic_dec_int(&la_hold_total); - len = mq_len(&la->la_mq); - + mq_delist(&la->la_mq, &ml); + len = ml_len(&ml); + while ((m = ml_dequeue(&ml)) != NULL) { ifp->if_output(ifp, m, rt_key(rt), rt); - - /* XXXSMP we discard if other CPU enqueues */ - if (mq_len(&la->la_mq) > len) { - /* mbuf is back in queue. Discard. */ - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); - break; - } } + /* XXXSMP we discard if other CPU enqueues */ + if (mq_len(&la->la_mq) > 0) { + /* mbuf is back in queue. Discard. */ + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); + } else + atomic_sub_int(&la_hold_total, len); return (0); }
Re: [External] : arp mbuf delist
On Wed, Apr 28, 2021 at 03:31:41PM +0200, Claudio Jeker wrote: > On Wed, Apr 28, 2021 at 02:56:27PM +0200, Alexandr Nedvedicky wrote: > > Hello, > > > > On Wed, Apr 28, 2021 at 02:25:26AM +0200, Alexander Bluhm wrote: > > > On Wed, Apr 28, 2021 at 03:19:47AM +0300, Vitaliy Makkoveev wrote: > > > > The code not only breaks loop but also cleans the queue. Should this be > > > > kept? > > > > > > > this is a good point > > > > > In IPv6 nd6_cache_lladdr() we have neither queue nor loop, just a > > > single mbuf on hold. But we have that discard logic, too. > > > > > > if (ln->ln_hold) { > > > struct mbuf *n = ln->ln_hold; > > > ln->ln_hold = NULL; > > > /* > > > * we assume ifp is not a p2p here, so > > > just > > > * set the 2nd argument as the 1st one. > > > */ > > > ifp->if_output(ifp, n, rt_key(rt), rt); > > > if (ln->ln_hold == n) { > > > /* n is back in ln_hold. Discard. > > > */ > > > m_freem(ln->ln_hold); > > > ln->ln_hold = NULL; > > > } > > > } > > > > > > So I guess there is a reason to clean up. > > > > > > > I would say the la->la_mq queue should be empty once we dispatch all > > packets to wire. If it is not empty, then something went wrong and > > packets > > got recirculated back to la->la_mq. I think dropping them is better > > plan than keeping them in queue. There is not much we can do in this > > case than to drop them. > > Another option is to assert on the condition. It is an error case > that should not exist. > I think assert won't hurt here as it will enable us to learn something. I suspect we can increase a chance to trigger error condition by doing 'arp -a -d'. > There are a few more issues in the arp code once the concurrency goes up. > The way the link-local address is updated looks not save to me. > At least arpresolve() and arpcache() need to make sure that access to > rt_gateway is either serialized or the updates happen so that the result > is always correct. At the moment this is not the case. > > I assume that arp_rtrequest() is accessed with an exclusive NET_LOCK if > not then there is more trouble waiting (setting rt_llinfo early is one > of them). > this is the case currently as far as I can tell. regards sashan
Re: [External] : arp mbuf delist
On Wed, Apr 28, 2021 at 02:56:27PM +0200, Alexandr Nedvedicky wrote: > Hello, > > On Wed, Apr 28, 2021 at 02:25:26AM +0200, Alexander Bluhm wrote: > > On Wed, Apr 28, 2021 at 03:19:47AM +0300, Vitaliy Makkoveev wrote: > > > The code not only breaks loop but also cleans the queue. Should this be > > > kept? > > > > this is a good point > > > In IPv6 nd6_cache_lladdr() we have neither queue nor loop, just a > > single mbuf on hold. But we have that discard logic, too. > > > > if (ln->ln_hold) { > > struct mbuf *n = ln->ln_hold; > > ln->ln_hold = NULL; > > /* > > * we assume ifp is not a p2p here, so just > > * set the 2nd argument as the 1st one. > > */ > > ifp->if_output(ifp, n, rt_key(rt), rt); > > if (ln->ln_hold == n) { > > /* n is back in ln_hold. Discard. */ > > m_freem(ln->ln_hold); > > ln->ln_hold = NULL; > > } > > } > > > > So I guess there is a reason to clean up. > > > > I would say the la->la_mq queue should be empty once we dispatch all > packets to wire. If it is not empty, then something went wrong and packets > got recirculated back to la->la_mq. I think dropping them is better > plan than keeping them in queue. There is not much we can do in this > case than to drop them. Another option is to assert on the condition. It is an error case that should not exist. There are a few more issues in the arp code once the concurrency goes up. The way the link-local address is updated looks not save to me. At least arpresolve() and arpcache() need to make sure that access to rt_gateway is either serialized or the updates happen so that the result is always correct. At the moment this is not the case. I assume that arp_rtrequest() is accessed with an exclusive NET_LOCK if not then there is more trouble waiting (setting rt_llinfo early is one of them). -- :wq Claudio
Re: [External] : arp mbuf delist
Hello, On Wed, Apr 28, 2021 at 02:25:26AM +0200, Alexander Bluhm wrote: > On Wed, Apr 28, 2021 at 03:19:47AM +0300, Vitaliy Makkoveev wrote: > > The code not only breaks loop but also cleans the queue. Should this be > > kept? > this is a good point > In IPv6 nd6_cache_lladdr() we have neither queue nor loop, just a > single mbuf on hold. But we have that discard logic, too. > > if (ln->ln_hold) { > struct mbuf *n = ln->ln_hold; > ln->ln_hold = NULL; > /* > * we assume ifp is not a p2p here, so just > * set the 2nd argument as the 1st one. > */ > ifp->if_output(ifp, n, rt_key(rt), rt); > if (ln->ln_hold == n) { > /* n is back in ln_hold. Discard. */ > m_freem(ln->ln_hold); > ln->ln_hold = NULL; > } > } > > So I guess there is a reason to clean up. > I would say the la->la_mq queue should be empty once we dispatch all packets to wire. If it is not empty, then something went wrong and packets got recirculated back to la->la_mq. I think dropping them is better plan than keeping them in queue. There is not much we can do in this case than to drop them. thanks and regards sashan
Re: [External] : arp mbuf delist
On Wed, Apr 28, 2021 at 03:19:47AM +0300, Vitaliy Makkoveev wrote: > The code not only breaks loop but also cleans the queue. Should this be > kept? In IPv6 nd6_cache_lladdr() we have neither queue nor loop, just a single mbuf on hold. But we have that discard logic, too. if (ln->ln_hold) { struct mbuf *n = ln->ln_hold; ln->ln_hold = NULL; /* * we assume ifp is not a p2p here, so just * set the 2nd argument as the 1st one. */ ifp->if_output(ifp, n, rt_key(rt), rt); if (ln->ln_hold == n) { /* n is back in ln_hold. Discard. */ m_freem(ln->ln_hold); ln->ln_hold = NULL; } } So I guess there is a reason to clean up. bluhm
Re: [External] : arp mbuf delist
On Wed, Apr 28, 2021 at 12:10:31AM +0200, Alexandr Nedvedicky wrote: > Hello, > > On Tue, Apr 27, 2021 at 11:45:19PM +0200, Alexander Bluhm wrote: > > On Sun, Apr 25, 2021 at 06:08:17PM +1000, Jonathan Matthew wrote: > > > On Sun, Apr 25, 2021 at 09:44:16AM +0200, Alexandr Nedvedicky wrote: > > > This already exists, it's called mq_delist() > > > > Here is the diff with mq_delist(). > > > > > We'd need some other way to do the 'mbuf is back in queue' detection, > > > but I agree this seems like a sensible thing to do. > > > > This part is ugly as before. I have put a printf in the discard > > case it and I never hit it. Any idea why we need this and how to > > fix it? > > > > 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 - 1.246 > > +++ netinet/if_ether.c 27 Apr 2021 21:02:27 - > > @@ -594,7 +594,9 @@ arpcache(struct ifnet *ifp, struct ether > > struct in_addr *spa = (struct in_addr *)ea->arp_spa; > > char addr[INET_ADDRSTRLEN]; > > struct ifnet *rifp; > > + struct mbuf_list ml; > > struct mbuf *m; > > + unsigned int len; > > int changed = 0; > > > > KERNEL_ASSERT_LOCKED(); > > @@ -666,21 +668,17 @@ arpcache(struct ifnet *ifp, struct ether > > > > la->la_asked = 0; > > la->la_refreshed = 0; > > - while ((m = mq_dequeue(&la->la_mq)) != NULL) { > > - unsigned int len; > > - > > - atomic_dec_int(&la_hold_total); > > - len = mq_len(&la->la_mq); > > - > > + mq_delist(&la->la_mq, &ml); > > + len = ml_len(&ml); > > + while ((m = ml_dequeue(&ml)) != NULL) { > > ifp->if_output(ifp, m, rt_key(rt), rt); > > - > > - /* XXXSMP we discard if other CPU enqueues */ > > - if (mq_len(&la->la_mq) > len) { > > - /* mbuf is back in queue. Discard. */ > > - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); > > - break; > > - } > > } > > > + /* XXXSMP we discard if other CPU enqueues */ > > + if (mq_len(&la->la_mq) > 0) { > > + /* mbuf is back in queue. Discard. */ > > + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); > > + } else > > + atomic_sub_int(&la_hold_total, len); > > > > I think /* XXXSMP ... */ code-chunk is no longer needed. I think your > change, which introduces ml_dequeue(), turns that into an useless > artifact. > > If I understand old code right this extra check tries to make sure > the while() loop completes. I assume ifp->if_output() calls > ether_output(), > which in turn calls ether_encap() -> ether_resolve(). If ether_resolve() > fails to find ARP entry (for whatever reason, perhaps explicit 'arp -d -a' > or interface down), the packet is inserted back to la->la_mq. This would > keep the while loop spinning. > The code not only breaks loop but also cleans the queue. Should this be kept? > using ml_dequeue() solves that problem, because the while() loop now > consumes a private list of packets, which can not grow. It's granted > the loop will finish. > > So I would just delete those lines. > > > thanks and > regards > sashan > > > > return (0); > > } >
Re: [External] : arp mbuf delist
Hello, On Tue, Apr 27, 2021 at 11:45:19PM +0200, Alexander Bluhm wrote: > On Sun, Apr 25, 2021 at 06:08:17PM +1000, Jonathan Matthew wrote: > > On Sun, Apr 25, 2021 at 09:44:16AM +0200, Alexandr Nedvedicky wrote: > > This already exists, it's called mq_delist() > > Here is the diff with mq_delist(). > > > We'd need some other way to do the 'mbuf is back in queue' detection, > > but I agree this seems like a sensible thing to do. > > This part is ugly as before. I have put a printf in the discard > case it and I never hit it. Any idea why we need this and how to > fix it? > > 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.c26 Apr 2021 07:55:16 - 1.246 > +++ netinet/if_ether.c27 Apr 2021 21:02:27 - > @@ -594,7 +594,9 @@ arpcache(struct ifnet *ifp, struct ether > struct in_addr *spa = (struct in_addr *)ea->arp_spa; > char addr[INET_ADDRSTRLEN]; > struct ifnet *rifp; > + struct mbuf_list ml; > struct mbuf *m; > + unsigned int len; > int changed = 0; > > KERNEL_ASSERT_LOCKED(); > @@ -666,21 +668,17 @@ arpcache(struct ifnet *ifp, struct ether > > la->la_asked = 0; > la->la_refreshed = 0; > - while ((m = mq_dequeue(&la->la_mq)) != NULL) { > - unsigned int len; > - > - atomic_dec_int(&la_hold_total); > - len = mq_len(&la->la_mq); > - > + mq_delist(&la->la_mq, &ml); > + len = ml_len(&ml); > + while ((m = ml_dequeue(&ml)) != NULL) { > ifp->if_output(ifp, m, rt_key(rt), rt); > - > - /* XXXSMP we discard if other CPU enqueues */ > - if (mq_len(&la->la_mq) > len) { > - /* mbuf is back in queue. Discard. */ > - atomic_sub_int(&la_hold_total, mq_purge(&la->la_mq)); > - break; > - } > } > + /* XXXSMP we discard if other CPU enqueues */ > + if (mq_len(&la->la_mq) > 0) { > + /* mbuf is back in queue. Discard. */ > + atomic_sub_int(&la_hold_total, len + mq_purge(&la->la_mq)); > + } else > + atomic_sub_int(&la_hold_total, len); > I think /* XXXSMP ... */ code-chunk is no longer needed. I think your change, which introduces ml_dequeue(), turns that into an useless artifact. If I understand old code right this extra check tries to make sure the while() loop completes. I assume ifp->if_output() calls ether_output(), which in turn calls ether_encap() -> ether_resolve(). If ether_resolve() fails to find ARP entry (for whatever reason, perhaps explicit 'arp -d -a' or interface down), the packet is inserted back to la->la_mq. This would keep the while loop spinning. using ml_dequeue() solves that problem, because the while() loop now consumes a private list of packets, which can not grow. It's granted the loop will finish. So I would just delete those lines. thanks and regards sashan > return (0); > }