Analyze if spinlocks have any benefit and remove them if not

[vh05]

Glusterfs currently support spinlocks and mutexes. It's not quite understandable to that if spinlocks have any advantage on user mode when running more threads than cores regularly and threads themselves don't run at high priority. If they don't really provide any benefit, we should remove them and simplify locking macros.

[itisravi]

With commit ac248c8, spinlocks were made to be enabled on multi core systems only, so not sure if we need to remove them.

[vh05]

Thanks, Ravi for the input.

The concern is why we need to use spin_locks when the job can be done using mutex.
The places where we used spin_locks

1. do not share the context with interrupts
2. priority is not a problem
3. the code under spin_lock is going to take more time than the context switch
4. The code may go to sleep

ex: aws_init() in cloudsync, doing dict_get holding a priv->spin_lock. Not sure why we need spin_lock in such instances.

Mostly in our code, we use spin_lock where the processing time is high.

Please give your opinion further

[itisravi]

@VHariharmath-rh I agree that mutexes are better than spinlocks in general. I was only saying that if you run tests on a multi core system (which is pretty much all machines), it already uses mutexes (because of the above patch). So I was wondering how you were planning to analyse the performance difference.

[vh05]

According to the patch, on multicore systems, the spin_locks are used by default, Am I missing anything?

So I was wondering how you were planning to analyse the performance difference.
I am thinking to do it in 2 phases.

1. analyze and replace explicit declarations spin_lock with mutex. (maybe gf_lock_t)
2. The macros LOCK and UNLOCK (introduced in the patch) made to use mutex default if spin_locks found to be not necessary.

With these changes, I collect the numbers running fio.

[itisravi]

According to the patch, on multicore systems, the spin_locks are used by default, Am I missing anything?

Ah sorry, my bad. I got it mixed up. The steps makes sense.

[mykaul]

If we can also remove this use_spinlocks variable and just normally ifdef the code, that'd be nice. I'm sure in debug mode it is calling this variable, I don't know about release. I don't see the value in that.

[vh05]

If I understand correctly, use_spinlocks is used to decide between spin_lock or mutex. If mutex makes sense than a spin_lock then HAVE_SPINLOCK section along with the use_spinlocks can be removed

[mykaul]

I'd start with a simple ifdef instead of consulting a variable.

[vh05]

The variable use_spinlocks is commented in the current codebase, holds the value = 0, and the default behavior is to use a mutex lock. So HAVE_SPINLOCK section in locking.h simply lead to mutex always.

libglusterfs/src/locking.c

int use_spinlocks = 0;
static void __attribute__((constructor)) gf_lock_setup(void)
{
    // use_spinlocks = (sysconf(_SC_NPROCESSORS_ONLN) > 1);
}

[mykaul]

Are you sure?

glusterfs/libglusterfs/src/locking.c

Line 20 in 2d8c88d

int use_spinlocks = 0;

[vh05]

Yes, @mykaul. I did not find anywhere in the code use_spinlocks is set.