Re: [interesting] smattering of possible memory ordering bugs

2007-10-26 Thread Dave Kleikamp 
Memory barriers aren't one of my strengths, but this appears correct.

Acked-by: Dave Kleikamp <[EMAIL PROTECTED]>

On Fri, 2007-10-26 at 12:09 +1000, Nick Piggin wrote:
> Index: linux-2.6/fs/jfs/jfs_metapage.c
> ===
> --- linux-2.6.orig/fs/jfs/jfs_metapage.c
> +++ linux-2.6/fs/jfs/jfs_metapage.c
> @@ -39,11 +39,12 @@ static struct {
>  #endif
>  
>  #define metapage_locked(mp) test_bit(META_locked, &(mp)->flag)
> -#define trylock_metapage(mp) test_and_set_bit(META_locked,
> &(mp)->flag)
> +#define trylock_metapage(mp) test_and_set_bit_lock(META_locked,
> &(mp)->flag)
>  
>  static inline void unlock_metapage(struct metapage *mp)
>  {
> -   clear_bit(META_locked, >flag);
> +   clear_bit_unlock(META_locked, >flag);
> +   smp_mb__after_clear_bit();
> wake_up(>wait);
>  }
>   

Thanks,
Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [interesting] smattering of possible memory ordering bugs

2007-10-26 Thread Dave Kleikamp 
Memory barriers aren't one of my strengths, but this appears correct.

Acked-by: Dave Kleikamp [EMAIL PROTECTED]

On Fri, 2007-10-26 at 12:09 +1000, Nick Piggin wrote:
 Index: linux-2.6/fs/jfs/jfs_metapage.c
 ===
 --- linux-2.6.orig/fs/jfs/jfs_metapage.c
 +++ linux-2.6/fs/jfs/jfs_metapage.c
 @@ -39,11 +39,12 @@ static struct {
  #endif
  
  #define metapage_locked(mp) test_bit(META_locked, (mp)-flag)
 -#define trylock_metapage(mp) test_and_set_bit(META_locked,
 (mp)-flag)
 +#define trylock_metapage(mp) test_and_set_bit_lock(META_locked,
 (mp)-flag)
  
  static inline void unlock_metapage(struct metapage *mp)
  {
 -   clear_bit(META_locked, mp-flag);
 +   clear_bit_unlock(META_locked, mp-flag);
 +   smp_mb__after_clear_bit();
 wake_up(mp-wait);
  }
   

Thanks,
Shaggy
-- 
David Kleikamp
IBM Linux Technology Center

-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [interesting] smattering of possible memory ordering bugs

2007-10-25 Thread Benjamin Herrenschmidt 

On Fri, 2007-10-26 at 13:47 +1000, Nick Piggin wrote:

> I don't think the previous code was wrong... it's not a locked section
> > and we don't care about ordering previous stores. It's an
> allocation, it
> > should be fine. In general, bitmap allocators should be allright.
> 
> Well if it is just allocating an arbitrary _number_ out of a bitmap
> and nothing else (eg. like the pid allocator), then you don't need
> barriers.

Yup, that's what it does.

Cheers,
Ben.


-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [interesting] smattering of possible memory ordering bugs

2007-10-25 Thread Nick Piggin 
On Friday 26 October 2007 13:35, Benjamin Herrenschmidt wrote:

[acks]

Thanks for those...

> > Index: linux-2.6/include/asm-powerpc/mmu_context.h
> > ===
> > --- linux-2.6.orig/include/asm-powerpc/mmu_context.h
> > +++ linux-2.6/include/asm-powerpc/mmu_context.h
> > @@ -129,7 +129,7 @@ static inline void get_mmu_context(struc
> > steal_context();
> >  #endif
> > ctx = next_mmu_context;
> > -   while (test_and_set_bit(ctx, context_map)) {
> > +   while (test_and_set_bit_lock(ctx, context_map)) {
> > ctx = find_next_zero_bit(context_map, LAST_CONTEXT+1,
> > ctx); if (ctx > LAST_CONTEXT)
> > ctx = 0;
> > @@ -158,7 +158,7 @@ static inline void destroy_context(struc
> >  {
> > preempt_disable();
> > if (mm->context.id != NO_CONTEXT) {
> > -   clear_bit(mm->context.id, context_map);
> > +   clear_bit_unlock(mm->context.id, context_map);
> > mm->context.id = NO_CONTEXT;
> >  #ifdef FEW_CONTEXTS
> > atomic_inc(_free_contexts);
>
> I don't think the previous code was wrong... it's not a locked section
> and we don't care about ordering previous stores. It's an allocation, it
> should be fine. In general, bitmap allocators should be allright.

Well if it is just allocating an arbitrary _number_ out of a bitmap
and nothing else (eg. like the pid allocator), then you don't need
barriers.


> Ignore the FEW_CONTEXTS stuff for now :-) At this point, it's UP only
> and will be replaced sooner or later.

OK. Then I agree, provided you're doing the correct synchronisation
or flushing etc. when destroying a context (which presumably you are).

I'll drop those bits then.

Thanks,
Nick
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [interesting] smattering of possible memory ordering bugs

2007-10-25 Thread Benjamin Herrenschmidt 

> Index: linux-2.6/arch/powerpc/mm/hash_native_64.c
> ===
> --- linux-2.6.orig/arch/powerpc/mm/hash_native_64.c
> +++ linux-2.6/arch/powerpc/mm/hash_native_64.c
> @@ -113,7 +113,7 @@ static inline void native_lock_hpte(stru
> unsigned long *word = >v;
>  
> while (1) {
> -   if (!test_and_set_bit(HPTE_LOCK_BIT, word))
> +   if (!test_and_set_bit_lock(HPTE_LOCK_BIT, word))
> break;
> while(test_bit(HPTE_LOCK_BIT, word))
> cpu_relax();
> @@ -124,8 +124,7 @@ static inline void native_unlock_hpte(st
>  {
> unsigned long *word = >v;
>  
> -   asm volatile("lwsync":::"memory");
> -   clear_bit(HPTE_LOCK_BIT, word);
> +   clear_bit_unlock(HPTE_LOCK_BIT, word);
>  }

Ack.

> Index: linux-2.6/arch/ppc/xmon/start.c
> ===
> --- linux-2.6.orig/arch/ppc/xmon/start.c
> +++ linux-2.6/arch/ppc/xmon/start.c
> @@ -92,16 +92,15 @@ xmon_write(void *handle, void *ptr, int 
>  {
> char *p = ptr;
> int i, c, ct;
> -
> -#ifdef CONFIG_SMP
> -   static unsigned long xmon_write_lock;
> -   int lock_wait = 100;
> +   static DEFINE_SPINLOCK(xmon_write_lock);
> +   int lock_udelay = 1;
> int locked;
>  
> -   while ((locked = test_and_set_bit(0, _write_lock)) != 0)
> -   if (--lock_wait == 0)
> +   while (!(locked = spin_trylock(_write_lock))) {
> +   udelay(1);
> +   if (--lock_udelay == 0)
> break;
> -#endif
> +   }
>  
> if (!scc_initialized)
> xmon_init_scc();
> @@ -122,10 +121,9 @@ xmon_write(void *handle, void *ptr, int 
> eieio();
> }
>  
> -#ifdef CONFIG_SMP
> -   if (!locked)
> -   clear_bit(0, _write_lock);
> -#endif
> +   if (locked)
> +   spin_unlock(_write_lock);
> +
> return nb;
>  }

Ah yeah, some dirt in xmon... ;-) ack.

> Index: linux-2.6/drivers/net/ibm_newemac/mal.c
> ===
> --- linux-2.6.orig/drivers/net/ibm_newemac/mal.c
> +++ linux-2.6/drivers/net/ibm_newemac/mal.c
> @@ -318,7 +318,7 @@ static irqreturn_t mal_rxde(int irq, voi
>  void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
>  {
> /* Spinlock-type semantics: only one caller disable poll at a time */
> -   while (test_and_set_bit(MAL_COMMAC_POLL_DISABLED, >flags))
> +   while (test_and_set_bit_lock(MAL_COMMAC_POLL_DISABLED, 
> >flags))
> msleep(1);
>  
> /* Synchronize with the MAL NAPI poller */
> @@ -327,8 +327,7 @@ void mal_poll_disable(struct mal_instanc
>  
>  void mal_poll_enable(struct mal_instance *mal, struct mal_commac *commac)
>  {
> -   smp_wmb();
> -   clear_bit(MAL_COMMAC_POLL_DISABLED, >flags);
> +   clear_bit_unlock(MAL_COMMAC_POLL_DISABLED, >flags);
>  
> /* Feels better to trigger a poll here to catch up with events that
>  * may have happened on this channel while disabled. It will most

Ack.
 
> Index: linux-2.6/include/asm-powerpc/mmu_context.h
> ===
> --- linux-2.6.orig/include/asm-powerpc/mmu_context.h
> +++ linux-2.6/include/asm-powerpc/mmu_context.h
> @@ -129,7 +129,7 @@ static inline void get_mmu_context(struc
> steal_context();
>  #endif
> ctx = next_mmu_context;
> -   while (test_and_set_bit(ctx, context_map)) {
> +   while (test_and_set_bit_lock(ctx, context_map)) {
> ctx = find_next_zero_bit(context_map, LAST_CONTEXT+1, ctx);
> if (ctx > LAST_CONTEXT)
> ctx = 0;
> @@ -158,7 +158,7 @@ static inline void destroy_context(struc
>  {
> preempt_disable();
> if (mm->context.id != NO_CONTEXT) {
> -   clear_bit(mm->context.id, context_map);
> +   clear_bit_unlock(mm->context.id, context_map);
> mm->context.id = NO_CONTEXT;
>  #ifdef FEW_CONTEXTS
> atomic_inc(_free_contexts);

I don't think the previous code was wrong... it's not a locked section
and we don't care about ordering previous stores. It's an allocation, it
should be fine. In general, bitmap allocators should be allright.

Ignore the FEW_CONTEXTS stuff for now :-) At this point, it's UP only
and will be replaced sooner or later.

> Index: linux-2.6/include/asm-ppc/mmu_context.h
> ===
> --- linux-2.6.orig/include/asm-ppc/mmu_context.h
> +++ linux-2.6/include/asm-ppc/mmu_context.h
> @@ -128,7 +128,7 @@ static inline void get_mmu_context(struc
> steal_context();
>  #endif
> ctx = next_mmu_context;
> -   while (test_and_set_bit(ctx, context_map)) {
> +   while

Re: [interesting] smattering of possible memory ordering bugs

2007-10-25 Thread Benjamin Herrenschmidt 

 Index: linux-2.6/arch/powerpc/mm/hash_native_64.c
 ===
 --- linux-2.6.orig/arch/powerpc/mm/hash_native_64.c
 +++ linux-2.6/arch/powerpc/mm/hash_native_64.c
 @@ -113,7 +113,7 @@ static inline void native_lock_hpte(stru
 unsigned long *word = hptep-v;
  
 while (1) {
 -   if (!test_and_set_bit(HPTE_LOCK_BIT, word))
 +   if (!test_and_set_bit_lock(HPTE_LOCK_BIT, word))
 break;
 while(test_bit(HPTE_LOCK_BIT, word))
 cpu_relax();
 @@ -124,8 +124,7 @@ static inline void native_unlock_hpte(st
  {
 unsigned long *word = hptep-v;
  
 -   asm volatile(lwsync:::memory);
 -   clear_bit(HPTE_LOCK_BIT, word);
 +   clear_bit_unlock(HPTE_LOCK_BIT, word);
  }

Ack.

 Index: linux-2.6/arch/ppc/xmon/start.c
 ===
 --- linux-2.6.orig/arch/ppc/xmon/start.c
 +++ linux-2.6/arch/ppc/xmon/start.c
 @@ -92,16 +92,15 @@ xmon_write(void *handle, void *ptr, int 
  {
 char *p = ptr;
 int i, c, ct;
 -
 -#ifdef CONFIG_SMP
 -   static unsigned long xmon_write_lock;
 -   int lock_wait = 100;
 +   static DEFINE_SPINLOCK(xmon_write_lock);
 +   int lock_udelay = 1;
 int locked;
  
 -   while ((locked = test_and_set_bit(0, xmon_write_lock)) != 0)
 -   if (--lock_wait == 0)
 +   while (!(locked = spin_trylock(xmon_write_lock))) {
 +   udelay(1);
 +   if (--lock_udelay == 0)
 break;
 -#endif
 +   }
  
 if (!scc_initialized)
 xmon_init_scc();
 @@ -122,10 +121,9 @@ xmon_write(void *handle, void *ptr, int 
 eieio();
 }
  
 -#ifdef CONFIG_SMP
 -   if (!locked)
 -   clear_bit(0, xmon_write_lock);
 -#endif
 +   if (locked)
 +   spin_unlock(xmon_write_lock);
 +
 return nb;
  }

Ah yeah, some dirt in xmon... ;-) ack.

 Index: linux-2.6/drivers/net/ibm_newemac/mal.c
 ===
 --- linux-2.6.orig/drivers/net/ibm_newemac/mal.c
 +++ linux-2.6/drivers/net/ibm_newemac/mal.c
 @@ -318,7 +318,7 @@ static irqreturn_t mal_rxde(int irq, voi
  void mal_poll_disable(struct mal_instance *mal, struct mal_commac *commac)
  {
 /* Spinlock-type semantics: only one caller disable poll at a time */
 -   while (test_and_set_bit(MAL_COMMAC_POLL_DISABLED, commac-flags))
 +   while (test_and_set_bit_lock(MAL_COMMAC_POLL_DISABLED, 
 commac-flags))
 msleep(1);
  
 /* Synchronize with the MAL NAPI poller */
 @@ -327,8 +327,7 @@ void mal_poll_disable(struct mal_instanc
  
  void mal_poll_enable(struct mal_instance *mal, struct mal_commac *commac)
  {
 -   smp_wmb();
 -   clear_bit(MAL_COMMAC_POLL_DISABLED, commac-flags);
 +   clear_bit_unlock(MAL_COMMAC_POLL_DISABLED, commac-flags);
  
 /* Feels better to trigger a poll here to catch up with events that
  * may have happened on this channel while disabled. It will most

Ack.
 
 Index: linux-2.6/include/asm-powerpc/mmu_context.h
 ===
 --- linux-2.6.orig/include/asm-powerpc/mmu_context.h
 +++ linux-2.6/include/asm-powerpc/mmu_context.h
 @@ -129,7 +129,7 @@ static inline void get_mmu_context(struc
 steal_context();
  #endif
 ctx = next_mmu_context;
 -   while (test_and_set_bit(ctx, context_map)) {
 +   while (test_and_set_bit_lock(ctx, context_map)) {
 ctx = find_next_zero_bit(context_map, LAST_CONTEXT+1, ctx);
 if (ctx  LAST_CONTEXT)
 ctx = 0;
 @@ -158,7 +158,7 @@ static inline void destroy_context(struc
  {
 preempt_disable();
 if (mm-context.id != NO_CONTEXT) {
 -   clear_bit(mm-context.id, context_map);
 +   clear_bit_unlock(mm-context.id, context_map);
 mm-context.id = NO_CONTEXT;
  #ifdef FEW_CONTEXTS
 atomic_inc(nr_free_contexts);

I don't think the previous code was wrong... it's not a locked section
and we don't care about ordering previous stores. It's an allocation, it
should be fine. In general, bitmap allocators should be allright.

Ignore the FEW_CONTEXTS stuff for now :-) At this point, it's UP only
and will be replaced sooner or later.

 Index: linux-2.6/include/asm-ppc/mmu_context.h
 ===
 --- linux-2.6.orig/include/asm-ppc/mmu_context.h
 +++ linux-2.6/include/asm-ppc/mmu_context.h
 @@ -128,7 +128,7 @@ static inline void get_mmu_context(struc
 steal_context();
  #endif
 ctx = next_mmu_context;
 -   while (test_and_set_bit(ctx, context_map)) {
 +   while (test_and_set_bit_lock(ctx, context_map)) {
 ctx =

Re: [interesting] smattering of possible memory ordering bugs

2007-10-25 Thread Nick Piggin 
On Friday 26 October 2007 13:35, Benjamin Herrenschmidt wrote:

[acks]

Thanks for those...

  Index: linux-2.6/include/asm-powerpc/mmu_context.h
  ===
  --- linux-2.6.orig/include/asm-powerpc/mmu_context.h
  +++ linux-2.6/include/asm-powerpc/mmu_context.h
  @@ -129,7 +129,7 @@ static inline void get_mmu_context(struc
  steal_context();
   #endif
  ctx = next_mmu_context;
  -   while (test_and_set_bit(ctx, context_map)) {
  +   while (test_and_set_bit_lock(ctx, context_map)) {
  ctx = find_next_zero_bit(context_map, LAST_CONTEXT+1,
  ctx); if (ctx  LAST_CONTEXT)
  ctx = 0;
  @@ -158,7 +158,7 @@ static inline void destroy_context(struc
   {
  preempt_disable();
  if (mm-context.id != NO_CONTEXT) {
  -   clear_bit(mm-context.id, context_map);
  +   clear_bit_unlock(mm-context.id, context_map);
  mm-context.id = NO_CONTEXT;
   #ifdef FEW_CONTEXTS
  atomic_inc(nr_free_contexts);

 I don't think the previous code was wrong... it's not a locked section
 and we don't care about ordering previous stores. It's an allocation, it
 should be fine. In general, bitmap allocators should be allright.

Well if it is just allocating an arbitrary _number_ out of a bitmap
and nothing else (eg. like the pid allocator), then you don't need
barriers.


 Ignore the FEW_CONTEXTS stuff for now :-) At this point, it's UP only
 and will be replaced sooner or later.

OK. Then I agree, provided you're doing the correct synchronisation
or flushing etc. when destroying a context (which presumably you are).

I'll drop those bits then.

Thanks,
Nick
-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [interesting] smattering of possible memory ordering bugs

2007-10-25 Thread Benjamin Herrenschmidt 

On Fri, 2007-10-26 at 13:47 +1000, Nick Piggin wrote:

 I don't think the previous code was wrong... it's not a locked section
  and we don't care about ordering previous stores. It's an
 allocation, it
  should be fine. In general, bitmap allocators should be allright.
 
 Well if it is just allocating an arbitrary _number_ out of a bitmap
 and nothing else (eg. like the pid allocator), then you don't need
 barriers.

Yup, that's what it does.

Cheers,
Ben.


-
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/