[PATCH] iio: make blocking read wait for data

[PATCH] iio: make blocking read wait for data

[Josselin Costanzi]

Currently the IIO buffer blocking read only wait until at least one
data element is available.
This patch makes the reader sleep until enough data is collected
before returning to userspace.
This commit also fix a bug where data is lost if an error happens
after some data is already read.

Signed-off-by: Josselin Costanzi <josselin.costanzi@mobile-devices.fr>
---
 drivers/iio/industrialio-buffer.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/iio/industrialio-buffer.c b/drivers/iio/industrialio-buffer.c
index e472cff..ce8a1df 100644
--- a/drivers/iio/industrialio-buffer.c
+++ b/drivers/iio/industrialio-buffer.c
@@ -56,6 +56,7 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 {
 	struct iio_dev *indio_dev = filp->private_data;
 	struct iio_buffer *rb = indio_dev->buffer;
+	size_t count = 0;
 	int ret;
 
 	if (!indio_dev->info)
@@ -66,24 +67,31 @@ ssize_t iio_buffer_read_first_n_outer(struct file *filp, char __user *buf,
 
 	do {
 		if (!iio_buffer_data_available(rb)) {
-			if (filp->f_flags & O_NONBLOCK)
-				return -EAGAIN;
+			if (filp->f_flags & O_NONBLOCK) {
+				ret = -EAGAIN;
+				break;
+			}
 
 			ret = wait_event_interruptible(rb->pollq,
 					iio_buffer_data_available(rb) ||
 					indio_dev->info == NULL);
 			if (ret)
-				return ret;
-			if (indio_dev->info == NULL)
-				return -ENODEV;
+				break;
+			if (indio_dev->info == NULL) {
+				ret = -ENODEV;
+				break;
+			}
 		}
 
-		ret = rb->access->read_first_n(rb, n, buf);
-		if (ret == 0 && (filp->f_flags & O_NONBLOCK))
-			ret = -EAGAIN;
-	 } while (ret == 0);
+		ret = rb->access->read_first_n(rb, n, buf + count);
+		if (ret < 0)
+			break;
 
-	return ret;
+		n -= ret;
+		count += ret;
+	 } while (n > 0);
+
+	return count ? count : ret;
 }
 
 /**
-- 
1.9.1

Re: [PATCH] iio: make blocking read wait for data

[Lars-Peter Clausen]

On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
> Currently the IIO buffer blocking read only wait until at least one
> data element is available.

But that is how it is supposed to work. With this patch for example you 
won't be able to read the last data from the buffer after the buffer has 
been disabled. Or if for example n is not aligned to the sample size you'll 
also continue to loop forever.

- Lars

Re: [PATCH] iio: make blocking read wait for data

[Josselin Costanzi]

2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>
> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>
>> Currently the IIO buffer blocking read only wait until at least one
>> data element is available.
>
> But that is how it is supposed to work. With this patch for example you won't be able to read the last data from the buffer after the buffer has been disabled.

I don't understand the usecase this patch breaks... If the buffer is
disabled then we return what was read alreay.


> Or if for example n is not aligned to the sample size you'll also continue to loop forever.

If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
still return data multiple of samples size? In that case we would copy
complete elements until we try to do a short read which would fail at
n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).

-- 
Josselin Costanzi
  Embedded Linux System Engineer

Re: [PATCH] iio: make blocking read wait for data

[Lars-Peter Clausen]

On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>
>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>
>>> Currently the IIO buffer blocking read only wait until at least one
>>> data element is available.
>>
>> But that is how it is supposed to work. With this patch for example you won't be able to read the last data from the buffer after the buffer has been disabled.
>
> I don't understand the usecase this patch breaks... If the buffer is
> disabled then we return what was read alreay.
>

If there is less data in the buffer than the amount of requested data you'll 
keep looping forever.

>
>> Or if for example n is not aligned to the sample size you'll also continue to loop forever.
>
> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
> still return data multiple of samples size? In that case we would copy
> complete elements until we try to do a short read which would fail at
> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>

The read_first_n() callback will make sure that it only returns full 
samples, which means if n is smaller than the sample size it will return 0.

Now in your patch you have
	do {
		ret = read_first_n(..., n, ...);
		n -= ret;
	} while (n > 0);

So if n is not a multiple of the sample size you'll end up with n being 
smaller than the size of one sample but larger than 0 and at that point 
read_first_n() will always return 0 and n will stay at its current value. 
This means the exit condition of the while loop will never be met and you 
keep on looping forever.

Maybe start with which problem you are trying to address with this patch and 
then we can work forward from there to a solution. The current form of the 
patch changes the semantics of read() in a way they shouldn't be changed, so 
this is not only about the implementation bugs.

- Lars

Re: [PATCH] iio: make blocking read wait for data

[Josselin Costanzi]

2014-06-06 17:40 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:

> On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
>>
>> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>
>>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>>
>>>> Currently the IIO buffer blocking read only wait until at least one
>>>> data element is available.
>>>
>>> But that is how it is supposed to work. With this patch for example you
>>> won't be able to read the last data from the buffer after the buffer has
>>> been disabled.
>>
>> I don't understand the usecase this patch breaks... If the buffer is
>> disabled then we return what was read alreay.
>
> If there is less data in the buffer than the amount of requested data you'll
> keep looping forever.

We are working with an accelerometer and the only trigger is periodic so we
didn't think of that since we are sure we end up with enough data.
Is there a way to know when if the trigger is done generating events and no
more data is expected?

Even with the patch as is, we can use a signal to interrupt the read so it's
possible for the userspace to set a timeout (this works with our userspace,
on a SIGINT we correctly get the data acquired so far).


>>> Or if for example n is not aligned to the sample size you'll also
>>> continue to loop forever.
>>
>> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
>> still return data multiple of samples size? In that case we would copy
>> complete elements until we try to do a short read which would fail at
>> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>
> The read_first_n() callback will make sure that it only returns full
> samples, which means if n is smaller than the sample size it will return 0.

In fact it won't return 0 but -EINVAL then read returns the size of
the data already
copied (this also works with our userspace program)


> Maybe start with which problem you are trying to address with this patch and
> then we can work forward from there to a solution. The current form of the
> patch changes the semantics of read() in a way they shouldn't be changed, so
> this is not only about the implementation bugs.

Our use case is we continuously get data from an accelerometer. The sample
frequency is relatively low (< 1KHz) so the userland ends up making one syscall
per sample when we would prefer to process the data by batches to reduce the cpu
load.
We would like to have the blocking read() semantics where we wait for the data
until the buffer is full unless there is an exceptional condition as
it's usually done for
ttys and sockets.

I think the patch doesn't break anything except for the case where the
trigger generates
a finite and unknown number of data samples and the userspace requests
more than this
amount of samples.
But in this scenario, without the patch, a blocking read does the same
as a poll followed
by a non blocking read which is the standard way to get a variable
amount of data.

-- 
Josselin Costanzi
  Embedded Linux System Engineer

Re: [PATCH] iio: make blocking read wait for data

[Jonathan Cameron]

On 06/06/14 18:15, Josselin Costanzi wrote:
> 2014-06-06 17:40 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>
>> On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
>>>
>>> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>
>>>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>>>
>>>>> Currently the IIO buffer blocking read only wait until at least one
>>>>> data element is available.
>>>>
>>>> But that is how it is supposed to work. With this patch for example you
>>>> won't be able to read the last data from the buffer after the buffer has
>>>> been disabled.
>>>
>>> I don't understand the usecase this patch breaks... If the buffer is
>>> disabled then we return what was read alreay.
>>
>> If there is less data in the buffer than the amount of requested data you'll
>> keep looping forever.
>
> We are working with an accelerometer and the only trigger is periodic so we
> didn't think of that since we are sure we end up with enough data.
> Is there a way to know when if the trigger is done generating events and no
> more data is expected?
It should be done when the sysfs write to turn the buffer off is complete.
Otherwise, no - there is is general no way to know when some triggers will
fire.
>
> Even with the patch as is, we can use a signal to interrupt the read so it's
> possible for the userspace to set a timeout (this works with our userspace,
> on a SIGINT we correctly get the data acquired so far).
We could, but see below...
>
>
>>>> Or if for example n is not aligned to the sample size you'll also
>>>> continue to loop forever.
>>>
>>> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
>>> still return data multiple of samples size? In that case we would copy
>>> complete elements until we try to do a short read which would fail at
>>> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>>
>> The read_first_n() callback will make sure that it only returns full
>> samples, which means if n is smaller than the sample size it will return 0.
>
> In fact it won't return 0 but -EINVAL then read returns the size of
> the data already
> copied (this also works with our userspace program)
>
>
>> Maybe start with which problem you are trying to address with this patch and
>> then we can work forward from there to a solution. The current form of the
>> patch changes the semantics of read() in a way they shouldn't be changed, so
>> this is not only about the implementation bugs.
>
> Our use case is we continuously get data from an accelerometer. The sample
> frequency is relatively low (< 1KHz) so the userland ends up making one syscall
> per sample when we would prefer to process the data by batches to reduce the cpu
> load.
> We would like to have the blocking read() semantics where we wait for the data
> until the buffer is full unless there is an exceptional condition as
> it's usually done for
> ttys and sockets.
Be careful with your examples.  This simply isn't true for sockets. Try sending
a large amount of date over a tcp connection using blocking reads. You frequently
get less data than requested as it tends to return when individual packets come
in.  Note the man page for read makes it absolutely clear that partial data may
be returned and if so the userspace program is responsible for reading again
until it gets what it wants.

>
> I think the patch doesn't break anything except for the case where the
> trigger generates
> a finite and unknown number of data samples and the userspace requests
> more than this
> amount of samples.
A very common situation...
> But in this scenario, without the patch, a blocking read does the same
> as a poll followed
> by a non blocking read which is the standard way to get a variable
> amount of data.
Sorry, but we simply aren't going to change the semantics of this as this
is a major userspace ABI change.  What we have at the moment conforms
entirely to what is permitted by the semantics of read.

Now having said that, we have had a number of discussions of how to support
watershed type events on the buffer over the years.  I think the current
consensus would be to use one of the more obscure POLL types to indicate
that there was more than a specified amount of data in a buffer ready
to be read.  The most recent thread was related specifically to hardware
buffers, but as I point out later in the discussion any interface should
work equally well with software buffers.

http://marc.info/?l=linux-iio&m=139939531422385&w=2

If/when this gets implemented the semantics (which will need a man page
of their own given the 'unusual' use of POLL_PRI) will be

poll (POLLIN,  POLLRDNORM) - wait for some data.
read non blocking - read anything that is there.
read blocking - read what is there or wait until something is then read that.
poll (POLLPRI, POLLRDBAND) - wait for a sysfs defined watershed level of data
be available.

Now to actually do this requires some additional functions for the buffers
interface, and an implementation within the kfifo buffer + any others
people are using.  As it stands, kfifo has the ability to query how
many elements are unused which gets us at least close to what we want...
I've cc'd Srinivas as he may have made some progress on an interface
for this.

Long ago we actually had this sort of functionality in sw_ring but no
one ever seemed to use it and the interface was hideous.

J

Re: [PATCH] iio: make blocking read wait for data

[Lars-Peter Clausen]

On 06/07/2014 12:18 PM, Jonathan Cameron wrote:
> On 06/06/14 18:15, Josselin Costanzi wrote:
>> 2014-06-06 17:40 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>
>>> On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
>>>>
>>>> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>>
>>>>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>>>>
>>>>>> Currently the IIO buffer blocking read only wait until at least one
>>>>>> data element is available.
>>>>>
>>>>> But that is how it is supposed to work. With this patch for example you
>>>>> won't be able to read the last data from the buffer after the buffer has
>>>>> been disabled.
>>>>
>>>> I don't understand the usecase this patch breaks... If the buffer is
>>>> disabled then we return what was read alreay.
>>>
>>> If there is less data in the buffer than the amount of requested data you'll
>>> keep looping forever.
>>
>> We are working with an accelerometer and the only trigger is periodic so we
>> didn't think of that since we are sure we end up with enough data.
>> Is there a way to know when if the trigger is done generating events and no
>> more data is expected?
> It should be done when the sysfs write to turn the buffer off is complete.
> Otherwise, no - there is is general no way to know when some triggers will
> fire.
>>
>> Even with the patch as is, we can use a signal to interrupt the read so it's
>> possible for the userspace to set a timeout (this works with our userspace,
>> on a SIGINT we correctly get the data acquired so far).
> We could, but see below...
>>
>>
>>>>> Or if for example n is not aligned to the sample size you'll also
>>>>> continue to loop forever.
>>>>
>>>> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
>>>> still return data multiple of samples size? In that case we would copy
>>>> complete elements until we try to do a short read which would fail at
>>>> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>>>
>>> The read_first_n() callback will make sure that it only returns full
>>> samples, which means if n is smaller than the sample size it will return 0.
>>
>> In fact it won't return 0 but -EINVAL then read returns the size of
>> the data already
>> copied (this also works with our userspace program)
>>
>>
>>> Maybe start with which problem you are trying to address with this patch and
>>> then we can work forward from there to a solution. The current form of the
>>> patch changes the semantics of read() in a way they shouldn't be changed, so
>>> this is not only about the implementation bugs.
>>
>> Our use case is we continuously get data from an accelerometer. The sample
>> frequency is relatively low (< 1KHz) so the userland ends up making one
>> syscall
>> per sample when we would prefer to process the data by batches to reduce
>> the cpu
>> load.
>> We would like to have the blocking read() semantics where we wait for the
>> data
>> until the buffer is full unless there is an exceptional condition as
>> it's usually done for
>> ttys and sockets.
> Be careful with your examples.  This simply isn't true for sockets. Try sending
> a large amount of date over a tcp connection using blocking reads. You
> frequently
> get less data than requested as it tends to return when individual packets come
> in.  Note the man page for read makes it absolutely clear that partial data may
> be returned and if so the userspace program is responsible for reading again
> until it gets what it wants.
>
>>
>> I think the patch doesn't break anything except for the case where the
>> trigger generates
>> a finite and unknown number of data samples and the userspace requests
>> more than this
>> amount of samples.
> A very common situation...
>> But in this scenario, without the patch, a blocking read does the same
>> as a poll followed
>> by a non blocking read which is the standard way to get a variable
>> amount of data.
> Sorry, but we simply aren't going to change the semantics of this as this
> is a major userspace ABI change.  What we have at the moment conforms
> entirely to what is permitted by the semantics of read.
>
> Now having said that, we have had a number of discussions of how to support
> watershed type events on the buffer over the years.  I think the current
> consensus would be to use one of the more obscure POLL types to indicate
> that there was more than a specified amount of data in a buffer ready
> to be read.  The most recent thread was related specifically to hardware
> buffers, but as I point out later in the discussion any interface should
> work equally well with software buffers.
>
> http://marc.info/?l=linux-iio&m=139939531422385&w=2
>
> If/when this gets implemented the semantics (which will need a man page
> of their own given the 'unusual' use of POLL_PRI) will be
>
> poll (POLLIN,  POLLRDNORM) - wait for some data.
> read non blocking - read anything that is there.
> read blocking - read what is there or wait until something is then read that.
> poll (POLLPRI, POLLRDBAND) - wait for a sysfs defined watershed level of data
> be available.
>
> Now to actually do this requires some additional functions for the buffers
> interface, and an implementation within the kfifo buffer + any others
> people are using.  As it stands, kfifo has the ability to query how
> many elements are unused which gets us at least close to what we want...
> I've cc'd Srinivas as he may have made some progress on an interface
> for this.

I'm not sure if we need to or should overload the POLLPRI/POLLRDBAND 
semantics with a IIO specific meaning. In my opinion introducing a watershed 
level should be enough. E.g. if it is set to 0 you get the current behavior. 
It should not be to hard to implement this, the important part is to make 
sure to not wakeup the pollqueue each time something is added to the FIFO, 
but only if the FIFO level rises above the threshold. And when the buffer is 
disabled the threshold is ignored and the pollqueue is woken unconditionally 
to make it possible to read any residue that is left in the FIFO.

It is probably also worth looking at how other frameworks handle this. (I 
think ALSA pretty much does what I just described).

- Lars

Re: [PATCH] iio: make blocking read wait for data

[Jonathan Cameron]

On 09/06/14 20:57, Lars-Peter Clausen wrote:
> On 06/07/2014 12:18 PM, Jonathan Cameron wrote:
>> On 06/06/14 18:15, Josselin Costanzi wrote:
>>> 2014-06-06 17:40 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>
>>>> On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
>>>>>
>>>>> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>>>
>>>>>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>>>>>
>>>>>>> Currently the IIO buffer blocking read only wait until at least one
>>>>>>> data element is available.
>>>>>>
>>>>>> But that is how it is supposed to work. With this patch for example you
>>>>>> won't be able to read the last data from the buffer after the buffer has
>>>>>> been disabled.
>>>>>
>>>>> I don't understand the usecase this patch breaks... If the buffer is
>>>>> disabled then we return what was read alreay.
>>>>
>>>> If there is less data in the buffer than the amount of requested data you'll
>>>> keep looping forever.
>>>
>>> We are working with an accelerometer and the only trigger is periodic so we
>>> didn't think of that since we are sure we end up with enough data.
>>> Is there a way to know when if the trigger is done generating events and no
>>> more data is expected?
>> It should be done when the sysfs write to turn the buffer off is complete.
>> Otherwise, no - there is is general no way to know when some triggers will
>> fire.
>>>
>>> Even with the patch as is, we can use a signal to interrupt the read so it's
>>> possible for the userspace to set a timeout (this works with our userspace,
>>> on a SIGINT we correctly get the data acquired so far).
>> We could, but see below...
>>>
>>>
>>>>>> Or if for example n is not aligned to the sample size you'll also
>>>>>> continue to loop forever.
>>>>>
>>>>> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
>>>>> still return data multiple of samples size? In that case we would copy
>>>>> complete elements until we try to do a short read which would fail at
>>>>> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>>>>
>>>> The read_first_n() callback will make sure that it only returns full
>>>> samples, which means if n is smaller than the sample size it will return 0.
>>>
>>> In fact it won't return 0 but -EINVAL then read returns the size of
>>> the data already
>>> copied (this also works with our userspace program)
>>>
>>>
>>>> Maybe start with which problem you are trying to address with this patch and
>>>> then we can work forward from there to a solution. The current form of the
>>>> patch changes the semantics of read() in a way they shouldn't be changed, so
>>>> this is not only about the implementation bugs.
>>>
>>> Our use case is we continuously get data from an accelerometer. The sample
>>> frequency is relatively low (< 1KHz) so the userland ends up making one
>>> syscall
>>> per sample when we would prefer to process the data by batches to reduce
>>> the cpu
>>> load.
>>> We would like to have the blocking read() semantics where we wait for the
>>> data
>>> until the buffer is full unless there is an exceptional condition as
>>> it's usually done for
>>> ttys and sockets.
>> Be careful with your examples.  This simply isn't true for sockets. Try sending
>> a large amount of date over a tcp connection using blocking reads. You
>> frequently
>> get less data than requested as it tends to return when individual packets come
>> in.  Note the man page for read makes it absolutely clear that partial data may
>> be returned and if so the userspace program is responsible for reading again
>> until it gets what it wants.
>>
>>>
>>> I think the patch doesn't break anything except for the case where the
>>> trigger generates
>>> a finite and unknown number of data samples and the userspace requests
>>> more than this
>>> amount of samples.
>> A very common situation...
>>> But in this scenario, without the patch, a blocking read does the same
>>> as a poll followed
>>> by a non blocking read which is the standard way to get a variable
>>> amount of data.
>> Sorry, but we simply aren't going to change the semantics of this as this
>> is a major userspace ABI change.  What we have at the moment conforms
>> entirely to what is permitted by the semantics of read.
>>
>> Now having said that, we have had a number of discussions of how to support
>> watershed type events on the buffer over the years.  I think the current
>> consensus would be to use one of the more obscure POLL types to indicate
>> that there was more than a specified amount of data in a buffer ready
>> to be read.  The most recent thread was related specifically to hardware
>> buffers, but as I point out later in the discussion any interface should
>> work equally well with software buffers.
>>
>> http://marc.info/?l=linux-iio&m=139939531422385&w=2
>>
>> If/when this gets implemented the semantics (which will need a man page
>> of their own given the 'unusual' use of POLL_PRI) will be
>>
>> poll (POLLIN,  POLLRDNORM) - wait for some data.
>> read non blocking - read anything that is there.
>> read blocking - read what is there or wait until something is then read that.
>> poll (POLLPRI, POLLRDBAND) - wait for a sysfs defined watershed level of data
>> be available.
>>
>> Now to actually do this requires some additional functions for the buffers
>> interface, and an implementation within the kfifo buffer + any others
>> people are using.  As it stands, kfifo has the ability to query how
>> many elements are unused which gets us at least close to what we want...
>> I've cc'd Srinivas as he may have made some progress on an interface
>> for this.
>
> I'm not sure if we need to or should overload the POLLPRI/POLLRDBAND
> semantics with a IIO specific meaning. In my opinion introducing a
> watershed level should be enough. E.g. if it is set to 0 you get the
> current behavior. It should not be to hard to implement this, the
> important part is to make sure to not wakeup the pollqueue each time
> something is added to the FIFO, but only if the FIFO level rises
> above the threshold. And when the buffer is disabled the threshold is
> ignored and the pollqueue is woken unconditionally to make it
> possible to read any residue that is left in the FIFO.
>
That works for me.
> It is probably also worth looking at how other frameworks handle
> this. (I think ALSA pretty much does what I just described).
Alsa is the only one I can think of that might do something similar.
Input definitely does straight forward polling as we currently do.

Anyone know if V4L allows multiple frames to be polled for?  Can't
think what it would be for, but it is a functionality decent computer
vision cameras offer so maybe it is there somewhere....

Otherwise we are into areas such as networking which don't correspond
that closely really as they mostly don't have a concept of 'timed' sets
of data.

Thoughts welcome, but right now I like the simplicity of what Lars
suggests. I think it will also work just fine for hardware buffers.
Their watershed would presumably be set to some divisor of the requested
length as appropriate for a given part...

J
> - Lars

Re: [PATCH] iio: make blocking read wait for data

[Josselin Costanzi]

2014-06-09 22:12 GMT+02:00 Jonathan Cameron <jic23@kernel.org>:
> On 09/06/14 20:57, Lars-Peter Clausen wrote:
>>
>> On 06/07/2014 12:18 PM, Jonathan Cameron wrote:
>>>
>>> On 06/06/14 18:15, Josselin Costanzi wrote:
>>>>
>>>> 2014-06-06 17:40 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>
>>>>> On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
>>>>>>
>>>>>>
>>>>>> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>>>>
>>>>>>>
>>>>>>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>> Currently the IIO buffer blocking read only wait until at least one
>>>>>>>> data element is available.
>>>>>>>
>>>>>>>
>>>>>>> But that is how it is supposed to work. With this patch for example
>>>>>>> you
>>>>>>> won't be able to read the last data from the buffer after the buffer
>>>>>>> has
>>>>>>> been disabled.
>>>>>>
>>>>>>
>>>>>> I don't understand the usecase this patch breaks... If the buffer is
>>>>>> disabled then we return what was read alreay.
>>>>>
>>>>>
>>>>> If there is less data in the buffer than the amount of requested data
>>>>> you'll
>>>>> keep looping forever.
>>>>
>>>>
>>>> We are working with an accelerometer and the only trigger is periodic so
>>>> we
>>>> didn't think of that since we are sure we end up with enough data.
>>>> Is there a way to know when if the trigger is done generating events and
>>>> no
>>>> more data is expected?
>>>
>>> It should be done when the sysfs write to turn the buffer off is
>>> complete.
>>> Otherwise, no - there is is general no way to know when some triggers
>>> will
>>> fire.
>>>>
>>>>
>>>> Even with the patch as is, we can use a signal to interrupt the read so
>>>> it's
>>>> possible for the userspace to set a timeout (this works with our
>>>> userspace,
>>>> on a SIGINT we correctly get the data acquired so far).
>>>
>>> We could, but see below...
>>>>
>>>>
>>>>
>>>>>>> Or if for example n is not aligned to the sample size you'll also
>>>>>>> continue to loop forever.
>>>>>>
>>>>>>
>>>>>> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
>>>>>> still return data multiple of samples size? In that case we would copy
>>>>>> complete elements until we try to do a short read which would fail at
>>>>>> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>>>>>
>>>>>
>>>>> The read_first_n() callback will make sure that it only returns full
>>>>> samples, which means if n is smaller than the sample size it will
>>>>> return 0.
>>>>
>>>>
>>>> In fact it won't return 0 but -EINVAL then read returns the size of
>>>> the data already
>>>> copied (this also works with our userspace program)
>>>>
>>>>
>>>>> Maybe start with which problem you are trying to address with this
>>>>> patch and
>>>>> then we can work forward from there to a solution. The current form of
>>>>> the
>>>>> patch changes the semantics of read() in a way they shouldn't be
>>>>> changed, so
>>>>> this is not only about the implementation bugs.
>>>>
>>>>
>>>> Our use case is we continuously get data from an accelerometer. The
>>>> sample
>>>> frequency is relatively low (< 1KHz) so the userland ends up making one
>>>> syscall
>>>> per sample when we would prefer to process the data by batches to reduce
>>>> the cpu
>>>> load.
>>>> We would like to have the blocking read() semantics where we wait for
>>>> the
>>>> data
>>>> until the buffer is full unless there is an exceptional condition as
>>>> it's usually done for
>>>> ttys and sockets.
>>>
>>> Be careful with your examples.  This simply isn't true for sockets. Try
>>> sending
>>> a large amount of date over a tcp connection using blocking reads. You
>>> frequently
>>> get less data than requested as it tends to return when individual
>>> packets come
>>> in.  Note the man page for read makes it absolutely clear that partial
>>> data may
>>> be returned and if so the userspace program is responsible for reading
>>> again
>>> until it gets what it wants.
>>>
>>>>
>>>> I think the patch doesn't break anything except for the case where the
>>>> trigger generates
>>>> a finite and unknown number of data samples and the userspace requests
>>>> more than this
>>>> amount of samples.
>>>
>>> A very common situation...
>>>>
>>>> But in this scenario, without the patch, a blocking read does the same
>>>> as a poll followed
>>>> by a non blocking read which is the standard way to get a variable
>>>> amount of data.
>>>
>>> Sorry, but we simply aren't going to change the semantics of this as this
>>> is a major userspace ABI change.  What we have at the moment conforms
>>> entirely to what is permitted by the semantics of read.
>>>
>>> Now having said that, we have had a number of discussions of how to
>>> support
>>> watershed type events on the buffer over the years.  I think the current
>>> consensus would be to use one of the more obscure POLL types to indicate
>>> that there was more than a specified amount of data in a buffer ready
>>> to be read.  The most recent thread was related specifically to hardware
>>> buffers, but as I point out later in the discussion any interface should
>>> work equally well with software buffers.
>>>
>>> http://marc.info/?l=linux-iio&m=139939531422385&w=2
>>>
>>> If/when this gets implemented the semantics (which will need a man page
>>> of their own given the 'unusual' use of POLL_PRI) will be
>>>
>>> poll (POLLIN,  POLLRDNORM) - wait for some data.
>>> read non blocking - read anything that is there.
>>> read blocking - read what is there or wait until something is then read
>>> that.
>>> poll (POLLPRI, POLLRDBAND) - wait for a sysfs defined watershed level of
>>> data
>>> be available.
>>>
>>> Now to actually do this requires some additional functions for the
>>> buffers
>>> interface, and an implementation within the kfifo buffer + any others
>>> people are using.  As it stands, kfifo has the ability to query how
>>> many elements are unused which gets us at least close to what we want...
>>> I've cc'd Srinivas as he may have made some progress on an interface
>>> for this.
>>
>>
>> I'm not sure if we need to or should overload the POLLPRI/POLLRDBAND
>> semantics with a IIO specific meaning. In my opinion introducing a
>> watershed level should be enough. E.g. if it is set to 0 you get the
>> current behavior. It should not be to hard to implement this, the
>> important part is to make sure to not wakeup the pollqueue each time
>> something is added to the FIFO, but only if the FIFO level rises
>> above the threshold. And when the buffer is disabled the threshold is
>> ignored and the pollqueue is woken unconditionally to make it
>> possible to read any residue that is left in the FIFO.
>>
> That works for me.
>
>> It is probably also worth looking at how other frameworks handle
>> this. (I think ALSA pretty much does what I just described).
>
> Alsa is the only one I can think of that might do something similar.
> Input definitely does straight forward polling as we currently do.
>
> Anyone know if V4L allows multiple frames to be polled for?  Can't
> think what it would be for, but it is a functionality decent computer
> vision cameras offer so maybe it is there somewhere....
>
> Otherwise we are into areas such as networking which don't correspond
> that closely really as they mostly don't have a concept of 'timed' sets
> of data.
>
> Thoughts welcome, but right now I like the simplicity of what Lars
> suggests. I think it will also work just fine for hardware buffers.
> Their watershed would presumably be set to some divisor of the requested
> length as appropriate for a given part...
>
> J
>>
>> - Lars
>
>

Ok, thanks for the pointers.
Adding a per buffer sysfs watermark and timeout seems like a good
solution for data streaming with IIO.
I will try to look into this.

-- 
Josselin Costanzi
  Embedded Linux System Engineer

Re: [PATCH] iio: make blocking read wait for data

[Srinivas Pandruvada]

On 06/10/2014 03:55 AM, Josselin Costanzi wrote:
> 2014-06-09 22:12 GMT+02:00 Jonathan Cameron <jic23@kernel.org>:
>> On 09/06/14 20:57, Lars-Peter Clausen wrote:
>>>
>>> On 06/07/2014 12:18 PM, Jonathan Cameron wrote:
>>>>
>>>> On 06/06/14 18:15, Josselin Costanzi wrote:
>>>>>
>>>>> 2014-06-06 17:40 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>>
>>>>>> On 06/06/2014 05:12 PM, Josselin Costanzi wrote:
>>>>>>>
>>>>>>>
>>>>>>> 2014-06-06 16:38 GMT+02:00 Lars-Peter Clausen <lars@metafoo.de>:
>>>>>>>>
>>>>>>>>
>>>>>>>> On 06/06/2014 04:30 PM, Josselin Costanzi wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Currently the IIO buffer blocking read only wait until at least one
>>>>>>>>> data element is available.
>>>>>>>>
>>>>>>>>
>>>>>>>> But that is how it is supposed to work. With this patch for example
>>>>>>>> you
>>>>>>>> won't be able to read the last data from the buffer after the buffer
>>>>>>>> has
>>>>>>>> been disabled.
>>>>>>>
>>>>>>>
>>>>>>> I don't understand the usecase this patch breaks... If the buffer is
>>>>>>> disabled then we return what was read alreay.
>>>>>>
>>>>>>
>>>>>> If there is less data in the buffer than the amount of requested data
>>>>>> you'll
>>>>>> keep looping forever.
>>>>>
>>>>>
>>>>> We are working with an accelerometer and the only trigger is periodic so
>>>>> we
>>>>> didn't think of that since we are sure we end up with enough data.
>>>>> Is there a way to know when if the trigger is done generating events and
>>>>> no
>>>>> more data is expected?
>>>>
>>>> It should be done when the sysfs write to turn the buffer off is
>>>> complete.
>>>> Otherwise, no - there is is general no way to know when some triggers
>>>> will
>>>> fire.
>>>>>
>>>>>
>>>>> Even with the patch as is, we can use a signal to interrupt the read so
>>>>> it's
>>>>> possible for the userspace to set a timeout (this works with our
>>>>> userspace,
>>>>> on a SIGINT we correctly get the data acquired so far).
>>>>
>>>> We could, but see below...
>>>>>
>>>>>
>>>>>
>>>>>>>> Or if for example n is not aligned to the sample size you'll also
>>>>>>>> continue to loop forever.
>>>>>>>
>>>>>>>
>>>>>>> If n isn't aligned to the sample size, wouldn't iio_read_first_n_kfifo
>>>>>>> still return data multiple of samples size? In that case we would copy
>>>>>>> complete elements until we try to do a short read which would fail at
>>>>>>> n < kfifo_esize(&kf->kf) (in iio_read_first_n_kfifo).
>>>>>>
>>>>>>
>>>>>> The read_first_n() callback will make sure that it only returns full
>>>>>> samples, which means if n is smaller than the sample size it will
>>>>>> return 0.
>>>>>
>>>>>
>>>>> In fact it won't return 0 but -EINVAL then read returns the size of
>>>>> the data already
>>>>> copied (this also works with our userspace program)
>>>>>
>>>>>
>>>>>> Maybe start with which problem you are trying to address with this
>>>>>> patch and
>>>>>> then we can work forward from there to a solution. The current form of
>>>>>> the
>>>>>> patch changes the semantics of read() in a way they shouldn't be
>>>>>> changed, so
>>>>>> this is not only about the implementation bugs.
>>>>>
>>>>>
>>>>> Our use case is we continuously get data from an accelerometer. The
>>>>> sample
>>>>> frequency is relatively low (< 1KHz) so the userland ends up making one
>>>>> syscall
>>>>> per sample when we would prefer to process the data by batches to reduce
>>>>> the cpu
>>>>> load.
>>>>> We would like to have the blocking read() semantics where we wait for
>>>>> the
>>>>> data
>>>>> until the buffer is full unless there is an exceptional condition as
>>>>> it's usually done for
>>>>> ttys and sockets.
>>>>
>>>> Be careful with your examples.  This simply isn't true for sockets. Try
>>>> sending
>>>> a large amount of date over a tcp connection using blocking reads. You
>>>> frequently
>>>> get less data than requested as it tends to return when individual
>>>> packets come
>>>> in.  Note the man page for read makes it absolutely clear that partial
>>>> data may
>>>> be returned and if so the userspace program is responsible for reading
>>>> again
>>>> until it gets what it wants.
>>>>
>>>>>
>>>>> I think the patch doesn't break anything except for the case where the
>>>>> trigger generates
>>>>> a finite and unknown number of data samples and the userspace requests
>>>>> more than this
>>>>> amount of samples.
>>>>
>>>> A very common situation...
>>>>>
>>>>> But in this scenario, without the patch, a blocking read does the same
>>>>> as a poll followed
>>>>> by a non blocking read which is the standard way to get a variable
>>>>> amount of data.
>>>>
>>>> Sorry, but we simply aren't going to change the semantics of this as this
>>>> is a major userspace ABI change.  What we have at the moment conforms
>>>> entirely to what is permitted by the semantics of read.
>>>>
>>>> Now having said that, we have had a number of discussions of how to
>>>> support
>>>> watershed type events on the buffer over the years.  I think the current
>>>> consensus would be to use one of the more obscure POLL types to indicate
>>>> that there was more than a specified amount of data in a buffer ready
>>>> to be read.  The most recent thread was related specifically to hardware
>>>> buffers, but as I point out later in the discussion any interface should
>>>> work equally well with software buffers.
>>>>
>>>> http://marc.info/?l=linux-iio&m=139939531422385&w=2
>>>>
>>>> If/when this gets implemented the semantics (which will need a man page
>>>> of their own given the 'unusual' use of POLL_PRI) will be
>>>>
>>>> poll (POLLIN,  POLLRDNORM) - wait for some data.
>>>> read non blocking - read anything that is there.
>>>> read blocking - read what is there or wait until something is then read
>>>> that.
>>>> poll (POLLPRI, POLLRDBAND) - wait for a sysfs defined watershed level of
>>>> data
>>>> be available.
>>>>
>>>> Now to actually do this requires some additional functions for the
>>>> buffers
>>>> interface, and an implementation within the kfifo buffer + any others
>>>> people are using.  As it stands, kfifo has the ability to query how
>>>> many elements are unused which gets us at least close to what we want...
>>>> I've cc'd Srinivas as he may have made some progress on an interface
>>>> for this.
>>>
>>>
>>> I'm not sure if we need to or should overload the POLLPRI/POLLRDBAND
>>> semantics with a IIO specific meaning. In my opinion introducing a
>>> watershed level should be enough. E.g. if it is set to 0 you get the
>>> current behavior. It should not be to hard to implement this, the
>>> important part is to make sure to not wakeup the pollqueue each time
>>> something is added to the FIFO, but only if the FIFO level rises
>>> above the threshold. And when the buffer is disabled the threshold is
>>> ignored and the pollqueue is woken unconditionally to make it
>>> possible to read any residue that is left in the FIFO.
>>>
>> That works for me.
>>
>>> It is probably also worth looking at how other frameworks handle
>>> this. (I think ALSA pretty much does what I just described).
>>
>> Alsa is the only one I can think of that might do something similar.
>> Input definitely does straight forward polling as we currently do.
>>
>> Anyone know if V4L allows multiple frames to be polled for?  Can't
>> think what it would be for, but it is a functionality decent computer
>> vision cameras offer so maybe it is there somewhere....
>>
>> Otherwise we are into areas such as networking which don't correspond
>> that closely really as they mostly don't have a concept of 'timed' sets
>> of data.
>>
>> Thoughts welcome, but right now I like the simplicity of what Lars
>> suggests. I think it will also work just fine for hardware buffers.
>> Their watershed would presumably be set to some divisor of the requested
>> length as appropriate for a given part...
>>
>> J
>>>
>>> - Lars
>>
>>
>
> Ok, thanks for the pointers.
> Adding a per buffer sysfs watermark and timeout seems like a good
> solution for data streaming with IIO.
> I will try to look into this.

Are you planning to implement this? If you are, then I will wait for 
your updates, otherwise this is my to-do list.

Thanks,
Srinivas

>