Re: [PATCH] IB/hfi1,IB/qib: Fix qp_stats sleep with rcu read lock held

[Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>]

[-- Attachment #1.1: Type: text/plain, Size: 2098 bytes --]

On 8/10/2016 1:51 AM, ira.weiny wrote:
> On Tue, Aug 09, 2016 at 09:11:46PM +0300, Leon Romanovsky wrote:
>> On Tue, Aug 09, 2016 at 11:16:26AM -0400, ira.weiny-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org wrote:
>>> From: Mike Marciniszyn <mike.marciniszyn-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org>
>>>
> 
> [snip]
> 
>>> diff --git a/drivers/infiniband/hw/hfi1/debugfs.c b/drivers/infiniband/hw/hfi1/debugfs.c
>>> index dbab9d9cc288..c35bef8dd5aa 100644
>>> --- a/drivers/infiniband/hw/hfi1/debugfs.c
>>> +++ b/drivers/infiniband/hw/hfi1/debugfs.c
>>> @@ -223,28 +223,35 @@ DEBUGFS_SEQ_FILE_OPEN(ctx_stats)
>>>  DEBUGFS_FILE_OPS(ctx_stats);
>>>  
>>>  static void *_qp_stats_seq_start(struct seq_file *s, loff_t *pos)
>>> -__acquires(RCU)
>>> +	__acquires(RCU)
>>>  {
>>>  	struct qp_iter *iter;
>>>  	loff_t n = *pos;
>>>  
>>> -	rcu_read_lock();
>>>  	iter = qp_iter_init(s->private);
>>> +
>>> +	/* stop calls rcu_read_unlock */
>>> +	rcu_read_lock();
>>
>> IMHO, it should be placed after your if(!iter) check below.
> 
> I know this seems weird but this makes the rcu locking "balanced".  Returning
> NULL here still calls "stop" which is going to call rcu_read_unlock.  If we
> move rcu_read_lock we need to maintain state to determine if we called lock or
> not.  This is easier.  To me it does seem odd that the sequence operation does
> not stop on its own when NULL is returned but that is the way it works.
> 
>>
>>> +
>>>  	if (!iter)
>>>  		return NULL;
>>>  
>>> -	while (n--) {
>>> +	if (qp_iter_next(iter)) {
>>> +		kfree(iter);
>>> +		return NULL;
>>> +	}
>>> +	while (n--)
>>>  		if (qp_iter_next(iter)) {
>>>  			kfree(iter);
>>>  			return NULL;
>>>  		}
>>
>> It looks like you forgot to remove the lines above.
> 
> Nope this replaces a call to qp_iter_next which was in qp_iter_init.
> 
> As the commit messages says we move the implict next's back into the respective
> start functions.
> 
> Ira

Thanks, applied.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
    GPG Key ID: 0E572FDD


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]