Messages in this thread | | | Subject | Re: [PATCH v4 14/16] locking/rwsem: Guard against making count negative | From | Waiman Long <> | Date | Wed, 24 Apr 2019 12:49:05 -0400 |
| |
On 4/24/19 3:09 AM, Peter Zijlstra wrote: > On Tue, Apr 23, 2019 at 03:12:16PM -0400, Waiman Long wrote: >> That is true in general, but doing preempt_disable/enable across >> function boundary is ugly and prone to further problems down the road. > We do worse things in this code, and the thing Linus proposes is > actually quite simple, something like so: > > --- > --- a/kernel/locking/rwsem.c > +++ b/kernel/locking/rwsem.c > @@ -912,7 +904,7 @@ rwsem_down_read_slowpath(struct rw_semap > raw_spin_unlock_irq(&sem->wait_lock); > break; > } > - schedule(); > + schedule_preempt_disabled(); > lockevent_inc(rwsem_sleep_reader); > } > > @@ -1121,6 +1113,7 @@ static struct rw_semaphore *rwsem_downgr > */ > inline void __down_read(struct rw_semaphore *sem) > { > + preempt_disable(); > if (unlikely(atomic_long_fetch_add_acquire(RWSEM_READER_BIAS, > &sem->count) & RWSEM_READ_FAILED_MASK)) { > rwsem_down_read_slowpath(sem, TASK_UNINTERRUPTIBLE); > @@ -1129,10 +1122,12 @@ inline void __down_read(struct rw_semaph > } else { > rwsem_set_reader_owned(sem); > } > + preempt_enable(); > } > > static inline int __down_read_killable(struct rw_semaphore *sem) > { > + preempt_disable(); > if (unlikely(atomic_long_fetch_add_acquire(RWSEM_READER_BIAS, > &sem->count) & RWSEM_READ_FAILED_MASK)) { > if (IS_ERR(rwsem_down_read_slowpath(sem, TASK_KILLABLE))) > @@ -1142,6 +1137,7 @@ static inline int __down_read_killable(s > } else { > rwsem_set_reader_owned(sem); > } > + preempt_enable(); > return 0; > } >
Making that change will help the slowpath to has less preemption points. For an uncontended rwsem, this offers no real benefit. Adding preempt_disable() is more complicated than I originally thought.
Maybe we are too paranoid about the possibility of a large number of preemptions happening just at the right moment. If p is the probably of a preemption in the middle of the inc-check-dec sequence, which I have already moved as close to each other as possible. We are talking a probability of p^32768. Since p will be really small, the compound probability will be infinitesimally small.
So I would like to not do preemption now for the current patchset. We can restart the discussion later on if there is a real concern that it may actually happen. Please let me know if you still want to add preempt_disable() for the read lock.
Cheers, Longman
| |