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 -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.
>
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);
> > }
>
