2020-09-21 23:49:16

by Shuah Khan

Subject: drivers/misc/habanalabs: atomic_t api usage inconsistencies

All,

While I was looking at the atomic_t api usages for an unrelated issue,
I noticed free_slots_cnt in struct hl_cq incerment/decrement/reads are
not consistent.

atomic_inc() and atomic_set() are used, however instead of atomic_read()
the value is referenced directly in
drivers/misc/habanalabs/common/hw_queue.c

hl_queue_add_ptr()
atomic_t *free_slots = &hdev->completion_queue[q->cq_id].free_slots_cnt;

hl_hw_queue_schedule_cs()

atomic_t *free_slots = &hdev->completion_queue[i].free_slots_cnt;

Any reason why this is necessary. I don't know that this is causing
any problems, it is just odd that access is inconsistent.

thanks,
-- Shuah

2020-09-23 08:43:43

by Oded Gabbay

Subject: Re: drivers/misc/habanalabs: atomic_t api usage inconsistencies

On Tue, Sep 22, 2020 at 1:08 AM Shuah Khan <[email protected]> wrote:
>
> All,
>
> While I was looking at the atomic_t api usages for an unrelated issue,
> I noticed free_slots_cnt in struct hl_cq incerment/decrement/reads are
> not consistent.
>
> atomic_inc() and atomic_set() are used, however instead of atomic_read()
> the value is referenced directly in
> drivers/misc/habanalabs/common/hw_queue.c
>
> hl_queue_add_ptr()
> atomic_t *free_slots = &hdev->completion_queue[q->cq_id].free_slots_cnt;
>
> hl_hw_queue_schedule_cs()
>
> atomic_t *free_slots = &hdev->completion_queue[i].free_slots_cnt;
>
> Any reason why this is necessary. I don't know that this is causing
> any problems, it is just odd that access is inconsistent.
>
> thanks,
> -- Shuah

Hi Shuah,
Thanks for taking notice of this issue :)
We will take a deeper look and fix the inconsistencies, although I
must say that we didn't notice any impact of this issue.
Thanks again.
Oded

2020-09-23 19:40:24

by Shuah Khan

Subject: Re: drivers/misc/habanalabs: atomic_t api usage inconsistencies

On 9/23/20 2:41 AM, Oded Gabbay wrote:
> On Tue, Sep 22, 2020 at 1:08 AM Shuah Khan <[email protected]> wrote:
>>
>> All,
>>
>> While I was looking at the atomic_t api usages for an unrelated issue,
>> I noticed free_slots_cnt in struct hl_cq incerment/decrement/reads are
>> not consistent.
>>
>> atomic_inc() and atomic_set() are used, however instead of atomic_read()
>> the value is referenced directly in
>> drivers/misc/habanalabs/common/hw_queue.c
>>
>> hl_queue_add_ptr()
>> atomic_t *free_slots = &hdev->completion_queue[q->cq_id].free_slots_cnt;
>>
>> hl_hw_queue_schedule_cs()
>>
>> atomic_t *free_slots = &hdev->completion_queue[i].free_slots_cnt;
>>
>> Any reason why this is necessary. I don't know that this is causing
>> any problems, it is just odd that access is inconsistent.
>>
>> thanks,
>> -- Shuah
>
> Hi Shuah,
> Thanks for taking notice of this issue :)
> We will take a deeper look and fix the inconsistencies, although I
> must say that we didn't notice any impact of this issue.

If you haven't noticed any problems, it could be that this variable
doesn't need to atomic or you haven't run into it yet?

thanks,
-- Shuah