Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
On Tue, 2018-09-11 at 15:50 +0200, Peter Zijlstra wrote: > On Tue, Sep 11, 2018 at 02:33:22PM +0100, Dmitry Safonov wrote: > > > > You might want to think about ditching that ldsem thing > > > > entirely, > > > > and use a regular rwsem ? > > > > > > Yeah, but AFAICS, regular rwsem will need to have a timeout then > > > (for > > > write). So, I thought fixing this pile would be simpler than > > > adding > > > timeout and probably writer-priority to generic rwsem? > > > > > > And I guess, we still will need fixes for stable for the bugs > > > here.. > > > > > > I expect that timeouts are ABI, while the gain of adding priority > > > may > > > be measured. I'll give it a shot (adding timeout/priority for > > > linux- > > > next) to rwsem if you say it's acceptable. > > > > Actually, priority looks quite simple: we can add writers in the > > head > > of wait_list and it probably may work. > > Timeout looks also not a rocket science. > > So, I can try to do that if you say it's acceptable (with the gain > > measures). > > So why do you need writer priority? The comment that goes with ldsems > doesn't explain I think, it just says it has it. Well, as far as I can fetch from the commit 4898e640caf0, it describes that you should halt and scrap pending i/o (reader side) to prevent the loss or change of the current line dicipline (write lock). So, AFAIU, line discipline is expected to change within 5 sec by ABI and write-priority makes it more likely. > In general I dislike unfair locks, they always cause trouble. > > > After this can of worms that I need to fix regardless. > > Sure. -- Thanks, Dmitry
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
On Tue, Sep 11, 2018 at 02:33:22PM +0100, Dmitry Safonov wrote: > > > You might want to think about ditching that ldsem thing entirely, > > > and use a regular rwsem ? > > > > Yeah, but AFAICS, regular rwsem will need to have a timeout then (for > > write). So, I thought fixing this pile would be simpler than adding > > timeout and probably writer-priority to generic rwsem? > > > > And I guess, we still will need fixes for stable for the bugs here.. > > > > I expect that timeouts are ABI, while the gain of adding priority may > > be measured. I'll give it a shot (adding timeout/priority for linux- > > next) to rwsem if you say it's acceptable. > > Actually, priority looks quite simple: we can add writers in the head > of wait_list and it probably may work. > Timeout looks also not a rocket science. > So, I can try to do that if you say it's acceptable (with the gain > measures). So why do you need writer priority? The comment that goes with ldsems doesn't explain I think, it just says it has it. In general I dislike unfair locks, they always cause trouble. > After this can of worms that I need to fix regardless. Sure.
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
On Tue, 2018-09-11 at 14:01 +0100, Dmitry Safonov wrote: > On Tue, 2018-09-11 at 14:02 +0200, Peter Zijlstra wrote: > > On Tue, Sep 11, 2018 at 02:48:21AM +0100, Dmitry Safonov wrote: > > > It seems like when ldsem_down_read() fails with timeout, it > > > misses > > > update for sem->wait_readers. By that reason, when writer finally > > > releases write end of the semaphore __ldsem_wake_readers() does > > > adjust > > > sem->count with wrong value: > > > sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS) > > > > > > I.e, if update comes with 1 missed wait_readers decrement, sem- > > > > count > > > > > > will be 0x10001 which means that there is active reader and > > > it'll > > > make any further writer to fail in acquiring the semaphore. > > > > > > It looks like, this is a dead-code, because ldsem_down_read() is > > > never > > > called with timeout different than MAX_SCHEDULE_TIMEOUT, so it > > > might be > > > worth to delete timeout parameter and error path fall-back.. > > > > You might want to think about ditching that ldsem thing entirely, > > and > > use a regular rwsem ? > > Yeah, but AFAICS, regular rwsem will need to have a timeout then (for > write). So, I thought fixing this pile would be simpler than adding > timeout and probably writer-priority to generic rwsem? > > And I guess, we still will need fixes for stable for the bugs here.. > > I expect that timeouts are ABI, while the gain of adding priority may > be measured. I'll give it a shot (adding timeout/priority for linux- > next) to rwsem if you say it's acceptable. Actually, priority looks quite simple: we can add writers in the head of wait_list and it probably may work. Timeout looks also not a rocket science. So, I can try to do that if you say it's acceptable (with the gain measures). After this can of worms that I need to fix regardless. -- Thanks, Dmitry
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
On Tue, 2018-09-11 at 14:02 +0200, Peter Zijlstra wrote: > On Tue, Sep 11, 2018 at 02:48:21AM +0100, Dmitry Safonov wrote: > > It seems like when ldsem_down_read() fails with timeout, it misses > > update for sem->wait_readers. By that reason, when writer finally > > releases write end of the semaphore __ldsem_wake_readers() does > > adjust > > sem->count with wrong value: > > sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS) > > > > I.e, if update comes with 1 missed wait_readers decrement, sem- > > >count > > will be 0x10001 which means that there is active reader and > > it'll > > make any further writer to fail in acquiring the semaphore. > > > > It looks like, this is a dead-code, because ldsem_down_read() is > > never > > called with timeout different than MAX_SCHEDULE_TIMEOUT, so it > > might be > > worth to delete timeout parameter and error path fall-back.. > > You might want to think about ditching that ldsem thing entirely, and > use a regular rwsem ? Yeah, but AFAICS, regular rwsem will need to have a timeout then (for write). So, I thought fixing this pile would be simpler than adding timeout and probably writer-priority to generic rwsem? And I guess, we still will need fixes for stable for the bugs here.. I expect that timeouts are ABI, while the gain of adding priority may be measured. I'll give it a shot (adding timeout/priority for linux- next) to rwsem if you say it's acceptable. -- Thanks, Dmitry
Re: [PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
On Tue, Sep 11, 2018 at 02:48:21AM +0100, Dmitry Safonov wrote: > It seems like when ldsem_down_read() fails with timeout, it misses > update for sem->wait_readers. By that reason, when writer finally > releases write end of the semaphore __ldsem_wake_readers() does adjust > sem->count with wrong value: > sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS) > > I.e, if update comes with 1 missed wait_readers decrement, sem->count > will be 0x10001 which means that there is active reader and it'll > make any further writer to fail in acquiring the semaphore. > > It looks like, this is a dead-code, because ldsem_down_read() is never > called with timeout different than MAX_SCHEDULE_TIMEOUT, so it might be > worth to delete timeout parameter and error path fall-back.. You might want to think about ditching that ldsem thing entirely, and use a regular rwsem ?
[PATCHv3 6/6] tty/ldsem: Decrement wait_readers on timeouted down_read()
It seems like when ldsem_down_read() fails with timeout, it misses
update for sem->wait_readers. By that reason, when writer finally
releases write end of the semaphore __ldsem_wake_readers() does adjust
sem->count with wrong value:
sem->wait_readers * (LDSEM_ACTIVE_BIAS - LDSEM_WAIT_BIAS)
I.e, if update comes with 1 missed wait_readers decrement, sem->count
will be 0x10001 which means that there is active reader and it'll
make any further writer to fail in acquiring the semaphore.
It looks like, this is a dead-code, because ldsem_down_read() is never
called with timeout different than MAX_SCHEDULE_TIMEOUT, so it might be
worth to delete timeout parameter and error path fall-back..
Cc: Greg Kroah-Hartman
Cc: Jiri Slaby
Signed-off-by: Dmitry Safonov
---
drivers/tty/tty_ldsem.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/tty_ldsem.c b/drivers/tty/tty_ldsem.c
index 832accbbcb6d..f7966ab7b450 100644
--- a/drivers/tty/tty_ldsem.c
+++ b/drivers/tty/tty_ldsem.c
@@ -237,6 +237,7 @@ down_read_failed(struct ld_semaphore *sem, long count, long
timeout)
raw_spin_lock_irq(&sem->wait_lock);
if (waiter.task) {
atomic_long_add_return(-LDSEM_WAIT_BIAS, &sem->count);
+ sem->wait_readers--;
list_del(&waiter.list);
raw_spin_unlock_irq(&sem->wait_lock);
put_task_struct(waiter.task);
--
2.13.6
