RE: [lwip-devel] Problem with UDP and ARP queuing

[Leon Woestenberg]

Hello Tony, David,

On Wed, 2004-08-18 at 15:42, Mountifield, Tony wrote:
> Hi David,
> 
> I'm not 100% conversant with how reference counts are supposed to work within 
> queues and chains. but I see that your pbuf_ref() is outside the if, and 
> therefore called also after pbuf_queue(). Is this correct?
> 
The reference count is equal to the number of pointers to the pbuf.
Both lwip-internal and lwip-external pointers.

There are usage contracts in the API, that clearly describe when you
may no longer reference a pbuf, or when you may.

I will double-check the proposed changes by David.

Regards,

Leon.

> Cheers
> Tony
> 
> > -----Original Message-----
> > From: David Haas [mailto:address@hidden
> > Sent: 18 August 2004 13:29
> > To: lwip-devel
> > Subject: Re: [lwip-devel] Problem with UDP and ARP queuing
> > 
> > 
> > Yes, I agree with you.
> > 
> > In fact I had made that change a few days ago, but had not 
> > gotton around 
> > to creating a patch yet. My code around this area looks like this:
> > 
> > 
> > #if ARP_QUEUEING /* queue the given q packet */
> >       /* copy any PBUF_REF referenced payloads into PBUF_RAM */
> >       /* (the caller of lwIP assumes the referenced payload can be
> >        * freed after it returns from the lwIP call that 
> > brought us here) */
> >       p = pbuf_take(q);
> >       /* packet could be taken over? */
> >       if (p != NULL) {
> >         /* queue packet */
> >         if (arp_table[i].p)
> >           pbuf_queue(arp_table[i].p, p);
> >         else
> >           arp_table[i].p = p;
> >       /* pbufs are queued, increase the reference count */
> >       pbuf_ref(p);
> >         LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("etharp_query: queued 
> > packet %p on ARP entry %d\n", (void *)q, i));
> >         result = ERR_OK;
> >       } else {
> >         LWIP_DEBUGF(ETHARP_DEBUG | DBG_TRACE, ("etharp_query: 
> > could not 
> > queue a copy of PBUF_REF packet %p (out of memory)\n", (void *)q));
> >         /* { result == ERR_MEM } through initialization */
> >       }
> > #else /* ARP_QUEUEING == 0 */
> > 
> > I've done some pretty extensive testing with TCP, but only limited 
> > testing with UDP.
> > 
> > David.
> > 
> > Mountifield, Tony wrote:
> > 
> > >Following up again:
> > >
> > >  
> > >
> > >>After further thought, an idea:
> > >>
> > >>    
> > >>
> > >>>It may be just that udp_send() has no reason to free the 
> > >>>header pbuf, and we can remove that bit of code, but I don't 
> > >>>know what else may rely on it.
> > >>>      
> > >>>
> > >>Perhaps it is a reference-counting issue. Does/should 
> > >>etharp_query increment a reference count in the chain/packet 
> > >>when queuing it on an ARP table entry? And then of course 
> > >>decrement it when dequeuing.
> > >>    
> > >>
> > >
> > >I think the following is certainly needed, but we will need 
> > to make sure there is a pbuf_free() in the right place (there 
> > may already be) to avoid a pbuf leak:
> > >
> > >Index: src/netif/etharp.c
> > >===================================================================
> > >RCS file: /cvsroot/lwip/lwip/src/netif/etharp.c,v
> > >retrieving revision 1.80
> > >diff -u -r1.80 etharp.c
> > >--- src/netif/etharp.c  17 Aug 2004 08:39:43 -0000      1.80
> > >+++ src/netif/etharp.c  18 Aug 2004 11:19:14 -0000
> > >@@ -755,6 +755,7 @@
> > >         /* queue packet ... */
> > >         if (arp_table[i].p == NULL) {
> > >                /* ... in the empty queue */
> > >+               pbuf_ref(p);
> > >                arp_table[i].p = p;
> > >         } else {
> > >                /* ... at tail of non-empty queue */
> > >
> > >Cheers
> > >Tony
> > >
> > >
> > >*************************************************************
> > **********************
> > >This email, its content and any attachments is PRIVATE AND
> > >CONFIDENTIAL to TANDBERG Television. If received in error please
> > >notify the sender and destroy the original message and attachments.
> > >
> > >www.tandbergtv.com
> > >*************************************************************
> > **********************
> > >
> > >
> > >
> > >_______________________________________________
> > >lwip-devel mailing list
> > >address@hidden
> > >http://lists.nongnu.org/mailman/listinfo/lwip-devel
> > >
> > >  
> > >
> > 
> > 
> > _______________________________________________
> > lwip-devel mailing list
> > address@hidden
> > http://lists.nongnu.org/mailman/listinfo/lwip-devel
> > 
> 
> 
> ***********************************************************************************
> This email, its content and any attachments is PRIVATE AND
> CONFIDENTIAL to TANDBERG Television. If received in error please
> notify the sender and destroy the original message and attachments.
> 
> www.tandbergtv.com
> ***********************************************************************************
> 
> 
> 
> _______________________________________________
> lwip-devel mailing list
> address@hidden
> http://lists.nongnu.org/mailman/listinfo/lwip-devel