linux-kernel - Re: [PATCH v2 16/19] locking/lockdep: Use function pointer to avoid constant checks

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: <1553014609.65329.12.camel@acm.org>
Date:   Tue, 19 Mar 2019 09:56:49 -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 16/19] locking/lockdep: Use function pointer to avoid
 constant checks

On Mon, 2019-03-18 at 16:57 +0800, Yuyang Du wrote:
> +static inline struct list_head *get_forward_dep(struct lock_list * lock)
> +{
> +       return &lock->class->locks_after;
> +}
> +
> +static inline struct list_head *get_backward_dep(struct lock_list * lock)
> +{
> +       return &lock->class->locks_before;
> +}
> +
>  static int __bfs(struct lock_list *source_entry,
>                  void *data,
>                  int (*match)(struct lock_list *entry, void *data),
>                  struct lock_list **target_entry,
> -                int forward)
> +                struct list_head *(*get_dep)(struct lock_list * lock))
>  {
>         struct lock_list *entry;
>         struct lock_list *lock;
> @@ -1392,11 +1402,7 @@ static int __bfs(struct lock_list *source_entry,
>                 goto exit;
>         }
>  
> -       if (forward)
> -               head = &source_entry->class->locks_after;
> -       else
> -               head = &source_entry->class->locks_before;
> -
> +       head = get_dep(source_entry);
>         if (list_empty(head))
>                 goto exit;
>  
> @@ -1410,10 +1416,7 @@ static int __bfs(struct lock_list *source_entry,
>                         goto exit;
>                 }
>  
> -               if (forward)
> -                       head = &lock->class->locks_after;
> -               else
> -                       head = &lock->class->locks_before;
> +               head = get_dep(lock);
>  
>                 DEBUG_LOCKS_WARN_ON(!irqs_disabled());
>  
> @@ -1445,7 +1448,7 @@ static inline int __bfs_forwards(struct lock_list *src_entry, void *data,
>                                  int (*match)(struct lock_list *entry, void *data),
>                                  struct lock_list **target_entry)
>  {
> -       return __bfs(src_entry, data, match, target_entry, 1);
> +       return __bfs(src_entry, data, match, target_entry, get_forward_dep);
>  
>  }
>  
> @@ -1453,7 +1456,7 @@ static inline int __bfs_backwards(struct lock_list *src_entry, void *data,
>                                   int (*match)(struct lock_list *entry, void *data),
>                                   struct lock_list **target_entry)
>  {
> -       return __bfs(src_entry, data, match, target_entry, 0);
> +       return __bfs(src_entry, data, match, target_entry, get_backward_dep);
>  
>  }

I think this patch makes the code harder to read. Additionally, if the compiler doesn't
do conditional folding, this patch introduces an indirect branch and hence will make the
lockdep code slower.

Bart.