[En-Nut-Discussion] ARP cache again

[Harald Kipp]

Hi Dusan,
Hi Marek,

At 13:38 19.02.2005 +0100, you wrote:

>-> (I suggest to change to:)
>         if (retries-- == 0)
>             break;
>         if (nb && NutArpOutput(dev, nb)) {
>             nb = 0;                             //<<added: mark buffer 
> was released
>             break;
>         }

Good point. I added this.




>- I do not know if it really can occure but it can happen that during a 
>wait ARP entry can be released
>     if (nb) {
>         NutNetBufFree(nb);
>         if (rc && entry) {                      //<< added entry check
>             entry->ae_flags |= ATF_REM;
>             ArpCacheFlush(ifn);
>         }
>     }

IMHO, this cannot happen because ArpCacheLookup()
is called after NutEventWait(). But it doesn't
hurt and makes the code more robust in case of later
changes. I added that one too.




>- if 2nd thread is waiting for an ARP entry completion and 1st thread 
>removes that entry, then 2nd thread is locked forever
>
>I suggest to announce such an ARP entry release in ArpCacheFlush():
>
>             *aep = ae->ae_next;
>             free(ae);
>             NutEventBroadcast(&ae->ae_tq);      //<< added to release 
> other possibly waiting thread(s)

I do not understand your point here. Where is it locked forever?




>- also at the beginning of NutArpCacheQuery if there is no heap for netbuf 
>ARP entry will stay allocated and it will take 10 minutes to  get rid of it
>(if intension is to keep ARP entry allocated for next usage then I suggest 
>to comment it here:
> >> here if there is no heap for netbuf, ARP entry remains allocated and 
> waits until it ages out or it is used again.
>)
>
>     if ((entry = ArpCacheLookup(ifn, ip)) == 0) {
>         if ((entry = ArpCacheNew(ifn, ip, 0)) == 0) {
>             return -1;
>         }
>         if ((nb = NutArpAllocNetBuf(ARPOP_REQUEST, ip, 0)) == 0) {
>             //<< release ARP entry (personally I vote for goto here)
>             return -1;
>         }
>     }

Well, if memory is so low, that there is not even space for
a tiny ARP telegram, then the whole system is busted anyway.
But I agree that it is cleaner to insert:

    entry->ae_flags |= ATF_REM;
    ArpCacheFlush(ifn);

Thanks for taking the time to analyze the code.

Harald