* Re: [PATCH net-next v5 0/3] hinic: add rss support and rss parameters configuration
From: David Miller @ 2019-06-19 1:52 UTC (permalink / raw)
To: xuechaojing
Cc: linux-kernel, netdev, luoshaokai, cloud.wangxiaoyun, chiqijun,
wulike1
In-Reply-To: <20190618062053.7545-1-xuechaojing@huawei.com>
From: Xue Chaojing <xuechaojing@huawei.com>
Date: Tue, 18 Jun 2019 06:20:50 +0000
> This series add rss support for HINIC driver and implement the ethtool
> interface related to rss parameter configuration. user can use ethtool
> configure rss parameters or show rss parameters.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net v2] hvsock: fix epollout hang from race condition
From: David Miller @ 2019-06-19 1:42 UTC (permalink / raw)
To: sunilmut
Cc: kys, haiyangz, sthemmin, sashal, decui, mikelley, netdev,
linux-hyperv, linux-kernel
In-Reply-To: <MW2PR2101MB111670139C7F5DC567129DD3C0EB0@MW2PR2101MB1116.namprd21.prod.outlook.com>
From: Sunil Muthuswamy <sunilmut@microsoft.com>
Date: Mon, 17 Jun 2019 19:26:25 +0000
> Currently, hvsock can enter into a state where epoll_wait on EPOLLOUT will
> not return even when the hvsock socket is writable, under some race
> condition. This can happen under the following sequence:
...
> Now, the EPOLLOUT will never return even if the socket write buffer is
> empty.
>
> The fix is to set the pending size to the default size and never change it.
> This way the host will always notify the guest whenever the writable space
> is bigger than the pending size. The host is already optimized to *only*
> notify the guest when the pending size threshold boundary is crossed and
> not everytime.
>
> This change also reduces the cpu usage somewhat since hv_stream_has_space()
> is in the hotpath of send:
> vsock_stream_sendmsg()->hv_stream_has_space()
> Earlier hv_stream_has_space was setting/clearing the pending size on every
> call.
>
> Signed-off-by: Sunil Muthuswamy <sunilmut@microsoft.com>
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
> ---
> - Resubmitting the patch after taking care of some spurious warnings.
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net v4] net/udp_gso: Allow TX timestamp with UDP GSO
From: David Miller @ 2019-06-19 1:39 UTC (permalink / raw)
To: fklassen; +Cc: netdev, linux-kernel, willemb
In-Reply-To: <20190617190507.12730-1-fklassen@appneta.com>
From: Fred Klassen <fklassen@appneta.com>
Date: Mon, 17 Jun 2019 12:05:07 -0700
> Fixes an issue where TX Timestamps are not arriving on the error queue
> when UDP_SEGMENT CMSG type is combined with CMSG type SO_TIMESTAMPING.
> This can be illustrated with an updated updgso_bench_tx program which
> includes the '-T' option to test for this condition. It also introduces
> the '-P' option which will call poll() before reading the error queue.
...
> The "poll timeout" message above indicates that TX timestamp never
> arrived.
>
> This patch preserves tx_flags for the first UDP GSO segment. Only the
> first segment is timestamped, even though in some cases there may be
> benefital in timestamping both the first and last segment.
>
> Factors in deciding on first segment timestamp only:
>
> - Timestamping both first and last segmented is not feasible. Hardware
> can only have one outstanding TS request at a time.
>
> - Timestamping last segment may under report network latency of the
> previous segments. Even though the doorbell is suppressed, the ring
> producer counter has been incremented.
>
> - Timestamping the first segment has the upside in that it reports
> timestamps from the application's view, e.g. RTT.
>
> - Timestamping the first segment has the downside that it may
> underreport tx host network latency. It appears that we have to pick
> one or the other. And possibly follow-up with a config flag to choose
> behavior.
...
> Fixes: ee80d1ebe5ba ("udp: add udp gso")
> Signed-off-by: Fred Klassen <fklassen@appneta.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH] net: stmmac: add sanity check to device_property_read_u32_array call
From: David Miller @ 2019-06-19 1:04 UTC (permalink / raw)
To: colin.king
Cc: peppe.cavallaro, alexandre.torgue, joabreu, mcoquelin.stm32,
netdev, linux-stm32, linux-arm-kernel, kernel-janitors,
linux-kernel
In-Reply-To: <20190617165836.4673-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Mon, 17 Jun 2019 17:58:36 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> Currently the call to device_property_read_u32_array is not error checked
> leading to potential garbage values in the delays array that are then used
> in msleep delays. Add a sanity check to the property fetching.
>
> Addresses-Coverity: ("Uninitialized scalar variable")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to net-next, thanks Colin.
Please make the destination tree explicit in the future by putting
something like "[PATCH net-next]" in the Subject line.
Thank you.
^ permalink raw reply
* Re: [PATCH][V2] net: lio_core: fix potential sign-extension overflow on large shift
From: David Miller @ 2019-06-19 1:01 UTC (permalink / raw)
To: colin.king
Cc: dchickles, sburla, fmanlunas, netdev, kernel-janitors,
linux-kernel
In-Reply-To: <20190617161249.28846-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Mon, 17 Jun 2019 17:12:49 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> Left shifting the signed int value 1 by 31 bits has undefined behaviour
> and the shift amount oq_no can be as much as 63. Fix this by using
> BIT_ULL(oq_no) instead.
>
> Addresses-Coverity: ("Bad shift operation")
> Fixes: f21fb3ed364b ("Add support of Cavium Liquidio ethernet adapters")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> ---
>
> V2: Use BIT_ULL(oq_no) instead of 1ULL << oq_no. Thanks to Dan Carpenter for
> noting this is more appropriate.
Applied.
^ permalink raw reply
* Re: [PATCH] net: hns3: Add missing newline at end of file
From: David Miller @ 2019-06-19 0:57 UTC (permalink / raw)
To: geert+renesas; +Cc: yisen.zhuang, salil.mehta, netdev, linux-kernel
In-Reply-To: <20190617143836.5154-1-geert+renesas@glider.be>
From: Geert Uytterhoeven <geert+renesas@glider.be>
Date: Mon, 17 Jun 2019 16:38:36 +0200
> "git diff" says:
>
> \ No newline at end of file
>
> after modifying the file.
>
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH net 0/3] net: fix quite a few dst_cache crashes reported by syzbot
From: David Miller @ 2019-06-19 0:49 UTC (permalink / raw)
To: lucien.xin
Cc: netdev, jon.maloy, ying.xue, tipc-discussion, marcelo.leitner,
nhorman, suyj.fnst, dsahern, syzkaller-bugs, dvyukov, pshelar
In-Reply-To: <cover.1560778340.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 17 Jun 2019 21:34:12 +0800
> There are two kinds of crashes reported many times by syzbot with no
> reproducer. Call Traces are like:
...
> They were caused by the fib_nh_common percpu member 'nhc_pcpu_rth_output'
> overwritten by another percpu variable 'dev->tstats' access overflow in
> tipc udp media xmit path when counting packets on a non tunnel device.
>
> The fix is to make udp tunnel work with no tunnel device by allowing not
> to count packets on the tstats when the tunnel dev is NULL in Patches 1/3
> and 2/3, then pass a NULL tunnel dev in tipc_udp_tunnel() in Patch 3/3.
Series applied and queued up for -stable.
Thanks for putting all of those syzbot reported by tags in there.
^ permalink raw reply
* Re: 4.19: udpgso_bench_tx: setsockopt zerocopy: Unknown error 524
From: Willem de Bruijn @ 2019-06-19 0:38 UTC (permalink / raw)
To: David Miller
Cc: Greg Kroah-Hartman, Naresh Kamboju, Network Development, LKML,
open list:KERNEL SELFTEST FRAMEWORK, Fred Klassen
In-Reply-To: <20190618.184409.2227845117139305004.davem@davemloft.net>
On Tue, Jun 18, 2019 at 6:44 PM David Miller <davem@davemloft.net> wrote:
>
> From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
> Date: Tue, 18 Jun 2019 14:58:26 -0400
>
> > I see that in similar such cases that use the test harness
> > (ksft_test_result_skip) the overall test returns success as long as
> > all individual cases return either success or skip.
> >
> > I think it's preferable to return KSFT_SKIP if any of the cases did so
> > (and none returned an error). I'll do that unless anyone objects.
>
> I guess this is a question of semantics.
>
> I mean, if you report skip at the top level does that mean that all
> sub tests were skipped? People may think so... :)
Yes, it's not ideal. Erring on the side of caution? Unlike pass, it is
a signal that an admin may or may not choose to act on. I run a
selected subset of tests from tools/testing that are all expected to
pass, so if one returns skip, I would want to take a closer look.
^ permalink raw reply
* Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000
From: Atish Patra @ 2019-06-18 23:40 UTC (permalink / raw)
To: anup@brainfault.org, paul.walmsley@sifive.com, palmer@sifive.com
Cc: bmeng.cn@gmail.com, davem@davemloft.net, Alistair Francis,
nicolas.ferre@microchip.com, aou@eecs.berkeley.edu,
yash.shah@sifive.com, linux-kernel@vger.kernel.org, ynezz@true.cz,
devicetree@vger.kernel.org, sachin.ghadi@sifive.com,
jamez@wit.com, mark.rutland@arm.com, robh+dt@kernel.org,
netdev@vger.kernel.org, schwab@suse.de,
lukas.auer@aisec.fraunhofer.de, troy.benjegerdes@sifive.com,
linux-riscv@lists.infradead.org
In-Reply-To: <CAAhSdy3zODw=JFaN=2F4K5-umihJDivLO8J8LBdkFkuZgzu41Q@mail.gmail.com>
On Tue, 2019-06-18 at 15:02 +0530, Anup Patel wrote:
> On Tue, Jun 18, 2019 at 8:56 AM Paul Walmsley <
> paul.walmsley@sifive.com> wrote:
> > On Mon, 17 Jun 2019, Alistair Francis wrote:
> >
> > > > The legacy M-mode U-boot handles the phy reset already, and
> > > > I’ve been
> > > > able to load upstream S-mode uboot as a payload via TFTP, and
> > > > then
> > > > load and boot a 4.19 kernel.
> > > >
> > > > It would be nice to get this all working with 5.x, however
> > > > there are
> > > > still
> > > > several missing pieces to really have it work well.
> > >
> > > Let me know what is still missing/doesn't work and I can add it.
> > > At the
> > > moment the only known issue I know of is a missing SD card driver
> > > in U-
> > > Boot.
> >
> > The DT data has changed between the non-upstream data that people
> > developed against previously, vs. the DT data that just went
> > upstream
> > here:
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852
> >
> > and
> >
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205
> >
> > So Upstream U-Boot is going to need several patches to get things
> > working
> > again. Clock identifiers and Ethernet are two known areas.
>
> Done.
>
> I just send-out few patches to fix U-Boot SiFive Clock driver.
>
> The U-Boot SiFive Clock driver fix series can be found in
> riscv_unleashed_clk_sync_v1 branch of:
> https://github.com/avpatel/u-boot.git
>
> Users will also require OpenSBI DTB fix which can be found
> in sifive_unleashed_dtb_fix_v1 branch of:
> https://github.com/avpatel/opensbi.git
>
Additionally, user can also use FW_PAYLOAD_FDT_PATH argument during
build to include the dtb built from kernel.
example:
make -j 32 PLATFORM=sifive/fu540 FW_PAYLOAD_PATH=<u-boot path>/u-
boot.bin FW_PAYLOAD_FDT_PATH=<kernel_dtb_path>
> With above fixes, we can now use same DTB for both U-Boot
> and Linux kernel (5.2-rc1). Although, users are free to pass a
> different DTB to Linux kernel via TFTP.
>
> I have tested SiFive serial and Cadance MACB ethernet on
> both U-Boot and Linux (5.2-rc1)
>
> Regards,
> Anup
--
Regards,
Atish
^ permalink raw reply
* Re: [PATCH v3 0/2] Add macb support for SiFive FU540-C000
From: Atish Patra @ 2019-06-18 23:27 UTC (permalink / raw)
To: linux-riscv@lists.infradead.org, linux-kernel@vger.kernel.org,
devicetree@vger.kernel.org, davem@davemloft.net,
yash.shah@sifive.com, netdev@vger.kernel.org
Cc: sachin.ghadi@sifive.com, nicolas.ferre@microchip.com,
mark.rutland@arm.com, paul.walmsley@sifive.com,
aou@eecs.berkeley.edu, ynezz@true.cz, palmer@sifive.com,
robh+dt@kernel.org
In-Reply-To: <1560844568-4746-1-git-send-email-yash.shah@sifive.com>
On Tue, 2019-06-18 at 13:26 +0530, Yash Shah wrote:
> On FU540, the management IP block is tightly coupled with the Cadence
> MACB IP block. It manages many of the boundary signals from the MACB
> IP
> This patchset controls the tx_clk input signal to the MACB IP. It
> switches between the local TX clock (125MHz) and PHY TX clocks. This
> is necessary to toggle between 1Gb and 100/10Mb speeds.
>
> Future patches may add support for monitoring or controlling other IP
> boundary signals.
>
> This patchset is mostly based on work done by
> Wesley Terpstra <wesley@sifive.com>
>
> This patchset is based on Linux v5.2-rc1 and tested on HiFive
> Unleashed
> board with additional board related patches needed for testing can be
> found at dev/yashs/ethernet_v3 branch of:
> https://github.com/yashshah7/riscv-linux.git
>
> Change History:
> V3:
> - Revert "MACB_SIFIVE_FU540" config changes in Kconfig and driver
> code.
> The driver does not depend on SiFive GPIO driver.
>
> V2:
> - Change compatible string from "cdns,fu540-macb" to "sifive,fu540-
> macb"
> - Add "MACB_SIFIVE_FU540" in Kconfig to support SiFive FU540 in macb
> driver. This is needed because on FU540, the macb driver depends on
> SiFive GPIO driver.
> - Avoid writing the result of a comparison to a register.
> - Fix the issue of probe fail on reloading the module reported by:
> Andreas Schwab <schwab@suse.de>
>
> Yash Shah (2):
> macb: bindings doc: add sifive fu540-c000 binding
> macb: Add support for SiFive FU540-C000
>
> Documentation/devicetree/bindings/net/macb.txt | 3 +
> drivers/net/ethernet/cadence/macb_main.c | 123
> +++++++++++++++++++++++++
> 2 files changed, 126 insertions(+)
>
Can you also post the ethernet dt entry[1] along with this patch ?
I think it would be good to all the dt related stuff reviewed together.
[1]
https://github.com/yashshah7/riscv-linux/commit/6d3af64bc5efed3915d658dc3bfe82dc1dbfafb3
I am able to get networking up in latest kernel + your patches in
OpenSBI + U-Boot(with updated compatible string) + Linux bootflow.
Tested-by: Atish Patra <atish.patra@wdc.com>
Regards,
Atish
^ permalink raw reply
* Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions
From: Nick Desaulniers @ 2019-06-18 23:23 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexander Duyck, netdev, LKML, Douglas Anderson,
Nathan Huckleberry, clang-built-linux, Nathan Chancellor
In-Reply-To: <20190618230420.GA84107@archlinux-epyc>
On Tue, Jun 18, 2019 at 4:04 PM Nathan Chancellor
<natechancellor@gmail.com> wrote:
>
> On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> > empty_child_inc/dec() use the ternary operator for conditional
> > operations. The conditions involve the post/pre in/decrement
> > operator and the operation is only performed when the condition
> > is *not* true. This is hard to parse for humans, use a regular
> > 'if' construct instead and perform the in/decrement separately.
> >
> > This also fixes two warnings that are emitted about the value
> > of the ternary expression being unused, when building the kernel
> > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > (https://lore.kernel.org/patchwork/patch/1089869/):
> >
> > CC net/ipv4/fib_trie.o
> > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> > ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> >
>
> As an FYI, this is also being fixed in clang:
>
> https://bugs.llvm.org/show_bug.cgi?id=42239
>
> https://reviews.llvm.org/D63369
While I'm glad we found and fixed a bug in Clang; I'm still for fixing
the underlying code from a readability perspective. If it takes
longer than a few seconds to understand a statement to the non-author,
the code as written may be too clever.
As a side note, I'm going to try to see if MAINTAINERS and
scripts/get_maintainers.pl supports regexes on the commit messages in
order to cc our mailing list (clang-built-linux
<clang-built-linux@googlegroups.com>) automatically; but in the
meantime please try to remember to cc the list manually. I usually
find it quickly from the bookmarked https://clangbuiltlinux.github.io/
which lists it there) (but I should still automate this).
>
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > I have no good understanding of the fib_trie code, but the
> > disentangled code looks wrong, and it should be equivalent to the
> > cryptic version, unless I messed it up. In empty_child_inc()
> > 'full_children' is only incremented when 'empty_children' is -1. I
> > suspect a bug in the cryptic code, but am surprised why it hasn't
> > blown up yet. Or is it intended behavior that is just
> > super-counterintuitive?
> >
> > For now I'm leaving it at disentangling the cryptic expressions,
> > if there is a bug we can discuss what action to take.
> > ---
> > net/ipv4/fib_trie.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> > index 868c74771fa9..cf7788e336b7 100644
> > --- a/net/ipv4/fib_trie.c
> > +++ b/net/ipv4/fib_trie.c
> > @@ -338,12 +338,18 @@ static struct tnode *tnode_alloc(int bits)
> >
> > static inline void empty_child_inc(struct key_vector *n)
> > {
> > - ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> > + tn_info(n)->empty_children++;
> > +
> > + if (!tn_info(n)->empty_children)
> > + tn_info(n)->full_children++;
> > }
> >
> > static inline void empty_child_dec(struct key_vector *n)
> > {
> > - tn_info(n)->empty_children-- ? : tn_info(n)->full_children--;
> > + if (!tn_info(n)->empty_children)
> > + tn_info(n)->full_children--;
> > +
> > + tn_info(n)->empty_children--;
This change looks correct to me, so thanks for the patch and:
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
(Bikeshed; I prefer pre-inc/dec to post-inc/dec unless you really care
about the previous value. Should be irrelevant with a sufficiently
smart compiler though. ;) )
--
Thanks,
~Nick Desaulniers
^ permalink raw reply
* Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions
From: Matthias Kaehlcke @ 2019-06-18 23:21 UTC (permalink / raw)
To: Nathan Chancellor
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI, netdev,
linux-kernel, Douglas Anderson, Nick Desaulniers,
Nathan Huckleberry, clang-built-linux
In-Reply-To: <20190618230420.GA84107@archlinux-epyc>
On Tue, Jun 18, 2019 at 04:04:20PM -0700, Nathan Chancellor wrote:
> On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> > empty_child_inc/dec() use the ternary operator for conditional
> > operations. The conditions involve the post/pre in/decrement
> > operator and the operation is only performed when the condition
> > is *not* true. This is hard to parse for humans, use a regular
> > 'if' construct instead and perform the in/decrement separately.
> >
> > This also fixes two warnings that are emitted about the value
> > of the ternary expression being unused, when building the kernel
> > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > (https://lore.kernel.org/patchwork/patch/1089869/):
> >
> > CC net/ipv4/fib_trie.o
> > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> > ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> >
>
> As an FYI, this is also being fixed in clang:
>
> https://bugs.llvm.org/show_bug.cgi?id=42239
>
> https://reviews.llvm.org/D63369
Great, thanks!
In this case it was actually useful to get the warning, even though it
didn't point out the actual bug. I think in general it would be
preferable to avoid such constructs, even when they are correct. But
then again, it's the reviewers/maintainers task to avoid unnecessarily
cryptic code from slipping in, and this just happens to be one instance
where the compiler could have helped.
^ permalink raw reply
* Re: [PATCH bpf-next] samples: bpf: Remove bpf_debug macro in favor of bpf_printk
From: Andrii Nakryiko @ 2019-06-18 23:14 UTC (permalink / raw)
To: Michal Rostecki
Cc: Doug Ledford, Jason Gunthorpe, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
linux-rdma, Networking, bpf, open list
In-Reply-To: <20190618181338.24145-1-mrostecki@opensuse.org>
On Tue, Jun 18, 2019 at 11:13 AM Michal Rostecki <mrostecki@opensuse.org> wrote:
>
> ibumad example was implementing the bpf_debug macro which is exactly the
> same as the bpf_printk macro available in bpf_helpers.h. This change
> makes use of bpf_printk instead of bpf_debug.
>
> Signed-off-by: Michal Rostecki <mrostecki@opensuse.org>
> ---
Acked-by: Andrii Nakryiko <andriin@fb.com>
> samples/bpf/ibumad_kern.c | 18 ++++++------------
> 1 file changed, 6 insertions(+), 12 deletions(-)
>
<snip>
^ permalink raw reply
* Re: [PATCH v2 0/2] Add macb support for SiFive FU540-C000
From: Alistair Francis @ 2019-06-18 23:11 UTC (permalink / raw)
To: paul.walmsley@sifive.com
Cc: davem@davemloft.net, palmer@sifive.com,
nicolas.ferre@microchip.com, aou@eecs.berkeley.edu,
yash.shah@sifive.com, linux-kernel@vger.kernel.org,
sachin.ghadi@sifive.com, devicetree@vger.kernel.org,
ynezz@true.cz, jamez@wit.com, mark.rutland@arm.com,
robh+dt@kernel.org, netdev@vger.kernel.org, schwab@suse.de,
troy.benjegerdes@sifive.com, linux-riscv@lists.infradead.org
In-Reply-To: <alpine.DEB.2.21.9999.1906172019040.15057@viisi.sifive.com>
On Mon, 2019-06-17 at 20:26 -0700, Paul Walmsley wrote:
> On Mon, 17 Jun 2019, Alistair Francis wrote:
>
> > > The legacy M-mode U-boot handles the phy reset already, and I’ve
> > > been
> > > able to load upstream S-mode uboot as a payload via TFTP, and
> > > then
> > > load and boot a 4.19 kernel.
> > >
> > > It would be nice to get this all working with 5.x, however there
> > > are
> > > still
> > > several missing pieces to really have it work well.
> >
> > Let me know what is still missing/doesn't work and I can add it. At
> > the
> > moment the only known issue I know of is a missing SD card driver
> > in U-
> > Boot.
>
> The DT data has changed between the non-upstream data that people
> developed against previously, vs. the DT data that just went
> upstream
> here:
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=72296bde4f4207566872ee355950a59cbc29f852
>
> and
>
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=c35f1b87fc595807ff15d2834d241f9771497205
>
> So Upstream U-Boot is going to need several patches to get things
> working
> again. Clock identifiers and Ethernet are two known areas.
Yep, Anup has sent patches to U-Boot and OpenSBI.
Alistair
>
>
> - Paul
^ permalink raw reply
* Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions
From: Nathan Chancellor @ 2019-06-18 23:04 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexander Duyck, netdev, linux-kernel, Douglas Anderson,
Nick Desaulniers, Nathan Huckleberry, clang-built-linux
In-Reply-To: <20190618211440.54179-1-mka@chromium.org>
On Tue, Jun 18, 2019 at 02:14:40PM -0700, Matthias Kaehlcke wrote:
> empty_child_inc/dec() use the ternary operator for conditional
> operations. The conditions involve the post/pre in/decrement
> operator and the operation is only performed when the condition
> is *not* true. This is hard to parse for humans, use a regular
> 'if' construct instead and perform the in/decrement separately.
>
> This also fixes two warnings that are emitted about the value
> of the ternary expression being unused, when building the kernel
> with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> (https://lore.kernel.org/patchwork/patch/1089869/):
>
> CC net/ipv4/fib_trie.o
> net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
>
As an FYI, this is also being fixed in clang:
https://bugs.llvm.org/show_bug.cgi?id=42239
https://reviews.llvm.org/D63369
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> I have no good understanding of the fib_trie code, but the
> disentangled code looks wrong, and it should be equivalent to the
> cryptic version, unless I messed it up. In empty_child_inc()
> 'full_children' is only incremented when 'empty_children' is -1. I
> suspect a bug in the cryptic code, but am surprised why it hasn't
> blown up yet. Or is it intended behavior that is just
> super-counterintuitive?
>
> For now I'm leaving it at disentangling the cryptic expressions,
> if there is a bug we can discuss what action to take.
> ---
> net/ipv4/fib_trie.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 868c74771fa9..cf7788e336b7 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -338,12 +338,18 @@ static struct tnode *tnode_alloc(int bits)
>
> static inline void empty_child_inc(struct key_vector *n)
> {
> - ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> + tn_info(n)->empty_children++;
> +
> + if (!tn_info(n)->empty_children)
> + tn_info(n)->full_children++;
> }
>
> static inline void empty_child_dec(struct key_vector *n)
> {
> - tn_info(n)->empty_children-- ? : tn_info(n)->full_children--;
> + if (!tn_info(n)->empty_children)
> + tn_info(n)->full_children--;
> +
> + tn_info(n)->empty_children--;
> }
>
> static struct key_vector *leaf_new(t_key key, struct fib_alias *fa)
> --
> 2.22.0.410.gd8fdbe21b5-goog
>
^ permalink raw reply
* Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions
From: Matthias Kaehlcke @ 2019-06-18 22:57 UTC (permalink / raw)
To: Doug Anderson
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexander Duyck, netdev, LKML, Greg Kroah-Hartman, Sasha Levin
In-Reply-To: <CAD=FV=V6TqT93Lb2UoQdkyO2j7OHrggCn-4qwDLEFw=N7RZ2Eg@mail.gmail.com>
On Tue, Jun 18, 2019 at 02:45:34PM -0700, Doug Anderson wrote:
> Hi,
>
> On Tue, Jun 18, 2019 at 2:14 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > empty_child_inc/dec() use the ternary operator for conditional
> > operations. The conditions involve the post/pre in/decrement
> > operator and the operation is only performed when the condition
> > is *not* true. This is hard to parse for humans, use a regular
> > 'if' construct instead and perform the in/decrement separately.
> >
> > This also fixes two warnings that are emitted about the value
> > of the ternary expression being unused, when building the kernel
> > with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> > (https://lore.kernel.org/patchwork/patch/1089869/):
> >
> > CC net/ipv4/fib_trie.o
> > net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> > ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > I have no good understanding of the fib_trie code, but the
> > disentangled code looks wrong, and it should be equivalent to the
> > cryptic version, unless I messed it up. In empty_child_inc()
> > 'full_children' is only incremented when 'empty_children' is -1. I
> > suspect a bug in the cryptic code, but am surprised why it hasn't
> > blown up yet. Or is it intended behavior that is just
> > super-counterintuitive?
> >
> > For now I'm leaving it at disentangling the cryptic expressions,
> > if there is a bug we can discuss what action to take.
> > ---
> > net/ipv4/fib_trie.c | 10 ++++++++--
> > 1 file changed, 8 insertions(+), 2 deletions(-)
>
> I have no knowledge of this code either but Matthias's patch looks
> sane to me and I agree with the disentangling before making functional
> changes.
I in terms of the -stable process it might make sense to either
disentangle & fix in a single step, or first fix the cryptic code
(shudder!) and then disentangle it. I guess if we make it a series
disentangle & fix could be separate steps.
I'm open to whatever maintainers & stable folks prefer.
> My own personal belief is that this is pointing out a bug somewhere.
> Since "empty_children" ends up being an unsigned type it doesn't feel
> like it was by-design that -1 is ever a value that should be in there.
good point that 'empty_children' is unsigned, that indeed reinforces
the bug theory.
> In any case:
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
Thanks!
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Richard Guy Briggs @ 2019-06-18 22:46 UTC (permalink / raw)
To: Paul Moore
Cc: Steve Grubb, Tycho Andersen, nhorman, linux-api, containers, LKML,
dhowells, Linux-Audit Mailing List, netfilter-devel, ebiederm,
simo, netdev, linux-fsdevel, Eric Paris, Serge E. Hallyn
In-Reply-To: <CAHC9VhQTPUnHt73SVn=iA_PW1mHkkDf=Nri1kgw_m5mcoiJV9A@mail.gmail.com>
On 2019-06-18 18:12, Paul Moore wrote:
> On Mon, Jun 3, 2019 at 4:24 PM Steve Grubb <sgrubb@redhat.com> wrote:
> > Hello Paul,
> >
> > I am curious about this. We seemed to be close to getting this patch pulled
> > in. A lot of people are waiting for it. Can you summarize what you think the
> > patches need and who we think needs to do it? I'm lost. Does LXC people need
> > to propose something? Does Richard? Someone else? Who's got the ball?
>
> [My apologies, this was lost in my inbox and I just not noticed it.]
>
> Please don't top post on things sent to the mailing lists Steve, you
> know better than that.
>
> Yes, things were moving along well, but upon talking with the LXC
> folks it appears we underestimated the importance of nested
> orchestrators. I suspect my reply to Dan on the 4th covered your
> questions, if you didn't see it, here is the relevant snippet:
>
> "To be clear, that's where we are at: we need to figure out what the
> kernel API would look like to support nested container orchestrators.
> My gut feeling is that this isn't going to be terribly complicated
> compared to the rest of the audit container ID work, but it is going
> to be some work. We had a discussion about some potential solutions
> in the cover letter and it sounds like Richard is working up some
> ideas now, let's wait to see what that looks like."
>
> ... and that is where we are at. I'm looking forward to seeing
> Richard's next patchset.
I've rebased everything and am trying out some code to see if it will
address the concerns raised... There will be more overhead on contid
write, and a tiny bit more for normal operations...
> > On Friday, May 31, 2019 8:44:45 AM EDT Paul Moore wrote:
> > > On Thu, May 30, 2019 at 8:21 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > > On 2019-05-30 19:26, Paul Moore wrote:
> > > > > On Thu, May 30, 2019 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > > On Thu, May 30, 2019 at 03:29:32PM -0400, Paul Moore wrote:
> > > > > > > [REMINDER: It is an "*audit* container ID" and not a general
> > > > > > > "container ID" ;) Smiley aside, I'm not kidding about that part.]
> > > > > >
> > > > > > This sort of seems like a distinction without a difference;
> > > > > > presumably
> > > > > > audit is going to want to differentiate between everything that
> > > > > > people
> > > > > > in userspace call a container. So you'll have to support all this
> > > > > > insanity anyway, even if it's "not a container ID".
> > > > >
> > > > > That's not quite right. Audit doesn't care about what a container is,
> > > > > or is not, it also doesn't care if the "audit container ID" actually
> > > > > matches the ID used by the container engine in userspace and I think
> > > > > that is a very important line to draw. Audit is simply given a value
> > > > > which it calls the "audit container ID", it ensures that the value is
> > > > > inherited appropriately (e.g. children inherit their parent's audit
> > > > > container ID), and it uses the value in audit records to provide some
> > > > > additional context for log analysis. The distinction isn't limited to
> > > > > the value itself, but also to how it is used; it is an "audit
> > > > > container ID" and not a "container ID" because this value is
> > > > > exclusively for use by the audit subsystem. We are very intentionally
> > > > > not adding a generic container ID to the kernel. If the kernel does
> > > > > ever grow a general purpose container ID we will be one of the first
> > > > > ones in line to make use of it, but we are not going to be the ones to
> > > > > generically add containers to the kernel. Enough people already hate
> > > > > audit ;)
> > > > >
> > > > > > > I'm not interested in supporting/merging something that isn't
> > > > > > > useful;
> > > > > > > if this doesn't work for your use case then we need to figure out
> > > > > > > what
> > > > > > > would work. It sounds like nested containers are much more common
> > > > > > > in
> > > > > > > the lxc world, can you elaborate a bit more on this?
> > > > > > >
> > > > > > > As far as the possible solutions you mention above, I'm not sure I
> > > > > > > like the per-userns audit container IDs, I'd much rather just emit
> > > > > > > the
> > > > > > > necessary tracking information via the audit record stream and let
> > > > > > > the
> > > > > > > log analysis tools figure it out. However, the bigger question is
> > > > > > > how
> > > > > > > to limit (re)setting the audit container ID when you are in a
> > > > > > > non-init
> > > > > > > userns. For reasons already mentioned, using capable() is a non
> > > > > > > starter for everything but the initial userns, and using
> > > > > > > ns_capable()
> > > > > > > is equally poor as it essentially allows any userns the ability to
> > > > > > > munge it's audit container ID (obviously not good). It appears we
> > > > > > > need a different method for controlling access to the audit
> > > > > > > container
> > > > > > > ID.
> > > > > >
> > > > > > One option would be to make it a string, and have it be append only.
> > > > > > That should be safe with no checks.
> > > > > >
> > > > > > I know there was a long thread about what type to make this thing. I
> > > > > > think you could accomplish the append-only-ness with a u64 if you had
> > > > > > some rule about only allowing setting lower order bits than those
> > > > > > that
> > > > > > are already set. With 4 bits for simplicity:
> > > > > >
> > > > > > 1100 # initial container id
> > > > > > 1100 -> 1011 # not allowed
> > > > > > 1100 -> 1101 # allowed, but now 1101 is set in stone since there are
> > > > > >
> > > > > > # no lower order bits left
> > > > > >
> > > > > > There are probably fancier ways to do it if you actually understand
> > > > > > math :)
> > > > >
> > > > > ;)
> > > > >
> > > > > > Since userns nesting is limited to 32 levels (right now, IIRC), and
> > > > > > you have 64 bits, this might be reasonable. You could just teach
> > > > > > container engines to use the first say N bits for themselves, with a
> > > > > > 1
> > > > > > bit for the barrier at the end.
> > > > >
> > > > > I like the creativity, but I worry that at some point these
> > > > > limitations are going to be raised (limits have a funny way of doing
> > > > > that over time) and we will be in trouble. I say "trouble" because I
> > > > > want to be able to quickly do an audit container ID comparison and
> > > > > we're going to pay a penalty for these larger values (we'll need this
> > > > > when we add multiple auditd support and the requisite record routing).
> > > > >
> > > > > Thinking about this makes me also realize we probably need to think a
> > > > > bit longer about audit container ID conflicts between orchestrators.
> > > > > Right now we just take the value that is given to us by the
> > > > > orchestrator, but if we want to allow multiple container orchestrators
> > > > > to work without some form of cooperation in userspace (I think we have
> > > > > to assume the orchestrators will not talk to each other) we likely
> > > > > need to have some way to block reuse of an audit container ID. We
> > > > > would either need to prevent the orchestrator from explicitly setting
> > > > > an audit container ID to a currently in use value, or instead generate
> > > > > the audit container ID in the kernel upon an event triggered by the
> > > > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > > > start looking at the idr code, I think we will need to make use of it.
> > > >
> > > > My first reaction to using the IDR code is that once an idr is given up,
> > > > it can be reused. I suppose we request IDRs and then never give them up
> > > > to avoid reuse...
> > >
> > > I'm not sure we ever what to guarantee that an audit container ID
> > > won't be reused during the lifetime of the system, it is a discrete
> > > integer after all. What I think we do want to guarantee is that we
> > > won't allow an unintentional audit container ID collision between
> > > different orchestrators; if a single orchestrator wants to reuse an
> > > audit container ID then that is its choice.
> > >
> > > > I already had some ideas of preventing an existing ID from being reused,
> > >
> > > Cool. I only made the idr suggestion since it is used for PIDs and
> > > solves a very similar problem.
> > >
> > > > but that makes the practice of some container engines injecting
> > > > processes into existing containers difficult if not impossible.
> > >
> > > Yes, we'll need some provision to indicate which orchestrator
> > > "controls" that particular audit container ID, and allow that
> > > orchestrator to reuse that particular audit container ID (until all
> > > those containers disappear and the audit container ID is given back to
> > > the pool).
>
> paul moore
- RGB
--
Richard Guy Briggs <rgb@redhat.com>
Sr. S/W Engineer, Kernel Security, Base Operating Systems
Remote, Ottawa, Red Hat Canada
IRC: rgb, SunRaycer
Voice: +1.647.777.2635, Internal: (81) 32635
^ permalink raw reply
* Re: 4.19: udpgso_bench_tx: setsockopt zerocopy: Unknown error 524
From: David Miller @ 2019-06-18 22:44 UTC (permalink / raw)
To: willemdebruijn.kernel
Cc: gregkh, naresh.kamboju, netdev, linux-kernel, linux-kselftest,
fklassen
In-Reply-To: <CA+FuTSdrphico4044QTD_-8VbanFFJx0FJuH+vVMfuHqbphkjw@mail.gmail.com>
From: Willem de Bruijn <willemdebruijn.kernel@gmail.com>
Date: Tue, 18 Jun 2019 14:58:26 -0400
> I see that in similar such cases that use the test harness
> (ksft_test_result_skip) the overall test returns success as long as
> all individual cases return either success or skip.
>
> I think it's preferable to return KSFT_SKIP if any of the cases did so
> (and none returned an error). I'll do that unless anyone objects.
I guess this is a question of semantics.
I mean, if you report skip at the top level does that mean that all
sub tests were skipped? People may think so... :)
^ permalink raw reply
* Re: [PATCH 2/2] net: fastopen: use endianness agnostic representation of the cookie
From: David Miller @ 2019-06-18 22:40 UTC (permalink / raw)
To: ard.biesheuvel
Cc: ebiggers, netdev, linux-crypto, herbert, edumazet, kuznet,
yoshfuji, jbaron, cpaasch, David.Laight, ycheng
In-Reply-To: <CAKv+Gu-xBCe09B1=GvXBH3V-p7=fOZcdL5pvFi5v19LT0J4KHA@mail.gmail.com>
From: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date: Tue, 18 Jun 2019 20:40:18 +0200
> Can we first agree on whether we care about this or not? If so, i
> can spin a v2.
Well, how can it possibly work otherwise in deployment scenerios involving
both big and little endian hosts?
^ permalink raw reply
* Re: [PATCH ghak90 V6 02/10] audit: add container id
From: Paul Moore @ 2019-06-18 22:12 UTC (permalink / raw)
To: Steve Grubb
Cc: Richard Guy Briggs, Tycho Andersen, Serge E. Hallyn, containers,
linux-api, Linux-Audit Mailing List, linux-fsdevel, LKML, netdev,
netfilter-devel, omosnace, dhowells, simo, Eric Paris, ebiederm,
nhorman
In-Reply-To: <97478582.yP93vGJyqj@x2>
On Mon, Jun 3, 2019 at 4:24 PM Steve Grubb <sgrubb@redhat.com> wrote:
> Hello Paul,
>
> I am curious about this. We seemed to be close to getting this patch pulled
> in. A lot of people are waiting for it. Can you summarize what you think the
> patches need and who we think needs to do it? I'm lost. Does LXC people need
> to propose something? Does Richard? Someone else? Who's got the ball?
[My apologies, this was lost in my inbox and I just not noticed it.]
Please don't top post on things sent to the mailing lists Steve, you
know better than that.
Yes, things were moving along well, but upon talking with the LXC
folks it appears we underestimated the importance of nested
orchestrators. I suspect my reply to Dan on the 4th covered your
questions, if you didn't see it, here is the relevant snippet:
"To be clear, that's where we are at: we need to figure out what the
kernel API would look like to support nested container orchestrators.
My gut feeling is that this isn't going to be terribly complicated
compared to the rest of the audit container ID work, but it is going
to be some work. We had a discussion about some potential solutions
in the cover letter and it sounds like Richard is working up some
ideas now, let's wait to see what that looks like."
... and that is where we are at. I'm looking forward to seeing
Richard's next patchset.
> On Friday, May 31, 2019 8:44:45 AM EDT Paul Moore wrote:
> > On Thu, May 30, 2019 at 8:21 PM Richard Guy Briggs <rgb@redhat.com> wrote:
> > > On 2019-05-30 19:26, Paul Moore wrote:
> > > > On Thu, May 30, 2019 at 5:29 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > On Thu, May 30, 2019 at 03:29:32PM -0400, Paul Moore wrote:
> > > > > > [REMINDER: It is an "*audit* container ID" and not a general
> > > > > > "container ID" ;) Smiley aside, I'm not kidding about that part.]
> > > > >
> > > > > This sort of seems like a distinction without a difference;
> > > > > presumably
> > > > > audit is going to want to differentiate between everything that
> > > > > people
> > > > > in userspace call a container. So you'll have to support all this
> > > > > insanity anyway, even if it's "not a container ID".
> > > >
> > > > That's not quite right. Audit doesn't care about what a container is,
> > > > or is not, it also doesn't care if the "audit container ID" actually
> > > > matches the ID used by the container engine in userspace and I think
> > > > that is a very important line to draw. Audit is simply given a value
> > > > which it calls the "audit container ID", it ensures that the value is
> > > > inherited appropriately (e.g. children inherit their parent's audit
> > > > container ID), and it uses the value in audit records to provide some
> > > > additional context for log analysis. The distinction isn't limited to
> > > > the value itself, but also to how it is used; it is an "audit
> > > > container ID" and not a "container ID" because this value is
> > > > exclusively for use by the audit subsystem. We are very intentionally
> > > > not adding a generic container ID to the kernel. If the kernel does
> > > > ever grow a general purpose container ID we will be one of the first
> > > > ones in line to make use of it, but we are not going to be the ones to
> > > > generically add containers to the kernel. Enough people already hate
> > > > audit ;)
> > > >
> > > > > > I'm not interested in supporting/merging something that isn't
> > > > > > useful;
> > > > > > if this doesn't work for your use case then we need to figure out
> > > > > > what
> > > > > > would work. It sounds like nested containers are much more common
> > > > > > in
> > > > > > the lxc world, can you elaborate a bit more on this?
> > > > > >
> > > > > > As far as the possible solutions you mention above, I'm not sure I
> > > > > > like the per-userns audit container IDs, I'd much rather just emit
> > > > > > the
> > > > > > necessary tracking information via the audit record stream and let
> > > > > > the
> > > > > > log analysis tools figure it out. However, the bigger question is
> > > > > > how
> > > > > > to limit (re)setting the audit container ID when you are in a
> > > > > > non-init
> > > > > > userns. For reasons already mentioned, using capable() is a non
> > > > > > starter for everything but the initial userns, and using
> > > > > > ns_capable()
> > > > > > is equally poor as it essentially allows any userns the ability to
> > > > > > munge it's audit container ID (obviously not good). It appears we
> > > > > > need a different method for controlling access to the audit
> > > > > > container
> > > > > > ID.
> > > > >
> > > > > One option would be to make it a string, and have it be append only.
> > > > > That should be safe with no checks.
> > > > >
> > > > > I know there was a long thread about what type to make this thing. I
> > > > > think you could accomplish the append-only-ness with a u64 if you had
> > > > > some rule about only allowing setting lower order bits than those
> > > > > that
> > > > > are already set. With 4 bits for simplicity:
> > > > >
> > > > > 1100 # initial container id
> > > > > 1100 -> 1011 # not allowed
> > > > > 1100 -> 1101 # allowed, but now 1101 is set in stone since there are
> > > > >
> > > > > # no lower order bits left
> > > > >
> > > > > There are probably fancier ways to do it if you actually understand
> > > > > math :)
> > > >
> > > > ;)
> > > >
> > > > > Since userns nesting is limited to 32 levels (right now, IIRC), and
> > > > > you have 64 bits, this might be reasonable. You could just teach
> > > > > container engines to use the first say N bits for themselves, with a
> > > > > 1
> > > > > bit for the barrier at the end.
> > > >
> > > > I like the creativity, but I worry that at some point these
> > > > limitations are going to be raised (limits have a funny way of doing
> > > > that over time) and we will be in trouble. I say "trouble" because I
> > > > want to be able to quickly do an audit container ID comparison and
> > > > we're going to pay a penalty for these larger values (we'll need this
> > > > when we add multiple auditd support and the requisite record routing).
> > > >
> > > > Thinking about this makes me also realize we probably need to think a
> > > > bit longer about audit container ID conflicts between orchestrators.
> > > > Right now we just take the value that is given to us by the
> > > > orchestrator, but if we want to allow multiple container orchestrators
> > > > to work without some form of cooperation in userspace (I think we have
> > > > to assume the orchestrators will not talk to each other) we likely
> > > > need to have some way to block reuse of an audit container ID. We
> > > > would either need to prevent the orchestrator from explicitly setting
> > > > an audit container ID to a currently in use value, or instead generate
> > > > the audit container ID in the kernel upon an event triggered by the
> > > > orchestrator (e.g. a write to a /proc file). I suspect we should
> > > > start looking at the idr code, I think we will need to make use of it.
> > >
> > > My first reaction to using the IDR code is that once an idr is given up,
> > > it can be reused. I suppose we request IDRs and then never give them up
> > > to avoid reuse...
> >
> > I'm not sure we ever what to guarantee that an audit container ID
> > won't be reused during the lifetime of the system, it is a discrete
> > integer after all. What I think we do want to guarantee is that we
> > won't allow an unintentional audit container ID collision between
> > different orchestrators; if a single orchestrator wants to reuse an
> > audit container ID then that is its choice.
> >
> > > I already had some ideas of preventing an existing ID from being reused,
> >
> > Cool. I only made the idr suggestion since it is used for PIDs and
> > solves a very similar problem.
> >
> > > but that makes the practice of some container engines injecting
> > > processes into existing containers difficult if not impossible.
> >
> > Yes, we'll need some provision to indicate which orchestrator
> > "controls" that particular audit container ID, and allow that
> > orchestrator to reuse that particular audit container ID (until all
> > those containers disappear and the audit container ID is given back to
> > the pool).
>
>
>
>
--
paul moore
www.paul-moore.com
^ permalink raw reply
* [PATCH v2] e1000e: Make watchdog use delayed work
From: Detlev Casanova @ 2019-06-18 22:08 UTC (permalink / raw)
To: Jeff Kirsher, David S. Miller, intel-wired-lan, netdev,
linux-kernel
Cc: Detlev Casanova
Use delayed work instead of timers to run the watchdog of the e1000e
driver.
Simplify the code with one less middle function.
Signed-off-by: Detlev Casanova <detlev.casanova@gmail.com>
---
drivers/net/ethernet/intel/e1000e/e1000.h | 3 +-
drivers/net/ethernet/intel/e1000e/netdev.c | 54 ++++++++++++----------
2 files changed, 30 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/intel/e1000e/e1000.h b/drivers/net/ethernet/intel/e1000e/e1000.h
index be13227f1697..942ab74030ca 100644
--- a/drivers/net/ethernet/intel/e1000e/e1000.h
+++ b/drivers/net/ethernet/intel/e1000e/e1000.h
@@ -186,12 +186,11 @@ struct e1000_phy_regs {
/* board specific private data structure */
struct e1000_adapter {
- struct timer_list watchdog_timer;
struct timer_list phy_info_timer;
struct timer_list blink_timer;
struct work_struct reset_task;
- struct work_struct watchdog_task;
+ struct delayed_work watchdog_task;
const struct e1000_info *ei;
diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 0e09bede42a2..22a5d594115f 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -39,6 +39,8 @@ static int debug = -1;
module_param(debug, int, 0);
MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
+static struct workqueue_struct *e1000_workqueue;
+
static const struct e1000_info *e1000_info_tbl[] = {
[board_82571] = &e1000_82571_info,
[board_82572] = &e1000_82572_info,
@@ -1780,7 +1782,8 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data)
}
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ queue_delayed_work(e1000_workqueue,
+ &adapter->watchdog_task, 1);
}
/* Reset on uncorrectable ECC error */
@@ -1860,7 +1863,8 @@ static irqreturn_t e1000_intr(int __always_unused irq, void *data)
}
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ queue_delayed_work(e1000_workqueue,
+ &adapter->watchdog_task, 1);
}
/* Reset on uncorrectable ECC error */
@@ -1905,7 +1909,8 @@ static irqreturn_t e1000_msix_other(int __always_unused irq, void *data)
hw->mac.get_link_status = true;
/* guard against interrupt when we're going down */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer, jiffies + 1);
+ queue_delayed_work(e1000_workqueue,
+ &adapter->watchdog_task, 1);
}
if (!test_bit(__E1000_DOWN, &adapter->state))
@@ -4278,7 +4283,6 @@ void e1000e_down(struct e1000_adapter *adapter, bool reset)
napi_synchronize(&adapter->napi);
- del_timer_sync(&adapter->watchdog_timer);
del_timer_sync(&adapter->phy_info_timer);
spin_lock(&adapter->stats64_lock);
@@ -5150,25 +5154,11 @@ static void e1000e_check_82574_phy_workaround(struct e1000_adapter *adapter)
}
}
-/**
- * e1000_watchdog - Timer Call-back
- * @data: pointer to adapter cast into an unsigned long
- **/
-static void e1000_watchdog(struct timer_list *t)
-{
- struct e1000_adapter *adapter = from_timer(adapter, t, watchdog_timer);
-
- /* Do the rest outside of interrupt context */
- schedule_work(&adapter->watchdog_task);
-
- /* TODO: make this use queue_delayed_work() */
-}
-
static void e1000_watchdog_task(struct work_struct *work)
{
struct e1000_adapter *adapter = container_of(work,
struct e1000_adapter,
- watchdog_task);
+ watchdog_task.work);
struct net_device *netdev = adapter->netdev;
struct e1000_mac_info *mac = &adapter->hw.mac;
struct e1000_phy_info *phy = &adapter->hw.phy;
@@ -5395,8 +5385,8 @@ static void e1000_watchdog_task(struct work_struct *work)
/* Reset the timer */
if (!test_bit(__E1000_DOWN, &adapter->state))
- mod_timer(&adapter->watchdog_timer,
- round_jiffies(jiffies + 2 * HZ));
+ queue_delayed_work(e1000_workqueue, &adapter->watchdog_task,
+ round_jiffies(2 * HZ));
}
#define E1000_TX_FLAGS_CSUM 0x00000001
@@ -7251,11 +7241,20 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
goto err_eeprom;
}
- timer_setup(&adapter->watchdog_timer, e1000_watchdog, 0);
+ e1000_workqueue = alloc_workqueue("%s", WQ_MEM_RECLAIM, 0,
+ e1000e_driver_name);
+
+ if (!e1000_workqueue) {
+ err = -ENOMEM;
+ goto err_workqueue;
+ }
+
+ INIT_DELAYED_WORK(&adapter->watchdog_task, e1000_watchdog_task);
+ queue_delayed_work(e1000_workqueue, &adapter->watchdog_task, 0);
+
timer_setup(&adapter->phy_info_timer, e1000_update_phy_info, 0);
INIT_WORK(&adapter->reset_task, e1000_reset_task);
- INIT_WORK(&adapter->watchdog_task, e1000_watchdog_task);
INIT_WORK(&adapter->downshift_task, e1000e_downshift_workaround);
INIT_WORK(&adapter->update_phy_task, e1000e_update_phy_task);
INIT_WORK(&adapter->print_hang_task, e1000_print_hw_hang);
@@ -7349,6 +7348,9 @@ static int e1000_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
return 0;
err_register:
+ flush_workqueue(e1000_workqueue);
+ destroy_workqueue(e1000_workqueue);
+err_workqueue:
if (!(adapter->flags & FLAG_HAS_AMT))
e1000e_release_hw_control(adapter);
err_eeprom:
@@ -7395,15 +7397,17 @@ static void e1000_remove(struct pci_dev *pdev)
*/
if (!down)
set_bit(__E1000_DOWN, &adapter->state);
- del_timer_sync(&adapter->watchdog_timer);
del_timer_sync(&adapter->phy_info_timer);
cancel_work_sync(&adapter->reset_task);
- cancel_work_sync(&adapter->watchdog_task);
cancel_work_sync(&adapter->downshift_task);
cancel_work_sync(&adapter->update_phy_task);
cancel_work_sync(&adapter->print_hang_task);
+ cancel_delayed_work(&adapter->watchdog_task);
+ flush_workqueue(e1000_workqueue);
+ destroy_workqueue(e1000_workqueue);
+
if (adapter->flags & FLAG_HAS_HW_TIMESTAMP) {
cancel_work_sync(&adapter->tx_hwtstamp_work);
if (adapter->tx_hwtstamp_skb) {
--
2.22.0
^ permalink raw reply related
* Re: [PATCH] net/ipv4: fib_trie: Avoid cryptic ternary expressions
From: Doug Anderson @ 2019-06-18 21:45 UTC (permalink / raw)
To: Matthias Kaehlcke
Cc: David S . Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Alexander Duyck, netdev, LKML
In-Reply-To: <20190618211440.54179-1-mka@chromium.org>
Hi,
On Tue, Jun 18, 2019 at 2:14 PM Matthias Kaehlcke <mka@chromium.org> wrote:
>
> empty_child_inc/dec() use the ternary operator for conditional
> operations. The conditions involve the post/pre in/decrement
> operator and the operation is only performed when the condition
> is *not* true. This is hard to parse for humans, use a regular
> 'if' construct instead and perform the in/decrement separately.
>
> This also fixes two warnings that are emitted about the value
> of the ternary expression being unused, when building the kernel
> with clang + "kbuild: Remove unnecessary -Wno-unused-value"
> (https://lore.kernel.org/patchwork/patch/1089869/):
>
> CC net/ipv4/fib_trie.o
> net/ipv4/fib_trie.c:351:2: error: expression result unused [-Werror,-Wunused-value]
> ++tn_info(n)->empty_children ? : ++tn_info(n)->full_children;
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> I have no good understanding of the fib_trie code, but the
> disentangled code looks wrong, and it should be equivalent to the
> cryptic version, unless I messed it up. In empty_child_inc()
> 'full_children' is only incremented when 'empty_children' is -1. I
> suspect a bug in the cryptic code, but am surprised why it hasn't
> blown up yet. Or is it intended behavior that is just
> super-counterintuitive?
>
> For now I'm leaving it at disentangling the cryptic expressions,
> if there is a bug we can discuss what action to take.
> ---
> net/ipv4/fib_trie.c | 10 ++++++++--
> 1 file changed, 8 insertions(+), 2 deletions(-)
I have no knowledge of this code either but Matthias's patch looks
sane to me and I agree with the disentangling before making functional
changes.
My own personal belief is that this is pointing out a bug somewhere.
Since "empty_children" ends up being an unsigned type it doesn't feel
like it was by-design that -1 is ever a value that should be in there.
In any case:
Reviewed-by: Douglas Anderson <dianders@chromium.org>
^ permalink raw reply
* Re: eBPF verifier slowness, more than 2 cpu seconds for about 600 instructions
From: Alexei Starovoitov @ 2019-06-18 21:40 UTC (permalink / raw)
To: Andreas Steinmetz, Daniel Borkmann, Andrii Nakryiko, Edward Cree
Cc: bpf, netdev
In-Reply-To: <CAADnVQ+wEdHKR2zR+E6vNQV_J8gfBmReYsLUQ2tegpX8ZFO=2A@mail.gmail.com>
On Mon, Jun 17, 2019 at 11:26:28AM -0700, Alexei Starovoitov wrote:
> On Sun, Jun 16, 2019 at 11:59 PM Alexei Starovoitov
> <alexei.starovoitov@gmail.com> wrote:
> >
> > On Thu, Jun 6, 2019 at 6:31 PM Andreas Steinmetz <ast@domdv.de> wrote:
> > >
> > > Below is the source in question. It may look a bit strange but I
> > > had to extract it from the project and preset parameters to fixed
> > > values.
> > > It takes from 2.8 to 4.5 seconds to load, depending on the processor.
> > > Just compile and run the code below.
> >
> > Thanks for the report.
> > It's interesting one indeed.
> > 600+ instructions consume
> > processed 280464 insns (limit 1000000) max_states_per_insn 15
> > total_states 87341 peak_states 580 mark_read 45
> >
> > The verifier finds a lot of different ways to go through branches
> > in the program and majority of the states are not equivalent and
> > do not help pruning, so it's doing full brute force walk of all possible
> > combinations.
> > We need to figure out whether there is a way to make it smarter.
>
> btw my pending backtracking logic helps it quite a bit:
> processed 164110 insns (limit 1000000) max_states_per_insn 11
> total_states 13398 peak_states 349 mark_read 10
>
> and it's 2x faster to verify, but 164k insns processed shows that
> there is still room for improvement.
Hi Andreas,
Could you please create selftests/bpf/verifier/.c out of it?
Currently we don't have a single test that exercises the verifier this way.
Could you also annotate instructions with comments like you did
at the top of your file?
The program logic is interesting.
If my understanding of assembler is correct it has unrolled
parsing of ipv6 extension headers. Then unrolled parsing of tcp options.
The way the program is using packet pointers forces the verifier to try
all possible combinations of extension headers and tcp options.
The precise backtracking logic helps to reduce amount of walking.
Also I think it's safe to reduce precision of variable part
of packet pointers. The following patch on top of bounded loop
series help to reduce it further.
Original:
processed 280464 insns (limit 1000000) max_states_per_insn 15
total_states 87341 peak_states 580 mark_read 45
Backtracking:
processed 164110 insns (limit 1000000) max_states_per_insn 11
total_states 13398 peak_states 349 mark_read 10
Backtracking + pkt_ptr var precision:
processed 96739 insns (limit 1000000) max_states_per_insn 11
total_states 7891 peak_states 329 mark_read 10
The patch helps w/o backtracking as well:
processed 165254 insns (limit 1000000) max_states_per_insn 15
total_states 51434 peak_states 572 mark_read 45
Backtracking and bounded loop heuristics reduce total memory
consumption quite a bit. Which was nice to see.
Anyway would be great if you could create a test out of it.
Would be even more awesome if you convert it to C code
and try to use bounded loops to parse extension headers
and tcp options. That would be a true test for both loops
and 'reduce precision' features.
Thanks!
From 4681224057af73335de0fdd629a2149bad91d59d Mon Sep 17 00:00:00 2001
From: Alexei Starovoitov <ast@kernel.org>
Date: Tue, 18 Jun 2019 13:40:29 -0700
Subject: [PATCH bpf-next] bpf: relax tracking of variable offset in packet pointers
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
kernel/bpf/verifier.c | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d2c8a6677ac4..e37c69ad57b3 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -3730,6 +3730,27 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
dst_reg->id = ++env->id_gen;
/* something was added to pkt_ptr, set range to zero */
dst_reg->raw = 0;
+ if (bpf_prog_is_dev_bound(env->prog->aux))
+ /* nfp offload needs accurate max_pkt_offset */
+ break;
+ if (env->strict_alignment)
+ break;
+ /* scalar added to pkt pointer is within BPF_MAX_VAR_OFF bounds.
+ * 64-bit pkt_data pointer can be safely compared with pkt_data_end
+ * even on 32-bit architectures.
+ * In case this scalar was positive the verifier
+ * doesn't need to track it precisely.
+ */
+ if (dst_reg->smin_value >= 0)
+ /* clear variable part of pkt pointer */
+ __mark_reg_known_zero(dst_reg);
+ /* no need to clear dst_reg->off.
+ * It's a known part of the pointer.
+ * When this pkt_ptr compared with pkt_end
+ * the 'range' will be initialized from 'off' and
+ * *(u8*)(dst_reg - off) is still more than packet start,
+ * since unknown value was positive.
+ */
}
break;
case BPF_SUB:
--
2.20.0
^ permalink raw reply related
* Re: [PATCH v2 bpf-next 00/11] BTF-defined BPF map definitions
From: Andrii Nakryiko @ 2019-06-18 21:37 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Andrii Nakryiko, Alexei Starovoitov, Networking, bpf, Kernel Team,
Jakub Kicinski, Joe Stringer
In-Reply-To: <30a2c470-5057-bd96-1889-e77fd5536960@iogearbox.net>
On Mon, Jun 17, 2019 at 2:17 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 06/17/2019 09:26 PM, Andrii Nakryiko wrote:
> > This patch set implements initial version (as discussed at LSF/MM2019
> > conference) of a new way to specify BPF maps, relying on BTF type information,
> > which allows for easy extensibility, preserving forward and backward
> > compatibility. See details and examples in description for patch #6.
Thanks for applying! Sorry for a bit of delayed in replying.
> >
> > [0] contains an outline of follow up extensions to be added after this basic
> > set of features lands. They are useful by itself, but also allows to bring
> > libbpf to feature-parity with iproute2 BPF loader. That should open a path
> > forward for BPF loaders unification.
> >
> > Patch #1 centralizes commonly used min/max macro in libbpf_internal.h.
> > Patch #2 extracts .BTF and .BTF.ext loading loging from elf_collect().
> > Patch #3 simplifies elf_collect() error-handling logic.
> > Patch #4 refactors map initialization logic into user-provided maps and global
> > data maps, in preparation to adding another way (BTF-defined maps).
> > Patch #5 adds support for map definitions in multiple ELF sections and
> > deprecates bpf_object__find_map_by_offset() API which doesn't appear to be
> > used anymore and makes assumption that all map definitions reside in single
> > ELF section.
> > Patch #6 splits BTF intialization from sanitization/loading into kernel to
> > preserve original BTF at the time of map initialization.
> > Patch #7 adds support for BTF-defined maps.
> > Patch #8 adds new test for BTF-defined map definition.
> > Patches #9-11 convert test BPF map definitions to use BTF way.
> >
> > [0] https://lore.kernel.org/bpf/CAEf4BzbfdG2ub7gCi0OYqBrUoChVHWsmOntWAkJt47=FE+km+A@mail.gmail.com/
>
> Quoting above in here for some clarifications on the approach. Basically for
> iproute2, we would add libbpf library support on top of the current loader,
> this means existing object files keep working as-is, and users would have to
> decide whether they want to go with the new format or stick to the old one;
> incentive for the new format would be to get all the other libbpf features
> from upstream automatically. Though it means that once they switch there is
> no object file compatibility with older iproute2 versions anymore. For the
> case of Cilium, the container image ships with its own iproute2 version as
> we don't want to rely on distros that they keep iproute2<->kernel release in
> sync (some really don't). Switch should be fine in our case. For people
> upgrading, the 'external' behavior (e.g. bpf fs interaction etc) would need
> to stay the same to not run into any service disruption when switching versions.
>
> >1. Pinning. This one is simple:
> > - add pinning attribute, that will either be "no pinning", "global
> >pinning", "object-scope pinning".
> > - by default pinning root will be "/sys/fs/bpf", but one will be
> >able to override this per-object using extra options (so that
> >"/sys/fs/bpf/tc" can be specified).
>
> I would just drop the object-scope pinning. We avoided using it and I'm not
> aware if anyone else make use. It also has the ugly side-effect that this
> relies on AF_ALG which e.g. on some cloud provider shipped kernels is disabled.
> The pinning attribute should be part of the standard set of map attributes for
> libbpf though as it's generally useful for networking applications.
Sounds good. I'll do some more surveying of use cases inside FB to see
if anyone needs object-scope pinning, just to be sure we are not
short-cutting anyone.
>
> >2. Map-in-map declaration:
> >
> >As outlined at LSF/MM, we can extend value type to be another map
> >definition, specifying a prototype for inner map:
> >
> >struct {
> > int type;
> > int max_entries;
> > struct outer_key *key;
> > struct { /* this is definition of inner map */
> > int type;
> > int max_entries;
> > struct inner_key *key;
> > struct inner_value *value;
> > } value;
> >} my_hash_of_arrays BPF_MAP = {
> > .type = BPF_MAP_TYPE_HASH_OF_MAPS,
> > .max_entries = 1024,
> > .value = {
> > .type = BPF_MAP_TYPE_ARRAY,
> > .max_entries = 64,
> > },
> >};
> >
> >This would declare a hash_of_maps, where inner maps are arrays of 64
> >elements each. Notice, that struct defining inner map can be declared
> >outside and shared with other maps:
> >
> >struct inner_map_t {
> > int type;
> > int max_entries;
> > struct inner_key *key;
> > struct inner_value *value;
> >};
> >
> >struct {
> > int type;
> > int max_entries;
> > struct outer_key *key;
> > struct inner_map_t value;
> >} my_hash_of_arrays BPF_MAP = {
> > .type = BPF_MAP_TYPE_HASH_OF_MAPS,
> > .max_entries = 1024,
> > .value = {
> > .type = BPF_MAP_TYPE_ARRAY,
> > .max_entries = 64,
> > },
> >};
>
> This one feels a bit weird to me. My expectation would have been something
> around the following to make this work:
>
> struct my_inner_map {
> int type;
> int max_entries;
> int *key;
> struct my_value *value;
> } btf_inner SEC(".maps") = {
> .type = BPF_MAP_TYPE_ARRAY,
> .max_entries = 16,
> };
>
> And:
>
> struct {
> int type;
> int max_entries;
> int *key;
> struct my_inner_map *value;
> } btf_outer SEC(".maps") = {
> .type = BPF_MAP_TYPE_ARRAY,
> .max_entries = 16,
> .value = &btf_inner,
> };
>
> And the loader should figure this out and combine everything in the background.
> Otherwise above 'struct inner_map_t value' would be mixing convention of using
> pointer vs non-pointer which may be even more confusing.
There are two reasons I didn't want to go with that approach:
1. This syntax makes my_inner_map usable as a stand-alone map, while
it's purpose is to serve as a inner map prototype. While technically
it is ok to use my_inner_map as real map, it's kind of confusing and
feels unclean.
2. This approach doesn't play well with case where we want to
pre-initialize array-of-maps with links to other maps. E.g., compare
w/ this:
struct {
int type;
int max_entries;
int *key;
struct my_inner_map *values[];
} btf_outer_initialized SEC(".maps") = {
.type = BPF_MAP_TYPE_ARRAY_OF_MAPS,
.max_entries = 16,
.values = {
&my_inner_map1,
&my_inner_map2,
},
};
In your case inner_map is a template, in this case my_inner_map1 and
my_inner_map2 is assigned to slots 0 and 1, respectively. But they
look deceivingly similar.
But in any case, we got to discussing details of map-in-map
initialization with Alexei, and concluded that for map-in-map cases
this split of key/value types being defined in struct definition and
flags/sizes/type being a compile-time assigned values becomes too much
of error-prone approach.
So we came up with a way to "encode" integer constants as part of BTF
type information, so that *all* declarative information is part of BTF
type, w/o the need to compile-time initialization. We tried to go the
other way (what Jakub was pushing for), but we couldn't figure out
anything that would work w/o more compiler hacks. So here's the
updated proposal:
#define __int(name, val) int (*name)[val]
#define __type(name, val) val (*foo)
struct my_value {
int a;
int b;
};
struct my_inner_map {
__int(type, BPF_MAP_TYPE_ARRAY);
__int(max_entries, 1000);
__type(key, int);
__type(value, struct my_value);
};
struct my_inner_map imap1 SEC(".maps");
struct my_inner_map imap2 SEC(".maps");
static struct {
__int(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
__int(max_entries, 1000);
__type(key, int);
__type(value, struct my_inner_map);
} my_outer_map SEC(".maps");
static struct {
__int(type, BPF_MAP_TYPE_ARRAY_OF_MAPS);
__int(max_entries, 1000);
__type(key, int);
__type(value, struct my_inner_map);
struct my_inner_map *values[];
} my_initialized_outer_map SEC(".maps") = {
.values = {
&imap1,
[500] = &imap2,
},
};
Here struct my_inner_map is complete definition of array map w/ 1000
elements w/ all the type info for k/v. That struct is used as a
template for my_outer_map map-in-map. my_initialized_outer_map is the
case of pre-initialization of array-of-maps w/ instances of existing
maps imap1 and imap2.
The idea is that we encode integer fields as array dimensions + use
pointer to an array to save space. Given that syntax in plain C is a
bit ugly and hard to remember, we hide that behind __int macro. Then
in line with __int, we also have __type macro, that hides that hateful
pointer for key/value types. This allows map definition to be
self-describing w/o having to look at initialized ELF data section at
all, except for special cases of explicitly initializing map-in-map or
prog_array.
What do you think?
>
> >3. Initialization of prog array. Iproute2 supports a convention-driven
> >initialization of BPF_MAP_TYPE_PROG_ARRAY using special section names
> >(wrapped into __section_tail(ID, IDX)):
> >
> >struct bpf_elf_map SEC("maps") POLICY_CALL_MAP = {
> > .type = BPF_MAP_TYPE_PROG_ARRAY,
> > .id = MAP_ID,
> > .size_key = sizeof(__u32),
> > .size_value = sizeof(__u32),
> > .max_elem = 16,
> >};
> >
> >__section_tail(MAP_ID, MAP_IDX) int handle_policy(struct __sk_buff *skb)
> >{
> > ...
> >}
> >
> >For each such program, iproute2 will put its FD (for later
> >tail-calling) into a corresponding MAP with id == MAP_ID at index
> >MAP_IDX.
> >
> >Here's how I see this supported in BTF-defined maps case.
> >
> >typedef int (* skbuff_tailcall_fn)(struct __sk_buff *);
> >
> >struct {
> > int type;
> > int max_entries;
> > int *key;
> > skbuff_tailcall_fb value[];
> >} POLICY_CALL_MAP SEC(".maps") = {
> > .type = BPF_MAP_TYPE_PROG_ARRAY,
> > .max_entries = 16,
> > .value = {
> > &handle_policy,
> > NULL,
> > &handle_some_other_policy,
> > },
> >};
> >
> >libbpf loader will greate BPF_MAP_TYPE_PROG_ARRAY map with 16 elements
> >and will initialize first and third entries with FDs of handle_policy
> >and handle_some_other_policy programs. As an added nice bonus,
> >compiler should also warn on signature mismatch. ;)
>
> Seems okay, I guess the explicit initialization could lead people to think
> that /after/ loading completed the NULL entries really won't have anything in
> that tail call slot. In our case, we share some of the tail call maps for different
> programs and each a different __section_tail(, idx), but in the end it's just a
> matter of initialization by index for the above. iproute2 today fetches the map
> from bpf fs if present, and only updates slots with __section_tail() present in
> the object file. Invocation would again be via index I presume (tail_call(skb,
> &policy_map, skb->mark), for example). For the __section_tail(MAP_ID, MAP_IDX),
> we do dynamically generate the MAP_IDX define in some cases, but that MAP_IDX
> would then simply be used in above POLICY_CALL_MAP instead; seems fine.
Yeah I can definitely see some confusion here. But it seems like this
is more of a semantics of map sharing, and maybe it should be some
extra option for when we have automatic support for extern (shared)
maps. E.g., something like
__int(sharing, SHARE_STRATEGY_MERGE) vs __int(sharing, SHARE_STRATEGY_OVERWRITE)
Haven't though through exact syntax, naming, semantics, but it seems
doable to support both, depending on desired behavior.
Maybe we should also unify this w/ pinning? E.g., there are many
sensible ways to handle already existing pinned map:
1. Reject program (e.g., if BPF application is the source of truth for that map)
2. Use pinned as is (e.g., if BPF application wants to consume data
from source of truth app)
3. Merge (what you described above)
4. Replace/reset - not sure if useful/desirable.
I'll need to study existing use cases a bit more though...
>
> >4. We can extend this idea into ARRAY_OF_MAPS initialization. This is
> >currently implemented in iproute2 using .id, .inner_id, and .inner_idx
> >fields.
> >
> >struct inner_map_t {
> > int type;
> > int max_entries;
> > struct inner_key *key;
> > struct inner_value *value;
> >};
> >
> >struct inner_map_t map1 = {...};
> >struct inner_map_t map2 = {...};
> >
> >struct {
> > int type;
> > int max_entries;
> > struct outer_key *key;
> > struct inner_map_t value[];
> >} my_hash_of_arrays BPF_MAP = {
> > .type = BPF_MAP_TYPE_ARRAY_OF_MAPS,
> > .max_entries = 2,
> > .value = {
> > &map1,
> > &map2,
> > },
> >};
>
> Yeah, agree.
See above w/ updated proposal. For exactly the same outcome.
>
> > v1->v2:
> > - more BTF-sanity checks in parsing map definitions (Song);
> > - removed confusing usage of "attribute", switched to "field;
> > - split off elf_collect() refactor from btf loading refactor (Song);
> > - split selftests conversion into 3 patches (Stanislav):
> > 1. test already relying on BTF;
> > 2. tests w/ custom types as key/value (so benefiting from BTF);
> > 3. all the rest tests (integers as key/value, special maps w/o BTF support).
> > - smaller code improvements (Song);
> >
> > rfc->v1:
> > - error out on unknown field by default (Stanislav, Jakub, Lorenz);
> >
> > Andrii Nakryiko (11):
> > libbpf: add common min/max macro to libbpf_internal.h
> > libbpf: extract BTF loading logic
> > libbpf: streamline ELF parsing error-handling
> > libbpf: refactor map initialization
> > libbpf: identify maps by section index in addition to offset
> > libbpf: split initialization and loading of BTF
> > libbpf: allow specifying map definitions using BTF
> > selftests/bpf: add test for BTF-defined maps
> > selftests/bpf: switch BPF_ANNOTATE_KV_PAIR tests to BTF-defined maps
> > selftests/bpf: convert tests w/ custom values to BTF-defined maps
> > selftests/bpf: convert remaining selftests to BTF-defined maps
> >
> > tools/lib/bpf/bpf.c | 7 +-
> > tools/lib/bpf/bpf_prog_linfo.c | 5 +-
> > tools/lib/bpf/btf.c | 3 -
> > tools/lib/bpf/btf.h | 1 +
> > tools/lib/bpf/btf_dump.c | 3 -
> > tools/lib/bpf/libbpf.c | 781 +++++++++++++-----
> > tools/lib/bpf/libbpf_internal.h | 7 +
> > tools/testing/selftests/bpf/progs/bpf_flow.c | 18 +-
> > .../selftests/bpf/progs/get_cgroup_id_kern.c | 18 +-
> > .../testing/selftests/bpf/progs/netcnt_prog.c | 22 +-
> > .../selftests/bpf/progs/sample_map_ret0.c | 18 +-
> > .../selftests/bpf/progs/socket_cookie_prog.c | 11 +-
> > .../bpf/progs/sockmap_verdict_prog.c | 36 +-
> > .../selftests/bpf/progs/test_btf_newkv.c | 73 ++
> > .../bpf/progs/test_get_stack_rawtp.c | 27 +-
> > .../selftests/bpf/progs/test_global_data.c | 27 +-
> > tools/testing/selftests/bpf/progs/test_l4lb.c | 45 +-
> > .../selftests/bpf/progs/test_l4lb_noinline.c | 45 +-
> > .../selftests/bpf/progs/test_map_in_map.c | 20 +-
> > .../selftests/bpf/progs/test_map_lock.c | 22 +-
> > .../testing/selftests/bpf/progs/test_obj_id.c | 9 +-
> > .../bpf/progs/test_select_reuseport_kern.c | 45 +-
> > .../bpf/progs/test_send_signal_kern.c | 22 +-
> > .../bpf/progs/test_skb_cgroup_id_kern.c | 9 +-
> > .../bpf/progs/test_sock_fields_kern.c | 60 +-
> > .../selftests/bpf/progs/test_spin_lock.c | 33 +-
> > .../bpf/progs/test_stacktrace_build_id.c | 44 +-
> > .../selftests/bpf/progs/test_stacktrace_map.c | 40 +-
> > .../testing/selftests/bpf/progs/test_tc_edt.c | 9 +-
> > .../bpf/progs/test_tcp_check_syncookie_kern.c | 9 +-
> > .../selftests/bpf/progs/test_tcp_estats.c | 9 +-
> > .../selftests/bpf/progs/test_tcpbpf_kern.c | 18 +-
> > .../selftests/bpf/progs/test_tcpnotify_kern.c | 18 +-
> > tools/testing/selftests/bpf/progs/test_xdp.c | 18 +-
> > .../selftests/bpf/progs/test_xdp_noinline.c | 60 +-
> > tools/testing/selftests/bpf/test_btf.c | 10 +-
> > .../selftests/bpf/test_queue_stack_map.h | 20 +-
> > .../testing/selftests/bpf/test_sockmap_kern.h | 72 +-
> > 38 files changed, 1199 insertions(+), 495 deletions(-)
> > create mode 100644 tools/testing/selftests/bpf/progs/test_btf_newkv.c
> >
>
^ permalink raw reply
* Re: [PATCH v2 1/6] net: macb: add phylink support
From: Andrew Lunn @ 2019-06-18 21:32 UTC (permalink / raw)
To: Parshuram Thombare
Cc: nicolas.ferre, davem, f.fainelli, netdev, hkallweit1,
linux-kernel, rafalc, aniljoy, piotrs, Russell King
In-Reply-To: <1560883265-6057-1-git-send-email-pthombar@cadence.com>
On Tue, Jun 18, 2019 at 07:41:05PM +0100, Parshuram Thombare wrote:
> This patch replace phylib API's by phylink API's.
Hi Parshuram
When you repost as a proper threaded patchset, please Cc: Russell
King, the phylink maintainer.
Thanks
Andrew
^ 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