* Re: [PATCH net-next 0/4] r8169: improve HW csum and TSO handling
From: David Miller @ 2019-07-27 21:25 UTC (permalink / raw)
To: hkallweit1; +Cc: nic_swsd, netdev
In-Reply-To: <d347af97-0b46-6c71-37ef-46ce2b46f4df@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Fri, 26 Jul 2019 21:47:30 +0200
> This series:
> - delegates more tasks from the driver to the core
> - enables HW csum and TSO per default
> - copies quirks for buggy chip versions from vendor driver
Series applied, thanks Heiner.
^ permalink raw reply
* Re: [PATCH net-next] ipv6: remove printk
From: David Miller @ 2019-07-27 21:24 UTC (permalink / raw)
To: jonathan.lemon; +Cc: netdev, kernel-team
In-Reply-To: <20190726191609.3601197-1-jonathan.lemon@gmail.com>
From: Jonathan Lemon <jonathan.lemon@gmail.com>
Date: Fri, 26 Jul 2019 12:16:09 -0700
> ipv6_find_hdr() prints a non-rate limited error message
> when it cannot find an ipv6 header at a specific offset.
> This could be used as a DoS, so just remove it.
>
> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next] r8169: align setting PME with vendor driver
From: David Miller @ 2019-07-27 21:23 UTC (permalink / raw)
To: hkallweit1; +Cc: nic_swsd, netdev
In-Reply-To: <dfc84691-5643-63be-6338-55fe56df18b9@gmail.com>
From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Fri, 26 Jul 2019 20:56:20 +0200
> Align setting PME with the vendor driver. PMEnable is writable on
> RTL8169 only, on later chip versions it's read-only. PME_SIGNAL is
> used on chip versions from RTL8168evl with the exception of the
> RTL8168f family.
>
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next v2] mlx4/en_netdev: allow offloading VXLAN over VLAN
From: David Miller @ 2019-07-27 21:22 UTC (permalink / raw)
To: dcaratti; +Cc: pabeni, marcelo.leitner, saeedm, tariqt, netdev
In-Reply-To: <2beb05557960e04aa588ecc09e9ee5e5a19fc651.1564164688.git.dcaratti@redhat.com>
From: Davide Caratti <dcaratti@redhat.com>
Date: Fri, 26 Jul 2019 20:18:12 +0200
> ConnectX-3 Pro can offload transmission of VLAN packets with VXLAN inside:
> enable tunnel offloads in dev->vlan_features, like it's done with other
> NIC drivers (e.g. be2net and ixgbe).
>
> It's no more necessary to change dev->hw_enc_features when VXLAN are added
> or removed, since .ndo_features_check() already checks for VXLAN packet
> where the UDP destination port matches the configured value. Just set
> dev->hw_enc_features when the NIC is initialized, so that overlying VLAN
> can correctly inherit the tunnel offload capabilities.
>
> Changes since v1:
> - avoid flipping hw_enc_features, instead of calling netdev notifiers,
> thanks to Saeed Mahameed
> - squash two patches into a single one
>
> CC: Paolo Abeni <pabeni@redhat.com>
> CC: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> Signed-off-by: Davide Caratti <dcaratti@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH] drivers: net: xgene: Move status variable declaration into CONFIG_ACPI block
From: David Miller @ 2019-07-27 21:18 UTC (permalink / raw)
To: natechancellor
Cc: iyappan, keyur, quan, netdev, linux-kernel, skunberg.kelsey
In-Reply-To: <20190726162037.37308-1-natechancellor@gmail.com>
From: Nathan Chancellor <natechancellor@gmail.com>
Date: Fri, 26 Jul 2019 09:20:37 -0700
> When CONFIG_ACPI is unset (arm allyesconfig), status is unused.
>
> drivers/net/ethernet/apm/xgene/xgene_enet_xgmac.c:383:14: warning:
> unused variable 'status' [-Wunused-variable]
> acpi_status status;
> ^
> drivers/net/ethernet/apm/xgene/xgene_enet_sgmac.c:440:14: warning:
> unused variable 'status' [-Wunused-variable]
> acpi_status status;
> ^
> drivers/net/ethernet/apm/xgene/xgene_enet_hw.c:697:14: warning: unused
> variable 'status' [-Wunused-variable]
> acpi_status status;
> ^
>
> Move the declaration into the CONFIG_ACPI block so that there are no
> compiler warnings.
>
> Fixes: 570d785ba46b ("drivers: net: xgene: Remove acpi_has_method() calls")
> Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next v2 1/2] qed: Add API for configuring NVM attributes.
From: David Miller @ 2019-07-27 21:00 UTC (permalink / raw)
To: skalluru; +Cc: netdev, mkalderon, aelior
In-Reply-To: <20190726155215.25151-2-skalluru@marvell.com>
From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Date: Fri, 26 Jul 2019 08:52:14 -0700
> +int qed_mcp_nvm_set_cfg(struct qed_hwfn *p_hwfn, struct qed_ptt *p_ptt,
> + u16 option_id, u8 entity_id, u16 flags, u8 *p_buf,
> + u32 len)
> +{
> + u32 mb_param = 0, resp, param;
> + int rc;
...
> + rc = qed_mcp_nvm_wr_cmd(p_hwfn, p_ptt,
> + DRV_MSG_CODE_SET_NVM_CFG_OPTION,
> + mb_param, &resp, ¶m, len, (u32 *)p_buf);
> +
> + return rc;
'rc' is completely unnecessary, please just return the function result
directly.
Thank you.
^ permalink raw reply
* Re: [PATCH] net: rds: Fix possible null-pointer dereferences in rds_rdma_cm_event_handler_cmn()
From: David Miller @ 2019-07-27 20:58 UTC (permalink / raw)
To: baijiaju1990
Cc: santosh.shilimkar, netdev, linux-rdma, rds-devel, linux-kernel
In-Reply-To: <20190726141705.9585-1-baijiaju1990@gmail.com>
From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Fri, 26 Jul 2019 22:17:05 +0800
> In rds_rdma_cm_event_handler_cmn(), there are some if statements to
> check whether conn is NULL, such as on lines 65, 96 and 112.
> But conn is not checked before being used on line 108:
> trans->cm_connect_complete(conn, event);
> and on lines 140-143:
> rdsdebug("DISCONNECT event - dropping connection "
> "%pI6c->%pI6c\n", &conn->c_laddr,
> &conn->c_faddr);
> rds_conn_drop(conn);
>
> Thus, possible null-pointer dereferences may occur.
>
> To fix these bugs, conn is checked before being used.
>
> These bugs are found by a static analysis tool STCheck written by us.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 2/2] net: stmmac: Do not request stmmaceth clock
From: David Miller @ 2019-07-27 20:52 UTC (permalink / raw)
To: thierry.reding
Cc: joabreu, alexandre.torgue, peppe.cavallaro, jonathanh, netdev,
linux-tegra, linux-arm-kernel
In-Reply-To: <20190726102741.27872-2-thierry.reding@gmail.com>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Fri, 26 Jul 2019 12:27:41 +0200
> From: Thierry Reding <treding@nvidia.com>
>
> The stmmaceth clock is specified by the slave_bus and apb_pclk clocks in
> the device tree bindings for snps,dwc-qos-ethernet-4.10 compatible nodes
> of this IP.
>
> The subdrivers for these bindings will be requesting the stmmac clock
> correctly at a later point, so there is no need to request it here and
> cause an error message to be printed to the kernel log.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: stmmac: Make MDIO bus reset optional
From: David Miller @ 2019-07-27 20:52 UTC (permalink / raw)
To: thierry.reding
Cc: joabreu, alexandre.torgue, peppe.cavallaro, jonathanh, netdev,
linux-tegra, linux-arm-kernel
In-Reply-To: <20190726102741.27872-1-thierry.reding@gmail.com>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Fri, 26 Jul 2019 12:27:40 +0200
> From: Thierry Reding <treding@nvidia.com>
>
> The Tegra EQOS driver already resets the MDIO bus at probe time via the
> reset GPIO specified in the phy-reset-gpios device tree property. There
> is no need to reset the bus again later on.
>
> This avoids the need to query the device tree for the snps,reset GPIO,
> which is not part of the Tegra EQOS device tree bindings. This quiesces
> an error message from the generic bus reset code if it doesn't find the
> snps,reset related delays.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Applied.
^ permalink raw reply
* Re: [PATCH] net: neigh: remove redundant assignment to variable bucket
From: David Miller @ 2019-07-27 20:32 UTC (permalink / raw)
To: colin.king; +Cc: dsahern, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190726094611.3597-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Fri, 26 Jul 2019 10:46:11 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable bucket is being initialized with a value that is never
> read and it is being updated later with a new value in a following
> for-loop. The initialization is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH] isdn: mISDN: hfcsusb: Fix possible null-pointer dereferences in start_isoc_chain()
From: David Miller @ 2019-07-27 20:29 UTC (permalink / raw)
To: baijiaju1990
Cc: isdn, pakki001, tranmanphong, gregkh, rfontana, gustavo, tglx,
netdev, linux-kernel
In-Reply-To: <20190726082736.8195-1-baijiaju1990@gmail.com>
From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Fri, 26 Jul 2019 16:27:36 +0800
> In start_isoc_chain(), usb_alloc_urb() on line 1392 may fail
> and return NULL. At this time, fifo->iso[i].urb is assigned to NULL.
>
> Then, fifo->iso[i].urb is used at some places, such as:
> LINE 1405: fill_isoc_urb(fifo->iso[i].urb, ...)
> urb->number_of_packets = num_packets;
> urb->transfer_flags = URB_ISO_ASAP;
> urb->actual_length = 0;
> urb->interval = interval;
> LINE 1416: fifo->iso[i].urb->...
> LINE 1419: fifo->iso[i].urb->...
>
> Thus, possible null-pointer dereferences may occur.
>
> To fix these bugs, "continue" is added to avoid using fifo->iso[i].urb
> when it is NULL.
>
> These bugs are found by a static analysis tool STCheck written by us.
>
> Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH 2/2] net: ipv6: Fix a possible null-pointer dereference in vti6_link_config()
From: David Miller @ 2019-07-27 20:27 UTC (permalink / raw)
To: baijiaju1990; +Cc: kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20190726080321.4466-1-baijiaju1990@gmail.com>
From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Fri, 26 Jul 2019 16:03:21 +0800
> @@ -646,9 +646,10 @@ static void vti6_link_config(struct ip6_tnl *t, bool keep_mtu)
> &p->raddr, &p->laddr,
> p->link, NULL, strict);
>
> - if (rt)
> + if (rt) {
> tdev = rt->dst.dev;
> - ip6_rt_put(rt);
> + ip6_rt_put(rt);
> + }
ip6_rt_put() can take a NULL argument without any problem.
I want to make it clear that because of mistakes of this nature, and
how frequently you make them, it is very tiring and exhausting to
review your changes...
^ permalink raw reply
* Re: [PATCH 1/2] net: ipv4: Fix a possible null-pointer dereference in inet_csk_rebuild_route()
From: David Miller @ 2019-07-27 20:25 UTC (permalink / raw)
To: baijiaju1990; +Cc: kuznet, yoshfuji, netdev, linux-kernel
In-Reply-To: <20190726022534.24994-1-baijiaju1990@gmail.com>
From: Jia-Ju Bai <baijiaju1990@gmail.com>
Date: Fri, 26 Jul 2019 10:25:34 +0800
> diff --git a/net/ipv4/inet_connection_sock.c b/net/ipv4/inet_connection_sock.c
> index f5c163d4771b..27d9d80f3401 100644
> --- a/net/ipv4/inet_connection_sock.c
> +++ b/net/ipv4/inet_connection_sock.c
> @@ -1073,7 +1073,10 @@ static struct dst_entry *inet_csk_rebuild_route(struct sock *sk, struct flowi *f
> sk_setup_caps(sk, &rt->dst);
> rcu_read_unlock();
>
> - return &rt->dst;
> + if (rt)
> + return &rt->dst;
> + else
> + return NULL;
If rt is NULL, &rt->dst is also NULL as has been pointed out to you.
Please fix your automated tools to understand this case before submitting
more changes.
Thank you.
^ permalink raw reply
* 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
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