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_mq)) != NULL) {
>>> - unsigned int len;
>>> -
>>> - atomic_dec_int(_hold_total);
>>> - len = mq_len(>la_mq);
>>> -
>>> + mq_delist(>la_mq, );
>>> + len = ml_len();
>>> + while ((m = ml_dequeue()) != NULL) {
>>> ifp->if_output(ifp, m, rt_key(rt), rt);
>>> -
>>> - /* XXXSMP we discard if other CPU enqueues */
>>> - if (mq_len(>la_mq) > len) {
>>> - /* mbuf is back in queue. Discard. */
>>> - atomic_sub_int(_hold_total, mq_purge(>la_mq));
>>> - break;
>>> - }
>>> }
>>> + /* XXXSMP we discard if other CPU enqueues */
>>> + if (mq_len(>la_mq) > 0) {
>>> + /* mbuf is back in queue. Discard. */
>>> + atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
>>> + } else
>>> + atomic_sub_int(_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_mq)) != NULL) {
> > - unsigned int len;
> > -
> > - atomic_dec_int(_hold_total);
> > - len = mq_len(>la_mq);
> > -
> > + mq_delist(>la_mq, );
> > + len = ml_len();
> > + while ((m = ml_dequeue()) != NULL) {
> > ifp->if_output(ifp, m, rt_key(rt), rt);
> > -
> > - /* XXXSMP we discard if other CPU enqueues */
> > - if (mq_len(>la_mq) > len) {
> > - /* mbuf is back in queue. Discard. */
> > - atomic_sub_int(_hold_total, mq_purge(>la_mq));
> > - break;
> > - }
> > }
> > + /* XXXSMP we discard if other CPU enqueues */
> > + if (mq_len(>la_mq) > 0) {
> > + /* mbuf is back in queue. Discard. */
> > + atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
> > + } else
> > + atomic_sub_int(_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_mq)) != NULL) {
> - unsigned int len;
> -
> - atomic_dec_int(_hold_total);
> - len = mq_len(>la_mq);
> -
> + mq_delist(>la_mq, );
> + len = ml_len();
> + while ((m = ml_dequeue()) != NULL) {
> ifp->if_output(ifp, m, rt_key(rt), rt);
> -
> - /* XXXSMP we discard if other CPU enqueues */
> - if (mq_len(>la_mq) > len) {
> - /* mbuf is back in queue. Discard. */
> - atomic_sub_int(_hold_total, mq_purge(>la_mq));
> - break;
> - }
> }
> + /* XXXSMP we discard if other CPU enqueues */
> + if (mq_len(>la_mq) > 0) {
> + /* mbuf is back in queue. Discard. */
> + atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
> + } else
> + atomic_sub_int(_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_mq)) != NULL) {
> - unsigned int len;
> -
> - atomic_dec_int(_hold_total);
> - len = mq_len(>la_mq);
> -
> + mq_delist(>la_mq, );
> + len = ml_len();
> + while ((m = ml_dequeue()) != NULL) {
> ifp->if_output(ifp, m, rt_key(rt), rt);
> -
> - /* XXXSMP we discard if other CPU enqueues */
> - if (mq_len(>la_mq) > len) {
> - /* mbuf is back in queue. Discard. */
> - atomic_sub_int(_hold_total, mq_purge(>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_mq) > 0) {
> + /* mbuf is back in queue. Discard. */
> + atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
> + } else
> + atomic_sub_int(_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_mq)) != NULL) {
- unsigned int len;
-
- atomic_dec_int(_hold_total);
- len = mq_len(>la_mq);
-
+ mq_delist(>la_mq, );
+ len = ml_len();
+ while ((m = ml_dequeue()) != NULL) {
ifp->if_output(ifp, m, rt_key(rt), rt);
-
- /* XXXSMP we discard if other CPU enqueues */
- if (mq_len(>la_mq) > len) {
- /* mbuf is back in queue. Discard. */
- atomic_sub_int(_hold_total, mq_purge(>la_mq));
- break;
- }
}
+ /* XXXSMP we discard if other CPU enqueues */
+ if (mq_len(>la_mq) > 0) {
+ /* mbuf is back in queue. Discard. */
+ atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
+ } else
+ atomic_sub_int(_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_mq)) != NULL) {
> > - unsigned int len;
> > -
> > - atomic_dec_int(_hold_total);
> > - len = mq_len(>la_mq);
> > -
> > + mq_delist(>la_mq, );
> > + len = ml_len();
> > + while ((m = ml_dequeue()) != NULL) {
> > ifp->if_output(ifp, m, rt_key(rt), rt);
> > -
> > - /* XXXSMP we discard if other CPU enqueues */
> > - if (mq_len(>la_mq) > len) {
> > - /* mbuf is back in queue. Discard. */
> > - atomic_sub_int(_hold_total, mq_purge(>la_mq));
> > - break;
> > - }
> > }
>
> > + /* XXXSMP we discard if other CPU enqueues */
> > + if (mq_len(>la_mq) > 0) {
> > + /* mbuf is back in queue. Discard. */
> > + atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
> > + } else
> > + atomic_sub_int(_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_mq)) != NULL) {
> - unsigned int len;
> -
> - atomic_dec_int(_hold_total);
> - len = mq_len(>la_mq);
> -
> + mq_delist(>la_mq, );
> + len = ml_len();
> + while ((m = ml_dequeue()) != NULL) {
> ifp->if_output(ifp, m, rt_key(rt), rt);
> -
> - /* XXXSMP we discard if other CPU enqueues */
> - if (mq_len(>la_mq) > len) {
> - /* mbuf is back in queue. Discard. */
> - atomic_sub_int(_hold_total, mq_purge(>la_mq));
> - break;
> - }
> }
> + /* XXXSMP we discard if other CPU enqueues */
> + if (mq_len(>la_mq) > 0) {
> + /* mbuf is back in queue. Discard. */
> + atomic_sub_int(_hold_total, len + mq_purge(>la_mq));
> + } else
> + atomic_sub_int(_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);
> }
