* Re: [PATCH net] mvpp2: refactor MTU change code
From: David Miller @ 2019-07-27 20:24 UTC (permalink / raw)
To: mcroce; +Cc: netdev, antoine.tenart, maxime.chevallier, mw, stefanc,
linux-kernel
In-Reply-To: <20190725231931.24073-1-mcroce@redhat.com>
From: Matteo Croce <mcroce@redhat.com>
Date: Fri, 26 Jul 2019 01:19:31 +0200
> The MTU change code can call napi_disable() with the device already down,
> leading to a deadlock. Also, lot of code is duplicated unnecessarily.
>
> Rework mvpp2_change_mtu() to avoid the deadlock and remove duplicated code.
>
> Signed-off-by: Matteo Croce <mcroce@redhat.com>
Please resubmit with the Fixes: tag.
Please do not line break the Fixes: tag no matter how many characters
it ends up being in total.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] mvpp2: document HW checksum behaviour
From: David Miller @ 2019-07-27 20:23 UTC (permalink / raw)
To: mcroce; +Cc: antoine.tenart, netdev, maxime.chevallier, mw, stefanc,
linux-kernel
In-Reply-To: <CAGnkfhy_h_UfoefRmBjQgUgiX+954fQjX2kqa2hPLbKpLHU4rg@mail.gmail.com>
From: Matteo Croce <mcroce@redhat.com>
Date: Fri, 26 Jul 2019 16:35:59 +0200
> I see, there is a similar statement in mvpp2_port_probe().
> What about adding a static function which sets the flag, and add the
> comment there instead of duplicating the comment?
That sounds good to me.
^ permalink raw reply
* Re: [PATCH 0/2] Make ipmr queue length configurable
From: David Miller @ 2019-07-27 20:18 UTC (permalink / raw)
To: brodie.greenfield
Cc: stephen, kuznet, yoshfuji, netdev, linux-kernel, chris.packham,
luuk.paulussen
In-Reply-To: <20190725204230.12229-1-brodie.greenfield@alliedtelesis.co.nz>
From: Brodie Greenfield <brodie.greenfield@alliedtelesis.co.nz>
Date: Fri, 26 Jul 2019 08:42:28 +1200
> We want to have some more space in our queue for processing incoming
> multicast packets, so we can process more of them without dropping
> them prematurely. It is useful to be able to increase this limit on
> higher-spec platforms that can handle more items.
>
> For the particular use case here at Allied Telesis, we have linux
> running on our switches and routers, with support for the number of
> multicast groups being increased. Basically, this queue length affects
> the time taken to fully learn all of the multicast streams.
>
> Changes in v3:
> - Corrected a v4 to v6 typo.
As others have voiced, I think it's dangerous to let every netns
increase this so readily.
We need to either put in a non-initns limit or simply not allow
non-init namespaces to change this.
But really socket queue limits are a better place to enforce this.
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: mv88e6xxx: avoid some redundant vtu load/purge operations
From: David Miller @ 2019-07-27 20:14 UTC (permalink / raw)
To: rasmus.villemoes
Cc: andrew, vivien.didelot, f.fainelli, Rasmus.Villemoes, netdev,
linux-kernel
In-Reply-To: <20190722233713.31396-1-rasmus.villemoes@prevas.dk>
Reminder that we need a review from Vivien on this.
Thanks.
^ permalink raw reply
* [PATCH] net: stmmac: manage errors returned by of_get_mac_address()
From: Martin Blumenstingl @ 2019-07-27 19:21 UTC (permalink / raw)
To: peppe.cavallaro, alexandre.torgue, joabreu, davem, netdev
Cc: linux-arm-kernel, linux-kernel, linux-amlogic,
Martin Blumenstingl
Commit d01f449c008a ("of_net: add NVMEM support to of_get_mac_address")
added support for reading the MAC address from an nvmem-cell. This
required changing the logic to return an error pointer upon failure.
If stmmac is loaded before the nvmem provider driver then
of_get_mac_address() return an error pointer with -EPROBE_DEFER.
Propagate this error so the stmmac driver will be probed again after the
nvmem provider driver is loaded.
Default to a random generated MAC address in case of any other error,
instead of using the error pointer as MAC address.
Fixes: d01f449c008a ("of_net: add NVMEM support to of_get_mac_address")
Signed-off-by: Martin Blumenstingl <martin.blumenstingl@googlemail.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
index 73fc2524372e..154daf4d1072 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c
@@ -370,6 +370,13 @@ stmmac_probe_config_dt(struct platform_device *pdev, const char **mac)
return ERR_PTR(-ENOMEM);
*mac = of_get_mac_address(np);
+ if (IS_ERR(*mac)) {
+ if (PTR_ERR(*mac) == -EPROBE_DEFER)
+ return ERR_CAST(*mac);
+
+ *mac = NULL;
+ }
+
plat->interface = of_get_phy_mode(np);
/* Some wrapper drivers still rely on phy_node. Let's save it while
--
2.22.0
^ permalink raw reply related
* Re: [PATCH bpf-next 02/10] libbpf: implement BPF CO-RE offset relocation algorithm
From: Andrii Nakryiko @ 2019-07-27 19:09 UTC (permalink / raw)
To: Song Liu
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Yonghong Song, Kernel Team
In-Reply-To: <9EE75932-5AED-49D3-86BF-D1FC2A139BF8@fb.com>
On Sat, Jul 27, 2019 at 11:59 AM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Jul 26, 2019, at 11:11 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Thu, Jul 25, 2019 at 12:32 PM Song Liu <songliubraving@fb.com> wrote:
> >>
> >>
> >>
> >>> On Jul 24, 2019, at 12:27 PM, Andrii Nakryiko <andriin@fb.com> wrote:
> >>>
> >>> This patch implements the core logic for BPF CO-RE offsets relocations.
> >>> All the details are described in code comments.
> >>
> >> Some description in the change log is still useful. Please at least
> >> copy-paste key comments here.
> >
> > OK, will add some more.
> >
> >>
> >> And, this is looooong. I think it is totally possible to split it into
> >> multiple smaller patches.
> >
> > I don't really know how to split it further without hurting reviewing
> > by artificially splitting related code into separate patches. Remove
> > any single function and algorithm will be incomplete.
> >
> > Let me give you some high-level overview of how pieces are put
> > together. There are 9 non-trivial functions, let's go over their
> > purpose in the orderd in which they are defined in file:
> >
> > 1. bpf_core_spec_parse()
> >
> > This one take bpf_offset_reloc's type_id and accessor string
> > ("0:1:2:3") and parses it into more convenient bpf_core_spec
> > datastructure, which has calculated offset and high-level spec
> > "steps": either named field or array access.
> >
> > 2. bpf_core_find_cands()
> >
> > Given local type name, finds all possible target BTF types with same
> > name (modulo "flavor" differences, ___flavor suffix is just ignored).
> >
> > 3. bpf_core_fields_are_compat()
> >
> > Given local and target field match, checks that their types are
> > compatible (so that we don't accidentally match, e.g., int against
> > struct).
> >
> > 4. bpf_core_match_member()
> >
> > Given named local field, find corresponding field in target struct. To
> > understand why it's not trivial, here's an example:
> >
> > Local type:
> >
> > struct s___local {
> > int a;
> > };
> >
> > Target type:
> >
> > struct s___target {
> > struct {
> > union {
> > int a;
> > };
> > };
> > };
> >
> > For both cases you can access a as s.a, but in local case, field a is
> > immediately inside s___local, while for s___target, you have to
> > traverse two levels deeper into anonymous fields to get to an `a`
> > inside anonymous union.
> >
> > So this function find that `a` by doing exhaustive search across all
> > named field and anonymous struct/unions. But otherwise it's pretty
> > straightforward recursive function.
> >
> > bpf_core_spec_match()
> >
> > Just goes over high-level spec steps in local spec and tries to figure
> > out both high-level and low-level steps for targe type. Consider the
> > above example. For both structs accessing s.a is one high-level step,
> > but for s___local it's single low-level step (just another :0 in spec
> > string), while for s___target it's three low-level steps: ":0:0:0",
> > one step for each BTF type we need to traverse.
> >
> > Array access is simpler, it's always one high-level and one low-level step.
> >
> > bpf_core_reloc_insn()
> >
> > Once we match local and target specs and have local and target
> > offsets, do the relocations - check that instruction has expected
> > local offset and replace it with target offset.
> >
> > bpf_core_find_kernel_btf()
> >
> > This is the only function that can be moved into separate patch, but
> > it's also very simple. It just iterates over few known possible
> > locations for vmlinux image and once found, tries to parse .BTF out of
> > it, to be used as target BTF.
> >
> > bpf_core_reloc_offset()
> >
> > It combines all the above functions to perform single relocation.
> > Parse spec, get candidates, for each candidate try to find matching
> > target spec. All candidates that matched are cached for given local
> > root type.
>
> Thanks for these explanation. They are really helpful.
>
> I think some example explaining each step of bpf_core_reloc_offset()
> will be very helpful. Something like:
>
> Example:
>
> struct s {
> int a;
> struct {
> int b;
> bool c;
> };
> };
>
> To get offset for c, we do:
>
> bpf_core_reloc_offset() {
>
> /* input data: xxx */
>
> /* first step: bpf_core_spec_parse() */
>
> /* data after first step */
>
> /* second step: bpf_core_find_cands() */
>
> /* candidate A and B after second step */
>
> ...
> }
>
> Well, it requires quite some work to document this way. Please let me
> know if you feel this is an overkill.
Yeah :) And it's not just work, but I think it's bad if comments
become too specific and document very low-level steps, because code
might evolve and comments can quickly get out of sync and just add to
confusion. Which is why I tried to document high-level ideas, leaving
it up to the source code to be the ultimate reference of minutia
details.
>
> >
> > bpf_core_reloc_offsets()
> >
> > High-level coordination. Iterate over all per-program .BTF.ext offset
> > reloc sections, each relocation within them. Find corresponding
> > program and try to apply relocations one by one.
> >
> >
> > I think the only non-obvious part here is to understand that
> > relocation records local raw spec with every single anonymous type
> > traversal, which is not that useful when we try to match it against
> > target type, which can have very different composition, but still the
> > same field access pattern, from C language standpoint (which hides all
> > those anonymous type traversals from programmer).
> >
> > But it should be pretty clear now, plus also check tests, they have
> > lots of cases showing what's compatible and what's not.
>
> I see. I will review the tests.
>
> >>>
> >>> static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> >>> - __u32 id)
> >>> + __u32 id,
> >>> + __u32 *res_id)
> >>> {
> >>> const struct btf_type *t = btf__type_by_id(btf, id);
> >>
> >> Maybe have a local "__u32 rid;"
> >>
> >>>
> >>> + if (res_id)
> >>> + *res_id = id;
> >>> +
> >>
> >> and do "rid = id;" here
> >>
> >>> while (true) {
> >>> switch (BTF_INFO_KIND(t->info)) {
> >>> case BTF_KIND_VOLATILE:
> >>> case BTF_KIND_CONST:
> >>> case BTF_KIND_RESTRICT:
> >>> case BTF_KIND_TYPEDEF:
> >>> + if (res_id)
> >>> + *res_id = t->type;
> >> and here
> >>
> >>> t = btf__type_by_id(btf, t->type);
> >>> break;
> >>> default:
> >> and "*res_id = rid;" right before return?
> >
> > Sure, but why?
>
> I think it is cleaner that way. But feel free to ignore if you
> think otherwise.
>
> >
> >>
> >>> @@ -1041,7 +1049,7 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> >>> static bool get_map_field_int(const char *map_name, const struct btf *btf,
> >>> const struct btf_type *def,
> >>> const struct btf_member *m, __u32 *res) {
> >
> > [...]
> >
> >>> +struct bpf_core_spec {
> >>> + const struct btf *btf;
> >>> + /* high-level spec: named fields and array indicies only */
> >>
> >> typo: indices
> >
> > thanks!
> >
> >>
> >>> + struct bpf_core_accessor spec[BPF_CORE_SPEC_MAX_LEN];
> >>> + /* high-level spec length */
> >>> + int len;
> >>> + /* raw, low-level spec: 1-to-1 with accessor spec string */
> >>> + int raw_spec[BPF_CORE_SPEC_MAX_LEN];
> >>> + /* raw spec length */
> >>> + int raw_len;
> >>> + /* field byte offset represented by spec */
> >>> + __u32 offset;
> >>> +};
> >
> > [...]
> >
> >>> + *
> >>> + * int x = &s->a[3]; // access string = '0:1:2:3'
> >>> + *
> >>> + * Low-level spec has 1:1 mapping with each element of access string (it's
> >>> + * just a parsed access string representation): [0, 1, 2, 3].
> >>> + *
> >>> + * High-level spec will capture only 3 points:
> >>> + * - intial zero-index access by pointer (&s->... is the same as &s[0]...);
> >>> + * - field 'a' access (corresponds to '2' in low-level spec);
> >>> + * - array element #3 access (corresponds to '3' in low-level spec).
> >>> + *
> >>> + */
> >>
> >> IIUC, high-level points are subset of low-level points. How about we introduce
> >> "anonymous" high-level points, so that high-level points and low-level points
> >> are 1:1 mapping?
> >
> > No, that will just hurt and complicate things. See above explanation
> > about why we need high-level points (it's what you as C programmer try
> > to achieve vs low-level spec is what C-language does in reality, with
> > all the anonymous struct/union traversal).
> >
> > What's wrong with this separation? Think about it as recording
> > "intent" (high-level spec) vs "mechanics" (low-level spec, how exactly
> > to achieve that intent, in excruciating details).
>
> There is nothing wrong with separation. I just personally think it is
> cleaner the other way. That's why I raised the question.
>
> I will go with your assessment, as you looked into this much more than
> I did. :-)
For me it's a machine view of the problem (raw spec) vs human view of
the problem (high-level spec, which resembles how you think about this
in C code). I'll keep it separate unless it proves to be problematic
going forward.
>
> [...]
>
> >>> +
> >>> + memset(spec, 0, sizeof(*spec));
> >>> + spec->btf = btf;
> >>> +
> >>> + /* parse spec_str="0:1:2:3:4" into array raw_spec=[0, 1, 2, 3, 4] */
> >>> + while (*spec_str) {
> >>> + if (*spec_str == ':')
> >>> + ++spec_str;
> >>> + if (sscanf(spec_str, "%d%n", &access_idx, &parsed_len) != 1)
> >>> + return -EINVAL;
> >>> + if (spec->raw_len == BPF_CORE_SPEC_MAX_LEN)
> >>> + return -E2BIG;
> >>> + spec_str += parsed_len;
> >>> + spec->raw_spec[spec->raw_len++] = access_idx;
> >>> + }
> >>> +
> >>> + if (spec->raw_len == 0)
> >>> + return -EINVAL;
> >>> +
> >>> + for (i = 0; i < spec->raw_len; i++) {
> >>> + t = skip_mods_and_typedefs(btf, id, &id);
> >>> + if (!t)
> >>> + return -EINVAL;
> >>> +
> >>> + access_idx = spec->raw_spec[i];
> >>> +
> >>> + if (i == 0) {
> >>> + /* first spec value is always reloc type array index */
> >>> + spec->spec[spec->len].type_id = id;
> >>> + spec->spec[spec->len].idx = access_idx;
> >>> + spec->len++;
> >>> +
> >>> + sz = btf__resolve_size(btf, id);
> >>> + if (sz < 0)
> >>> + return sz;
> >>> + spec->offset += access_idx * sz;
> >> spec->offset = access_idx * sz; should be enough
> >
> > No. spec->offset is carefully maintained across multiple low-level
> > steps, as we traverse down embedded structs/unions.
> >
> > Think about, e.g.:
> >
> > struct s {
> > int a;
> > struct {
> > int b;
> > };
> > };
> >
> > Imagine you are trying to match s.b access. With what you propose
> > you'll end up with offset 0, but it should be 4.
>
> Hmm... this is just for i == 0, right? Which line updated spec->offset
> after "memset(spec, 0, sizeof(*spec));"?
Ah, I missed that you are referring to the special i == 0 case. I can
do assignment, yes, you are right. I'll probably also extract it out
of the loop to make it less confusing.
>
> >
> >>
> >>> + continue;
> >>> + }
> >>
> >> Maybe pull i == 0 case out of the for loop?
> >>
> >>> +
> >>> + if (btf_is_composite(t)) {
> >
> > [...]
> >
> >>> +
> >>> + if (spec->len == 0)
> >>> + return -EINVAL;
> >>
> >> Can this ever happen?
> >
> > Not really, because I already check raw_len == 0 and exit with error.
> > I'll remove.
> >
> >>
> >>> +
> >>> + return 0;
> >>> +}
> >>> +
> >
> > [...]
> >
> >>> +
> >>> +/*
> >>> + * Given single high-level accessor (either named field or array index) in
> >>> + * local type, find corresponding high-level accessor for a target type. Along
> >>> + * the way, maintain low-level spec for target as well. Also keep updating
> >>> + * target offset.
> >>> + */
> >>
> >> Please describe the recursive algorithm here. I am kinda lost.
> >
> > Explained above. I'll extend description a bit. But it's just
> > recursive exhaustive search:
> > 1. if struct field is anonymous and is struct/union, go one level
> > deeper and try to find field with given name inside those.
> > 2. if field has name and it matched what we are searching - check type
> > compatibility. It has to be compatible, so if it's not, then it's not
> > a match.
> >
> >> Also, please document the meaning of zero, positive, negative return values.
> >
> > Ok. It's standard <0 - error, 0 - false, 1 - true.
> >
> >>
> >>> +static int bpf_core_match_member(const struct btf *local_btf,
> >>> + const struct bpf_core_accessor *local_acc,
> >>> + const struct btf *targ_btf,
> >>> + __u32 targ_id,
> >>> + struct bpf_core_spec *spec,
> >>> + __u32 *next_targ_id)
> >>> +{
> >
> > [...]
> >
> >>> + if (local_acc->name) {
> >>> + if (!btf_is_composite(targ_type))
> >>> + return 0;
> >>> +
> >>> + matched = bpf_core_match_member(local_spec->btf,
> >>> + local_acc,
> >>> + targ_btf, targ_id,
> >>> + targ_spec, &targ_id);
> >>> + if (matched <= 0)
> >>> + return matched;
> >>> + } else {
> >>> + /* for i=0, targ_id is already treated as array element
> >>> + * type (because it's the original struct), for others
> >>> + * we should find array element type first
> >>> + */
> >>> + if (i > 0) {
> >>
> >> i == 0 case would go into "if (local_acc->name)" branch, no?
> >
> > No, i == 0 is always an array access. s->a.b.c is the same as
> > s[0].a.b.c, so relocation's first spec element is always either zero
> > for pointer access or any non-negative index for array access. But it
> > is always array access.
>
> I see. Thanks for the explanation.
>
> Song
^ permalink raw reply
* [PATCH v2 bpf-next 8/9] selftests/bpf: convert bpf_verif_scale.c to sub-tests API
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Expose each BPF verifier scale test as individual sub-test to allow
independent results output and test selection.
Test run results now look like this:
$ sudo ./test_progs -t verif/
#3/1 loop3.o:OK
#3/2 test_verif_scale1.o:OK
#3/3 test_verif_scale2.o:OK
#3/4 test_verif_scale3.o:OK
#3/5 pyperf50.o:OK
#3/6 pyperf100.o:OK
#3/7 pyperf180.o:OK
#3/8 pyperf600.o:OK
#3/9 pyperf600_nounroll.o:OK
#3/10 loop1.o:OK
#3/11 loop2.o:OK
#3/12 strobemeta.o:OK
#3/13 strobemeta_nounroll1.o:OK
#3/14 strobemeta_nounroll2.o:OK
#3/15 test_sysctl_loop1.o:OK
#3/16 test_sysctl_loop2.o:OK
#3/17 test_xdp_loop.o:OK
#3/18 test_seg6_loop.o:OK
#3 bpf_verif_scale:OK
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../bpf/prog_tests/bpf_verif_scale.c | 77 ++++++++++---------
1 file changed, 40 insertions(+), 37 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index b59017279e0b..b4be96162ff4 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -33,14 +33,25 @@ static int check_load(const char *file, enum bpf_prog_type type)
return err;
}
+struct scale_test_def {
+ const char *file;
+ enum bpf_prog_type attach_type;
+ bool fails;
+};
+
void test_bpf_verif_scale(void)
{
- const char *sched_cls[] = {
- "./test_verif_scale1.o", "./test_verif_scale2.o", "./test_verif_scale3.o",
- };
- const char *raw_tp[] = {
+ struct scale_test_def tests[] = {
+ { "loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT, true /* fails */ },
+
+ { "test_verif_scale1.o", BPF_PROG_TYPE_SCHED_CLS },
+ { "test_verif_scale2.o", BPF_PROG_TYPE_SCHED_CLS },
+ { "test_verif_scale3.o", BPF_PROG_TYPE_SCHED_CLS },
+
/* full unroll by llvm */
- "./pyperf50.o", "./pyperf100.o", "./pyperf180.o",
+ { "pyperf50.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "pyperf100.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "pyperf180.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* partial unroll. llvm will unroll loop ~150 times.
* C loop count -> 600.
@@ -48,7 +59,7 @@ void test_bpf_verif_scale(void)
* 16k insns in loop body.
* Total of 5 such loops. Total program size ~82k insns.
*/
- "./pyperf600.o",
+ { "pyperf600.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* no unroll at all.
* C loop count -> 600.
@@ -56,22 +67,26 @@ void test_bpf_verif_scale(void)
* ~110 insns in loop body.
* Total of 5 such loops. Total program size ~1500 insns.
*/
- "./pyperf600_nounroll.o",
+ { "pyperf600_nounroll.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
- "./loop1.o", "./loop2.o",
+ { "loop1.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "loop2.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* partial unroll. 19k insn in a loop.
* Total program size 20.8k insn.
* ~350k processed_insns
*/
- "./strobemeta.o",
+ { "strobemeta.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
/* no unroll, tiny loops */
- "./strobemeta_nounroll1.o",
- "./strobemeta_nounroll2.o",
- };
- const char *cg_sysctl[] = {
- "./test_sysctl_loop1.o", "./test_sysctl_loop2.o",
+ { "strobemeta_nounroll1.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+ { "strobemeta_nounroll2.o", BPF_PROG_TYPE_RAW_TRACEPOINT },
+
+ { "test_sysctl_loop1.o", BPF_PROG_TYPE_CGROUP_SYSCTL },
+ { "test_sysctl_loop2.o", BPF_PROG_TYPE_CGROUP_SYSCTL },
+
+ { "test_xdp_loop.o", BPF_PROG_TYPE_XDP },
+ { "test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL },
};
libbpf_print_fn_t old_print_fn = NULL;
int err, i;
@@ -81,33 +96,21 @@ void test_bpf_verif_scale(void)
old_print_fn = libbpf_set_print(libbpf_debug_print);
}
- err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
- test__printf("test_scale:loop3:%s\n",
- err ? (error_cnt--, "OK") : "FAIL");
+ for (i = 0; i < ARRAY_SIZE(tests); i++) {
+ const struct scale_test_def *test = &tests[i];
- for (i = 0; i < ARRAY_SIZE(sched_cls); i++) {
- err = check_load(sched_cls[i], BPF_PROG_TYPE_SCHED_CLS);
- test__printf("test_scale:%s:%s\n", sched_cls[i],
- err ? "FAIL" : "OK");
- }
+ if (!test__start_subtest(test->file))
+ continue;
- for (i = 0; i < ARRAY_SIZE(raw_tp); i++) {
- err = check_load(raw_tp[i], BPF_PROG_TYPE_RAW_TRACEPOINT);
- test__printf("test_scale:%s:%s\n", raw_tp[i],
- err ? "FAIL" : "OK");
+ err = check_load(test->file, test->attach_type);
+ if (test->fails) { /* expected to fail */
+ if (err)
+ error_cnt--;
+ else
+ error_cnt++;
+ }
}
- for (i = 0; i < ARRAY_SIZE(cg_sysctl); i++) {
- err = check_load(cg_sysctl[i], BPF_PROG_TYPE_CGROUP_SYSCTL);
- test__printf("test_scale:%s:%s\n", cg_sysctl[i],
- err ? "FAIL" : "OK");
- }
- err = check_load("./test_xdp_loop.o", BPF_PROG_TYPE_XDP);
- test__printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
-
- err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
- test__printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
-
if (env.verifier_stats)
libbpf_set_print(old_print_fn);
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 9/9] selftests/bpf: convert send_signal.c to use subtests
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Convert send_signal set of tests to be exposed as three sub-tests.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/prog_tests/send_signal.c | 9 ++++++---
1 file changed, 6 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index d950f4558897..461b423d0584 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -219,7 +219,10 @@ void test_send_signal(void)
{
int ret = 0;
- ret |= test_send_signal_tracepoint();
- ret |= test_send_signal_perf();
- ret |= test_send_signal_nmi();
+ if (test__start_subtest("send_signal_tracepoint"))
+ ret |= test_send_signal_tracepoint();
+ if (test__start_subtest("send_signal_perf"))
+ ret |= test_send_signal_perf();
+ if (test__start_subtest("send_signal_nmi"))
+ ret |= test_send_signal_nmi();
}
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 5/9] selftest/bpf: centralize libbpf logging management for test_progs
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Make test_progs test runner own libbpf logging. Also introduce two
levels of verbosity: -v and -vv. First one will be used in subsequent
patches to enable test log output always. Second one increases verbosity
level of libbpf logging further to include debug output as well.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../bpf/prog_tests/bpf_verif_scale.c | 6 +++-
.../bpf/prog_tests/reference_tracking.c | 15 +++-------
tools/testing/selftests/bpf/test_progs.c | 29 +++++++++++++++++++
3 files changed, 38 insertions(+), 12 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index e1b55261526f..ceddb8cc86f4 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -70,10 +70,11 @@ void test_bpf_verif_scale(void)
const char *cg_sysctl[] = {
"./test_sysctl_loop1.o", "./test_sysctl_loop2.o",
};
+ libbpf_print_fn_t old_print_fn = NULL;
int err, i;
if (verifier_stats)
- libbpf_set_print(libbpf_debug_print);
+ old_print_fn = libbpf_set_print(libbpf_debug_print);
err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
printf("test_scale:loop3:%s\n", err ? (error_cnt--, "OK") : "FAIL");
@@ -97,4 +98,7 @@ void test_bpf_verif_scale(void)
err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
+
+ if (verifier_stats)
+ libbpf_set_print(old_print_fn);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
index 5633be43828f..4a4f428d1a78 100644
--- a/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
+++ b/tools/testing/selftests/bpf/prog_tests/reference_tracking.c
@@ -1,15 +1,6 @@
// SPDX-License-Identifier: GPL-2.0
#include <test_progs.h>
-static int libbpf_debug_print(enum libbpf_print_level level,
- const char *format, va_list args)
-{
- if (level == LIBBPF_DEBUG)
- return 0;
-
- return vfprintf(stderr, format, args);
-}
-
void test_reference_tracking(void)
{
const char *file = "./test_sk_lookup_kern.o";
@@ -36,9 +27,11 @@ void test_reference_tracking(void)
/* Expect verifier failure if test name has 'fail' */
if (strstr(title, "fail") != NULL) {
- libbpf_set_print(NULL);
+ libbpf_print_fn_t old_print_fn;
+
+ old_print_fn = libbpf_set_print(NULL);
err = !bpf_program__load(prog, "GPL", 0);
- libbpf_set_print(libbpf_debug_print);
+ libbpf_set_print(old_print_fn);
} else {
err = bpf_program__load(prog, "GPL", 0);
}
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 6e04b9f83777..94b6951b90b3 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -186,6 +186,8 @@ enum ARG_KEYS {
ARG_TEST_NUM = 'n',
ARG_TEST_NAME = 't',
ARG_VERIFIER_STATS = 's',
+
+ ARG_VERBOSE = 'v',
};
static const struct argp_option opts[] = {
@@ -195,6 +197,8 @@ static const struct argp_option opts[] = {
"Run tests with names containing NAME" },
{ "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
"Output verifier statistics", },
+ { "verbose", ARG_VERBOSE, "LEVEL", OPTION_ARG_OPTIONAL,
+ "Verbose output (use -vv for extra verbose output)" },
{},
};
@@ -202,12 +206,22 @@ struct test_env {
int test_num_selector;
const char *test_name_selector;
bool verifier_stats;
+ bool verbose;
+ bool very_verbose;
};
static struct test_env env = {
.test_num_selector = -1,
};
+static int libbpf_print_fn(enum libbpf_print_level level,
+ const char *format, va_list args)
+{
+ if (!env.very_verbose && level == LIBBPF_DEBUG)
+ return 0;
+ return vfprintf(stderr, format, args);
+}
+
static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
struct test_env *env = state->input;
@@ -229,6 +243,19 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
case ARG_VERIFIER_STATS:
env->verifier_stats = true;
break;
+ case ARG_VERBOSE:
+ if (arg) {
+ if (strcmp(arg, "v") == 0) {
+ env->very_verbose = true;
+ } else {
+ fprintf(stderr,
+ "Unrecognized verbosity setting ('%s'), only -v and -vv are supported\n",
+ arg);
+ return -EINVAL;
+ }
+ }
+ env->verbose = true;
+ break;
case ARGP_KEY_ARG:
argp_usage(state);
break;
@@ -255,6 +282,8 @@ int main(int argc, char **argv)
if (err)
return err;
+ libbpf_set_print(libbpf_print_fn);
+
srand(time(NULL));
jit_enabled = is_jit_enabled();
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 7/9] selftests/bpf: add sub-tests support for test_progs
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Allow tests to have their own set of sub-tests. Also add ability to do
test/subtest selection using `-t <test-name>/<subtest-name>` and `-n
<test-nums-set>/<subtest-nums-set>`, as an extension of existing -t/-n
selector options. For the <test-num-set> format: it's a comma-separated
list of either individual test numbers (1-based), or range of test
numbers. E.g., all of the following are valid sets of test numbers:
- 10
- 1,2,3
- 1-3
- 5-10,1,3-4
'/<subtest' part is optional, but has the same format. E.g., to select
test #3 and its sub-tests #10 through #15, use: -t 3/10-15.
Similarly, to select tests by name, use `-t verif/strobe`:
$ sudo ./test_progs -t verif/strobe
#3/12 strobemeta.o:OK
#3/13 strobemeta_nounroll1.o:OK
#3/14 strobemeta_nounroll2.o:OK
#3 bpf_verif_scale:OK
Summary: 1/3 PASSED, 0 FAILED
Example of using subtest API is in the next patch, converting
bpf_verif_scale.c tests to use sub-tests.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_progs.c | 198 ++++++++++++++++++++---
tools/testing/selftests/bpf/test_progs.h | 16 +-
2 files changed, 185 insertions(+), 29 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 3cf3ebda1d31..7a2db48b6fd1 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -7,9 +7,7 @@
#include <string.h>
/* defined in test_progs.h */
-struct test_env env = {
- .test_num_selector = -1,
-};
+struct test_env env;
int error_cnt, pass_cnt;
struct prog_test_def {
@@ -20,8 +18,82 @@ struct prog_test_def {
int pass_cnt;
int error_cnt;
bool tested;
+
+ const char *subtest_name;
+ int subtest_num;
+
+ /* store counts before subtest started */
+ int old_pass_cnt;
+ int old_error_cnt;
};
+static bool should_run(struct test_selector *sel, int num, const char *name)
+{
+ if (sel->name && sel->name[0] && !strstr(name, sel->name))
+ return false;
+
+ if (!sel->num_set)
+ return true;
+
+ return num < sel->num_set_len && sel->num_set[num];
+}
+
+static void dump_test_log(const struct prog_test_def *test, bool failed)
+{
+ if (env.verbose || test->force_log || failed) {
+ if (env.log_cnt) {
+ fprintf(stdout, "%s", env.log_buf);
+ if (env.log_buf[env.log_cnt - 1] != '\n')
+ fprintf(stdout, "\n");
+ }
+ env.log_cnt = 0;
+ }
+}
+
+void test__end_subtest()
+{
+ struct prog_test_def *test = env.test;
+ int sub_error_cnt = error_cnt - test->old_error_cnt;
+
+ if (sub_error_cnt)
+ env.fail_cnt++;
+ else
+ env.sub_succ_cnt++;
+
+ dump_test_log(test, sub_error_cnt);
+
+ printf("#%d/%d %s:%s\n",
+ test->test_num, test->subtest_num,
+ test->subtest_name, sub_error_cnt ? "FAIL" : "OK");
+}
+
+bool test__start_subtest(const char *name)
+{
+ struct prog_test_def *test = env.test;
+
+ if (test->subtest_name) {
+ test__end_subtest();
+ test->subtest_name = NULL;
+ }
+
+ test->subtest_num++;
+
+ if (!name || !name[0]) {
+ fprintf(stderr, "Subtest #%d didn't provide sub-test name!\n",
+ test->subtest_num);
+ return false;
+ }
+
+ if (!should_run(&env.subtest_selector, test->subtest_num, name))
+ return false;
+
+ test->subtest_name = name;
+ env.test->old_pass_cnt = pass_cnt;
+ env.test->old_error_cnt = error_cnt;
+
+ return true;
+}
+
void test__force_log() {
env.test->force_log = true;
}
@@ -271,24 +343,103 @@ static int libbpf_print_fn(enum libbpf_print_level level,
return 0;
}
+int parse_num_list(const char *s, struct test_selector *sel)
+{
+ int i, set_len = 0, num, start = 0, end = -1;
+ bool *set = NULL, *tmp, parsing_end = false;
+ char *next;
+
+ while (s[0]) {
+ errno = 0;
+ num = strtol(s, &next, 10);
+ if (errno)
+ return -errno;
+
+ if (parsing_end)
+ end = num;
+ else
+ start = num;
+
+ if (!parsing_end && *next == '-') {
+ s = next + 1;
+ parsing_end = true;
+ continue;
+ } else if (*next == ',') {
+ parsing_end = false;
+ s = next + 1;
+ end = num;
+ } else if (*next == '\0') {
+ parsing_end = false;
+ s = next;
+ end = num;
+ } else {
+ return -EINVAL;
+ }
+
+ if (start > end)
+ return -EINVAL;
+
+ if (end + 1 > set_len) {
+ set_len = end + 1;
+ tmp = realloc(set, set_len);
+ if (!tmp) {
+ free(set);
+ return -ENOMEM;
+ }
+ set = tmp;
+ }
+ for (i = start; i <= end; i++) {
+ set[i] = true;
+ }
+
+ }
+
+ if (!set)
+ return -EINVAL;
+
+ sel->num_set = set;
+ sel->num_set_len = set_len;
+
+ return 0;
+}
+
static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
struct test_env *env = state->input;
switch (key) {
case ARG_TEST_NUM: {
- int test_num;
+ char *subtest_str = strchr(arg, '/');
- errno = 0;
- test_num = strtol(arg, NULL, 10);
- if (errno)
- return -errno;
- env->test_num_selector = test_num;
+ if (subtest_str) {
+ *subtest_str = '\0';
+ if (parse_num_list(subtest_str + 1,
+ &env->subtest_selector)) {
+ fprintf(stderr,
+ "Failed to parse subtest numbers.\n");
+ return -EINVAL;
+ }
+ }
+ if (parse_num_list(arg, &env->test_selector)) {
+ fprintf(stderr, "Failed to parse test numbers.\n");
+ return -EINVAL;
+ }
break;
}
- case ARG_TEST_NAME:
- env->test_name_selector = arg;
+ case ARG_TEST_NAME: {
+ char *subtest_str = strchr(arg, '/');
+
+ if (subtest_str) {
+ *subtest_str = '\0';
+ env->subtest_selector.name = strdup(subtest_str + 1);
+ if (!env->subtest_selector.name)
+ return -ENOMEM;
+ }
+ env->test_selector.name = strdup(arg);
+ if (!env->test_selector.name)
+ return -ENOMEM;
break;
+ }
case ARG_VERIFIER_STATS:
env->verifier_stats = true;
break;
@@ -343,14 +494,15 @@ int main(int argc, char **argv)
env.test = test;
test->test_num = i + 1;
- if (env.test_num_selector >= 0 &&
- test->test_num != env.test_num_selector)
- continue;
- if (env.test_name_selector &&
- !strstr(test->test_name, env.test_name_selector))
+ if (!should_run(&env.test_selector,
+ test->test_num, test->test_name))
continue;
test->run_test();
+ /* ensure last sub-test is finalized properly */
+ if (test->subtest_name)
+ test__end_subtest();
+
test->tested = true;
test->pass_cnt = pass_cnt - old_pass_cnt;
test->error_cnt = error_cnt - old_error_cnt;
@@ -359,21 +511,17 @@ int main(int argc, char **argv)
else
env.succ_cnt++;
- if (env.verbose || test->force_log || test->error_cnt) {
- if (env.log_cnt) {
- fprintf(stdout, "%s", env.log_buf);
- if (env.log_buf[env.log_cnt - 1] != '\n')
- fprintf(stdout, "\n");
- }
- }
- env.log_cnt = 0;
+ dump_test_log(test, test->error_cnt);
printf("#%d %s:%s\n", test->test_num, test->test_name,
test->error_cnt ? "FAIL" : "OK");
}
- printf("Summary: %d PASSED, %d FAILED\n", env.succ_cnt, env.fail_cnt);
+ printf("Summary: %d/%d PASSED, %d FAILED\n",
+ env.succ_cnt, env.sub_succ_cnt, env.fail_cnt);
free(env.log_buf);
+ free(env.test_selector.num_set);
+ free(env.subtest_selector.num_set);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 62f55a4231e9..afd14962456f 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -40,9 +40,15 @@ typedef __u16 __sum16;
struct prog_test_def;
+struct test_selector {
+ const char *name;
+ bool *num_set;
+ int num_set_len;
+};
+
struct test_env {
- int test_num_selector;
- const char *test_name_selector;
+ struct test_selector test_selector;
+ struct test_selector subtest_selector;
bool verifier_stats;
bool verbose;
bool very_verbose;
@@ -54,8 +60,9 @@ struct test_env {
size_t log_cnt;
size_t log_cap;
- int succ_cnt;
- int fail_cnt;
+ int succ_cnt; /* successful tests */
+ int sub_succ_cnt; /* successful sub-tests */
+ int fail_cnt; /* total failed tests + sub-tests */
};
extern int error_cnt;
@@ -65,6 +72,7 @@ extern struct test_env env;
extern void test__printf(const char *fmt, ...);
extern void test__vprintf(const char *fmt, va_list args);
extern void test__force_log();
+extern bool test__start_subtest(const char *name);
#define MAGIC_BYTES 123
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 4/9] libbpf: return previous print callback from libbpf_set_print
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
By returning previously set print callback from libbpf_set_print, it's
possible to restore it, eventually. This is useful when running many
independent test with one default print function, but overriding log
verbosity for particular subset of tests.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/libbpf.c | 5 ++++-
tools/lib/bpf/libbpf.h | 2 +-
2 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index 8741c39adb1c..ead915aec349 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -74,9 +74,12 @@ static int __base_pr(enum libbpf_print_level level, const char *format,
static libbpf_print_fn_t __libbpf_pr = __base_pr;
-void libbpf_set_print(libbpf_print_fn_t fn)
+libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn)
{
+ libbpf_print_fn_t old_print_fn = __libbpf_pr;
+
__libbpf_pr = fn;
+ return old_print_fn;
}
__printf(2, 3)
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 5cbf459ece0b..8a9d462a6f6d 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -57,7 +57,7 @@ enum libbpf_print_level {
typedef int (*libbpf_print_fn_t)(enum libbpf_print_level level,
const char *, va_list ap);
-LIBBPF_API void libbpf_set_print(libbpf_print_fn_t fn);
+LIBBPF_API libbpf_print_fn_t libbpf_set_print(libbpf_print_fn_t fn);
/* Hide internal to user */
struct bpf_object;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 6/9] selftests/bpf: abstract away test log output
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
This patch changes how test output is printed out. By default, if test
had no errors, the only output will be a single line with test number,
name, and verdict at the end, e.g.:
#31 xdp:OK
If test had any errors, all log output captured during test execution
will be output after test completes.
It's possible to force output of log with `-v` (`--verbose`) option, in
which case output won't be buffered and will be output immediately.
To support this, individual tests are required to use helper methods for
logging: `test__printf()` and `test__vprintf()`.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
.../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
.../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
.../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
.../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
.../selftests/bpf/prog_tests/map_lock.c | 10 +-
.../selftests/bpf/prog_tests/send_signal.c | 8 +-
.../selftests/bpf/prog_tests/spinlock.c | 2 +-
.../bpf/prog_tests/stacktrace_build_id.c | 4 +-
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
.../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
tools/testing/selftests/bpf/test_progs.h | 37 ++++-
12 files changed, 173 insertions(+), 73 deletions(-)
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
index cb827383db4d..fb5840a62548 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_obj_id.c
@@ -106,8 +106,8 @@ void test_bpf_obj_id(void)
if (CHECK(err ||
prog_infos[i].type != BPF_PROG_TYPE_SOCKET_FILTER ||
info_len != sizeof(struct bpf_prog_info) ||
- (jit_enabled && !prog_infos[i].jited_prog_len) ||
- (jit_enabled &&
+ (env.jit_enabled && !prog_infos[i].jited_prog_len) ||
+ (env.jit_enabled &&
!memcmp(jited_insns, zeros, sizeof(zeros))) ||
!prog_infos[i].xlated_prog_len ||
!memcmp(xlated_insns, zeros, sizeof(zeros)) ||
@@ -121,7 +121,7 @@ void test_bpf_obj_id(void)
err, errno, i,
prog_infos[i].type, BPF_PROG_TYPE_SOCKET_FILTER,
info_len, sizeof(struct bpf_prog_info),
- jit_enabled,
+ env.jit_enabled,
prog_infos[i].jited_prog_len,
prog_infos[i].xlated_prog_len,
!!memcmp(jited_insns, zeros, sizeof(zeros)),
diff --git a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
index ceddb8cc86f4..b59017279e0b 100644
--- a/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
+++ b/tools/testing/selftests/bpf/prog_tests/bpf_verif_scale.c
@@ -4,12 +4,15 @@
static int libbpf_debug_print(enum libbpf_print_level level,
const char *format, va_list args)
{
- if (level != LIBBPF_DEBUG)
- return vfprintf(stderr, format, args);
+ if (level != LIBBPF_DEBUG) {
+ test__vprintf(format, args);
+ return 0;
+ }
if (!strstr(format, "verifier log"))
return 0;
- return vfprintf(stderr, "%s", args);
+ test__vprintf("%s", args);
+ return 0;
}
static int check_load(const char *file, enum bpf_prog_type type)
@@ -73,32 +76,38 @@ void test_bpf_verif_scale(void)
libbpf_print_fn_t old_print_fn = NULL;
int err, i;
- if (verifier_stats)
+ if (env.verifier_stats) {
+ test__force_log();
old_print_fn = libbpf_set_print(libbpf_debug_print);
+ }
err = check_load("./loop3.o", BPF_PROG_TYPE_RAW_TRACEPOINT);
- printf("test_scale:loop3:%s\n", err ? (error_cnt--, "OK") : "FAIL");
+ test__printf("test_scale:loop3:%s\n",
+ err ? (error_cnt--, "OK") : "FAIL");
for (i = 0; i < ARRAY_SIZE(sched_cls); i++) {
err = check_load(sched_cls[i], BPF_PROG_TYPE_SCHED_CLS);
- printf("test_scale:%s:%s\n", sched_cls[i], err ? "FAIL" : "OK");
+ test__printf("test_scale:%s:%s\n", sched_cls[i],
+ err ? "FAIL" : "OK");
}
for (i = 0; i < ARRAY_SIZE(raw_tp); i++) {
err = check_load(raw_tp[i], BPF_PROG_TYPE_RAW_TRACEPOINT);
- printf("test_scale:%s:%s\n", raw_tp[i], err ? "FAIL" : "OK");
+ test__printf("test_scale:%s:%s\n", raw_tp[i],
+ err ? "FAIL" : "OK");
}
for (i = 0; i < ARRAY_SIZE(cg_sysctl); i++) {
err = check_load(cg_sysctl[i], BPF_PROG_TYPE_CGROUP_SYSCTL);
- printf("test_scale:%s:%s\n", cg_sysctl[i], err ? "FAIL" : "OK");
+ test__printf("test_scale:%s:%s\n", cg_sysctl[i],
+ err ? "FAIL" : "OK");
}
err = check_load("./test_xdp_loop.o", BPF_PROG_TYPE_XDP);
- printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
+ test__printf("test_scale:test_xdp_loop:%s\n", err ? "FAIL" : "OK");
err = check_load("./test_seg6_loop.o", BPF_PROG_TYPE_LWT_SEG6LOCAL);
- printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
+ test__printf("test_scale:test_seg6_loop:%s\n", err ? "FAIL" : "OK");
- if (verifier_stats)
+ if (env.verifier_stats)
libbpf_set_print(old_print_fn);
}
diff --git a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
index 9d73a8f932ac..3d59b3c841fe 100644
--- a/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
+++ b/tools/testing/selftests/bpf/prog_tests/get_stack_raw_tp.c
@@ -41,7 +41,7 @@ static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
* just assume it is good if the stack is not empty.
* This could be improved in the future.
*/
- if (jit_enabled) {
+ if (env.jit_enabled) {
found = num_stack > 0;
} else {
for (i = 0; i < num_stack; i++) {
@@ -58,7 +58,7 @@ static void get_stack_print_output(void *ctx, int cpu, void *data, __u32 size)
}
} else {
num_stack = e->kern_stack_size / sizeof(__u64);
- if (jit_enabled) {
+ if (env.jit_enabled) {
good_kern_stack = num_stack > 0;
} else {
for (i = 0; i < num_stack; i++) {
diff --git a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
index 20ddca830e68..5ce572c03a5f 100644
--- a/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
+++ b/tools/testing/selftests/bpf/prog_tests/l4lb_all.c
@@ -74,7 +74,7 @@ static void test_l4lb(const char *file)
}
if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
error_cnt++;
- printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
+ test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
}
out:
bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
index ee99368c595c..2e78217ed3fd 100644
--- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
+++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
@@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
for (i = 0; i < 10000; i++) {
err = bpf_map_lookup_elem_flags(map_fd, &key, vars, BPF_F_LOCK);
if (err) {
- printf("lookup failed\n");
+ test__printf("lookup failed\n");
error_cnt++;
goto out;
}
if (vars[0] != 0) {
- printf("lookup #%d var[0]=%d\n", i, vars[0]);
+ test__printf("lookup #%d var[0]=%d\n", i, vars[0]);
error_cnt++;
goto out;
}
@@ -22,8 +22,8 @@ static void *parallel_map_access(void *arg)
for (j = 2; j < 17; j++) {
if (vars[j] == rnd)
continue;
- printf("lookup #%d var[1]=%d var[%d]=%d\n",
- i, rnd, j, vars[j]);
+ test__printf("lookup #%d var[1]=%d var[%d]=%d\n",
+ i, rnd, j, vars[j]);
error_cnt++;
goto out;
}
@@ -43,7 +43,7 @@ void test_map_lock(void)
err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
if (err) {
- printf("test_map_lock:bpf_prog_load errno %d\n", errno);
+ test__printf("test_map_lock:bpf_prog_load errno %d\n", errno);
goto close_prog;
}
map_fd[0] = bpf_find_map(__func__, obj, "hash_map");
diff --git a/tools/testing/selftests/bpf/prog_tests/send_signal.c b/tools/testing/selftests/bpf/prog_tests/send_signal.c
index 54218ee3c004..d950f4558897 100644
--- a/tools/testing/selftests/bpf/prog_tests/send_signal.c
+++ b/tools/testing/selftests/bpf/prog_tests/send_signal.c
@@ -202,8 +202,8 @@ static int test_send_signal_nmi(void)
-1 /* cpu */, -1 /* group_fd */, 0 /* flags */);
if (pmu_fd == -1) {
if (errno == ENOENT) {
- printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
- __func__);
+ test__printf("%s:SKIP:no PERF_COUNT_HW_CPU_CYCLES\n",
+ __func__);
return 0;
}
/* Let the test fail with a more informative message */
@@ -222,8 +222,4 @@ void test_send_signal(void)
ret |= test_send_signal_tracepoint();
ret |= test_send_signal_perf();
ret |= test_send_signal_nmi();
- if (!ret)
- printf("test_send_signal:OK\n");
- else
- printf("test_send_signal:FAIL\n");
}
diff --git a/tools/testing/selftests/bpf/prog_tests/spinlock.c b/tools/testing/selftests/bpf/prog_tests/spinlock.c
index 114ebe6a438e..deb2db5b85b0 100644
--- a/tools/testing/selftests/bpf/prog_tests/spinlock.c
+++ b/tools/testing/selftests/bpf/prog_tests/spinlock.c
@@ -12,7 +12,7 @@ void test_spinlock(void)
err = bpf_prog_load(file, BPF_PROG_TYPE_CGROUP_SKB, &obj, &prog_fd);
if (err) {
- printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
+ test__printf("test_spin_lock:bpf_prog_load errno %d\n", errno);
goto close_prog;
}
for (i = 0; i < 4; i++)
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
index ac44fda84833..356d2c017a9c 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id.c
@@ -109,8 +109,8 @@ void test_stacktrace_build_id(void)
if (build_id_matches < 1 && retry--) {
bpf_link__destroy(link);
bpf_object__close(obj);
- printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
- __func__);
+ test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+ __func__);
goto retry;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
index 9557b7dfb782..f44f2c159714 100644
--- a/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
+++ b/tools/testing/selftests/bpf/prog_tests/stacktrace_build_id_nmi.c
@@ -140,8 +140,8 @@ void test_stacktrace_build_id_nmi(void)
if (build_id_matches < 1 && retry--) {
bpf_link__destroy(link);
bpf_object__close(obj);
- printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
- __func__);
+ test__printf("%s:WARN:Didn't find expected build ID from the map, retrying\n",
+ __func__);
goto retry;
}
diff --git a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
index 09e6b46f5515..b5404494b8aa 100644
--- a/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
+++ b/tools/testing/selftests/bpf/prog_tests/xdp_noinline.c
@@ -75,7 +75,8 @@ void test_xdp_noinline(void)
}
if (bytes != MAGIC_BYTES * NUM_ITER * 2 || pkts != NUM_ITER * 2) {
error_cnt++;
- printf("test_xdp_noinline:FAIL:stats %lld %lld\n", bytes, pkts);
+ test__printf("test_xdp_noinline:FAIL:stats %lld %lld\n",
+ bytes, pkts);
}
out:
bpf_object__close(obj);
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index 94b6951b90b3..3cf3ebda1d31 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -6,9 +6,75 @@
#include <argp.h>
#include <string.h>
+/* defined in test_progs.h */
+struct test_env env = {
+ .test_num_selector = -1,
+};
int error_cnt, pass_cnt;
-bool jit_enabled;
-bool verifier_stats = false;
+
+struct prog_test_def {
+ const char *test_name;
+ int test_num;
+ void (*run_test)(void);
+ bool force_log;
+ int pass_cnt;
+ int error_cnt;
+ bool tested;
+};
+
+void test__force_log() {
+ env.test->force_log = true;
+}
+
+void test__vprintf(const char *fmt, va_list args)
+{
+ size_t rem_sz;
+ int ret;
+
+ if (env.verbose || (env.test && env.test->force_log)) {
+ vfprintf(stderr, fmt, args);
+ return;
+ }
+
+try_again:
+ rem_sz = env.log_cap - env.log_cnt;
+ if (rem_sz) {
+ ret = vsnprintf(env.log_buf + env.log_cnt, rem_sz, fmt, args);
+ if (ret < 0) {
+ fprintf(stderr, "failed to log message w/ fmt '%s'\n", fmt);
+ return;
+ }
+ }
+
+ if (ret >= rem_sz) {
+ size_t new_sz = env.log_cap * 3 / 2;
+ char *new_buf;
+
+ if (new_sz < 4096)
+ new_sz = 4096;
+
+ new_buf = realloc(env.log_buf, new_sz);
+ if (!new_buf) {
+ fprintf(stderr, "failed to realloc log buffer: %d\n",
+ errno);
+ return;
+ }
+ env.log_buf = new_buf;
+ env.log_cap = new_sz;
+ goto try_again;
+ }
+
+ env.log_cnt += ret + 1;
+}
+
+void test__printf(const char *fmt, ...)
+{
+ va_list args;
+
+ va_start(args, fmt);
+ test__vprintf(fmt, args);
+ va_end(args);
+}
struct ipv4_packet pkt_v4 = {
.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
@@ -163,20 +229,15 @@ void *spin_lock_thread(void *arg)
#include <prog_tests/tests.h>
#undef DEFINE_TEST
-struct prog_test_def {
- const char *test_name;
- int test_num;
- void (*run_test)(void);
-};
-
static struct prog_test_def prog_test_defs[] = {
-#define DEFINE_TEST(name) { \
- .test_name = #name, \
- .run_test = &test_##name, \
+#define DEFINE_TEST(name) { \
+ .test_name = #name, \
+ .run_test = &test_##name, \
},
#include <prog_tests/tests.h>
#undef DEFINE_TEST
};
+const int prog_test_cnt = ARRAY_SIZE(prog_test_defs);
const char *argp_program_version = "test_progs 0.1";
const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
@@ -186,7 +247,6 @@ enum ARG_KEYS {
ARG_TEST_NUM = 'n',
ARG_TEST_NAME = 't',
ARG_VERIFIER_STATS = 's',
-
ARG_VERBOSE = 'v',
};
@@ -202,24 +262,13 @@ static const struct argp_option opts[] = {
{},
};
-struct test_env {
- int test_num_selector;
- const char *test_name_selector;
- bool verifier_stats;
- bool verbose;
- bool very_verbose;
-};
-
-static struct test_env env = {
- .test_num_selector = -1,
-};
-
static int libbpf_print_fn(enum libbpf_print_level level,
const char *format, va_list args)
{
if (!env.very_verbose && level == LIBBPF_DEBUG)
return 0;
- return vfprintf(stderr, format, args);
+ test__vprintf(format, args);
+ return 0;
}
static error_t parse_arg(int key, char *arg, struct argp_state *state)
@@ -267,7 +316,6 @@ static error_t parse_arg(int key, char *arg, struct argp_state *state)
return 0;
}
-
int main(int argc, char **argv)
{
static const struct argp argp = {
@@ -275,7 +323,6 @@ int main(int argc, char **argv)
.parser = parse_arg,
.doc = argp_program_doc,
};
- struct prog_test_def *test;
int err, i;
err = argp_parse(&argp, argc, argv, 0, NULL, &env);
@@ -286,13 +333,14 @@ int main(int argc, char **argv)
srand(time(NULL));
- jit_enabled = is_jit_enabled();
+ env.jit_enabled = is_jit_enabled();
- verifier_stats = env.verifier_stats;
-
- for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
- test = &prog_test_defs[i];
+ for (i = 0; i < prog_test_cnt; i++) {
+ struct prog_test_def *test = &prog_test_defs[i];
+ int old_pass_cnt = pass_cnt;
+ int old_error_cnt = error_cnt;
+ env.test = test;
test->test_num = i + 1;
if (env.test_num_selector >= 0 &&
@@ -303,8 +351,29 @@ int main(int argc, char **argv)
continue;
test->run_test();
+ test->tested = true;
+ test->pass_cnt = pass_cnt - old_pass_cnt;
+ test->error_cnt = error_cnt - old_error_cnt;
+ if (test->error_cnt)
+ env.fail_cnt++;
+ else
+ env.succ_cnt++;
+
+ if (env.verbose || test->force_log || test->error_cnt) {
+ if (env.log_cnt) {
+ fprintf(stdout, "%s", env.log_buf);
+ if (env.log_buf[env.log_cnt - 1] != '\n')
+ fprintf(stdout, "\n");
+ }
+ }
+ env.log_cnt = 0;
+
+ printf("#%d %s:%s\n", test->test_num, test->test_name,
+ test->error_cnt ? "FAIL" : "OK");
}
+ printf("Summary: %d PASSED, %d FAILED\n", env.succ_cnt, env.fail_cnt);
+
+ free(env.log_buf);
- printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
}
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
index 49e0f7d85643..62f55a4231e9 100644
--- a/tools/testing/selftests/bpf/test_progs.h
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -38,9 +38,33 @@ typedef __u16 __sum16;
#include "trace_helpers.h"
#include "flow_dissector_load.h"
-extern int error_cnt, pass_cnt;
-extern bool jit_enabled;
-extern bool verifier_stats;
+struct prog_test_def;
+
+struct test_env {
+ int test_num_selector;
+ const char *test_name_selector;
+ bool verifier_stats;
+ bool verbose;
+ bool very_verbose;
+
+ bool jit_enabled;
+
+ struct prog_test_def *test;
+ char *log_buf;
+ size_t log_cnt;
+ size_t log_cap;
+
+ int succ_cnt;
+ int fail_cnt;
+};
+
+extern int error_cnt;
+extern int pass_cnt;
+extern struct test_env env;
+
+extern void test__printf(const char *fmt, ...);
+extern void test__vprintf(const char *fmt, va_list args);
+extern void test__force_log();
#define MAGIC_BYTES 123
@@ -64,11 +88,12 @@ extern struct ipv6_packet pkt_v6;
int __ret = !!(condition); \
if (__ret) { \
error_cnt++; \
- printf("%s:FAIL:%s ", __func__, tag); \
- printf(format); \
+ test__printf("%s:FAIL:%s ", __func__, tag); \
+ test__printf(format); \
} else { \
pass_cnt++; \
- printf("%s:PASS:%s %d nsec\n", __func__, tag, duration);\
+ test__printf("%s:PASS:%s %d nsec\n", \
+ __func__, tag, duration); \
} \
__ret; \
})
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Apprently listing header as a normal dependency for a binary output
makes it go through compilation as if it was C code. This currently
works without a problem, but in subsequent commits causes problems for
differently generated test.h for test_progs. Marking those headers as
order-only dependency solves the issue.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/Makefile | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index 11c9c62c3362..bb66cc4a7f34 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
test_progs.c: $(PROG_TESTS_H)
$(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
-$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
+$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
$(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
$(shell ( cd prog_tests/; \
echo '/* Generated header, do not edit */'; \
@@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
MAP_TESTS_FILES := $(wildcard map_tests/*.c)
test_maps.c: $(MAP_TESTS_H)
$(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
-$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
+$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
$(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
$(shell ( cd map_tests/; \
echo '/* Generated header, do not edit */'; \
@@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
test_verifier.c: $(VERIFIER_TESTS_H)
$(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
-$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
+$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
$(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
$(shell ( cd verifier/; \
echo '/* Generated header, do not edit */'; \
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 2/9] selftests/bpf: revamp test_progs to allow more control
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Refactor test_progs to allow better control on what's being run.
Also use argp to do argument parsing, so that it's easier to keep adding
more options.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/Makefile | 8 +--
tools/testing/selftests/bpf/test_progs.c | 84 +++++++++++++++++++++---
2 files changed, 77 insertions(+), 15 deletions(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index bb66cc4a7f34..3bd0f4a0336a 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -239,14 +239,8 @@ $(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
$(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
$(shell ( cd prog_tests/; \
echo '/* Generated header, do not edit */'; \
- echo '#ifdef DECLARE'; \
ls *.c 2> /dev/null | \
- sed -e 's@\([^\.]*\)\.c@extern void test_\1(void);@'; \
- echo '#endif'; \
- echo '#ifdef CALL'; \
- ls *.c 2> /dev/null | \
- sed -e 's@\([^\.]*\)\.c@test_\1();@'; \
- echo '#endif' \
+ sed -e 's@\([^\.]*\)\.c@DEFINE_TEST(\1)@'; \
) > $(PROG_TESTS_H))
MAP_TESTS_DIR = $(OUTPUT)/map_tests
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index dae0819b1141..eea88ba59225 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -3,6 +3,7 @@
*/
#include "test_progs.h"
#include "bpf_rlimit.h"
+#include <argp.h>
int error_cnt, pass_cnt;
bool jit_enabled;
@@ -156,22 +157,89 @@ void *spin_lock_thread(void *arg)
pthread_exit(arg);
}
-#define DECLARE
+/* extern declarations for test funcs */
+#define DEFINE_TEST(name) extern void test_##name();
#include <prog_tests/tests.h>
-#undef DECLARE
+#undef DEFINE_TEST
-int main(int ac, char **av)
+struct prog_test_def {
+ const char *test_name;
+ void (*run_test)(void);
+};
+
+static struct prog_test_def prog_test_defs[] = {
+#define DEFINE_TEST(name) { \
+ .test_name = #name, \
+ .run_test = &test_##name, \
+},
+#include <prog_tests/tests.h>
+#undef DEFINE_TEST
+};
+
+const char *argp_program_version = "test_progs 0.1";
+const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
+const char argp_program_doc[] = "BPF selftests test runner";
+
+enum ARG_KEYS {
+ ARG_VERIFIER_STATS = 's',
+};
+
+static const struct argp_option opts[] = {
+ { "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
+ "Output verifier statistics", },
+ {},
+};
+
+struct test_env {
+ bool verifier_stats;
+};
+
+static struct test_env env = {};
+
+static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
+ struct test_env *env = state->input;
+
+ switch (key) {
+ case ARG_VERIFIER_STATS:
+ env->verifier_stats = true;
+ break;
+ case ARGP_KEY_ARG:
+ argp_usage(state);
+ break;
+ case ARGP_KEY_END:
+ break;
+ default:
+ return ARGP_ERR_UNKNOWN;
+ }
+ return 0;
+}
+
+
+int main(int argc, char **argv)
+{
+ static const struct argp argp = {
+ .options = opts,
+ .parser = parse_arg,
+ .doc = argp_program_doc,
+ };
+ const struct prog_test_def *def;
+ int err, i;
+
+ err = argp_parse(&argp, argc, argv, 0, NULL, &env);
+ if (err)
+ return err;
+
srand(time(NULL));
jit_enabled = is_jit_enabled();
- if (ac == 2 && strcmp(av[1], "-s") == 0)
- verifier_stats = true;
+ verifier_stats = env.verifier_stats;
-#define CALL
-#include <prog_tests/tests.h>
-#undef CALL
+ for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
+ def = &prog_test_defs[i];
+ def->run_test();
+ }
printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
return error_cnt ? EXIT_FAILURE : EXIT_SUCCESS;
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 3/9] selftests/bpf: add test selectors by number and name to test_progs
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
In-Reply-To: <20190727190150.649137-1-andriin@fb.com>
Add ability to specify either test number or test name substring to
narrow down a set of test to run.
Usage:
sudo ./test_progs -n 1
sudo ./test_progs -t attach_probe
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_progs.c | 43 +++++++++++++++++++++---
1 file changed, 39 insertions(+), 4 deletions(-)
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index eea88ba59225..6e04b9f83777 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -4,6 +4,7 @@
#include "test_progs.h"
#include "bpf_rlimit.h"
#include <argp.h>
+#include <string.h>
int error_cnt, pass_cnt;
bool jit_enabled;
@@ -164,6 +165,7 @@ void *spin_lock_thread(void *arg)
struct prog_test_def {
const char *test_name;
+ int test_num;
void (*run_test)(void);
};
@@ -181,26 +183,49 @@ const char *argp_program_bug_address = "<bpf@vger.kernel.org>";
const char argp_program_doc[] = "BPF selftests test runner";
enum ARG_KEYS {
+ ARG_TEST_NUM = 'n',
+ ARG_TEST_NAME = 't',
ARG_VERIFIER_STATS = 's',
};
static const struct argp_option opts[] = {
+ { "num", ARG_TEST_NUM, "NUM", 0,
+ "Run test number NUM only " },
+ { "name", ARG_TEST_NAME, "NAME", 0,
+ "Run tests with names containing NAME" },
{ "verifier-stats", ARG_VERIFIER_STATS, NULL, 0,
"Output verifier statistics", },
{},
};
struct test_env {
+ int test_num_selector;
+ const char *test_name_selector;
bool verifier_stats;
};
-static struct test_env env = {};
+static struct test_env env = {
+ .test_num_selector = -1,
+};
static error_t parse_arg(int key, char *arg, struct argp_state *state)
{
struct test_env *env = state->input;
switch (key) {
+ case ARG_TEST_NUM: {
+ int test_num;
+
+ errno = 0;
+ test_num = strtol(arg, NULL, 10);
+ if (errno)
+ return -errno;
+ env->test_num_selector = test_num;
+ break;
+ }
+ case ARG_TEST_NAME:
+ env->test_name_selector = arg;
+ break;
case ARG_VERIFIER_STATS:
env->verifier_stats = true;
break;
@@ -223,7 +248,7 @@ int main(int argc, char **argv)
.parser = parse_arg,
.doc = argp_program_doc,
};
- const struct prog_test_def *def;
+ struct prog_test_def *test;
int err, i;
err = argp_parse(&argp, argc, argv, 0, NULL, &env);
@@ -237,8 +262,18 @@ int main(int argc, char **argv)
verifier_stats = env.verifier_stats;
for (i = 0; i < ARRAY_SIZE(prog_test_defs); i++) {
- def = &prog_test_defs[i];
- def->run_test();
+ test = &prog_test_defs[i];
+
+ test->test_num = i + 1;
+
+ if (env.test_num_selector >= 0 &&
+ test->test_num != env.test_num_selector)
+ continue;
+ if (env.test_name_selector &&
+ !strstr(test->test_name, env.test_name_selector))
+ continue;
+
+ test->run_test();
}
printf("Summary: %d PASSED, %d FAILED\n", pass_cnt, error_cnt);
--
2.17.1
^ permalink raw reply related
* [PATCH v2 bpf-next 0/9] Revamp test_progs as a test running framework
From: Andrii Nakryiko @ 2019-07-27 19:01 UTC (permalink / raw)
To: bpf, netdev, ast, daniel, sdf
Cc: andrii.nakryiko, kernel-team, Andrii Nakryiko
This patch set makes a number of changes to test_progs selftest, which is
a collection of many other tests (and sometimes sub-tests as well), to provide
better testing experience and allow to start convering many individual test
programs under selftests/bpf into a single and convenient test runner.
Patch #1 fixes issue with Makefile, which makes prog_tests/test.h compiled as
a C code. This fix allows to change how test.h is generated, providing ability
to have more control on what and how tests are run.
Patch #2 changes how test.h is auto-generated, which allows to have test
definitions, instead of just running test functions. This gives ability to do
more complicated test run policies.
Patch #3 adds `-t <test-name>` and `-n <test-num>` selectors to run only
subset of tests.
Patch #4 changes libbpf_set_print() to return previously set print callback,
allowing to temporarily replace current print callback and then set it back.
This is necessary for some tests that want more control over libbpf logging.
Patch #5 sets up and takes over libbpf logging from individual tests to
test_prog runner, adding -vv verbosity to capture debug output from libbpf.
This is useful when debugging failing tests.
Patch #6 furthers test output management and buffers it by default, emitting
log output only if test fails. This give succinct and clean default test
output. It's possible to bypass this behavior with -v flag, which will turn
off test output buffering.
Patch #7 adds support for sub-tests. It also enhances -t and -n selectors to
both support ability to specify sub-test selectors, as well as enhancing
number selector to accept sets of test, instead of just individual test
number.
Patch #8 converts bpf_verif_scale.c test to use sub-test APIs.
Patch #9 converts send_signal.c tests to use sub-test APIs.
v1->v2:
- drop libbpf_swap_print, instead return previous function from
libbpf_set_print (Stanislav);
Andrii Nakryiko (9):
selftests/bpf: prevent headers to be compiled as C code
selftests/bpf: revamp test_progs to allow more control
selftests/bpf: add test selectors by number and name to test_progs
libbpf: return previous print callback from libbpf_set_print
selftest/bpf: centralize libbpf logging management for test_progs
selftests/bpf: abstract away test log output
selftests/bpf: add sub-tests support for test_progs
selftests/bpf: convert bpf_verif_scale.c to sub-tests API
selftests/bpf: convert send_signal.c to use subtests
tools/lib/bpf/libbpf.c | 5 +-
tools/lib/bpf/libbpf.h | 2 +-
tools/testing/selftests/bpf/Makefile | 14 +-
.../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
.../bpf/prog_tests/bpf_verif_scale.c | 90 +++--
.../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
.../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
.../selftests/bpf/prog_tests/map_lock.c | 10 +-
.../bpf/prog_tests/reference_tracking.c | 15 +-
.../selftests/bpf/prog_tests/send_signal.c | 17 +-
.../selftests/bpf/prog_tests/spinlock.c | 2 +-
.../bpf/prog_tests/stacktrace_build_id.c | 4 +-
.../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
.../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
tools/testing/selftests/bpf/test_progs.c | 373 +++++++++++++++++-
tools/testing/selftests/bpf/test_progs.h | 45 ++-
16 files changed, 492 insertions(+), 104 deletions(-)
--
2.17.1
^ permalink raw reply
* Re: [PATCH bpf-next 02/10] libbpf: implement BPF CO-RE offset relocation algorithm
From: Song Liu @ 2019-07-27 18:59 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Yonghong Song, Kernel Team
In-Reply-To: <CAEf4BzZKA29xudKC8WWEXJq+egTCgX4bV9KaE0Y+_u50=D70iQ@mail.gmail.com>
> On Jul 26, 2019, at 11:11 PM, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Thu, Jul 25, 2019 at 12:32 PM Song Liu <songliubraving@fb.com> wrote:
>>
>>
>>
>>> On Jul 24, 2019, at 12:27 PM, Andrii Nakryiko <andriin@fb.com> wrote:
>>>
>>> This patch implements the core logic for BPF CO-RE offsets relocations.
>>> All the details are described in code comments.
>>
>> Some description in the change log is still useful. Please at least
>> copy-paste key comments here.
>
> OK, will add some more.
>
>>
>> And, this is looooong. I think it is totally possible to split it into
>> multiple smaller patches.
>
> I don't really know how to split it further without hurting reviewing
> by artificially splitting related code into separate patches. Remove
> any single function and algorithm will be incomplete.
>
> Let me give you some high-level overview of how pieces are put
> together. There are 9 non-trivial functions, let's go over their
> purpose in the orderd in which they are defined in file:
>
> 1. bpf_core_spec_parse()
>
> This one take bpf_offset_reloc's type_id and accessor string
> ("0:1:2:3") and parses it into more convenient bpf_core_spec
> datastructure, which has calculated offset and high-level spec
> "steps": either named field or array access.
>
> 2. bpf_core_find_cands()
>
> Given local type name, finds all possible target BTF types with same
> name (modulo "flavor" differences, ___flavor suffix is just ignored).
>
> 3. bpf_core_fields_are_compat()
>
> Given local and target field match, checks that their types are
> compatible (so that we don't accidentally match, e.g., int against
> struct).
>
> 4. bpf_core_match_member()
>
> Given named local field, find corresponding field in target struct. To
> understand why it's not trivial, here's an example:
>
> Local type:
>
> struct s___local {
> int a;
> };
>
> Target type:
>
> struct s___target {
> struct {
> union {
> int a;
> };
> };
> };
>
> For both cases you can access a as s.a, but in local case, field a is
> immediately inside s___local, while for s___target, you have to
> traverse two levels deeper into anonymous fields to get to an `a`
> inside anonymous union.
>
> So this function find that `a` by doing exhaustive search across all
> named field and anonymous struct/unions. But otherwise it's pretty
> straightforward recursive function.
>
> bpf_core_spec_match()
>
> Just goes over high-level spec steps in local spec and tries to figure
> out both high-level and low-level steps for targe type. Consider the
> above example. For both structs accessing s.a is one high-level step,
> but for s___local it's single low-level step (just another :0 in spec
> string), while for s___target it's three low-level steps: ":0:0:0",
> one step for each BTF type we need to traverse.
>
> Array access is simpler, it's always one high-level and one low-level step.
>
> bpf_core_reloc_insn()
>
> Once we match local and target specs and have local and target
> offsets, do the relocations - check that instruction has expected
> local offset and replace it with target offset.
>
> bpf_core_find_kernel_btf()
>
> This is the only function that can be moved into separate patch, but
> it's also very simple. It just iterates over few known possible
> locations for vmlinux image and once found, tries to parse .BTF out of
> it, to be used as target BTF.
>
> bpf_core_reloc_offset()
>
> It combines all the above functions to perform single relocation.
> Parse spec, get candidates, for each candidate try to find matching
> target spec. All candidates that matched are cached for given local
> root type.
Thanks for these explanation. They are really helpful.
I think some example explaining each step of bpf_core_reloc_offset()
will be very helpful. Something like:
Example:
struct s {
int a;
struct {
int b;
bool c;
};
};
To get offset for c, we do:
bpf_core_reloc_offset() {
/* input data: xxx */
/* first step: bpf_core_spec_parse() */
/* data after first step */
/* second step: bpf_core_find_cands() */
/* candidate A and B after second step */
...
}
Well, it requires quite some work to document this way. Please let me
know if you feel this is an overkill.
>
> bpf_core_reloc_offsets()
>
> High-level coordination. Iterate over all per-program .BTF.ext offset
> reloc sections, each relocation within them. Find corresponding
> program and try to apply relocations one by one.
>
>
> I think the only non-obvious part here is to understand that
> relocation records local raw spec with every single anonymous type
> traversal, which is not that useful when we try to match it against
> target type, which can have very different composition, but still the
> same field access pattern, from C language standpoint (which hides all
> those anonymous type traversals from programmer).
>
> But it should be pretty clear now, plus also check tests, they have
> lots of cases showing what's compatible and what's not.
I see. I will review the tests.
>>>
>>> static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
>>> - __u32 id)
>>> + __u32 id,
>>> + __u32 *res_id)
>>> {
>>> const struct btf_type *t = btf__type_by_id(btf, id);
>>
>> Maybe have a local "__u32 rid;"
>>
>>>
>>> + if (res_id)
>>> + *res_id = id;
>>> +
>>
>> and do "rid = id;" here
>>
>>> while (true) {
>>> switch (BTF_INFO_KIND(t->info)) {
>>> case BTF_KIND_VOLATILE:
>>> case BTF_KIND_CONST:
>>> case BTF_KIND_RESTRICT:
>>> case BTF_KIND_TYPEDEF:
>>> + if (res_id)
>>> + *res_id = t->type;
>> and here
>>
>>> t = btf__type_by_id(btf, t->type);
>>> break;
>>> default:
>> and "*res_id = rid;" right before return?
>
> Sure, but why?
I think it is cleaner that way. But feel free to ignore if you
think otherwise.
>
>>
>>> @@ -1041,7 +1049,7 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
>>> static bool get_map_field_int(const char *map_name, const struct btf *btf,
>>> const struct btf_type *def,
>>> const struct btf_member *m, __u32 *res) {
>
> [...]
>
>>> +struct bpf_core_spec {
>>> + const struct btf *btf;
>>> + /* high-level spec: named fields and array indicies only */
>>
>> typo: indices
>
> thanks!
>
>>
>>> + struct bpf_core_accessor spec[BPF_CORE_SPEC_MAX_LEN];
>>> + /* high-level spec length */
>>> + int len;
>>> + /* raw, low-level spec: 1-to-1 with accessor spec string */
>>> + int raw_spec[BPF_CORE_SPEC_MAX_LEN];
>>> + /* raw spec length */
>>> + int raw_len;
>>> + /* field byte offset represented by spec */
>>> + __u32 offset;
>>> +};
>
> [...]
>
>>> + *
>>> + * int x = &s->a[3]; // access string = '0:1:2:3'
>>> + *
>>> + * Low-level spec has 1:1 mapping with each element of access string (it's
>>> + * just a parsed access string representation): [0, 1, 2, 3].
>>> + *
>>> + * High-level spec will capture only 3 points:
>>> + * - intial zero-index access by pointer (&s->... is the same as &s[0]...);
>>> + * - field 'a' access (corresponds to '2' in low-level spec);
>>> + * - array element #3 access (corresponds to '3' in low-level spec).
>>> + *
>>> + */
>>
>> IIUC, high-level points are subset of low-level points. How about we introduce
>> "anonymous" high-level points, so that high-level points and low-level points
>> are 1:1 mapping?
>
> No, that will just hurt and complicate things. See above explanation
> about why we need high-level points (it's what you as C programmer try
> to achieve vs low-level spec is what C-language does in reality, with
> all the anonymous struct/union traversal).
>
> What's wrong with this separation? Think about it as recording
> "intent" (high-level spec) vs "mechanics" (low-level spec, how exactly
> to achieve that intent, in excruciating details).
There is nothing wrong with separation. I just personally think it is
cleaner the other way. That's why I raised the question.
I will go with your assessment, as you looked into this much more than
I did. :-)
[...]
>>> +
>>> + memset(spec, 0, sizeof(*spec));
>>> + spec->btf = btf;
>>> +
>>> + /* parse spec_str="0:1:2:3:4" into array raw_spec=[0, 1, 2, 3, 4] */
>>> + while (*spec_str) {
>>> + if (*spec_str == ':')
>>> + ++spec_str;
>>> + if (sscanf(spec_str, "%d%n", &access_idx, &parsed_len) != 1)
>>> + return -EINVAL;
>>> + if (spec->raw_len == BPF_CORE_SPEC_MAX_LEN)
>>> + return -E2BIG;
>>> + spec_str += parsed_len;
>>> + spec->raw_spec[spec->raw_len++] = access_idx;
>>> + }
>>> +
>>> + if (spec->raw_len == 0)
>>> + return -EINVAL;
>>> +
>>> + for (i = 0; i < spec->raw_len; i++) {
>>> + t = skip_mods_and_typedefs(btf, id, &id);
>>> + if (!t)
>>> + return -EINVAL;
>>> +
>>> + access_idx = spec->raw_spec[i];
>>> +
>>> + if (i == 0) {
>>> + /* first spec value is always reloc type array index */
>>> + spec->spec[spec->len].type_id = id;
>>> + spec->spec[spec->len].idx = access_idx;
>>> + spec->len++;
>>> +
>>> + sz = btf__resolve_size(btf, id);
>>> + if (sz < 0)
>>> + return sz;
>>> + spec->offset += access_idx * sz;
>> spec->offset = access_idx * sz; should be enough
>
> No. spec->offset is carefully maintained across multiple low-level
> steps, as we traverse down embedded structs/unions.
>
> Think about, e.g.:
>
> struct s {
> int a;
> struct {
> int b;
> };
> };
>
> Imagine you are trying to match s.b access. With what you propose
> you'll end up with offset 0, but it should be 4.
Hmm... this is just for i == 0, right? Which line updated spec->offset
after "memset(spec, 0, sizeof(*spec));"?
>
>>
>>> + continue;
>>> + }
>>
>> Maybe pull i == 0 case out of the for loop?
>>
>>> +
>>> + if (btf_is_composite(t)) {
>
> [...]
>
>>> +
>>> + if (spec->len == 0)
>>> + return -EINVAL;
>>
>> Can this ever happen?
>
> Not really, because I already check raw_len == 0 and exit with error.
> I'll remove.
>
>>
>>> +
>>> + return 0;
>>> +}
>>> +
>
> [...]
>
>>> +
>>> +/*
>>> + * Given single high-level accessor (either named field or array index) in
>>> + * local type, find corresponding high-level accessor for a target type. Along
>>> + * the way, maintain low-level spec for target as well. Also keep updating
>>> + * target offset.
>>> + */
>>
>> Please describe the recursive algorithm here. I am kinda lost.
>
> Explained above. I'll extend description a bit. But it's just
> recursive exhaustive search:
> 1. if struct field is anonymous and is struct/union, go one level
> deeper and try to find field with given name inside those.
> 2. if field has name and it matched what we are searching - check type
> compatibility. It has to be compatible, so if it's not, then it's not
> a match.
>
>> Also, please document the meaning of zero, positive, negative return values.
>
> Ok. It's standard <0 - error, 0 - false, 1 - true.
>
>>
>>> +static int bpf_core_match_member(const struct btf *local_btf,
>>> + const struct bpf_core_accessor *local_acc,
>>> + const struct btf *targ_btf,
>>> + __u32 targ_id,
>>> + struct bpf_core_spec *spec,
>>> + __u32 *next_targ_id)
>>> +{
>
> [...]
>
>>> + if (local_acc->name) {
>>> + if (!btf_is_composite(targ_type))
>>> + return 0;
>>> +
>>> + matched = bpf_core_match_member(local_spec->btf,
>>> + local_acc,
>>> + targ_btf, targ_id,
>>> + targ_spec, &targ_id);
>>> + if (matched <= 0)
>>> + return matched;
>>> + } else {
>>> + /* for i=0, targ_id is already treated as array element
>>> + * type (because it's the original struct), for others
>>> + * we should find array element type first
>>> + */
>>> + if (i > 0) {
>>
>> i == 0 case would go into "if (local_acc->name)" branch, no?
>
> No, i == 0 is always an array access. s->a.b.c is the same as
> s[0].a.b.c, so relocation's first spec element is always either zero
> for pointer access or any non-negative index for array access. But it
> is always array access.
I see. Thanks for the explanation.
Song
^ permalink raw reply
* Re: [PATCH bpf-next 6/9] selftests/bpf: abstract away test log output
From: Andrii Nakryiko @ 2019-07-27 18:56 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <20190726222652.GG24397@mini-arch>
On Fri, Jul 26, 2019 at 3:26 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:31 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > This patch changes how test output is printed out. By default, if test
> > > > had no errors, the only output will be a single line with test number,
> > > > name, and verdict at the end, e.g.:
> > > >
> > > > #31 xdp:OK
> > > >
> > > > If test had any errors, all log output captured during test execution
> > > > will be output after test completes.
> > > >
> > > > It's possible to force output of log with `-v` (`--verbose`) option, in
> > > > which case output won't be buffered and will be output immediately.
> > > >
> > > > To support this, individual tests are required to use helper methods for
> > > > logging: `test__printf()` and `test__vprintf()`.
> > > >
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > > .../selftests/bpf/prog_tests/bpf_obj_id.c | 6 +-
> > > > .../bpf/prog_tests/bpf_verif_scale.c | 31 ++--
> > > > .../bpf/prog_tests/get_stack_raw_tp.c | 4 +-
> > > > .../selftests/bpf/prog_tests/l4lb_all.c | 2 +-
> > > > .../selftests/bpf/prog_tests/map_lock.c | 10 +-
> > > > .../selftests/bpf/prog_tests/send_signal.c | 8 +-
> > > > .../selftests/bpf/prog_tests/spinlock.c | 2 +-
> > > > .../bpf/prog_tests/stacktrace_build_id.c | 4 +-
> > > > .../bpf/prog_tests/stacktrace_build_id_nmi.c | 4 +-
> > > > .../selftests/bpf/prog_tests/xdp_noinline.c | 3 +-
> > > > tools/testing/selftests/bpf/test_progs.c | 135 +++++++++++++-----
> > > > tools/testing/selftests/bpf/test_progs.h | 37 ++++-
> > > > 12 files changed, 173 insertions(+), 73 deletions(-)
> > > >
> >
> > [...]
> >
> > > > error_cnt++;
> > > > - printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > > + test__printf("test_l4lb:FAIL:stats %lld %lld\n", bytes, pkts);
> > > #define printf(...) test__printf(...) in tests.h?
> > >
> > > A bit ugly, but no need to retrain everyone to use new printf wrappers.
> >
> > I try to reduce amount of magic and surprising things, not add new
> > ones :) I also led by example and converted all current instances of
> > printf usage to test__printf, so anyone new will just copy/paste good
> > example, hopefully. Even if not, this non-buffered output will be
> > immediately obvious to anyone who just runs `sudo ./test_progs`.
>
> [..]
> > And
> > author of new test with this problem should hopefully be the first and
> > the only one to catch and fix this.
> Yeah, that is my only concern, that regular printfs will eventually
> creep in. It's already confusing to go to/from printf/printk.
We should catch that in code review, but Alexei and Daniel will be the
last line of defense anywas, as they run test_progs before merging
stuff and will immediately notice extra non-buffered output, given
that successful output from test_progs is now very laconic.
>
> 2c:
>
> I'm coming from a perspective of tools/testing/selftests/kselftest.h
> which is supposed to be a generic framework with custom
> printf variants (ksft_print_msg), but I still see a bunch of tests
> calling printf :-/
>
> grep -ril ksft_exit_fail_msg selftests/ | xargs -n1 grep -w printf
>
> Since we don't expect regular buffered io from the tests anyway
> it might be easier just to add a bit of magic and call it a day.
>
> > > > }
> > > > out:
> > > > bpf_object__close(obj);
> > > > diff --git a/tools/testing/selftests/bpf/prog_tests/map_lock.c b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > > index ee99368c595c..2e78217ed3fd 100644
> > > > --- a/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > > +++ b/tools/testing/selftests/bpf/prog_tests/map_lock.c
> > > > @@ -9,12 +9,12 @@ static void *parallel_map_access(void *arg)
> >
> > [...]
^ permalink raw reply
* Re: [PATCH bpf-next 1/9] selftests/bpf: prevent headers to be compiled as C code
From: Andrii Nakryiko @ 2019-07-27 18:53 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Andrii Nakryiko, bpf, Networking, Alexei Starovoitov,
Daniel Borkmann, Kernel Team
In-Reply-To: <20190726220119.GE24397@mini-arch>
On Fri, Jul 26, 2019 at 3:01 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
>
> On 07/26, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:21 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > Apprently listing header as a normal dependency for a binary output
> > > > makes it go through compilation as if it was C code. This currently
> > > > works without a problem, but in subsequent commits causes problems for
> > > > differently generated test.h for test_progs. Marking those headers as
> > > > order-only dependency solves the issue.
> > > Are you sure it will not result in a situation where
> > > test_progs/test_maps is not regenerated if tests.h is updated.
> > >
> > > If I read the following doc correctly, order deps make sense for
> > > directories only:
> > > https://www.gnu.org/software/make/manual/html_node/Prerequisite-Types.html
> > >
> > > Can you maybe double check it with:
> > > * make
> > > * add new prog_tests/test_something.c
> > > * make
> > > to see if the binary is regenerated with test_something.c?
> >
> > Yeah, tested that, it triggers test_progs rebuild.
> >
> > Ordering is still preserved, because test.h is dependency of
> > test_progs.c, which is dependency of test_progs binary, so that's why
> > it works.
> >
> > As to why .h file is compiled as C file, I have no idea and ideally
> > that should be fixed somehow.
> I guess that's because it's a prerequisite and we have a target that
> puts all prerequisites when calling CC:
>
> test_progs: a.c b.c tests.h
> gcc a.c b.c tests.h -o test_progs
>
> So gcc compiles each input file.
If that's really a definition of the rule, then it seems not exactly
correct. What if some of the prerequisites are some object files,
directories, etc. I'd say the rule should only include .c files. I'll
add it to my TODO list to go and check how all this is defined
somewhere, but for now I'm leaving everything as is in this patch.
>
> I'm not actually sure why default dependency system that uses 'gcc -M'
> is not working for us (see scripts/Kbuild.include) and we need to manually
> add tests.h dependency. But that's outside of the scope..
>
> > I also started with just removing header as dependency completely
> > (because it's indirect dependency of test_progs.c), but that broke the
> > build logic. Dunno, too much magic... This works, tested many-many
> > times, so I was satisfied enough :)
> Yeah, that's my only concern, too much magic already and we add
> quite a bit more.
>
> > > Maybe fix the problem of header compilation by having '#ifndef
> > > DECLARE_TEST #define DECLARE_TEST() #endif' in tests.h instead?
> >
> > That's ugly, I'd like to avoid doing that.
> That's your call, but I'm not sure what's uglier: complicating already
> complex make rules or making a header self contained.
The right call is to fix selftests/bpf Makefile for good (there are
more issues, e.g., we rebuild all the prog_tests/*.c for alu32 tests,
even if we only modified test_progs.c), but that's for another patch
set, unless someone beats me and fixes that first.
>
> > > > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > > > ---
> > > > tools/testing/selftests/bpf/Makefile | 6 +++---
> > > > 1 file changed, 3 insertions(+), 3 deletions(-)
> > > >
> > > > diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> > > > index 11c9c62c3362..bb66cc4a7f34 100644
> > > > --- a/tools/testing/selftests/bpf/Makefile
> > > > +++ b/tools/testing/selftests/bpf/Makefile
> > > > @@ -235,7 +235,7 @@ PROG_TESTS_H := $(PROG_TESTS_DIR)/tests.h
> > > > PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
> > > > test_progs.c: $(PROG_TESTS_H)
> > > > $(OUTPUT)/test_progs: CFLAGS += $(TEST_PROGS_CFLAGS)
> > > > -$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_H) $(PROG_TESTS_FILES)
> > > > +$(OUTPUT)/test_progs: test_progs.c $(PROG_TESTS_FILES) | $(PROG_TESTS_H)
> > > > $(PROG_TESTS_H): $(PROG_TESTS_FILES) | $(PROG_TESTS_DIR)
> > > > $(shell ( cd prog_tests/; \
> > > > echo '/* Generated header, do not edit */'; \
> > > > @@ -256,7 +256,7 @@ MAP_TESTS_H := $(MAP_TESTS_DIR)/tests.h
> > > > MAP_TESTS_FILES := $(wildcard map_tests/*.c)
> > > > test_maps.c: $(MAP_TESTS_H)
> > > > $(OUTPUT)/test_maps: CFLAGS += $(TEST_MAPS_CFLAGS)
> > > > -$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_H) $(MAP_TESTS_FILES)
> > > > +$(OUTPUT)/test_maps: test_maps.c $(MAP_TESTS_FILES) | $(MAP_TESTS_H)
> > > > $(MAP_TESTS_H): $(MAP_TESTS_FILES) | $(MAP_TESTS_DIR)
> > > > $(shell ( cd map_tests/; \
> > > > echo '/* Generated header, do not edit */'; \
> > > > @@ -277,7 +277,7 @@ VERIFIER_TESTS_H := $(VERIFIER_TESTS_DIR)/tests.h
> > > > VERIFIER_TEST_FILES := $(wildcard verifier/*.c)
> > > > test_verifier.c: $(VERIFIER_TESTS_H)
> > > > $(OUTPUT)/test_verifier: CFLAGS += $(TEST_VERIFIER_CFLAGS)
> > > > -$(OUTPUT)/test_verifier: test_verifier.c $(VERIFIER_TESTS_H)
> > > > +$(OUTPUT)/test_verifier: test_verifier.c | $(VERIFIER_TEST_FILES) $(VERIFIER_TESTS_H)
> > > > $(VERIFIER_TESTS_H): $(VERIFIER_TEST_FILES) | $(VERIFIER_TESTS_DIR)
> > > > $(shell ( cd verifier/; \
> > > > echo '/* Generated header, do not edit */'; \
> > > > --
> > > > 2.17.1
> > > >
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: dsa: mt7530: Add support for port 5
From: Russell King - ARM Linux admin @ 2019-07-27 18:53 UTC (permalink / raw)
To: René van Dorst
Cc: netdev, frank-w, sean.wang, f.fainelli, davem, matthias.bgg,
andrew, vivien.didelot, john, linux-mediatek, linux-mips, robh+dt,
devicetree
In-Reply-To: <20190724192549.24615-4-opensource@vdorst.com>
On Wed, Jul 24, 2019 at 09:25:49PM +0200, René van Dorst wrote:
> Adding support for port 5.
>
> Port 5 can muxed/interface to:
> - internal 5th GMAC of the switch; can be used as 2nd CPU port or as
> extra port with an external phy for a 6th ethernet port.
> - internal PHY of port 0 or 4; Used in most applications so that port 0
> or 4 is the WAN port and interfaces with the 2nd GMAC of the SOC.
...
> @@ -1381,15 +1506,19 @@ static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> phylink_set_port_modes(mask);
> phylink_set(mask, Autoneg);
>
> - if (state->interface != PHY_INTERFACE_MODE_TRGMII) {
> + if (state->interface == PHY_INTERFACE_MODE_TRGMII) {
> + phylink_set(mask, 1000baseT_Full);
> + } else {
> phylink_set(mask, 10baseT_Half);
> phylink_set(mask, 10baseT_Full);
> phylink_set(mask, 100baseT_Half);
> phylink_set(mask, 100baseT_Full);
> - phylink_set(mask, 1000baseT_Half);
> - }
>
> - phylink_set(mask, 1000baseT_Full);
> + if (state->interface != PHY_INTERFACE_MODE_MII) {
> + phylink_set(mask, 1000baseT_Half);
> + phylink_set(mask, 1000baseT_Full);
> + }
> + }
As port 5 could use an external PHY, and it supports gigabit speeds,
consider that the PHY may provide not only copper but also fiber
connectivity, so port 5 should probably also have 1000baseX modes
too, which would allow such a PHY to bridge the switch to fiber.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH bpf-next 4/9] libbpf: add libbpf_swap_print to get previous print func
From: Andrii Nakryiko @ 2019-07-27 18:49 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Stanislav Fomichev, Andrii Nakryiko, bpf, Networking,
Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190727003045.63s6qau6kcnpkgxq@ast-mbp.dhcp.thefacebook.com>
On Fri, Jul 26, 2019 at 5:30 PM Alexei Starovoitov
<alexei.starovoitov@gmail.com> wrote:
>
> On Fri, Jul 26, 2019 at 02:47:28PM -0700, Andrii Nakryiko wrote:
> > On Fri, Jul 26, 2019 at 2:28 PM Stanislav Fomichev <sdf@fomichev.me> wrote:
> > >
> > > On 07/26, Andrii Nakryiko wrote:
> > > > libbpf_swap_print allows to restore previously set print function.
> > > > This is useful when running many independent test with one default print
> > > > function, but overriding log verbosity for particular subset of tests.
> > > Can we change the return type of libbpf_set_print instead and return
> > > the old function from it? Will it break ABI?
> >
> > Yeah, thought about that, but I wasn't sure about ABI breakage. It
> > seems like it shouldn't, so I'll just change libbpf_set_print
> > signature instead.
>
> I think it's ok to change return value of libbpf_set_print() from void
> to useful pointer.
Some googling gave inconclusive results. StackOverflow answers claim
it is compatible ABI change ([0]), but I also found some guidelines
for Android that designate any return type change as incompatible
([1]). [2] wasn't very helpful about defining compatibility rules,
unfortunately. I'm going with [0], though, and changing return type.
[0] https://stackoverflow.com/questions/15626579/c-abi-is-changing-a-void-function-to-return-an-int-a-breaking-change
[1] https://source.android.com/devices/architecture/vndk/abi-stability
[2] https://www.cs.dartmouth.edu/~sergey/cs258/ABI/UlrichDrepper-How-To-Write-Shared-Libraries.pdf
> This function is not marked as __attribute__((__warn_unused_result__)),
> so there should be no abi issues.
>
> Please double check by compiler perf with different gcc-s as Arnaldo's setup does.
>
Compiled (make -C tools/perf) with GCC 4.8.5, GCC 7, and Clang 8. None
of them produced any warning, so I'm going forward with just return
type change.
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: dsa: mt7530: Convert to PHYLINK API
From: Russell King - ARM Linux admin @ 2019-07-27 18:48 UTC (permalink / raw)
To: René van Dorst
Cc: netdev, frank-w, sean.wang, f.fainelli, davem, matthias.bgg,
andrew, vivien.didelot, john, linux-mediatek, linux-mips, robh+dt,
devicetree
In-Reply-To: <20190724192549.24615-2-opensource@vdorst.com>
Hi,
Just a couple of minor points.
On Wed, Jul 24, 2019 at 09:25:47PM +0200, René van Dorst wrote:
> +static void mt7530_phylink_mac_config(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + const struct phylink_link_state *state)
> +{
> + struct mt7530_priv *priv = ds->priv;
> + u32 mcr_cur, mcr_new;
> +
> + switch (port) {
> + case 0: /* Internal phy */
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + if (state->interface != PHY_INTERFACE_MODE_GMII)
> + return;
> + break;
> + /* case 5: Port 5 is not supported! */
> + case 6: /* 1st cpu port */
> + if (state->interface != PHY_INTERFACE_MODE_RGMII &&
> + state->interface != PHY_INTERFACE_MODE_TRGMII)
> + return;
> +
> + if (priv->p6_interface == state->interface)
> + break;
> + /* Setup TX circuit incluing relevant PAD and driving */
> + mt7530_pad_clk_setup(ds, state->interface);
> +
> + if (priv->id == ID_MT7530) {
> + /* Setup RX circuit, relevant PAD and driving on the
> + * host which must be placed after the setup on the
> + * device side is all finished.
> + */
> + mt7623_pad_clk_setup(ds);
> + }
> + priv->p6_interface = state->interface;
> + break;
> + default:
> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
> + return;
> + }
if (phylink_autoneg_inband(mode)) {
dev_err(ds->dev, "%s: in-band negotiation unsupported\n",
__func__);
return;
}
or similar, since you don't support inband AN in this code path.
> +
> + mcr_cur = mt7530_read(priv, MT7530_PMCR_P(port));
> + mcr_new = mcr_cur;
> + mcr_new &= ~(PMCR_FORCE_SPEED_1000 | PMCR_FORCE_SPEED_100 |
> + PMCR_FORCE_FDX | PMCR_TX_FC_EN | PMCR_RX_FC_EN);
> + mcr_new |= PMCR_IFG_XMIT(1) | PMCR_MAC_MODE | PMCR_BACKOFF_EN |
> + PMCR_BACKPR_EN | PMCR_FORCE_MODE | PMCR_FORCE_LNK;
> +
> + switch (state->speed) {
> + case SPEED_1000:
> + mcr_new |= PMCR_FORCE_SPEED_1000;
> + break;
> + case SPEED_100:
> + mcr_new |= PMCR_FORCE_SPEED_100;
> + break;
> + }
> + if (state->duplex == DUPLEX_FULL) {
> + mcr_new |= PMCR_FORCE_FDX;
> + if (state->pause & MLO_PAUSE_TX)
> + mcr_new |= PMCR_TX_FC_EN;
> + if (state->pause & MLO_PAUSE_RX)
> + mcr_new |= PMCR_RX_FC_EN;
> + }
> +
> + if (mcr_new != mcr_cur)
> + mt7530_write(priv, MT7530_PMCR_P(port), mcr_new);
> +}
> +
> +static void mt7530_phylink_mac_link_down(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface)
> +{
> + struct mt7530_priv *priv = ds->priv;
> +
> + mt7530_port_set_status(priv, port, 0);
> +}
> +
> +static void mt7530_phylink_mac_link_up(struct dsa_switch *ds, int port,
> + unsigned int mode,
> + phy_interface_t interface,
> + struct phy_device *phydev)
> +{
> + struct mt7530_priv *priv = ds->priv;
> +
> + mt7530_port_set_status(priv, port, 1);
> +}
> +
> +static void mt7530_phylink_validate(struct dsa_switch *ds, int port,
> + unsigned long *supported,
> + struct phylink_link_state *state)
> +{
> + __ETHTOOL_DECLARE_LINK_MODE_MASK(mask) = { 0, };
> +
> + switch (port) {
> + case 0: /* Internal phy */
> + case 1:
> + case 2:
> + case 3:
> + case 4:
> + if (state->interface != PHY_INTERFACE_MODE_NA &&
> + state->interface != PHY_INTERFACE_MODE_GMII)
> + goto unsupported;
> + break;
> + /* case 5: Port 5 not supported! */
> + case 6: /* 1st cpu port */
> + if (state->interface != PHY_INTERFACE_MODE_NA &&
> + state->interface != PHY_INTERFACE_MODE_RGMII &&
> + state->interface != PHY_INTERFACE_MODE_TRGMII)
> + goto unsupported;
> + break;
> + default:
> + linkmode_zero(supported);
> + dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
You could reorder this as:
default:
dev_err(ds->dev, "%s: unsupported port: %i\n", __func__, port);
unsupported:
linkmode_zero(supported);
and save having the "unsupported" at the end of the function. Not sure
what DaveM would think of it though.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: dsa: mt7530: Add support for port 5
From: René van Dorst @ 2019-07-27 18:43 UTC (permalink / raw)
To: David Miller
Cc: netdev, frank-w, sean.wang, f.fainelli, linux, matthias.bgg,
andrew, vivien.didelot, john, linux-mediatek, linux-mips, robh+dt,
devicetree
In-Reply-To: <20190726.140420.688330328284393964.davem@davemloft.net>
Quoting David Miller <davem@davemloft.net>:
> From: René van Dorst <opensource@vdorst.com>
> Date: Wed, 24 Jul 2019 21:25:49 +0200
>
>> @@ -1167,6 +1236,10 @@ mt7530_setup(struct dsa_switch *ds)
>> u32 id, val;
>> struct device_node *dn;
>> struct mt7530_dummy_poll p;
>> + phy_interface_t interface;
>> + struct device_node *mac_np;
>> + struct device_node *phy_node;
>> + const __be32 *_id;
>
Hi David,
> Reverse christmas tree here please.
>
> Thank you.
OK, I shall change that.
I spin a new version.
Greats,
René
^ permalink raw reply
* Re: [PATCH net-next 3/3] net: dsa: mt7530: Add support for port 5
From: René van Dorst @ 2019-07-27 18:38 UTC (permalink / raw)
To: Florian Fainelli
Cc: netdev, frank-w, sean.wang, linux, davem, matthias.bgg, andrew,
vivien.didelot, john, linux-mediatek, linux-mips, robh+dt,
devicetree
In-Reply-To: <f4a9e219-cd03-1512-874d-925c43e3c44f@gmail.com>
Quoting Florian Fainelli <f.fainelli@gmail.com>:
> On 7/24/2019 9:25 PM, René van Dorst wrote:
>> Adding support for port 5.
>>
>> Port 5 can muxed/interface to:
>> - internal 5th GMAC of the switch; can be used as 2nd CPU port or as
>> extra port with an external phy for a 6th ethernet port.
>> - internal PHY of port 0 or 4; Used in most applications so that port 0
>> or 4 is the WAN port and interfaces with the 2nd GMAC of the SOC.
>>
>> Signed-off-by: René van Dorst <opensource@vdorst.com>
>
> [snip]
>
>> + /* Setup port 5 */
>> + priv->p5_intf_sel = P5_DISABLED;
>> + interface = PHY_INTERFACE_MODE_NA;
>> +
>> + if (!dsa_is_unused_port(ds, 5)) {
>> + priv->p5_intf_sel = P5_INTF_SEL_GMAC5;
>> + interface = of_get_phy_mode(ds->ports[5].dn);
>> + } else {
>> + /* Scan the ethernet nodes. Look for GMAC1, Lookup used phy */
>> + for_each_child_of_node(dn, mac_np) {
>> + if (!of_device_is_compatible(mac_np,
>> + "mediatek,eth-mac"))
>> + continue;
>> + _id = of_get_property(mac_np, "reg", NULL);
>> + if (be32_to_cpup(_id) != 1)
>> + continue;
>> +
>> + interface = of_get_phy_mode(mac_np);
>> + phy_node = of_parse_phandle(mac_np, "phy-handle", 0);
>> +
>> + if (phy_node->parent == priv->dev->of_node->parent) {
>> + _id = of_get_property(phy_node, "reg", NULL);
>> + id = be32_to_cpup(_id);
>> + if (id == 0)
>> + priv->p5_intf_sel = P5_INTF_SEL_PHY_P0;
>> + if (id == 4)
>> + priv->p5_intf_sel = P5_INTF_SEL_PHY_P4;
>
Hi Florian,
> Can you use of_mdio_parse_addr() here?
Yes that function be used.
Thanks for mention this function.
I see that I can refactor this scan routine a bit more.
Also I missing a of_node_put(phy_node) at the end.
> --
> Florian
Greats,
René
^ permalink raw reply
* Re: [PATCH bpf-next 02/10] libbpf: implement BPF CO-RE offset relocation algorithm
From: Andrii Nakryiko @ 2019-07-27 18:24 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, Andrii Nakryiko, bpf, Networking,
Daniel Borkmann, Yonghong Song, Kernel Team
In-Reply-To: <957fff81-d845-ebc9-0e80-dbb1f1736b40@fb.com>
On Sat, Jul 27, 2019 at 10:00 AM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 7/26/19 11:25 PM, Andrii Nakryiko wrote:
> >>> + } else if (class == BPF_ST && BPF_MODE(insn->code) == BPF_MEM) {
> >>> + if (insn->imm != orig_off)
> >>> + return -EINVAL;
> >>> + insn->imm = new_off;
> >>> + pr_debug("prog '%s': patched insn #%d (ST | MEM) imm %d -> %d\n",
> >>> + bpf_program__title(prog, false),
> >>> + insn_idx, orig_off, new_off);
> >> I'm pretty sure llvm was not capable of emitting BPF_ST insn.
> >> When did that change?
> > I just looked at possible instructions that could have 32-bit
> > immediate value. This is `*(rX) = offsetof(struct s, field)`, which I
> > though is conceivable. Do you think I should drop it?
>
> Just trying to point out that since it's not emitted by llvm
> this code is likely untested ?
> Or you've created a bpf asm test for this?
Yeah, it's untested right now. Let me try to come up with LLVM
assembly + relocation (not yet sure how/whether builtin works with
inline assembly), if that works out, I'll leave this, if not, I'll
drop BPF_ST|BPF_MEM part.
>
>
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox