Re: [PATCH 0/6] kernfs: proposed locking and concurrency improvement

On Wed, 2020-12-23 at 15:30 +0800, Fox Chen wrote:

Hi Ian,

On Tue, Dec 22, 2020 at 3:47 PM Ian Kent [off-list ref] wrote:

quoted

Here is what I currently have for the patch series we were
discussing
recently.

I'm interested to see how this goes with the problem you are
seeing.

The last patch in the series (the attributes update patch) has seen
no more than compile testing, I hope I haven't messed up on that.

OK, I just ignored the last patch (kernfs: add a spinlock to kernfs
iattrs for inode updates)
because I can not boot the kernel after applying it.

Right, clearly I haven't got that last patch right yet.

It looks like the problem there is that the iattrs structure might
not be allocated at the time so clearly I can't just use the spin
lock inside.

It would be simple enough to resolve except for the need to set
the inode link count too.

Ian

Applying the first five patches, my benchmark shows no improvement
this time,
one open+read+close cycle spends 500us. :(

perf suggests that down_write in kernfs_iop_permission drags the most
performance
(full report see attachment) which makes sense to me, since
down_write
has no difference
here compared to the mutex, they all allow only one thread running
simultaneously.

sigh... It's really hard to fix.

   |
   |--51.81%--inode_permission
   |          |
   |           --51.36%--kernfs_iop_permission
   |                     |
   |                     |--50.41%--down_write
   |                     |          |
   |                     |           --50.32%
--rwsem_down_write_slowpath
   |                     |                     |
   |                     |                      --49.67%
--rwsem_optimistic_spin
   |                     |                                |
   |                     |                                 --49.31%
--osq_lock
   |                     |
   |                      --0.74%--up_write
   |                                rwsem_wake.isra.0
   |                                |
   |                                 --0.69%--wake_up_q
   |                                           |
   |                                            --0.62%
--try_to_wake_up
   |                                                      |
   |
--0.54%--_raw_spin_lock_irqsave
   |                                                                 
|
   |
--0.52%--native_queued
   |
    --29.90%--walk_component
              |
               --29.72%--lookup_fast
                         |
                          --29.51%--kernfs_dop_revalidate
                                    |
                                    |--28.88%--down_read
                                    |          |
                                    |
--28.75%--rwsem_down_read_slowpath
                                    |                     |
                                    |
--28.50%--rwsem_optimistic_spin
                                    |                                
|
                                    |
--28.38%--osq_lock
                                    |
                                     --0.59%--up_read
                                               rwsem_wake.isra.0
                                               |
                                                --0.54%--wake_up_q
                                                          |

--0.53%--try_to_wake_up

quoted

Please keep in mind that Greg's continued silence on the question
of whether the series might be re-considered indicates to me that
he has not changed his position on this.

Since the problem doesn't break the system, it doesn't need immediate
attention.
So I bet it must be labeled with a low priority in Greg's to-do list.
And it's merging
window & holiday season. Let's just give him more time. :)

quoted

---

Ian Kent (6):
      kernfs: move revalidate to be near lookup
      kernfs: use VFS negative dentry caching
      kernfs: use revision to identify directory node changes
      kernfs: switch kernfs to use an rwsem
      kernfs: stay in rcu-walk mode if possible
      kernfs: add a spinlock to kernfs iattrs for inode updates


 fs/kernfs/dir.c             |  285 ++++++++++++++++++++++++++++---
------------
 fs/kernfs/file.c            |    4 -
 fs/kernfs/inode.c           |   19 ++-
 fs/kernfs/kernfs-internal.h |   30 ++++-
 fs/kernfs/mount.c           |   12 +-
 fs/kernfs/symlink.c         |    4 -
 include/linux/kernfs.h      |    5 +
 7 files changed, 238 insertions(+), 121 deletions(-)

--
Ian

thanks,
fox