diff options
author | Costa Tsaousis <costa@netdata.cloud> | 2024-01-10 00:49:27 +0200 |
---|---|---|
committer | GitHub <noreply@github.com> | 2024-01-10 00:49:27 +0200 |
commit | 8d397b116a9f817e1d184dc867f66cc0382f69c6 (patch) | |
tree | cd7801ca93a856ee87fc1425a6fa3c986f4d13f7 /collectors | |
parent | 2c97b33d7a38e29dc3281b15823b089bc2a4ca03 (diff) |
cleanup proc net-dev renames (#16745)
* cleanup proc net-dev renames
* more cleanup
* remove unused variable
* fix memory leak in pluginsd parser
* fix double-free
Diffstat (limited to 'collectors')
-rw-r--r-- | collectors/cgroups.plugin/cgroup-discovery.c | 11 | ||||
-rw-r--r-- | collectors/plugins.d/pluginsd_parser.c | 3 | ||||
-rw-r--r-- | collectors/proc.plugin/plugin_proc.h | 4 | ||||
-rw-r--r-- | collectors/proc.plugin/proc_net_dev.c | 290 | ||||
-rw-r--r-- | collectors/proc.plugin/proc_net_dev_renames.c | 53 | ||||
-rw-r--r-- | collectors/proc.plugin/proc_net_dev_renames.h | 26 |
6 files changed, 141 insertions, 246 deletions
diff --git a/collectors/cgroups.plugin/cgroup-discovery.c b/collectors/cgroups.plugin/cgroup-discovery.c index ede35ed8a2..78820c4cc7 100644 --- a/collectors/cgroups.plugin/cgroup-discovery.c +++ b/collectors/cgroups.plugin/cgroup-discovery.c @@ -42,7 +42,7 @@ static inline void cgroup_free_network_interfaces(struct cgroup *cg) { cg->interfaces = i->next; // delete the registration of proc_net_dev rename - netdev_rename_device_del(i->host_device); + cgroup_rename_task_device_del(i->host_device); freez((void *)i->host_device); freez((void *)i->container_device); @@ -1084,8 +1084,13 @@ static inline void read_cgroup_network_interfaces(struct cgroup *cg) { collector_info("CGROUP: cgroup '%s' has network interface '%s' as '%s'", cg->id, i->host_device, i->container_device); // register a device rename to proc_net_dev.c - netdev_rename_device_add(i->host_device, i->container_device, cg->chart_id, cg->chart_labels, - k8s_is_kubepod(cg) ? "k8s." : "", cgroup_netdev_get(cg)); + cgroup_rename_task_add( + i->host_device, + i->container_device, + cg->chart_id, + cg->chart_labels, + k8s_is_kubepod(cg) ? "k8s." : "", + cgroup_netdev_get(cg)); } } diff --git a/collectors/plugins.d/pluginsd_parser.c b/collectors/plugins.d/pluginsd_parser.c index e52111c479..96967bd00d 100644 --- a/collectors/plugins.d/pluginsd_parser.c +++ b/collectors/plugins.d/pluginsd_parser.c @@ -1225,7 +1225,7 @@ inline size_t pluginsd_process(RRDHOST *host, struct plugind *cd, FILE *fp_plugi ND_LOG_STACK_PUSH(lgs); buffered_reader_init(&parser->reader); - BUFFER *buffer = buffer_create(sizeof(parser->reader.read_buffer) + 2, NULL); + CLEAN_BUFFER *buffer = buffer_create(sizeof(parser->reader.read_buffer) + 2, NULL); while(likely(service_running(SERVICE_COLLECTORS))) { if(unlikely(!buffered_reader_next_line(&parser->reader, buffer))) { @@ -1247,7 +1247,6 @@ inline size_t pluginsd_process(RRDHOST *host, struct plugind *cd, FILE *fp_plugi buffer->len = 0; buffer->buffer[0] = '\0'; } - buffer_free(buffer); cd->unsafe.enabled = parser->user.enabled; count = parser->user.data_collections_count; diff --git a/collectors/proc.plugin/plugin_proc.h b/collectors/proc.plugin/plugin_proc.h index e4fc105bac..187e76a971 100644 --- a/collectors/proc.plugin/plugin_proc.h +++ b/collectors/proc.plugin/plugin_proc.h @@ -54,7 +54,7 @@ extern unsigned long long zfs_arcstats_shrinkable_cache_size_bytes; extern bool inside_lxc_container; // netdev renames -void netdev_rename_device_add( +void cgroup_rename_task_add( const char *host_device, const char *container_device, const char *container_name, @@ -62,7 +62,7 @@ void netdev_rename_device_add( const char *ctx_prefix, const DICTIONARY_ITEM *cgroup_netdev_link); -void netdev_rename_device_del(const char *host_device); +void cgroup_rename_task_device_del(const char *host_device); #include "proc_self_mountinfo.h" #include "proc_pressure.h" diff --git a/collectors/proc.plugin/proc_net_dev.c b/collectors/proc.plugin/proc_net_dev.c index 677d87be24..1afec2d8c9 100644 --- a/collectors/proc.plugin/proc_net_dev.c +++ b/collectors/proc.plugin/proc_net_dev.c @@ -1,6 +1,7 @@ // SPDX-License-Identifier: GPL-3.0-or-later #include "plugin_proc.h" +#include "proc_net_dev_renames.h" #define PLUGIN_PROC_MODULE_NETDEV_NAME "/proc/net/dev" #define CONFIG_SECTION_PLUGIN_PROC_NETDEV "plugin:" PLUGIN_PROC_CONFIG_NAME ":" PLUGIN_PROC_MODULE_NETDEV_NAME @@ -13,19 +14,13 @@ time_t double_linked_device_collect_delay_secs = 120; -void cgroup_netdev_reset_all(void); -void cgroup_netdev_release(const DICTIONARY_ITEM *link); -const void *cgroup_netdev_dup(const DICTIONARY_ITEM *link); -void cgroup_netdev_add_bandwidth(const DICTIONARY_ITEM *link, NETDATA_DOUBLE received, NETDATA_DOUBLE sent); - enum { NETDEV_DUPLEX_UNKNOWN, NETDEV_DUPLEX_HALF, NETDEV_DUPLEX_FULL }; -static const char *get_duplex_string(int duplex) -{ +static const char *get_duplex_string(int duplex) { switch (duplex) { case NETDEV_DUPLEX_FULL: return "full"; @@ -46,8 +41,7 @@ enum { NETDEV_OPERSTATE_UP }; -static inline int get_operstate(char *operstate) -{ +static inline int get_operstate(char *operstate) { // As defined in https://www.kernel.org/doc/Documentation/ABI/testing/sysfs-class-net if (!strcmp(operstate, "up")) return NETDEV_OPERSTATE_UP; @@ -65,8 +59,7 @@ static inline int get_operstate(char *operstate) return NETDEV_OPERSTATE_UNKNOWN; } -static const char *get_operstate_string(int operstate) -{ +static const char *get_operstate_string(int operstate) { switch (operstate) { case NETDEV_OPERSTATE_UP: return "up"; @@ -94,13 +87,11 @@ static struct netdev { size_t len; // flags - int virtual; - int configured; + bool virtual; + bool configured; int enabled; - int updated; - + bool updated; bool function_ready; - bool double_linked; // iflink != ifindex time_t discover_time; @@ -255,10 +246,8 @@ static struct netdev { const DICTIONARY_ITEM *cgroup_netdev_link; - struct netdev *next; -} *netdev_root = NULL, *netdev_last_used = NULL; - -static size_t netdev_added = 0, netdev_found = 0; + struct netdev *prev, *next; +} *netdev_root = NULL; // ---------------------------------------------------------------------------- @@ -382,133 +371,21 @@ static void netdev_free(struct netdev *d) { freez((void *)d->filename_carrier); freez((void *)d->filename_mtu); freez((void *)d); - netdev_added--; -} - -// ---------------------------------------------------------------------------- -// netdev renames - -static struct netdev_rename { - const char *host_device; - uint32_t hash; - - const char *container_device; - const char *container_name; - const char *ctx_prefix; - - RRDLABELS *chart_labels; - - int processed; - - const DICTIONARY_ITEM *cgroup_netdev_link; - - struct netdev_rename *next; -} *netdev_rename_root = NULL; - -static int netdev_pending_renames = 0; -static netdata_mutex_t netdev_rename_mutex = NETDATA_MUTEX_INITIALIZER; -static netdata_mutex_t netdev_dev_mutex = NETDATA_MUTEX_INITIALIZER; - -static struct netdev_rename *netdev_rename_find(const char *host_device, uint32_t hash) { - struct netdev_rename *r; - - for(r = netdev_rename_root; r ; r = r->next) - if(r->hash == hash && !strcmp(host_device, r->host_device)) - return r; - - return NULL; -} - -// other threads can call this function to register a rename to a netdev -void netdev_rename_device_add( - const char *host_device, - const char *container_device, - const char *container_name, - RRDLABELS *labels, - const char *ctx_prefix, - const DICTIONARY_ITEM *cgroup_netdev_link) -{ - netdata_mutex_lock(&netdev_rename_mutex); - - uint32_t hash = simple_hash(host_device); - struct netdev_rename *r = netdev_rename_find(host_device, hash); - if(!r) { - r = callocz(1, sizeof(struct netdev_rename)); - r->host_device = strdupz(host_device); - r->container_device = strdupz(container_device); - r->container_name = strdupz(container_name); - r->ctx_prefix = strdupz(ctx_prefix); - r->chart_labels = rrdlabels_create(); - rrdlabels_migrate_to_these(r->chart_labels, labels); - r->hash = hash; - r->next = netdev_rename_root; - r->processed = 0; - r->cgroup_netdev_link = cgroup_netdev_link; - - netdev_rename_root = r; - netdev_pending_renames++; - collector_info("CGROUP: registered network interface rename for '%s' as '%s' under '%s'", r->host_device, r->container_device, r->container_name); - } - else { - if(strcmp(r->container_device, container_device) != 0 || strcmp(r->container_name, container_name) != 0) { - freez((void *) r->container_device); - freez((void *) r->container_name); - - r->container_device = strdupz(container_device); - r->container_name = strdupz(container_name); - - rrdlabels_migrate_to_these(r->chart_labels, labels); - - r->processed = 0; - r->cgroup_netdev_link = cgroup_netdev_link; - - netdev_pending_renames++; - collector_info("CGROUP: altered network interface rename for '%s' as '%s' under '%s'", r->host_device, r->container_device, r->container_name); - } - } - - netdata_mutex_unlock(&netdev_rename_mutex); } -// other threads can call this function to delete a rename to a netdev -void netdev_rename_device_del(const char *host_device) { - netdata_mutex_lock(&netdev_rename_mutex); - - struct netdev_rename *r, *last = NULL; - - uint32_t hash = simple_hash(host_device); - for(r = netdev_rename_root; r ; last = r, r = r->next) { - if (r->hash == hash && !strcmp(host_device, r->host_device)) { - if (netdev_rename_root == r) - netdev_rename_root = r->next; - else if (last) - last->next = r->next; - - if(!r->processed) - netdev_pending_renames--; - - collector_info("CGROUP: unregistered network interface rename for '%s' as '%s' under '%s'", r->host_device, r->container_device, r->container_name); - - freez((void *) r->host_device); - freez((void *) r->container_name); - freez((void *) r->container_device); - freez((void *) r->ctx_prefix); - rrdlabels_destroy(r->chart_labels); - cgroup_netdev_release(r->cgroup_netdev_link); - freez((void *) r); - break; - } - } +static netdata_mutex_t netdev_mutex = NETDATA_MUTEX_INITIALIZER; - netdata_mutex_unlock(&netdev_rename_mutex); -} +// ---------------------------------------------------------------------------- -static inline void netdev_rename_cgroup(struct netdev *d, struct netdev_rename *r) { - collector_info("CGROUP: renaming network interface '%s' as '%s' under '%s'", r->host_device, r->container_device, r->container_name); +static inline void netdev_rename(struct netdev *d, struct rename_task *r) { + collector_info("CGROUP: renaming network interface '%s' as '%s' under '%s'", d->name, r->container_device, r->container_name); netdev_charts_release(d); netdev_free_chart_strings(d); + + cgroup_netdev_release(d->cgroup_netdev_link); d->cgroup_netdev_link = cgroup_netdev_dup(r->cgroup_netdev_link); + d->discover_time = 0; char buffer[RRD_ID_LENGTH_MAX + 1]; @@ -585,33 +462,15 @@ static inline void netdev_rename_cgroup(struct netdev *d, struct netdev_rename * d->flipped = 1; } -static inline void netdev_rename(struct netdev *d) { - struct netdev_rename *r = netdev_rename_find(d->name, d->hash); - if(unlikely(r && !r->processed)) { - netdev_rename_cgroup(d, r); - r->processed = 1; - d->discover_time = 0; - netdev_pending_renames--; +static void netdev_rename_this_device(struct netdev *d) { + const DICTIONARY_ITEM *item = dictionary_get_and_acquire_item(netdev_renames, d->name); + if(item) { + struct rename_task *r = dictionary_acquired_item_value(item); + netdev_rename(d, r); + dictionary_acquired_item_release(netdev_renames, item); } } -static inline void netdev_rename_lock(struct netdev *d) { - netdata_mutex_lock(&netdev_rename_mutex); - netdev_rename(d); - netdata_mutex_unlock(&netdev_rename_mutex); -} - -static inline void netdev_rename_all_lock(void) { - netdata_mutex_lock(&netdev_rename_mutex); - - struct netdev *d; - for(d = netdev_root; d ; d = d->next) - netdev_rename(d); - - netdev_pending_renames = 0; - netdata_mutex_unlock(&netdev_rename_mutex); -} - // ---------------------------------------------------------------------------- int netdev_function_net_interfaces(uuid_t *transaction __maybe_unused, BUFFER *wb, @@ -645,11 +504,11 @@ int netdev_function_net_interfaces(uuid_t *transaction __maybe_unused, BUFFER *w double max_drops_rx = 0.0; double max_drops_tx = 0.0; - netdata_mutex_lock(&netdev_dev_mutex); + netdata_mutex_lock(&netdev_mutex); RRDDIM *rd = NULL; - for (struct netdev *d = netdev_root; d != netdev_last_used; d = d->next) { + for (struct netdev *d = netdev_root; d ; d = d->next) { if (unlikely(!d->function_ready)) continue; @@ -709,7 +568,7 @@ int netdev_function_net_interfaces(uuid_t *transaction __maybe_unused, BUFFER *w buffer_json_array_close(wb); } - netdata_mutex_unlock(&netdev_dev_mutex); + netdata_mutex_unlock(&netdev_mutex); buffer_json_array_close(wb); // data buffer_json_member_add_object(wb, "columns"); @@ -918,49 +777,26 @@ int netdev_function_net_interfaces(uuid_t *transaction __maybe_unused, BUFFER *w buffer_json_member_add_time_t(wb, "expires", now_realtime_sec() + 1); buffer_json_finalize(wb); - int response = HTTP_RESP_OK; - if(is_cancelled_cb && is_cancelled_cb(is_cancelled_cb_data)) { - buffer_flush(wb); - response = HTTP_RESP_CLIENT_CLOSED_REQUEST; - } - - if(result_cb) - result_cb(wb, response, result_cb_data); - - return response; + return HTTP_RESP_OK; } // netdev data collection static void netdev_cleanup() { - if(likely(netdev_found == netdev_added)) return; - - netdev_added = 0; - struct netdev *d = netdev_root, *last = NULL; + struct netdev *d = netdev_root; while(d) { if(unlikely(!d->updated)) { - // collector_info("Removing network device '%s', linked after '%s'", d->name, last?last->name:"ROOT"); - - if(netdev_last_used == d) - netdev_last_used = last; + struct netdev *next = d->next; // keep the next, to continue; - struct netdev *t = d; + DOUBLE_LINKED_LIST_REMOVE_ITEM_UNSAFE(netdev_root, d, prev, next); - if(d == netdev_root || !last) - netdev_root = d = d->next; - - else - last->next = d = d->next; - - t->next = NULL; - netdev_free(t); - } - else { - netdev_added++; - last = d; - d->updated = 0; - d = d->next; + netdev_free(d); + d = next; + continue; } + + d->updated = false; + d = d->next; } } @@ -970,19 +806,9 @@ static struct netdev *get_netdev(const char *name) { uint32_t hash = simple_hash(name); // search it, from the last position to the end - for(d = netdev_last_used ; d ; d = d->next) { - if(unlikely(hash == d->hash && !strcmp(name, d->name))) { - netdev_last_used = d->next; + for(d = netdev_root ; d ; d = d->next) { + if(unlikely(hash == d->hash && !strcmp(name, d->name))) return d; - } - } - - // search it from the beginning to the last position we used - for(d = netdev_root ; d != netdev_last_used ; d = d->next) { - if(unlikely(hash == d->hash && !strcmp(name, d->name))) { - netdev_last_used = d->next; - return d; - } } // create a new one @@ -1036,18 +862,7 @@ static struct netdev *get_netdev(const char *name) { d->chart_family = strdupz(d->name); d->priority = NETDATA_CHART_PRIO_FIRST_NET_IFACE; - netdev_rename_lock(d); - - netdev_added++; - - // link it to the end - if(netdev_root) { - struct netdev *e; - for(e = netdev_root; e->next ; e = e->next) ; - e->next = d; - } - else - netdev_root = d; + DOUBLE_LINKED_LIST_APPEND_ITEM_UNSAFE(netdev_root, d, prev, next); return d; } @@ -1125,6 +940,8 @@ int do_proc_net_dev(int update_every, usec_t dt) { disabled_list = simple_pattern_create( config_get(CONFIG_SECTION_PLUGIN_PROC_NETDEV, "disable by default interfaces matching", "lo fireqos* *-ifb fwpr* fwbr* fwln*"), NULL, SIMPLE_PATTERN_EXACT, true); + + netdev_renames_init(); } if(unlikely(!ff)) { @@ -1135,12 +952,6 @@ int do_proc_net_dev(int update_every, usec_t dt) { ff = procfile_readall(ff); if(unlikely(!ff)) return 0; // we return 0, so that we will retry to open it next time - // rename all the devices, if we have pending renames - if(unlikely(netdev_pending_renames)) - netdev_rename_all_lock(); - - netdev_found = 0; - kernel_uint_t system_rbytes = 0; kernel_uint_t system_tbytes = 0; @@ -1156,14 +967,13 @@ int do_proc_net_dev(int update_every, usec_t dt) { if(name[len - 1] == ':') name[len - 1] = '\0'; struct netdev *d = get_netdev(name); - d->updated = 1; - netdev_found++; + d->updated = true; if(unlikely(!d->configured)) { - // this is the first time we see this interface + // the first time we see this interface // remember we configured it - d->configured = 1; + d->configured = true; d->discover_time = now; d->enabled = enable_new_interfaces; @@ -1174,12 +984,12 @@ int do_proc_net_dev(int update_every, usec_t dt) { char buf[FILENAME_MAX + 1]; snprintfz(buf, FILENAME_MAX, path_to_sys_devices_virtual_net, d->name); - d->virtual = likely(access(buf, R_OK) == 0) ? 1 : 0; + d->virtual = likely(access(buf, R_OK) == 0) ? true : false; // At least on Proxmox inside LXC: eth0 is virtual. // Virtual interfaces are not taken into account in system.net calculations if (inside_lxc_container && d->virtual && strncmp(d->name, "eth", 3) == 0) - d->virtual = 0; + d->virtual = false; if (d->virtual) rrdlabels_add(d->chart_labels, "interface_type", "virtual", RRDLABEL_SRC_AUTO); @@ -1260,13 +1070,15 @@ int do_proc_net_dev(int update_every, usec_t dt) { if(unlikely(!d->enabled)) continue; + if(!d->cgroup_netdev_link) + netdev_rename_this_device(d); + // See https://github.com/netdata/netdata/issues/15206 // This is necessary to prevent the creation of charts for virtual interfaces that will later be // recreated as container interfaces (create container) or // rediscovered and recreated only to be deleted almost immediately (stop/remove container) - if (d->double_linked && d->virtual && (now - d->discover_time < double_linked_device_collect_delay_secs)) { + if (d->double_linked && d->virtual && (now - d->discover_time < double_linked_device_collect_delay_secs)) continue; - } if(likely(d->do_bandwidth != CONFIG_BOOLEAN_NO || !d->virtual)) { d->rbytes = str2kernel_uint_t(procfile_lineword(ff, l, 1)); @@ -1974,10 +1786,10 @@ void *netdev_main(void *ptr) worker_is_busy(0); - netdata_mutex_lock(&netdev_dev_mutex); + netdata_mutex_lock(&netdev_mutex); if (do_proc_net_dev(localhost->rrd_update_every, hb_dt)) break; - netdata_mutex_unlock(&netdev_dev_mutex); + netdata_mutex_unlock(&netdev_mutex); } } netdata_thread_cleanup_pop(1); diff --git a/collectors/proc.plugin/proc_net_dev_renames.c b/collectors/proc.plugin/proc_net_dev_renames.c new file mode 100644 index 0000000000..fb50ce66c6 --- /dev/null +++ b/collectors/proc.plugin/proc_net_dev_renames.c @@ -0,0 +1,53 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +#include "proc_net_dev_renames.h" + +DICTIONARY *netdev_renames = NULL; + +static void dictionary_netdev_rename_delete_cb(const DICTIONARY_ITEM *item __maybe_unused, void *value, void *data __maybe_unused) { + struct rename_task *r = value; + + cgroup_netdev_release(r->cgroup_netdev_link); + rrdlabels_destroy(r->chart_labels); + freez((void *) r->container_name); + freez((void *) r->container_device); + freez((void *) r->ctx_prefix); +} + +void netdev_renames_init(void) { + static SPINLOCK spinlock = NETDATA_SPINLOCK_INITIALIZER; + + spinlock_lock(&spinlock); + if(!netdev_renames) { + netdev_renames = dictionary_create_advanced(DICT_OPTION_FIXED_SIZE, NULL, sizeof(struct rename_task)); + dictionary_register_delete_callback(netdev_renames, dictionary_netdev_rename_delete_cb, NULL); + } + spinlock_unlock(&spinlock); +} + +void cgroup_rename_task_add( + const char *host_device, + const char *container_device, + const char *container_name, + RRDLABELS *labels, + const char *ctx_prefix, + const DICTIONARY_ITEM *cgroup_netdev_link) +{ + netdev_renames_init(); + + struct rename_task tmp = { + .container_device = strdupz(container_device), + .container_name = strdupz(container_name), + .ctx_prefix = strdupz(ctx_prefix), + .chart_labels = rrdlabels_create(), + .cgroup_netdev_link = cgroup_netdev_link, + }; + rrdlabels_migrate_to_these(tmp.chart_labels, labels); + + dictionary_set(netdev_renames, host_device, &tmp, sizeof(tmp)); +} + +// other threads can call this function to delete a rename to a netdev +void cgroup_rename_task_device_del(const char *host_device) { + dictionary_del(netdev_renames, host_device); +} diff --git a/collectors/proc.plugin/proc_net_dev_renames.h b/collectors/proc.plugin/proc_net_dev_renames.h new file mode 100644 index 0000000000..51f3cfd94f --- /dev/null +++ b/collectors/proc.plugin/proc_net_dev_renames.h @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-3.0-or-later + +#ifndef NETDATA_PROC_NET_DEV_RENAMES_H +#define NETDATA_PROC_NET_DEV_RENAMES_H + +#include "plugin_proc.h" + +extern DICTIONARY *netdev_renames; + +struct rename_task { + const char *container_device; + const char *container_name; + const char *ctx_prefix; + RRDLABELS *chart_labels; + const DICTIONARY_ITEM *cgroup_netdev_link; +}; + +void netdev_renames_init(void); + +void cgroup_netdev_reset_all(void); +void cgroup_netdev_release(const DICTIONARY_ITEM *link); +const void *cgroup_netdev_dup(const DICTIONARY_ITEM *link); +void cgroup_netdev_add_bandwidth(const DICTIONARY_ITEM *link, NETDATA_DOUBLE received, NETDATA_DOUBLE sent); + + +#endif //NETDATA_PROC_NET_DEV_RENAMES_H |