On 03/10/2019 22:01, Chris Wilson wrote:
A few callers need to serialise the destruction of their drm_mm_node and ensure it is removed from the drm_mm before freeing. However, to be completely sure that any access from another thread is complete before we free the struct, we require the RELEASE semantics of clear_bit_unlock().
This allows the conditional locking such as
Thread A Thread B mutex_lock(mm_lock); if (drm_mm_node_allocated(node)) { drm_mm_node_remove(node); mutex_lock(mm_lock); mutex_unlock(mm_lock); drm_mm_node_remove(node); mutex_unlock(mm_lock); } kfree(node);
My understanding is that release semantics on node allocated mean 1 -> 0 transition is guaranteed to be seen only when thread A drm_mm_node_remove is fully done with the removal. But if it is in the middle of removal, node is still seen as allocated outside and thread B can enter the if-body, wait for the lock, and then drm_mm_node_remove will attempt a double removal. So I think another drm_mm_node_allocated under the lock is needed.
But the fkree part looks correct. There can be no false negatives with the release semantics on the allocated bit.
Regards,
Tvrtko
to serialise correctly without any lingering accesses from A to the freed node. Allocation / insertion of the node is assumed never to race with removal or eviction scanning.
Signed-off-by: Chris Wilson chris@chris-wilson.co.uk
drivers/gpu/drm/drm_mm.c | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/drm_mm.c b/drivers/gpu/drm/drm_mm.c index a9cab5e53731..2a6e34663146 100644 --- a/drivers/gpu/drm/drm_mm.c +++ b/drivers/gpu/drm/drm_mm.c @@ -424,9 +424,9 @@ int drm_mm_reserve_node(struct drm_mm *mm, struct drm_mm_node *node)
node->mm = mm;
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); list_add(&node->node_list, &hole->node_list); drm_mm_interval_tree_add_node(hole, node);
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); node->hole_size = 0;
rm_hole(hole);
@@ -543,9 +543,9 @@ int drm_mm_insert_node_in_range(struct drm_mm * const mm, node->color = color; node->hole_size = 0;
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
__set_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);rm_hole(hole); if (adj_start > hole_start)
@@ -589,11 +589,12 @@ void drm_mm_remove_node(struct drm_mm_node *node)
drm_mm_interval_tree_remove(node, &mm->interval_tree); list_del(&node->node_list);
__clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &node->flags);
if (drm_mm_hole_follows(prev_node)) rm_hole(prev_node); add_hole(prev_node);
- clear_bit_unlock(DRM_MM_NODE_ALLOCATED_BIT, &node->flags); } EXPORT_SYMBOL(drm_mm_remove_node);
@@ -614,6 +615,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new)
*new = *old;
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags); list_replace(&old->node_list, &new->node_list); rb_replace_node_cached(&old->rb, &new->rb, &mm->interval_tree);
@@ -627,8 +629,7 @@ void drm_mm_replace_node(struct drm_mm_node *old, struct drm_mm_node *new) &mm->holes_addr); }
- __clear_bit(DRM_MM_NODE_ALLOCATED_BIT, &old->flags);
- __set_bit(DRM_MM_NODE_ALLOCATED_BIT, &new->flags);
- clear_bit_unlock(DRM_MM_NODE_ALLOCATED_BIT, &old->flags); } EXPORT_SYMBOL(drm_mm_replace_node);