linux-kernel - Re: [PATCH v2 15/19] locking/lockdep: Remove __cq

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1553014484.65329.10.camel@acm.org>
Date:   Tue, 19 Mar 2019 09:54:44 -0700
From:   Bart Van Assche <bvanassche@....org>
To:     Yuyang Du <duyuyang@...il.com>, peterz@...radead.org,
        will.deacon@....com, mingo@...nel.org
Cc:     ming.lei@...hat.com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 15/19] locking/lockdep: Remove __cq_empty()

On Mon, 2019-03-18 at 16:57 +0800, Yuyang Du wrote:
> __cq_empty() can be embeded in __cq_dequeue(), removing it. We get slightly
> simpler code. No functional change.

Does inlining __cq_empty() really improve readability of the lockdep code?

> -static inline int __cq_dequeue(struct circular_queue *cq, struct lock_list **elem)
> +/*
> + * Dequeue an element from the circular_queue, return the lock if the queue
> + * is not empty, or NULL if otherwise
> + */
> +static inline struct lock_list * __cq_dequeue(struct circular_queue *cq)
>  {
> -       if (__cq_empty(cq))
> -               return -1;
> +       struct lock_list * lock;
>  
> -       *elem = cq->element[cq->front];
> +       /*
> +        * Is the circular_queue empty?
> +        */
> +       if (cq->front == cq->rear)
> +               return NULL;
> +
> +       lock = cq->element[cq->front];
>         cq->front = (cq->front + 1) & CQ_MASK;
> -       return 0;
> +
> +       return lock;
>  }
>  
>  static inline unsigned int  __cq_get_elem_count(struct circular_queue *cq)
> @@ -1376,6 +1381,7 @@ static int __bfs(struct lock_list *source_entry,
>                  int forward)
>  {
>         struct lock_list *entry;
> +       struct lock_list *lock;
>         struct list_head *head;
>         struct circular_queue *cq = &lock_cq;
>         int ret = 1;
> @@ -1397,10 +1403,7 @@ static int __bfs(struct lock_list *source_entry,
>         __cq_init(cq);
>         __cq_enqueue(cq, source_entry);
>  
> -       while (!__cq_empty(cq)) {
> -               struct lock_list *lock;
> -
> -               __cq_dequeue(cq, &lock);
> +       while ((lock = __cq_dequeue(cq))) {
>  
>                 if (!lock->class) {
>                         ret = -2;

This is the most important change in this patch. Using the title "Remove __cq_empty()"
for this patch is misleading because the above patch does something else, namely changing
the return type of __cq_dequeue() from int into a pointer. Should this patch perhaps be
split or should the __cq_empty() removal be left out from this patch?

Bart.