| Message ID | 20190624083323.11876-1-oneukum@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gpss: core: no waiters left behind on deregister | expand |
On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote: > If you deregister a device you need to wake up all waiters > as there will be no further wakeups. > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > --- > drivers/gnss/core.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c > index e6f94501cb28..0d13bd2cefd5 100644 > --- a/drivers/gnss/core.c > +++ b/drivers/gnss/core.c > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev) > down_write(&gdev->rwsem); > gdev->disconnected = true; > if (gdev->count) { > - wake_up_interruptible(&gdev->read_queue); > + wake_up_interruptible_all(&gdev->read_queue); GNSS core doesn't have any exclusive waiters, so no need to use use the exclusive wake-up (all) interface. > gdev->ops->close(gdev); > } > up_write(&gdev->rwsem); Thanks, Johan
Hello!
The subject has gpss ISO gnss. You hardly meant the General Purpose
System Simulation. :-)
On 24.06.2019 11:33, Oliver Neukum wrote:
> If you deregister a device you need to wake up all waiters
> as there will be no further wakeups.
>
> Signed-off-by: Oliver Neukum <oneukum@suse.com>
> ---
> drivers/gnss/core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c
> index e6f94501cb28..0d13bd2cefd5 100644
> --- a/drivers/gnss/core.c
> +++ b/drivers/gnss/core.c
> @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev)
> down_write(&gdev->rwsem);
> gdev->disconnected = true;
> if (gdev->count) {
> - wake_up_interruptible(&gdev->read_queue);
> + wake_up_interruptible_all(&gdev->read_queue);
> gdev->ops->close(gdev);
> }
> up_write(&gdev->rwsem);
MBR, Sergei
Am Dienstag, den 25.06.2019, 09:04 +0200 schrieb Johan Hovold: > On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote: > > If you deregister a device you need to wake up all waiters > > as there will be no further wakeups. > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > --- > > drivers/gnss/core.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c > > index e6f94501cb28..0d13bd2cefd5 100644 > > --- a/drivers/gnss/core.c > > +++ b/drivers/gnss/core.c > > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev) > > down_write(&gdev->rwsem); > > gdev->disconnected = true; > > if (gdev->count) { > > - wake_up_interruptible(&gdev->read_queue); > > + wake_up_interruptible_all(&gdev->read_queue); > > GNSS core doesn't have any exclusive waiters, so no need to use use the > exclusive wake-up (all) interface. Well, yes, but that is the problem. In gnss_read() you drop the lock: mutex_lock(&gdev->read_mutex); while (kfifo_is_empty(&gdev->read_fifo)) { mutex_unlock(&gdev->read_mutex); if (gdev->disconnected) return 0; if (file->f_flags & O_NONBLOCK) return -EAGAIN; That means that an arbitrary number of tasks can get here. ret = wait_event_interruptible(gdev->read_queue, gdev->disconnected || !kfifo_is_empty(&gdev->read_fifo)); Meaning that an arbitrary number can be sleeping here. Yet in gnss_deregister_device() you use a simple wake_up: void gnss_deregister_device(struct gnss_device *gdev) { down_write(&gdev->rwsem); gdev->disconnected = true; if (gdev->count) { wake_up_interruptible(&gdev->read_queue); wake_up_interruptible() will wake up one waiting task. But after that the device is gone. There will be no further events. The other tasks will sleep forever. Regards Oliver
On Wed, Jun 26, 2019 at 01:04:07PM +0200, Oliver Neukum wrote: > Am Dienstag, den 25.06.2019, 09:04 +0200 schrieb Johan Hovold: > > On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote: > > > If you deregister a device you need to wake up all waiters > > > as there will be no further wakeups. > > > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > > --- > > > drivers/gnss/core.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c > > > index e6f94501cb28..0d13bd2cefd5 100644 > > > --- a/drivers/gnss/core.c > > > +++ b/drivers/gnss/core.c > > > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev) > > > down_write(&gdev->rwsem); > > > gdev->disconnected = true; > > > if (gdev->count) { > > > - wake_up_interruptible(&gdev->read_queue); > > > + wake_up_interruptible_all(&gdev->read_queue); > > > > GNSS core doesn't have any exclusive waiters, so no need to use use the > > exclusive wake-up (all) interface. > > Well, yes, but that is the problem. In gnss_read() you drop the lock: > That means that an arbitrary number of tasks can get here. > > ret = wait_event_interruptible(gdev->read_queue, > gdev->disconnected || > !kfifo_is_empty(&gdev->read_fifo)); > > Meaning that an arbitrary number can be sleeping here. I understand wait you're getting at, but I think your mistaken regarding exclusive wait. Note that wait_event_interruptible() uses nonexclusive wait. > Yet in gnss_deregister_device() you use a simple wake_up: > > void gnss_deregister_device(struct gnss_device *gdev) > > { > > down_write(&gdev->rwsem); > gdev->disconnected = true; > if (gdev->count) { > wake_up_interruptible(&gdev->read_queue); > > > wake_up_interruptible() will wake up one waiting task. But after that > the device is gone. There will be no further events. The other tasks > will sleep forever. No, wake_up_interruptible() will wake up all nonexclusive waiters, which is all we care about here. Johan
Am Mittwoch, den 26.06.2019, 13:41 +0200 schrieb Johan Hovold: > On Wed, Jun 26, 2019 at 01:04:07PM +0200, Oliver Neukum wrote: > > Am Dienstag, den 25.06.2019, 09:04 +0200 schrieb Johan Hovold: > > > On Mon, Jun 24, 2019 at 10:33:23AM +0200, Oliver Neukum wrote: > > > > If you deregister a device you need to wake up all waiters > > > > as there will be no further wakeups. > > > > > > > > Signed-off-by: Oliver Neukum <oneukum@suse.com> > > > > --- > > > > drivers/gnss/core.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > > > diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c > > > > index e6f94501cb28..0d13bd2cefd5 100644 > > > > --- a/drivers/gnss/core.c > > > > +++ b/drivers/gnss/core.c > > > > @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev) > > > > down_write(&gdev->rwsem); > > > > gdev->disconnected = true; > > > > if (gdev->count) { > > > > - wake_up_interruptible(&gdev->read_queue); > > > > + wake_up_interruptible_all(&gdev->read_queue); > > > > > > GNSS core doesn't have any exclusive waiters, so no need to use use the > > > exclusive wake-up (all) interface. > > > > Well, yes, but that is the problem. In gnss_read() you drop the lock: > > That means that an arbitrary number of tasks can get here. > > > > ret = wait_event_interruptible(gdev->read_queue, > > gdev->disconnected || > > !kfifo_is_empty(&gdev->read_fifo)); > > > > Meaning that an arbitrary number can be sleeping here. > > I understand wait you're getting at, but I think your mistaken regarding > exclusive wait. Note that wait_event_interruptible() uses nonexclusive > wait. > > > Yet in gnss_deregister_device() you use a simple wake_up: > > > > void gnss_deregister_device(struct gnss_device *gdev) > > > > { > > > > down_write(&gdev->rwsem); > > gdev->disconnected = true; > > if (gdev->count) { > > wake_up_interruptible(&gdev->read_queue); > > > > > > wake_up_interruptible() will wake up one waiting task. But after that > > the device is gone. There will be no further events. The other tasks > > will sleep forever. > > No, wake_up_interruptible() will wake up all nonexclusive waiters, > which is all we care about here. You are right and tracing this is hard. Regards & Sorry Oliver
diff --git a/drivers/gnss/core.c b/drivers/gnss/core.c index e6f94501cb28..0d13bd2cefd5 100644 --- a/drivers/gnss/core.c +++ b/drivers/gnss/core.c @@ -303,7 +303,7 @@ void gnss_deregister_device(struct gnss_device *gdev) down_write(&gdev->rwsem); gdev->disconnected = true; if (gdev->count) { - wake_up_interruptible(&gdev->read_queue); + wake_up_interruptible_all(&gdev->read_queue); gdev->ops->close(gdev); } up_write(&gdev->rwsem);
If you deregister a device you need to wake up all waiters as there will be no further wakeups. Signed-off-by: Oliver Neukum <oneukum@suse.com> --- drivers/gnss/core.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)