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

[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