CPU ordering with respect to krefs

CPU ordering with respect to krefs

[Oliver Neukum]

Hi,

some atomic operations are only atomic, not ordered. Thus a CPU is allowed
to reorder memory references to an object to before the reference is
obtained. This fixes it.

	Regards
		Oliver
Signed-off-by: Oliver Neukum <oneukum@suse.de>
------

--- a/lib/kref.c	2007-04-02 14:40:40.000000000 +0200
+++ b/lib/kref.c	2007-04-02 14:40:50.000000000 +0200
@@ -21,6 +21,7 @@
 void kref_init(struct kref *kref)
 {
 	atomic_set(&kref->refcount,1);
+	smp_mb();
 }
 
 /**
@@ -31,6 +32,7 @@
 {
 	WARN_ON(!atomic_read(&kref->refcount));
 	atomic_inc(&kref->refcount);
+	smp_mb__after_atomic_inc();
 }
 
 /**

Re: CPU ordering with respect to krefs

[Eric Dumazet]

On Mon, 2 Apr 2007 14:47:59 +0200
Oliver Neukum <oneukum@suse.de> wrote:

> Hi,
> 
> some atomic operations are only atomic, not ordered. Thus a CPU is allowed
> to reorder memory references to an object to before the reference is
> obtained. This fixes it.
> 
> 	Regards
> 		Oliver
> Signed-off-by: Oliver Neukum <oneukum@suse.de>
> ------
> 
> --- a/lib/kref.c	2007-04-02 14:40:40.000000000 +0200
> +++ b/lib/kref.c	2007-04-02 14:40:50.000000000 +0200
> @@ -21,6 +21,7 @@
>  void kref_init(struct kref *kref)
>  {
>  	atomic_set(&kref->refcount,1);
> +	smp_mb();
>  }

I dont understand why smp_mb() is needed here, and not in spinlock_init() for example.

If you have ordering issues, then the caller of kref_init() should take care of it, not kref_init() itself.

Random example taken in drivers/usb/gadget/file_storage.c :

static int __init fsg_alloc(void)
{
...
kref_init(&fsg->ref);
init_completion(&fsg->thread_notifier);

the_fsg = fsg;
}

In this example, "the_fsg = fsg" memory write might be visible before the memory writes done in init_completion().
Doing a smp_mb() in kref_init() wont help.

AFAIK kref implementation doesnt need this extra smp_mb().

Re: CPU ordering with respect to krefs

[Greg KH]

On Mon, Apr 02, 2007 at 04:33:54PM +0200, Eric Dumazet wrote:
> On Mon, 2 Apr 2007 14:47:59 +0200
> Oliver Neukum <oneukum@suse.de> wrote:
> 
> > Hi,
> > 
> > some atomic operations are only atomic, not ordered. Thus a CPU is allowed
> > to reorder memory references to an object to before the reference is
> > obtained. This fixes it.
> > 
> > 	Regards
> > 		Oliver
> > Signed-off-by: Oliver Neukum <oneukum@suse.de>
> > ------
> > 
> > --- a/lib/kref.c	2007-04-02 14:40:40.000000000 +0200
> > +++ b/lib/kref.c	2007-04-02 14:40:50.000000000 +0200
> > @@ -21,6 +21,7 @@
> >  void kref_init(struct kref *kref)
> >  {
> >  	atomic_set(&kref->refcount,1);
> > +	smp_mb();
> >  }
> 
> I dont understand why smp_mb() is needed here, and not in
> spinlock_init() for example.

I think, after reading the Documentation/memory-barriers.txt and
Documentation/atomic_ops.txt documentation, that spin_lock_init() also
needs this kind of memory barrier.

>From what I can tell (Oliver, please correct me if I'm wrong, you know
this much better than I do), the issue is that atomic init has no memory
barrier, and you need to handle that.

thanks,

greg k-h

Re: CPU ordering with respect to krefs

[Oliver Neukum]

Am Donnerstag, 12. April 2007 08:27 schrieb Greg KH:
> On Mon, Apr 02, 2007 at 04:33:54PM +0200, Eric Dumazet wrote:
> > On Mon, 2 Apr 2007 14:47:59 +0200
> > Oliver Neukum <oneukum@suse.de> wrote:
> > 
> > > Hi,
> > > 
> > > some atomic operations are only atomic, not ordered. Thus a CPU is allowed
> > > to reorder memory references to an object to before the reference is
> > > obtained. This fixes it.
> > > 
> > > 	Regards
> > > 		Oliver
> > > Signed-off-by: Oliver Neukum <oneukum@suse.de>
> > > ------
> > > 
> > > --- a/lib/kref.c	2007-04-02 14:40:40.000000000 +0200
> > > +++ b/lib/kref.c	2007-04-02 14:40:50.000000000 +0200
> > > @@ -21,6 +21,7 @@
> > >  void kref_init(struct kref *kref)
> > >  {
> > >  	atomic_set(&kref->refcount,1);
> > > +	smp_mb();
> > >  }
> > 
> > I dont understand why smp_mb() is needed here, and not in
> > spinlock_init() for example.
> 
> I think, after reading the Documentation/memory-barriers.txt and
> Documentation/atomic_ops.txt documentation, that spin_lock_init() also
> needs this kind of memory barrier.

spin_lock_init() is not an atomic operation.
In principle, the issue exists. However, the whole issue is a bit of a grey
area. You might take the viewpoint that upping the refcount needs to be
under lock, which needs to take care of ordering issues in case of krefs.
A new spinlock has the same issue. You need to be careful making them
accessible to other CPUs.

If you take code like:

static int producer()
{
	...
	data = kmalloc(...);
	spin_lock_init(&data->lock);
	data->value = some_value;
	data->next = global_pointer;

	global_pointer = data;
	...
}
	
You have an ordering bug anyway, which you can't fix in spin_lock_init().

	Regards
		Oliver