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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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
>>