Hello,
On Sun, May 08, 2022 at 07:31:46PM +0200, Alexandr Nedvedicky wrote:
> Hello,
>
> On Fri, May 06, 2022 at 09:50:30PM +0200, Alexander Bluhm wrote:
> > Hi,
> >
> > When creating network load by forwarding packets, SSH gets unusable
> > and ping time gets above 10 seconds.
> >
> > Problem is that while multiple forwarding threads are running with
> > shared net lock, the exclusive lock cannot be acquired. This is
> > unfair.
> >
> > Diff below prevents that a read lock is granted when another thread
> > is waiting for the exclusive lock. With that ping time stays under
> > 300 ms.
> >
> > Does this read write lock prio change make sense?
>
> I can confirm this diff helps. And if I understand things right,
> then we basically let all readers to take a slow path via a sleep,
> if there is a writer waiting (think of like putting readers
> behind the waiting writer in queue).
>
> I was afraid the change can trade writer starvation for reader
> starvation. However I think it is not the case after seeing
> rw_do_exit(), where we zero out lock after writer does rw_exit().
> this basically gives equal chance to all readers/writers to acquire
> the lock.
>
> not my department, however I would say change looks good to me.
>
I was perhaps too fast in my judgement. I think we should also
look more closely at rw_do_exit():
335 /* membar_exit_before_atomic() has to precede call of this function. */
336 void
337 rw_do_exit(struct rwlock *rwl, unsigned long wrlock)
338 {
339 unsigned long owner, set;
340
341 do {
342 owner = rwl->rwl_owner;
343 if (wrlock)
344 set = 0;
345 else
346 set = (owner - RWLOCK_READ_INCR) &
347 ~(RWLOCK_WAIT|RWLOCK_WRWANT);
348 } while (__predict_false(rw_cas(&rwl->rwl_owner, owner, set)));
349
350 if (owner & RWLOCK_WAIT)
351 wakeup(rwl);
352
my question is why do we reset RWLOCK_WAIT and RWLOCK_WRWANT flags?
I think those flags should be reset the last reader gone. Perhaps
the else branch for reader requires this:
else {
set = (owner - RWLOCK_READ_INCR) &
~(RWLOCK_WAIT|RWLOCK_WRWANT)
if (set != 0)
set |= (owner & RWLOCK_MASK);
}
thanks and
regards
sashan
