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

[Davidlohr Bueso]

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