From 245cc87c4d5680c9623dda665d2d7bc4b057baff Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jarom=C3=ADr=20Smr=C4=8Dek?= <4plague@gmail.com> Date: Wed, 1 Nov 2023 14:44:39 +0100 Subject: [PATCH] Improved dpservice-dump filter parameters --- docs/deployment/help_dpservice-dump.md | 3 +- include/monitoring/dp_graphtrace_shared.h | 19 +++++--- src/monitoring/dp_graphtrace.c | 53 +++++++++++++++++------ tools/dump/dp_conf.json | 9 +--- tools/dump/main.c | 33 +++++++++----- tools/dump/opts.c | 33 +++++--------- tools/dump/opts.h | 1 - 7 files changed, 87 insertions(+), 64 deletions(-) diff --git a/docs/deployment/help_dpservice-dump.md b/docs/deployment/help_dpservice-dump.md index baf03343f..e2d8639c7 100644 --- a/docs/deployment/help_dpservice-dump.md +++ b/docs/deployment/help_dpservice-dump.md @@ -5,8 +5,7 @@ | -h, --help | None | display this help and exit | | | -v, --version | None | display version and exit | | | --drops | None | show dropped packets | | -| --nodes | None | show graph node traversal | | -| --node-filter | REGEX | show only nodes with name matching REGEX | | +| --nodes | REGEX | show graph node traversal, limit to REGEX-matched nodes (empty string for all) | | | --filter | FILTER | show only packets matching a pcap-style FILTER | | | --hw | None | capture offloaded packets (only outgoing VF->PF packets supported) | | | --pcap | FILE | write packets into a PCAP file | | diff --git a/include/monitoring/dp_graphtrace_shared.h b/include/monitoring/dp_graphtrace_shared.h index ee131e032..13ef43686 100644 --- a/include/monitoring/dp_graphtrace_shared.h +++ b/include/monitoring/dp_graphtrace_shared.h @@ -4,20 +4,22 @@ #include #include #include +#include #include #include "dp_mbuf_dyn.h" #define DP_GRAPHTRACE_MEMPOOL_NAME "dp_graphtrace_mempool" #define DP_GRAPHTRACE_RINGBUF_NAME "dp_graphtrace_ringbuf" +#define DP_GRAPHTRACE_FILTERS_NAME "dp_graphtrace_filters" // DPDK requirement: power of 2 and less than INT32_MAX #define DP_GRAPHTRACE_RINGBUF_SIZE 65536 #define DP_MP_ACTION_GRAPHTRACE "dp_mp_graphtrace" -#define DP_GRAPHTRACE_NODE_FILTER_SIZE 64 -#define DP_GRAPHTRACE_PCAP_FILTER_SIZE 180 +#define DP_GRAPHTRACE_NODE_REGEX_MAXLEN 256 +#define DP_GRAPHTRACE_FILTER_MAXLEN 1024 #ifdef __cplusplus extern "C" { @@ -34,8 +36,14 @@ enum dp_graphtrace_pkt_type { }; struct dp_graphtrace { - struct rte_mempool *mempool; - struct rte_ring *ringbuf; + struct rte_mempool *mempool; + struct rte_ring *ringbuf; + const struct rte_memzone *filters; +}; + +struct dp_graphtrace_params { + char node_regex[DP_GRAPHTRACE_NODE_REGEX_MAXLEN]; + char filter_string[DP_GRAPHTRACE_FILTER_MAXLEN]; }; struct dp_graphtrace_pktinfo { @@ -46,12 +54,9 @@ struct dp_graphtrace_pktinfo { uint16_t dst_port_id; }; -// TODO this is bad! this is too small to be used right! struct dp_graphtrace_params_start { bool drops; bool nodes; - char node_filter[DP_GRAPHTRACE_NODE_FILTER_SIZE]; - char pcap_filter[DP_GRAPHTRACE_PCAP_FILTER_SIZE]; bool hw; }; diff --git a/src/monitoring/dp_graphtrace.c b/src/monitoring/dp_graphtrace.c index 09bec9793..7f0354848 100644 --- a/src/monitoring/dp_graphtrace.c +++ b/src/monitoring/dp_graphtrace.c @@ -31,7 +31,7 @@ static regex_t nodename_re; static bool bpf_filtered; static struct bpf_program bpf; -static int dp_graphtrace_init_memzone(void) +static int dp_graphtrace_init_memory(void) { // DPDK recommendation for mempool size: power of 2 minus one for best memory utilization // So using ringbuffer size minus one, when the ring buffer is (almost) full, allocation will start failing @@ -53,35 +53,51 @@ static int dp_graphtrace_init_memzone(void) return DP_ERROR; } + graphtrace.filters = rte_memzone_reserve(DP_GRAPHTRACE_FILTERS_NAME, sizeof(struct dp_graphtrace_params), + rte_socket_id(), 0); + if (!graphtrace.filters) { + DPS_LOG_ERR("Cannot create graphtrace filter definition memory", DP_LOG_RET(rte_errno)); + rte_mempool_free(graphtrace.mempool); + rte_ring_free(graphtrace.ringbuf); + return DP_ERROR; + } + offload_enabled = dp_conf_is_offload_enabled(); return DP_OK; } -static void dp_graphtrace_free_memzone(void) +static void dp_graphtrace_free_memory(void) { rte_ring_free(graphtrace.ringbuf); graphtrace.ringbuf = NULL; rte_mempool_free(graphtrace.mempool); graphtrace.mempool = NULL; + rte_memzone_free(graphtrace.filters); + graphtrace.filters = NULL; } static int dp_handle_graphtrace_start(const struct dp_graphtrace_mp_request *request) { + struct dp_graphtrace_params *filters = (struct dp_graphtrace_params *)graphtrace.filters->addr; int ret; - nodename_filtered = request->params.start.node_filter[0] != '\0'; + // there are additional parameters in shared memory (cannot fit into the request) + if (!DP_IS_NUL_TERMINATED(filters->node_regex) + || !DP_IS_NUL_TERMINATED(filters->filter_string)) + return -EINVAL; + + nodename_filtered = *filters->node_regex; if (nodename_filtered) { - if (!DP_IS_NUL_TERMINATED(request->params.start.node_filter) - || regcomp(&nodename_re, request->params.start.node_filter, REG_NOMATCH) != 0) + if (regcomp(&nodename_re, filters->node_regex, REG_NOSUB) != 0) return -EINVAL; } - bpf_filtered = request->params.start.pcap_filter[0] != '\0'; + bpf_filtered = *filters->filter_string; if (bpf_filtered) { - if (!DP_IS_NUL_TERMINATED(request->params.start.pcap_filter) - || DP_FAILED(dp_compile_bpf(&bpf, request->params.start.pcap_filter))) { - regfree(&nodename_re); + if (DP_FAILED(dp_compile_bpf(&bpf, filters->filter_string))) { + if (nodename_filtered) + regfree(&nodename_re); return -EINVAL; } } @@ -191,7 +207,7 @@ int dp_graphtrace_init(void) { int ret; - ret = dp_graphtrace_init_memzone(); + ret = dp_graphtrace_init_memory(); if (DP_FAILED(ret)) { DPS_LOG_ERR("Failed to init memzone for dp graphtrace", DP_LOG_RET(ret)); return DP_ERROR; @@ -200,7 +216,7 @@ int dp_graphtrace_init(void) ret = rte_mp_action_register(DP_MP_ACTION_GRAPHTRACE, dp_handle_mp_graphtrace_action); if (DP_FAILED(ret)) { DPS_LOG_ERR("Cannot register graphtrace action", DP_LOG_RET(ret)); - dp_graphtrace_free_memzone(); + dp_graphtrace_free_memory(); return DP_ERROR; } @@ -213,10 +229,21 @@ int dp_graphtrace_init(void) void dp_graphtrace_free(void) { rte_mp_action_unregister(DP_MP_ACTION_GRAPHTRACE); - dp_graphtrace_free_memzone(); + dp_graphtrace_free_memory(); } +static __rte_always_inline +bool dp_is_node_match(regex_t *re, const struct rte_node *node, const struct rte_node *next_node) +{ + if (node) + return regexec(re, node->name, 0, NULL, 0) == 0; + else if (next_node) + return regexec(re, next_node->name, 0, NULL, 0) == 0; + else + return false; +} + void _dp_graphtrace_send(enum dp_graphtrace_pkt_type type, const struct rte_node *node, const struct rte_node *next_node, @@ -230,7 +257,7 @@ void _dp_graphtrace_send(enum dp_graphtrace_pkt_type type, uint32_t sent; for (uint32_t i = 0; i < nb_objs; ++i) { - if (nodename_filtered && node && regexec(&nodename_re, node->name, 0, NULL, 0) == REG_NOMATCH) + if (nodename_filtered && !dp_is_node_match(&nodename_re, node, next_node)) continue; if (bpf_filtered && !dp_is_bpf_match(&bpf, objs[i])) continue; diff --git a/tools/dump/dp_conf.json b/tools/dump/dp_conf.json index c1eecf312..97984f167 100644 --- a/tools/dump/dp_conf.json +++ b/tools/dump/dp_conf.json @@ -12,14 +12,7 @@ }, { "lgopt": "nodes", - "help": "show graph node traversal", - "var": "showing_nodes", - "type": "bool", - "default": "false" - }, - { - "lgopt": "node-filter", - "help": "show only nodes with name matching REGEX", + "help": "show graph node traversal, limit to REGEX-matched nodes (empty string for all)", "arg": "REGEX" }, { diff --git a/tools/dump/main.c b/tools/dump/main.c index 747da06a2..cda5cea5b 100644 --- a/tools/dump/main.c +++ b/tools/dump/main.c @@ -49,8 +49,9 @@ static char *eal_args[RTE_DIM(eal_args_mem)]; static const char *pcap_path = NULL; static struct dp_pcap dp_pcap; -static char node_filter[DP_GRAPHTRACE_NODE_FILTER_SIZE] = ""; -static char packet_filter[DP_GRAPHTRACE_PCAP_FILTER_SIZE] = ""; +static bool showing_nodes = false; +static char node_filter[DP_GRAPHTRACE_NODE_REGEX_MAXLEN] = ""; +static char packet_filter[DP_GRAPHTRACE_FILTER_MAXLEN] = ""; static bool interrupt = false; static bool primary_alive = false; @@ -90,6 +91,10 @@ static int dp_graphtrace_connect(struct dp_graphtrace *graphtrace) if (!graphtrace->ringbuf) return DP_ERROR; + graphtrace->filters = rte_memzone_lookup(DP_GRAPHTRACE_FILTERS_NAME); + if (!graphtrace->filters) + return DP_ERROR; + return DP_OK; } @@ -202,22 +207,21 @@ static int dp_graphtrace_request(struct dp_graphtrace_mp_request *request, struc return DP_OK; } -static int dp_graphtrace_start(void) +static int dp_graphtrace_start(struct dp_graphtrace *graphtrace) { int ret; struct dp_graphtrace_mp_reply reply; struct dp_graphtrace_mp_request request = { .action = DP_GRAPHTRACE_ACTION_START, .params.start.drops = dp_conf_is_showing_drops(), - .params.start.nodes = dp_conf_is_showing_nodes(), + .params.start.nodes = showing_nodes, .params.start.hw = dp_conf_is_offload_enabled(), }; + struct dp_graphtrace_params *filters = (struct dp_graphtrace_params *)graphtrace->filters->addr; - if (*node_filter) - snprintf(request.params.start.node_filter, sizeof(request.params.start.node_filter), "%s", node_filter); - - if (*packet_filter) - snprintf(request.params.start.pcap_filter, sizeof(request.params.start.pcap_filter), "%s", packet_filter); + // sizes already checked by argument parser, default is an empty string + snprintf(filters->node_regex, sizeof(filters->node_regex), "%s", node_filter); + snprintf(filters->filter_string, sizeof(filters->filter_string), "%s", packet_filter); ret = dp_graphtrace_request(&request, &reply); if (DP_FAILED(ret)) @@ -295,7 +299,7 @@ static int dp_graphtrace_main(void) return ret; } - if (DP_FAILED(dp_graphtrace_start())) { + if (DP_FAILED(dp_graphtrace_start(&graphtrace))) { fprintf(stderr, "Failed to request graph tracing\n"); rte_eal_alarm_cancel(dp_graphtrace_monitor_primary, (void *)-1); return DP_ERROR; @@ -359,12 +363,18 @@ static int dp_argparse_opt_pcap(const char *arg) return DP_OK; } -static int dp_argparse_opt_node_filter(const char *arg) +static int dp_argparse_opt_nodes(const char *arg) { regex_t validation_re; char reg_error[256]; int reg_ret; + // prevent accidental '--nodes -option' meaning '--nodes="-option"' + if (*arg == '-') { + fprintf(stderr, "Node filter regular expression starts with ambiguous '-'\n"); + return DP_ERROR; + } + if ((size_t)snprintf(node_filter, sizeof(node_filter), "%s", arg) >= sizeof(node_filter)) { fprintf(stderr, "Node filter regular expression is too long\n"); return DP_ERROR; @@ -379,6 +389,7 @@ static int dp_argparse_opt_node_filter(const char *arg) regfree(&validation_re); + showing_nodes = true; return DP_OK; } diff --git a/tools/dump/opts.c b/tools/dump/opts.c index 1b4695d7b..fe26e1bb9 100644 --- a/tools/dump/opts.c +++ b/tools/dump/opts.c @@ -17,7 +17,6 @@ enum { _OPT_SHOPT_MAX = 255, OPT_DROPS, OPT_NODES, - OPT_NODE_FILTER, OPT_FILTER, OPT_HW, OPT_PCAP, @@ -30,8 +29,7 @@ static const struct option dp_conf_longopts[] = { { "help", 0, 0, OPT_HELP }, { "version", 0, 0, OPT_VERSION }, { "drops", 0, 0, OPT_DROPS }, - { "nodes", 0, 0, OPT_NODES }, - { "node-filter", 1, 0, OPT_NODE_FILTER }, + { "nodes", 1, 0, OPT_NODES }, { "filter", 1, 0, OPT_FILTER }, { "hw", 0, 0, OPT_HW }, { "pcap", 1, 0, OPT_PCAP }, @@ -40,7 +38,6 @@ static const struct option dp_conf_longopts[] = { }; static bool showing_drops = false; -static bool showing_nodes = false; static bool offload_enabled = false; static bool stop_mode = false; @@ -49,11 +46,6 @@ bool dp_conf_is_showing_drops(void) return showing_drops; } -bool dp_conf_is_showing_nodes(void) -{ - return showing_nodes; -} - bool dp_conf_is_offload_enabled(void) { return offload_enabled; @@ -68,7 +60,7 @@ bool dp_conf_is_stop_mode(void) /* These functions need to be implemented by the user of this generated code */ static void dp_argparse_version(void); -static int dp_argparse_opt_node_filter(const char *arg); +static int dp_argparse_opt_nodes(const char *arg); static int dp_argparse_opt_filter(const char *arg); static int dp_argparse_opt_pcap(const char *arg); @@ -76,15 +68,14 @@ static int dp_argparse_opt_pcap(const char *arg); static inline void dp_argparse_help(const char *progname, FILE *outfile) { fprintf(outfile, "Usage: %s [options]\n" - " -h, --help display this help and exit\n" - " -v, --version display version and exit\n" - " --drops show dropped packets\n" - " --nodes show graph node traversal\n" - " --node-filter=REGEX show only nodes with name matching REGEX\n" - " --filter=FILTER show only packets matching a pcap-style FILTER\n" - " --hw capture offloaded packets (only outgoing VF->PF packets supported)\n" - " --pcap=FILE write packets into a PCAP file\n" - " --stop do nothing, only make sure tracing is disabled in dp-service\n" + " -h, --help display this help and exit\n" + " -v, --version display version and exit\n" + " --drops show dropped packets\n" + " --nodes=REGEX show graph node traversal, limit to REGEX-matched nodes (empty string for all)\n" + " --filter=FILTER show only packets matching a pcap-style FILTER\n" + " --hw capture offloaded packets (only outgoing VF->PF packets supported)\n" + " --pcap=FILE write packets into a PCAP file\n" + " --stop do nothing, only make sure tracing is disabled in dp-service\n" , progname); } @@ -95,9 +86,7 @@ static int dp_conf_parse_arg(int opt, const char *arg) case OPT_DROPS: return dp_argparse_store_true(&showing_drops); case OPT_NODES: - return dp_argparse_store_true(&showing_nodes); - case OPT_NODE_FILTER: - return dp_argparse_opt_node_filter(arg); + return dp_argparse_opt_nodes(arg); case OPT_FILTER: return dp_argparse_opt_filter(arg); case OPT_HW: diff --git a/tools/dump/opts.h b/tools/dump/opts.h index 0432754da..6c8830f27 100644 --- a/tools/dump/opts.h +++ b/tools/dump/opts.h @@ -6,7 +6,6 @@ /***********************************************************************/ bool dp_conf_is_showing_drops(void); -bool dp_conf_is_showing_nodes(void); bool dp_conf_is_offload_enabled(void); bool dp_conf_is_stop_mode(void);