Other than what Matt brought up, it would be nice to have a way to
override the default values without recompiling. But we can add that to
the Linux port as needed. I don't really know how you guys support that
sort of thing.
Also, it _might_ be useful to move statements like:
Using the default settings these values translate to ~1 MB of hash
tables for each 1 GB of physical memory.
closer to the variable declarations just to try and ensure the comment
is updated if the default values are ever changed. But maybe I'm being
overly pedantic.
Do you have any before/after values for the max chain length from your
testing? It should be easy(?) to grab it from the arc stats and would be
nice to have numbers to justify the change.
Looks good, overall. Thanks.
--
Cheers, Prakash
On Wed, Oct 16, 2013 at 05:50:38PM -0700, Matthew Ahrens wrote:
> I like these changes -- fix the constant, make it tunable, keep it simple.
>
> 86-90: I think for a big theory statement we should document what the
> parameters are for the hash table size, rather than the exact algorithm for
> achieving it. More of the "why" and less of the "how".
> You might mention:
> - number of buckets is at least enough to have 1 bucket for every 8k of
> physical memory
> - number of buckets is a power of 2
> - at least 4096 buckets
> - results in about 1MB of hash tables for every 1GB of memory.
>
> 233: zfs_arc_ht_base_masklen - I don't understand the name of this variable.
> I
> would recommend:
>
> /*
> * This variable sets the assumed average block size. The ARC hash table will
> be sized large enough to
> * not have too much chaining even if all of memory used to cache blocks of
> this size.
> */
> uint64_t zfs_arc_average_block_size = 8 * 1024;
>
> (buf_init() will then need to do a multiply rather than <<(ht_maxlen + this).)
>
> 632: declare each member on its own line.
>
> 748: does declaring it inline improve performance? I suspect that it does, at
> least on gcc with illumos. But in the past "inline" has been
> ignored
> given the compiler flags used, so it's worth checking.
>
> 983: doesn't ht_num_locks need to be a power of 2? physmem is not necessarily
> a power of 2.
>
> If you want to add a tunable like zfs_arc_average_block_size for the
> dbuf_hash_table, that would be cool too (to replace the constant 4096 in
> dbuf_init).
>
> --matt
>
>
>
> On Wed, Oct 16, 2013 at 4:09 PM, Saso Kiselkov <[email protected]> wrote:
> Okay, I've reworked and simplified the ARC buf hash changes to just
> change the way we size the hash table and bump up the amount of hash
> tables we allocate to 1MB/1GB.
> Webrev: http://cr.illumos.org/~webrev/skiselkov/new_buf_hash/
> Issue: https://www.illumos.org/issues/4218
>
> Cheers,
> --
> Saso
>
>
> -------------------------------------------
> illumos-zfs
> Archives: https://www.listbox.com/member/archive/182191/=now
> RSS Feed: https://www.listbox.com/member/archive/rss/182191/21635000-
> ebd1d460
> Modify Your Subscription: https://www.listbox.com/member/?&
> Powered by Listbox: http://www.listbox.com
>
> illumos-zfs | Archives [http://postlink.www.listbox.com/1594930/
>
> [http://postlink.www.listbox.com/1594931/3379085af0f1cf7fc3708f04b4471ae2/23963346/
> 833487e62783d55fe81f119fb93ef644/23963346/
>
> 8cdfacfb.png?uri=aHR0cHM6Ly93d3cubGlzdGJveC5jb20vaW1hZ2VzL2xpc3Rib3gtbG9nby1zbWFsbC5wbmc]
> 8cdfacfb.jpg?uri=aHR0cHM6Ly93d3cubGlzdGJveC5jb20vaW1hZ2VzL2ZlZWQtaWNvbi0xMHgxMC5qcGc]
> | Modify Your Subscription
_______________________________________________
developer mailing list
[email protected]
http://lists.open-zfs.org/mailman/listinfo/developer
