Skip to content
This repository was archived by the owner on Feb 16, 2026. It is now read-only.

Scale private oh #4364

Merged
nigoroll merged 2 commits into from
Jul 21, 2025
+47 −14
Merged

Scale private oh #4364

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
be83e3d
cache_hash: split objhead alloc / init
nigoroll Jul 11, 2025
445a843
cache_hash: scale the private_oh
nigoroll Jul 11, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 47 additions & 14 deletions bin/varnishd/cache/cache_hash.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,8 @@ struct rush {
};

static const struct hash_slinger *hash;
static struct objhead *private_oh;
#define PRIVATE_OH_EXP 7
static struct objhead private_ohs[1 << PRIVATE_OH_EXP];
Comment on lines +76 to +77
Copy link
Copy Markdown
Member

@dridi dridi Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not define a constant?

#define PRIVATE_OH_EXP 7
#define PRIVATE_OH_CNT (1 << PRIVATE_OH_EXP)
static struct objhead private_ohs[PRIVATE_OH_CNT];

I understand the purpose of vcountof() but if it confuses our tooling, it's maybe not worth using.

Copy link
Copy Markdown
Member Author

@nigoroll nigoroll Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does it confuse our tooling?

Copy link
Copy Markdown
Member

@dridi dridi Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This part of the pull request description, before you edited it to strike the second sentence out:

The drawback of the chosen solution is that flexelint thinks all objheads would need to be from the array, and hence reports out of bounds access all over the place. Suppressing these for the whole source file is far from ideal, but I have not found a better option (yet).

Copy link
Copy Markdown
Member Author

@nigoroll nigoroll Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The relevant part is the other bit of the description:

The choice of an array of objheads rather than an array of objhead pointers is motivated by hsh_deref_objhead_unlock(), which behaves differently for private objects.

This has nothing to do with vcountof(), what misleads flexelint is the fact that, with the static array for private objheads, the "others" are on the heap with arbitrary addresses. The reasoning of the tool is here "hey, if there is a check for an upper bound, but then an else branch, that else branch will probably be out of bounds".

I think the improved more selective silencing of flexelint is acceptable now.


static void hsh_rush1(const struct worker *, struct objhead *,
struct rush *, int);
Expand All @@ -93,17 +94,23 @@ VCF_RETURNS()

/*---------------------------------------------------------------------*/

static struct objhead *
hsh_newobjhead(void)
static void
hsh_initobjhead(struct objhead *oh)
{
struct objhead *oh;

ALLOC_OBJ(oh, OBJHEAD_MAGIC);
XXXAN(oh);
INIT_OBJ(oh, OBJHEAD_MAGIC);
oh->refcnt = 1;
VTAILQ_INIT(&oh->objcs);
VTAILQ_INIT(&oh->waitinglist);
Lck_New(&oh->mtx, lck_objhdr);
}

static struct objhead *
hsh_newobjhead(void)
{
struct objhead *oh = malloc(sizeof *oh);
hsh_initobjhead(oh);
return (oh);
}

Expand Down Expand Up @@ -131,22 +138,37 @@ hsh_prealloc(struct worker *wrk)

/*---------------------------------------------------------------------*/

// https://probablydance.com/2018/06/16/fibonacci-hashing-the-optimization-that-the-world-forgot-or-a-better-alternative-to-integer-modulo/
static inline size_t
fib(uint64_t n, uint8_t bits)
{
const uint64_t gr = 11400714819323198485LLU;
uint64_t r;

r = n * gr;
r >>= (sizeof(gr) * 8) - bits;
assert(r < (size_t)1 << bits);
return ((size_t)r);
}
Comment on lines +141 to +152
Copy link
Copy Markdown
Member

@dridi dridi Jul 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not take the first byte of the request hash? No modulo involved, possibly one & PRIVATE_OH_MASK if you want less than 8 bits worth of private objheads.

Copy link
Copy Markdown
Member Author

@nigoroll nigoroll Jul 11, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main driver is vcl_synth {}. As requests to the same resource have the same hash, using it would defeat the purpose of the patch and not solve the lock contention, just move it to a lock "per host+url" (actually hash, and actually "set of...").


struct objcore *
HSH_Private(const struct worker *wrk)
{
struct objcore *oc;
struct objhead *oh;

CHECK_OBJ_NOTNULL(private_oh, OBJHEAD_MAGIC);
oh = &private_ohs[fib((uintptr_t)wrk, PRIVATE_OH_EXP)];
CHECK_OBJ_NOTNULL(oh, OBJHEAD_MAGIC);

oc = ObjNew(wrk);
AN(oc);
oc->refcnt = 1;
oc->objhead = private_oh;
oc->objhead = oh;
oc->flags |= OC_F_PRIVATE;
Lck_Lock(&private_oh->mtx);
VTAILQ_INSERT_TAIL(&private_oh->objcs, oc, hsh_list);
private_oh->refcnt++;
Lck_Unlock(&private_oh->mtx);
Lck_Lock(&oh->mtx);
VTAILQ_INSERT_TAIL(&oh->objcs, oc, hsh_list);
oh->refcnt++;
Lck_Unlock(&oh->mtx);
return (oc);
}

Expand Down Expand Up @@ -1108,14 +1130,21 @@ hsh_deref_objhead_unlock(struct worker *wrk, struct objhead **poh, int max)

Lck_AssertHeld(&oh->mtx);

if (oh == private_oh) {
if (oh >= private_ohs && oh < private_ohs + vcountof(private_ohs)) {
assert(VTAILQ_EMPTY(&oh->waitinglist));
assert(oh->refcnt > 1);
oh->refcnt--;
Lck_Unlock(&oh->mtx);
return (1);
}

//lint --e{661}
//lint -specific(-e661)
//
// because of the static array, flexelint thinks that all ohs were from
// the static array :( the above suppression applies to the remainder of
// this function body and specific walks involving this function

INIT_OBJ(&rush, RUSH_MAGIC);
if (!VTAILQ_EMPTY(&oh->waitinglist)) {
assert(oh->refcnt > 1);
Expand Down Expand Up @@ -1151,6 +1180,10 @@ HSH_Init(const struct hash_slinger *slinger)
hash = slinger;
if (hash->start != NULL)
hash->start();
private_oh = hsh_newobjhead();
private_oh->refcnt = 1;
for (struct objhead *oh = private_ohs;
oh < private_ohs + vcountof(private_ohs);
oh++) {
hsh_initobjhead(oh);
assert(oh->refcnt == 1);
}
}