Re: [PATCH 1/7] dma-buf: fix inconsistent debug print

Am 02.06.21 um 14:33 schrieb Daniel Vetter:

On Wed, Jun 02, 2021 at 01:17:08PM +0200, Christian König wrote:

...

The code tries to acquire the rcu protected fence list, but then ignores individual fences which have been modified while holding the rcu.

Stop that madness and just note cleanly that the list was concurrently modified.

Signed-off-by: Christian König christian.koenig@amd.com

Yeah it's debugfs, it's better not to be fancy here and if you race you can just re-grab it all.

What's worse, we do grab the dma_resv_lock, which means no one should be able to race with us. I think 100% right thing here is actually to drop the rcu_read_lock too, and switch over to rcu_dereference_protected().

And also drop the seqcount check, that would be a bug. seqcount is only to get a consistent snapshot of all fences on the read (i.e. protected by rcu only) section. We hold the write lock with dma_resv_lock here.

Yes that what I had in mind as alternative as well.

Just wasn't 100% sure which way to go here.

Going to adjust that, Christian.

Cheers, Daniel

...

---

drivers/dma-buf/dma-buf.c | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-)

diff --git a/drivers/dma-buf/dma-buf.c b/drivers/dma-buf/dma-buf.c index eadd1eaa2fb5..d3b4e370dbc1 100644 --- a/drivers/dma-buf/dma-buf.c +++ b/drivers/dma-buf/dma-buf.c @@ -1383,22 +1383,17 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) buf_obj->name ?: "");

robj = buf_obj->resv;

• while (true) {
  

• 	seq = read_seqcount_begin(&robj->seq);
  

• 	rcu_read_lock();
  

• 	fobj = rcu_dereference(robj->fence);
  

• 	shared_count = fobj ? fobj->shared_count : 0;
  

• 	fence = rcu_dereference(robj->fence_excl);
  

• 	if (!read_seqcount_retry(&robj->seq, seq))
  

• 		break;
  

• 	rcu_read_unlock();
  

• }
  

• seq = read_seqcount_begin(&robj->seq);
  

• rcu_read_lock();
  

• fence = rcu_dereference(robj->fence_excl);
  

  if (fence) seq_printf(s, "\tExclusive fence: %s %s %ssignalled\n", fence->ops->get_driver_name(fence), fence->ops->get_timeline_name(fence), dma_fence_is_signaled(fence) ? "" : "un");

• fobj = rcu_dereference(robj->fence);
  

• shared_count = fobj ? fobj->shared_count : 0;
  

  for (i = 0; i < shared_count; i++) { fence = rcu_dereference(fobj->shared[i]); if (!dma_fence_get_rcu(fence))

@@ -1410,6 +1405,8 @@ static int dma_buf_debug_show(struct seq_file *s, void *unused) dma_fence_put(fence); } rcu_read_unlock();

• if (read_seqcount_retry(&robj->seq, seq))
  

• 	seq_printf(s, "\tFences concurrently modified\n");
  
  

  seq_puts(s, "\tAttached Devices:\n"); attach_count = 0;

-- 2.25.1