> On 28 Apr 2021, at 23:40, Alexander Bluhm <[email protected]> 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 -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);
>>> }
>>>
>>
>
