Message-ID: <36253b17-0892-83f7-2f17-367298246d8b@foss.st.com>
Date:   Thu, 21 Sep 2023 15:05:01 +0200
From:   Fabrice Gasnier <fabrice.gasnier@...s.st.com>
To:     William Breathitt Gray <william.gray@...aro.org>
CC:     <lee@...nel.org>, <alexandre.torgue@...s.st.com>,
        <linux-iio@...r.kernel.org>,
        <linux-stm32@...md-mailman.stormreply.com>,
        <linux-arm-kernel@...ts.infradead.org>,
        <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 3/8] tools/counter: add a flexible watch events tool

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@...s.st.com>
>>
>> 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 

Powered by blists - more mailing lists