Page MenuHomeFreeBSD

Add unlocked/SMR fast path to getblk()
ClosedPublic

Authored by cem on Jul 22 2020, 11:29 PM.

Details

Summary

Convert the bufobj tries to an SMR zone/PCTRIE and add a gbincore_unlocked()
API wrapping this functionality. Use it for a fast path in getblkx(),
falling back to locked lookup if we raced a thread changing the buf's
identity.

Test Plan

Quite surprisingly to me, it doesn't seem to immediately panic. So I'm probably missing something. ;-)

In our getblk-heavy workloads (which have other scaling limitations beyond those in FreeBSD), on 2x48 CPU (2P) metal, we see significantly better scaling with this change at 6+ threads (4.3x single-thread vs 3.7x single-thread), and the gap gets wider at higher thread counts. I don't have any great benchmarks on vanilla FreeBSD.

Diff Detail

Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 32529
Build 30001: arc lint + arc unit

Event Timeline

cem requested review of this revision.Jul 22 2020, 11:29 PM
cem created this revision.
cem added reviewers: alc, dougm, markj, rlibby, kib, jeff.

Quite surprisingly to me, it doesn't seem to immediately panic. So I'm probably missing something.

Perhaps @pho would be willing to try and test this together with D25781?

Quite surprisingly to me, it doesn't seem to immediately panic. So I'm probably missing something.

Perhaps @pho would be willing to try and test this together with D25781?

I'd certainly appreciate it.

In D25782#571014, @cem wrote:

Quite surprisingly to me, it doesn't seem to immediately panic. So I'm probably missing something.

Perhaps @pho would be willing to try and test this together with D25781?

I'd certainly appreciate it.

I'm building a kernel right now.

sys/kern/vfs_bio.c
3866

The blocking there is not needed.

3878

I think the rule should be that any failure on lockless path for buffer lookup results in locked retry. Imagine that buffer was reclaimed meantime, then you would not reach the check for bufobj/blkno at all.

3883

Why do you think it should be handled there ?

sys/kern/vfs_subr.c
2369

return ()

Thanks!

sys/kern/vfs_bio.c
3866

will fix

3878

We can abort without retry for EINTR/ERESTART, right? Retrying in all other cases makes sense to me.

3883
#define BUF_UNLOCK(bp) do {                                             \
        KASSERT(((bp)->b_flags & B_REMFREE) == 0,                       \
            ("BUF_UNLOCK %p while B_REMFREE is still set.", (bp)));     \
                                                                        \
        (void)_lockmgr_args(&(bp)->b_lock, LK_RELEASE, NULL,            \
            LK_WMESG_DEFAULT, LK_PRIO_DEFAULT, LK_TIMO_DEFAULT,         \
            LOCK_FILE, LOCK_LINE);                                      \
sys/kern/vfs_subr.c
2369

will fix

cem marked 2 inline comments as done.
  • Fix style issues
  • Fallback to locked getblk path on all BUF_TIMELOCK errors except EINTR/ERESTART.
sys/kern/vfs_bio.c
3883

Huh. I think you should use lockmgr() directly there, to not touch buffer we do not intend to operate on, at all.

Add BUF_UNLOCK_NOASSERT macro for raw lockmgr unlock operation, and use it in unlocked identity-race path.

sys/kern/vfs_bio.c
3910

I must admit that 'unlocked' suffix is misleading.

sys/sys/buf.h
330

I would name it BUF_UNLOCK_RAW()

cem marked 2 inline comments as done.Jul 24 2020, 2:06 PM
cem added inline comments.
sys/kern/vfs_bio.c
3910

Will change.

sys/sys/buf.h
330

Will change

Rename new BUF_UNLOCK variant, goto label, for clarity.

This revision is now accepted and ready to land.Jul 24 2020, 2:24 PM
sys/kern/vfs_bio.c
3881

If you invert the condition, the control flow can be simplified a bit:

if (bp->b_bufobj == && bp->b_lblkno == blkno)
    goto foundbuf_fastpath;
/* It changed, fall back to locked lookup. */
BUF_UNLOCK_RAW(bp);
loop:
sys/kern/vfs_subr.c
495

Missing parens.

cem marked 2 inline comments as done.Jul 24 2020, 2:30 PM
cem added inline comments.
sys/kern/vfs_bio.c
3881

Sure, will do.

cem marked an inline comment as done.

Invert sense of identity check to simplify goto logic

This revision now requires review to proceed.Jul 24 2020, 2:32 PM
cem marked an inline comment as done.

Fix MarkJ's style comment I missed earlier.

This revision is now accepted and ready to land.Jul 24 2020, 2:49 PM
This revision was automatically updated to reflect the committed changes.