Re: [Syllable-kernel] Patch: Updated atomic primitives

[Daniel G. <da...@fp...>]

On Sat, 2004-10-16 at 13:25 -0700, Jake Hamby wrote:
> As discussed recently on syllable-developer (in the "LiveCD 0.5.4 Aterm 
> problems" thread), the atomic primitives in Syllable badly needed 
> updating for SMP safety.  Based on a suggestion by Daniel Gryniewicz, I 
> replaced the primitives in atheos/atomic.h with versions based on Linux 
> 2.6.8's include/asm-i386/atomic.h.  Many locations in the kernel needed 
> to be patched, but the patches were generally minor.  Also, I replaced 
> occurrences of atomic_add(&foo, 1) and atomic_add(&foo, -1) with 
> atomic_inc(&foo) and atomic_dec(&foo), which are slightly faster and 
> more self-explanatory.
> 
> I also fixed a few typos and changed kernel/debug.c to use ints instead 
> of floats.  Amazingly, the new kernel appears to run okay on my VMware 
> setup, although I don't have an SMP installation to test on, and so this 
> should be considered highly experimental.  I still need to patch the 
> drivers and there may be other code that is broken by this change. 
> Fortunately, the size of atomic_t stayed the same so new and old code 
> should interoperate.  If you have any trouble with the bz2'd attachment, 
> I can make the patches available for download.
> 
> Enjoy!
> --
> Jake Hamby

Great work.  I especially like the atomic_add(..., -1) -> atomic_dec().
Some of those old uses were quite scary... I'm not surprised we had SMP
issues.

I didn't notice anything wrong with the patch.  You might want to
consider a few new local variables, where consecutive atomic_read()'s
occure on the same variable.  For example, this hunk:

@@ -242,14 +243,14 @@

                        for ( ; psTmp <= psPage; psTmp++ )
                        {
-                           if ( 0 != psTmp->p_nCount )
+                           if ( 0 != atomic_read( &psTmp->p_nCount ) )
                            {
-                               printk( "Page %d present on free list
with count of %d\n", psTmp->p_nPageNum, psTmp->p_nCount );
+                               printk( "Page %d present on free list
with count of %d\n", psTmp->p_nPageNum,
atomic_read( &psTmp->p_nCount ) );
                            }
-                           atomic_add( &psTmp->p_nCount, 1 );
+                           atomic_inc( &psTmp->p_nCount );
                            psTmp->p_psNext = NULL;
                            g_nAllocatedPages++;
-                           g_sSysBase.ex_nFreePageCount--;
+                           atomic_dec( &g_sSysBase.ex_nFreePageCount );
                        }
                        break;
                    }

It seems quite possible that p_nCount gets changed between the reads.
It would only cause printing problems, but could make debugging harder,
and should be easy to fix.

Reading this patch made me think I should be re-visiting the atomic
usage in the irq code... <sigh>

Great work, thank you much.

Daniel