From 20ca0f1376199648b1dece3a4b6b0f99e9ec9a9d Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sat, 31 Dec 2016 03:08:17 +0100 Subject: [PATCH 1/5] Eliminate memory leak in FreeBSD devstat collector The memory allocated by calloc was never freed. Since the devinfo struct never leaves the function, anyway, we might as well just allocate it on the stack. --- collector/devstat_freebsd.go | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/collector/devstat_freebsd.go b/collector/devstat_freebsd.go index e4be57ff..7ea5a22a 100644 --- a/collector/devstat_freebsd.go +++ b/collector/devstat_freebsd.go @@ -64,11 +64,8 @@ typedef struct { int _get_ndevs() { struct statinfo current; - int num_devices; - - current.dinfo = (struct devinfo *)calloc(1, sizeof(struct devinfo)); - if (current.dinfo == NULL) - return -2; + struct devinfo info = {}; + current.dinfo = &info; devstat_checkversion(NULL); @@ -80,12 +77,11 @@ int _get_ndevs() { Stats _get_stats(int i) { struct statinfo current; - int num_devices; + struct devinfo info = {}; + current.dinfo = &info; - current.dinfo = (struct devinfo *)calloc(1, sizeof(struct devinfo)); devstat_getdevs(NULL, ¤t); - num_devices = current.dinfo->numdevs; Stats stats; uint64_t bytes_read, bytes_write, bytes_free; uint64_t transfers_other, transfers_read, transfers_write, transfers_free; @@ -186,9 +182,6 @@ func (c *devstatCollector) Update(ch chan<- prometheus.Metric) (err error) { if count == -1 { return errors.New("devstat_getdevs() failed") } - if count == -2 { - return errors.New("calloc() failed") - } for i := C.int(0); i < count; i++ { stats := C._get_stats(i) From 5e220c1665e1cdbdb69cdcf6789f176ca907bd6e Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sat, 31 Dec 2016 04:15:25 +0100 Subject: [PATCH 2/5] Move cgo portions of FreeBSD devstat collector into own file Embedding 100 lines of code in a comment doesn't make for good reading, editing or code quality. --- collector/devstat_freebsd.c | 74 ++++++++++++++++++++++++ collector/devstat_freebsd.go | 106 +---------------------------------- collector/devstat_freebsd.h | 41 ++++++++++++++ 3 files changed, 117 insertions(+), 104 deletions(-) create mode 100644 collector/devstat_freebsd.c create mode 100644 collector/devstat_freebsd.h diff --git a/collector/devstat_freebsd.c b/collector/devstat_freebsd.c new file mode 100644 index 00000000..3a8e6d31 --- /dev/null +++ b/collector/devstat_freebsd.c @@ -0,0 +1,74 @@ +// +build !nodevstat + +#include +#include +#include +#include +#include +#include +#include +#include + +int _get_ndevs() { + struct statinfo current; + struct devinfo info = {}; + current.dinfo = &info; + + devstat_checkversion(NULL); + + if (devstat_getdevs(NULL, ¤t) == -1) + return -1; + + return current.dinfo->numdevs; +} + +Stats _get_stats(int i) { + struct statinfo current; + struct devinfo info = {}; + current.dinfo = &info; + + devstat_getdevs(NULL, ¤t); + + Stats stats; + uint64_t bytes_read, bytes_write, bytes_free; + uint64_t transfers_other, transfers_read, transfers_write, transfers_free; + long double duration_other, duration_read, duration_write, duration_free; + long double busy_time; + uint64_t blocks; + + strcpy(stats.device, current.dinfo->devices[i].device_name); + stats.unit = current.dinfo->devices[i].unit_number; + devstat_compute_statistics(¤t.dinfo->devices[i], + NULL, + 1.0, + DSM_TOTAL_BYTES_READ, &bytes_read, + DSM_TOTAL_BYTES_WRITE, &bytes_write, + DSM_TOTAL_BYTES_FREE, &bytes_free, + DSM_TOTAL_TRANSFERS_OTHER, &transfers_other, + DSM_TOTAL_TRANSFERS_READ, &transfers_read, + DSM_TOTAL_TRANSFERS_WRITE, &transfers_write, + DSM_TOTAL_TRANSFERS_FREE, &transfers_free, + DSM_TOTAL_DURATION_OTHER, &duration_other, + DSM_TOTAL_DURATION_READ, &duration_read, + DSM_TOTAL_DURATION_WRITE, &duration_write, + DSM_TOTAL_DURATION_FREE, &duration_free, + DSM_TOTAL_BUSY_TIME, &busy_time, + DSM_TOTAL_BLOCKS, &blocks, + DSM_NONE); + + stats.bytes.read = bytes_read; + stats.bytes.write = bytes_write; + stats.bytes.free = bytes_free; + stats.transfers.other = transfers_other; + stats.transfers.read = transfers_read; + stats.transfers.write = transfers_write; + stats.transfers.free = transfers_free; + stats.duration.other = duration_other; + stats.duration.read = duration_read; + stats.duration.write = duration_write; + stats.duration.free = duration_free; + stats.busyTime = busy_time; + stats.blocks = blocks; + + return stats; +} diff --git a/collector/devstat_freebsd.go b/collector/devstat_freebsd.go index 7ea5a22a..6f3b3e91 100644 --- a/collector/devstat_freebsd.go +++ b/collector/devstat_freebsd.go @@ -22,110 +22,8 @@ import ( "github.com/prometheus/client_golang/prometheus" ) -/* -#cgo LDFLAGS: -ldevstat -lkvm -#include -#include -#include -#include -#include -#include -#include - -typedef struct { - uint64_t read; - uint64_t write; - uint64_t free; -} Bytes; - -typedef struct { - uint64_t other; - uint64_t read; - uint64_t write; - uint64_t free; -} Transfers; - -typedef struct { - double other; - double read; - double write; - double free; -} Duration; - -typedef struct { - char device[DEVSTAT_NAME_LEN]; - int unit; - Bytes bytes; - Transfers transfers; - Duration duration; - long busyTime; - uint64_t blocks; -} Stats; - -int _get_ndevs() { - struct statinfo current; - struct devinfo info = {}; - current.dinfo = &info; - - devstat_checkversion(NULL); - - if (devstat_getdevs(NULL, ¤t) == -1) - return -1; - - return current.dinfo->numdevs; -} - -Stats _get_stats(int i) { - struct statinfo current; - struct devinfo info = {}; - current.dinfo = &info; - - devstat_getdevs(NULL, ¤t); - - Stats stats; - uint64_t bytes_read, bytes_write, bytes_free; - uint64_t transfers_other, transfers_read, transfers_write, transfers_free; - long double duration_other, duration_read, duration_write, duration_free; - long double busy_time; - uint64_t blocks; - - strcpy(stats.device, current.dinfo->devices[i].device_name); - stats.unit = current.dinfo->devices[i].unit_number; - devstat_compute_statistics(¤t.dinfo->devices[i], - NULL, - 1.0, - DSM_TOTAL_BYTES_READ, &bytes_read, - DSM_TOTAL_BYTES_WRITE, &bytes_write, - DSM_TOTAL_BYTES_FREE, &bytes_free, - DSM_TOTAL_TRANSFERS_OTHER, &transfers_other, - DSM_TOTAL_TRANSFERS_READ, &transfers_read, - DSM_TOTAL_TRANSFERS_WRITE, &transfers_write, - DSM_TOTAL_TRANSFERS_FREE, &transfers_free, - DSM_TOTAL_DURATION_OTHER, &duration_other, - DSM_TOTAL_DURATION_READ, &duration_read, - DSM_TOTAL_DURATION_WRITE, &duration_write, - DSM_TOTAL_DURATION_FREE, &duration_free, - DSM_TOTAL_BUSY_TIME, &busy_time, - DSM_TOTAL_BLOCKS, &blocks, - DSM_NONE); - - stats.bytes.read = bytes_read; - stats.bytes.write = bytes_write; - stats.bytes.free = bytes_free; - stats.transfers.other = transfers_other; - stats.transfers.read = transfers_read; - stats.transfers.write = transfers_write; - stats.transfers.free = transfers_free; - stats.duration.other = duration_other; - stats.duration.read = duration_read; - stats.duration.write = duration_write; - stats.duration.free = duration_free; - stats.busyTime = busy_time; - stats.blocks = blocks; - - return stats; -} -*/ +// #cgo LDFLAGS: -ldevstat -lkvm +// #include "devstat_freebsd.h" import "C" const ( diff --git a/collector/devstat_freebsd.h b/collector/devstat_freebsd.h new file mode 100644 index 00000000..13ea4bc6 --- /dev/null +++ b/collector/devstat_freebsd.h @@ -0,0 +1,41 @@ +#include +#include +#include +#include +#include +#include +#include + +typedef struct { + uint64_t read; + uint64_t write; + uint64_t free; +} Bytes; + +typedef struct { + uint64_t other; + uint64_t read; + uint64_t write; + uint64_t free; +} Transfers; + +typedef struct { + double other; + double read; + double write; + double free; +} Duration; + +typedef struct { + char device[DEVSTAT_NAME_LEN]; + int unit; + Bytes bytes; + Transfers transfers; + Duration duration; + long busyTime; + uint64_t blocks; +} Stats; + + +int _get_ndevs(); +Stats _get_stats(int i); From ea55d0f5cb8062f311891c20f2d5d11c62241cf8 Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sat, 31 Dec 2016 06:52:44 +0100 Subject: [PATCH 3/5] Don't race in FreeBSD devstat collector Querying the number of devices separately from the device list itself is racy. Devices may be added or removed between the two calls; and removed devices would lead to a segfault. --- collector/devstat_freebsd.c | 103 ++++++++++++++++------------------- collector/devstat_freebsd.go | 42 ++++++++------ collector/devstat_freebsd.h | 2 +- 3 files changed, 73 insertions(+), 74 deletions(-) diff --git a/collector/devstat_freebsd.c b/collector/devstat_freebsd.c index 3a8e6d31..80a24e41 100644 --- a/collector/devstat_freebsd.c +++ b/collector/devstat_freebsd.c @@ -9,66 +9,59 @@ #include #include -int _get_ndevs() { + +int _get_stats(Stats **stats) { struct statinfo current; struct devinfo info = {}; current.dinfo = &info; - devstat_checkversion(NULL); - - if (devstat_getdevs(NULL, ¤t) == -1) + if (devstat_getdevs(NULL, ¤t) == -1) { return -1; + } + Stats *p = (Stats*)calloc(current.dinfo->numdevs, sizeof(Stats)); + for (int i = 0; i < current.dinfo->numdevs; i++) { + uint64_t bytes_read, bytes_write, bytes_free; + uint64_t transfers_other, transfers_read, transfers_write, transfers_free; + long double duration_other, duration_read, duration_write, duration_free; + long double busy_time; + uint64_t blocks; + + strcpy(p[i].device, current.dinfo->devices[i].device_name); + p[i].unit = current.dinfo->devices[i].unit_number; + devstat_compute_statistics(¤t.dinfo->devices[i], + NULL, + 1.0, + DSM_TOTAL_BYTES_READ, &bytes_read, + DSM_TOTAL_BYTES_WRITE, &bytes_write, + DSM_TOTAL_BYTES_FREE, &bytes_free, + DSM_TOTAL_TRANSFERS_OTHER, &transfers_other, + DSM_TOTAL_TRANSFERS_READ, &transfers_read, + DSM_TOTAL_TRANSFERS_WRITE, &transfers_write, + DSM_TOTAL_TRANSFERS_FREE, &transfers_free, + DSM_TOTAL_DURATION_OTHER, &duration_other, + DSM_TOTAL_DURATION_READ, &duration_read, + DSM_TOTAL_DURATION_WRITE, &duration_write, + DSM_TOTAL_DURATION_FREE, &duration_free, + DSM_TOTAL_BUSY_TIME, &busy_time, + DSM_TOTAL_BLOCKS, &blocks, + DSM_NONE); + + p[i].bytes.read = bytes_read; + p[i].bytes.write = bytes_write; + p[i].bytes.free = bytes_free; + p[i].transfers.other = transfers_other; + p[i].transfers.read = transfers_read; + p[i].transfers.write = transfers_write; + p[i].transfers.free = transfers_free; + p[i].duration.other = duration_other; + p[i].duration.read = duration_read; + p[i].duration.write = duration_write; + p[i].duration.free = duration_free; + p[i].busyTime = busy_time; + p[i].blocks = blocks; + } + + *stats = p; return current.dinfo->numdevs; } - -Stats _get_stats(int i) { - struct statinfo current; - struct devinfo info = {}; - current.dinfo = &info; - - devstat_getdevs(NULL, ¤t); - - Stats stats; - uint64_t bytes_read, bytes_write, bytes_free; - uint64_t transfers_other, transfers_read, transfers_write, transfers_free; - long double duration_other, duration_read, duration_write, duration_free; - long double busy_time; - uint64_t blocks; - - strcpy(stats.device, current.dinfo->devices[i].device_name); - stats.unit = current.dinfo->devices[i].unit_number; - devstat_compute_statistics(¤t.dinfo->devices[i], - NULL, - 1.0, - DSM_TOTAL_BYTES_READ, &bytes_read, - DSM_TOTAL_BYTES_WRITE, &bytes_write, - DSM_TOTAL_BYTES_FREE, &bytes_free, - DSM_TOTAL_TRANSFERS_OTHER, &transfers_other, - DSM_TOTAL_TRANSFERS_READ, &transfers_read, - DSM_TOTAL_TRANSFERS_WRITE, &transfers_write, - DSM_TOTAL_TRANSFERS_FREE, &transfers_free, - DSM_TOTAL_DURATION_OTHER, &duration_other, - DSM_TOTAL_DURATION_READ, &duration_read, - DSM_TOTAL_DURATION_WRITE, &duration_write, - DSM_TOTAL_DURATION_FREE, &duration_free, - DSM_TOTAL_BUSY_TIME, &busy_time, - DSM_TOTAL_BLOCKS, &blocks, - DSM_NONE); - - stats.bytes.read = bytes_read; - stats.bytes.write = bytes_write; - stats.bytes.free = bytes_free; - stats.transfers.other = transfers_other; - stats.transfers.read = transfers_read; - stats.transfers.write = transfers_write; - stats.transfers.free = transfers_free; - stats.duration.other = duration_other; - stats.duration.read = duration_read; - stats.duration.write = duration_write; - stats.duration.free = duration_free; - stats.busyTime = busy_time; - stats.blocks = blocks; - - return stats; -} diff --git a/collector/devstat_freebsd.go b/collector/devstat_freebsd.go index 6f3b3e91..34bc53c8 100644 --- a/collector/devstat_freebsd.go +++ b/collector/devstat_freebsd.go @@ -18,6 +18,7 @@ package collector import ( "errors" "fmt" + "unsafe" "github.com/prometheus/client_golang/prometheus" ) @@ -75,25 +76,30 @@ func NewDevstatCollector() (Collector, error) { }, nil } -func (c *devstatCollector) Update(ch chan<- prometheus.Metric) (err error) { - count := C._get_ndevs() - if count == -1 { - return errors.New("devstat_getdevs() failed") +func (c *devstatCollector) Update(ch chan<- prometheus.Metric) error { + var stats *C.Stats + n := C._get_stats(&stats) + if n == -1 { + return errors.New("devstat_getdevs failed") } - for i := C.int(0); i < count; i++ { - stats := C._get_stats(i) - device := fmt.Sprintf("%s%d", C.GoString(&stats.device[0]), stats.unit) - ch <- c.bytes.mustNewConstMetric(float64(stats.bytes.read), device, "read") - ch <- c.bytes.mustNewConstMetric(float64(stats.bytes.write), device, "write") - ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.other), device, "other") - ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.read), device, "read") - ch <- c.transfers.mustNewConstMetric(float64(stats.transfers.write), device, "write") - ch <- c.duration.mustNewConstMetric(float64(stats.duration.other), device, "other") - ch <- c.duration.mustNewConstMetric(float64(stats.duration.read), device, "read") - ch <- c.duration.mustNewConstMetric(float64(stats.duration.write), device, "write") - ch <- c.busyTime.mustNewConstMetric(float64(stats.busyTime), device) - ch <- c.blocks.mustNewConstMetric(float64(stats.blocks), device) + base := unsafe.Pointer(stats) + for i := C.int(0); i < n; i++ { + offset := i * C.int(C.sizeof_Stats) + stat := (*C.Stats)(unsafe.Pointer(uintptr(base) + uintptr(offset))) + + device := fmt.Sprintf("%s%d", C.GoString(&stat.device[0]), stat.unit) + ch <- c.bytes.mustNewConstMetric(float64(stat.bytes.read), device, "read") + ch <- c.bytes.mustNewConstMetric(float64(stat.bytes.write), device, "write") + ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.other), device, "other") + ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.read), device, "read") + ch <- c.transfers.mustNewConstMetric(float64(stat.transfers.write), device, "write") + ch <- c.duration.mustNewConstMetric(float64(stat.duration.other), device, "other") + ch <- c.duration.mustNewConstMetric(float64(stat.duration.read), device, "read") + ch <- c.duration.mustNewConstMetric(float64(stat.duration.write), device, "write") + ch <- c.busyTime.mustNewConstMetric(float64(stat.busyTime), device) + ch <- c.blocks.mustNewConstMetric(float64(stat.blocks), device) } - return err + C.free(unsafe.Pointer(stats)) + return nil } diff --git a/collector/devstat_freebsd.h b/collector/devstat_freebsd.h index 13ea4bc6..87f09f0b 100644 --- a/collector/devstat_freebsd.h +++ b/collector/devstat_freebsd.h @@ -38,4 +38,4 @@ typedef struct { int _get_ndevs(); -Stats _get_stats(int i); +int _get_stats(Stats **stats); From 38c589042815e09ce3bda631f3e244f952ed1721 Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Sat, 31 Dec 2016 07:23:55 +0100 Subject: [PATCH 4/5] Reuse devinfo struct The devstat API expects us to reuse one devinfo for many invocations of devstat_getstats. In particular, it allocates and resizes memory referenced by devinfo. --- collector/devstat_freebsd.c | 5 ++--- collector/devstat_freebsd.go | 10 +++++++++- collector/devstat_freebsd.h | 2 +- 3 files changed, 12 insertions(+), 5 deletions(-) diff --git a/collector/devstat_freebsd.c b/collector/devstat_freebsd.c index 80a24e41..c3352e2c 100644 --- a/collector/devstat_freebsd.c +++ b/collector/devstat_freebsd.c @@ -10,10 +10,9 @@ #include -int _get_stats(Stats **stats) { +int _get_stats(struct devinfo *info, Stats **stats) { struct statinfo current; - struct devinfo info = {}; - current.dinfo = &info; + current.dinfo = info; if (devstat_getdevs(NULL, ¤t) == -1) { return -1; diff --git a/collector/devstat_freebsd.go b/collector/devstat_freebsd.go index 34bc53c8..79176026 100644 --- a/collector/devstat_freebsd.go +++ b/collector/devstat_freebsd.go @@ -18,6 +18,7 @@ package collector import ( "errors" "fmt" + "sync" "unsafe" "github.com/prometheus/client_golang/prometheus" @@ -32,6 +33,9 @@ const ( ) type devstatCollector struct { + mu sync.Mutex + devinfo *C.struct_devinfo + bytes typedDesc bytes_total typedDesc transfers typedDesc @@ -48,6 +52,7 @@ func init() { // Device stats. func NewDevstatCollector() (Collector, error) { return &devstatCollector{ + devinfo: &C.struct_devinfo{}, bytes: typedDesc{prometheus.NewDesc( prometheus.BuildFQName(Namespace, devstatSubsystem, "bytes_total"), "The total number of bytes in transactions.", @@ -77,8 +82,11 @@ func NewDevstatCollector() (Collector, error) { } func (c *devstatCollector) Update(ch chan<- prometheus.Metric) error { + c.mu.Lock() + defer c.mu.Unlock() + var stats *C.Stats - n := C._get_stats(&stats) + n := C._get_stats(c.devinfo, &stats) if n == -1 { return errors.New("devstat_getdevs failed") } diff --git a/collector/devstat_freebsd.h b/collector/devstat_freebsd.h index 87f09f0b..4ee1c62d 100644 --- a/collector/devstat_freebsd.h +++ b/collector/devstat_freebsd.h @@ -38,4 +38,4 @@ typedef struct { int _get_ndevs(); -int _get_stats(Stats **stats); +int _get_stats(struct devinfo *info, Stats **stats); From 9847257bc0626f74599e730b03e0219a063120cc Mon Sep 17 00:00:00 2001 From: Dominik Honnef Date: Thu, 5 Jan 2017 06:18:34 +0100 Subject: [PATCH 5/5] Add missing license headers --- collector/devstat_freebsd.c | 13 +++++++++++++ collector/devstat_freebsd.h | 13 +++++++++++++ 2 files changed, 26 insertions(+) diff --git a/collector/devstat_freebsd.c b/collector/devstat_freebsd.c index c3352e2c..43539b77 100644 --- a/collector/devstat_freebsd.c +++ b/collector/devstat_freebsd.c @@ -1,3 +1,16 @@ +// Copyright 2017 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + // +build !nodevstat #include diff --git a/collector/devstat_freebsd.h b/collector/devstat_freebsd.h index 4ee1c62d..b0cd4a1e 100644 --- a/collector/devstat_freebsd.h +++ b/collector/devstat_freebsd.h @@ -1,3 +1,16 @@ +// Copyright 2017 The Prometheus Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + #include #include #include