Re: [patch] cpufreq: mark cpufreq_tsc() as core_initcall_sync

["Paul E. McKenney" <paulmck@linux.vnet.ibm.com>]

On Wed, Nov 29, 2006 at 11:16:46PM +0300, Oleg Nesterov wrote:
> On 11/29, Paul E. McKenney wrote:
> >
> > 1.	The spinlock version will be easier for most people to understand.
> >
> > 2.	The atomic version has better read-side overhead -- probably
> > 	roughly twice as fast on most machines.
> 
> synchronize_xxx() should be a little bit faster too

Good point.

> > 3.	The atomic version will have better worst-case latency under
> > 	heavy read-side load -- at least assuming that the underlying
> > 	hardware is fair.
> >
> > 4.	The spinlock version would have better fairness in face of
> > 	synchronize_xxx() overload.
> 
> Not sure I understand (both 3 and 4) ...

The differences will be slight, and so hardware-dependent that they
don't mean much.

> > 5.	Neither version can be used from irq (but the same is true of
> > 	SRCU as well).
> 
> Hmm... SRCU can't be used from irq, yes. But I think that both versions
> (spinlock needs _irqsave) can ?

I didn't think you could call wait_event() from irq.

For the locked version, you would also need spin_lock_irqsave() or some
such to avoid self-deadlock.

For the atomic version, the fact that synchronize_qrcu() increments
the new counter before decrmenting the old one should mean that calls
to qrcu_read_lock() and qrcu_read_unlock() can be called from irq.
But synchronize_qrcu() must be called from process context, since it
can block.

This might well be important.

> > If I was to choose, I would probably go with the easy-to-understand
> > case, which would push me towards the spinlocks.  If there is a
> > read-side performance problem, then the atomic version can be easily
> > resurrected from the LKML archives.  Maybe have a URL in a comment
> > pointing to the atomic implementation?  ;-)
> 
> But it is so ugly to use spinlock to impement the memory barrier semantics!
> 
> Look,
> 
> 	void synchronize_xxx(struct xxx_struct *sp)
> 	{
> 		int idx;
> 
> 		mutex_lock(&sp->mutex);
> 
> 		spin_lock();
> 		idx = sp->completed++ & 0x1;
> 		spin_unlock();
> 
> 		wait_event(sp->wq, !sp->ctr[idx]);
> 
> 		spin_lock();
> 		spin_unlock();
> 
> 		mutex_unlock(&sp->mutex);
> 	}
> 
> Yes, it looks simpler. But why do we need an empty critical section? it is
> a memory barrier, we can (should?) instead do
> 
> 		/* for wait_event() above */
> 		smp_rmb();
> 		spin_unlock_wait();
> 		smp_mb();
> 
> Then,
> 
> 		spin_lock();
> 		idx = sp->completed++ & 0x1;
> 		spin_unlock();
> 
> means
> 		idx = sp->completed & 0x1;
> 		spin_lock();
> 		sp->completed++
> 		spin_unlock();
> 
> Again, this is a barrier, not a lock! ->completed protected by ->mutex,
> 
> 		sp->completed++;
> 		smp_mb();
> 		spin_unlock_wait(&sp->lock);
> 		/* for wait_event() below */
> 		smp_rmb();
> 
> So in fact spinlock_t is used to make inc/dec of ->ctr atomic. Doesn't
> we have atomic_t for that ?
> 
> That said, if you both think it is better - please send a patch. This is
> a matter of taste, and I am far from sure my taste is the best :)

Heck no!  I was under the mistaken impression that you had coded and
tested both patches.  If you have the atomic one working but the
spinlock one is not to that stage, I am all for going with the atomic
version.  ;-)

> > > Note: I suspect that Documentation/ lies about atomic_add_unless(), see
> > >
> > > 	http://marc.theaimsgroup.com/?l=linux-kernel&m=116448966030359
> >
> > Hmmm...  Some do and some don't:
> >
> > i386:	The x86 semantics, as I understand them, are in fact equivalent
> > 	to having a memory barrier before and after the operation.
> > 	However, the documentation I have is not as clear as it might be.
> 
> Even i386 has non-empty mb(), but atomic_read() is a plain LOAD.

Ah -- I was forgetting the failure path.  You are quite correct.

> > So either the docs or several of the architectures need fixing.
> 
> I think its better to fix the docs.

I must defer to the two Davids...

							Thanx, Paul