Problem with uipc_mbuf.c

[Randall Stewart]

John-Mark:

I had read that in the atomic_fetchadd_int() comments.. and
was wondering if that was the case.. I don't have an
X86 assembler reference.. so I did not bother to
see if the asm and comments were in sync... you
are right if this is true.. and I can surely make
SCTP have this occur.. note that I can minimize how
often it hits this .. I had an optimziation where I
would steal the last mbuf if no other chunk is being
pulled and used as data.. otherwise do a mcopy()... which
is then later freed... The optimzation had a bug.. it mis-calculated
and NEVER stole the last mbuf.. which meant we were ALWAYS
heading for this race condition on an SMP box..

Since I have fixed the optimzation bug.. I see it much less
now.. but a simple un-fix of it can quickly test any change..

Let me go change the code to test for 1 (as you suggest) and
then un-fix my optimzation.. and see if it fixes the bug..

R

John-Mark Gurney wrote:
> Randall Stewart wrote this message on Mon, Aug 28, 2006 at 17:04 -0400:
> 
>>	    atomic_fetchadd_int(m->m_ext.ref_cnt, -1) == 0) {
> 
> 							 ^
> 
> This should be 1 not 0.. as apparently fetchadd_int returns the old value
> (at least that's what atomic(9) says), which means that if we ever race
> on this comparision, we won't free though we should of...
> 
> if we look at refcount.h, it does:
>         return (atomic_fetchadd_int(count, -1) == 1);
> 
> which release a reference and apparently returns true if it needs to
> be free'd...
> 
> Though the wierd part is that andre, "fixed" it to be 0 in 1.157:
> Fix a logic error introduced with mandatory mbuf cluster refcounting and
> freeing of mbufs+clusters back to the packet zone.
> 
> 
>>I am thinking about restoring the old code.. since
>>it appears to work...
>>
>>Any comments or help would be appreciated..
> 
> 
> Lets see what andre has to say about this.
> 


-- 
Randall Stewart
NSSTG - Cisco Systems Inc.
803-345-0369 <or> 815-342-5222 (cell)