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?
