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 -0000 1.246
> +++ netinet/if_ether.c 27 Apr 2021 21:02:27 -0000
> @@ -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);
> }
