* [bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func
2020-09-20 14:45 [bpf-next 0/3] Using log2 histogram to display the statistics of every softirq execution Xin Hao
@ 2020-09-20 14:45 ` Xin Hao
2020-09-21 16:56 ` John Fastabend
2020-09-20 14:45 ` [bpf-next 2/3] sample/bpf: Add log2 histogram function support Xin Hao
2020-09-20 14:45 ` [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics Xin Hao
2 siblings, 1 reply; 11+ messages in thread
From: Xin Hao @ 2020-09-20 14:45 UTC (permalink / raw)
To: ast; +Cc: daniel, kafai, andriin, xhao, bpf
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* RE: [bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func
2020-09-20 14:45 ` [bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func Xin Hao
@ 2020-09-21 16:56 ` John Fastabend
0 siblings, 0 replies; 11+ messages in thread
From: John Fastabend @ 2020-09-21 16:56 UTC (permalink / raw)
To: Xin Hao, ast; +Cc: daniel, kafai, andriin, xhao, bpf
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>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [bpf-next 2/3] sample/bpf: Add log2 histogram function support
2020-09-20 14:45 [bpf-next 0/3] Using log2 histogram to display the statistics of every softirq execution Xin Hao
2020-09-20 14:45 ` [bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func Xin Hao
@ 2020-09-20 14:45 ` Xin Hao
2020-09-21 17:16 ` John Fastabend
2020-09-20 14:45 ` [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics Xin Hao
2 siblings, 1 reply; 11+ messages in thread
From: Xin Hao @ 2020-09-20 14:45 UTC (permalink / raw)
To: ast; +Cc: daniel, kafai, andriin, xhao, bpf
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* RE: [bpf-next 2/3] sample/bpf: Add log2 histogram function support
2020-09-20 14:45 ` [bpf-next 2/3] sample/bpf: Add log2 histogram function support Xin Hao
@ 2020-09-21 17:16 ` John Fastabend
2020-09-21 21:40 ` Andrii Nakryiko
0 siblings, 1 reply; 11+ messages in thread
From: John Fastabend @ 2020-09-21 17:16 UTC (permalink / raw)
To: Xin Hao, ast; +Cc: daniel, kafai, andriin, xhao, bpf
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
>
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bpf-next 2/3] sample/bpf: Add log2 histogram function support
2020-09-21 17:16 ` John Fastabend
@ 2020-09-21 21:40 ` Andrii Nakryiko
2020-09-22 2:06 ` Xin Hao
0 siblings, 1 reply; 11+ messages in thread
From: Andrii Nakryiko @ 2020-09-21 21:40 UTC (permalink / raw)
To: John Fastabend
Cc: Xin Hao, Alexei Starovoitov, Daniel Borkmann, Martin Lau,
Andrii Nakryiko, bpf, Wenbo Zhang
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.
>
> > +
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bpf-next 2/3] sample/bpf: Add log2 histogram function support
2020-09-21 21:40 ` Andrii Nakryiko
@ 2020-09-22 2:06 ` Xin Hao
2020-09-23 5:32 ` Wenbo Zhang
0 siblings, 1 reply; 11+ messages in thread
From: Xin Hao @ 2020-09-22 2:06 UTC (permalink / raw)
To: Andrii Nakryiko, John Fastabend
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko,
bpf, Wenbo Zhang
在 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
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bpf-next 2/3] sample/bpf: Add log2 histogram function support
2020-09-22 2:06 ` Xin Hao
@ 2020-09-23 5:32 ` Wenbo Zhang
0 siblings, 0 replies; 11+ messages in thread
From: Wenbo Zhang @ 2020-09-23 5:32 UTC (permalink / raw)
To: xhao
Cc: Andrii Nakryiko, John Fastabend, Alexei Starovoitov,
Daniel Borkmann, Martin Lau, Andrii Nakryiko, bpf
>>
>> 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics
2020-09-20 14:45 [bpf-next 0/3] Using log2 histogram to display the statistics of every softirq execution Xin Hao
2020-09-20 14:45 ` [bpf-next 1/3] sample/bpf: Avoid repetitive definition of log2 func Xin Hao
2020-09-20 14:45 ` [bpf-next 2/3] sample/bpf: Add log2 histogram function support Xin Hao
@ 2020-09-20 14:45 ` Xin Hao
2020-09-21 17:33 ` John Fastabend
2020-09-21 21:33 ` Andrii Nakryiko
2 siblings, 2 replies; 11+ messages in thread
From: Xin Hao @ 2020-09-20 14:45 UTC (permalink / raw)
To: ast; +Cc: daniel, kafai, andriin, xhao, bpf
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
^ permalink raw reply related [flat|nested] 11+ messages in thread* RE: [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics
2020-09-20 14:45 ` [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics Xin Hao
@ 2020-09-21 17:33 ` John Fastabend
2020-09-21 21:33 ` Andrii Nakryiko
1 sibling, 0 replies; 11+ messages in thread
From: John Fastabend @ 2020-09-21 17:33 UTC (permalink / raw)
To: Xin Hao, ast; +Cc: daniel, kafai, andriin, xhao, bpf
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.
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread* Re: [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics
2020-09-20 14:45 ` [bpf-next 3/3] samples/bpf: Add soft irq execution time statistics Xin Hao
2020-09-21 17:33 ` John Fastabend
@ 2020-09-21 21:33 ` Andrii Nakryiko
1 sibling, 0 replies; 11+ messages in thread
From: Andrii Nakryiko @ 2020-09-21 21:33 UTC (permalink / raw)
To: Xin Hao
Cc: Alexei Starovoitov, Daniel Borkmann, Martin Lau, Andrii Nakryiko,
bpf
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;
[...]
^ permalink raw reply [flat|nested] 11+ messages in thread