Re: [PATCH 1/5] locking: Introduce range reader/writer lock

[Davidlohr Bueso <dbueso@suse.de>]

On 2017-03-30 07:56, Laurent Dufour wrote:
> On 07/03/2017 06:03, Davidlohr Bueso wrote:
>> +static inline int wait_for_ranges(struct range_rwlock_tree *tree,
>> +				  struct range_rwlock *lock, long state)
>> +{
>> +	int ret = 0;
>> +
>> +	while (true) {
>> +		set_current_state(state);
>> +
>> +		if (unlikely(signal_pending_state(state, current))) {
>> +			unsigned long flags;
>> +
>> +			ret = -EINTR;
>> +			/*
>> +			 * We're not taking the lock after all, cleanup
>> +			 * after ourselves.
>> +			 */
>> +			spin_lock_irqsave(&tree->lock, flags);
>> +			lock->reader = false;
>> +			__range_tree_remove(tree, lock);
>> +			spin_unlock_irqrestore(&tree->lock, flags);
>> +			break;
>> +		}
>> +
>> +		/* do we need to go to sleep? */
>> +		if (!lock->blocking_ranges)
>> +			break;
> 
> Hi Davidlohr,
> 
> While building a kernel on top of a patched kernel using full range 
> lock
> in the place of mmap_sem, I found that fork() sometimes failed 
> returning
> ENOMEM.
> It happens that if fork() get called at the time signal is sent to the
> calling process, the call to range_write_lock_interruptible() is 
> failing
> even if there is no contention on the lock. This is because we check 
> for
> the signal pending before checking for the lock contention in
> wait_for_ranges().
> 
> The loop here should rather be :
> 
> 	while (true) {
> 		/* do we need to go to sleep? */
> 		if (!lock->blocking_ranges)
> 			break;
> 

Thanks, this makes sense, and is actually the standard way of waiting in
most locks.

> 		if (unlikely(signal_pending_state(state, current))) {
> 			unsigned long flags;
> 
> 			ret = -EINTR;
> 			/*
> 			 * We're not taking the lock after all, cleanup
> 			 * after ourselves.
> 			 */
> 			spin_lock_irqsave(&tree->lock, flags);
> 			lock->reader = false;
> 			__range_tree_remove(tree, lock);
> 			spin_unlock_irqrestore(&tree->lock, flags);
> 			break;
> 		}
> 
> 		set_current_state(state);
> 
> 		schedule();
> 	}
> 
> I also moved the call to set_current_state() before calling schedule(),
> not sure this has to be done this way, but my system seems to work fine
> like this.

No, we do not hold any locks. Please keep set_current_state() the very 
first
thing we do in the loop. You can check Documentation/memory-barriers.txt
for details :-)

Thanks,
Davidlohr