* Re: [PATCH] brcmfmac: remove set but not used variable 'old_state'
From: Kalle Valo @ 2019-02-19 15:08 UTC (permalink / raw)
To: YueHaibing
Cc: Arend van Spriel, Franky Lin, Hante Meuleman, Chi-Hsien Lin,
Wright Feng, YueHaibing, linux-wireless, netdev,
brcm80211-dev-list.pdl, brcm80211-dev-list, kernel-janitors
In-Reply-To: <20190218080846.187942-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c: In function 'brcmf_usb_state_change':
> drivers/net/wireless/broadcom/brcm80211/brcmfmac/usb.c:578:6: warning:
> variable 'old_state' set but not used [-Wunused-but-set-variable]
>
> It's never used and can be removed.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> Acked-by: Arend van Spriel <arend.vanspriel@broadcom.com>
Patch applied to wireless-drivers-next.git, thanks.
e4d1b2716b88 brcmfmac: remove set but not used variable 'old_state'
--
https://patchwork.kernel.org/patch/10817429/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] arm64: do_csum: implement accelerated scalar version
From: Ilias Apalodimas @ 2019-02-19 15:08 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: linux-arm-kernel, will.deacon, steve.capper, netdev,
huanglingyan (A)
In-Reply-To: <20190218230842.11448-1-ard.biesheuvel@linaro.org>
On Tue, Feb 19, 2019 at 12:08:42AM +0100, Ard Biesheuvel wrote:
> It turns out that the IP checksumming code is still exercised often,
> even though one might expect that modern NICs with checksum offload
> have no use for it. However, as Lingyan points out, there are
> combinations of features where the network stack may still fall back
> to software checksumming, and so it makes sense to provide an
> optimized implementation in software as well.
>
> So provide an implementation of do_csum() in scalar assembler, which,
> unlike C, gives direct access to the carry flag, making the code run
> substantially faster. The routine uses overlapping 64 byte loads for
> all input size > 64 bytes, in order to reduce the number of branches
> and improve performance on cores with deep pipelines.
>
> On Cortex-A57, this implementation is on par with Lingyan's NEON
> implementation, and roughly 7x as fast as the generic C code.
>
> Cc: "huanglingyan (A)" <huanglingyan2@huawei.com>
> Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> ---
> Test code after the patch.
>
> arch/arm64/include/asm/checksum.h | 3 +
> arch/arm64/lib/Makefile | 2 +-
> arch/arm64/lib/csum.S | 127 ++++++++++++++++++++
> 3 files changed, 131 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/include/asm/checksum.h b/arch/arm64/include/asm/checksum.h
> index 0b6f5a7d4027..e906b956c1fc 100644
> --- a/arch/arm64/include/asm/checksum.h
> +++ b/arch/arm64/include/asm/checksum.h
> @@ -46,6 +46,9 @@ static inline __sum16 ip_fast_csum(const void *iph, unsigned int ihl)
> }
> #define ip_fast_csum ip_fast_csum
>
> +extern unsigned int do_csum(const unsigned char *buff, int len);
> +#define do_csum do_csum
> +
> #include <asm-generic/checksum.h>
>
> #endif /* __ASM_CHECKSUM_H */
> diff --git a/arch/arm64/lib/Makefile b/arch/arm64/lib/Makefile
> index 5540a1638baf..a7606007a749 100644
> --- a/arch/arm64/lib/Makefile
> +++ b/arch/arm64/lib/Makefile
> @@ -3,7 +3,7 @@ lib-y := clear_user.o delay.o copy_from_user.o \
> copy_to_user.o copy_in_user.o copy_page.o \
> clear_page.o memchr.o memcpy.o memmove.o memset.o \
> memcmp.o strcmp.o strncmp.o strlen.o strnlen.o \
> - strchr.o strrchr.o tishift.o
> + strchr.o strrchr.o tishift.o csum.o
>
> ifeq ($(CONFIG_KERNEL_MODE_NEON), y)
> obj-$(CONFIG_XOR_BLOCKS) += xor-neon.o
> diff --git a/arch/arm64/lib/csum.S b/arch/arm64/lib/csum.S
> new file mode 100644
> index 000000000000..534e2ebdc426
> --- /dev/null
> +++ b/arch/arm64/lib/csum.S
> @@ -0,0 +1,127 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +/*
> + * Copyright (C) 2019 Linaro, Ltd. <ard.biesheuvel@linaro.org>
> + */
> +
> +#include <linux/linkage.h>
> +#include <asm/assembler.h>
> +
> +ENTRY(do_csum)
> + adds x2, xzr, xzr // clear x2 and C flag
> +
> + // 64 bytes at a time
> + lsr x3, x1, #6
> + and x1, x1, #63
> + cbz x3, 1f
> +
> + // Eight 64-bit adds per iteration
> +0: ldp x4, x5, [x0], #64
> + ldp x6, x7, [x0, #-48]
> + ldp x8, x9, [x0, #-32]
> + ldp x10, x11, [x0, #-16]
> + adcs x2, x2, x4
> + sub x3, x3, #1
> + adcs x2, x2, x5
> + adcs x2, x2, x6
> + adcs x2, x2, x7
> + adcs x2, x2, x8
> + adcs x2, x2, x9
> + adcs x2, x2, x10
> + adcs x2, x2, x11
> + cbnz x3, 0b
> + adc x2, x2, xzr
> +
> + cbz x1, 7f
> + bic x3, x1, #1
> + add x12, x0, x1
> + add x0, x0, x3
> + neg x3, x3
> + add x3, x3, #64
> + lsl x3, x3, #3
> +
> + // Handle remaining 63 bytes or less using an overlapping 64-byte load
> + // and a branchless code path to complete the calculation
> + ldp x4, x5, [x0, #-64]
> + ldp x6, x7, [x0, #-48]
> + ldp x8, x9, [x0, #-32]
> + ldp x10, x11, [x0, #-16]
> + ldrb w12, [x12, #-1]
> +
> + .irp reg, x4, x5, x6, x7, x8, x9, x10, x11
> + cmp x3, #64
> + csel \reg, \reg, xzr, lt
> + ccmp x3, xzr, #0, lt
> + csel x13, x3, xzr, gt
> + sub x3, x3, #64
> +CPU_LE( lsr \reg, \reg, x13 )
> +CPU_BE( lsl \reg, \reg, x13 )
> + .endr
> +
> + adds x2, x2, x4
> + adcs x2, x2, x5
> + adcs x2, x2, x6
> + adcs x2, x2, x7
> + adcs x2, x2, x8
> + adcs x2, x2, x9
> + adcs x2, x2, x10
> + adcs x2, x2, x11
> + adc x2, x2, xzr
> +
> +CPU_LE( adds x12, x2, x12 )
> +CPU_BE( adds x12, x2, x12, lsl #8 )
> + adc x12, x12, xzr
> + tst x1, #1
> + csel x2, x2, x12, eq
> +
> +7: lsr x1, x2, #32
> + adds w2, w2, w1
> + adc w2, w2, wzr
> +
> + lsr w1, w2, #16
> + uxth w2, w2
> + add w2, w2, w1
> +
> + lsr w1, w2, #16 // handle the carry by hand
> + add w2, w2, w1
> +
> + uxth w0, w2
> + ret
> +
> + // Handle 63 bytes or less
> +1: tbz x1, #5, 2f
> + ldp x4, x5, [x0], #32
> + ldp x6, x7, [x0, #-16]
> + adds x2, x2, x4
> + adcs x2, x2, x5
> + adcs x2, x2, x6
> + adcs x2, x2, x7
> + adc x2, x2, xzr
> +
> +2: tbz x1, #4, 3f
> + ldp x4, x5, [x0], #16
> + adds x2, x2, x4
> + adcs x2, x2, x5
> + adc x2, x2, xzr
> +
> +3: tbz x1, #3, 4f
> + ldr x4, [x0], #8
> + adds x2, x2, x4
> + adc x2, x2, xzr
> +
> +4: tbz x1, #2, 5f
> + ldr w4, [x0], #4
> + adds x2, x2, x4
> + adc x2, x2, xzr
> +
> +5: tbz x1, #1, 6f
> + ldrh w4, [x0], #2
> + adds x2, x2, x4
> + adc x2, x2, xzr
> +
> +6: tbz x1, #0, 7b
> + ldrb w4, [x0]
> +CPU_LE( adds x2, x2, x4 )
> +CPU_BE( adds x2, x2, x4, lsl #8 )
> + adc x2, x2, xzr
> + b 7b
> +ENDPROC(do_csum)
> --
> 2.20.1
>
> diff --git a/lib/checksum.c b/lib/checksum.c
> index d3ec93f9e5f3..7711f1186f71 100644
> --- a/lib/checksum.c
> +++ b/lib/checksum.c
> @@ -37,7 +37,7 @@
>
> #include <asm/byteorder.h>
>
> -#ifndef do_csum
> +#if 1 //ndef do_csum
> static inline unsigned short from32to16(unsigned int x)
> {
> /* add up 16-bit and 16-bit for 16+c bit */
> @@ -47,7 +47,7 @@ static inline unsigned short from32to16(unsigned int x)
> return x;
> }
>
> -static unsigned int do_csum(const unsigned char *buff, int len)
> +static unsigned int __do_csum(const unsigned char *buff, int len)
> {
> int odd;
> unsigned int result = 0;
> @@ -206,3 +206,23 @@ __wsum csum_tcpudp_nofold(__be32 saddr, __be32 daddr,
> }
> EXPORT_SYMBOL(csum_tcpudp_nofold);
> #endif
> +
> +extern u8 crypto_ft_tab[];
> +
> +static int __init do_selftest(void)
> +{
> + int i, j;
> + u16 c1, c2;
> +
> + for (i = 0; i < 1024; i++) {
> + for (j = i + 1; j <= 1024; j++) {
> + c1 = __do_csum(crypto_ft_tab + i, j - i);
> + c2 = do_csum(crypto_ft_tab + i, j - i);
> +
> + if (c1 != c2)
> + pr_err("######### %d %d %x %x\n", i, j, c1, c2);
> + }
> + }
> + return 0;
> +}
> +late_initcall(do_selftest);
Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
^ permalink raw reply
* Re: [PATCH] rsi: remove set but not used variables 'info, vif'
From: Kalle Valo @ 2019-02-19 15:09 UTC (permalink / raw)
To: YueHaibing
Cc: Amitkumar Karwar, Siva Rebbagondla, YueHaibing, linux-wireless,
netdev, kernel-janitors
In-Reply-To: <20190218075156.183879-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/rsi/rsi_91x_main.c: In function 'rsi_prepare_skb':
> drivers/net/wireless/rsi/rsi_91x_main.c:127:24: warning:
> variable 'vif' set but not used [-Wunused-but-set-variable]
>
> drivers/net/wireless/rsi/rsi_91x_main.c:124:28: warning:
> variable 'info' set but not used [-Wunused-but-set-variable]
>
> They're not used any more since 160ee2a11ce0 ("rsi: fill rx_params only once.")
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Patch applied to wireless-drivers-next.git, thanks.
6f6e4f98ee52 rsi: remove set but not used variables 'info, vif'
--
https://patchwork.kernel.org/patch/10817423/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Paul Kocialkowski @ 2019-02-19 15:06 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, Heiner Kallweit, netdev, Thomas Petazzoni,
Mylène Josserand
In-Reply-To: <20190219133629.GN14879@lunn.ch>
Hi Andrew,
On Tue, 2019-02-19 at 14:36 +0100, Andrew Lunn wrote:
> On Tue, Feb 19, 2019 at 10:14:20AM +0100, Paul Kocialkowski wrote:
> > Hi,
> >
> > We are dealing with an Ethernet PHY (Marvell 88E1512) that comes with a
> > CONFIG pin that must be connected to one of the other pins of the PHY
> > to configure the LSB of the PHY address as well as I/O voltages (see
> > section 2.18.1 Hardware Configuration of the datasheet). It must be
> > connected "soon after reset" for the PHY to be correctly configured.
>
> Hi Paul
>
> I assume there are two PHYs on the MDIO bus, and you need to ensure
> they have different addresses? Do we have an Ethernet switch involved
> here, or are they two SoC Ethernet networks with one shared MDIO bus?
Thanks for your answer!
I think the reason why we need to deal with the CONFIG pin is more
about setting the correct I/O voltage than the PHY address (it just
happens that the CONFIG pin configures both at once).
With our setup, we only have a single PHY and no fancy eth switch setup
(although there is a GMII2RGMII converter that is controlled through
the MDIO bus, but there is no risk of address conflict).
> This seems like an odd design. I've normally seen weak pull up/down
> resistors, not a switch, so i'm wondering why it is designed like
> this.
Yes, that's definitely unusual and pretty specific to the PHY. I would
also have expected pull resistors but the way it's done is to connect
one pin to another at reset and disconnect them later on so that both
can be used for the intended function (PTP clock and LED).
I hope this clarifies our situation a bit.
Cheers,
Paul
--
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com
^ permalink raw reply
* Re: [PATCH] libertas_tf: remove set but not used variable 'flags'
From: Kalle Valo @ 2019-02-19 15:14 UTC (permalink / raw)
To: YueHaibing
Cc: Colin Ian King, YueHaibing, linux-wireless, kernel-janitors,
netdev
In-Reply-To: <20190213014917.164506-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/marvell/libertas_tf/main.c: In function 'lbtf_rx':
> drivers/net/wireless/marvell/libertas_tf/main.c:554:15: warning:
> variable 'flags' set but not used [-Wunused-but-set-variable]
>
> It never used and can be removed.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> Acked-by: Steve deRosier <derosier@cal-sierra.com>
Patch applied to wireless-drivers-next.git, thanks.
e97cb6ea71b0 libertas_tf: remove set but not used variable 'flags'
--
https://patchwork.kernel.org/patch/10808997/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH net] mwifiex: Fix NL80211_TX_POWER_LIMITED
From: Kalle Valo @ 2019-02-19 15:15 UTC (permalink / raw)
To: Adrian Bunk
Cc: netdev, Amitkumar Karwar, Nishant Sarmukadam, Ganapathi Bhat,
Xinming Hu, linux-wireless
In-Reply-To: <20190213135938.13664-1-bunk@kernel.org>
Adrian Bunk <bunk@kernel.org> wrote:
> NL80211_TX_POWER_LIMITED was treated as NL80211_TX_POWER_AUTOMATIC,
> which is the opposite of what should happen and can cause nasty
> regulatory problems.
>
> if/else converted to a switch without default to make gcc warn
> on unhandled enum values.
>
> Signed-off-by: Adrian Bunk <bunk@kernel.org>
Patch applied to wireless-drivers-next.git, thanks.
65a576e27309 mwifiex: Fix NL80211_TX_POWER_LIMITED
--
https://patchwork.kernel.org/patch/10810011/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v2 1/2] Provide in-kernel headers for making it easy to extend the kernel
From: Joel Fernandes @ 2019-02-19 15:16 UTC (permalink / raw)
To: Masahiro Yamada
Cc: Alexei Starovoitov, Networking, Linux Kernel Mailing List,
Andrew Morton, Alexei Starovoitov, atish patra, Daniel Colascione,
Dan Williams, Greg Kroah-Hartman, Jonathan Corbet, Karim Yaghmour,
Kees Cook, kernel-team, open list:DOCUMENTATION,
open list:KERNEL SELFTEST FRAMEWORK, Manoj Rao, Paul McKenney,
Peter Zijlstra (Intel), Randy Dunlap, Steven Rostedt, Shuah Khan,
Thomas Gleixner, Yonghong Song
In-Reply-To: <CAK7LNATf4adEEqWko2YO46wHHi-Vvc+_LBjQGNnM7rt8GSOgZQ@mail.gmail.com>
On Tue, Feb 19, 2019 at 01:42:13PM +0900, Masahiro Yamada wrote:
> On Tue, Feb 19, 2019 at 1:14 PM Masahiro Yamada
> <yamada.masahiro@socionext.com> wrote:
> >
> > On Fri, Feb 15, 2019 at 11:48 PM Alexei Starovoitov
> > <alexei.starovoitov@gmail.com> wrote:
> > >
> > > On Mon, Feb 11, 2019 at 09:35:59AM -0500, Joel Fernandes (Google) wrote:
> > > > Introduce in-kernel headers and other artifacts which are made available
> > > > as an archive through proc (/proc/kheaders.txz file).
> >
> >
> > The extension '.txz' is not used in kernel code.
> >
> >
> > '.tar.xz' is used for 'tarxz-pkg', 'perf-tarxz-src-pkg' etc.
> >
> >
> > $ git grep '\.txz'
> > $ git grep '\.tar\.xz'
> > Documentation/admin-guide/README.rst: xz -cd linux-4.X.tar.xz | tar xvf -
> > arch/x86/crypto/camellia-aesni-avx-asm_64.S: *
> > http://koti.mbnet.fi/axh/crypto/camellia-BSD-1.2.0-aesni1.tar.xz
> > crypto/testmgr.h: * https://bench.cr.yp.to/supercop/supercop-20170228.tar.xz
> > crypto/testmgr.h: * https://bench.cr.yp.to/supercop/supercop-20170228.tar.xz
> > crypto/testmgr.h: * https://bench.cr.yp.to/supercop/supercop-20170228.tar.xz
> > crypto/testmgr.h: * https://bench.cr.yp.to/supercop/supercop-20170228.tar.xz
> > crypto/testmgr.h: * https://bench.cr.yp.to/supercop/supercop-20170228.tar.xz
> > scripts/package/Makefile: @echo ' perf-tarxz-src-pkg - Build
> > $(perf-tar).tar.xz source tarball'
> > tools/testing/selftests/gen_kselftest_tar.sh:
> > ext=".tar.xz"
> >
> >
> >
> > I prefer '.tar.xz' for consistency.
> >
> >
> >
> >
> >
> >
> > BTW, have you ever looked at scripts/extract-ikconfig?
> >
> > You added IKHD_ST and IKHD_ED
> > just to mimic kernel/configs.c
> >
> > It is currently pointless without the extracting tool,
> > but you might think it is useful to extract headers
> > from vmlinux or the module without mounting procfs.
> >
> >
> >
> >
> > > > This archive makes
> > > > it possible to build kernel modules, run eBPF programs, and other
> > > > tracing programs that need to extend the kernel for tracing purposes
> > > > without any dependency on the file system having headers and build
> > > > artifacts.
> > > >
> > > > On Android and embedded systems, it is common to switch kernels but not
> > > > have kernel headers available on the file system. Raw kernel headers
> > > > also cannot be copied into the filesystem like they can be on other
> > > > distros, due to licensing and other issues. There's no linux-headers
> > > > package on Android. Further once a different kernel is booted, any
> > > > headers stored on the file system will no longer be useful. By storing
> > > > the headers as a compressed archive within the kernel, we can avoid these
> > > > issues that have been a hindrance for a long time.
> > >
> > > The set looks good to me and since the main use case is building bpf progs
> > > I can route it via bpf-next tree if there are no objections.
> > > Masahiro, could you please ack it?
> >
> >
> > Honestly, I was not tracking this thread
> > since I did not know I was responsible for this.
> >
> >
> > I just started to take a closer look, then immediately got scared.
> >
> > This version is not mature enough for the merge.
> >
> >
> >
> > First of all, this patch cannot be compiled out-of-tree (O= option).
> >
> >
> > I do not know why 0-day bot did not catch this apparent breakage.
> >
> >
> > $ make -j8 O=hoge
> > make[1]: Entering directory '/home/masahiro/workspace/bsp/linux/hoge'
> > GEN Makefile
> > Using .. as source for kernel
> > DESCEND objtool
> > CALL ../scripts/checksyscalls.sh
> > CHK include/generated/compile.h
> > make[2]: *** No rule to make target 'Module.symvers', needed by
> > 'kernel/kheaders_data.txz'. Stop.
> > make[2]: *** Waiting for unfinished jobs....
> > /home/masahiro/workspace/bsp/linux/Makefile:1043: recipe for target
> > 'kernel' failed
> > make[1]: *** [kernel] Error 2
> > make[1]: *** Waiting for unfinished jobs....
> > make[1]: Leaving directory '/home/masahiro/workspace/bsp/linux/hoge'
> > Makefile:152: recipe for target 'sub-make' failed
> > make: *** [sub-make] Error 2
>
>
>
>
> I saw this build error for in-tree building as well.
>
> We cannot build this from a pristine source tree.
>
> For example, I observed the build error in the following procedure.
>
> $ make mrproper
> $ make defconfig
> Set CONFIG_IKHEADERS_PROC=y
> $ make
>
>
>
>
> Module.symvers is generated in the modpost stage
> (the very last stage of build).
>
> But, vmlinux depends on kernel/kheaders_data.txz,
> which includes Module.symvers.
Firstly, I want to apologize for not testing this and other corner cases you
brought up. I should have known better. Since my build was working, I assumed
that the feature is working. For that, I am very sorry.
Secondly, it turns out Module.symvers circularly dependency problem also
exists with another use case.
If one does 'make modules_prepare' in a base kernel tree and then tries to
build modules with that tree, a warning like this is printed but the module
still gets built:
WARNING: Symbol version dump ./Module.symvers
is missing; modules will have no dependencies and modversions.
CC [M] /tmp/testmod/test.o
Building modules, stage 2.
MODPOST 1 modules
CC /tmp/testmod/test.mod.o
LD [M] /tmp/testmod/test.ko
So, I am thinking that at least for first pass I will just drop the inclusion
of Module.symvers in the archive and allow any modules built using
/proc/kheaders.tar.xz to not use it.
Kbuild will print a warning anyway when anyone tries to build using
/proc/kheaders.tar.xz, so if the user really wants module symbol versioning
then they should probably use a full kernel source tree with Module.symvers
available. For our usecase, kernel symbol versioning is a bit useless when
using /proc/kheaders.tar.gz because the proc file is generated with the same
kernel that the module is being built against, and subsequently loaded into
the kernel. So it is not likely that the CRC of a kernel symbol will be
different from what the module expects.
I can't think any other ways at the moment to break the circular dependency
so I'm thinking this is good enough for now especially since Kbuild will
print a proper warning. Let me know what you think?
thanks,
- Joel
^ permalink raw reply
* Re: [PATCH] orinoco : Replace function name in string with __func__
From: Kalle Valo @ 2019-02-19 15:17 UTC (permalink / raw)
To: Keyur Patel
In-Reply-To: <20190217162548.18257-1-iamkeyur96@gmail.com>
Keyur Patel <iamkeyur96@gmail.com> wrote:
> From: Keyur Patel <iamkeyur96@gmail.com>
>
> Replace hard coded function name with __func__, to
> improve robustness and to conform to the Linux kernel coding
> style. Issue found using checkpatch.
>
> Signed-off-by: Keyur Patel <iamkeyur96@gmail.com>
Patch applied to wireless-drivers-next.git, thanks.
25f87d8b63b8 orinoco : Replace function name in string with __func__
--
https://patchwork.kernel.org/patch/10817203/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] rtl818x_pci: Remove set but not used variables 'io_addr, mem_addr'
From: Kalle Valo @ 2019-02-19 15:18 UTC (permalink / raw)
To: YueHaibing
Cc: John W. Linville, Colin Ian King, YueHaibing, linux-wireless,
netdev, kernel-janitors
In-Reply-To: <20190218075226.183964-1-yuehaibing@huawei.com>
YueHaibing <yuehaibing@huawei.com> wrote:
> Fixes gcc '-Wunused-but-set-variable' warning:
>
> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c: In function 'rtl8180_probe':
> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c:1727:15: warning:
> variable 'io_addr' set but not used [-Wunused-but-set-variable]
>
> drivers/net/wireless/realtek/rtl818x/rtl8180/dev.c:1726:16: warning:
> variable 'mem_addr' set but not used [-Wunused-but-set-variable]
>
> They're never used since introduction.
>
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Patch applied to wireless-drivers-next.git, thanks.
b9b81d152cfb rtl818x_pci: Remove set but not used variables 'io_addr, mem_addr'
--
https://patchwork.kernel.org/patch/10817425/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH net-next v4 07/17] net: sched: protect filter_chain list with filter_chain_lock mutex
From: Vlad Buslov @ 2019-02-19 15:20 UTC (permalink / raw)
To: Cong Wang
Cc: Ido Schimmel, netdev@vger.kernel.org, jhs@mojatatu.com,
jiri@resnulli.us, davem@davemloft.net, ast@kernel.org,
daniel@iogearbox.net
In-Reply-To: <CAM_iQpU=sFqoSZUZaeRM7d4ZFMqiEaY2mn1G_gB5KvkzLdcQgw@mail.gmail.com>
On Tue 19 Feb 2019 at 05:08, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Fri, Feb 15, 2019 at 2:02 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> I looked at the code and problem seems to be matchall classifier
>> specific. My implementation of unlocked cls API assumes that concurrent
>> insertions are possible and checks for it when deleting "empty" tp.
>> Since classifiers don't expose number of elements, the only way to test
>> this is to do tp->walk() on them and assume that walk callback is called
>> once per filter on every classifier. In your example new tp is created
>> for second filter, filter insertion fails, number of elements on newly
>> created tp is checked with tp->walk() before deleting it. However,
>> matchall classifier always calls the tp->walk() callback once, even when
>> it doesn't have a valid filter (in this case with NULL filter pointer).
>
> Again, this can be eliminated by just switching to normal
> non-retry logic. This is yet another headache to review this
> kind of unlock-and-retry logic, I have no idea why you are such
> a big fan of it.
The retry approach was suggested to me multiple times by Jiri on
previous code reviews so I assumed it is preferred approach in such
cases. I don't have a strong preference in this regard, but locking
whole tp on filter update will remove any parallelism when updating same
classifier instance concurrently. The goal of these changes is to allow
parallel rule update and to achieve that I had to introduce some
complexity into the code.
Now let me explain why these two approaches result completely different
performance in this case. Lets start with a list of most CPU-consuming
parts in new filter creation process in descending order (raw data at
the end of this mail):
1) Hardware offload - if available and no skip_hw.
2) Exts (actions) initalization - most expensive part even with single
action, CPU usage increases with number of actions per filter.
3) cls API.
4) Flower classifier data structure initialization.
Note that 1)+2) is ~80% of cost of creating a flower filter. So if we
just lock the whole flower classifier instance during rule update we
serialize 1, 2 and 4, and only cls API (~13% of CPU cost) can be
executed concurrently. However, in proposed flower implementation hw
offloading and action initialization code is called without any locks
and tp->lock is only obtained when modifying flower data structures,
which means that only 3) is serialized and everything else (87% of CPU
cost) can be executed in parallel.
First page of profiling data:
Samples: 100K of event 'cycles:ppp', Event count (approx.): 11191878316
Children Self Command Shared Object Symbol
+ 84.71% 0.08% tc [kernel.vmlinux] [k] entry_SYSCALL_64_after_hwframe
+ 84.62% 0.06% tc [kernel.vmlinux] [k] do_syscall_64
+ 82.63% 0.01% tc libc-2.25.so [.] __libc_sendmsg
+ 82.37% 0.00% tc [kernel.vmlinux] [k] __sys_sendmsg
+ 82.37% 0.00% tc [kernel.vmlinux] [k] ___sys_sendmsg
+ 82.34% 0.00% tc [kernel.vmlinux] [k] sock_sendmsg
+ 82.34% 0.01% tc [kernel.vmlinux] [k] netlink_sendmsg
+ 82.15% 0.15% tc [kernel.vmlinux] [k] netlink_unicast
+ 82.10% 0.11% tc [kernel.vmlinux] [k] netlink_rcv_skb
+ 80.76% 0.22% tc [kernel.vmlinux] [k] rtnetlink_rcv_msg
+ 80.10% 0.24% tc [kernel.vmlinux] [k] tc_new_tfilter
+ 69.30% 2.11% tc [cls_flower] [k] fl_change
+ 33.56% 0.05% tc [kernel.vmlinux] [k] tcf_exts_validate
+ 33.50% 0.12% tc [kernel.vmlinux] [k] tcf_action_init
+ 33.30% 0.10% tc [kernel.vmlinux] [k] tcf_action_init_1
+ 32.78% 0.11% tc [act_gact] [k] tcf_gact_init
+ 30.93% 0.16% tc [kernel.vmlinux] [k] tc_setup_cb_call
+ 29.96% 0.60% tc [mlx5_core] [k] mlx5e_configure_flower
+ 27.62% 0.23% tc [mlx5_core] [k] mlx5e_tc_add_nic_flow
+ 27.31% 0.45% tc [kernel.vmlinux] [k] tcf_idr_create
+ 25.45% 1.75% tc [kernel.vmlinux] [k] pcpu_alloc
+ 16.33% 0.07% tc [mlx5_core] [k] mlx5_cmd_exec
+ 16.26% 1.96% tc [mlx5_core] [k] cmd_exec
+ 14.28% 1.05% tc [mlx5_core] [k] mlx5_add_flow_rules
+ 14.02% 0.26% tc [kernel.vmlinux] [k] pcpu_alloc_area
+ 13.09% 0.13% tc [mlx5_core] [k] mlx5_fc_create
+ 9.77% 0.30% tc [mlx5_core] [k] add_rule_fg.isra.28
+ 9.08% 0.84% tc [mlx5_core] [k] mlx5_cmd_set_fte
+ 8.90% 0.09% tc [mlx5_core] [k] mlx5_cmd_fc_alloc
+ 7.90% 0.12% tc [kernel.vmlinux] [k] tfilter_notify
+ 7.34% 0.61% tc [kernel.vmlinux] [k] __queue_work
+ 7.25% 0.26% tc [kernel.vmlinux] [k] tcf_fill_node
+ 6.73% 0.23% tc [kernel.vmlinux] [k] wait_for_completion_timeout
+ 6.67% 0.20% tc [cls_flower] [k] fl_dump
+ 6.52% 5.93% tc [kernel.vmlinux] [k] memset_erms
+ 5.77% 0.49% tc [kernel.vmlinux] [k] schedule_timeout
+ 5.57% 1.29% tc [kernel.vmlinux] [k] try_to_wake_up
+ 5.50% 0.11% tc [kernel.vmlinux] [k] pcpu_block_update_hint_alloc
+ 5.40% 0.85% tc [kernel.vmlinux] [k] pcpu_block_refresh_hint
+ 5.28% 0.11% tc [kernel.vmlinux] [k] queue_work_on
+ 5.19% 4.96% tc [kernel.vmlinux] [k] find_next_bit
+ 4.77% 0.11% tc [kernel.vmlinux] [k] idr_alloc_u32
+ 4.71% 0.10% tc [kernel.vmlinux] [k] schedule
+ 4.62% 0.30% tc [kernel.vmlinux] [k] __sched_text_start
+ 4.48% 4.41% tc [kernel.vmlinux] [k] idr_get_free
+ 4.19% 0.04% tc [kernel.vmlinux] [k] tcf_idr_check_alloc
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Andrew Lunn @ 2019-02-19 15:40 UTC (permalink / raw)
To: Paul Kocialkowski
Cc: Florian Fainelli, Heiner Kallweit, netdev, Thomas Petazzoni,
Mylène Josserand
In-Reply-To: <5a23b65bb9209cab5616ea06cbbb9c86dcaad1df.camel@bootlin.com>
> I think the reason why we need to deal with the CONFIG pin is more
> about setting the correct I/O voltage than the PHY address (it just
> happens that the CONFIG pin configures both at once).
Hi Paul
I don't have the datasheet...
What I/O voltages are we talking about? Is the device addressable over
the MDIO bus without this configuration? Can the voltages be
configured via register writes during probe? I assume not, or you
would be doing that...
Andrew
^ permalink raw reply
* Re: [Bridge] [RFC v2] net: bridge: don't flood known multicast traffic when snooping is enabled
From: Linus Lüssing @ 2019-02-19 15:42 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: bridge, netdev, roopa, f.fainelli, idosch
In-Reply-To: <7a95e759-12dd-7e7b-f2ce-f9e7f3d473f2@cumulusnetworks.com>
On Tue, Feb 19, 2019 at 03:31:42PM +0200, Nikolay Aleksandrov wrote:
> On 19/02/2019 11:21, Linus Lüssing wrote:
> > On Tue, Feb 19, 2019 at 09:57:16AM +0100, Linus Lüssing wrote:
> >> On Mon, Feb 18, 2019 at 02:21:07PM +0200, Nikolay Aleksandrov wrote:
> >>> This is v2 of the RFC patch which aims to forward packets to known
> >>> mdsts' ports only (the no querier case). After v1 I've kept
> >>> the previous behaviour when it comes to unregistered traffic or when
> >>> a querier is present. All of this is of course only with snooping
> >>> enabled. So with this patch the following changes should occur:
> >>> - No querier: forward known mdst traffic to its registered ports,
> >>> no change about unknown mcast (flood)
> >>> - Querier present: no change
> >>>
> >>> The reason to do this is simple - we want to respect the user's mdb
> >>> configuration in both cases, that is if the user adds static mdb entries
> >>> manually then we should use that information about forwarding traffic.
> >>>
> >>> What do you think ?
> >>>
> >>> * Notes
> >>> Traffic that is currently marked as mrouters_only:
> >>> - IPv4: non-local mcast traffic, igmp reports
> >>> - IPv6: non-all-nodes-dst mcast traffic, mldv1 reports
> >>>
> >>> Simple use case:
> >>> $ echo 1 > /sys/class/net/bridge/bridge/multicast_snooping
> >>> $ bridge mdb add dev bridge port swp1 grp 239.0.0.1
> >>> - without a querier currently traffic for 239.0.0.1 will still be flooded,
> >>> with this change it will be forwarded only to swp1
> >>
> >> There is still the issue with unsolicited reports adding mdst
> >> entries here, too. Leading to unwanted packet loss and connectivity issues.
> >
> > Or in other words, an unsolicited report will turn a previously
> > unregistered multicast group into a registered one. However in the
> > absence of a querier the knowledge about this newly registered multicast group
> > will be incomplete. And therefore still needs to be flooded to avoid packet
> > loss.
> >
>
> Right, this is expected. If the user has enabled igmp snooping and doesn't have
> a querier present then such behaviour is to be expected.
IGMP snooping is currently enabled by default. And IGMP/MLD
querier is disabled by default. I wouldn't want packet loss to be
the expected behaviour in a default setup.
This default setup currently works. However with this change it
will introduce packet loss, as you acknowledged (if I understood
you correctly?).
> What is surprising is
> the user explicitly enabling igmp snooping, adding an mdst and then still
> getting it flooded. :)
Hm, for me that does not seem surprising. I would not expect igmp
snooping to work without a querier on the link. Why don't you just
add/enable a querier in your setups then if you want to avoid
flooding?
> An alternative is to drop all unregistered traffic when a querier is not present.
> But that will surely break setups and at best should be a configurable option that
> is disabled by default.
Absolutely right. Always dropping with no querier is no option. That's why I'd say
you should always flood multicast packets if there is no querier.
> So in effect and to try and make everybody happy we can add an option to control
> this behaviour with keeping the current as default and adding the following options:
> - no querier: flood all (default, current)
ACK
For the other options maybe I do not understand your scenario yet.
Wouldn't these two options result in eratic behaviour?
> - no querier: flood unregistered, forward registered
> - no querier: drop unregistered, forward registered
Let's call these two cases A) - flood unregistered, forward
registered and B) - drop unregistered, forward registered.
Let's say you have a bridge with two ports:
(1)<-[br]->(2). And no querier.
Behind (2) there is a listener for group M. M is not
registered by the bridge because either that listener joined
before the bridge came up or the entry was registered once but had
timed out. Or there was packet loss and the report did not arrive
at the bridge (for instance bc. listener is behind a wireless
connection).
For case B) we can immediately see that the listener at (2) will
not receive the traffic it signed up for. And this is a permanent
issue as the listener will not send any further reports.
Case A) is ok, the listener behind port (2) receives its traffic.
Now, a listener for M joins at (1). It sends an unsolicited
report. Group M becomes registered by the bridge. Both for
cases (A) and (B) this new listener at (1) will receive its
traffic. However, not only in case B) now, but in case A), too,
the listener at (2) will rceive no more traffic for M.
Now 260 seconds pass (multicast_membership_interval). The entry
for M times out and is deleted on the bridge. It becomes
unregistered.
Now for case (A) things would be ok again, both listeners at (1)
and (2) would receive traffic. For now - as long as no new listener
joins again.
For case (B), both the listener at port (1) and (2) will fail to
receive the traffic they signed up for.
---
I hope this illustrates a bit what I'm afraid of? If you have any
measures to prevent such behavior in your setup, I'd be curious to
know.
Regards, Linus
^ permalink raw reply
* Re: [PATCH net-next 1/3] net: dsa: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 15:42 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Florian Fainelli, Andrew Lunn, David S. Miller, netdev
In-Reply-To: <20190218112355.gxroatrcaigo5nqh@shell.armlinux.org.uk>
Hi Russell,
On Mon, 18 Feb 2019 11:23:55 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> On Mon, Feb 18, 2019 at 12:50:31AM +0000, Russell King - ARM Linux admin wrote:
> > From what I can see, the port_vlan_add()/port_vlan_del() implementation
> > is far from ideal, just like "always enabling flooding for CPU/DSA
> > ports" is not ideal.
>
> There also seems to be a discrepency between what net/dsa wants to do
> and some of the implementations in drivers/net/dsa:
>
> dsa_switch_vlan_add() does this:
>
> bitmap_zero(ds->bitmap, ds->num_ports);
> if (ds->index == info->sw_index)
> set_bit(info->port, ds->bitmap);
> for (port = 0; port < ds->num_ports; port++)
> if (dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port))
> set_bit(port, ds->bitmap);
>
> We then call ds->ops->port_vlan_add() for each port in ds->bitmap,
> which will include DSA and CPU ports on every switch in the tree.
>
> For rtl8366, this calls into rtl8366_vlan_add(), which does:
>
> if (dsa_is_dsa_port(ds, port) || dsa_is_cpu_port(ds, port))
> dev_err(smi->dev, "port is DSA or CPU port\n");
>
> which is surely a guaranteed error message if we have any CPU or DSA
> ports specified on a rtl8366. The example in the DT documentation
> for this driver does suggest that it is capable of having CPU ports.
Correct, at the very beginning, most of the logic was implemented by the DSA
drivers themselves, like programming DSA and CPU ports. But this makes them
more complicated for no value, so the logic is slowly moved up to the core,
which now handles more of the generic logic.
Ideally I would like DSA drivers to be dumb, implementing simple switch-wide
ops reflecting their specs, e.g. switch-wide vlan_add ops taking a bitmap
of ports for a given VID.
This is a work in progress and we are slowly getting there. In the meantime
there are indeed discrepencies between DSA core and drivers as you pointed out.
Thanks,
Vivien
^ permalink raw reply
* [PATCH] sky2: Increase D3 delay again
From: Kai-Heng Feng @ 2019-02-19 15:45 UTC (permalink / raw)
To: mlindner, stephen, davem; +Cc: netdev, linux-kernel, Kai-Heng Feng, stable
Another platform requires even longer delay to make the device work
correctly after S3.
So increase the delay to 300ms.
BugLink: https://bugs.launchpad.net/bugs/1798921
Cc: stable@vger.kernel.org
Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
---
drivers/net/ethernet/marvell/sky2.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/marvell/sky2.c b/drivers/net/ethernet/marvell/sky2.c
index f3a5fa84860f..57727fe1501e 100644
--- a/drivers/net/ethernet/marvell/sky2.c
+++ b/drivers/net/ethernet/marvell/sky2.c
@@ -5073,7 +5073,7 @@ static int sky2_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
INIT_WORK(&hw->restart_work, sky2_restart);
pci_set_drvdata(pdev, hw);
- pdev->d3_delay = 200;
+ pdev->d3_delay = 300;
return 0;
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Jesper Dangaard Brouer @ 2019-02-19 15:57 UTC (permalink / raw)
To: Daniel Borkmann; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov, brouer
In-Reply-To: <11d2572e-3ff5-2fc0-8f05-c50dd0fb1d6d@iogearbox.net>
On Tue, 19 Feb 2019 12:46:57 +0100
Daniel Borkmann <daniel@iogearbox.net> wrote:
> > Due to lack of TC examples, lets show howto attach clsact BPF programs:
> >
> > # tc qdisc add dev ixgbe2 clsact
> > # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
> > # tc filter list dev ixgbe2 egress
Recommending the "replace" is wrong is seems, as does not replace the
existing, but keeps adding more filter entries.
What is the recommended procedure for unloading and loading a newer
version of the BPF TC program?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net] sk_msg: Keep reference on socket file while psock lives
From: Daniel Borkmann @ 2019-02-19 16:00 UTC (permalink / raw)
To: Jakub Sitnicki; +Cc: netdev, John Fastabend, Marek Majkowski
In-Reply-To: <20190211090949.18560-1-jakub@cloudflare.com>
Hi Jakub,
On 02/11/2019 10:09 AM, Jakub Sitnicki wrote:
> Backlog work for psock (sk_psock_backlog) might sleep while waiting for
> memory to free up when sending packets. While sleeping, socket can
> disappear from under our feet together with its wait queue because the
> userspace has closed it.
>
> This breaks an assumption in sk_stream_wait_memory, which expects the
> wait queue to be still there when it wakes up resulting in a
> use-after-free:
>
> ==================================================================
> BUG: KASAN: use-after-free in remove_wait_queue+0x31/0x70
> Write of size 8 at addr ffff888069a0c4e8 by task kworker/0:2/110
>
> CPU: 0 PID: 110 Comm: kworker/0:2 Not tainted 5.0.0-rc2-00335-g28f9d1a3d4fe-dirty #14
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-2.fc27 04/01/2014
> Workqueue: events sk_psock_backlog
> Call Trace:
> print_address_description+0x6e/0x2b0
> ? remove_wait_queue+0x31/0x70
> kasan_report+0xfd/0x177
> ? remove_wait_queue+0x31/0x70
> ? remove_wait_queue+0x31/0x70
> remove_wait_queue+0x31/0x70
> sk_stream_wait_memory+0x4dd/0x5f0
> ? sk_stream_wait_close+0x1b0/0x1b0
> ? wait_woken+0xc0/0xc0
> ? tcp_current_mss+0xc5/0x110
> tcp_sendmsg_locked+0x634/0x15d0
> ? tcp_set_state+0x2e0/0x2e0
> ? __kasan_slab_free+0x1d1/0x230
> ? kmem_cache_free+0x70/0x140
> ? sk_psock_backlog+0x40c/0x4b0
> ? process_one_work+0x40b/0x660
> ? worker_thread+0x82/0x680
> ? kthread+0x1b9/0x1e0
> ? ret_from_fork+0x1f/0x30
> ? check_preempt_curr+0xaf/0x130
> ? iov_iter_kvec+0x5f/0x70
> ? kernel_sendmsg_locked+0xa0/0xe0
> skb_send_sock_locked+0x273/0x3c0
> ? skb_splice_bits+0x180/0x180
> ? start_thread+0xe0/0xe0
> ? update_min_vruntime.constprop.27+0x88/0xc0
> sk_psock_backlog+0xb3/0x4b0
> ? strscpy+0xbf/0x1e0
> process_one_work+0x40b/0x660
> worker_thread+0x82/0x680
> ? process_one_work+0x660/0x660
> kthread+0x1b9/0x1e0
> ? __kthread_create_on_node+0x250/0x250
> ret_from_fork+0x1f/0x30
>
> Allocated by task 109:
> sock_alloc_inode+0x54/0x120
> alloc_inode+0x28/0xb0
> new_inode_pseudo+0x7/0x60
> sock_alloc+0x21/0xe0
> __sys_accept4+0xc2/0x330
> __x64_sys_accept+0x3b/0x50
> do_syscall_64+0xb2/0x3e0
> entry_SYSCALL_64_after_hwframe+0x44/0xa9
>
> Freed by task 7:
> kfree+0x7f/0x140
> rcu_process_callbacks+0xe0/0x100
> __do_softirq+0xe5/0x29a
>
> The buggy address belongs to the object at ffff888069a0c4e0
> which belongs to the cache kmalloc-64 of size 64
> The buggy address is located 8 bytes inside of
> 64-byte region [ffff888069a0c4e0, ffff888069a0c520)
> The buggy address belongs to the page:
> page:ffffea0001a68300 count:1 mapcount:0 mapping:ffff88806d4018c0 index:0x0
> flags: 0x4000000000000200(slab)
> raw: 4000000000000200 dead000000000100 dead000000000200 ffff88806d4018c0
> raw: 0000000000000000 00000000002a002a 00000001ffffffff 0000000000000000
> page dumped because: kasan: bad access detected
>
> Memory state around the buggy address:
> ffff888069a0c380: fb fb fb fb fc fc fc fc fb fb fb fb fb fb fb fb
> ffff888069a0c400: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
>> ffff888069a0c480: 00 00 00 00 00 00 00 00 fc fc fc fc fb fb fb fb
> ^
> ffff888069a0c500: fb fb fb fb fc fc fc fc fb fb fb fb fb fb fb fb
> ffff888069a0c580: fc fc fc fc fb fb fb fb fb fb fb fb fc fc fc fc
> ==================================================================
>
> Avoid it by keeping a reference to the socket file until the psock gets
> destroyed.
>
> While at it, rearrange the order of reference grabbing and
> initialization to match the destructor in reverse.
>
> Reported-by: Marek Majkowski <marek@cloudflare.com>
> Link: https://lore.kernel.org/netdev/CAJPywTLwgXNEZ2dZVoa=udiZmtrWJ0q5SuBW64aYs0Y1khXX3A@mail.gmail.com
> Signed-off-by: Jakub Sitnicki <jakub@cloudflare.com>
> ---
> net/core/skmsg.c | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
>
> diff --git a/net/core/skmsg.c b/net/core/skmsg.c
> index 8c826603bf36..a38442b8580b 100644
> --- a/net/core/skmsg.c
> +++ b/net/core/skmsg.c
> @@ -493,8 +493,13 @@ struct sk_psock *sk_psock_init(struct sock *sk, int node)
> sk_psock_set_state(psock, SK_PSOCK_TX_ENABLED);
> refcount_set(&psock->refcnt, 1);
>
> - rcu_assign_sk_user_data(sk, psock);
> + /* Hold on to socket wait queue. Backlog work waits on it for
> + * memory when sending. We must cancel work before socket wait
> + * queue can go away.
> + */
> + get_file(sk->sk_socket->file);
> sock_hold(sk);
> + rcu_assign_sk_user_data(sk, psock);
>
> return psock;
> }
> @@ -558,6 +563,7 @@ static void sk_psock_destroy_deferred(struct work_struct *gc)
> if (psock->sk_redir)
> sock_put(psock->sk_redir);
> sock_put(psock->sk);
> + fput(psock->sk->sk_socket->file);
Thanks for the report (and sorry for the late reply). I think holding ref on
the struct file just so we keep it alive till deferred destruction might be
papering over the actual underlying bug. Nothing obvious pops out from staring
at the code right now; as a reproducer to run, did you use the prog in the link
above and hit it after your strparser fix?
Thanks again,
Daniel
^ permalink raw reply
* Re: [PATCH net-next v4 05/17] net: sched: traverse chains in block with tcf_get_next_chain()
From: Vlad Buslov @ 2019-02-19 16:04 UTC (permalink / raw)
To: Cong Wang
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller, Alexei Starovoitov, Daniel Borkmann
In-Reply-To: <CAM_iQpVCP=6f4iRVqbgHxZcNHgmsdDJmuUQLk-9uPZar2xTGfw@mail.gmail.com>
On Mon 18 Feb 2019 at 18:26, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Mon, Feb 18, 2019 at 2:07 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>>
>> Hi Cong,
>>
>> Thanks for reviewing!
>>
>> On Fri 15 Feb 2019 at 22:21, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>> > (Sorry for joining this late.)
>> >
>> > On Mon, Feb 11, 2019 at 12:56 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>> >> @@ -2432,7 +2474,11 @@ static int tc_dump_chain(struct sk_buff *skb, struct netlink_callback *cb)
>> >> index_start = cb->args[0];
>> >> index = 0;
>> >>
>> >> - list_for_each_entry(chain, &block->chain_list, list) {
>> >> + for (chain = __tcf_get_next_chain(block, NULL);
>> >> + chain;
>> >> + chain_prev = chain,
>> >> + chain = __tcf_get_next_chain(block, chain),
>> >> + tcf_chain_put(chain_prev)) {
>> >
>> > Why do you want to take the block->lock in each iteration
>> > of the loop rather than taking once for the whole loop?
>>
>> This loop calls classifier ops callback in tc_chain_fill_node(). I don't
>> call any classifier ops callbacks while holding block or chain lock in
>> this change because the goal is to achieve fine-grained locking for data
>> structures used by filter update path. Locking per-block or per-chain is
>> much coarser than taking reference counters to parent structures and
>> allowing classifiers to implement their own locking.
>
> That is the problem, when we have N filter chains in a block, you
> lock and unlock mutex N times... And what __tcf_get_next_chain()
> does is basically just retrieving the next entry in the list, so the
> overhead of mutex is likely more than the list operation itself in
> contention situation.
>
> Now I can see why you complained about mutex before, it is
> how you use it, not actually its own problem. :)
>
>>
>> In this case call to ops->tmplt_dump() is probably quite fast and its
>> execution time doesn't depend on number of filters on the classifier, so
>> releasing block->lock on each iteration doesn't provide much benefit, if
>> at all. However, it is easier for me to reason about locking correctness
>> in this refactoring by following a simple rule that no locks (besides
>> rtnl mutex) can be held when calling classifier ops callbacks.
>
> Well, for me, a hierarchy locking is always simple when you take
> them in the right order, that is locking the larger-scope lock first
> and then smaller-scope one.
>
> The way you use the locking here is actually harder for me to
> review, because it is hard to valid its atomicity when you unlock
> the larger scope lock and re-take the smaller scope lock. You
> use refcnt to ensure it will not go way, but that is still far from
> guarantee of the atomicity.
>
> For example, tp->ops->change() which changes an existing
> filter, I don't see you lock either block->lock or
> chain->filter_chain_lock when calling it. How does it even work?
Sorry for not describing it in more details in cover letter. Basic
approach is that cls API obtains references to all necessary data
structures (Qdisc, block, chain, tp) and calls classifier ops callbacks
after releasing all the locks. This defers all locking and atomicity
concerns to actual classifier implementations. In case of
tp->ops->change() flower classifier uses tp->lock. In case of dump code
(both chain and filter) there is no atomicity with or without my changes
because rtnl lock is released after sending each skb to userspace and
concurrent changes to tc structures can occur in between.
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding
From: Vivien Didelot @ 2019-02-19 16:05 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190218125345.eq3mhmgsmgm7jmem@shell.armlinux.org.uk>
Hi Russell,
On Mon, 18 Feb 2019 12:53:45 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> > Switches work by learning the MAC address for each attached station by
> > monitoring traffic from each station. When a station sends a packet,
> > the switch records which port the MAC address is connected to.
> >
> > With IPv4 networking, before communication commences with a neighbour,
> > an ARP packet is broadcasted to all stations asking for the MAC address
> > corresponding with the IPv4. The desired station responds with an ARP
> > reply, and the ARP reply causes the switch to learn which port the
> > station is connected to.
> >
> > With IPv6 networking, the situation is rather different. Rather than
> > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > rather than broadcasted. This multicast needs to reach the intended
> > station in order for the neighbour to be discovered.
> >
> > Once a neighbour has been discovered, and entered into the sending
> > stations neighbour cache, communication can restart at a point later
> > without sending a new neighbour solicitation, even if the entry in
> > the neighbour cache is marked as stale. This can be after the MAC
> > address has expired from the forwarding cache of the DSA switch -
> > when that occurs, there is a long pause in communication.
Thank you for the very informative message above.
> > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > multicast and unicast flooding disabled. As per the above description,
> > this is fine for IPv4 networking, since the broadcasted ARP queries
> > will be sent to and received by all stations on the same network.
> > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > and later causing connections to stall.
> >
> > The defaults that the Linux bridge code expect from bridges are that
> > unknown unicast frames and unknown multicast frames are flooded to
> > all stations, which is at odds to the defaults adopted by our DSA
> > implementation for mv88e6xxx switches.
> >
> > This commit enables by default flooding of both unknown unicast and
> > unknown multicast frames. This means that mv88e6xxx DSA switches now
> > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > such a switch.
>
> Thinking about this a bit more, this approach probably isn't the best.
> If we have a port that goes through this life-cycle:
>
> 1. assigned to a bridge
> 2. configured not to flood
> 3. reassigned to a new bridge
>
> the port will retain its settings from the first bridge, which will be
> at odds with the settings that the Linux bridge code expects and the
> settings visible to the user.
>
> So, how about this, which basically reverts this patch and applies the
> flood settings each time a port joins a bridge, and clears them when
> the port leaves a bridge.
Isn't the bridge code programming flooding on the port correctly on leave/join,
because the BR_*FLOOD flags have been learned? I would expect that.
Thanks,
Vivien
^ permalink raw reply
* Re: Handling an Extra Signal at PHY Reset
From: Florian Fainelli @ 2019-02-19 16:07 UTC (permalink / raw)
To: Paul Kocialkowski, Andrew Lunn, Heiner Kallweit
Cc: netdev, Thomas Petazzoni, Mylène Josserand
In-Reply-To: <e647050a0fbadb8445cf6a7a5a2ccfbfd0865592.camel@bootlin.com>
On February 19, 2019 1:14:20 AM PST, Paul Kocialkowski <paul.kocialkowski@bootlin.com> wrote:
>Hi,
>
>We are dealing with an Ethernet PHY (Marvell 88E1512) that comes with a
>CONFIG pin that must be connected to one of the other pins of the PHY
>to configure the LSB of the PHY address as well as I/O voltages (see
>section 2.18.1 Hardware Configuration of the datasheet). It must be
>connected "soon after reset" for the PHY to be correctly configured.
Even voltage? What guarantees do you have that you are not reducing the lifetime of your pads if e.g.: you are configured for 3.3V while the other end is 1.8/2.5V? Is there some kind of embedded voltage comparator that can be used to help making the right decision?
>
>We have a switch for connecting the CONFIG pin to the other pin (LED0),
>which needs to be controlled by Linux. The CONFIG pin seems to be used
>for a PTP clock the rest of the time.
>
>So we are wondering how to properly represent this case, especially on
>the device-tree side.
>
>The trick here is that this step is necessary before the PHY can be
>discovered on the MDIO bus (and thus the PHY driver selected) so we
>can't rely on the PHY driver to do this. Basically, it looks like we
>need to handle this like the reset pin and describe it at the MDIO bus
>level.
>
>Here are some ideas for potential solutions:
>- Allowing more than a single GPIO to be passed to the MDIO bus' reset-
>gpios via device-tree and toggling all the passed GPIOs at once;
That would be a mis-representstion of the HW though, since the reset line is tied to the PHY package. Making use of the current implementation details to put a second reset line does not sound great.
>
>- Adding a new optional GPIO for the MDIO bus dedicated to controlling
>switches for such config switching, perhaps called "config-gpios"
>(quite a narrow solution);
Indeed, and still has the same design flaw as 1) outline above.
>
>- Adding a broader power sequence description to the MDIO bus (a bit
>like it's done with the mmc pwrseq descriptions) which would allow
>specifying the toggle order/delays of various GPIOs (would probably be
>the most extensive solution);
That one looks the most compelling and future proof although I do wonder how many people would make use of that.
>
>- Adding the extra GPIO control to the MAC description and toggling it
>through bus->reset (probably the less invasive solution for the core
>but not very satisfying from the description perspective, since this is
>definitely not MAC-specific).
>
>What do you think about how we could solve this issue?
>Do you see other options that I missed here?
You could explore having the MDIO bus layer scan its children nodes (PHY nodes) and handle properties in there before registering devices, so for insurance your PHY DT nodes can have an arbitrary number of reset lines, power sequencing properties etc. and the MDIO bus layer knowing it's internal implementation does make sure that it makes use of these properties in order to make PHY devices functional.
Does that make sense? One possible caveat is that the CONFIG pin also dictates the address on the bus, so what do we do with the PHY's "reg" property, is it it's actual address or is it the desired one that we should configure through reset?
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Daniel Borkmann @ 2019-02-19 16:08 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20190219165745.676097a3@carbon>
On 02/19/2019 04:57 PM, Jesper Dangaard Brouer wrote:
> On Tue, 19 Feb 2019 12:46:57 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>>> Due to lack of TC examples, lets show howto attach clsact BPF programs:
>>>
>>> # tc qdisc add dev ixgbe2 clsact
>>> # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
>>> # tc filter list dev ixgbe2 egress
>
> Recommending the "replace" is wrong is seems, as does not replace the
> existing, but keeps adding more filter entries.
>
> What is the recommended procedure for unloading and loading a newer
> version of the BPF TC program?
You would need to specify prio / handle in order to select a particular
instance for atomic replacement:
tc filter replace dev foo {e,in}gress prio 1 handle 1 bpf da obj foo.o
^ permalink raw reply
* Re: [PATCH net] team: use operstate consistently for linkup
From: Jiri Pirko @ 2019-02-19 16:00 UTC (permalink / raw)
To: George Wilkie; +Cc: David S. Miller, netdev
In-Reply-To: <20190219155715.769-1-gwilkie@vyatta.att-mail.com>
Tue, Feb 19, 2019 at 04:57:15PM CET, gwilkie@vyatta.att-mail.com wrote:
>When a port is added to a team, its initial state is derived
>from netif_carrier_ok rather than netif_oper_up.
>If it is carrier up but operationally down at the time of being
>added, the port state.linkup will be set prematurely.
>port state.linkup should be set consistently using
>netif_oper_up rather than netif_carrier_ok.
>
>Fixes: f1d22a1e0595 ("team: account for oper state")
>
>Signed-off-by: George Wilkie <gwilkie@vyatta.att-mail.com>
Acked-by: Jiri Pirko <jiri@mellanox.com>
^ permalink raw reply
* pull-request: ieee802154-next 2019-02-19
From: Stefan Schmidt @ 2019-02-19 16:11 UTC (permalink / raw)
To: davem; +Cc: linux-wpan, alex.aring, netdev
Hello Dave.
An update from ieee802154 for *net-next*
Another quite quite cycle in the ieee802154 subsystem.
Peter did a rework of the IP frag queue handling to make it use rbtree and get
in line with the core IPv4 and IPv6 implementatiosn in the kernel.
Please pull, or let me know if there are any problems.
regards
Stefan Schmidt
The following changes since commit c9b747dbc2036c917b1067fbb78dc38b105c4454:
bnx2x: Remove set but not used variable 'mfw_vn' (2019-02-18 16:47:32 -0800)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/sschmidt/wpan-next.git ieee802154-for-davem-2019-02-19
for you to fetch changes up to 254c5dbe15d44c9d76a3ccd3724eb1befb7adf99:
6lowpan: use rbtree for IP frag queue (2019-02-19 10:38:01 +0100)
----------------------------------------------------------------
Peter Oskolkov (1):
6lowpan: use rbtree for IP frag queue
net/ieee802154/6lowpan/reassembly.c | 141 ++++++++----------------------------
1 file changed, 29 insertions(+), 112 deletions(-)
^ permalink raw reply
* Re: [PATCH net-next v2 2/3] net: dsa: mv88e6xxx: add support for bridge flags
From: Vivien Didelot @ 2019-02-19 16:16 UTC (permalink / raw)
To: Russell King
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <E1gvPMo-0000Ah-Vy@rmk-PC.armlinux.org.uk>
Hi Russell,
On Sun, 17 Feb 2019 16:32:34 +0000, Russell King <rmk+kernel@armlinux.org.uk> wrote:
> +static int mv88e6xxx_port_bridge_flags(struct dsa_switch *ds, int port,
> + unsigned long flags)
> +{
> + struct mv88e6xxx_chip *chip = ds->priv;
> + bool unicast, multicast;
> + int ret = -EOPNOTSUPP;
> +
> + unicast = dsa_is_cpu_port(ds, port) || dsa_is_dsa_port(ds, port) ||
> + flags & BR_FLOOD;
> + multicast = flags & BR_MCAST_FLOOD;
> +
> + mutex_lock(&chip->reg_lock);
> + if (chip->info->ops->port_set_egress_floods)
> + ret = chip->info->ops->port_set_egress_floods(chip, port,
> + unicast,
> + multicast);
> + mutex_unlock(&chip->reg_lock);
> +
> + return ret;
> +}
> +
> +static unsigned long mv88e6xxx_bridge_flags_support(struct dsa_switch *ds)
> +{
> + struct mv88e6xxx_chip *chip = ds->priv;
> + unsigned long support = 0;
> +
> + if (chip->info->ops->port_set_egress_floods)
> + support |= BR_FLOOD | BR_MCAST_FLOOD;
> +
> + return support;
> +}
I think that it isn't necessary to propagate the notion of bridge flags down
to the DSA drivers. It might be just enough to add:
port_egress_flood(dsa_switch *ds, int port, bool uc, bool mc)
to dsa_switch_ops and set BR_FLOOD | BR_MCAST_FLOOD from the DSA core,
if the targeted driver has ds->ops->port_set_egress_flood. What do you think?
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH net-next v2 3/3] net: dsa: mv88e6xxx: default to multicast and unicast flooding
From: Russell King - ARM Linux admin @ 2019-02-19 16:18 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, Heiner Kallweit, David S. Miller,
netdev
In-Reply-To: <20190219110537.GD27578@t480s.localdomain>
On Tue, Feb 19, 2019 at 11:05:37AM -0500, Vivien Didelot wrote:
> Hi Russell,
>
> On Mon, 18 Feb 2019 12:53:45 +0000, Russell King - ARM Linux admin <linux@armlinux.org.uk> wrote:
> > On Sun, Feb 17, 2019 at 04:32:40PM +0000, Russell King wrote:
> > > Switches work by learning the MAC address for each attached station by
> > > monitoring traffic from each station. When a station sends a packet,
> > > the switch records which port the MAC address is connected to.
> > >
> > > With IPv4 networking, before communication commences with a neighbour,
> > > an ARP packet is broadcasted to all stations asking for the MAC address
> > > corresponding with the IPv4. The desired station responds with an ARP
> > > reply, and the ARP reply causes the switch to learn which port the
> > > station is connected to.
> > >
> > > With IPv6 networking, the situation is rather different. Rather than
> > > broadcasting ARP packets, a "neighbour solicitation" is multicasted
> > > rather than broadcasted. This multicast needs to reach the intended
> > > station in order for the neighbour to be discovered.
> > >
> > > Once a neighbour has been discovered, and entered into the sending
> > > stations neighbour cache, communication can restart at a point later
> > > without sending a new neighbour solicitation, even if the entry in
> > > the neighbour cache is marked as stale. This can be after the MAC
> > > address has expired from the forwarding cache of the DSA switch -
> > > when that occurs, there is a long pause in communication.
>
> Thank you for the very informative message above.
>
> > > Our DSA implementation for mv88e6xxx switches has defaulted to having
> > > multicast and unicast flooding disabled. As per the above description,
> > > this is fine for IPv4 networking, since the broadcasted ARP queries
> > > will be sent to and received by all stations on the same network.
> > > However, this breaks IPv6 very badly - blocking neighbour solicitations
> > > and later causing connections to stall.
> > >
> > > The defaults that the Linux bridge code expect from bridges are that
> > > unknown unicast frames and unknown multicast frames are flooded to
> > > all stations, which is at odds to the defaults adopted by our DSA
> > > implementation for mv88e6xxx switches.
> > >
> > > This commit enables by default flooding of both unknown unicast and
> > > unknown multicast frames. This means that mv88e6xxx DSA switches now
> > > behave as per the bridge(8) man page, and IPv6 works flawlessly through
> > > such a switch.
> >
> > Thinking about this a bit more, this approach probably isn't the best.
> > If we have a port that goes through this life-cycle:
> >
> > 1. assigned to a bridge
> > 2. configured not to flood
> > 3. reassigned to a new bridge
> >
> > the port will retain its settings from the first bridge, which will be
> > at odds with the settings that the Linux bridge code expects and the
> > settings visible to the user.
> >
> > So, how about this, which basically reverts this patch and applies the
> > flood settings each time a port joins a bridge, and clears them when
> > the port leaves a bridge.
>
> Isn't the bridge code programming flooding on the port correctly on leave/join,
> because the BR_*FLOOD flags have been learned? I would expect that.
If you're asking whether the bridge code sends a
SWITCHDEV_ATTR_ID_PORT_BRIDGE_FLAGS message on leave/join, it seems
that it does not.
There is only one place in the bridge code that this message is
generated, that is in net/bridge/br_switchdev.c
br_switchdev_set_port_flag(). That is called from one place in the
bridge code, which is br_set_port_flag() in net/bridge/br_netlink.c,
which is in response to a RTM_SETLINK netlink message where the bridge
code processes all the various bridge link options.
There appears to be no call when adding or removing an interface to/
from a bridge.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH bpf-next] bpf: add skb->queue_mapping write access from tc clsact
From: Daniel Borkmann @ 2019-02-19 16:18 UTC (permalink / raw)
To: Jesper Dangaard Brouer; +Cc: netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <20190219155259.677d195c@carbon>
On 02/19/2019 03:52 PM, Jesper Dangaard Brouer wrote:
> On Tue, 19 Feb 2019 12:46:57 +0100
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 02/19/2019 11:24 AM, Jesper Dangaard Brouer wrote:
>>> The skb->queue_mapping already have read access, via __sk_buff->queue_mapping.
>>>
>>> This patch allow BPF tc qdisc clsact write access to the queue_mapping via
>>> tc_cls_act_is_valid_access.
>>>
>>> It is already possible to change this via TC filter action skbedit
>>> tc-skbedit(8). Due to the lack of TC examples, lets show one:
>>>
>>> # tc qdisc add dev ixgbe1 handle ffff: ingress
>>> # tc filter add dev ixgbe1 parent ffff: matchall action skbedit queue_mapping 5
>>> # tc filter list dev ixgbe1 parent ffff:
>>
>> Using handles was in the old days, if we add examples, then lets do
>> something more user friendly ;)
>>
>> # tc qdisc add dev ixgbe1 clsact
>> # tc filter replace dev ixgbe1 ingress matchall action skbedit queue_mapping 5
>> # tc filter list dev ixgbe1 ingress
>>
>>> The most common mistake is that XPS (Transmit Packet Steering) takes
>>> precedence over setting skb->queue_mapping. XPS is configured per DEVICE
>>> via /sys/class/net/DEVICE/queues/tx-*/xps_cpus via a CPU hex mask. To
>>> disable set mask=00.
>>>
>>> The purpose of changing skb->queue_mapping is to influence the selection of
>>> the net_device "txq" (struct netdev_queue), which influence selection of
>>> the qdisc "root_lock" (via txq->qdisc->q.lock) and txq->_xmit_lock. When
>>> using the MQ qdisc the txq->qdisc points to different qdiscs and associated
>>> locks, and HARD_TX_LOCK (txq->_xmit_lock), allowing for CPU scalability.
>>>
>>> Due to lack of TC examples, lets show howto attach clsact BPF programs:
>>>
>>> # tc qdisc add dev ixgbe2 clsact
>>> # tc filter replace dev ixgbe2 egress bpf da obj XXX_kern.o sec tc_qmap2cpu
>>> # tc filter list dev ixgbe2 egress
>>>
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>> net/core/filter.c | 14 +++++++++++---
>>> 1 file changed, 11 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 353735575204..d05ae8d05397 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -6238,6 +6238,7 @@ static bool tc_cls_act_is_valid_access(int off, int size,
>>> case bpf_ctx_range(struct __sk_buff, tc_classid):
>>> case bpf_ctx_range_till(struct __sk_buff, cb[0], cb[4]):
>>> case bpf_ctx_range(struct __sk_buff, tstamp):
>>> + case bpf_ctx_range(struct __sk_buff, queue_mapping):
>>> break;
>>> default:
>>> return false;
>>> @@ -6642,9 +6643,16 @@ static u32 bpf_convert_ctx_access(enum bpf_access_type type,
>>> break;
>>>
>>> case offsetof(struct __sk_buff, queue_mapping):
>>> - *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> - bpf_target_off(struct sk_buff, queue_mapping, 2,
>>> - target_size));
>>> + if (type == BPF_WRITE)
>>> + *insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> + bpf_target_off(struct sk_buff,
>>> + queue_mapping,
>>> + 2, target_size));
>>> + else
>>> + *insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
>>> + bpf_target_off(struct sk_buff,
>>> + queue_mapping,
>>> + 2, target_size));
>>
>> One thing we should avoid would be to allow user to write NO_QUEUE_MAPPING
>> into skb->queue_mapping so we don't hit the warn in sk_tx_queue_set(), I'd
>> add this into the ctx rewrite here.
>
> Makes sense. I would really appreciate if you could help me out writing
> the needed BPF instructions, as I'm not an expert here.
Untested / uncompiled, but should be:
case offsetof(struct __sk_buff, queue_mapping):
if (type == BPF_WRITE) {
*insn++ = BPF_JMP_IMM(BPF_JGE, si->src_reg, NO_QUEUE_MAPPING, 1);
*insn++ = BPF_STX_MEM(BPF_H, si->dst_reg, si->src_reg,
bpf_target_off(struct sk_buff, queue_mapping, 2,
target_size));
} else {
*insn++ = BPF_LDX_MEM(BPF_H, si->dst_reg, si->src_reg,
bpf_target_off(struct sk_buff, queue_mapping, 2,
target_size));
}
break;
^ 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