[PATCH 1/3] drm/amdgpu: fix a typo

[axie]

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
>