Re: csky: smp_mb__after_spinlock

[peterz]

On Fri, Aug 07, 2020 at 08:22:36AM +0800, Guo Ren wrote:
> Hi Peter,
> 
> On Thu, Aug 6, 2020 at 3:53 AM <peterz@xxxxxxxxxxxxx> wrote:
> >
> > Hi,
> >
> > While doing an audit of smp_mb__after_spinlock, I found that csky
> > defines it, why?
> >
> > CSKY only has smp_mb(), it doesn't override __atomic_acquire_fence or
> > otherwise special cases it's atomic*_acquire() primitives. It has an
> > explicit smp_mb() in its arch_spin_lock().
> 
> Yes, csky didn't implement ACQUIRE/RELEASE in spinlock.h. So it is a
> redundant and side-effect wrong macro, we should remove it to fixup.
> 
> TODO:
>  - implement csky's ACQUIRE/RELEASE barrier

Fair enough; please Cc me when you get to it.

> > Also, why have two implementations of all the locking?
> 
> I just kept my baby's codes :P. Now, we could remove it and just keep
> the ticket's one.
> 
> BTW, I want to change the spinlock to qspinlock, but csky only has
> 32-bit atomic operation in hardware.

Yeah, that's okay, you can do 'short' atomics using wider RmW.

> Any plan to deal with this in spinlock?
> 
> Maybe for the embedded scenario, qspinlock seems a bit heavy,

I tend to agree, qspinlock only really makes sense when you have
multiple cache domains and NUMA makes it shine.

> are any
> tickets-like comm spinlock infrastructures in the plan?

No. ticket locks are 'simple' and their implementation fairly straight
forward. Also, since the generic code only has cmpxchg, a generic ticket
lock would be sub-optimal on LL/SC (or even x86 for that matter).

It would look something like this, which I think is pretty terrible for
just about any arch compared to what you can do with inline asm.

struct ticket_lock {
	union {
		atomic_t	val;
		struct {
#ifdef __LITTLE_ENDIAN
			u16	head;
			u16	tail;
#else
			u16	tail;
			u16	head;
#endif
		};
	};
};

void ticket_lock(struct ticket_lock *lock)
{
	unsigned int val = atomic_read(&lock->val);
	struct ticket_lock lock;

	do {
		lock.val = ATOMIC_INIT(val);
		lock.head++;
	} while (!atomic_try_cmpxchg_relaxed(&lock->lock, &val, lock.val));

	atomic_cond_read_acquire(&lock->val, (VAL >> 16) == lock.head);
}

void ticket_unlock(struct ticket_lock *lock)
{
	atomic_fetch_inc_release(&lock->val);
}