That's all nice, but does it improve performance? Have you been able
to measure some performance difference with that code? Were you
targeting a specific inefficiency you had seen e.g. with a CPU
profiler?
Marek
On Thu, Jun 22, 2017 at 8:19 PM, axie <axie at amd.com> wrote:
> To clarify, local IRQ is disabled by calling raw_local_irq_save(flags);
>
> Function __lock_acquire double checks that the local IRQ is really disabled.
>
>
>
> On 2017-06-22 01:34 PM, axie wrote:
>>
>> Hi Marek,
>>
>> Spin lock and spin unlock is fast. But it is not so fast compared with
>> atomic, which is a single CPU instruction in x86.
>>
>>
>> 1. spinlock does NOT allow preemption at local CPU. Let us have a look at
>> how spin lock was implemented.
>>
>> static inline void __raw_spin_lock(raw_spinlock_t *lock)
>> {
>> preempt_disable(); <<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<<--This is
>> memory barrier operation too.
>> spin_acquire(&lock->dep_map, 0, 0, _RET_IP_);
>> LOCK_CONTENDED(lock, do_raw_spin_trylock, do_raw_spin_lock);
>> }
>>
>> 2. A function __lock_acquire called by spinlock. The function is so long
>> that I would not attach all of it here.
>>
>> There is atomic operation inside and 12 meta data updates and 14 if
>> statements and it calls quite some other functions.
>>
>> Note that it disable IRQ...
>>
>> static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>> int trylock, int read, int check, int hardirqs_off,
>> struct lockdep_map *nest_lock, unsigned long ip,
>> int references, int pin_count)
>> {
>> struct task_struct *curr = current;
>> struct lock_class *class = NULL;
>> struct held_lock *hlock;
>> unsigned int depth;
>> int chain_head = 0;
>> int class_idx;
>> u64 chain_key;
>>
>> if (unlikely(!debug_locks))
>> return 0;
>>
>> /*
>> * Lockdep should run with IRQs disabled, otherwise we could
>> * get an interrupt which would want to take locks, which would
>> * end up in lockdep and have you got a head-ache already?
>> */
>> if (DEBUG_LOCKS_WARN_ON(!irqs_disabled())) <<<<<<<<<<<<<<<Disable IRQ
>> return 0;
>>
>> ....
>>
>> 3. Another function called by spinlock in a higher level:
>>
>> void lock_acquire(struct lockdep_map *lock, unsigned int subclass,
>>
>> int trylock, int read, int check,
>> struct lockdep_map *nest_lock, unsigned long ip)
>> {
>> unsigned long flags;
>>
>> if (unlikely(current->lockdep_recursion))
>> return;
>>
>> raw_local_irq_save(flags);
>> check_flags(flags);
>>
>> current->lockdep_recursion = 1;
>> trace_lock_acquire(lock, subclass, trylock, read, check, nest_lock,
>> ip);
>> __lock_acquire(lock, subclass, trylock, read, check,
>> irqs_disabled_flags(flags), nest_lock, ip, 0, 0);
>> current->lockdep_recursion = 0;
>> raw_local_irq_restore(flags);
>> }
>>
>>
>> Thanks,
>>
>> Alex Bin
>>
>>
>> On 2017-06-22 12:27 PM, Marek Olšák wrote:
>>>
>>> On Thu, Jun 22, 2017 at 5:33 PM, Xie, AlexBin <AlexBin.Xie at amd.com>
>>> wrote:
>>>>
>>>> Hi Christian,
>>>>
>>>>
>>>> In fact, the change from spinlock to atomic is quite painful. When I
>>>> started, I thought it was easy but later I found there might be race
>>>> condition here and there. Now I think the change looks more robust. In
>>>> kernel source, there are several other drivers used the same trick.
>>>>
>>>>
>>>> On the other hand, I think the logic itself might be optimized
>>>> considering
>>>> the locking. But I had spent quite some effort to maintain original
>>>> logic.
>>>
>>> It seems quite complicated and I don't know if there is any
>>> performance benefit. Spinlocks are nice because they allow preemption.
>>>
>>> It would be more interesting to merge the CS and BO_LIST ioctls into one.
>>>
>>> Marek
>>
>>
>
