Page MenuHomeFreeBSD

NFSv4 server tunable hash table sizing
AcceptedPublic

Authored by rmacklem on May 19 2015, 11:28 PM.

Details

Reviewers
kib
Summary

This patch makes the sizes of hash tables used by the NFSv4 server tunable.
Although I do not see any measurable performance change when the table
sizes are increased, a server under heavy NFSv4 load from several clients
may need larger hash tables for good performance. One email report of
high CPU overheads for an NFSv4 server might have been caused by small
hash tables. By making them tunable, NFSv4 server sysadmins can test larger
hash tables.
The default sizing of the hash tables is not changed by this patch.

Test Plan

Tested on a small single client setup and found to be performance neutral
for that case.

Diff Detail

Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

rmacklem updated this revision to Diff 5504.May 19 2015, 11:28 PM
rmacklem retitled this revision from to NFSv4 server tunable hash table sizing.
rmacklem updated this object.
rmacklem edited the test plan for this revision. (Show Details)

Konstantin already provided this review via email. I've pasted
it here so he doesn't need to type it again:

I cannot see anything wrong with allocating the hash tables with malloc(9).
Even if the tables are relatively large, i.e. require large contig KVA for
allocation to succeed, it is not a problem during boot, when the kmem
is not fragmented.

One thing which I note in the patch is cosmetic issue with the lc_stateid[]
variadic array. You save one malloc call by the trick of the zero-length
last member, but this is IMO not needed anymore, lc_statid may become
the normal pointer and memory allocated by the malloc(9) after struct
nfsclient is malloced.

rmacklem updated this revision to Diff 5718.May 26 2015, 10:59 PM

Change to using 2 mallocs as Konstantin suggested in his
email review.

The second revision is applied to the first. I guess I should
have submitted an entire new diff. Oh well...

kib added inline comments.May 27 2015, 8:34 AM
fs/nfs/nfsrvstate.h
106

Wouldn't it be cleaner to declare lc_id as u_char *lc_id ? Also, why the type is u_char ?

fs/nfsserver/nfs_nfsdserv.c
3472

Why +1 ?

rmacklem added inline comments.May 27 2015, 12:06 PM
fs/nfs/nfsrvstate.h
106

Well, the structure is allocated large enough to
store the actual "id", which is an 8bit byte "string"
that the server handles opaquely. (ie. Just
bcmp()'s == or !=). It was made lc_id[1], so I
didn't need to "+ 1" for the null termination byte
when I allocated it. (This code was written long
ago. If the diff was against -head, this wouldn't
have been a change.)

If you think lc_id[0] is clearer, that's easy to change. All that is needed is a "+ 1" on the size
allocated via malloc().

This is normally small, so I don't think a 3rd
malloc() is justified, but I can do it that way.
(If it is defined as *lc_id, I think I need to do a
third malloc?)

As for u_char, it's just 8bit arbitrary bytes, so
that seemed more appropriate to char, but
I don't think anything in the code cares what
the type is, so long as it stores 8bits.

fs/nfsserver/nfs_nfsdserv.c
3472

It's "+ i", where "i" is the number of non-null
bytes stored in lc_id. If it is declared lc_id[0],
this needs to change to "+ i + 1".
(And the font seems to make it look like a "1"
for me too. I love web based stuff;-).)

kib accepted this revision.May 27 2015, 12:27 PM
kib edited edge metadata.
This revision is now accepted and ready to land.May 27 2015, 12:27 PM
julian added a subscriber: julian.Mar 16 2017, 5:11 AM

note that this was committed