On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
> This adds a new counter tool to be able to test various watch events.
> A flexible watch array can be populated from command line, each field
> may be tuned with a dedicated command line argument.
> Each argument can be repeated several times: each time it gets repeated,
> a corresponding new watch element is allocated.
>
> It also comes with a simple default watch (to monitor overflows), used
> when no watch parameters are provided.
>
> The print_usage() routine proposes another example, from the command line,
> which generates a 2 elements watch array, to monitor:
> - overflow events
> - capture events, on channel 3, that reads read captured data by
> specifying the component id (capture3_component_id being 7 here).
>
> Signed-off-by: Fabrice Gasnier <[email protected]>
Hi Fabrice,
This is great idea, it'll make it so much easier to test out drivers
so I'm excited! :-)
This is a new tool, so would you add a MAINTAINERS entry for the
counter_watch_events.c file?
More comments inline below.
> ---
> tools/counter/Build | 1 +
> tools/counter/Makefile | 8 +-
> tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
> 3 files changed, 356 insertions(+), 1 deletion(-)
> create mode 100644 tools/counter/counter_watch_events.c
>
> diff --git a/tools/counter/Build b/tools/counter/Build
> index 33f4a51d715e..4bbadb7ec93a 100644
> --- a/tools/counter/Build
> +++ b/tools/counter/Build
> @@ -1 +1,2 @@
> counter_example-y += counter_example.o
> +counter_watch_events-y += counter_watch_events.o
> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
> index b2c2946f44c9..00e211edd768 100644
> --- a/tools/counter/Makefile
> +++ b/tools/counter/Makefile
> @@ -14,7 +14,7 @@ MAKEFLAGS += -r
>
> override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>
> -ALL_TARGETS := counter_example
> +ALL_TARGETS := counter_example counter_watch_events
> ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>
> all: $(ALL_PROGRAMS)
> @@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
>
> prepare: $(OUTPUT)include/linux/counter.h
>
> +COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
> +$(COUNTER_WATCH_EVENTS): prepare FORCE
> + $(Q)$(MAKE) $(build)=counter_watch_events
> +$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
> + $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
> +
Move this below the COUNTER_EXAMPLE block just so we can keep the
recipes in alphabetical order.
> COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
> $(COUNTER_EXAMPLE): prepare FORCE
> $(Q)$(MAKE) $(build)=counter_example
> diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
> new file mode 100644
> index 000000000000..7f73a1519d8e
> --- /dev/null
> +++ b/tools/counter/counter_watch_events.c
> @@ -0,0 +1,348 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Counter - Test various watch events in a userspace application
"Counter" should be "Counter Watch Events" (or "counter_watch_events").
> + * inspired by counter_example.c
No need to mention counter_example.c, this utility does far more than
and bares little resemblance at this point to counter_example.c which is
really just a bare minimal example of watching Counter events.
> + */
> +
> +#include <errno.h>
> +#include <fcntl.h>
> +#include <getopt.h>
> +#include <linux/counter.h>
> +#include <stdlib.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <unistd.h>
> +
> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
My initial reaction was that this macro is already exposed in some
header for us, but my local /usr/include/linux/kernel.h file doesn't
appear to bare it so I guess not. Perhaps it'll be fine for our needs --
I think the only difference between this ARRAY_SIZE and the Linux kernel
one is the addition of __must_be_array(x).
> +
> +static struct counter_watch simple_watch[] = {
> + {
> + /* Component data: Count 0 count */
> + .component.type = COUNTER_COMPONENT_COUNT,
> + .component.scope = COUNTER_SCOPE_COUNT,
> + .component.parent = 0,
> + /* Event type: Index */
> + .event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
There's a bit of confusion here. The comment says the event type is
INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also,
the commit description states that we're monitoring "overflows" which
implies to me the OVERFLOW event type. So which of the three is it?
> + /* Device event channel 0 */
> + .channel = 0,
> + },
> +};
> +
> +static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
> +{
> + unsigned int i;
> + char *dummy;
> + unsigned long idx;
> + int ret;
> +
> + for (i = 0; i < sz; i++) {
> + ret = strncmp(arg, str[i], strlen(str[i]));
> + if (!ret && strlen(str[i]) == strlen(arg)) {
> + *val = i;
> + return 0;
> + }
This has several strlen calls so I wonder if it's more wasteful than it
needs to me. I suppose the compiler would optimize this away, but I
think there is an alternative solution.
We're checking for an exact match, so you don't need the string length.
Instead, you can compare each character, break when characters differ,
or return 0 when you reached the null byte for both. So something like
this:
for (j = 0; arg[j] == str[i][j]; j++) {
/* If we reached the end of the strings */
if (arg[j] == '\0') {
*val = i;
return 0;
}
}
/* Strings do not match; continue to the next string */
We end up with the same number of lines, so I'll leave it up to you
whether you want to use this solution, or if you consider the existing
code clearer read.
> + }
> +
> + /* fallback to number */
I'm not sure it makes sense to support numbers. Although it's true that
the component type, component scope, and event type are passed as __u8
values, users are expected to treat those values are opaque and pass
them via the respective enum constants. Since we don't guarantee that
the specific enum constant values will remain consistent between kernel
versions, I don't think it's a good idea to give to users that sort of
implication by allowing them to use raw numbers for configuration
selection.
> + idx = strtoul(optarg, &dummy, 10);
> + if (!errno) {
> + if (idx >= sz)
> + return -EINVAL;
> + *val = idx;
> + return 0;
> + }
> +
> + return -errno;
> +}
> +
> +static const char * const counter_event_type_name[] = {
> + "COUNTER_EVENT_OVERFLOW",
> + "COUNTER_EVENT_UNDERFLOW",
> + "COUNTER_EVENT_OVERFLOW_UNDERFLOW",
> + "COUNTER_EVENT_THRESHOLD",
> + "COUNTER_EVENT_INDEX",
> + "COUNTER_EVENT_CHANGE_OF_STATE",
> + "COUNTER_EVENT_CAPTURE",
> +};
> +
> +static int counter_arg_to_event_type(char *arg, __u8 *event)
> +{
> + return find_match_or_number_from_array(arg, counter_event_type_name,
> + ARRAY_SIZE(counter_event_type_name), event);
> +}
> +
> +static const char * const counter_component_type_name[] = {
> + "COUNTER_COMPONENT_NONE",
> + "COUNTER_COMPONENT_SIGNAL",
> + "COUNTER_COMPONENT_COUNT",
> + "COUNTER_COMPONENT_FUNCTION",
> + "COUNTER_COMPONENT_SYNAPSE_ACTION",
> + "COUNTER_COMPONENT_EXTENSION",
> +};
> +
> +static int counter_arg_to_component_type(char *arg, __u8 *type)
> +{
> + return find_match_or_number_from_array(arg, counter_component_type_name,
> + ARRAY_SIZE(counter_component_type_name), type);
> +}
> +
> +static const char * const counter_scope_name[] = {
> + "COUNTER_SCOPE_DEVICE",
> + "COUNTER_SCOPE_SIGNAL",
> + "COUNTER_SCOPE_COUNT",
> +};
> +
> +static int counter_arg_to_scope(char *arg, __u8 *type)
> +{
> + return find_match_or_number_from_array(arg, counter_scope_name,
> + ARRAY_SIZE(counter_scope_name), type);
> +}
> +
> +static void print_usage(void)
> +{
> + fprintf(stderr, "Usage: counter_watch_events [options]...\n"
> + "Test various watch events for given counter device\n"
> + " --channel -c <n>\n"
> + " Set watch.channel\n"
> + " --debug -d\n"
> + " Prints debug information\n"
> + " --event -e <number or counter_event_type string>\n"
> + " Sets watch.event\n"
> + " --help -h\n"
> + " Prints usage\n"
> + " --device-num -n <n>\n"
> + " Set device number (/dev/counter<n>, default to 0)\n"
> + " --id -i <n>\n"
> + " Set watch.component.id\n"
> + " --loop -l <n>\n"
> + " Loop for a number of events (forever if n < 0)\n"
> + " --parent -p <n>\n"
> + " Set watch.component.parent number\n"
> + " --scope -s <number or counter_scope string>\n"
> + " Set watch.component.scope\n"
> + " --type -t <number or counter_component_type string>\n"
> + " Set watch.component.type\n"
> + "\n"
> + "Example with two watched events:\n\n"
> + "counter_watch_events -d \\\n"
> + "\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
> + " -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
> + "\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
> + " -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
> + );
> +}
Are you following any particular convention for the usage description? I
wonder if there is a particular preferred standard for command-line
interface descriptions. A quick search brought up a few, such as the
POSIX Utility Conventions[^1] and docopt[^2].
One improvement I would recommend here is to put the short form of the
option before the long form and separate them with a command to make it
clearer (e.g. "-h, --help").
[^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
[^2] http://docopt.org
> +
> +static void print_watch(struct counter_watch *watch, int nwatch)
> +{
> + int i;
> +
> + /* prints the watch array in C-like structure */
> + printf("watch[%d] = {\n", nwatch);
> + for (i = 0; i < nwatch; i++) {
> + printf(" [%d] =\t{\n"
> + "\t\t.component.type = %s\n"
> + "\t\t.component.scope = %s\n"
> + "\t\t.component.parent = %d\n"
> + "\t\t.component.id = %d\n"
> + "\t\t.event = %s\n"
> + "\t\t.channel = %d\n"
> + "\t},\n",
> + i,
> + counter_component_type_name[watch[i].component.type],
> + counter_scope_name[watch[i].component.scope],
> + watch[i].component.parent,
> + watch[i].component.id,
> + counter_event_type_name[watch[i].event],
> + watch[i].channel);
> + }
> + printf("};\n");
> +}
> +
> +static const struct option longopts[] = {
> + { "channel", required_argument, 0, 'c' },
> + { "debug", no_argument, 0, 'd' },
> + { "event", required_argument, 0, 'e' },
> + { "help", no_argument, 0, 'h' },
> + { "device-num", required_argument, 0, 'n' },
> + { "id", required_argument, 0, 'i' },
> + { "loop", required_argument, 0, 'l' },
> + { "parent", required_argument, 0, 'p' },
> + { "scope", required_argument, 0, 's' },
> + { "type", required_argument, 0, 't' },
> + { },
> +};
> +
> +int main(int argc, char **argv)
> +{
> + int c, fd, i, ret;
> + struct counter_event event_data;
> + char *device_name = NULL;
> + int debug = 0, loop = -1;
> + char *dummy;
> + int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
> + int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
> + struct counter_watch *watches;
> +
> + /*
> + * 1st pass: count events configurations number to allocate the watch array.
> + * Each watch argument can be repeated several times: each time it gets repeated,
> + * a corresponding watch is allocated (and configured) in 2nd pass.
> + */
It feels a somewhat prone to error (at least cumbersome) to populate
each watch via individual arguments for each field. Since a watch always
has these fields, perhaps instead we could pass some format string that
represents a watch, and deliminate watches via commas. For example, we
could have --watch="cco00,ecc73" to represent the two watches in the
usage example.
Of course, we'd need to define a more robust format string convention
than in my example to ensure the correct configuration is properly
communicated. What do you think, would this approach would make things
simpler, or just more complicated in the end?
> + while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
> + switch (c) {
> + case 'c':
> + ncfg[0]++;
> + break;
> + case 'e':
> + ncfg[1]++;
> + break;
> + case 'i':
> + ncfg[2]++;
> + break;
> + case 'p':
> + ncfg[3]++;
> + break;
> + case 's':
> + ncfg[4]++;
> + break;
> + case 't':
> + ncfg[5]++;
> + break;
> + };
> + };
> +
> + for (i = 0; i < ARRAY_SIZE(ncfg); i++)
> + if (ncfg[i] > nwatch)
> + nwatch = ncfg[i];
> +
> + if (nwatch) {
> + watches = calloc(nwatch, sizeof(*watches));
We need to check if calloc fails, right?
William Breathitt Gray
On 9/17/23 21:07, William Breathitt Gray wrote:
> On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
>> This adds a new counter tool to be able to test various watch events.
>> A flexible watch array can be populated from command line, each field
>> may be tuned with a dedicated command line argument.
>> Each argument can be repeated several times: each time it gets repeated,
>> a corresponding new watch element is allocated.
>>
>> It also comes with a simple default watch (to monitor overflows), used
>> when no watch parameters are provided.
>>
>> The print_usage() routine proposes another example, from the command line,
>> which generates a 2 elements watch array, to monitor:
>> - overflow events
>> - capture events, on channel 3, that reads read captured data by
>> specifying the component id (capture3_component_id being 7 here).
>>
>> Signed-off-by: Fabrice Gasnier <[email protected]>
>
> Hi Fabrice,
>
> This is great idea, it'll make it so much easier to test out drivers
> so I'm excited! :-)
Hi William,
Thanks
>
> This is a new tool, so would you add a MAINTAINERS entry for the
> counter_watch_events.c file?
I haven't thought about it.
I can add a MAINTAINERS entry, yes!
Who would you suggest ?
>
> More comments inline below.
>
>> ---
>> tools/counter/Build | 1 +
>> tools/counter/Makefile | 8 +-
>> tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
>> 3 files changed, 356 insertions(+), 1 deletion(-)
>> create mode 100644 tools/counter/counter_watch_events.c
>>
>> diff --git a/tools/counter/Build b/tools/counter/Build
>> index 33f4a51d715e..4bbadb7ec93a 100644
>> --- a/tools/counter/Build
>> +++ b/tools/counter/Build
>> @@ -1 +1,2 @@
>> counter_example-y += counter_example.o
>> +counter_watch_events-y += counter_watch_events.o
>> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
>> index b2c2946f44c9..00e211edd768 100644
>> --- a/tools/counter/Makefile
>> +++ b/tools/counter/Makefile
>> @@ -14,7 +14,7 @@ MAKEFLAGS += -r
>>
>> override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>>
>> -ALL_TARGETS := counter_example
>> +ALL_TARGETS := counter_example counter_watch_events
>> ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>>
>> all: $(ALL_PROGRAMS)
>> @@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
>>
>> prepare: $(OUTPUT)include/linux/counter.h
>>
>> +COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
>> +$(COUNTER_WATCH_EVENTS): prepare FORCE
>> + $(Q)$(MAKE) $(build)=counter_watch_events
>> +$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
>> + $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
>> +
>
> Move this below the COUNTER_EXAMPLE block just so we can keep the
> recipes in alphabetical order.
Ack, will update it.
>
>> COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
>> $(COUNTER_EXAMPLE): prepare FORCE
>> $(Q)$(MAKE) $(build)=counter_example
>> diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
>> new file mode 100644
>> index 000000000000..7f73a1519d8e
>> --- /dev/null
>> +++ b/tools/counter/counter_watch_events.c
>> @@ -0,0 +1,348 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/* Counter - Test various watch events in a userspace application
>
> "Counter" should be "Counter Watch Events" (or "counter_watch_events").
>
>> + * inspired by counter_example.c
>
> No need to mention counter_example.c, this utility does far more than
> and bares little resemblance at this point to counter_example.c which is
> really just a bare minimal example of watching Counter events.
Ack
>
>> + */
>> +
>> +#include <errno.h>
>> +#include <fcntl.h>
>> +#include <getopt.h>
>> +#include <linux/counter.h>
>> +#include <stdlib.h>
>> +#include <stdio.h>
>> +#include <string.h>
>> +#include <sys/ioctl.h>
>> +#include <unistd.h>
>> +
>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>
> My initial reaction was that this macro is already exposed in some
> header for us, but my local /usr/include/linux/kernel.h file doesn't
> appear to bare it so I guess not. Perhaps it'll be fine for our needs --
> I think the only difference between this ARRAY_SIZE and the Linux kernel
> one is the addition of __must_be_array(x).
I had the same reaction when trying to use it. Then, I figured out
several tools redefine this macro.
Digging further, I just found out some tools have in their Makefile CFLAGS:
-I$(srctree)/tools/include
and include from there: <linux/kernel.h>
I'll update the Makefile in v2, and remove this definition from here.
>
>> +
>> +static struct counter_watch simple_watch[] = {
>> + {
>> + /* Component data: Count 0 count */
>> + .component.type = COUNTER_COMPONENT_COUNT,
>> + .component.scope = COUNTER_SCOPE_COUNT,
>> + .component.parent = 0,
>> + /* Event type: Index */
>> + .event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
>
> There's a bit of confusion here. The comment says the event type is
> INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also,
> the commit description states that we're monitoring "overflows" which
> implies to me the OVERFLOW event type. So which of the three is it?
Ah yes, It's a mix of bad copy paste and updates. I'll fix it.
>
>> + /* Device event channel 0 */
>> + .channel = 0,
>> + },
>> +};
>> +
>> +static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
>> +{
>> + unsigned int i;
>> + char *dummy;
>> + unsigned long idx;
>> + int ret;
>> +
>> + for (i = 0; i < sz; i++) {
>> + ret = strncmp(arg, str[i], strlen(str[i]));
>> + if (!ret && strlen(str[i]) == strlen(arg)) {
>> + *val = i;
>> + return 0;
>> + }
>
> This has several strlen calls so I wonder if it's more wasteful than it
> needs to me. I suppose the compiler would optimize this away, but I
> think there is an alternative solution.
>
> We're checking for an exact match, so you don't need the string length.
> Instead, you can compare each character, break when characters differ,
> or return 0 when you reached the null byte for both. So something like
> this:
>
> for (j = 0; arg[j] == str[i][j]; j++) {
> /* If we reached the end of the strings */
> if (arg[j] == '\0') {
> *val = i;
> return 0;
> }
> }
> /* Strings do not match; continue to the next string */
>
> We end up with the same number of lines, so I'll leave it up to you
> whether you want to use this solution, or if you consider the existing
> code clearer read.
I'll look forward in the direction you propose. First, we need to
confirm in which form the arguments can be expected. It depends on your
proposal to use a --watch string formatted arguments.
>
>> + }
>> +
>> + /* fallback to number */
>
> I'm not sure it makes sense to support numbers. Although it's true that
> the component type, component scope, and event type are passed as __u8
> values, users are expected to treat those values are opaque and pass
> them via the respective enum constants. Since we don't guarantee that
> the specific enum constant values will remain consistent between kernel
> versions, I don't think it's a good idea to give to users that sort of
> implication by allowing them to use raw numbers for configuration
> selection.
Ack, I can remove this.
I'm a bit surprised by this statement. I may be wrong... I'd expect a
userland binary to be compatible when updating to a newer kernel: e.g.
user API (ABI?) definitions to be stable (including enum constants) ?
>
>> + idx = strtoul(optarg, &dummy, 10);
>> + if (!errno) {
>> + if (idx >= sz)
>> + return -EINVAL;
>> + *val = idx;
>> + return 0;
>> + }
>> +
>> + return -errno;
>> +}
>> +
>> +static const char * const counter_event_type_name[] = {
>> + "COUNTER_EVENT_OVERFLOW",
>> + "COUNTER_EVENT_UNDERFLOW",
>> + "COUNTER_EVENT_OVERFLOW_UNDERFLOW",
>> + "COUNTER_EVENT_THRESHOLD",
>> + "COUNTER_EVENT_INDEX",
>> + "COUNTER_EVENT_CHANGE_OF_STATE",
>> + "COUNTER_EVENT_CAPTURE",
>> +};
>> +
>> +static int counter_arg_to_event_type(char *arg, __u8 *event)
>> +{
>> + return find_match_or_number_from_array(arg, counter_event_type_name,
>> + ARRAY_SIZE(counter_event_type_name), event);
>> +}
>> +
>> +static const char * const counter_component_type_name[] = {
>> + "COUNTER_COMPONENT_NONE",
>> + "COUNTER_COMPONENT_SIGNAL",
>> + "COUNTER_COMPONENT_COUNT",
>> + "COUNTER_COMPONENT_FUNCTION",
>> + "COUNTER_COMPONENT_SYNAPSE_ACTION",
>> + "COUNTER_COMPONENT_EXTENSION",
>> +};
>> +
>> +static int counter_arg_to_component_type(char *arg, __u8 *type)
>> +{
>> + return find_match_or_number_from_array(arg, counter_component_type_name,
>> + ARRAY_SIZE(counter_component_type_name), type);
>> +}
>> +
>> +static const char * const counter_scope_name[] = {
>> + "COUNTER_SCOPE_DEVICE",
>> + "COUNTER_SCOPE_SIGNAL",
>> + "COUNTER_SCOPE_COUNT",
>> +};
>> +
>> +static int counter_arg_to_scope(char *arg, __u8 *type)
>> +{
>> + return find_match_or_number_from_array(arg, counter_scope_name,
>> + ARRAY_SIZE(counter_scope_name), type);
>> +}
>> +
>> +static void print_usage(void)
>> +{
>> + fprintf(stderr, "Usage: counter_watch_events [options]...\n"
>> + "Test various watch events for given counter device\n"
>> + " --channel -c <n>\n"
>> + " Set watch.channel\n"
>> + " --debug -d\n"
>> + " Prints debug information\n"
>> + " --event -e <number or counter_event_type string>\n"
>> + " Sets watch.event\n"
>> + " --help -h\n"
>> + " Prints usage\n"
>> + " --device-num -n <n>\n"
>> + " Set device number (/dev/counter<n>, default to 0)\n"
>> + " --id -i <n>\n"
>> + " Set watch.component.id\n"
>> + " --loop -l <n>\n"
>> + " Loop for a number of events (forever if n < 0)\n"
>> + " --parent -p <n>\n"
>> + " Set watch.component.parent number\n"
>> + " --scope -s <number or counter_scope string>\n"
>> + " Set watch.component.scope\n"
>> + " --type -t <number or counter_component_type string>\n"
>> + " Set watch.component.type\n"
>> + "\n"
>> + "Example with two watched events:\n\n"
>> + "counter_watch_events -d \\\n"
>> + "\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
>> + " -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
>> + "\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
>> + " -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
>> + );
>> +}
>
> Are you following any particular convention for the usage description? I
> wonder if there is a particular preferred standard for command-line
> interface descriptions. A quick search brought up a few, such as the
> POSIX Utility Conventions[^1] and docopt[^2].
>
> One improvement I would recommend here is to put the short form of the
> option before the long form and separate them with a command to make it
> clearer (e.g. "-h, --help").
>
> [^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
> [^2] http://docopt.org
Thanks for pointing this! So definitely a good pointer, and suggestion
to look at!
I'll try to improve in v2.
>
>> +
>> +static void print_watch(struct counter_watch *watch, int nwatch)
>> +{
>> + int i;
>> +
>> + /* prints the watch array in C-like structure */
>> + printf("watch[%d] = {\n", nwatch);
>> + for (i = 0; i < nwatch; i++) {
>> + printf(" [%d] =\t{\n"
>> + "\t\t.component.type = %s\n"
>> + "\t\t.component.scope = %s\n"
>> + "\t\t.component.parent = %d\n"
>> + "\t\t.component.id = %d\n"
>> + "\t\t.event = %s\n"
>> + "\t\t.channel = %d\n"
>> + "\t},\n",
>> + i,
>> + counter_component_type_name[watch[i].component.type],
>> + counter_scope_name[watch[i].component.scope],
>> + watch[i].component.parent,
>> + watch[i].component.id,
>> + counter_event_type_name[watch[i].event],
>> + watch[i].channel);
>> + }
>> + printf("};\n");
>> +}
>> +
>> +static const struct option longopts[] = {
>> + { "channel", required_argument, 0, 'c' },
>> + { "debug", no_argument, 0, 'd' },
>> + { "event", required_argument, 0, 'e' },
>> + { "help", no_argument, 0, 'h' },
>> + { "device-num", required_argument, 0, 'n' },
>> + { "id", required_argument, 0, 'i' },
>> + { "loop", required_argument, 0, 'l' },
>> + { "parent", required_argument, 0, 'p' },
>> + { "scope", required_argument, 0, 's' },
>> + { "type", required_argument, 0, 't' },
>> + { },
>> +};
>> +
>> +int main(int argc, char **argv)
>> +{
>> + int c, fd, i, ret;
>> + struct counter_event event_data;
>> + char *device_name = NULL;
>> + int debug = 0, loop = -1;
>> + char *dummy;
>> + int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
>> + int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
>> + struct counter_watch *watches;
>> +
>> + /*
>> + * 1st pass: count events configurations number to allocate the watch array.
>> + * Each watch argument can be repeated several times: each time it gets repeated,
>> + * a corresponding watch is allocated (and configured) in 2nd pass.
>> + */
>
> It feels a somewhat prone to error (at least cumbersome) to populate
Yes, this could be error prone. This is also why I added a print of the
gathered arguments when using --debug option.
Perhaps this could be better to always print it (e.g. print_watch()) ?
> each watch via individual arguments for each field. Since a watch always
> has these fields, perhaps instead we could pass some format string that
> represents a watch, and deliminate watches via commas. For example, we
> could have --watch="cco00,ecc73" to represent the two watches in the
> usage example.
I like the idea, to concatenate as a string. With current approach, the
command line quickly becomes very long.
It makes it obvious in your example, that two watches are used, and no
argument is omitted.
On the opposite, each argument isn't very easy to understand compared to
plain text definition.
>
> Of course, we'd need to define a more robust format string convention
> than in my example to ensure the correct configuration is properly
Indeed, by using a single letter, we could face limitations (ex:
overflow, underflow, overflow_underflow, which letter for the 3rd here?)
If we go this way, probably need to brainstorm a bit.
> communicated. What do you think, would this approach would make things
> simpler, or just more complicated in the end?
I'm not 100% sure if some helpers like getopt() will help here? So, I
guess this could be more complicated. This may also be against the
guideline "options should be preceded by the '-' delimiter character."
in [^1] (Ok, this would rather be the --watch option, fed with watch data.)
Would you have suggestions regarding possible helpers ? Or do you have
in mind some others tools that already adopted such approach ?
>
>> + while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
>> + switch (c) {
>> + case 'c':
>> + ncfg[0]++;
>> + break;
>> + case 'e':
>> + ncfg[1]++;
>> + break;
>> + case 'i':
>> + ncfg[2]++;
>> + break;
>> + case 'p':
>> + ncfg[3]++;
>> + break;
>> + case 's':
>> + ncfg[4]++;
>> + break;
>> + case 't':
>> + ncfg[5]++;
>> + break;
>> + };
>> + };
>> +
>> + for (i = 0; i < ARRAY_SIZE(ncfg); i++)
>> + if (ncfg[i] > nwatch)
>> + nwatch = ncfg[i];
>> +
>> + if (nwatch) {
>> + watches = calloc(nwatch, sizeof(*watches));
>
> We need to check if calloc fails, right?
Yes, you're right, will fix this too.
Thanks for reviewing!
Best regards,
Fabrice
>
> William Breathitt Gray
On 9/19/23 17:37, Fabrice Gasnier wrote:
> On 9/17/23 21:07, William Breathitt Gray wrote:
>> On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
>>> This adds a new counter tool to be able to test various watch events.
>>> A flexible watch array can be populated from command line, each field
>>> may be tuned with a dedicated command line argument.
>>> Each argument can be repeated several times: each time it gets repeated,
>>> a corresponding new watch element is allocated.
>>>
>>> It also comes with a simple default watch (to monitor overflows), used
>>> when no watch parameters are provided.
>>>
>>> The print_usage() routine proposes another example, from the command line,
>>> which generates a 2 elements watch array, to monitor:
>>> - overflow events
>>> - capture events, on channel 3, that reads read captured data by
>>> specifying the component id (capture3_component_id being 7 here).
>>>
>>> Signed-off-by: Fabrice Gasnier <[email protected]>
>>
>> Hi Fabrice,
>>
>> This is great idea, it'll make it so much easier to test out drivers
>> so I'm excited! :-)
>
> Hi William,
>
> Thanks
>
>>
>> This is a new tool, so would you add a MAINTAINERS entry for the
>> counter_watch_events.c file?
>
> I haven't thought about it.
> I can add a MAINTAINERS entry, yes!
> Who would you suggest ?
>
>>
>> More comments inline below.
>>
>>> ---
>>> tools/counter/Build | 1 +
>>> tools/counter/Makefile | 8 +-
>>> tools/counter/counter_watch_events.c | 348 +++++++++++++++++++++++++++
>>> 3 files changed, 356 insertions(+), 1 deletion(-)
>>> create mode 100644 tools/counter/counter_watch_events.c
>>>
>>> diff --git a/tools/counter/Build b/tools/counter/Build
>>> index 33f4a51d715e..4bbadb7ec93a 100644
>>> --- a/tools/counter/Build
>>> +++ b/tools/counter/Build
>>> @@ -1 +1,2 @@
>>> counter_example-y += counter_example.o
>>> +counter_watch_events-y += counter_watch_events.o
>>> diff --git a/tools/counter/Makefile b/tools/counter/Makefile
>>> index b2c2946f44c9..00e211edd768 100644
>>> --- a/tools/counter/Makefile
>>> +++ b/tools/counter/Makefile
>>> @@ -14,7 +14,7 @@ MAKEFLAGS += -r
>>>
>>> override CFLAGS += -O2 -Wall -g -D_GNU_SOURCE -I$(OUTPUT)include
>>>
>>> -ALL_TARGETS := counter_example
>>> +ALL_TARGETS := counter_example counter_watch_events
>>> ALL_PROGRAMS := $(patsubst %,$(OUTPUT)%,$(ALL_TARGETS))
>>>
>>> all: $(ALL_PROGRAMS)
>>> @@ -31,6 +31,12 @@ $(OUTPUT)include/linux/counter.h: ../../include/uapi/linux/counter.h
>>>
>>> prepare: $(OUTPUT)include/linux/counter.h
>>>
>>> +COUNTER_WATCH_EVENTS := $(OUTPUT)counter_watch_events.o
>>> +$(COUNTER_WATCH_EVENTS): prepare FORCE
>>> + $(Q)$(MAKE) $(build)=counter_watch_events
>>> +$(OUTPUT)counter_watch_events: $(COUNTER_WATCH_EVENTS)
>>> + $(QUIET_LINK)$(CC) $(CFLAGS) $(LDFLAGS) $< -o $@
>>> +
>>
>> Move this below the COUNTER_EXAMPLE block just so we can keep the
>> recipes in alphabetical order.
>
> Ack, will update it.
>
>>
>>> COUNTER_EXAMPLE := $(OUTPUT)counter_example.o
>>> $(COUNTER_EXAMPLE): prepare FORCE
>>> $(Q)$(MAKE) $(build)=counter_example
>>> diff --git a/tools/counter/counter_watch_events.c b/tools/counter/counter_watch_events.c
>>> new file mode 100644
>>> index 000000000000..7f73a1519d8e
>>> --- /dev/null
>>> +++ b/tools/counter/counter_watch_events.c
>>> @@ -0,0 +1,348 @@
>>> +// SPDX-License-Identifier: GPL-2.0-only
>>> +/* Counter - Test various watch events in a userspace application
>>
>> "Counter" should be "Counter Watch Events" (or "counter_watch_events").
>>
>>> + * inspired by counter_example.c
>>
>> No need to mention counter_example.c, this utility does far more than
>> and bares little resemblance at this point to counter_example.c which is
>> really just a bare minimal example of watching Counter events.
>
> Ack
>
>>
>>> + */
>>> +
>>> +#include <errno.h>
>>> +#include <fcntl.h>
>>> +#include <getopt.h>
>>> +#include <linux/counter.h>
>>> +#include <stdlib.h>
>>> +#include <stdio.h>
>>> +#include <string.h>
>>> +#include <sys/ioctl.h>
>>> +#include <unistd.h>
>>> +
>>> +#define ARRAY_SIZE(x) (sizeof(x) / sizeof((x)[0]))
>>
>> My initial reaction was that this macro is already exposed in some
>> header for us, but my local /usr/include/linux/kernel.h file doesn't
>> appear to bare it so I guess not. Perhaps it'll be fine for our needs --
>> I think the only difference between this ARRAY_SIZE and the Linux kernel
>> one is the addition of __must_be_array(x).
>
> I had the same reaction when trying to use it. Then, I figured out
> several tools redefine this macro.
> Digging further, I just found out some tools have in their Makefile CFLAGS:
> -I$(srctree)/tools/include
> and include from there: <linux/kernel.h>
>
> I'll update the Makefile in v2, and remove this definition from here.
>
>>
>>> +
>>> +static struct counter_watch simple_watch[] = {
>>> + {
>>> + /* Component data: Count 0 count */
>>> + .component.type = COUNTER_COMPONENT_COUNT,
>>> + .component.scope = COUNTER_SCOPE_COUNT,
>>> + .component.parent = 0,
>>> + /* Event type: Index */
>>> + .event = COUNTER_EVENT_OVERFLOW_UNDERFLOW,
>>
>> There's a bit of confusion here. The comment says the event type is
>> INDEX, but the structure event type is set for OVERFLOW_UNDERFLOW; also,
>> the commit description states that we're monitoring "overflows" which
>> implies to me the OVERFLOW event type. So which of the three is it?
>
> Ah yes, It's a mix of bad copy paste and updates. I'll fix it.
>
>>
>>> + /* Device event channel 0 */
>>> + .channel = 0,
>>> + },
>>> +};
>>> +
>>> +static int find_match_or_number_from_array(char *arg, const char * const str[], int sz, __u8 *val)
>>> +{
>>> + unsigned int i;
>>> + char *dummy;
>>> + unsigned long idx;
>>> + int ret;
>>> +
>>> + for (i = 0; i < sz; i++) {
>>> + ret = strncmp(arg, str[i], strlen(str[i]));
>>> + if (!ret && strlen(str[i]) == strlen(arg)) {
>>> + *val = i;
>>> + return 0;
>>> + }
>>
>> This has several strlen calls so I wonder if it's more wasteful than it
>> needs to me. I suppose the compiler would optimize this away, but I
>> think there is an alternative solution.
>>
>> We're checking for an exact match, so you don't need the string length.
>> Instead, you can compare each character, break when characters differ,
>> or return 0 when you reached the null byte for both. So something like
>> this:
>>
>> for (j = 0; arg[j] == str[i][j]; j++) {
>> /* If we reached the end of the strings */
>> if (arg[j] == '\0') {
>> *val = i;
>> return 0;
>> }
>> }
>> /* Strings do not match; continue to the next string */
>>
>> We end up with the same number of lines, so I'll leave it up to you
>> whether you want to use this solution, or if you consider the existing
>> code clearer read.
>
> I'll look forward in the direction you propose. First, we need to
> confirm in which form the arguments can be expected. It depends on your
> proposal to use a --watch string formatted arguments.
>
>>
>>> + }
>>> +
>>> + /* fallback to number */
>>
>> I'm not sure it makes sense to support numbers. Although it's true that
>> the component type, component scope, and event type are passed as __u8
>> values, users are expected to treat those values are opaque and pass
>> them via the respective enum constants. Since we don't guarantee that
>> the specific enum constant values will remain consistent between kernel
>> versions, I don't think it's a good idea to give to users that sort of
>> implication by allowing them to use raw numbers for configuration
>> selection.
>
> Ack, I can remove this.
>
> I'm a bit surprised by this statement. I may be wrong... I'd expect a
> userland binary to be compatible when updating to a newer kernel: e.g.
> user API (ABI?) definitions to be stable (including enum constants) ?
>
>>
>>> + idx = strtoul(optarg, &dummy, 10);
>>> + if (!errno) {
>>> + if (idx >= sz)
>>> + return -EINVAL;
>>> + *val = idx;
>>> + return 0;
>>> + }
>>> +
>>> + return -errno;
>>> +}
>>> +
>>> +static const char * const counter_event_type_name[] = {
>>> + "COUNTER_EVENT_OVERFLOW",
>>> + "COUNTER_EVENT_UNDERFLOW",
>>> + "COUNTER_EVENT_OVERFLOW_UNDERFLOW",
>>> + "COUNTER_EVENT_THRESHOLD",
>>> + "COUNTER_EVENT_INDEX",
>>> + "COUNTER_EVENT_CHANGE_OF_STATE",
>>> + "COUNTER_EVENT_CAPTURE",
>>> +};
>>> +
>>> +static int counter_arg_to_event_type(char *arg, __u8 *event)
>>> +{
>>> + return find_match_or_number_from_array(arg, counter_event_type_name,
>>> + ARRAY_SIZE(counter_event_type_name), event);
>>> +}
>>> +
>>> +static const char * const counter_component_type_name[] = {
>>> + "COUNTER_COMPONENT_NONE",
>>> + "COUNTER_COMPONENT_SIGNAL",
>>> + "COUNTER_COMPONENT_COUNT",
>>> + "COUNTER_COMPONENT_FUNCTION",
>>> + "COUNTER_COMPONENT_SYNAPSE_ACTION",
>>> + "COUNTER_COMPONENT_EXTENSION",
>>> +};
>>> +
>>> +static int counter_arg_to_component_type(char *arg, __u8 *type)
>>> +{
>>> + return find_match_or_number_from_array(arg, counter_component_type_name,
>>> + ARRAY_SIZE(counter_component_type_name), type);
>>> +}
>>> +
>>> +static const char * const counter_scope_name[] = {
>>> + "COUNTER_SCOPE_DEVICE",
>>> + "COUNTER_SCOPE_SIGNAL",
>>> + "COUNTER_SCOPE_COUNT",
>>> +};
>>> +
>>> +static int counter_arg_to_scope(char *arg, __u8 *type)
>>> +{
>>> + return find_match_or_number_from_array(arg, counter_scope_name,
>>> + ARRAY_SIZE(counter_scope_name), type);
>>> +}
>>> +
>>> +static void print_usage(void)
>>> +{
>>> + fprintf(stderr, "Usage: counter_watch_events [options]...\n"
>>> + "Test various watch events for given counter device\n"
>>> + " --channel -c <n>\n"
>>> + " Set watch.channel\n"
>>> + " --debug -d\n"
>>> + " Prints debug information\n"
>>> + " --event -e <number or counter_event_type string>\n"
>>> + " Sets watch.event\n"
>>> + " --help -h\n"
>>> + " Prints usage\n"
>>> + " --device-num -n <n>\n"
>>> + " Set device number (/dev/counter<n>, default to 0)\n"
>>> + " --id -i <n>\n"
>>> + " Set watch.component.id\n"
>>> + " --loop -l <n>\n"
>>> + " Loop for a number of events (forever if n < 0)\n"
>>> + " --parent -p <n>\n"
>>> + " Set watch.component.parent number\n"
>>> + " --scope -s <number or counter_scope string>\n"
>>> + " Set watch.component.scope\n"
>>> + " --type -t <number or counter_component_type string>\n"
>>> + " Set watch.component.type\n"
>>> + "\n"
>>> + "Example with two watched events:\n\n"
>>> + "counter_watch_events -d \\\n"
>>> + "\t-t COUNTER_COMPONENT_COUNT -s COUNTER_SCOPE_COUNT"
>>> + " -e COUNTER_EVENT_OVERFLOW_UNDERFLOW -i 0 -c 0 \\\n"
>>> + "\t-t COUNTER_COMPONENT_EXTENSION -s COUNTER_SCOPE_COUNT"
>>> + " -e COUNTER_EVENT_CAPTURE -i 7 -c 3\n"
>>> + );
>>> +}
>>
>> Are you following any particular convention for the usage description? I
>> wonder if there is a particular preferred standard for command-line
>> interface descriptions. A quick search brought up a few, such as the
>> POSIX Utility Conventions[^1] and docopt[^2].
>>
>> One improvement I would recommend here is to put the short form of the
>> option before the long form and separate them with a command to make it
>> clearer (e.g. "-h, --help").
>>
>> [^1] https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html
>> [^2] http://docopt.org
>
> Thanks for pointing this! So definitely a good pointer, and suggestion
> to look at!
>
> I'll try to improve in v2.
>
>>
>>> +
>>> +static void print_watch(struct counter_watch *watch, int nwatch)
>>> +{
>>> + int i;
>>> +
>>> + /* prints the watch array in C-like structure */
>>> + printf("watch[%d] = {\n", nwatch);
>>> + for (i = 0; i < nwatch; i++) {
>>> + printf(" [%d] =\t{\n"
>>> + "\t\t.component.type = %s\n"
>>> + "\t\t.component.scope = %s\n"
>>> + "\t\t.component.parent = %d\n"
>>> + "\t\t.component.id = %d\n"
>>> + "\t\t.event = %s\n"
>>> + "\t\t.channel = %d\n"
>>> + "\t},\n",
>>> + i,
>>> + counter_component_type_name[watch[i].component.type],
>>> + counter_scope_name[watch[i].component.scope],
>>> + watch[i].component.parent,
>>> + watch[i].component.id,
>>> + counter_event_type_name[watch[i].event],
>>> + watch[i].channel);
>>> + }
>>> + printf("};\n");
>>> +}
>>> +
>>> +static const struct option longopts[] = {
>>> + { "channel", required_argument, 0, 'c' },
>>> + { "debug", no_argument, 0, 'd' },
>>> + { "event", required_argument, 0, 'e' },
>>> + { "help", no_argument, 0, 'h' },
>>> + { "device-num", required_argument, 0, 'n' },
>>> + { "id", required_argument, 0, 'i' },
>>> + { "loop", required_argument, 0, 'l' },
>>> + { "parent", required_argument, 0, 'p' },
>>> + { "scope", required_argument, 0, 's' },
>>> + { "type", required_argument, 0, 't' },
>>> + { },
>>> +};
>>> +
>>> +int main(int argc, char **argv)
>>> +{
>>> + int c, fd, i, ret;
>>> + struct counter_event event_data;
>>> + char *device_name = NULL;
>>> + int debug = 0, loop = -1;
>>> + char *dummy;
>>> + int dev_num = 0, nwatch = 0, ncfg[] = {0, 0, 0, 0, 0, 0};
>>> + int num_chan = 0, num_evt = 0, num_id = 0, num_p = 0, num_s = 0, num_t = 0;
>>> + struct counter_watch *watches;
>>> +
>>> + /*
>>> + * 1st pass: count events configurations number to allocate the watch array.
>>> + * Each watch argument can be repeated several times: each time it gets repeated,
>>> + * a corresponding watch is allocated (and configured) in 2nd pass.
>>> + */
>>
>> It feels a somewhat prone to error (at least cumbersome) to populate
>
> Yes, this could be error prone. This is also why I added a print of the
> gathered arguments when using --debug option.
> Perhaps this could be better to always print it (e.g. print_watch()) ?
>
>> each watch via individual arguments for each field. Since a watch always
>> has these fields, perhaps instead we could pass some format string that
>> represents a watch, and deliminate watches via commas. For example, we
>> could have --watch="cco00,ecc73" to represent the two watches in the
>> usage example.
>
> I like the idea, to concatenate as a string. With current approach, the
> command line quickly becomes very long.
>
> It makes it obvious in your example, that two watches are used, and no
> argument is omitted.
> On the opposite, each argument isn't very easy to understand compared to
> plain text definition.
>
>>
>> Of course, we'd need to define a more robust format string convention
>> than in my example to ensure the correct configuration is properly
>
> Indeed, by using a single letter, we could face limitations (ex:
> overflow, underflow, overflow_underflow, which letter for the 3rd here?)
>
> If we go this way, probably need to brainstorm a bit.
>
>> communicated. What do you think, would this approach would make things
>> simpler, or just more complicated in the end?
>
> I'm not 100% sure if some helpers like getopt() will help here? So, I
> guess this could be more complicated. This may also be against the
> guideline "options should be preceded by the '-' delimiter character."
> in [^1] (Ok, this would rather be the --watch option, fed with watch data.)
>
> Would you have suggestions regarding possible helpers ? Or do you have
> in mind some others tools that already adopted such approach ?
Hi William,
I've prototyped something to follow your suggestion regarding --watch=
string arguments. This may endup in more easy to read, and hopefully
simpler approach :-).
I'll post a V2 soon for this series (removing some patches that seems
already applied), or just this tool.
Thanks,
Fabrice
>
>>
>>> + while ((c = getopt_long(argc, argv, "c:de:hn:i:l:p:s:t:", longopts, NULL)) != -1) {
>>> + switch (c) {
>>> + case 'c':
>>> + ncfg[0]++;
>>> + break;
>>> + case 'e':
>>> + ncfg[1]++;
>>> + break;
>>> + case 'i':
>>> + ncfg[2]++;
>>> + break;
>>> + case 'p':
>>> + ncfg[3]++;
>>> + break;
>>> + case 's':
>>> + ncfg[4]++;
>>> + break;
>>> + case 't':
>>> + ncfg[5]++;
>>> + break;
>>> + };
>>> + };
>>> +
>>> + for (i = 0; i < ARRAY_SIZE(ncfg); i++)
>>> + if (ncfg[i] > nwatch)
>>> + nwatch = ncfg[i];
>>> +
>>> + if (nwatch) {
>>> + watches = calloc(nwatch, sizeof(*watches));
>>
>> We need to check if calloc fails, right?
>
> Yes, you're right, will fix this too.
>
> Thanks for reviewing!
> Best regards,
> Fabrice
>
>>
>> William Breathitt Gray
On Tue, Sep 19, 2023 at 05:37:34PM +0200, Fabrice Gasnier wrote:
> On 9/17/23 21:07, William Breathitt Gray wrote:
> > On Tue, Aug 29, 2023 at 03:40:24PM +0200, Fabrice Gasnier wrote:
> >> This adds a new counter tool to be able to test various watch events.
> >> A flexible watch array can be populated from command line, each field
> >> may be tuned with a dedicated command line argument.
> >> Each argument can be repeated several times: each time it gets repeated,
> >> a corresponding new watch element is allocated.
> >>
> >> It also comes with a simple default watch (to monitor overflows), used
> >> when no watch parameters are provided.
> >>
> >> The print_usage() routine proposes another example, from the command line,
> >> which generates a 2 elements watch array, to monitor:
> >> - overflow events
> >> - capture events, on channel 3, that reads read captured data by
> >> specifying the component id (capture3_component_id being 7 here).
> >>
> >> Signed-off-by: Fabrice Gasnier <[email protected]>
Fabrice,
Sorry for the delay, I'm currently working through my backlog. I see you
have already submitted a version 2 of this patchset, so I'll continue my
review there but I do want to make some direct replys here below as
well.
> > This is a new tool, so would you add a MAINTAINERS entry for the
> > counter_watch_events.c file?
>
> I haven't thought about it.
> I can add a MAINTAINERS entry, yes!
> Who would you suggest ?
Typically the author of the initial patch will also maintain what they
are introducing, but an alternative person is acceptable to serve as
maintainer if that's the plan that's agreed upon. I assume you're
introducing this tool because you're using it internally at ST for
testing, so I suppose the intention is not to get this merged upstream
just to abandon it (i.e. you intend to keep using and improving it). Is
the plan for you to maintain this utility, or is someone else at ST
interested in it?
> >> + }
> >> +
> >> + /* fallback to number */
> >
> > I'm not sure it makes sense to support numbers. Although it's true that
> > the component type, component scope, and event type are passed as __u8
> > values, users are expected to treat those values are opaque and pass
> > them via the respective enum constants. Since we don't guarantee that
> > the specific enum constant values will remain consistent between kernel
> > versions, I don't think it's a good idea to give to users that sort of
> > implication by allowing them to use raw numbers for configuration
> > selection.
>
> Ack, I can remove this.
>
> I'm a bit surprised by this statement. I may be wrong... I'd expect a
> userland binary to be compatible when updating to a newer kernel: e.g.
> user API (ABI?) definitions to be stable (including enum constants) ?
I was wrong in my previous reply. You're absolutely correct[^1] that
userspace ABI must be consistent ("Breaking user programs simply isn't
acceptable"[^2]) so the specific values must remain the same between
kernel versions.
Regardless, I don't think raw numbers provide much benefit for the
use-case of this particular utility; users are testing watch
configurations for a particular device, not the specific constant values
in the data structures. So in the end I still think the raw numbers
code path should be removed.
[^1] Well technically Linux kernel ABI README documentation file
(Documentation/ABI/README) states that backwards compatibility for
stable interfaces is only guaranteed for at least 2 years, but in
practice we strive to never change the user-facing ABI.
[^2] https://yarchive.net/comp/linux/gcc_vs_kernel_stability.html
William Breathitt Gray