LinuxLists.cc - [PATCH v1] counter: interrupt-cnt: add counter_push

2021-11-23 13:45:46

Subject: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

Add counter_push_event() to notify user space about new pulses

Signed-off-by: Oleksij Rempel <[email protected]>
---
drivers/counter/interrupt-cnt.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
index 8514a87fcbee..b237137b552b 100644
--- a/drivers/counter/interrupt-cnt.c
+++ b/drivers/counter/interrupt-cnt.c
@@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)

atomic_inc(&priv->count);

+ counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
+
return IRQ_HANDLED;
}

--
2.30.2

2021-11-24 06:09:15

On Wed, Nov 24, 2021 at 08:27:20AM +0100, Oleksij Rempel wrote:
> Hi William,
>
> On Wed, Nov 24, 2021 at 03:09:05PM +0900, William Breathitt Gray wrote:
> > On Tue, Nov 23, 2021 at 02:45:40PM +0100, Oleksij Rempel wrote:
> > > Add counter_push_event() to notify user space about new pulses
> > >
> > > Signed-off-by: Oleksij Rempel <[email protected]>
> > > ---
> > > drivers/counter/interrupt-cnt.c | 2 ++
> > > 1 file changed, 2 insertions(+)
> > >
> > > diff --git a/drivers/counter/interrupt-cnt.c b/drivers/counter/interrupt-cnt.c
> > > index 8514a87fcbee..b237137b552b 100644
> > > --- a/drivers/counter/interrupt-cnt.c
> > > +++ b/drivers/counter/interrupt-cnt.c
> > > @@ -31,6 +31,8 @@ static irqreturn_t interrupt_cnt_isr(int irq, void *dev_id)
> > >
> > > atomic_inc(&priv->count);
> > >
> > > + counter_push_event(&priv->counter, COUNTER_EVENT_OVERFLOW, 0);
> > > +
> > > return IRQ_HANDLED;
> > > }
> > >
> > > --
> > > 2.30.2
> >
> > Hi Oleksij,
> >
> > It looks like this is pushing a COUNTER_EVENT_OVERFLOW event every time
> > an interrupt is handled, which I suspect is not what you want to happen.
> > The COUNTER_EVENT_OVERFLOW event indicates a count value overflow event,
> > so you'll need to check for a count value overflow before pushing the
> > event.
> >
> > It would be good idea to implement a ceiling extension as well (you can
> > use the COUNTER_COMP_CEILING() macro) so that users can configure the
> > particular point where the value overflows.
>
> Thank you!
>
> What would be the best and resource effective strategy for periodically
> getting frequency of interrupts/pulses? This is actual information which is
> needed for my use case.
>
> So far, I was pushing every event to the user space, which is working
> but probably not the most resource effective method of doing it.
>
> Regards,
> Oleskij
> --
> Pengutronix e.K. | |
> Steuerwalder Str. 21 | http://www.pengutronix.de/ |
> 31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
> Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

We could introduce a new Counter change-of-state event type which would
trigger whenever the count value changes, but I agree with you that this
is likely not the best way for us to derive the frequency of the
interrupts due to the indirection of handling and parsing the event
data.

Instead, perhaps introducing a "frequency" or "period" Count extension
would make more sense here. This extension could report the value delta
between counts, or alternatively the time delta from which you can
derive frequency. Regarding implementation, you can store the previous
value in a variable, updating it whenever an interrupt occurs, and
compute the particular delta every time a read is requested by the user.

David Lechner is implementing something similar for the TI eQEP driver
to expose speed, so I'm CCing them here in case this is of interest to
them.

William Breathitt Gray

Attachments:

(No filename) (2.99 kB)
signature.asc (833.00 B)
Download all attachments

2021-11-25 07:37:56

by David Jander

[permalink] [raw]

Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

by David Lechner

[permalink] [raw]

Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

On 2/2/22 6:32 AM, Oleksij Rempel wrote:
> Hi William,
>
> On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
> ...
>> So the counter_push_event() function interacts with two spinlocks:
>> events_list_lock and events_in_lock. The events_list_lock spinlock is
>> necessary because userspace can modify the events_list list via the
>> counter_enable_events() and counter_disable_events() functions. The
>> events_in_lock spinlock is necessary because userspace can modify the
>> events kfifo via the counter_events_queue_size_write() function.
>>
>> A lockless solution for this might be possible if the driver maintains
>> its own circular buffer as you suggest. The driver's IRQ handler can
>> write to this circular buffer without calling the counter_push_event()
>> function, and then flush the buffer to the Counter character device via
>> a userspace write to a "flush_events" sysfs attribute or similar; this
>> eliminates the need for the events_in_lock spinlock. The state of the
>> events_list list can be captured in the driver's events_configure()
>> callback and stored locally in the driver for reference, thus
>> eliminating the need for the events_list_lock; interrupts can be
>> disabled before the driver's local copy of events_list is modified.
>>
>> With only one reader and one writer operating on the driver's buffer,
>> you can use the normal kfifo_in and kfifo_out calls for lockless
>> operations. Perhaps that is a way forward for this problem.
>
> As proof of concept, I implemented the double buffered version with the
> sysfs flush_events interface. Currently it feels kind of wired, I use
> poll and wait until it timeouts to run the sysfs_flush_counter() to
> trigger new data.
>
> Here is example:
> int main(void)
> {
> ret = sysfs_enable_counter();
> ...
>
> fd = open("/dev/counter0", O_RDWR);
> ...
>
> ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> ...
>
> ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> ...
>
> for (;;) {
> struct pollfd fds[] = {
> {
> .fd = fd,
> .events = POLLIN,
> },
> };
> ssize_t i;
>
> /* wait for 10 sec */
> ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
> if (ret == -EINTR)
> continue;
> else if (ret < 0)
> return -errno;
> else if (ret == 0) {
> sysfs_flush_counter(); <---- request to flush queued events from the driver
> continue;
> }
>
> ret = read(fd, event_data, sizeof(event_data));
> ...
>
> for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
> /* process event */
> ....
> }
> }
>
> return ret;
> }
>
> If it is still the only way to go, I'll send kernel patches.
>
> Regards,
> Oleksij
>

Couldn't the flush be implicit in the `read()` implementation
instead of requiring a separate sysfs attribute to trigger it?

2022-02-03 07:48:43

by Oleksij Rempel

[permalink] [raw]

Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

Hi William,

On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
...
> So the counter_push_event() function interacts with two spinlocks:
> events_list_lock and events_in_lock. The events_list_lock spinlock is
> necessary because userspace can modify the events_list list via the
> counter_enable_events() and counter_disable_events() functions. The
> events_in_lock spinlock is necessary because userspace can modify the
> events kfifo via the counter_events_queue_size_write() function.
>
> A lockless solution for this might be possible if the driver maintains
> its own circular buffer as you suggest. The driver's IRQ handler can
> write to this circular buffer without calling the counter_push_event()
> function, and then flush the buffer to the Counter character device via
> a userspace write to a "flush_events" sysfs attribute or similar; this
> eliminates the need for the events_in_lock spinlock. The state of the
> events_list list can be captured in the driver's events_configure()
> callback and stored locally in the driver for reference, thus
> eliminating the need for the events_list_lock; interrupts can be
> disabled before the driver's local copy of events_list is modified.
>
> With only one reader and one writer operating on the driver's buffer,
> you can use the normal kfifo_in and kfifo_out calls for lockless
> operations. Perhaps that is a way forward for this problem.

As proof of concept, I implemented the double buffered version with the
sysfs flush_events interface. Currently it feels kind of wired, I use
poll and wait until it timeouts to run the sysfs_flush_counter() to
trigger new data.

Here is example:
int main(void)
{
ret = sysfs_enable_counter();
...

fd = open("/dev/counter0", O_RDWR);
...

ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
...

ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
...

for (;;) {
struct pollfd fds[] = {
{
.fd = fd,
.events = POLLIN,
},
};
ssize_t i;

/* wait for 10 sec */
ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
if (ret == -EINTR)
continue;
else if (ret < 0)
return -errno;
else if (ret == 0) {
sysfs_flush_counter(); <---- request to flush queued events from the driver
continue;
}

ret = read(fd, event_data, sizeof(event_data));
...

for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
/* process event */
....
}
}

return ret;
}

If it is still the only way to go, I'll send kernel patches.

Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2022-02-03 14:29:06

by Oleksij Rempel

[permalink] [raw]

Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

On Wed, Feb 02, 2022 at 09:17:57AM -0600, David Lechner wrote:
> On 2/2/22 6:32 AM, Oleksij Rempel wrote:
> > Hi William,
> >
> > On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
> > ...
> > > So the counter_push_event() function interacts with two spinlocks:
> > > events_list_lock and events_in_lock. The events_list_lock spinlock is
> > > necessary because userspace can modify the events_list list via the
> > > counter_enable_events() and counter_disable_events() functions. The
> > > events_in_lock spinlock is necessary because userspace can modify the
> > > events kfifo via the counter_events_queue_size_write() function.
> > >
> > > A lockless solution for this might be possible if the driver maintains
> > > its own circular buffer as you suggest. The driver's IRQ handler can
> > > write to this circular buffer without calling the counter_push_event()
> > > function, and then flush the buffer to the Counter character device via
> > > a userspace write to a "flush_events" sysfs attribute or similar; this
> > > eliminates the need for the events_in_lock spinlock. The state of the
> > > events_list list can be captured in the driver's events_configure()
> > > callback and stored locally in the driver for reference, thus
> > > eliminating the need for the events_list_lock; interrupts can be
> > > disabled before the driver's local copy of events_list is modified.
> > >
> > > With only one reader and one writer operating on the driver's buffer,
> > > you can use the normal kfifo_in and kfifo_out calls for lockless
> > > operations. Perhaps that is a way forward for this problem.
> >
> > As proof of concept, I implemented the double buffered version with the
> > sysfs flush_events interface. Currently it feels kind of wired, I use
> > poll and wait until it timeouts to run the sysfs_flush_counter() to
> > trigger new data.
> >
> > Here is example:
> > int main(void)
> > {
> > ret = sysfs_enable_counter();
> > ...
> >
> > fd = open("/dev/counter0", O_RDWR);
> > ...
> >
> > ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> > ...
> >
> > ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> > ...
> >
> > for (;;) {
> > struct pollfd fds[] = {
> > {
> > .fd = fd,
> > .events = POLLIN,
> > },
> > };
> > ssize_t i;
> >
> > /* wait for 10 sec */
> > ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
> > if (ret == -EINTR)
> > continue;
> > else if (ret < 0)
> > return -errno;
> > else if (ret == 0) {
> > sysfs_flush_counter(); <---- request to flush queued events from the driver
> > continue;
> > }
> >
> > ret = read(fd, event_data, sizeof(event_data));
> > ...
> >
> > for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
> > /* process event */
> > ....
> > }
> > }
> >
> > return ret;
> > }
> >
> > If it is still the only way to go, I'll send kernel patches.
> >
> > Regards,
> > Oleksij
> >
>
> Couldn't the flush be implicit in the `read()` implementation
> instead of requiring a separate sysfs attribute to trigger it?

Hm...

To detect pulse frequency, I need a burst of sequential time-stamps
without drops. In case the pulse frequency is higher then the use space
is able to get it out of FIFO, we will get high number of drops.
So, we do not need all time stamps. Only bunch of them without drops in
the middle.

I know, at some frequency we wont be able to collect all pulses any way.
Internal FIFO is just increasing the max detectable frequency. So, it is
sort of optimization.

My current driver version has own FIFO which is filled directly by the
IRQ handler and user space trigger flush_cb to push all collected
time stamps. The main question is: how the flush procedure should be
controlled. We have following options:

- Attach it to the read(). The disadvantage: at high frequencies, we
wont be able to get a burst with time stamps without drops in the
middle
- Trigger flush from user space. In this case, we make user space a bit
more complicated and cant really get all advantages of poll().
- kernel driver is using own timer to trigger flush. The timer can be
configured from user space. The advantage of it, the user space is
simple and has full advantage of using poll()

Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |

2022-02-03 20:31:40

by William Breathitt Gray

[permalink] [raw]

Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

On Thu, Feb 03, 2022 at 08:24:11AM +0100, Oleksij Rempel wrote:
> On Wed, Feb 02, 2022 at 09:17:57AM -0600, David Lechner wrote:
> > On 2/2/22 6:32 AM, Oleksij Rempel wrote:
> > > Hi William,
> > >
> > > On Sat, Dec 25, 2021 at 01:07:44PM +0900, William Breathitt Gray wrote:
> > > ...
> > > > So the counter_push_event() function interacts with two spinlocks:
> > > > events_list_lock and events_in_lock. The events_list_lock spinlock is
> > > > necessary because userspace can modify the events_list list via the
> > > > counter_enable_events() and counter_disable_events() functions. The
> > > > events_in_lock spinlock is necessary because userspace can modify the
> > > > events kfifo via the counter_events_queue_size_write() function.
> > > >
> > > > A lockless solution for this might be possible if the driver maintains
> > > > its own circular buffer as you suggest. The driver's IRQ handler can
> > > > write to this circular buffer without calling the counter_push_event()
> > > > function, and then flush the buffer to the Counter character device via
> > > > a userspace write to a "flush_events" sysfs attribute or similar; this
> > > > eliminates the need for the events_in_lock spinlock. The state of the
> > > > events_list list can be captured in the driver's events_configure()
> > > > callback and stored locally in the driver for reference, thus
> > > > eliminating the need for the events_list_lock; interrupts can be
> > > > disabled before the driver's local copy of events_list is modified.
> > > >
> > > > With only one reader and one writer operating on the driver's buffer,
> > > > you can use the normal kfifo_in and kfifo_out calls for lockless
> > > > operations. Perhaps that is a way forward for this problem.
> > >
> > > As proof of concept, I implemented the double buffered version with the
> > > sysfs flush_events interface. Currently it feels kind of wired, I use
> > > poll and wait until it timeouts to run the sysfs_flush_counter() to
> > > trigger new data.
> > >
> > > Here is example:
> > > int main(void)
> > > {
> > > ret = sysfs_enable_counter();
> > > ...
> > >
> > > fd = open("/dev/counter0", O_RDWR);
> > > ...
> > >
> > > ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> > > ...
> > >
> > > ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> > > ...
> > >
> > > for (;;) {
> > > struct pollfd fds[] = {
> > > {
> > > .fd = fd,
> > > .events = POLLIN,
> > > },
> > > };
> > > ssize_t i;
> > >
> > > /* wait for 10 sec */
> > > ret = poll(fds, ARRAY_SIZE(fds), DEFAULT_TIMEOUT_MS);
> > > if (ret == -EINTR)
> > > continue;
> > > else if (ret < 0)
> > > return -errno;
> > > else if (ret == 0) {
> > > sysfs_flush_counter(); <---- request to flush queued events from the driver
> > > continue;
> > > }
> > >
> > > ret = read(fd, event_data, sizeof(event_data));
> > > ...
> > >
> > > for (i = 0; i < ret / (ssize_t)sizeof(event_data[0]); i++)
> > > /* process event */
> > > ....
> > > }
> > > }
> > >
> > > return ret;
> > > }
> > >
> > > If it is still the only way to go, I'll send kernel patches.
> > >
> > > Regards,
> > > Oleksij
> > >
> >
> > Couldn't the flush be implicit in the `read()` implementation
> > instead of requiring a separate sysfs attribute to trigger it?
>
> Hm...
>
> To detect pulse frequency, I need a burst of sequential time-stamps
> without drops. In case the pulse frequency is higher then the use space
> is able to get it out of FIFO, we will get high number of drops.
> So, we do not need all time stamps. Only bunch of them without drops in
> the middle.
>
> I know, at some frequency we wont be able to collect all pulses any way.
> Internal FIFO is just increasing the max detectable frequency. So, it is
> sort of optimization.
>
> My current driver version has own FIFO which is filled directly by the
> IRQ handler and user space trigger flush_cb to push all collected
> time stamps. The main question is: how the flush procedure should be
> controlled. We have following options:
>
> - Attach it to the read(). The disadvantage: at high frequencies, we
> wont be able to get a burst with time stamps without drops in the
> middle
> - Trigger flush from user space. In this case, we make user space a bit
> more complicated and cant really get all advantages of poll().
> - kernel driver is using own timer to trigger flush. The timer can be
> configured from user space. The advantage of it, the user space is
> simple and has full advantage of using poll()
>
> Regards,
> Oleksij

Hi Oleksij,

Earlier in this thread, Jonathan Cameron suggested using the RCU macros
to protect access to the events. Taking an RCU approach would eliminate
the need for spinlocks because the memory barriers are built-in to the
macros, so I assume flushing would no longer be necessary. Would RCU be
a viable solution for your needs?

William Breathitt Gray

Attachments:

(No filename) (4.95 kB)
signature.asc (849.00 B)
Download all attachments

2022-02-07 09:54:19

by Oleksij Rempel

[permalink] [raw]

Subject: Re: [PATCH v1] counter: interrupt-cnt: add counter_push_event()

Hi William,

On Thu, Feb 03, 2022 at 04:50:54PM +0900, William Breathitt Gray wrote:
> > Hm...
> >
> > To detect pulse frequency, I need a burst of sequential time-stamps
> > without drops. In case the pulse frequency is higher then the use space
> > is able to get it out of FIFO, we will get high number of drops.
> > So, we do not need all time stamps. Only bunch of them without drops in
> > the middle.
> >
> > I know, at some frequency we wont be able to collect all pulses any way.
> > Internal FIFO is just increasing the max detectable frequency. So, it is
> > sort of optimization.
> >
> > My current driver version has own FIFO which is filled directly by the
> > IRQ handler and user space trigger flush_cb to push all collected
> > time stamps. The main question is: how the flush procedure should be
> > controlled. We have following options:
> >
> > - Attach it to the read(). The disadvantage: at high frequencies, we
> > wont be able to get a burst with time stamps without drops in the
> > middle
> > - Trigger flush from user space. In this case, we make user space a bit
> > more complicated and cant really get all advantages of poll().
> > - kernel driver is using own timer to trigger flush. The timer can be
> > configured from user space. The advantage of it, the user space is
> > simple and has full advantage of using poll()
> >
> > Regards,
> > Oleksij
>
> Hi Oleksij,
>
> Earlier in this thread, Jonathan Cameron suggested using the RCU macros
> to protect access to the events. Taking an RCU approach would eliminate
> the need for spinlocks because the memory barriers are built-in to the
> macros, so I assume flushing would no longer be necessary. Would RCU be
> a viable solution for your needs?

IMO, RCU is the wrong word in this context. It provide an advantage
where we need to reuse/read less frequently changed data. In this use
case we need to move data ASAP, so KFIFO seems to work just fine here.

In any case, after implementi double FIFO and more testing I would
prefer to stay with my initial patch. On a single core system, with have
no waiting time at all. No concurrent access. And on the SMP system
(iMX6Q), currently I can measure higher frequency with initial not
optimized driver:
- with counter_push_event() directly from IRQ: max freq 30-35kHz
- with double FIFO, i have max freq of ~25kHz

Your suggestion was to add COUNTER_EVENT_CHANGE_OF_STATE and use it for
my use case. Correct?

Regards,
Oleksij
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |