Changes that didn't make it into the drm/ttm: Get rid of a number of atomic read-modify-write ops v3 patch series.
While hashtab should now be RCU-safe, Add a drm_ht_xxx_api for consumers to use to make it obvious what locking mechanism is used.
Document the way the rcu-safe interface should be used.
Don't use rcu-safe list traversal in modify operations where we should use a spinlock / mutex anyway.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- drivers/gpu/drm/drm_hashtab.c | 26 ++++++++++++++++++++++---- include/drm/drm_hashtab.h | 14 ++++++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-)
diff --git a/drivers/gpu/drm/drm_hashtab.c b/drivers/gpu/drm/drm_hashtab.c index 5729e39..8025454 100644 --- a/drivers/gpu/drm/drm_hashtab.c +++ b/drivers/gpu/drm/drm_hashtab.c @@ -67,7 +67,7 @@ void drm_ht_verbose_list(struct drm_open_hash *ht, unsigned long key) hashed_key = hash_long(key, ht->order); DRM_DEBUG("Key is 0x%08lx, Hashed key is 0x%08x\n", key, hashed_key); h_list = &ht->table[hashed_key]; - hlist_for_each_entry_rcu(entry, list, h_list, head) + hlist_for_each_entry(entry, list, h_list, head) DRM_DEBUG("count %d, key: 0x%08lx\n", count++, entry->key); }
@@ -81,7 +81,7 @@ static struct hlist_node *drm_ht_find_key(struct drm_open_hash *ht,
hashed_key = hash_long(key, ht->order); h_list = &ht->table[hashed_key]; - hlist_for_each_entry_rcu(entry, list, h_list, head) { + hlist_for_each_entry(entry, list, h_list, head) { if (entry->key == key) return list; if (entry->key > key) @@ -90,6 +90,24 @@ static struct hlist_node *drm_ht_find_key(struct drm_open_hash *ht, return NULL; }
+static struct hlist_node *drm_ht_find_key_rcu(struct drm_open_hash *ht, + unsigned long key) +{ + struct drm_hash_item *entry; + struct hlist_head *h_list; + struct hlist_node *list; + unsigned int hashed_key; + + hashed_key = hash_long(key, ht->order); + h_list = &ht->table[hashed_key]; + hlist_for_each_entry_rcu(entry, list, h_list, head) { + if (entry->key == key) + return list; + if (entry->key > key) + break; + } + return NULL; +}
int drm_ht_insert_item(struct drm_open_hash *ht, struct drm_hash_item *item) { @@ -102,7 +120,7 @@ int drm_ht_insert_item(struct drm_open_hash *ht, struct drm_hash_item *item) hashed_key = hash_long(key, ht->order); h_list = &ht->table[hashed_key]; parent = NULL; - hlist_for_each_entry_rcu(entry, list, h_list, head) { + hlist_for_each_entry(entry, list, h_list, head) { if (entry->key == key) return -EINVAL; if (entry->key > key) @@ -152,7 +170,7 @@ int drm_ht_find_item(struct drm_open_hash *ht, unsigned long key, { struct hlist_node *list;
- list = drm_ht_find_key(ht, key); + list = drm_ht_find_key_rcu(ht, key); if (!list) return -EINVAL;
diff --git a/include/drm/drm_hashtab.h b/include/drm/drm_hashtab.h index 3650d5d..fce2ef3 100644 --- a/include/drm/drm_hashtab.h +++ b/include/drm/drm_hashtab.h @@ -61,5 +61,19 @@ extern int drm_ht_remove_key(struct drm_open_hash *ht, unsigned long key); extern int drm_ht_remove_item(struct drm_open_hash *ht, struct drm_hash_item *item); extern void drm_ht_remove(struct drm_open_hash *ht);
+/* + * RCU-safe interface + * + * The user of this API needs to make sure that two or more instances of the + * hash table manipulation functions are never run simultaneously. + * The lookup function drm_ht_find_item_rcu may, however, run simultaneously + * with any of the manipulation functions as long as it's called from within + * an RCU read-locked section. + */ +#define drm_ht_insert_item_rcu drm_ht_insert_item +#define drm_ht_just_insert_please_rcu drm_ht_just_insert_please +#define drm_ht_remove_key_rcu drm_ht_remove_key +#define drm_ht_remove_item_rcu drm_ht_remove_item +#define drm_ht_find_item_rcu drm_ht_find_item
#endif
Document how kref_get_unless_zero should be used and how it helps solve a typical kref / locking problem.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- Documentation/kref.txt | 88 ++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 88 insertions(+), 0 deletions(-)
diff --git a/Documentation/kref.txt b/Documentation/kref.txt index 48ba715..ddf85a5 100644 --- a/Documentation/kref.txt +++ b/Documentation/kref.txt @@ -213,3 +213,91 @@ presentation on krefs, which can be found at: and: http://www.kroah.com/linux/talks/ols_2004_kref_talk/
+ +The above example could also be optimized using kref_get_unless_zero() in +the following way: + +static struct my_data *get_entry() +{ + struct my_data *entry = NULL; + mutex_lock(&mutex); + if (!list_empty(&q)) { + entry = container_of(q.next, struct my_data, link); + if (!kref_get_unless_zero(&entry->refcount)) + entry = NULL; + } + mutex_unlock(&mutex); + return entry; +} + +static void release_entry(struct kref *ref) +{ + struct my_data *entry = container_of(ref, struct my_data, refcount); + + mutex_lock(&mutex); + list_del(&entry->link); + mutex_unlock(&mutex); + kfree(entry); +} + +static void put_entry(struct my_data *entry) +{ + kref_put(&entry->refcount, release_entry); +} + +Which is useful to remove the mutex lock around kref_put() in put_entry(), but +it's important that kref_get_unless_zero is enclosed in the same critical +section that finds the entry in the lookup table, +otherwise kref_get_unless_zero may reference already freed memory. +Note that it is illegal to use kref_get_unless_zero without checking its +return value. If you are sure (by already having a valid pointer) that +kref_get_unless_zero() will return true, then use kref_get() instead. + +The function kref_get_unless_zero also makes it possible to use rcu +locking for lookups in the above example: + +struct my_data +{ + struct rcu_head rhead; + . + struct kref refcount; + . + . +}; + +static struct my_data *get_entry_rcu() +{ + struct my_data *entry = NULL; + rcu_read_lock(); + if (!list_empty(&q)) { + entry = container_of(q.next, struct my_data, link); + if (!kref_get_unless_zero(&entry->refcount)) + entry = NULL; + } + rcu_read_unlock(); + return entry; +} + +static void release_entry_rcu(struct kref *ref) +{ + struct my_data *entry = container_of(ref, struct my_data, refcount); + + mutex_lock(&mutex); + list_del_rcu(&entry->link); + mutex_unlock(&mutex); + kfree_rcu(entry, rhead); +} + +static void put_entry(struct my_data *entry) +{ + kref_put(&entry->refcount, release_entry_rcu); +} + +But note that the struct kref member needs to remain in valid memory for a +rcu grace period after release_entry_rcu was called. That can be accomplished +by using kfree_rcu(entry, rhead) as done above, or by calling synchronize_rcu() +before using kfree, but note that synchronize_rcu() may sleep for a +substantial amount of time. + + +Thomas Hellstrom thellstrom@vmware.com
They need to be freed after an rcu grace period.
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- drivers/gpu/drm/vmwgfx/vmwgfx_fence.c | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c index bc187fa..c62d20e 100644 --- a/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c +++ b/drivers/gpu/drm/vmwgfx/vmwgfx_fence.c @@ -537,7 +537,7 @@ static void vmw_user_fence_destroy(struct vmw_fence_obj *fence) container_of(fence, struct vmw_user_fence, fence); struct vmw_fence_manager *fman = fence->fman;
- kfree(ufence); + ttm_base_object_kfree(ufence, base); /* * Free kernel space accounting. */
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- drivers/gpu/drm/ttm/ttm_object.c | 2 ++ 1 files changed, 2 insertions(+), 0 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_object.c b/drivers/gpu/drm/ttm/ttm_object.c index f18eeb4..0e13d9f 100644 --- a/drivers/gpu/drm/ttm/ttm_object.c +++ b/drivers/gpu/drm/ttm/ttm_object.c @@ -174,7 +174,9 @@ int ttm_base_object_init(struct ttm_object_file *tfile,
return 0; out_err1: + spin_lock(&tdev->object_lock); (void)drm_ht_remove_item(&tdev->object_hash, &base->hash); + spin_unlock(&tdev->object_lock); out_err0: return ret; }
Also move a kref_init() out of spinlocked region
Signed-off-by: Thomas Hellstrom thellstrom@vmware.com --- drivers/gpu/drm/ttm/ttm_object.c | 21 ++++++++++++++------- 1 files changed, 14 insertions(+), 7 deletions(-)
diff --git a/drivers/gpu/drm/ttm/ttm_object.c b/drivers/gpu/drm/ttm/ttm_object.c index 0e13d9f..58a5f32 100644 --- a/drivers/gpu/drm/ttm/ttm_object.c +++ b/drivers/gpu/drm/ttm/ttm_object.c @@ -157,11 +157,11 @@ int ttm_base_object_init(struct ttm_object_file *tfile, base->refcount_release = refcount_release; base->ref_obj_release = ref_obj_release; base->object_type = object_type; - spin_lock(&tdev->object_lock); kref_init(&base->refcount); - ret = drm_ht_just_insert_please(&tdev->object_hash, - &base->hash, - (unsigned long)base, 31, 0, 0); + spin_lock(&tdev->object_lock); + ret = drm_ht_just_insert_please_rcu(&tdev->object_hash, + &base->hash, + (unsigned long)base, 31, 0, 0); spin_unlock(&tdev->object_lock); if (unlikely(ret != 0)) goto out_err0; @@ -175,7 +175,7 @@ int ttm_base_object_init(struct ttm_object_file *tfile, return 0; out_err1: spin_lock(&tdev->object_lock); - (void)drm_ht_remove_item(&tdev->object_hash, &base->hash); + (void)drm_ht_remove_item_rcu(&tdev->object_hash, &base->hash); spin_unlock(&tdev->object_lock); out_err0: return ret; @@ -189,8 +189,15 @@ static void ttm_release_base(struct kref *kref) struct ttm_object_device *tdev = base->tfile->tdev;
spin_lock(&tdev->object_lock); - (void)drm_ht_remove_item(&tdev->object_hash, &base->hash); + (void)drm_ht_remove_item_rcu(&tdev->object_hash, &base->hash); spin_unlock(&tdev->object_lock); + + /* + * Note: We don't use synchronize_rcu() here because it's far + * too slow. It's up to the user to free the object using + * call_rcu() or ttm_base_object_kfree(). + */ + if (base->refcount_release) { ttm_object_file_unref(&base->tfile); base->refcount_release(&base); @@ -216,7 +223,7 @@ struct ttm_base_object *ttm_base_object_lookup(struct ttm_object_file *tfile, int ret;
rcu_read_lock(); - ret = drm_ht_find_item(&tdev->object_hash, key, &hash); + ret = drm_ht_find_item_rcu(&tdev->object_hash, key, &hash);
if (likely(ret == 0)) { base = drm_hash_entry(hash, struct ttm_base_object, hash);
dri-devel@lists.freedesktop.org
-
Thomas Hellstrom