Date:   Wed, 4 Jan 2023 20:11:05 +1100
From:   Imran Khan <imran.f.khan@...cle.com>
To:     Tejun Heo <tj@...nel.org>
Cc:     gregkh@...uxfoundation.org, viro@...iv.linux.org.uk,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [RESEND PATCH 4/5] kernfs: Replace per-fs rwsem with hashed
 rwsems.

Hello Tejun

On 14/11/2022 4:30 am, Imran Khan wrote:

[...]
> 
> Below is reasoning and data for my experiments with other approaches.
> 
> Since hashed kernfs_rwsem approach has been encountering problems in addressing
> some corner cases, I am thinking if some alternative approach can be taken here
> to keep kernfs_rwsem global, but replace its usage at some places with
> alternative global/hashed rwsems.
> 
> For example from the current kernfs code we can see that most of the contention
> towards kernfs_rwsem is observed in down_read/up_read emanating from
> kernfs_iop_permission and kernfs_dop_revalidate:
> 
> 	-   39.16%    38.98%  showgids     [kernel.kallsyms]      [k] down_read
>              38.98% __libc_start_main
>                 __open_nocancel
>                 entry_SYSCALL_64_after_hwframe
>                 do_syscall_64
>                 sys_open
>                 do_sys_open
>                 do_filp_open
>               - path_openat
>                  - 36.54% link_path_walk
>                     - 20.23% inode_permission
>                        - __inode_permission
>                           - 20.22% kernfs_iop_permission
>                                down_read
>                     - 15.06% walk_component
>                          lookup_fast
>                          d_revalidate.part.24
>                          kernfs_dop_revalidate
>                          down_read
>                     - 1.25% kernfs_iop_get_link
>                          down_read
>                  - 1.25% may_open
>                       inode_permission
>                       __inode_permission
>                       kernfs_iop_permission
>                       down_read
>                  - 1.20% lookup_fast
>                       d_revalidate.part.24
>                       kernfs_dop_revalidate
>                       down_read
> 	-   28.96%    28.83%  showgids     [kernel.kallsyms]       [k] up_read
>              28.83% __libc_start_main
>                 __open_nocancel
>                 entry_SYSCALL_64_after_hwframe
>                 do_syscall_64
>                 sys_open
>                 do_sys_open
>                 do_filp_open
>               - path_openat
>                  - 28.42% link_path_walk
>                     - 18.09% inode_permission
>                        - __inode_permission
>                           - 18.07% kernfs_iop_permission
>                                up_read
>                     - 9.08% walk_component
>                          lookup_fast
>                        - d_revalidate.part.24
>                           - 9.08% kernfs_dop_revalidate
>                                up_read
>                     - 1.25% kernfs_iop_get_link
> 
> In the above snippet down_read/up_read of kernfs_rwsem is taking ~68% of CPU. We
> also know that cache line bouncing for kernfs_rwsem is major contributor towards
> this overhead because as per [2], changing kernfs_rwsem to a per-cpu
> kernfs_rwsem reduced this to a large extent.
> 
> Now kernfs_iop_permission is taking kernfs_rwsem to access inode attributes
> which are not accessed in kernfs_dop_revalidate (the other path contending for
> kernfs_rwsem). So if we use a separate rwsem for protecting inode attributes we
> can see that contention towards kernfs_rwsem greatly reduces. For example using
> a global rwsem (kernfs_iattr_rwsem) to protect inode attributes as per following
> patch:
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 6acd9c3d4cff..f185427131f9 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -757,11 +757,13 @@ int kernfs_add_one(struct kernfs_node *kn)
>                 goto out_unlock;
> 
>         /* Update timestamps on the parent */
> +       down_write(&root->kernfs_iattr_rwsem);
>         ps_iattr = parent->iattr;
>         if (ps_iattr) {
>                 ktime_get_real_ts64(&ps_iattr->ia_ctime);
>                 ps_iattr->ia_mtime = ps_iattr->ia_ctime;
>         }
> +       up_write(&root->kernfs_iattr_rwsem);
> 
>         up_write(&root->kernfs_rwsem);
> 
> @@ -1442,10 +1444,12 @@ static void __kernfs_remove(struct kernfs_node *kn)
>                                 pos->parent ? pos->parent->iattr : NULL;
> 
>                         /* update timestamps on the parent */
> +                       down_write(&root->kernfs_iattr_rwsem);
>                         if (ps_iattr) {
>                                 ktime_get_real_ts64(&ps_iattr->ia_ctime);
>                                 ps_iattr->ia_mtime = ps_iattr->ia_ctime;
>                         }
> +                       up_write(&root->kernfs_iattr_rwsem);
> 
>                         kernfs_put(pos);
>                 }
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 74f3453f4639..1b8bffc6d2d3 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -101,9 +101,9 @@ int kernfs_setattr(struct kernfs_node *kn, const struct
> iattr *iattr)
>         int ret;
>         struct kernfs_root *root = kernfs_root(kn);
> 
> -       down_write(&root->kernfs_rwsem);
> +       down_write(&root->kernfs_iattr_rwsem);
>         ret = __kernfs_setattr(kn, iattr);
> -       up_write(&root->kernfs_rwsem);
> +       up_write(&root->kernfs_iattr_rwsem);
>         return ret;
>  }
> 
> @@ -119,7 +119,7 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
> struct dentry *dentry,
>                 return -EINVAL;
> 
>         root = kernfs_root(kn);
> -       down_write(&root->kernfs_rwsem);
> +       down_write(&root->kernfs_iattr_rwsem);
>         error = setattr_prepare(&init_user_ns, dentry, iattr);
>         if (error)
>                 goto out;
> @@ -132,7 +132,7 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
> struct dentry *dentry,
>         setattr_copy(&init_user_ns, inode, iattr);
> 
>  out:
> -       up_write(&root->kernfs_rwsem);
> +       up_write(&root->kernfs_iattr_rwsem);
>         return error;
>  }
> @@ -189,10 +189,10 @@ int kernfs_iop_getattr(struct user_namespace *mnt_userns,
>         struct kernfs_node *kn = inode->i_private;
>         struct kernfs_root *root = kernfs_root(kn);
> 
> -       down_read(&root->kernfs_rwsem);
> +       down_read(&root->kernfs_iattr_rwsem);
>         kernfs_refresh_inode(kn, inode);
>         generic_fillattr(&init_user_ns, inode, stat);
> -       up_read(&root->kernfs_rwsem);
> +       up_read(&root->kernfs_iattr_rwsem);
> 
>         return 0;
>  }
> 
> @@ -285,10 +285,10 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns,
>         kn = inode->i_private;
>         root = kernfs_root(kn);
> 
> -       down_read(&root->kernfs_rwsem);
> +       down_read(&root->kernfs_iattr_rwsem);
>         kernfs_refresh_inode(kn, inode);
>         ret = generic_permission(&init_user_ns, inode, mask);
> -       up_read(&root->kernfs_rwsem);
> +       up_read(&root->kernfs_iattr_rwsem);
> 
>         return ret;
>  }
> 
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index fc5821effd97..4620b74f44b0 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -47,6 +47,7 @@ struct kernfs_root {
> 
>         wait_queue_head_t       deactivate_waitq;
>         struct rw_semaphore     kernfs_rwsem;
> +       struct rw_semaphore     kernfs_iattr_rwsem;
>  };
> 
> 
> 
> greatly reduces the CPU usage seen earlier in down_read/up_read:
> 
> 
>  -   13.08%    13.02%  showgids       [kernel.kallsyms]       [k] down_read
>              13.02% __libc_start_main
>                 __open_nocancel
>                 entry_SYSCALL_64_after_hwframe
>                 do_syscall_64
>                 sys_open
>                 do_sys_open
>                 do_filp_open
>               - path_openat
>                  - 12.18% link_path_walk
>                     - 9.44% inode_permission
>                        - __inode_permission
>                           - 9.43% kernfs_iop_permission
>                                down_read
>                     - 2.53% walk_component
>                          lookup_fast
>                          d_revalidate.part.24
>                          kernfs_dop_revalidate
>                          down_read
>                  + 0.62% may_open
>         -   13.03%    12.97%  showgids       [kernel.kallsyms]      [k] up_read
>              12.97% __libc_start_main
>                 __open_nocancel
>                 entry_SYSCALL_64_after_hwframe
>                 do_syscall_64
>                 sys_open
>                 do_sys_open
>                 do_filp_open
>               - path_openat
>                  - 12.86% link_path_walk
>                     - 11.55% inode_permission
>                        - __inode_permission
>                           - 11.54% kernfs_iop_permission
>                                up_read
>                     - 1.06% walk_component
>                          lookup_fast
>                        - d_revalidate.part.24
>                           - 1.06% kernfs_dop_revalidate
> 
> As can be seen down_read/up_read CPU usage is ~26% (compared to ~68% of default
> case).
> 
> Further using a hashed rwsem for protecting inode attributes as per following patch:
> 
> diff --git a/fs/kernfs/dir.c b/fs/kernfs/dir.c
> index 6acd9c3d4cff..dfc0d2167d86 100644
> --- a/fs/kernfs/dir.c
> +++ b/fs/kernfs/dir.c
> @@ -734,6 +734,7 @@ int kernfs_add_one(struct kernfs_node *kn)
>         struct kernfs_iattrs *ps_iattr;
>         bool has_ns;
>         int ret;
> +       struct rw_semaphore *rwsem;
> 
>         down_write(&root->kernfs_rwsem);
> 
> @@ -757,11 +758,13 @@ int kernfs_add_one(struct kernfs_node *kn)
>                 goto out_unlock;
> 
>         /* Update timestamps on the parent */
> +       rwsem = kernfs_iattr_down_write(kn);
>         ps_iattr = parent->iattr;
>         if (ps_iattr) {
>                 ktime_get_real_ts64(&ps_iattr->ia_ctime);
>                 ps_iattr->ia_mtime = ps_iattr->ia_ctime;
>         }
> +       kernfs_iattr_up_write(rwsem, kn);
> 
>         up_write(&root->kernfs_rwsem);
> 
> @@ -1443,8 +1446,10 @@ static void __kernfs_remove(struct kernfs_node *kn)
> 
>                         /* update timestamps on the parent */
>                         if (ps_iattr) {
> +                               struct rw_semaphore *rwsem =
> kernfs_iattr_down_write(pos->parent);
>                                 ktime_get_real_ts64(&ps_iattr->ia_ctime);
>                                 ps_iattr->ia_mtime = ps_iattr->ia_ctime;
> +                               kernfs_iattr_up_write(rwsem, kn);
>                         }
> 
>                         kernfs_put(pos);
> diff --git a/fs/kernfs/inode.c b/fs/kernfs/inode.c
> index 74f3453f4639..2b3cd5a9464f 100644
> --- a/fs/kernfs/inode.c
> +++ b/fs/kernfs/inode.c
> @@ -99,11 +99,12 @@ int __kernfs_setattr(struct kernfs_node *kn, const struct
> iattr *iattr)
>  int kernfs_setattr(struct kernfs_node *kn, const struct iattr *iattr)
>  {
>         int ret;
> +       struct rw_semaphore *rwsem;
>         struct kernfs_root *root = kernfs_root(kn);
> 
> -       down_write(&root->kernfs_rwsem);
> +       rwsem = kernfs_iattr_down_write(kn);
>         ret = __kernfs_setattr(kn, iattr);
> -       up_write(&root->kernfs_rwsem);
> +       kernfs_iattr_up_write(rwsem, kn);
>         return ret;
>  }
> 
> @@ -114,12 +115,13 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
> struct dentry *dentry,
>         struct kernfs_node *kn = inode->i_private;
>         struct kernfs_root *root;
>         int error;
> +       struct rw_semaphore *rwsem;
> 
>         if (!kn)
>                 return -EINVAL;
> 
>         root = kernfs_root(kn);
> -       down_write(&root->kernfs_rwsem);
> +       rwsem = kernfs_iattr_down_write(kn);
>         error = setattr_prepare(&init_user_ns, dentry, iattr);
>         if (error)
>                 goto out;
> @@ -132,7 +134,7 @@ int kernfs_iop_setattr(struct user_namespace *mnt_userns,
> struct dentry *dentry,
>         setattr_copy(&init_user_ns, inode, iattr);
> 
>  out:
> -       up_write(&root->kernfs_rwsem);
> +       kernfs_iattr_up_write(rwsem, kn);
>         return error;
>  }
> 
> @@ -188,11 +190,12 @@ int kernfs_iop_getattr(struct user_namespace *mnt_userns,
>         struct inode *inode = d_inode(path->dentry);
>         struct kernfs_node *kn = inode->i_private;
>         struct kernfs_root *root = kernfs_root(kn);
> +       struct rw_semaphore *rwsem;
> 
> -       down_read(&root->kernfs_rwsem);
> +       rwsem = kernfs_iattr_down_read(kn);
>         kernfs_refresh_inode(kn, inode);
>         generic_fillattr(&init_user_ns, inode, stat);
> -       up_read(&root->kernfs_rwsem);
> +       kernfs_iattr_up_read(rwsem, kn);
> 
>         return 0;
>  }
> @@ -278,6 +281,7 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns,
>         struct kernfs_node *kn;
>         struct kernfs_root *root;
>         int ret;
> +       struct rw_semaphore *rwsem;
> 
>         if (mask & MAY_NOT_BLOCK)
>                 return -ECHILD;
> @@ -285,10 +289,10 @@ int kernfs_iop_permission(struct user_namespace *mnt_userns,
>         kn = inode->i_private;
>         root = kernfs_root(kn);
> 
> -       down_read(&root->kernfs_rwsem);
> +       rwsem = kernfs_iattr_down_read(kn);
>         kernfs_refresh_inode(kn, inode);
>         ret = generic_permission(&init_user_ns, inode, mask);
> -       up_read(&root->kernfs_rwsem);
> +       kernfs_iattr_up_read(rwsem, kn);
> 
>         return ret;
>  }
> diff --git a/fs/kernfs/kernfs-internal.h b/fs/kernfs/kernfs-internal.h
> index fc5821effd97..bd1ecd126395 100644
> --- a/fs/kernfs/kernfs-internal.h
> +++ b/fs/kernfs/kernfs-internal.h
> @@ -169,4 +169,53 @@ extern const struct inode_operations kernfs_symlink_iops;
>   * kernfs locks
>   */
>  extern struct kernfs_global_locks *kernfs_locks;
> +
> +static inline struct rw_semaphore *kernfs_iattr_rwsem_ptr(struct kernfs_node *kn)
> +{
> +       int idx = hash_ptr(kn, NR_KERNFS_LOCK_BITS);
> +
> +       return &kernfs_locks->iattr_rwsem[idx];
> +}
> +
> +static inline struct rw_semaphore *kernfs_iattr_down_write(struct kernfs_node *kn)
> +{
> +       struct rw_semaphore *rwsem;
> +
> +       kernfs_get(kn);
> +
> +       rwsem = kernfs_iattr_rwsem_ptr(kn);
> +
> +       down_write(rwsem);
> +
> +       return rwsem;
> +}
> +
> +static inline void kernfs_iattr_up_write(struct rw_semaphore *rwsem,
> +                                        struct kernfs_node *kn)
> +{
> +       up_write(rwsem);
> +
> +       kernfs_put(kn);
> +}
> +
> +
> +static inline struct rw_semaphore *kernfs_iattr_down_read(struct kernfs_node *kn)
> +{
> +       struct rw_semaphore *rwsem;
> +
> +       kernfs_get(kn);
> +
> +       rwsem = kernfs_iattr_rwsem_ptr(kn);
> +
> +       down_read(rwsem);
> +
> +       return rwsem;
> +}
> +
> +static inline void kernfs_iattr_up_read(struct rw_semaphore *rwsem,
> +                                       struct kernfs_node *kn)
> +{
> +       up_read(rwsem);
> +
> +       kernfs_put(kn);
> +}
>  #endif /* __KERNFS_INTERNAL_H */
> diff --git a/fs/kernfs/mount.c b/fs/kernfs/mount.c
> index d0859f72d2d6..f282e5d65d04 100644
> --- a/fs/kernfs/mount.c
> +++ b/fs/kernfs/mount.c
> @@ -392,8 +392,10 @@ static void __init kernfs_mutex_init(void)
>  {
>         int count;
> 
> -       for (count = 0; count < NR_KERNFS_LOCKS; count++)
> +       for (count = 0; count < NR_KERNFS_LOCKS; count++) {
>                 mutex_init(&kernfs_locks->open_file_mutex[count]);
> +               init_rwsem(&kernfs_locks->iattr_rwsem[count]);
> +       }
>  }
> 
>  static void __init kernfs_lock_init(void)
> diff --git a/include/linux/kernfs.h b/include/linux/kernfs.h
> index 73f5c120def8..fcbf5e7c921c 100644
> --- a/include/linux/kernfs.h
> +++ b/include/linux/kernfs.h
> @@ -89,6 +89,7 @@ struct kernfs_iattrs;
>   */
>  struct kernfs_global_locks {
>         struct mutex open_file_mutex[NR_KERNFS_LOCKS];
> +       struct rw_semaphore iattr_rwsem[NR_KERNFS_LOCKS];
>  };
> 
>  enum kernfs_node_type {
> 
> 
> gives further improvement in CPU usage:
> 
> -    8.26%     8.22%  showgids         [kernel.kallsyms]       [k] down_read
>              8.19% __libc_start_main
>                 __open_nocancel
>                 entry_SYSCALL_64_after_hwframe
>                 do_syscall_64
>                 sys_open
>                 do_sys_open
>                 do_filp_open
>               - path_openat
>                  - 7.59% link_path_walk
>                     - 6.66% walk_component
>                          lookup_fast
>                        - d_revalidate.part.24
>                           - 6.66% kernfs_dop_revalidate
>                                down_read
>                     - 0.71% kernfs_iop_get_link
>                          down_read
>                  - 0.58% lookup_fast
>                       d_revalidate.part.24
>                       kernfs_dop_revalidate
>                       down_read
>         -    7.44%     7.41%  showgids         [kernel.kallsyms]       [k] up_read
>              7.39% __libc_start_main
>                 __open_nocancel
>                 entry_SYSCALL_64_after_hwframe
>                 do_syscall_64
>                 sys_open
>                 do_sys_open
>                 do_filp_open
>               - path_openat
>                  - 7.36% link_path_walk
>                     - 6.45% walk_component
>                          lookup_fast
>                          d_revalidate.part.24
>                          kernfs_dop_revalidate
>                          up_read
> 
> In above snippet CPU usage in down_read/up_read has gone down to ~16%
> 
> So do you think that rather than replacing global kernfs_rwsem with a hashed one
> , any of the above mentioned 2 patches (1. Use a global rwsem for protecting
> inode attributes or 2. Use a hashed rwsem for protecting inode attributes)
> can be used. These patches are not breaking code paths involving multiple nodes
> that currently use global kernfs_rwsem.
> With hashed kernfs_rwsem I guess there will always be a risk of some corner case
> getting missed.
> 
> If any of these approaches are acceptable, I can send the patch for review along
> with other changes of this series
> 
Could you please share your feedback about the above mentioned 2 approaches to
reduce contention around global kernfs_rwsem ? Also the first 2 patches of this
series [1] are not dealing specifically with with kernfs_rwsem, so could you
please share your feedback about those 2 as well.

Thanks,
 -- Imran

[1]: https://lore.kernel.org/lkml/20220810111017.2267160-1-imran.f.khan@oracle.com/

Powered by blists - more mailing lists