> On 28 Apr 2021, at 00:45, Alexander Bluhm <alexander.bl...@gmx.net> 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?
>
We did ARP resolution before this loop and `rt’ contains valid
address. Also `la_mq’ manipulations are serialized so I doubt this
thread could reinsert packet while performing ifp->if_enqueue().
But I can’t said this never happened.
But you are going to make this code run in parallel other thread
could fail ARP resolution and insert (not reinsert) packet while
this thread performs local `ml’ walkthrough. For this case I doubt
we should clean `la_mq’. But can we be sure we are not reinserted
packet?
> 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);
>
> return (0);
> }
>
