Re: [RFC][PATCH] mips: Fix arch_spin_unlock()

[Paul E. McKenney]

On Tue, Feb 02, 2016 at 12:19:04AM -0800, Linus Torvalds wrote:
> On Tue, Feb 2, 2016 at 12:07 AM, Linus Torvalds
> <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote:
> >
> > So we *absolutely* should say that *OF COURSE* these things work:
> >
> >  - CPU A:
> >
> >    .. initialize data structure -> smp_wmb() -> WRITE_ONCE(ptr);
> >
> >  - CPU B:
> >
> >    smp_load_acquire(ptr)  - we can rely on things behind "ptr" being initialized
> 
> That's a bad example, btw. I shouldn't have made it be a "pointer",
> because then we get the whole address dependency chain ordering
> anyway.
> 
> So instead of "ptr", read "state flag". It might just be an "int" that
> says "data has been initialized".
> 
> So
> 
>     .. initialize memory ..
>     smp_wmb();
>     WRITE_ONCE(&is_initialized, 1);
> 
> should pair with
> 
>     if (smp_load_acquire(&is_initialized))
>         ... we can read and write the data, knowing it has been initialized ..
> 
> exactly because "smp_wmb()" (cheap write barrier) might be cheaper
> than "smp_store_release()" (expensive full barrier) and thus
> preferred.
> 
> So mixing ordering metaphors actually does make sense, and should be
> entirely well-defined.

I don't believe that anyone is arguing that this particular example
should not work the way that you want it to.

> There's likely less reason to do it the other way (ie
> "smp_store_release()" on one side pairing with "LOAD_ONCE() +
> smp_rmb()" on the other) since there likely isn't the same kind of
> performance reason for that pairing. But even if we would never
> necessarily want to do it, I think our memory ordering rules would be
> *much* better for strongly stating that it has to work, than being
> timid and trying to make the rules weak.
> 
> Memory ordering is confusing enough as it is. We should not make
> people worry more than they already have to. Strong rules are good.

The sorts of things I am really worried about are abominations like this
(and far worse):

	void thread0(void)
	{
		r1 = smp_load_acquire(&a);
		smp_store_release(&b, 1);
	}

	void thread1(void)
	{
		r2 = smp_load_acquire(&b);
		smp_store_release(&c, 1);
	}

	void thread2(void)
	{
		WRITE_ONCE(c, 2);
		smp_mb();
		r3 = READ_ONCE(d);
	}

	void thread3(void)
	{
		WRITE_ONCE(d, 1);
		smp_store_release(&a, 1);
	}

	r1 == 1 && r2 == 1 && c == 2 && r3 == 0 ???

I advise discouraging this sort of thing.  But it is your kernel, so
what is your preference?

							Thanx, Paul