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 <fabrice.gasnier@xxxxxxxxxxx>
>>
>> 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
