Re: [PATCH v12 12/17] tools/counter: Create Counter tools

[William Breathitt Gray <vilhelm.gray@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 6018 bytes --]

On Sat, Jul 10, 2021 at 11:53:35AM -0500, David Lechner wrote:
> On 7/5/21 3:19 AM, William Breathitt Gray wrote:
> > This creates an example Counter program under tools/counter/*
> > to exemplify the Counter character device interface.
> > 
> > Cc: Pavel Machek <pavel@ucw.cz>
> > Signed-off-by: William Breathitt Gray <vilhelm.gray@gmail.com>
> > ---
> 
> 
> > --- a/tools/Makefile
> > +++ b/tools/Makefile
> > @@ -12,6 +12,7 @@ help:
> >   	@echo '  acpi                   - ACPI tools'
> >   	@echo '  bpf                    - misc BPF tools'
> >   	@echo '  cgroup                 - cgroup tools'
> > +	@echo '  counter                - Counter tools'
> 
> nit: other descriptions start with lower case letter, so to be
> consistent, this should too

Ack.

> > --- /dev/null
> > +++ b/tools/counter/counter_example.c
> > @@ -0,0 +1,95 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Counter - example userspace application
> > + *
> > + * The userspace application opens /dev/counter0, configures the
> > + * COUNTER_EVENT_INDEX event channel 0 to gather Count 0 count and Count
> > + * 1 count, and prints out the data as it becomes available on the
> > + * character device node.
> > + *
> > + * Copyright (C) 2021 William Breathitt Gray
> > + */
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <linux/counter.h>
> > +#include <stdio.h>
> > +#include <string.h>
> > +#include <sys/ioctl.h>
> > +#include <unistd.h>
> > +
> > +struct counter_watch watches[2] = {
> 
> nit: this can be static

Ack.

> > +	{
> > +		/* Component data: Count 0 count */
> > +		.component.type = COUNTER_COMPONENT_COUNT,
> > +		.component.scope = COUNTER_SCOPE_COUNT,
> > +		.component.parent = 0,
> > +		/* Event type: Index */
> > +		.event = COUNTER_EVENT_INDEX,
> > +		/* Device event channel 0 */
> > +		.channel = 0,
> > +	},
> > +	{
> > +		/* Component data: Count 1 count */
> > +		.component.type = COUNTER_COMPONENT_COUNT,
> > +		.component.scope = COUNTER_SCOPE_COUNT,
> > +		.component.parent = 1,
> > +		/* Event type: Index */
> > +		.event = COUNTER_EVENT_INDEX,
> > +		/* Device event channel 0 */
> > +		.channel = 0,
> > +	},
> > +};
> > +
> > +int main(void)
> > +{
> > +	int fd;
> > +	int ret;
> > +	struct counter_event event_data[2];
> > +
> > +	fd = open("/dev/counter0", O_RDWR);
> > +	if (fd == -1) {
> > +		perror("Unable to open /dev/counter0");
> > +		return -errno;
> 
> errno is no longer valid after calling perror(). Since this
> is example code, we can just return 1 instead (exit codes
> positive number between 0 and 255 so -1 would be 255).

Ack.

> > +	}
> > +
> > +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches);
> > +	if (ret == -1) {
> > +		perror("Error adding watches[0]");
> > +		return -errno;
> > +	}
> > +	ret = ioctl(fd, COUNTER_ADD_WATCH_IOCTL, watches + 1);
> > +	if (ret == -1) {
> > +		perror("Error adding watches[1]");
> > +		return -errno;
> > +	}
> > +	ret = ioctl(fd, COUNTER_ENABLE_EVENTS_IOCTL);
> > +	if (ret == -1) {
> > +		perror("Error enabling events");
> > +		return -errno;
> > +	}
> > +
> > +	for (;;) {
> > +		ret = read(fd, event_data, sizeof(event_data));
> > +		if (ret == -1) {
> > +			perror("Failed to read event data");
> > +			return -errno;
> > +		}
> > +
> > +		if (ret != sizeof(event_data)) {
> > +			fprintf(stderr, "Failed to read event data\n");
> > +			return -EIO;
> > +		}
> > +
> > +		printf("Timestamp 0: %llu\tCount 0: %llu\n"
> > +		       "Error Message 0: %s\n"
> > +		       "Timestamp 1: %llu\tCount 1: %llu\n"
> > +		       "Error Message 1: %s\n",
> > +		       (unsigned long long)event_data[0].timestamp,
> > +		       (unsigned long long)event_data[0].value,
> > +		       strerror(event_data[0].status),
> > +		       (unsigned long long)event_data[1].timestamp,
> > +		       (unsigned long long)event_data[1].value,
> > +		       strerror(event_data[1].status));
> > +	}
> 
> Aren't the Count 0 and Count 1 events independent? Why should we expect to
> always get both events at the same time in the same order?

Watch 0 and Watch 1 are both triggered by the same event: a
COUNTER_EVENT_INDEX event on device event channel 0. If we had set
channel to 1 for Watch 1, then we would have two independent events, but
in this case both Watches have their respective channel set to 0.

To make the sequence of events clearer, here's a timeline:

* The user configures the watch list via COUNTER_ADD_WATCH_IOCTL.

* The watch list consists of Watch 0 and Watch 1. Watch 0 is configured
  to report the Count 0 count, while Watch 1 is configured to report the
  Count 1 count. Both watches are configured to trigger on the same
  event (COUNTER_EVENT_INDEX on device event channel 0).

* The user enables Counter events via COUNTER_ENABLE_EVENTS_IOCTL.

* The user calls read() from userspace and blocks until data is
  available in the Counter events list kfifo; this corresponds to
  wait_event_interruptible() in counter_chrdev_read().

* A COUNTER_EVENT_INDEX event occurs on device event channel 0.

* All Watches in the watch list that are waiting for COUNTER_EVENT_INDEX
  on device event channel 0 will now trigger; both Watch 0 and Watch 1
  will trigger, one after the other.

* A read operation is performed for the Count 0 count component; the
  data is pushed to the Counter event list.

* A read operation is performed for the Count 1 count component; the
  data is pushed to the Counter event list.

* Counter subsystem notifies that data is available in the Counter
  events list kfifo; this corresponds to the wake_up_poll() in
  counter_push_event().

* The userspace read() call returns the Counter event list data.

So in the counter_example.c reference code, we will always get both
event data elements returned to the user at the same time (with the
exception of errors which break early).

William Breathitt Gray

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]