linux-kernel - Re: [PATCH kernfs 1/3] kernfs: switch global kernfs_idr

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: <20250414032054.72526-1-alexjlzheng@tencent.com>
Date: Mon, 14 Apr 2025 11:20:54 +0800
From: Jinliang Zheng <alexjlzheng@...il.com>
To: gregkh@...uxfoundation.org
Cc: alexjlzheng@...il.com,
	alexjlzheng@...cent.com,
	linux-kernel@...r.kernel.org,
	tj@...nel.org
Subject: Re: [PATCH kernfs 1/3] kernfs: switch global kernfs_idr_lock to per-fs lock

> On Sat, Apr 12, 2025 at 07:50:54PM +0800, alexjlzheng@...il.com wrote:
> > On Sat, 12 Apr 2025 08:12:22 +0200, gregkh@...uxfoundation.org wrote:
> > > On Sat, Apr 13, 2025 at 02:31:07AM +0800, alexjlzheng@...il.com wrote:
> > > > From: Jinliang Zheng <alexjlzheng@...cent.com>
> > > > 
> > > > The kernfs implementation has big lock granularity(kernfs_idr_lock) so
> > > > every kernfs-based(e.g., sysfs, cgroup) fs are able to compete the lock.
> > > > 
> > > > This patch switches the global kernfs_idr_lock to per-fs lock, which
> > > > put the spinlock into kernfs_root.
> > > > 
> > > > Signed-off-by: Jinliang Zheng <alexjlzheng@...cent.com>
> > > > ---
> > > >  fs/kernfs/dir.c             | 14 +++++++-------
> > > >  fs/kernfs/kernfs-internal.h |  1 +
> > > >  2 files changed, 8 insertions(+), 7 deletions(-)
> > > 
> > > What kind of testing / benchmark did you do for this series that shows
> > > that this works, AND that this actually is measureable?  What workload
> > > are you doing that causes these changes to be needed?
> > 
> > Thank you for your reply. :)
> > 
> > We are trying to implement a kernfs-based filesystem that will have
> > multiple instances running at the same time, i.e., multiple kernfs_roots.
> 
> I don't think that kernfs is meant for that very well, what is that
> filesystem going to be for?

Thank you for your reply. :)

Similar to cgroupfs and sysfs, it is used to export the status and configurations
of some kernel variables in hierarchical modes of the kernel. The only difference
is that it may have many instances, that is, many kernfs_roots.

> 
> > While investigating the kernfs implementation, we found some global locks
> > that would cause noticeable lock contention when there are many filesystem
> > instances.
> > 
> > Fortunately, we found that some optimizations have been made in [1], which
> > moved kernfs_rwsem into kernfs_root. But there are still some global locks
> > left.
> > 
> > We think it is also necessary to switch the remaining global locks to
> > per-fs. Moreover, we strongly agree with Tejun Heo's point in [1]:
> > 
> >   "... this is the right thing to do even if there is no concrete
> >    performance argument (not saying there isn't). It's just weird to
> >    entangle these completely unrelated users in a single rwsem."
> > 
> > We think kernfs will be widely used to build other filesystems, so we
> > strongly recommend switching global locks to per-fs.
> 
> I don't strongly object, but I would like to see some real-world numbers first.

Haha, we are still evaluating whether to implement it based on kernfs, so it
has not been implemented and tested yet. However, for these global locks, I
think it is more reasonable to switch to per-fs, regardless of whether there
is a significant performance improvement (in fact, when the number of kernfs-based
filesystem instances increases, the lock contention will definitely be reduced.
At least, it will not get worse, hahaha).

Haha, but if you think it is not necessary to do this, just ignore this patchset.

Thank you,
Jinliang Zheng. :)

> 
> thanks,
> 
> greg k-h