> On 28 Apr 2021, at 22:40, Alexander Bluhm <alexander.bl...@gmx.net> 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.c 28 Apr 2021 10:33:34 -0000 1.247
> +++ netinet/if_ether.c 28 Apr 2021 19:35:19 -0000
> @@ -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);
> }
>
