On 11/18/24 16:55, Mikulas Patocka wrote: > > > On Mon, 18 Nov 2024, Meir Elisha wrote: > >>> diff --git a/drivers/md/persistent-data/dm-transaction-manager.c b/drivers/md/persistent-data/dm-transaction-manager.c >>> index c7ba4e6cbbc7..8d486b1e6693 100644 >>> --- a/drivers/md/persistent-data/dm-transaction-manager.c >>> +++ b/drivers/md/persistent-data/dm-transaction-manager.c >>> @@ -10,6 +10,7 @@ >>> #include "dm-space-map-metadata.h" >>> #include "dm-persistent-data-internal.h" >>> >>> +#include <linux/moduleparam.h> >>> #include <linux/export.h> >>> #include <linux/mutex.h> >>> #include <linux/hash.h> >>> @@ -84,8 +85,35 @@ struct shadow_info { >>> /* >>> * It would be nice if we scaled with the size of transaction. >>> */ >>> -#define DM_HASH_SIZE 256 >>> -#define DM_HASH_MASK (DM_HASH_SIZE - 1) >>> +static uint tm_hash_table_size = 256; >>> + >>> +static int param_set_hash_size(const char *val, const struct kernel_param *kp) >>> +{ >>> + unsigned int num; >>> + int ret; >>> + >>> + if (!val) >>> + return -EINVAL; >>> + >>> + ret = kstrtouint(val, 0, &num); >>> + if (ret) >>> + return ret; >>> + >>> + /* Hash size must be a power of 2 */ >>> + if (!(num && !(num & (num - 1)))) >>> + return -EINVAL; >>> + >>> + *((unsigned int *)kp->arg) = num; >>> + return 0; >>> +} >>> + >>> +static const struct kernel_param_ops tm_hash_table_size_ops = { >>> + .set = param_set_hash_size, >>> + .get = param_get_uint >>> +}; >>> + >>> +module_param_cb(tm_hash_table_size, &tm_hash_table_size_ops, &tm_hash_table_size, 0644); >>> +MODULE_PARM_DESC(tm_hash_table_size, "transaction manager hash size"); >>> >>> struct dm_transaction_manager { >>> int is_clone; >>> @@ -95,8 +123,9 @@ struct dm_transaction_manager { >>> struct dm_space_map *sm; >>> >>> spinlock_t lock; >>> - struct hlist_head buckets[DM_HASH_SIZE]; >>> - >>> + struct hlist_head *buckets; >>> + uint hash_size; >>> + uint hash_mask; >>> struct prefetch_set prefetches; >>> }; >>> >>> @@ -105,7 +134,7 @@ struct dm_transaction_manager { >>> static int is_shadow(struct dm_transaction_manager *tm, dm_block_t b) >>> { >>> int r = 0; >>> - unsigned int bucket = dm_hash_block(b, DM_HASH_MASK); >>> + unsigned int bucket = dm_hash_block(b, tm->hash_mask); >>> struct shadow_info *si; >>> >>> spin_lock(&tm->lock); >>> @@ -131,7 +160,7 @@ static void insert_shadow(struct dm_transaction_manager *tm, dm_block_t b) >>> si = kmalloc(sizeof(*si), GFP_NOIO); >>> if (si) { >>> si->where = b; >>> - bucket = dm_hash_block(b, DM_HASH_MASK); >>> + bucket = dm_hash_block(b, tm->hash_mask); >>> spin_lock(&tm->lock); >>> hlist_add_head(&si->hlist, tm->buckets + bucket); >>> spin_unlock(&tm->lock); >>> @@ -146,7 +175,7 @@ static void wipe_shadow_table(struct dm_transaction_manager *tm) >>> int i; >>> >>> spin_lock(&tm->lock); >>> - for (i = 0; i < DM_HASH_SIZE; i++) { >>> + for (i = 0; i < tm->hash_size; i++) { >>> bucket = tm->buckets + i; >>> hlist_for_each_entry_safe(si, tmp, bucket, hlist) >>> kfree(si); >>> @@ -169,13 +198,19 @@ static struct dm_transaction_manager *dm_tm_create(struct dm_block_manager *bm, >>> if (!tm) >>> return ERR_PTR(-ENOMEM); >>> >>> + tm->hash_size = tm_hash_table_size; >>> + tm->buckets = kmalloc_array(tm->hash_size, sizeof(*tm->buckets), GFP_KERNEL); >>> + if (!tm->buckets) >>> + return ERR_PTR(-ENOMEM); >>> + >>> tm->is_clone = 0; >>> tm->real = NULL; >>> tm->bm = bm; >>> tm->sm = sm; >>> + tm->hash_mask = tm->hash_size - 1; >>> >>> spin_lock_init(&tm->lock); >>> - for (i = 0; i < DM_HASH_SIZE; i++) >>> + for (i = 0; i < tm->hash_size; i++) >>> INIT_HLIST_HEAD(tm->buckets + i); >>> >>> prefetch_init(&tm->prefetches); >> >> Hi all, >> >> I wanted to follow up on this patch. >> Could you please review it when you have a chance? >> If you need any additional information or have concerns, let me know. >> Thanks for your time. > > Hi > > I discussed it with Joe. We concluded that the patch is not much useful, > because few users will know about this option and few users will change it > if they hit bad performance. > > Joe will be reworking the code so that it will use rb-trees instead of > linked lists when there will be hash collision, this could improve > performance without adding a tunable variable. > > Mikulas > First of all, thanks for the response. I get that probably not too many users will use the module_param but it has a significant performance impact on our use case. Any general idea when the rb-trees solution should be submitted?