[bpf-next 0/3] Using log2 histogram to display the statistics of every softirq execution

[bpf-next 0/3] Using log2 histogram to display the statistics of every softirq execution

[Xin Hao]

The purpose of this series of patches is to provide a way of log2
histogram to display in sample/bpf dir, you can refer to the
patch three to use it.

Xin Hao (3):
  sample/bpf: Avoid repetitive definition of log2 func
  sample/bpf: Add log2 histogram function support
  samples/bpf: Add soft irq execution time statistics

 samples/bpf/Makefile            |  3 ++
 samples/bpf/common.h            | 67 +++++++++++++++++++++++++
 samples/bpf/common_kern.h       | 30 +++++++++++
 samples/bpf/lathist_kern.c      | 25 +---------
 samples/bpf/lwt_len_hist_kern.c | 23 +--------
 samples/bpf/soft_irq_kern.c     | 87 ++++++++++++++++++++++++++++++++
 samples/bpf/soft_irq_user.c     | 88 +++++++++++++++++++++++++++++++++
 samples/bpf/tracex2_kern.c      | 23 +--------
 8 files changed, 278 insertions(+), 68 deletions(-)
 create mode 100644 samples/bpf/common.h
 create mode 100644 samples/bpf/common_kern.h
 create mode 100644 samples/bpf/soft_irq_kern.c
 create mode 100644 samples/bpf/soft_irq_user.c

--
2.28.0

[bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func

[Xin Hao]

log2 func is defined and used in following three files:
    samples/bpf/lathist_kern.c
    samples/bpf/lwt_len_hist_kern.c
    samples/bpf/tracex2_kern.c

There's no need to repeat define them many times, so i added
a "common.h" file which maintains common codes, you just need
to include it in your file and future we can put more common codes
into this file.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 samples/bpf/common_kern.h       | 30 ++++++++++++++++++++++++++++++
 samples/bpf/lathist_kern.c      | 25 +------------------------
 samples/bpf/lwt_len_hist_kern.c | 23 +----------------------
 samples/bpf/tracex2_kern.c      | 23 +----------------------
 4 files changed, 33 insertions(+), 68 deletions(-)
 create mode 100644 samples/bpf/common_kern.h

diff --git a/samples/bpf/common_kern.h b/samples/bpf/common_kern.h
new file mode 100644
index 000000000000..c23b44238d02
--- /dev/null
+++ b/samples/bpf/common_kern.h
@@ -0,0 +1,30 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+
+static unsigned int log2(unsigned int v)
+{
+	unsigned int r;
+	unsigned int shift;
+
+	r = (v > 0xFFFF) << 4; v >>= r;
+	shift = (v > 0xFF) << 3; v >>= shift; r |= shift;
+	shift = (v > 0xF) << 2; v >>= shift; r |= shift;
+	shift = (v > 0x3) << 1; v >>= shift; r |= shift;
+	r |= (v >> 1);
+
+	return r;
+}
+
+static unsigned int log2l(unsigned long v)
+{
+	unsigned int hi = v >> 32;
+
+	if (hi)
+		return log2(hi) + 32;
+	else
+		return log2(v);
+}
diff --git a/samples/bpf/lathist_kern.c b/samples/bpf/lathist_kern.c
index ca9c2e4e69aa..0978e24dd01c 100644
--- a/samples/bpf/lathist_kern.c
+++ b/samples/bpf/lathist_kern.c
@@ -9,6 +9,7 @@
 #include <linux/ptrace.h>
 #include <uapi/linux/bpf.h>
 #include <bpf/bpf_helpers.h>
+#include "common_kern.h"
 
 #define MAX_ENTRIES	20
 #define MAX_CPU		4
@@ -37,30 +38,6 @@ int bpf_prog1(struct pt_regs *ctx)
 	return 0;
 }
 
-static unsigned int log2(unsigned int v)
-{
-	unsigned int r;
-	unsigned int shift;
-
-	r = (v > 0xFFFF) << 4; v >>= r;
-	shift = (v > 0xFF) << 3; v >>= shift; r |= shift;
-	shift = (v > 0xF) << 2; v >>= shift; r |= shift;
-	shift = (v > 0x3) << 1; v >>= shift; r |= shift;
-	r |= (v >> 1);
-
-	return r;
-}
-
-static unsigned int log2l(unsigned long v)
-{
-	unsigned int hi = v >> 32;
-
-	if (hi)
-		return log2(hi) + 32;
-	else
-		return log2(v);
-}
-
 struct bpf_map_def SEC("maps") my_lat = {
 	.type = BPF_MAP_TYPE_ARRAY,
 	.key_size = sizeof(int),
diff --git a/samples/bpf/lwt_len_hist_kern.c b/samples/bpf/lwt_len_hist_kern.c
index 9ed63e10e170..6e61072d602b 100644
--- a/samples/bpf/lwt_len_hist_kern.c
+++ b/samples/bpf/lwt_len_hist_kern.c
@@ -15,6 +15,7 @@
 #include <uapi/linux/ip.h>
 #include <uapi/linux/in.h>
 #include <bpf/bpf_helpers.h>
+#include "common_kern.h"
 
 # define printk(fmt, ...)						\
 		({							\
@@ -41,28 +42,6 @@ struct bpf_elf_map SEC("maps") lwt_len_hist_map = {
 	.max_elem = 1024,
 };
 
-static unsigned int log2(unsigned int v)
-{
-	unsigned int r;
-	unsigned int shift;
-
-	r = (v > 0xFFFF) << 4; v >>= r;
-	shift = (v > 0xFF) << 3; v >>= shift; r |= shift;
-	shift = (v > 0xF) << 2; v >>= shift; r |= shift;
-	shift = (v > 0x3) << 1; v >>= shift; r |= shift;
-	r |= (v >> 1);
-	return r;
-}
-
-static unsigned int log2l(unsigned long v)
-{
-	unsigned int hi = v >> 32;
-	if (hi)
-		return log2(hi) + 32;
-	else
-		return log2(v);
-}
-
 SEC("len_hist")
 int do_len_hist(struct __sk_buff *skb)
 {
diff --git a/samples/bpf/tracex2_kern.c b/samples/bpf/tracex2_kern.c
index 5bc696bac27d..7a8cda216d54 100644
--- a/samples/bpf/tracex2_kern.c
+++ b/samples/bpf/tracex2_kern.c
@@ -11,6 +11,7 @@
 #include <bpf/bpf_helpers.h>
 #include <bpf/bpf_tracing.h>
 #include "trace_common.h"
+#include "common_kern.h"
 
 struct {
 	__uint(type, BPF_MAP_TYPE_HASH);
@@ -42,28 +43,6 @@ int bpf_prog2(struct pt_regs *ctx)
 	return 0;
 }
 
-static unsigned int log2(unsigned int v)
-{
-	unsigned int r;
-	unsigned int shift;
-
-	r = (v > 0xFFFF) << 4; v >>= r;
-	shift = (v > 0xFF) << 3; v >>= shift; r |= shift;
-	shift = (v > 0xF) << 2; v >>= shift; r |= shift;
-	shift = (v > 0x3) << 1; v >>= shift; r |= shift;
-	r |= (v >> 1);
-	return r;
-}
-
-static unsigned int log2l(unsigned long v)
-{
-	unsigned int hi = v >> 32;
-	if (hi)
-		return log2(hi) + 32;
-	else
-		return log2(v);
-}
-
 struct hist_key {
 	char comm[16];
 	u64 pid_tgid;
-- 
2.28.0

[bpf-next 2/3] sample/bpf: Add log2 histogram function support

[Xin Hao]

The relative functions is copy from bcc tools
source code: libbpf-tools/trace_helpers.c.
URL: https://github.com/iovisor/bcc.git

Log2 histogram can display the change of the collected
data more conveniently.

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 samples/bpf/common.h | 67 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)
 create mode 100644 samples/bpf/common.h

diff --git a/samples/bpf/common.h b/samples/bpf/common.h
new file mode 100644
index 000000000000..ec60fb665544
--- /dev/null
+++ b/samples/bpf/common.h
@@ -0,0 +1,67 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+
+#define min(x, y) ({				 \
+	typeof(x) _min1 = (x);			 \
+	typeof(y) _min2 = (y);			 \
+	(void) (&_min1 == &_min2);		 \
+	_min1 < _min2 ? _min1 : _min2; })
+
+static void print_stars(unsigned int val, unsigned int val_max, int width)
+{
+	int num_stars, num_spaces, i;
+	bool need_plus;
+
+	num_stars = min(val, val_max) * width / val_max;
+	num_spaces = width - num_stars;
+	need_plus = val > val_max;
+
+	for (i = 0; i < num_stars; i++)
+		printf("*");
+	for (i = 0; i < num_spaces; i++)
+		printf(" ");
+	if (need_plus)
+		printf("+");
+}
+
+static void print_log2_hist(unsigned int *vals, int vals_size, char *val_type)
+{
+	int stars_max = 40, idx_max = -1;
+	unsigned int val, val_max = 0;
+	unsigned long long low, high;
+	int stars, width, i;
+
+	for (i = 0; i < vals_size; i++) {
+		val = vals[i];
+		if (val > 0)
+			idx_max = i;
+		if (val > val_max)
+			val_max = val;
+	}
+
+	if (idx_max < 0)
+		return;
+
+	printf("%*s%-*s : count    distribution\n", idx_max <= 32 ? 5 : 15, "",
+		idx_max <= 32 ? 19 : 29, val_type);
+	if (idx_max <= 32)
+		stars = stars_max;
+	else
+		stars = stars_max / 2;
+
+	for (i = 0; i <= idx_max; i++) {
+		low = (1ULL << (i + 1)) >> 1;
+		high = (1ULL << (i + 1)) - 1;
+		if (low == high)
+			low -= 1;
+		val = vals[i];
+		width = idx_max <= 32 ? 10 : 20;
+		printf("%*lld -> %-*lld : %-8d |", width, low, width, high, val);
+		print_stars(val, val_max, stars);
+		printf("|\n");
+	}
+}
-- 
2.28.0

[bpf-next 3/3] samples/bpf: Add soft irq execution time statistics

[Xin Hao]

This patch is aimed to count the execution time of
each soft irq and it supports log2 histogram display.

Soft irq counts:
     us	             : count    distribution

     0 -> 1	     : 151      |****************************************|
     2 -> 3          : 86       |**********************                  |
     4 -> 7          : 59       |***************                         |
     8 -> 15         : 20       |*****                                   |
    16 -> 31         : 3        |			                 |

Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
---
 samples/bpf/Makefile        |  3 ++
 samples/bpf/soft_irq_kern.c | 87 ++++++++++++++++++++++++++++++++++++
 samples/bpf/soft_irq_user.c | 88 +++++++++++++++++++++++++++++++++++++
 3 files changed, 178 insertions(+)
 create mode 100644 samples/bpf/soft_irq_kern.c
 create mode 100644 samples/bpf/soft_irq_user.c

diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index f87ee02073ba..0774f0fb8bdf 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -53,6 +53,7 @@ tprogs-y += task_fd_query
 tprogs-y += xdp_sample_pkts
 tprogs-y += ibumad
 tprogs-y += hbm
+tprogs-y += soft_irq
 
 # Libbpf dependencies
 LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
@@ -109,6 +110,7 @@ task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS)
 xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS)
 ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS)
 hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
+soft_irq-objs := bpf_load.o soft_irq_user.o $(TRACE_HELPERS)
 
 # Tell kbuild to always build the programs
 always-y := $(tprogs-y)
@@ -170,6 +172,7 @@ always-y += ibumad_kern.o
 always-y += hbm_out_kern.o
 always-y += hbm_edt_kern.o
 always-y += xdpsock_kern.o
+always-y += soft_irq_kern.o
 
 ifeq ($(ARCH), arm)
 # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
diff --git a/samples/bpf/soft_irq_kern.c b/samples/bpf/soft_irq_kern.c
new file mode 100644
index 000000000000..e63f829cf7c7
--- /dev/null
+++ b/samples/bpf/soft_irq_kern.c
@@ -0,0 +1,87 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+
+#include <uapi/linux/bpf.h>
+#include <bpf/bpf_helpers.h>
+#include <bpf/bpf_tracing.h>
+#include <uapi/linux/ptrace.h>
+#include <uapi/linux/perf_event.h>
+#include <linux/version.h>
+#include <linux/sched.h>
+#include "common_kern.h"
+
+typedef struct key {
+	u32 pid;
+	u32 cpu;
+} irqkey_t;
+
+typedef struct val {
+	u64 ts;
+	u32 vec;
+} val_t;
+
+typedef struct delta_irq {
+	u64 delta;
+    u32 value;
+} delta_irq_t;
+
+struct bpf_map_def SEC("maps") start = {
+	.type = BPF_MAP_TYPE_HASH,
+	.key_size = sizeof(irqkey_t),
+	.value_size = sizeof(val_t),
+	.max_entries = 1000,
+};
+
+struct soft_irq {
+	u64 pad;
+    u32 vec;
+};
+SEC("tracepoint/irq/softirq_entry")
+int entryirq(struct soft_irq *ctx)
+{
+    irqkey_t entry_key = {};
+	val_t val = {};
+
+	entry_key.pid = bpf_get_current_pid_tgid();
+    entry_key.cpu = bpf_get_smp_processor_id();
+
+	val.ts = bpf_ktime_get_ns();
+	val.vec = ctx->vec;
+
+	bpf_map_update_elem(&start, &entry_key, &val, BPF_ANY);
+	return 0;
+}
+
+struct bpf_map_def SEC("maps") over = {
+	.type = BPF_MAP_TYPE_HASH,
+	.key_size = sizeof(irqkey_t),
+	.value_size = sizeof(delta_irq_t),
+	.max_entries = 10000,
+};
+SEC("tracepoint/irq/softirq_exit")
+int exitirq(struct soft_irq *ctx)
+{
+    irqkey_t entry_key = {};
+	delta_irq_t delta_val = {};
+	val_t *valp;
+
+	entry_key.pid = bpf_get_current_pid_tgid();
+    entry_key.cpu = bpf_get_smp_processor_id();
+
+	valp = bpf_map_lookup_elem(&start, &entry_key);
+	if (valp) {
+		delta_val.delta = (bpf_ktime_get_ns() - valp->ts) / 1000; /* us */
+		delta_val.value = log2l(delta_val.delta);
+
+		bpf_map_update_elem(&over, &entry_key, &delta_val, BPF_ANY);
+		bpf_map_delete_elem(&start, valp);
+	}
+
+	return 0;
+}
+char _license[] SEC("license") = "GPL";
+u32 _version SEC("version") = LINUX_VERSION_CODE;
diff --git a/samples/bpf/soft_irq_user.c b/samples/bpf/soft_irq_user.c
new file mode 100644
index 000000000000..1bb338eb337c
--- /dev/null
+++ b/samples/bpf/soft_irq_user.c
@@ -0,0 +1,88 @@
+/* SPDX-License-Identifier: GPL-2.0
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <stdlib.h>
+#include <signal.h>
+#include <linux/bpf.h>
+#include <string.h>
+#include <linux/perf_event.h>
+#include <errno.h>
+#include <assert.h>
+#include <stdbool.h>
+#include <sys/resource.h>
+#include <bpf/libbpf.h>
+#include "bpf_load.h"
+#include "trace_helpers.h"
+#include "common.h"
+
+#define MAX_SLOT_LEN 40
+
+struct hist {
+	unsigned int slots[MAX_SLOT_LEN];
+};
+
+struct hist new_hist;
+
+typedef struct delta_irq {
+	__u64 delta;
+    __u32 value;
+} delta_irq_t;
+
+typedef struct key {
+	__u32 pid;
+	__u32 cpu;
+} irqkey_t;
+
+static void print_hist(int fd)
+{
+    irqkey_t entry_key = {}, next_key;
+	delta_irq_t delta1;
+
+	printf("Soft irq counts: \n");
+	while (bpf_map_get_next_key(fd, &entry_key, &next_key) == 0) {
+		bpf_map_lookup_elem(fd, &next_key, &delta1);
+		new_hist.slots[delta1.value] += 1;
+		entry_key = next_key;
+	}
+	print_log2_hist(new_hist.slots, MAX_SLOT_LEN, "us");
+}
+
+static void int_exit(int sig)
+{
+	print_hist(map_fd[1]);
+	exit(0);
+}
+
+int main(int argc, char **argv)
+{
+	int i = 0;
+	struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+	char filename[256];
+
+	memset(&new_hist, 0, sizeof(struct hist));
+
+	snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
+	setrlimit(RLIMIT_MEMLOCK, &r);
+
+	signal(SIGINT, int_exit);
+	signal(SIGTERM, int_exit);
+
+	if (load_bpf_file(filename)) {
+		printf("%s", bpf_log_buf);
+		return 1;
+	}
+
+	while (i < 10)
+	{
+		sleep(5);
+		print_hist(map_fd[1]);
+		i++;
+	}
+	return 0;
+}
-- 
2.28.0

RE: [bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func

[John Fastabend]

Xin Hao wrote:
> log2 func is defined and used in following three files:
>     samples/bpf/lathist_kern.c
>     samples/bpf/lwt_len_hist_kern.c
>     samples/bpf/tracex2_kern.c
> 
> There's no need to repeat define them many times, so i added
> a "common.h" file which maintains common codes, you just need
> to include it in your file and future we can put more common codes
> into this file.
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---

LGTM

Acked-by: John Fastabend <john.fastabend@gmail.com>

RE: [bpf-next 2/3] sample/bpf: Add log2 histogram function support

[John Fastabend]

Xin Hao wrote:
> The relative functions is copy from bcc tools
> source code: libbpf-tools/trace_helpers.c.
> URL: https://github.com/iovisor/bcc.git
> 
> Log2 histogram can display the change of the collected
> data more conveniently.
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---
>  samples/bpf/common.h | 67 ++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 67 insertions(+)
>  create mode 100644 samples/bpf/common.h
> 
> diff --git a/samples/bpf/common.h b/samples/bpf/common.h
> new file mode 100644
> index 000000000000..ec60fb665544
> --- /dev/null
> +++ b/samples/bpf/common.h
> @@ -0,0 +1,67 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + */
> +

nit, for this patch and the last one we don't need the text. Just the SPDX
identifier should be enough. Its at least in line with everything we have
elsewhere.

Also if there is a copyright on that original file we should pull it over
as far as I understand it. I don't see anything there though so maybe
not.

> +#define min(x, y) ({				 \
> +	typeof(x) _min1 = (x);			 \
> +	typeof(y) _min2 = (y);			 \
> +	(void) (&_min1 == &_min2);		 \
> +	_min1 < _min2 ? _min1 : _min2; })

What was wrong with 'min(a,b) ((a) < (b) ? (a) : (b))' looks like
below its just used for comparing two unsigned ints?

Thanks.

> +
> +static void print_stars(unsigned int val, unsigned int val_max, int width)
> +{
> +	int num_stars, num_spaces, i;
> +	bool need_plus;
> +
> +	num_stars = min(val, val_max) * width / val_max;
> +	num_spaces = width - num_stars;
> +	need_plus = val > val_max;
> +
> +	for (i = 0; i < num_stars; i++)
> +		printf("*");
> +	for (i = 0; i < num_spaces; i++)
> +		printf(" ");
> +	if (need_plus)
> +		printf("+");
> +}
> +
> +static void print_log2_hist(unsigned int *vals, int vals_size, char *val_type)
> +{
> +	int stars_max = 40, idx_max = -1;
> +	unsigned int val, val_max = 0;
> +	unsigned long long low, high;
> +	int stars, width, i;
> +
> +	for (i = 0; i < vals_size; i++) {
> +		val = vals[i];
> +		if (val > 0)
> +			idx_max = i;
> +		if (val > val_max)
> +			val_max = val;
> +	}
> +
> +	if (idx_max < 0)
> +		return;
> +
> +	printf("%*s%-*s : count    distribution\n", idx_max <= 32 ? 5 : 15, "",
> +		idx_max <= 32 ? 19 : 29, val_type);
> +	if (idx_max <= 32)
> +		stars = stars_max;
> +	else
> +		stars = stars_max / 2;
> +
> +	for (i = 0; i <= idx_max; i++) {
> +		low = (1ULL << (i + 1)) >> 1;
> +		high = (1ULL << (i + 1)) - 1;
> +		if (low == high)
> +			low -= 1;
> +		val = vals[i];
> +		width = idx_max <= 32 ? 10 : 20;
> +		printf("%*lld -> %-*lld : %-8d |", width, low, width, high, val);
> +		print_stars(val, val_max, stars);
> +		printf("|\n");
> +	}
> +}
> -- 
> 2.28.0
> 

RE: [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics

[John Fastabend]

Xin Hao wrote:
> This patch is aimed to count the execution time of
> each soft irq and it supports log2 histogram display.
> 
> Soft irq counts:
>      us	             : count    distribution
> 
>      0 -> 1	     : 151      |****************************************|
>      2 -> 3          : 86       |**********************                  |
>      4 -> 7          : 59       |***************                         |
>      8 -> 15         : 20       |*****                                   |
>     16 -> 31         : 3        |			                 |
> 
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>

Couple nits otherwise LGTM.

> ---

[...]

> +
> +typedef struct key {
> +	u32 pid;
> +	u32 cpu;
> +} irqkey_t;
> +
> +typedef struct val {
> +	u64 ts;
> +	u32 vec;
> +} val_t;
> +
> +typedef struct delta_irq {
> +	u64 delta;
> +    u32 value;

should be a tab

> +} delta_irq_t;
> +
> +struct bpf_map_def SEC("maps") start = {
> +	.type = BPF_MAP_TYPE_HASH,
> +	.key_size = sizeof(irqkey_t),
> +	.value_size = sizeof(val_t),
> +	.max_entries = 1000,
> +};

Seems more common to use the style,

struct {
	__uint(type, ...);
	__uint(max_entries,...);
	...

} start SEC{".maps");

> +
> +struct soft_irq {
> +	u64 pad;
> +    u32 vec;

spaces -> tabs, probably run ./contrib/scripts/checkpatch on the rest and
cleanup the other cases as well.

[...]

Re: [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics

[Andrii Nakryiko]

On Sun, Sep 20, 2020 at 7:47 AM Xin Hao <xhao@linux.alibaba.com> wrote:
>
> This patch is aimed to count the execution time of
> each soft irq and it supports log2 histogram display.
>
> Soft irq counts:
>      us              : count    distribution
>
>      0 -> 1          : 151      |****************************************|
>      2 -> 3          : 86       |**********************                  |
>      4 -> 7          : 59       |***************                         |
>      8 -> 15         : 20       |*****                                   |
>     16 -> 31         : 3        |                                        |
>
> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> ---

I assume you saw softirqs tools ([0]) from libbpf-tools in BCC, right?
Could you mention in the commit message how this tool is different and
why there is a need for another one?

BTW, if you are interested in contributing more and making samples/bpf
better overall, consider looking at adding BPF skeleton integration
into samples/bpf, similarly to how all libbpf-tools use skeletons.

  [0] https://github.com/iovisor/bcc/pull/3076

>  samples/bpf/Makefile        |  3 ++
>  samples/bpf/soft_irq_kern.c | 87 ++++++++++++++++++++++++++++++++++++
>  samples/bpf/soft_irq_user.c | 88 +++++++++++++++++++++++++++++++++++++
>  3 files changed, 178 insertions(+)
>  create mode 100644 samples/bpf/soft_irq_kern.c
>  create mode 100644 samples/bpf/soft_irq_user.c
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index f87ee02073ba..0774f0fb8bdf 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -53,6 +53,7 @@ tprogs-y += task_fd_query
>  tprogs-y += xdp_sample_pkts
>  tprogs-y += ibumad
>  tprogs-y += hbm
> +tprogs-y += soft_irq
>
>  # Libbpf dependencies
>  LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
> @@ -109,6 +110,7 @@ task_fd_query-objs := bpf_load.o task_fd_query_user.o $(TRACE_HELPERS)
>  xdp_sample_pkts-objs := xdp_sample_pkts_user.o $(TRACE_HELPERS)
>  ibumad-objs := bpf_load.o ibumad_user.o $(TRACE_HELPERS)
>  hbm-objs := bpf_load.o hbm.o $(CGROUP_HELPERS)
> +soft_irq-objs := bpf_load.o soft_irq_user.o $(TRACE_HELPERS)
>
>  # Tell kbuild to always build the programs
>  always-y := $(tprogs-y)
> @@ -170,6 +172,7 @@ always-y += ibumad_kern.o
>  always-y += hbm_out_kern.o
>  always-y += hbm_edt_kern.o
>  always-y += xdpsock_kern.o
> +always-y += soft_irq_kern.o
>
>  ifeq ($(ARCH), arm)
>  # Strip all except -D__LINUX_ARM_ARCH__ option needed to handle linux
> diff --git a/samples/bpf/soft_irq_kern.c b/samples/bpf/soft_irq_kern.c
> new file mode 100644
> index 000000000000..e63f829cf7c7
> --- /dev/null
> +++ b/samples/bpf/soft_irq_kern.c
> @@ -0,0 +1,87 @@
> +/* SPDX-License-Identifier: GPL-2.0
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + */
> +
> +#include <uapi/linux/bpf.h>
> +#include <bpf/bpf_helpers.h>
> +#include <bpf/bpf_tracing.h>
> +#include <uapi/linux/ptrace.h>
> +#include <uapi/linux/perf_event.h>
> +#include <linux/version.h>
> +#include <linux/sched.h>
> +#include "common_kern.h"
> +
> +typedef struct key {
> +       u32 pid;
> +       u32 cpu;
> +} irqkey_t;
> +
> +typedef struct val {
> +       u64 ts;
> +       u32 vec;
> +} val_t;
> +
> +typedef struct delta_irq {
> +       u64 delta;
> +    u32 value;
> +} delta_irq_t;

kernel style generally discourages unnecessary typedefs, I think
following that is a good idea. I'm not sure those typedefs provide
much value and they save only few letters when typing.

> +
> +struct bpf_map_def SEC("maps") start = {
> +       .type = BPF_MAP_TYPE_HASH,
> +       .key_size = sizeof(irqkey_t),
> +       .value_size = sizeof(val_t),
> +       .max_entries = 1000,
> +};
> +
> +struct soft_irq {
> +       u64 pad;
> +    u32 vec;
> +};


it's a good idea to keep empty line between type declarations and
variables/functions

> +SEC("tracepoint/irq/softirq_entry")
> +int entryirq(struct soft_irq *ctx)
> +{
> +    irqkey_t entry_key = {};
> +       val_t val = {};
> +
> +       entry_key.pid = bpf_get_current_pid_tgid();
> +    entry_key.cpu = bpf_get_smp_processor_id();
> +
> +       val.ts = bpf_ktime_get_ns();
> +       val.vec = ctx->vec;
> +
> +       bpf_map_update_elem(&start, &entry_key, &val, BPF_ANY);
> +       return 0;
> +}
> +
> +struct bpf_map_def SEC("maps") over = {
> +       .type = BPF_MAP_TYPE_HASH,
> +       .key_size = sizeof(irqkey_t),
> +       .value_size = sizeof(delta_irq_t),
> +       .max_entries = 10000,
> +};

same

> +SEC("tracepoint/irq/softirq_exit")
> +int exitirq(struct soft_irq *ctx)
> +{
> +    irqkey_t entry_key = {};
> +       delta_irq_t delta_val = {};
> +       val_t *valp;
> +
> +       entry_key.pid = bpf_get_current_pid_tgid();
> +    entry_key.cpu = bpf_get_smp_processor_id();
> +
> +       valp = bpf_map_lookup_elem(&start, &entry_key);
> +       if (valp) {
> +               delta_val.delta = (bpf_ktime_get_ns() - valp->ts) / 1000; /* us */
> +               delta_val.value = log2l(delta_val.delta);
> +
> +               bpf_map_update_elem(&over, &entry_key, &delta_val, BPF_ANY);
> +               bpf_map_delete_elem(&start, valp);
> +       }
> +
> +       return 0;
> +}

here as well

> +char _license[] SEC("license") = "GPL";
> +u32 _version SEC("version") = LINUX_VERSION_CODE;

[...]

Re: [bpf-next 2/3] sample/bpf: Add log2 histogram function support

[Andrii Nakryiko]

On Mon, Sep 21, 2020 at 10:18 AM John Fastabend
<john.fastabend@gmail.com> wrote:
>
> Xin Hao wrote:
> > The relative functions is copy from bcc tools

you probably meant relevant, not relative?

> > source code: libbpf-tools/trace_helpers.c.
> > URL: https://github.com/iovisor/bcc.git
> >
> > Log2 histogram can display the change of the collected
> > data more conveniently.
> >
> > Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> > ---
> >  samples/bpf/common.h | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 67 insertions(+)
> >  create mode 100644 samples/bpf/common.h
> >
> > diff --git a/samples/bpf/common.h b/samples/bpf/common.h
> > new file mode 100644
> > index 000000000000..ec60fb665544
> > --- /dev/null
> > +++ b/samples/bpf/common.h
> > @@ -0,0 +1,67 @@
> > +/* SPDX-License-Identifier: GPL-2.0
> > + *
> > + * This program is free software; you can redistribute it and/or
> > + * modify it under the terms of version 2 of the GNU General Public
> > + * License as published by the Free Software Foundation.
> > + */
> > +
>
> nit, for this patch and the last one we don't need the text. Just the SPDX
> identifier should be enough. Its at least in line with everything we have
> elsewhere.
>
> Also if there is a copyright on that original file we should pull it over
> as far as I understand it. I don't see anything there though so maybe
> not.

Original code is dual-licensed as (LGPL-2.1 OR BSD-2-Clause), probably
leaving a comment with original location and mentioning the original
license would be ok?

I've also CC'ed original author (Wenbo Zhang), just for visibility.

>
> > +#define min(x, y) ({                          \
> > +     typeof(x) _min1 = (x);                   \
> > +     typeof(y) _min2 = (y);                   \
> > +     (void) (&_min1 == &_min2);               \
> > +     _min1 < _min2 ? _min1 : _min2; })
>
> What was wrong with 'min(a,b) ((a) < (b) ? (a) : (b))' looks like
> below its just used for comparing two unsigned ints?
>
> Thanks.
>
> > +

[...]

Re: [bpf-next 2/3] sample/bpf: Add log2 histogram function support

[Xin Hao]

在 2020/9/22 上午5:40, Andrii Nakryiko 写道:
> On Mon, Sep 21, 2020 at 10:18 AM John Fastabend
> <john.fastabend@gmail.com> wrote:
>> Xin Hao wrote:
>>> The relative functions is copy from bcc tools
> you probably meant relevant, not relative?
Yes
>
>>> source code: libbpf-tools/trace_helpers.c.
>>> URL: https://github.com/iovisor/bcc.git
>>>
>>> Log2 histogram can display the change of the collected
>>> data more conveniently.
>>>
>>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
>>> ---
>>>   samples/bpf/common.h | 67 ++++++++++++++++++++++++++++++++++++++++++++
>>>   1 file changed, 67 insertions(+)
>>>   create mode 100644 samples/bpf/common.h
>>>
>>> diff --git a/samples/bpf/common.h b/samples/bpf/common.h
>>> new file mode 100644
>>> index 000000000000..ec60fb665544
>>> --- /dev/null
>>> +++ b/samples/bpf/common.h
>>> @@ -0,0 +1,67 @@
>>> +/* SPDX-License-Identifier: GPL-2.0
>>> + *
>>> + * This program is free software; you can redistribute it and/or
>>> + * modify it under the terms of version 2 of the GNU General Public
>>> + * License as published by the Free Software Foundation.
>>> + */
>>> +
>> nit, for this patch and the last one we don't need the text. Just the SPDX
>> identifier should be enough. Its at least in line with everything we have
>> elsewhere.
Thanks, i will change it.
>>
>> Also if there is a copyright on that original file we should pull it over
>> as far as I understand it. I don't see anything there though so maybe
>> not.
> There no copyright, it follow dual-licensed as (LGPL-2.1 OR BSD-2-Clause)
>
> Original code is dual-licensed as (LGPL-2.1 OR BSD-2-Clause), probably
> leaving a comment with original location and mentioning the original
> license would be ok?
Ok, thanks
>
> I've also CC'ed original author (Wenbo Zhang), just for visibility.
Thanks
>
>>> +#define min(x, y) ({                          \
>>> +     typeof(x) _min1 = (x);                   \
>>> +     typeof(y) _min2 = (y);                   \
>>> +     (void) (&_min1 == &_min2);               \
>>> +     _min1 < _min2 ? _min1 : _min2; })
>> What was wrong with 'min(a,b) ((a) < (b) ? (a) : (b))' looks like
>> below its just used for comparing two unsigned ints?
>>
>> Thanks.
  I do not chang any codes, That's what the original code looks like

>>
>>> +
> [...]

-- 
Best Regards!
Xin Hao

Re: [bpf-next 2/3] sample/bpf: Add log2 histogram function support

[Wenbo Zhang]

>>
>> Also if there is a copyright on that original file we should pull it over
>> as far as I understand it. I don't see anything there though so maybe
>> not.
> There no copyright, it follow dual-licensed as (LGPL-2.1 OR BSD-2-Clause)
>
> Original code is dual-licensed as (LGPL-2.1 OR BSD-2-Clause), probably
> leaving a comment with original location and mentioning the original
> license would be ok?
> Ok, thanks
>
> I've also CC'ed original author (Wenbo Zhang), just for visibility.
> Thanks

Or you can add Copyright (c) 2020 Wenbo Zhang for fewer chars , either
way, thanks. :)


On Tue, Sep 22, 2020 at 10:06 AM Xin Hao <xhao@linux.alibaba.com> wrote:
>
>
> 在 2020/9/22 上午5:40, Andrii Nakryiko 写道:
> > On Mon, Sep 21, 2020 at 10:18 AM John Fastabend
> > <john.fastabend@gmail.com> wrote:
> >> Xin Hao wrote:
> >>> The relative functions is copy from bcc tools
> > you probably meant relevant, not relative?
> Yes
> >
> >>> source code: libbpf-tools/trace_helpers.c.
> >>> URL: https://github.com/iovisor/bcc.git
> >>>
> >>> Log2 histogram can display the change of the collected
> >>> data more conveniently.
> >>>
> >>> Signed-off-by: Xin Hao <xhao@linux.alibaba.com>
> >>> ---
> >>>   samples/bpf/common.h | 67 ++++++++++++++++++++++++++++++++++++++++++++
> >>>   1 file changed, 67 insertions(+)
> >>>   create mode 100644 samples/bpf/common.h
> >>>
> >>> diff --git a/samples/bpf/common.h b/samples/bpf/common.h
> >>> new file mode 100644
> >>> index 000000000000..ec60fb665544
> >>> --- /dev/null
> >>> +++ b/samples/bpf/common.h
> >>> @@ -0,0 +1,67 @@
> >>> +/* SPDX-License-Identifier: GPL-2.0
> >>> + *
> >>> + * This program is free software; you can redistribute it and/or
> >>> + * modify it under the terms of version 2 of the GNU General Public
> >>> + * License as published by the Free Software Foundation.
> >>> + */
> >>> +
> >> nit, for this patch and the last one we don't need the text. Just the SPDX
> >> identifier should be enough. Its at least in line with everything we have
> >> elsewhere.
> Thanks, i will change it.
> >>
> >> Also if there is a copyright on that original file we should pull it over
> >> as far as I understand it. I don't see anything there though so maybe
> >> not.
> > There no copyright, it follow dual-licensed as (LGPL-2.1 OR BSD-2-Clause)
> >
> > Original code is dual-licensed as (LGPL-2.1 OR BSD-2-Clause), probably
> > leaving a comment with original location and mentioning the original
> > license would be ok?
> Ok, thanks
> >
> > I've also CC'ed original author (Wenbo Zhang), just for visibility.
> Thanks
> >
> >>> +#define min(x, y) ({                          \
> >>> +     typeof(x) _min1 = (x);                   \
> >>> +     typeof(y) _min2 = (y);                   \
> >>> +     (void) (&_min1 == &_min2);               \
> >>> +     _min1 < _min2 ? _min1 : _min2; })
> >> What was wrong with 'min(a,b) ((a) < (b) ? (a) : (b))' looks like
> >> below its just used for comparing two unsigned ints?
> >>
> >> Thanks.
>   I do not chang any codes, That's what the original code looks like
>
> >>
> >>> +
> > [...]
>
> --
> Best Regards!
> Xin Hao
>