on 2/14/03 7:14 PM, cliff neighbors at cliff@allegronetworks.com wrote:

> Not so useless I think.  The reservation set/clear can be used to wrap
> arbitrary chunks of code allowing much flexibility to implement atomic ops, as
> long as the code inside can manage a restart when the reservation collision is
> detected.
> 
> The reason it shouldn't clear on access from local CPU is exactly becauase the
> local CPU gets the reservation.

Right.  Because of the granularity of the reservation (I think it's 32
bytes), there could be problems if the CPU needed to store something else
into the same reservation granule without losing the reservation.  There's a
warning about a variation on this problem causing "live-lock" in multi-cpu
enviromnents at the end of section E.1 of the PowerPC book.

> To use reservation within a single CPU, just
> dump it on context switch or reentrance -- e.g. when switching processes
> (switch_exited:\powerpc/locore.S).
> 
> One might use the reservation mechanism to implement shared memory lock
> primitives, on single CPU systems (e.g. mem shared between processes) as well
> as SMP.

Okay,

Here's take 2.  I believe (after a bit of poking around) that all references
to ci_ipending are in extintr.c, so no other files need to be updated with
lwarx/stwcx for this change.  (I'm guessing the initialization of
ci_ipending in cpu_hatch() doesn't need them. ;)

I suspect that my mail client will mutilate this enough to make it unusable
by 'patch'.  If you want a clean version of the diff, I've posted it here:

http://users.easystreet.com/monroe/extintr.patch

--- snip ---
Index: extintr.c
===================================================================
RCS file: /cvsroot/src/sys/arch/macppc/macppc/extintr.c,v
retrieving revision 1.36
diff -u -r1.36 extintr.c
--- extintr.c    2003/02/03 17:09:57    1.36
+++ extintr.c    2003/02/15 08:02:57
@@ -66,6 +66,8 @@
 int fakeintr __P((void *));
 
 static inline int cntlzw __P((int));
+static inline void atomic_set __P((volatile int *, int));
+static inline void atomic_clear __P((volatile int *, int));
 static inline int gc_read_irq __P((void));
 static inline int mapirq __P((int));
 static void gc_enable_irq __P((int));
@@ -150,6 +152,42 @@
     return a;
 }
 
+/* 
+ * Set bit(s) atomically
+ */
+static __inline void
+atomic_set(x, mask)
+    volatile int *x;
+    int mask;
+{
+    int scratch;
+    asm volatile (
+        "1:"
+        "lwarx    %0, 0, %1;"
+        "or        %0, %0, %2;"
+        "stwcx. %0, 0, %1;"
+        "bne- 1b;"
+        : "+r"(scratch) : "r"(x), "r"(mask));
+}
+
+/* 
+ * Clear bit(s) atomically
+ */
+static __inline void
+atomic_clear(x, mask)
+    volatile int *x;
+    int mask;
+{
+    int scratch;
+    asm volatile (
+        "1:"
+        "lwarx    %0, 0, %1;"
+        "andc    %0, %0, %2;"
+        "stwcx. %0, 0, %1;"
+        "bne- 1b;"
+        : "+r"(scratch) : "r"(x), "r"(mask));
+}
+
 int
 gc_read_irq()
 {
@@ -520,7 +558,7 @@
     is = &intrsources[irq];
 
     if ((pcpl & r_imen) != 0) {
-        ci->ci_ipending |= r_imen;    /* Masked! Mark this as pending */
+        atomic_set(&ci->ci_ipending, r_imen);    /* Masked! Mark this as
pending */
         gc_disable_irq(is->is_hwirq);
     } else {
         splraise(is->is_mask);
@@ -601,7 +639,7 @@
     is = &intrsources[irq];
 
     if ((pcpl & r_imen) != 0) {
-        ci->ci_ipending |= r_imen;    /* Masked! Mark this as pending */
+        atomic_set(&ci->ci_ipending, r_imen);    /* Masked! Mark this as
pending */
         openpic_disable_irq(realirq);
     } else {
         splraise(is->is_mask);
@@ -663,7 +701,7 @@
         if (!have_openpic)
             gc_enable_irq(is->is_hwirq);
 
-        ci->ci_ipending &= ~(1 << irq);
+        atomic_clear(&ci->ci_ipending, 1 << irq);
         splraise(is->is_mask);
         mtmsr(emsr);
         KERNEL_LOCK(LK_CANRECURSE|LK_EXCLUSIVE);
@@ -685,7 +723,7 @@
 #endif
 
     if ((ci->ci_ipending & ~pcpl) & (1 << SIR_SERIAL)) {
-        ci->ci_ipending &= ~(1 << SIR_SERIAL);
+        atomic_clear(&ci->ci_ipending, 1 << SIR_SERIAL);
         splsoftserial();
         mtmsr(emsr);
         KERNEL_LOCK(LK_CANRECURSE|LK_EXCLUSIVE);
@@ -698,7 +736,7 @@
     }
     if ((ci->ci_ipending & ~pcpl) & (1 << SIR_NET)) {
         int pisr;
-        ci->ci_ipending &= ~(1 << SIR_NET);
+        atomic_clear(&ci->ci_ipending, 1 << SIR_NET);
         splsoftnet();
         pisr = netisr;
         netisr = 0;
@@ -712,7 +750,7 @@
         goto again;
     }
     if ((ci->ci_ipending & ~pcpl) & (1 << SIR_CLOCK)) {
-        ci->ci_ipending &= ~(1 << SIR_CLOCK);
+        atomic_clear(&ci->ci_ipending, 1 << SIR_CLOCK);
         splsoftclock();
         mtmsr(emsr);
         KERNEL_LOCK(LK_CANRECURSE|LK_EXCLUSIVE);
@@ -778,18 +816,11 @@
     return ocpl;
 }
 
-/* Following code should be implemented with lwarx/stwcx to avoid
- * the disable/enable. i need to read the manual once more.... */
 void
 softintr(ipl)
     int ipl;
 {
-    int msrsave;
-
-    msrsave = mfmsr();
-    mtmsr(msrsave & ~PSL_EE);
-    curcpu()->ci_ipending |= 1 << ipl;
-    mtmsr(msrsave);
+    atomic_set(&(curcpu()->ci_ipending), 1 << ipl);
 }
 
 void
--- snip ---

Of course, now that I've implemented this and posted the patch, I see that
there are atomic_setbits_ulong() and atomic_clearbits_ulong() functions in
src/sys/arch/powerpc/include/atomic.h that do almost exactly the same thing.

Ironically, these don't seem to be used anywhere in src/sys.  It looks like
someone copied them from the alpha port, where functions with the same names
_are_ used.  

The primary difference (aside from the fact that my atomic_clear saves one
instruction ;) is that the ones in atomic.h end with a 'sync' instruction.

I'm pretty sure 'sync' isn't necessary for atomicity.  The examples of
atomic updates in section E.2 of the PowerPC family reference book don't use
it, though the ones in section E.4 ("Lock Acqusition and Release") do.

-- monroe
------------------------------------------------------------------------
Monroe Williams                                         monroe@pobox.com