sw_ring.c poll problem

sw_ring.c poll problem

[Ge Gao]

Dear all,
	I found that ring_sw.c the poll function has to read half of the
data for poll to work. Basically, you have to fill half of the ring buffer
in order for poll to be triggrerred. You have to also read more than half
of the data for poll to be disappeared. This would pose problems. If you
have big ring buffer, the data will lost its immediacy. If you have small
ring buffer, the data could lost if not buffered enough. Is that possible
this poll action configurable? Or I missed anything.
	Thanks.

Best regards,

Ge GAO

Re: sw_ring.c poll problem

[Jonathan Cameron]

Ge Gao <ggao@invensense.com> wrote:

>Dear all,
>	I found that ring_sw.c the poll function has to read half of the
>data for poll to work. Basically, you have to fill half of the ring
>buffer
>in order for poll to be triggrerred. You have to also read more than
>half
>of the data for poll to be disappeared. This would pose problems. If
>you
>have big ring buffer, the data will lost its immediacy. If you have
>small
>ring buffer, the data could lost if not buffered enough. Is that
>possible
>this poll action configurable? Or I missed anything.

Use kfifobuf instead. Sw _ring is going away anyway.
>	Thanks.
>
>Best regards,
>
>Ge GAO
>--
>To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>the body of a message to majordomo@vger.kernel.org
>More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Sent from my Android phone with K-9 Mail. Please excuse my brevity.

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 5/16/2012 6:46 AM, Jonathan Cameron wrote:
>
>
> Ge Gao<ggao@invensense.com>  wrote:
>
>> Dear all,
>> 	I found that ring_sw.c the poll function has to read half of the
>> data for poll to work. Basically, you have to fill half of the ring
>> buffer
>> in order for poll to be triggrerred. You have to also read more than
>> half
>> of the data for poll to be disappeared. This would pose problems. If
>> you
>> have big ring buffer, the data will lost its immediacy. If you have
>> small
>> ring buffer, the data could lost if not buffered enough. Is that
>> possible
>> this poll action configurable? Or I missed anything.
>
> Use kfifobuf instead. Sw _ring is going away anyway.

Hi Ge,

I realised after sending that message that I was being rather dismissive
of your query.  Got up far too early this morning (as every morning ;)

Anyhow, to give more details. sw_ring is probably never going to make it
out of staging, hence the move to kfifo_buf. At somepoint we need to 
work out how to do equivalent functionality of sw_ring but I've not had
time to more than start looking into this.

As you saw, poll on sw_ring is a watershead signal indicating (in theory
and last I checked it worked) that the ring is more than half full.
Any read that takes the fill level below half (test code just reads half
the size of the buffer), should allow a new passing of the watershead
to resignal poll. It's entirely possible there is a bug in there though
I know it is been getting a fair bit of testing with some other drivers
so could be todo with the precise way you are reading it hitting some
corner case? (I'm stretching...)

Right now I'd just move over to kfifo_buf if I were you. It's much more
'standard' in that it's a fifo and poll indicates if there is anything
there at all.
>> 	Thanks.
>>
>> Best regards,
>>
>> Ge GAO
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

RE: sw_ring.c poll problem

[Ge Gao]

Dear Jonathan,
	Thank you very much for the information .  I will switch to kfifo. The
problem was that a lot of drivers,  which are already in the kernel,  seem
to use sw_ring, I was following other driver's route to do it.
	There is no problem with the sw_ring if we follow the way it should be
used. However, the motion integration algorithm requires that the data comes
in immediately when it is available and don't lose data. This half full
requirement poses some problems. If we set the buffer length too small, we
could lose data if the CPU is accidently delayed. If we set the length too
big, it will take a while for the data to come and the data becomes too old
when it comes to user application. It would be better that we have a ring
buffer that is long enough yet the poll will pass when there is only one
sample in the ring buffer. Hopefully, this kfifo can do the job.
	Now that I understand the mechanism underneath, I will switch to kfifo for
our driver resubmitted patch.

Best regards,

Ge GAO


-----Original Message-----
From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
Sent: Wednesday, May 16, 2012 12:16 AM
To: Ge Gao; Jonathan Cameron
Cc: linux-iio@vger.kernel.org
Subject: Re: sw_ring.c poll problem

On 5/16/2012 6:46 AM, Jonathan Cameron wrote:
>
>
> Ge Gao<ggao@invensense.com>  wrote:
>
>> Dear all,
>> 	I found that ring_sw.c the poll function has to read half of the
>> data for poll to work. Basically, you have to fill half of the ring
>> buffer in order for poll to be triggrerred. You have to also read
>> more than half of the data for poll to be disappeared. This would
>> pose problems. If you have big ring buffer, the data will lost its
>> immediacy. If you have small ring buffer, the data could lost if not
>> buffered enough. Is that possible this poll action configurable? Or I
>> missed anything.
>
> Use kfifobuf instead. Sw _ring is going away anyway.

Hi Ge,

I realised after sending that message that I was being rather dismissive of
your query.  Got up far too early this morning (as every morning ;)

Anyhow, to give more details. sw_ring is probably never going to make it out
of staging, hence the move to kfifo_buf. At somepoint we need to work out
how to do equivalent functionality of sw_ring but I've not had time to more
than start looking into this.

As you saw, poll on sw_ring is a watershead signal indicating (in theory and
last I checked it worked) that the ring is more than half full.
Any read that takes the fill level below half (test code just reads half the
size of the buffer), should allow a new passing of the watershead to
resignal poll. It's entirely possible there is a bug in there though I know
it is been getting a fair bit of testing with some other drivers so could be
todo with the precise way you are reading it hitting some corner case? (I'm
stretching...)

Right now I'd just move over to kfifo_buf if I were you. It's much more
'standard' in that it's a fifo and poll indicates if there is anything there
at all.
>> 	Thanks.
>>
>> Best regards,
>>
>> Ge GAO
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>> in the body of a message to majordomo@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>

RE: sw_ring.c poll problem

[Ge Gao]

Dear Jonathan,
	I check the kfifo buffer implementation under IIO directory. However, I
didn't find the "pollq" is released anywhere as sw_ring does.  Without it,
how can kfifo be polled if it is used by industrial-buffer.c? Thanks.

Best Regards,

Ge GAO


Hi Ge,

I realised after sending that message that I was being rather dismissive of
your query.  Got up far too early this morning (as every morning ;)

Anyhow, to give more details. sw_ring is probably never going to make it out
of staging, hence the move to kfifo_buf. At somepoint we need to work out
how to do equivalent functionality of sw_ring but I've not had time to more
than start looking into this.

As you saw, poll on sw_ring is a watershead signal indicating (in theory and
last I checked it worked) that the ring is more than half full.
Any read that takes the fill level below half (test code just reads half the
size of the buffer), should allow a new passing of the watershead to
resignal poll. It's entirely possible there is a bug in there though I know
it is been getting a fair bit of testing with some other drivers so could be
todo with the precise way you are reading it hitting some corner case? (I'm
stretching...)

Right now I'd just move over to kfifo_buf if I were you. It's much more
'standard' in that it's a fifo and poll indicates if there is anything there
at all.
>> 	Thanks.
>>
>> Best regards,
>>
>> Ge GAO
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>> in the body of a message to majordomo@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 05/18/2012 02:08 AM, Ge Gao wrote:
> Dear Jonathan,
> 	I check the kfifo buffer implementation under IIO directory. However, I
> didn't find the "pollq" is released anywhere as sw_ring does.  Without it,
> how can kfifo be polled if it is used by industrial-buffer.c? Thanks.
Sorry for not getting back to you sooner.  Totally manic at the day job
today tracking two delightful bugs.

Anyhow, looks like I'd imagined we'd actually added poll support to
kfifo when it never got implemented.  Should be easy enough though.

diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
index 6bf9d05..e69d6ce 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -6,6 +6,7 @@
 #include <linux/kfifo.h>
 #include <linux/mutex.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/sched.h>

 struct iio_kfifo {
 	struct iio_buffer buffer;
@@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer *r)
 	kfifo_free(&buf->kf);
 	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
 				   buf->buffer.length);
+	r->stufftoread = false;
 error_ret:
 	return ret;
 }
@@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
 	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
 	if (ret != r->bytes_per_datum)
 		return -EBUSY;
+	
+	r->stufftoread = true;
+	wake_up_interruptible(&r->pollq);
+
 	return 0;
 }

@@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer *r,
 	n = rounddown(n, r->bytes_per_datum);
 	ret = kfifo_to_user(&kf->kf, buf, n, &copied);

+	if (kfifo_is_empty(&kf->kf))
+		r->stufftoread = false;
+	/* verify it is still empty to avoid race */
+	if (!kfifo_is_empty(&kf->kf))
+		r->stufftoread = true;
+
 	return copied;
 }

.. is a totally untested patch to add poll support to it.
The little dance at the end is to avoid a write having
occured between the kfifo_is_empty and setting the
flag false as that could wipe out the existence of some
data in the buffer. Given we only ever have one writer and
one reader that should work I think.

Jonathan

> 
> Hi Ge,
> 
> I realised after sending that message that I was being rather dismissive of
> your query.  Got up far too early this morning (as every morning ;)
> 
> Anyhow, to give more details. sw_ring is probably never going to make it out
> of staging, hence the move to kfifo_buf. At somepoint we need to work out
> how to do equivalent functionality of sw_ring but I've not had time to more
> than start looking into this.
> 
> As you saw, poll on sw_ring is a watershead signal indicating (in theory and
> last I checked it worked) that the ring is more than half full.
> Any read that takes the fill level below half (test code just reads half the
> size of the buffer), should allow a new passing of the watershead to
> resignal poll. It's entirely possible there is a bug in there though I know
> it is been getting a fair bit of testing with some other drivers so could be
> todo with the precise way you are reading it hitting some corner case? (I'm
> stretching...)
> 
> Right now I'd just move over to kfifo_buf if I were you. It's much more
> 'standard' in that it's a fifo and poll indicates if there is anything there
> at all.
>>> 	Thanks.
>>>
>>> Best regards,
>>>
>>> Ge GAO
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>>

RE: sw_ring.c poll problem

[Ge Gao]

Thanks for the patch. There could be one problem with the implementation.
Consider the situation:
1. there are 10 packets in the kfifo.
2. Read 1 packet. Then poll again.
3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
needs the next store to wake it up, while in reality, it should return
immediately if there is data already in the kfifo.
4. It might be better to put poll inside ring buffer implementation itself
rather than industrial-buffer.c, such as providing a new method like poll
to do it.
What do you think?
Thanks.

Best Regards,

GE GAO



-----Original Message-----
From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
Sent: Friday, May 18, 2012 10:31 AM
To: Ge Gao
Cc: Jonathan Cameron; linux-iio@vger.kernel.org
Subject: Re: sw_ring.c poll problem



On 05/18/2012 02:08 AM, Ge Gao wrote:
> Dear Jonathan,
> 	I check the kfifo buffer implementation under IIO directory.
However,
> I didn't find the "pollq" is released anywhere as sw_ring does.
> Without it, how can kfifo be polled if it is used by
industrial-buffer.c? Thanks.
Sorry for not getting back to you sooner.  Totally manic at the day job
today tracking two delightful bugs.

Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
when it never got implemented.  Should be easy enough though.

diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
6bf9d05..e69d6ce 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -6,6 +6,7 @@
 #include <linux/kfifo.h>
 #include <linux/mutex.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/sched.h>

 struct iio_kfifo {
 	struct iio_buffer buffer;
@@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
*r)
 	kfifo_free(&buf->kf);
 	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
 				   buf->buffer.length);
+	r->stufftoread = false;
 error_ret:
 	return ret;
 }
@@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
 	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
 	if (ret != r->bytes_per_datum)
 		return -EBUSY;
+	
+	r->stufftoread = true;
+	wake_up_interruptible(&r->pollq);
+
 	return 0;
 }

@@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
*r,
 	n = rounddown(n, r->bytes_per_datum);
 	ret = kfifo_to_user(&kf->kf, buf, n, &copied);

+	if (kfifo_is_empty(&kf->kf))
+		r->stufftoread = false;
+	/* verify it is still empty to avoid race */
+	if (!kfifo_is_empty(&kf->kf))
+		r->stufftoread = true;
+
 	return copied;
 }

.. is a totally untested patch to add poll support to it.
The little dance at the end is to avoid a write having occured between the
kfifo_is_empty and setting the flag false as that could wipe out the
existence of some data in the buffer. Given we only ever have one writer
and one reader that should work I think.

Jonathan

>
> Hi Ge,
>
> I realised after sending that message that I was being rather
> dismissive of your query.  Got up far too early this morning (as every
> morning ;)
>
> Anyhow, to give more details. sw_ring is probably never going to make
> it out of staging, hence the move to kfifo_buf. At somepoint we need
> to work out how to do equivalent functionality of sw_ring but I've not
> had time to more than start looking into this.
>
> As you saw, poll on sw_ring is a watershead signal indicating (in
> theory and last I checked it worked) that the ring is more than half
full.
> Any read that takes the fill level below half (test code just reads
> half the size of the buffer), should allow a new passing of the
> watershead to resignal poll. It's entirely possible there is a bug in
> there though I know it is been getting a fair bit of testing with some
> other drivers so could be todo with the precise way you are reading it
> hitting some corner case? (I'm
> stretching...)
>
> Right now I'd just move over to kfifo_buf if I were you. It's much
> more 'standard' in that it's a fifo and poll indicates if there is
> anything there at all.
>>> 	Thanks.
>>>
>>> Best regards,
>>>
>>> Ge GAO
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>>

RE: sw_ring.c poll problem

[Ge Gao]

I put a patch for the industrial-buffer.c to fix the problem of pollq
always waiting. It is just two lines fix.
I also add the quaternion definition to the types as well as core parts.
Quaternion is a four-element stuff. It has a real part and three imaginary
parts. Basically, it can be reprensented as (r + xi + yj + zk). This is
important in rotation. It is computed by invensense MPU dmp(digital motion
processing) unit and output as a data. So the real parts, I added one more
modifier, the imaginary parts I am still using x, y, z as the modifier.
Thanks.

Best Regards,

GE GAO


diff --git a/drivers/staging/iio/industrialio-buffer.c
b/drivers/staging/iio/industrialio-buffer.c
index 386ba76..3f82ded 100644
--- a/drivers/staging/iio/industrialio-buffer.c
+++ b/drivers/staging/iio/industrialio-buffer.c
@@ -56,6 +56,8 @@ unsigned int iio_buffer_poll(struct file *filp,
 {
        struct iio_dev *indio_dev = filp->private_data;
        struct iio_buffer *rb = indio_dev->buffer;
+        if (rb->stufftoread)
+                return POLLIN | POLLRDNORM;

        poll_wait(filp, &rb->pollq, wait);
        if (rb->stufftoread)

--------------------------------------------------------------------------
-------------------------------------------
diff --git a/drivers/staging/iio/types.h b/drivers/staging/iio/types.h
index 0c32136..291c783 100644
--- a/drivers/staging/iio/types.h
+++ b/drivers/staging/iio/types.h
@@ -27,6 +27,7 @@ enum iio_chan_type {
        IIO_ANGL,
        IIO_TIMESTAMP,
        IIO_CAPACITANCE,
+       IIO_QUATERNION,
 };

 enum iio_modifier {
@@ -44,6 +45,7 @@ enum iio_modifier {
        IIO_MOD_X_OR_Y_OR_Z,
        IIO_MOD_LIGHT_BOTH,
        IIO_MOD_LIGHT_IR,
+       IIO_MOD_R,
 };

 #define IIO_VAL_INT 1
--------------------------------------------------------------------------
-----------------------
diff --git a/drivers/staging/iio/industrialio-core.c
b/drivers/staging/iio/industrialio-core.c
index d303bfb..deb80aa 100644
--- a/drivers/staging/iio/industrialio-core.c
+++ b/drivers/staging/iio/industrialio-core.c
@@ -68,6 +68,7 @@ static const char * const iio_chan_type_name_spec[] = {
        [IIO_ANGL] = "angl",
        [IIO_TIMESTAMP] = "timestamp",
        [IIO_CAPACITANCE] = "capacitance",
+       [IIO_QUATERNION] = "quaternion",
 };

 static const char * const iio_modifier_names[] = {
@@ -76,6 +77,7 @@ static const char * const iio_modifier_names[] = {
        [IIO_MOD_Z] = "z",
        [IIO_MOD_LIGHT_BOTH] = "both",
        [IIO_MOD_LIGHT_IR] = "ir",
+       [IIO_MOD_R]  = "r",
 };

 /* relies on pairs of these shared then separate */


--------------------------------------------------------------------------
-----------

-----Original Message-----
From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
Sent: Friday, May 18, 2012 10:31 AM
To: Ge Gao
Cc: Jonathan Cameron; linux-iio@vger.kernel.org
Subject: Re: sw_ring.c poll problem



On 05/18/2012 02:08 AM, Ge Gao wrote:
> Dear Jonathan,
> 	I check the kfifo buffer implementation under IIO directory.
However,
> I didn't find the "pollq" is released anywhere as sw_ring does.
> Without it, how can kfifo be polled if it is used by
industrial-buffer.c? Thanks.
Sorry for not getting back to you sooner.  Totally manic at the day job
today tracking two delightful bugs.

Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
when it never got implemented.  Should be easy enough though.

diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
6bf9d05..e69d6ce 100644
--- a/drivers/iio/kfifo_buf.c
+++ b/drivers/iio/kfifo_buf.c
@@ -6,6 +6,7 @@
 #include <linux/kfifo.h>
 #include <linux/mutex.h>
 #include <linux/iio/kfifo_buf.h>
+#include <linux/sched.h>

 struct iio_kfifo {
 	struct iio_buffer buffer;
@@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
*r)
 	kfifo_free(&buf->kf);
 	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
 				   buf->buffer.length);
+	r->stufftoread = false;
 error_ret:
 	return ret;
 }
@@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
 	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
 	if (ret != r->bytes_per_datum)
 		return -EBUSY;
+	
+	r->stufftoread = true;
+	wake_up_interruptible(&r->pollq);
+
 	return 0;
 }

@@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
*r,
 	n = rounddown(n, r->bytes_per_datum);
 	ret = kfifo_to_user(&kf->kf, buf, n, &copied);

+	if (kfifo_is_empty(&kf->kf))
+		r->stufftoread = false;
+	/* verify it is still empty to avoid race */
+	if (!kfifo_is_empty(&kf->kf))
+		r->stufftoread = true;
+
 	return copied;
 }

.. is a totally untested patch to add poll support to it.
The little dance at the end is to avoid a write having occured between the
kfifo_is_empty and setting the flag false as that could wipe out the
existence of some data in the buffer. Given we only ever have one writer
and one reader that should work I think.

Jonathan

>
> Hi Ge,
>
> I realised after sending that message that I was being rather
> dismissive of your query.  Got up far too early this morning (as every
> morning ;)
>
> Anyhow, to give more details. sw_ring is probably never going to make
> it out of staging, hence the move to kfifo_buf. At somepoint we need
> to work out how to do equivalent functionality of sw_ring but I've not
> had time to more than start looking into this.
>
> As you saw, poll on sw_ring is a watershead signal indicating (in
> theory and last I checked it worked) that the ring is more than half
full.
> Any read that takes the fill level below half (test code just reads
> half the size of the buffer), should allow a new passing of the
> watershead to resignal poll. It's entirely possible there is a bug in
> there though I know it is been getting a fair bit of testing with some
> other drivers so could be todo with the precise way you are reading it
> hitting some corner case? (I'm
> stretching...)
>
> Right now I'd just move over to kfifo_buf if I were you. It's much
> more 'standard' in that it's a fifo and poll indicates if there is
> anything there at all.
>>> 	Thanks.
>>>
>>> Best regards,
>>>
>>> Ge GAO
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>> info at  http://vger.kernel.org/majordomo-info.html
>>

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 05/18/2012 07:26 PM, Ge Gao wrote:
> Thanks for the patch. There could be one problem with the implementation.
> Consider the situation:
> 1. there are 10 packets in the kfifo.
> 2. Read 1 packet. Then poll again.
> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
> needs the next store to wake it up, while in reality, it should return
> immediately if there is data already in the kfifo.
Agreed that is what should happen.  I'm just getting my head around why
it doesn't (or more specifically what is different about how we are
doing it from other similar cases).


> 4. It might be better to put poll inside ring buffer implementation itself
> rather than industrial-buffer.c, such as providing a new method like poll
> to do it.
Certainly could do that, but I'm not sure how it will help.



> What do you think?
> Thanks.
> 
> Best Regards,
> 
> GE GAO
> 
> 
> 
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
> Sent: Friday, May 18, 2012 10:31 AM
> To: Ge Gao
> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
> Subject: Re: sw_ring.c poll problem
> 
> 
> 
> On 05/18/2012 02:08 AM, Ge Gao wrote:
>> Dear Jonathan,
>> 	I check the kfifo buffer implementation under IIO directory.
> However,
>> I didn't find the "pollq" is released anywhere as sw_ring does.
>> Without it, how can kfifo be polled if it is used by
> industrial-buffer.c? Thanks.
> Sorry for not getting back to you sooner.  Totally manic at the day job
> today tracking two delightful bugs.
> 
> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
> when it never got implemented.  Should be easy enough though.
> 
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
> 6bf9d05..e69d6ce 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -6,6 +6,7 @@
>  #include <linux/kfifo.h>
>  #include <linux/mutex.h>
>  #include <linux/iio/kfifo_buf.h>
> +#include <linux/sched.h>
> 
>  struct iio_kfifo {
>  	struct iio_buffer buffer;
> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
> *r)
>  	kfifo_free(&buf->kf);
>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>  				   buf->buffer.length);
> +	r->stufftoread = false;
>  error_ret:
>  	return ret;
>  }
> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>  	if (ret != r->bytes_per_datum)
>  		return -EBUSY;
> +	
> +	r->stufftoread = true;
> +	wake_up_interruptible(&r->pollq);
> +
>  	return 0;
>  }
> 
> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
> *r,
>  	n = rounddown(n, r->bytes_per_datum);
>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
> 
> +	if (kfifo_is_empty(&kf->kf))
> +		r->stufftoread = false;
> +	/* verify it is still empty to avoid race */
> +	if (!kfifo_is_empty(&kf->kf))
> +		r->stufftoread = true;
> +
>  	return copied;
>  }
> 
> .. is a totally untested patch to add poll support to it.
> The little dance at the end is to avoid a write having occured between the
> kfifo_is_empty and setting the flag false as that could wipe out the
> existence of some data in the buffer. Given we only ever have one writer
> and one reader that should work I think.
> 
> Jonathan
> 
>>
>> Hi Ge,
>>
>> I realised after sending that message that I was being rather
>> dismissive of your query.  Got up far too early this morning (as every
>> morning ;)
>>
>> Anyhow, to give more details. sw_ring is probably never going to make
>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>> to work out how to do equivalent functionality of sw_ring but I've not
>> had time to more than start looking into this.
>>
>> As you saw, poll on sw_ring is a watershead signal indicating (in
>> theory and last I checked it worked) that the ring is more than half
> full.
>> Any read that takes the fill level below half (test code just reads
>> half the size of the buffer), should allow a new passing of the
>> watershead to resignal poll. It's entirely possible there is a bug in
>> there though I know it is been getting a fair bit of testing with some
>> other drivers so could be todo with the precise way you are reading it
>> hitting some corner case? (I'm
>> stretching...)
>>
>> Right now I'd just move over to kfifo_buf if I were you. It's much
>> more 'standard' in that it's a fifo and poll indicates if there is
>> anything there at all.
>>>> 	Thanks.
>>>>
>>>> Best regards,
>>>>
>>>> Ge GAO
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 05/18/2012 08:27 PM, Ge Gao wrote:
> I put a patch for the industrial-buffer.c to fix the problem of pollq
> always waiting. It is just two lines fix.
Hmm.. Might work given we have only one reader but smacks of a race
condition. What bothers me is that this issue should effect lots of
other subsystems and doesn't.  Evdev does it pretty much the same way we
do for example.

> I also add the quaternion definition to the types as well as core parts.
> Quaternion is a four-element stuff. It has a real part and three imaginary
> parts. Basically, it can be reprensented as (r + xi + yj + zk). This is
> important in rotation. It is computed by invensense MPU dmp(digital motion
> processing) unit and output as a data. So the real parts, I added one more
> modifier, the imaginary parts I am still using x, y, z as the modifier.
> Thanks.
Handling quaternions needs some thought.  The components do not have
independent value (it's useless knowing any one of them on their own) so
we should bind them into one channel element.  This obviously requires
us to extend the current scan_type bit of the channel spec.  That's
fine, though it will require some changes in the demux unit amongst
others which could get a little fiddly.  I suspect the biggest issue
with doing this will be preventing people using for channels where the
individual components do have independant value.  I guess I'll just
have to shout at people ;)

Jonathan
> 
> Best Regards,
> 
> GE GAO
> 
> 
> diff --git a/drivers/staging/iio/industrialio-buffer.c
> b/drivers/staging/iio/industrialio-buffer.c
> index 386ba76..3f82ded 100644
> --- a/drivers/staging/iio/industrialio-buffer.c
> +++ b/drivers/staging/iio/industrialio-buffer.c
> @@ -56,6 +56,8 @@ unsigned int iio_buffer_poll(struct file *filp,
>  {
>         struct iio_dev *indio_dev = filp->private_data;
>         struct iio_buffer *rb = indio_dev->buffer;
> +        if (rb->stufftoread)
> +                return POLLIN | POLLRDNORM;
> 
>         poll_wait(filp, &rb->pollq, wait);
>         if (rb->stufftoread)
> 
> --------------------------------------------------------------------------
> -------------------------------------------
> diff --git a/drivers/staging/iio/types.h b/drivers/staging/iio/types.h
> index 0c32136..291c783 100644
> --- a/drivers/staging/iio/types.h
> +++ b/drivers/staging/iio/types.h
> @@ -27,6 +27,7 @@ enum iio_chan_type {
>         IIO_ANGL,
>         IIO_TIMESTAMP,
>         IIO_CAPACITANCE,
> +       IIO_QUATERNION,
>  };
> 
>  enum iio_modifier {
> @@ -44,6 +45,7 @@ enum iio_modifier {
>         IIO_MOD_X_OR_Y_OR_Z,
>         IIO_MOD_LIGHT_BOTH,
>         IIO_MOD_LIGHT_IR,
> +       IIO_MOD_R,
>  };
> 
>  #define IIO_VAL_INT 1
> --------------------------------------------------------------------------
> -----------------------
> diff --git a/drivers/staging/iio/industrialio-core.c
> b/drivers/staging/iio/industrialio-core.c
> index d303bfb..deb80aa 100644
> --- a/drivers/staging/iio/industrialio-core.c
> +++ b/drivers/staging/iio/industrialio-core.c
> @@ -68,6 +68,7 @@ static const char * const iio_chan_type_name_spec[] = {
>         [IIO_ANGL] = "angl",
>         [IIO_TIMESTAMP] = "timestamp",
>         [IIO_CAPACITANCE] = "capacitance",
> +       [IIO_QUATERNION] = "quaternion",
>  };
> 
>  static const char * const iio_modifier_names[] = {
> @@ -76,6 +77,7 @@ static const char * const iio_modifier_names[] = {
>         [IIO_MOD_Z] = "z",
>         [IIO_MOD_LIGHT_BOTH] = "both",
>         [IIO_MOD_LIGHT_IR] = "ir",
> +       [IIO_MOD_R]  = "r",
>  };
> 
>  /* relies on pairs of these shared then separate */
> 
> 
> --------------------------------------------------------------------------
> -----------
> 
> -----Original Message-----
> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
> Sent: Friday, May 18, 2012 10:31 AM
> To: Ge Gao
> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
> Subject: Re: sw_ring.c poll problem
> 
> 
> 
> On 05/18/2012 02:08 AM, Ge Gao wrote:
>> Dear Jonathan,
>> 	I check the kfifo buffer implementation under IIO directory.
> However,
>> I didn't find the "pollq" is released anywhere as sw_ring does.
>> Without it, how can kfifo be polled if it is used by
> industrial-buffer.c? Thanks.
> Sorry for not getting back to you sooner.  Totally manic at the day job
> today tracking two delightful bugs.
> 
> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
> when it never got implemented.  Should be easy enough though.
> 
> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
> 6bf9d05..e69d6ce 100644
> --- a/drivers/iio/kfifo_buf.c
> +++ b/drivers/iio/kfifo_buf.c
> @@ -6,6 +6,7 @@
>  #include <linux/kfifo.h>
>  #include <linux/mutex.h>
>  #include <linux/iio/kfifo_buf.h>
> +#include <linux/sched.h>
> 
>  struct iio_kfifo {
>  	struct iio_buffer buffer;
> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
> *r)
>  	kfifo_free(&buf->kf);
>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>  				   buf->buffer.length);
> +	r->stufftoread = false;
>  error_ret:
>  	return ret;
>  }
> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>  	if (ret != r->bytes_per_datum)
>  		return -EBUSY;
> +	
> +	r->stufftoread = true;
> +	wake_up_interruptible(&r->pollq);
> +
>  	return 0;
>  }
> 
> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
> *r,
>  	n = rounddown(n, r->bytes_per_datum);
>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
> 
> +	if (kfifo_is_empty(&kf->kf))
> +		r->stufftoread = false;
> +	/* verify it is still empty to avoid race */
> +	if (!kfifo_is_empty(&kf->kf))
> +		r->stufftoread = true;
> +
>  	return copied;
>  }
> 
> .. is a totally untested patch to add poll support to it.
> The little dance at the end is to avoid a write having occured between the
> kfifo_is_empty and setting the flag false as that could wipe out the
> existence of some data in the buffer. Given we only ever have one writer
> and one reader that should work I think.
> 
> Jonathan
> 
>>
>> Hi Ge,
>>
>> I realised after sending that message that I was being rather
>> dismissive of your query.  Got up far too early this morning (as every
>> morning ;)
>>
>> Anyhow, to give more details. sw_ring is probably never going to make
>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>> to work out how to do equivalent functionality of sw_ring but I've not
>> had time to more than start looking into this.
>>
>> As you saw, poll on sw_ring is a watershead signal indicating (in
>> theory and last I checked it worked) that the ring is more than half
> full.
>> Any read that takes the fill level below half (test code just reads
>> half the size of the buffer), should allow a new passing of the
>> watershead to resignal poll. It's entirely possible there is a bug in
>> there though I know it is been getting a fair bit of testing with some
>> other drivers so could be todo with the precise way you are reading it
>> hitting some corner case? (I'm
>> stretching...)
>>
>> Right now I'd just move over to kfifo_buf if I were you. It's much
>> more 'standard' in that it's a fifo and poll indicates if there is
>> anything there at all.
>>>> 	Thanks.
>>>>
>>>> Best regards,
>>>>
>>>> Ge GAO
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>

Re: sw_ring.c poll problem

[Lars-Peter Clausen]

On 05/19/2012 11:16 AM, Jonathan Cameron wrote:
> On 05/18/2012 07:26 PM, Ge Gao wrote:
>> Thanks for the patch. There could be one problem with the implementation.
>> Consider the situation:
>> 1. there are 10 packets in the kfifo.
>> 2. Read 1 packet. Then poll again.
>> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
>> needs the next store to wake it up, while in reality, it should return
>> immediately if there is data already in the kfifo.
> Agreed that is what should happen.  I'm just getting my head around why
> it doesn't (or more specifically what is different about how we are
> doing it from other similar cases).

I'm not sure that this is what should happen. We most certainly want to have a
threshold for poll. Otherwise we'd switch back and forth between userspace and
kernel space for each sample individual sample captured. This will decrease the
overall system performance (because of all the context switching). So userspace
should be aware of that threshold and that poll might not wake the process up
even if there are samples in the buffer. ALSA basically does the same.

- Lars

> 
> 
>> 4. It might be better to put poll inside ring buffer implementation itself
>> rather than industrial-buffer.c, such as providing a new method like poll
>> to do it.
> Certainly could do that, but I'm not sure how it will help.
> 
> 
> 
>> What do you think?
>> Thanks.
>>
>> Best Regards,
>>
>> GE GAO
>>
>>
>>
>> -----Original Message-----
>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>> Sent: Friday, May 18, 2012 10:31 AM
>> To: Ge Gao
>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
>> Subject: Re: sw_ring.c poll problem
>>
>>
>>
>> On 05/18/2012 02:08 AM, Ge Gao wrote:
>>> Dear Jonathan,
>>> 	I check the kfifo buffer implementation under IIO directory.
>> However,
>>> I didn't find the "pollq" is released anywhere as sw_ring does.
>>> Without it, how can kfifo be polled if it is used by
>> industrial-buffer.c? Thanks.
>> Sorry for not getting back to you sooner.  Totally manic at the day job
>> today tracking two delightful bugs.
>>
>> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
>> when it never got implemented.  Should be easy enough though.
>>
>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
>> 6bf9d05..e69d6ce 100644
>> --- a/drivers/iio/kfifo_buf.c
>> +++ b/drivers/iio/kfifo_buf.c
>> @@ -6,6 +6,7 @@
>>  #include <linux/kfifo.h>
>>  #include <linux/mutex.h>
>>  #include <linux/iio/kfifo_buf.h>
>> +#include <linux/sched.h>
>>
>>  struct iio_kfifo {
>>  	struct iio_buffer buffer;
>> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
>> *r)
>>  	kfifo_free(&buf->kf);
>>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>>  				   buf->buffer.length);
>> +	r->stufftoread = false;
>>  error_ret:
>>  	return ret;
>>  }
>> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>>  	if (ret != r->bytes_per_datum)
>>  		return -EBUSY;
>> +	
>> +	r->stufftoread = true;
>> +	wake_up_interruptible(&r->pollq);
>> +
>>  	return 0;
>>  }
>>
>> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
>> *r,
>>  	n = rounddown(n, r->bytes_per_datum);
>>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>>
>> +	if (kfifo_is_empty(&kf->kf))
>> +		r->stufftoread = false;
>> +	/* verify it is still empty to avoid race */
>> +	if (!kfifo_is_empty(&kf->kf))
>> +		r->stufftoread = true;
>> +
>>  	return copied;
>>  }
>>
>> .. is a totally untested patch to add poll support to it.
>> The little dance at the end is to avoid a write having occured between the
>> kfifo_is_empty and setting the flag false as that could wipe out the
>> existence of some data in the buffer. Given we only ever have one writer
>> and one reader that should work I think.
>>
>> Jonathan
>>
>>>
>>> Hi Ge,
>>>
>>> I realised after sending that message that I was being rather
>>> dismissive of your query.  Got up far too early this morning (as every
>>> morning ;)
>>>
>>> Anyhow, to give more details. sw_ring is probably never going to make
>>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>>> to work out how to do equivalent functionality of sw_ring but I've not
>>> had time to more than start looking into this.
>>>
>>> As you saw, poll on sw_ring is a watershead signal indicating (in
>>> theory and last I checked it worked) that the ring is more than half
>> full.
>>> Any read that takes the fill level below half (test code just reads
>>> half the size of the buffer), should allow a new passing of the
>>> watershead to resignal poll. It's entirely possible there is a bug in
>>> there though I know it is been getting a fair bit of testing with some
>>> other drivers so could be todo with the precise way you are reading it
>>> hitting some corner case? (I'm
>>> stretching...)
>>>
>>> Right now I'd just move over to kfifo_buf if I were you. It's much
>>> more 'standard' in that it's a fifo and poll indicates if there is
>>> anything there at all.
>>>>> 	Thanks.
>>>>>
>>>>> Best regards,
>>>>>
>>>>> Ge GAO
>>>>> --
>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>>

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 05/19/2012 10:41 AM, Lars-Peter Clausen wrote:
> On 05/19/2012 11:16 AM, Jonathan Cameron wrote:
>> On 05/18/2012 07:26 PM, Ge Gao wrote:
>>> Thanks for the patch. There could be one problem with the implementation.
>>> Consider the situation:
>>> 1. there are 10 packets in the kfifo.
>>> 2. Read 1 packet. Then poll again.
>>> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
>>> needs the next store to wake it up, while in reality, it should return
>>> immediately if there is data already in the kfifo.
>> Agreed that is what should happen.  I'm just getting my head around why
>> it doesn't (or more specifically what is different about how we are
>> doing it from other similar cases).
> 
> I'm not sure that this is what should happen. We most certainly want to have a
> threshold for poll. Otherwise we'd switch back and forth between userspace and
> kernel space for each sample individual sample captured. This will decrease the
> overall system performance (because of all the context switching). So userspace
> should be aware of that threshold and that poll might not wake the process up
> even if there are samples in the buffer. ALSA basically does the same.
When this originally came up, one suggestion was to use one of the other
types of poll for this (can't recall, but maybe POLLRDBAND)

For Ge's case it actually makes sense to do it for every scan as the
scans can be very large and at relatively low frequency.

What you are asking for is one of the primary things that we loose in
moving from ring_sw to kfifo.   I keep meaning to take time to look
at adding watersheads to the kfifo implementation. Right now we just
have to dead recon in userspace and read slightly faster than data is
being generated.  We had some bells and whistles in that sw_ring that
aren't anywhere else at the moment but we'll work on that going forward.


I don't think there is any harm in Ge's fix of adding a check before
poll_wait. Worst thing it does is tell userspace there is data when
there isn't.  Slight overhead due to the race, but trivial given we
can argue the read and the poll should be in the same thread anyway
in almost all cases.

Jonathan
> 
> - Lars
> 
>>
>>
>>> 4. It might be better to put poll inside ring buffer implementation itself
>>> rather than industrial-buffer.c, such as providing a new method like poll
>>> to do it.
>> Certainly could do that, but I'm not sure how it will help.
>>
>>
>>
>>> What do you think?
>>> Thanks.
>>>
>>> Best Regards,
>>>
>>> GE GAO
>>>
>>>
>>>
>>> -----Original Message-----
>>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>>> Sent: Friday, May 18, 2012 10:31 AM
>>> To: Ge Gao
>>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
>>> Subject: Re: sw_ring.c poll problem
>>>
>>>
>>>
>>> On 05/18/2012 02:08 AM, Ge Gao wrote:
>>>> Dear Jonathan,
>>>> 	I check the kfifo buffer implementation under IIO directory.
>>> However,
>>>> I didn't find the "pollq" is released anywhere as sw_ring does.
>>>> Without it, how can kfifo be polled if it is used by
>>> industrial-buffer.c? Thanks.
>>> Sorry for not getting back to you sooner.  Totally manic at the day job
>>> today tracking two delightful bugs.
>>>
>>> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
>>> when it never got implemented.  Should be easy enough though.
>>>
>>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
>>> 6bf9d05..e69d6ce 100644
>>> --- a/drivers/iio/kfifo_buf.c
>>> +++ b/drivers/iio/kfifo_buf.c
>>> @@ -6,6 +6,7 @@
>>>  #include <linux/kfifo.h>
>>>  #include <linux/mutex.h>
>>>  #include <linux/iio/kfifo_buf.h>
>>> +#include <linux/sched.h>
>>>
>>>  struct iio_kfifo {
>>>  	struct iio_buffer buffer;
>>> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
>>> *r)
>>>  	kfifo_free(&buf->kf);
>>>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>>>  				   buf->buffer.length);
>>> +	r->stufftoread = false;
>>>  error_ret:
>>>  	return ret;
>>>  }
>>> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>>>  	if (ret != r->bytes_per_datum)
>>>  		return -EBUSY;
>>> +	
>>> +	r->stufftoread = true;
>>> +	wake_up_interruptible(&r->pollq);
>>> +
>>>  	return 0;
>>>  }
>>>
>>> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
>>> *r,
>>>  	n = rounddown(n, r->bytes_per_datum);
>>>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>>>
>>> +	if (kfifo_is_empty(&kf->kf))
>>> +		r->stufftoread = false;
>>> +	/* verify it is still empty to avoid race */
>>> +	if (!kfifo_is_empty(&kf->kf))
>>> +		r->stufftoread = true;
>>> +
>>>  	return copied;
>>>  }
>>>
>>> .. is a totally untested patch to add poll support to it.
>>> The little dance at the end is to avoid a write having occured between the
>>> kfifo_is_empty and setting the flag false as that could wipe out the
>>> existence of some data in the buffer. Given we only ever have one writer
>>> and one reader that should work I think.
>>>
>>> Jonathan
>>>
>>>>
>>>> Hi Ge,
>>>>
>>>> I realised after sending that message that I was being rather
>>>> dismissive of your query.  Got up far too early this morning (as every
>>>> morning ;)
>>>>
>>>> Anyhow, to give more details. sw_ring is probably never going to make
>>>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>>>> to work out how to do equivalent functionality of sw_ring but I've not
>>>> had time to more than start looking into this.
>>>>
>>>> As you saw, poll on sw_ring is a watershead signal indicating (in
>>>> theory and last I checked it worked) that the ring is more than half
>>> full.
>>>> Any read that takes the fill level below half (test code just reads
>>>> half the size of the buffer), should allow a new passing of the
>>>> watershead to resignal poll. It's entirely possible there is a bug in
>>>> there though I know it is been getting a fair bit of testing with some
>>>> other drivers so could be todo with the precise way you are reading it
>>>> hitting some corner case? (I'm
>>>> stretching...)
>>>>
>>>> Right now I'd just move over to kfifo_buf if I were you. It's much
>>>> more 'standard' in that it's a fifo and poll indicates if there is
>>>> anything there at all.
>>>>>> 	Thanks.
>>>>>>
>>>>>> Best regards,
>>>>>>
>>>>>> Ge GAO
>>>>>> --
>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: sw_ring.c poll problem

[Lars-Peter Clausen]

On 05/19/2012 11:50 AM, Jonathan Cameron wrote:
> On 05/19/2012 10:41 AM, Lars-Peter Clausen wrote:
>> On 05/19/2012 11:16 AM, Jonathan Cameron wrote:
>>> On 05/18/2012 07:26 PM, Ge Gao wrote:
>>>> Thanks for the patch. There could be one problem with the implementation.
>>>> Consider the situation:
>>>> 1. there are 10 packets in the kfifo.
>>>> 2. Read 1 packet. Then poll again.
>>>> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
>>>> needs the next store to wake it up, while in reality, it should return
>>>> immediately if there is data already in the kfifo.
>>> Agreed that is what should happen.  I'm just getting my head around why
>>> it doesn't (or more specifically what is different about how we are
>>> doing it from other similar cases).
>>
>> I'm not sure that this is what should happen. We most certainly want to have a
>> threshold for poll. Otherwise we'd switch back and forth between userspace and
>> kernel space for each sample individual sample captured. This will decrease the
>> overall system performance (because of all the context switching). So userspace
>> should be aware of that threshold and that poll might not wake the process up
>> even if there are samples in the buffer. ALSA basically does the same.
> When this originally came up, one suggestion was to use one of the other
> types of poll for this (can't recall, but maybe POLLRDBAND)
> 
> For Ge's case it actually makes sense to do it for every scan as the
> scans can be very large and at relatively low frequency.

Well, for the sw_ring buffer the threshold was buffer_size/2, so if you'd set
your buffer size to 2 you'd get woken up for each sample written.

> 
> What you are asking for is one of the primary things that we loose in
> moving from ring_sw to kfifo.   I keep meaning to take time to look
> at adding watersheads to the kfifo implementation. Right now we just
> have to dead recon in userspace and read slightly faster than data is
> being generated.  We had some bells and whistles in that sw_ring that
> aren't anywhere else at the moment but we'll work on that going forward.
> 

I think it is better to get rid of that stufftoread flag and add a
samples_available() callback. It'd serve a similar purpose, but instead of just
telling you that something is available or not it will tell you the exact
number of samples. Which then allows us to implement stuff like watersheads in
the generic code.

> 
> I don't think there is any harm in Ge's fix of adding a check before
> poll_wait. Worst thing it does is tell userspace there is data when
> there isn't.  Slight overhead due to the race, but trivial given we
> can argue the read and the poll should be in the same thread anyway
> in almost all cases.


I suppose it makes sense, but I'm still a bit confused as to why it works for
everybody else (all the other subsystems) without this extra check.

> 
> Jonathan
>>
>> - Lars
>>
>>>
>>>
>>>> 4. It might be better to put poll inside ring buffer implementation itself
>>>> rather than industrial-buffer.c, such as providing a new method like poll
>>>> to do it.
>>> Certainly could do that, but I'm not sure how it will help.
>>>
>>>
>>>
>>>> What do you think?
>>>> Thanks.
>>>>
>>>> Best Regards,
>>>>
>>>> GE GAO
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>>>> Sent: Friday, May 18, 2012 10:31 AM
>>>> To: Ge Gao
>>>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
>>>> Subject: Re: sw_ring.c poll problem
>>>>
>>>>
>>>>
>>>> On 05/18/2012 02:08 AM, Ge Gao wrote:
>>>>> Dear Jonathan,
>>>>> 	I check the kfifo buffer implementation under IIO directory.
>>>> However,
>>>>> I didn't find the "pollq" is released anywhere as sw_ring does.
>>>>> Without it, how can kfifo be polled if it is used by
>>>> industrial-buffer.c? Thanks.
>>>> Sorry for not getting back to you sooner.  Totally manic at the day job
>>>> today tracking two delightful bugs.
>>>>
>>>> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
>>>> when it never got implemented.  Should be easy enough though.
>>>>
>>>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
>>>> 6bf9d05..e69d6ce 100644
>>>> --- a/drivers/iio/kfifo_buf.c
>>>> +++ b/drivers/iio/kfifo_buf.c
>>>> @@ -6,6 +6,7 @@
>>>>  #include <linux/kfifo.h>
>>>>  #include <linux/mutex.h>
>>>>  #include <linux/iio/kfifo_buf.h>
>>>> +#include <linux/sched.h>
>>>>
>>>>  struct iio_kfifo {
>>>>  	struct iio_buffer buffer;
>>>> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
>>>> *r)
>>>>  	kfifo_free(&buf->kf);
>>>>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>>>>  				   buf->buffer.length);
>>>> +	r->stufftoread = false;
>>>>  error_ret:
>>>>  	return ret;
>>>>  }
>>>> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>>>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>>>>  	if (ret != r->bytes_per_datum)
>>>>  		return -EBUSY;
>>>> +	
>>>> +	r->stufftoread = true;
>>>> +	wake_up_interruptible(&r->pollq);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>
>>>> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
>>>> *r,
>>>>  	n = rounddown(n, r->bytes_per_datum);
>>>>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>>>>
>>>> +	if (kfifo_is_empty(&kf->kf))
>>>> +		r->stufftoread = false;
>>>> +	/* verify it is still empty to avoid race */
>>>> +	if (!kfifo_is_empty(&kf->kf))
>>>> +		r->stufftoread = true;
>>>> +
>>>>  	return copied;
>>>>  }
>>>>
>>>> .. is a totally untested patch to add poll support to it.
>>>> The little dance at the end is to avoid a write having occured between the
>>>> kfifo_is_empty and setting the flag false as that could wipe out the
>>>> existence of some data in the buffer. Given we only ever have one writer
>>>> and one reader that should work I think.
>>>>
>>>> Jonathan
>>>>
>>>>>
>>>>> Hi Ge,
>>>>>
>>>>> I realised after sending that message that I was being rather
>>>>> dismissive of your query.  Got up far too early this morning (as every
>>>>> morning ;)
>>>>>
>>>>> Anyhow, to give more details. sw_ring is probably never going to make
>>>>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>>>>> to work out how to do equivalent functionality of sw_ring but I've not
>>>>> had time to more than start looking into this.
>>>>>
>>>>> As you saw, poll on sw_ring is a watershead signal indicating (in
>>>>> theory and last I checked it worked) that the ring is more than half
>>>> full.
>>>>> Any read that takes the fill level below half (test code just reads
>>>>> half the size of the buffer), should allow a new passing of the
>>>>> watershead to resignal poll. It's entirely possible there is a bug in
>>>>> there though I know it is been getting a fair bit of testing with some
>>>>> other drivers so could be todo with the precise way you are reading it
>>>>> hitting some corner case? (I'm
>>>>> stretching...)
>>>>>
>>>>> Right now I'd just move over to kfifo_buf if I were you. It's much
>>>>> more 'standard' in that it's a fifo and poll indicates if there is
>>>>> anything there at all.
>>>>>>> 	Thanks.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Ge GAO
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 05/19/2012 11:43 AM, Lars-Peter Clausen wrote:
> On 05/19/2012 11:50 AM, Jonathan Cameron wrote:
>> On 05/19/2012 10:41 AM, Lars-Peter Clausen wrote:
>>> On 05/19/2012 11:16 AM, Jonathan Cameron wrote:
>>>> On 05/18/2012 07:26 PM, Ge Gao wrote:
>>>>> Thanks for the patch. There could be one problem with the implementation.
>>>>> Consider the situation:
>>>>> 1. there are 10 packets in the kfifo.
>>>>> 2. Read 1 packet. Then poll again.
>>>>> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
>>>>> needs the next store to wake it up, while in reality, it should return
>>>>> immediately if there is data already in the kfifo.
>>>> Agreed that is what should happen.  I'm just getting my head around why
>>>> it doesn't (or more specifically what is different about how we are
>>>> doing it from other similar cases).
>>>
>>> I'm not sure that this is what should happen. We most certainly want to have a
>>> threshold for poll. Otherwise we'd switch back and forth between userspace and
>>> kernel space for each sample individual sample captured. This will decrease the
>>> overall system performance (because of all the context switching). So userspace
>>> should be aware of that threshold and that poll might not wake the process up
>>> even if there are samples in the buffer. ALSA basically does the same.
>> When this originally came up, one suggestion was to use one of the other
>> types of poll for this (can't recall, but maybe POLLRDBAND)
>>
>> For Ge's case it actually makes sense to do it for every scan as the
>> scans can be very large and at relatively low frequency.
> 
> Well, for the sw_ring buffer the threshold was buffer_size/2, so if you'd set
> your buffer size to 2 you'd get woken up for each sample written.
True. But the chances of loosing stuff are extremely high.  Obviously
a variable watershed would have done the trick (and not been that hard
in that implementation).  Right now I have two kfifo targets.
1) Work out how to use the fixed length record form without defining a
datatype for the record.
2) Watersheads.

Do both of those and we have covered the main stuff sw_ring had going
for it.

> 
>>
>> What you are asking for is one of the primary things that we loose in
>> moving from ring_sw to kfifo.   I keep meaning to take time to look
>> at adding watersheads to the kfifo implementation. Right now we just
>> have to dead recon in userspace and read slightly faster than data is
>> being generated.  We had some bells and whistles in that sw_ring that
>> aren't anywhere else at the moment but we'll work on that going forward.
>>
> 
> I think it is better to get rid of that stufftoread flag and add a
> samples_available() callback. It'd serve a similar purpose, but instead of just
> telling you that something is available or not it will tell you the exact
> number of samples. Which then allows us to implement stuff like watersheads in
> the generic code.
I'm warey of doing this at this stage for a couple of reasons.

a) Some buffer types allow setting a watershead but do not give easy
access to the number of elements currently there.
b) Buffers also get used in forms where this stuff has no meaning.

So lets do it in an implementation first then think about moving it out
to the general code.
> 
>>
>> I don't think there is any harm in Ge's fix of adding a check before
>> poll_wait. Worst thing it does is tell userspace there is data when
>> there isn't.  Slight overhead due to the race, but trivial given we
>> can argue the read and the poll should be in the same thread anyway
>> in almost all cases.
> 
> 
> I suppose it makes sense, but I'm still a bit confused as to why it works for
> everybody else (all the other subsystems) without this extra check.
I think this is because convention is to read everything present if
using poll.  There are cases that do the equivalent of Ge's fix to
be found though.

Certainly can't see how it work as poll man page says it should for
evdev etc. (can't find any poll code evdev as it's buried in Xorg
somewhere not directly associated with evdev)

Jonathan

> 
>>
>> Jonathan
>>>
>>> - Lars
>>>
>>>>
>>>>
>>>>> 4. It might be better to put poll inside ring buffer implementation itself
>>>>> rather than industrial-buffer.c, such as providing a new method like poll
>>>>> to do it.
>>>> Certainly could do that, but I'm not sure how it will help.
>>>>
>>>>
>>>>
>>>>> What do you think?
>>>>> Thanks.
>>>>>
>>>>> Best Regards,
>>>>>
>>>>> GE GAO
>>>>>
>>>>>
>>>>>
>>>>> -----Original Message-----
>>>>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>>>>> Sent: Friday, May 18, 2012 10:31 AM
>>>>> To: Ge Gao
>>>>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
>>>>> Subject: Re: sw_ring.c poll problem
>>>>>
>>>>>
>>>>>
>>>>> On 05/18/2012 02:08 AM, Ge Gao wrote:
>>>>>> Dear Jonathan,
>>>>>> 	I check the kfifo buffer implementation under IIO directory.
>>>>> However,
>>>>>> I didn't find the "pollq" is released anywhere as sw_ring does.
>>>>>> Without it, how can kfifo be polled if it is used by
>>>>> industrial-buffer.c? Thanks.
>>>>> Sorry for not getting back to you sooner.  Totally manic at the day job
>>>>> today tracking two delightful bugs.
>>>>>
>>>>> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
>>>>> when it never got implemented.  Should be easy enough though.
>>>>>
>>>>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
>>>>> 6bf9d05..e69d6ce 100644
>>>>> --- a/drivers/iio/kfifo_buf.c
>>>>> +++ b/drivers/iio/kfifo_buf.c
>>>>> @@ -6,6 +6,7 @@
>>>>>  #include <linux/kfifo.h>
>>>>>  #include <linux/mutex.h>
>>>>>  #include <linux/iio/kfifo_buf.h>
>>>>> +#include <linux/sched.h>
>>>>>
>>>>>  struct iio_kfifo {
>>>>>  	struct iio_buffer buffer;
>>>>> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
>>>>> *r)
>>>>>  	kfifo_free(&buf->kf);
>>>>>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>>>>>  				   buf->buffer.length);
>>>>> +	r->stufftoread = false;
>>>>>  error_ret:
>>>>>  	return ret;
>>>>>  }
>>>>> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>>>>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>>>>>  	if (ret != r->bytes_per_datum)
>>>>>  		return -EBUSY;
>>>>> +	
>>>>> +	r->stufftoread = true;
>>>>> +	wake_up_interruptible(&r->pollq);
>>>>> +
>>>>>  	return 0;
>>>>>  }
>>>>>
>>>>> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
>>>>> *r,
>>>>>  	n = rounddown(n, r->bytes_per_datum);
>>>>>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>>>>>
>>>>> +	if (kfifo_is_empty(&kf->kf))
>>>>> +		r->stufftoread = false;
>>>>> +	/* verify it is still empty to avoid race */
>>>>> +	if (!kfifo_is_empty(&kf->kf))
>>>>> +		r->stufftoread = true;
>>>>> +
>>>>>  	return copied;
>>>>>  }
>>>>>
>>>>> .. is a totally untested patch to add poll support to it.
>>>>> The little dance at the end is to avoid a write having occured between the
>>>>> kfifo_is_empty and setting the flag false as that could wipe out the
>>>>> existence of some data in the buffer. Given we only ever have one writer
>>>>> and one reader that should work I think.
>>>>>
>>>>> Jonathan
>>>>>
>>>>>>
>>>>>> Hi Ge,
>>>>>>
>>>>>> I realised after sending that message that I was being rather
>>>>>> dismissive of your query.  Got up far too early this morning (as every
>>>>>> morning ;)
>>>>>>
>>>>>> Anyhow, to give more details. sw_ring is probably never going to make
>>>>>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>>>>>> to work out how to do equivalent functionality of sw_ring but I've not
>>>>>> had time to more than start looking into this.
>>>>>>
>>>>>> As you saw, poll on sw_ring is a watershead signal indicating (in
>>>>>> theory and last I checked it worked) that the ring is more than half
>>>>> full.
>>>>>> Any read that takes the fill level below half (test code just reads
>>>>>> half the size of the buffer), should allow a new passing of the
>>>>>> watershead to resignal poll. It's entirely possible there is a bug in
>>>>>> there though I know it is been getting a fair bit of testing with some
>>>>>> other drivers so could be todo with the precise way you are reading it
>>>>>> hitting some corner case? (I'm
>>>>>> stretching...)
>>>>>>
>>>>>> Right now I'd just move over to kfifo_buf if I were you. It's much
>>>>>> more 'standard' in that it's a fifo and poll indicates if there is
>>>>>> anything there at all.
>>>>>>>> 	Thanks.
>>>>>>>>
>>>>>>>> Best regards,
>>>>>>>>
>>>>>>>> Ge GAO
>>>>>>>> --
>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 

Re: sw_ring.c poll problem

[Jonathan Cameron]

On 05/19/2012 03:12 PM, Jonathan Cameron wrote:
> On 05/19/2012 11:43 AM, Lars-Peter Clausen wrote:
>> On 05/19/2012 11:50 AM, Jonathan Cameron wrote:
>>> On 05/19/2012 10:41 AM, Lars-Peter Clausen wrote:
>>>> On 05/19/2012 11:16 AM, Jonathan Cameron wrote:
>>>>> On 05/18/2012 07:26 PM, Ge Gao wrote:
>>>>>> Thanks for the patch. There could be one problem with the implementation.
>>>>>> Consider the situation:
>>>>>> 1. there are 10 packets in the kfifo.
>>>>>> 2. Read 1 packet. Then poll again.
>>>>>> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because it
>>>>>> needs the next store to wake it up, while in reality, it should return
>>>>>> immediately if there is data already in the kfifo.
>>>>> Agreed that is what should happen.  I'm just getting my head around why
>>>>> it doesn't (or more specifically what is different about how we are
>>>>> doing it from other similar cases).
>>>>
>>>> I'm not sure that this is what should happen. We most certainly want to have a
>>>> threshold for poll. Otherwise we'd switch back and forth between userspace and
>>>> kernel space for each sample individual sample captured. This will decrease the
>>>> overall system performance (because of all the context switching). So userspace
>>>> should be aware of that threshold and that poll might not wake the process up
>>>> even if there are samples in the buffer. ALSA basically does the same.
>>> When this originally came up, one suggestion was to use one of the other
>>> types of poll for this (can't recall, but maybe POLLRDBAND)
>>>
>>> For Ge's case it actually makes sense to do it for every scan as the
>>> scans can be very large and at relatively low frequency.
>>
>> Well, for the sw_ring buffer the threshold was buffer_size/2, so if you'd set
>> your buffer size to 2 you'd get woken up for each sample written.
> True. But the chances of loosing stuff are extremely high.  Obviously
> a variable watershed would have done the trick (and not been that hard
> in that implementation).  Right now I have two kfifo targets.
> 1) Work out how to use the fixed length record form without defining a
> datatype for the record.
Just for reference as we've ended up in this discussion here.  I'm
looking at this first element now and it 'looks' straight forward
if I use the __kfifo* functions directly.  I'll put a patch together
shortly as this should make for a fair improvement in efficiency.

> 2) Watersheads.
> 
> Do both of those and we have covered the main stuff sw_ring had going
> for it.
> 
>>
>>>
>>> What you are asking for is one of the primary things that we loose in
>>> moving from ring_sw to kfifo.   I keep meaning to take time to look
>>> at adding watersheads to the kfifo implementation. Right now we just
>>> have to dead recon in userspace and read slightly faster than data is
>>> being generated.  We had some bells and whistles in that sw_ring that
>>> aren't anywhere else at the moment but we'll work on that going forward.
>>>
>>
>> I think it is better to get rid of that stufftoread flag and add a
>> samples_available() callback. It'd serve a similar purpose, but instead of just
>> telling you that something is available or not it will tell you the exact
>> number of samples. Which then allows us to implement stuff like watersheads in
>> the generic code.
> I'm warey of doing this at this stage for a couple of reasons.
> 
> a) Some buffer types allow setting a watershead but do not give easy
> access to the number of elements currently there.
> b) Buffers also get used in forms where this stuff has no meaning.
> 
> So lets do it in an implementation first then think about moving it out
> to the general code.
>>
>>>
>>> I don't think there is any harm in Ge's fix of adding a check before
>>> poll_wait. Worst thing it does is tell userspace there is data when
>>> there isn't.  Slight overhead due to the race, but trivial given we
>>> can argue the read and the poll should be in the same thread anyway
>>> in almost all cases.
>>
>>
>> I suppose it makes sense, but I'm still a bit confused as to why it works for
>> everybody else (all the other subsystems) without this extra check.
> I think this is because convention is to read everything present if
> using poll.  There are cases that do the equivalent of Ge's fix to
> be found though.
> 
> Certainly can't see how it work as poll man page says it should for
> evdev etc. (can't find any poll code evdev as it's buried in Xorg
> somewhere not directly associated with evdev)
> 
> Jonathan
> 
>>
>>>
>>> Jonathan
>>>>
>>>> - Lars
>>>>
>>>>>
>>>>>
>>>>>> 4. It might be better to put poll inside ring buffer implementation itself
>>>>>> rather than industrial-buffer.c, such as providing a new method like poll
>>>>>> to do it.
>>>>> Certainly could do that, but I'm not sure how it will help.
>>>>>
>>>>>
>>>>>
>>>>>> What do you think?
>>>>>> Thanks.
>>>>>>
>>>>>> Best Regards,
>>>>>>
>>>>>> GE GAO
>>>>>>
>>>>>>
>>>>>>
>>>>>> -----Original Message-----
>>>>>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>>>>>> Sent: Friday, May 18, 2012 10:31 AM
>>>>>> To: Ge Gao
>>>>>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
>>>>>> Subject: Re: sw_ring.c poll problem
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 05/18/2012 02:08 AM, Ge Gao wrote:
>>>>>>> Dear Jonathan,
>>>>>>> 	I check the kfifo buffer implementation under IIO directory.
>>>>>> However,
>>>>>>> I didn't find the "pollq" is released anywhere as sw_ring does.
>>>>>>> Without it, how can kfifo be polled if it is used by
>>>>>> industrial-buffer.c? Thanks.
>>>>>> Sorry for not getting back to you sooner.  Totally manic at the day job
>>>>>> today tracking two delightful bugs.
>>>>>>
>>>>>> Anyhow, looks like I'd imagined we'd actually added poll support to kfifo
>>>>>> when it never got implemented.  Should be easy enough though.
>>>>>>
>>>>>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c index
>>>>>> 6bf9d05..e69d6ce 100644
>>>>>> --- a/drivers/iio/kfifo_buf.c
>>>>>> +++ b/drivers/iio/kfifo_buf.c
>>>>>> @@ -6,6 +6,7 @@
>>>>>>  #include <linux/kfifo.h>
>>>>>>  #include <linux/mutex.h>
>>>>>>  #include <linux/iio/kfifo_buf.h>
>>>>>> +#include <linux/sched.h>
>>>>>>
>>>>>>  struct iio_kfifo {
>>>>>>  	struct iio_buffer buffer;
>>>>>> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct iio_buffer
>>>>>> *r)
>>>>>>  	kfifo_free(&buf->kf);
>>>>>>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>>>>>>  				   buf->buffer.length);
>>>>>> +	r->stufftoread = false;
>>>>>>  error_ret:
>>>>>>  	return ret;
>>>>>>  }
>>>>>> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer *r,
>>>>>>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>>>>>>  	if (ret != r->bytes_per_datum)
>>>>>>  		return -EBUSY;
>>>>>> +	
>>>>>> +	r->stufftoread = true;
>>>>>> +	wake_up_interruptible(&r->pollq);
>>>>>> +
>>>>>>  	return 0;
>>>>>>  }
>>>>>>
>>>>>> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct iio_buffer
>>>>>> *r,
>>>>>>  	n = rounddown(n, r->bytes_per_datum);
>>>>>>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>>>>>>
>>>>>> +	if (kfifo_is_empty(&kf->kf))
>>>>>> +		r->stufftoread = false;
>>>>>> +	/* verify it is still empty to avoid race */
>>>>>> +	if (!kfifo_is_empty(&kf->kf))
>>>>>> +		r->stufftoread = true;
>>>>>> +
>>>>>>  	return copied;
>>>>>>  }
>>>>>>
>>>>>> .. is a totally untested patch to add poll support to it.
>>>>>> The little dance at the end is to avoid a write having occured between the
>>>>>> kfifo_is_empty and setting the flag false as that could wipe out the
>>>>>> existence of some data in the buffer. Given we only ever have one writer
>>>>>> and one reader that should work I think.
>>>>>>
>>>>>> Jonathan
>>>>>>
>>>>>>>
>>>>>>> Hi Ge,
>>>>>>>
>>>>>>> I realised after sending that message that I was being rather
>>>>>>> dismissive of your query.  Got up far too early this morning (as every
>>>>>>> morning ;)
>>>>>>>
>>>>>>> Anyhow, to give more details. sw_ring is probably never going to make
>>>>>>> it out of staging, hence the move to kfifo_buf. At somepoint we need
>>>>>>> to work out how to do equivalent functionality of sw_ring but I've not
>>>>>>> had time to more than start looking into this.
>>>>>>>
>>>>>>> As you saw, poll on sw_ring is a watershead signal indicating (in
>>>>>>> theory and last I checked it worked) that the ring is more than half
>>>>>> full.
>>>>>>> Any read that takes the fill level below half (test code just reads
>>>>>>> half the size of the buffer), should allow a new passing of the
>>>>>>> watershead to resignal poll. It's entirely possible there is a bug in
>>>>>>> there though I know it is been getting a fair bit of testing with some
>>>>>>> other drivers so could be todo with the precise way you are reading it
>>>>>>> hitting some corner case? (I'm
>>>>>>> stretching...)
>>>>>>>
>>>>>>> Right now I'd just move over to kfifo_buf if I were you. It's much
>>>>>>> more 'standard' in that it's a fifo and poll indicates if there is
>>>>>>> anything there at all.
>>>>>>>>> 	Thanks.
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>>
>>>>>>>>> Ge GAO
>>>>>>>>> --
>>>>>>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>>>>>>>>> in the body of a message to majordomo@vger.kernel.org More majordomo
>>>>>>>>> info at  http://vger.kernel.org/majordomo-info.html
>>>>>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-iio" in
>>>> the body of a message to majordomo@vger.kernel.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 

RE: sw_ring.c poll problem

[Ge Gao]

-----Original Message-----
From: Lars-Peter Clausen [mailto:lars@metafoo.de]
Sent: Saturday, May 19, 2012 3:43 AM
To: Jonathan Cameron
Cc: Ge Gao; Jonathan Cameron; linux-iio@vger.kernel.org
Subject: Re: sw_ring.c poll problem

On 05/19/2012 11:50 AM, Jonathan Cameron wrote:
> On 05/19/2012 10:41 AM, Lars-Peter Clausen wrote:
>> On 05/19/2012 11:16 AM, Jonathan Cameron wrote:
>>> On 05/18/2012 07:26 PM, Ge Gao wrote:
>>>> Thanks for the patch. There could be one problem with the
implementation.
>>>> Consider the situation:
>>>> 1. there are 10 packets in the kfifo.
>>>> 2. Read 1 packet. Then poll again.
>>>> 3. the poll will stuck at poll_wait(filp, &rb->pollq, wait) because
>>>> it needs the next store to wake it up, while in reality, it should
>>>> return immediately if there is data already in the kfifo.
>>> Agreed that is what should happen.  I'm just getting my head around
>>> why it doesn't (or more specifically what is different about how we
>>> are doing it from other similar cases).
>>
>> I'm not sure that this is what should happen. We most certainly want
>> to have a threshold for poll. Otherwise we'd switch back and forth
>> between userspace and kernel space for each sample individual sample
>> captured. This will decrease the overall system performance (because
>> of all the context switching). So userspace should be aware of that
>> threshold and that poll might not wake the process up even if there are
samples in the buffer. ALSA basically does the same.
> When this originally came up, one suggestion was to use one of the
> other types of poll for this (can't recall, but maybe POLLRDBAND)
>
> For Ge's case it actually makes sense to do it for every scan as the
> scans can be very large and at relatively low frequency.

Well, for the sw_ring buffer the threshold was buffer_size/2, so if you'd
set your buffer size to 2 you'd get woken up for each sample written.

>From GE: Clearly setting the sw_ring size to 2 won't buffer well. It will
easily lose when system load is heavy. A mechanism of both instancy and
buffering capability is required in some situations.

>
> What you are asking for is one of the primary things that we loose in
> moving from ring_sw to kfifo.   I keep meaning to take time to look
> at adding watersheads to the kfifo implementation. Right now we just
> have to dead recon in userspace and read slightly faster than data is
> being generated.  We had some bells and whistles in that sw_ring that
> aren't anywhere else at the moment but we'll work on that going forward.
>

I think it is better to get rid of that stufftoread flag and add a
samples_available() callback. It'd serve a similar purpose, but instead of
just telling you that something is available or not it will tell you the
exact number of samples. Which then allows us to implement stuff like
watersheads in the generic code.

>
> I don't think there is any harm in Ge's fix of adding a check before
> poll_wait. Worst thing it does is tell userspace there is data when
> there isn't.  Slight overhead due to the race, but trivial given we
> can argue the read and the poll should be in the same thread anyway in
> almost all cases.


I suppose it makes sense, but I'm still a bit confused as to why it works
for everybody else (all the other subsystems) without this extra check.

Frem GE: the sw_ring is working OK in my driver. However, it may have
problem in some situations, like I said earlier.

>
> Jonathan
>>
>> - Lars
>>
>>>
>>>
>>>> 4. It might be better to put poll inside ring buffer implementation
>>>> itself rather than industrial-buffer.c, such as providing a new
>>>> method like poll to do it.
>>> Certainly could do that, but I'm not sure how it will help.
>>>
>>>
>>>
>>>> What do you think?
>>>> Thanks.
>>>>
>>>> Best Regards,
>>>>
>>>> GE GAO
>>>>
>>>>
>>>>
>>>> -----Original Message-----
>>>> From: Jonathan Cameron [mailto:jic23@cam.ac.uk]
>>>> Sent: Friday, May 18, 2012 10:31 AM
>>>> To: Ge Gao
>>>> Cc: Jonathan Cameron; linux-iio@vger.kernel.org
>>>> Subject: Re: sw_ring.c poll problem
>>>>
>>>>
>>>>
>>>> On 05/18/2012 02:08 AM, Ge Gao wrote:
>>>>> Dear Jonathan,
>>>>> 	I check the kfifo buffer implementation under IIO directory.
>>>> However,
>>>>> I didn't find the "pollq" is released anywhere as sw_ring does.
>>>>> Without it, how can kfifo be polled if it is used by
>>>> industrial-buffer.c? Thanks.
>>>> Sorry for not getting back to you sooner.  Totally manic at the day
>>>> job today tracking two delightful bugs.
>>>>
>>>> Anyhow, looks like I'd imagined we'd actually added poll support to
>>>> kfifo when it never got implemented.  Should be easy enough though.
>>>>
>>>> diff --git a/drivers/iio/kfifo_buf.c b/drivers/iio/kfifo_buf.c
>>>> index 6bf9d05..e69d6ce 100644
>>>> --- a/drivers/iio/kfifo_buf.c
>>>> +++ b/drivers/iio/kfifo_buf.c
>>>> @@ -6,6 +6,7 @@
>>>>  #include <linux/kfifo.h>
>>>>  #include <linux/mutex.h>
>>>>  #include <linux/iio/kfifo_buf.h>
>>>> +#include <linux/sched.h>
>>>>
>>>>  struct iio_kfifo {
>>>>  	struct iio_buffer buffer;
>>>> @@ -35,6 +36,7 @@ static int iio_request_update_kfifo(struct
>>>> iio_buffer
>>>> *r)
>>>>  	kfifo_free(&buf->kf);
>>>>  	ret = __iio_allocate_kfifo(buf, buf->buffer.bytes_per_datum,
>>>>  				   buf->buffer.length);
>>>> +	r->stufftoread = false;
>>>>  error_ret:
>>>>  	return ret;
>>>>  }
>>>> @@ -97,6 +99,10 @@ static int iio_store_to_kfifo(struct iio_buffer
*r,
>>>>  	ret = kfifo_in(&kf->kf, data, r->bytes_per_datum);
>>>>  	if (ret != r->bytes_per_datum)
>>>>  		return -EBUSY;
>>>> +	
>>>> +	r->stufftoread = true;
>>>> +	wake_up_interruptible(&r->pollq);
>>>> +
>>>>  	return 0;
>>>>  }
>>>>
>>>> @@ -112,6 +118,12 @@ static int iio_read_first_n_kfifo(struct
>>>> iio_buffer *r,
>>>>  	n = rounddown(n, r->bytes_per_datum);
>>>>  	ret = kfifo_to_user(&kf->kf, buf, n, &copied);
>>>>
>>>> +	if (kfifo_is_empty(&kf->kf))
>>>> +		r->stufftoread = false;
>>>> +	/* verify it is still empty to avoid race */
>>>> +	if (!kfifo_is_empty(&kf->kf))
>>>> +		r->stufftoread = true;
>>>> +
>>>>  	return copied;
>>>>  }
>>>>
>>>> .. is a totally untested patch to add poll support to it.
>>>> The little dance at the end is to avoid a write having occured
>>>> between the kfifo_is_empty and setting the flag false as that could
>>>> wipe out the existence of some data in the buffer. Given we only
>>>> ever have one writer and one reader that should work I think.
>>>>
>>>> Jonathan
>>>>
>>>>>
>>>>> Hi Ge,
>>>>>
>>>>> I realised after sending that message that I was being rather
>>>>> dismissive of your query.  Got up far too early this morning (as
>>>>> every morning ;)
>>>>>
>>>>> Anyhow, to give more details. sw_ring is probably never going to
>>>>> make it out of staging, hence the move to kfifo_buf. At somepoint
>>>>> we need to work out how to do equivalent functionality of sw_ring
>>>>> but I've not had time to more than start looking into this.
>>>>>
>>>>> As you saw, poll on sw_ring is a watershead signal indicating (in
>>>>> theory and last I checked it worked) that the ring is more than
>>>>> half
>>>> full.
>>>>> Any read that takes the fill level below half (test code just
>>>>> reads half the size of the buffer), should allow a new passing of
>>>>> the watershead to resignal poll. It's entirely possible there is a
>>>>> bug in there though I know it is been getting a fair bit of
>>>>> testing with some other drivers so could be todo with the precise
>>>>> way you are reading it hitting some corner case? (I'm
>>>>> stretching...)
>>>>>
>>>>> Right now I'd just move over to kfifo_buf if I were you. It's much
>>>>> more 'standard' in that it's a fifo and poll indicates if there is
>>>>> anything there at all.
>>>>>>> 	Thanks.
>>>>>>>
>>>>>>> Best regards,
>>>>>>>
>>>>>>> Ge GAO
>>>>>>> --
>>>>>>> To unsubscribe from this list: send the line "unsubscribe
linux-iio"
>>>>>>> in the body of a message to majordomo@vger.kernel.org More
>>>>>>> majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-iio"
>> in the body of a message to majordomo@vger.kernel.org More majordomo
>> info at  http://vger.kernel.org/majordomo-info.html
>