Re: [PATCH 11/17] fuse: add number of waiting requests attribute

[Andrew Morton]

Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> > This doesn't get initialised anywhere.
> > 
> > Presumably you're relying on a memset somewhere.  That might work on all
> > architectures, AFAIK.  But in theory it's wrong.  If, for example, the
> > architecture implements atomic_t via a spinlock-plus-integer, and that
> > spinlock's unlocked state is not all-bits-zero, we're dead.
> > 
> > So we should initialise it with
> > 
> > 	foo->num_waiting = ATOMIC_INIT(0);
> > 
> 
> Is it correct to use a structure initializer this way?

Yes, if it's typecast to the right type.

ATOMIC_INIT is not.  I had a brainfart.

> > nb: it is not correct to initialise an atomic_t with
> > 
> > 	atomic_set(a, 0);
> > 
> > because in the above theoretical case case where the arch uses a spinlock
> > in the atomic_t, that spinlock doesn't get initialised.  I bet we've got code
> > in there which does this.
> 
> According to Documentation/atomic_ops.txt, this is the correct usage
> of atomic_set():
> 
> |           The first operations to implement for atomic_t's are the
> |   initializers and plain reads.
> |   
> |           #define ATOMIC_INIT(i)          { (i) }
> |           #define atomic_set(v, i)        ((v)->counter = (i))
> |   
> |   The first macro is used in definitions, such as:
> |   
> |   static atomic_t my_counter = ATOMIC_INIT(1);
> |   
> |   The second interface can be used at runtime, as in:
> |   
> |           struct foo { atomic_t counter; };
> |           ...
> |   
> |           struct foo *k;
> |   
> |           k = kmalloc(sizeof(*k), GFP_KERNEL);
> |           if (!k)
> |                   return -ENOMEM;
> |           atomic_set(&k->counter, 0);
> 
> So in fact atomic_set() is an initializer, and should be named
> atomic_init() accordingly.

Yes, we're screwed.  I don't think it's possible to implement atomic_t as
spinlock+int due to this.

>  Is atomic_set() ever used as an atomic
> operation rather than an initializer?
> 

Sure, lots of places.  Lots of places where you _don't_ want your
atomic_t's spinlock to be reinitialised.

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