* Re: pegged softirq and NAPI race (?)
From: Rik van Riel @ 2018-09-18 16:31 UTC (permalink / raw)
To: Song Liu, Eric Dumazet
Cc: netdev, Jeff Kirsher, Alexander Duyck, michael.chan@broadcom.com,
Kernel Team
In-Reply-To: <6FA4008E-CEEB-4EAB-BAD8-267D41574248@fb.com>
On Tue, 2018-09-18 at 12:19 -0400, Song Liu wrote:
> > On Sep 18, 2018, at 6:45 AM, Eric Dumazet <edumazet@google.com>
> > wrote:
> >
> > On Tue, Sep 18, 2018 at 1:41 AM Song Liu <songliubraving@fb.com>
> > wrote:
> > >
> > > We are debugging this issue that netconsole message triggers
> > > pegged softirq
> > > (ksoftirqd taking 100% CPU for many seconds). We found this issue
> > > in
> > > production with both bnxt and ixgbe, on a 4.11 based kernel. This
> > > is easily
> > > reproducible with ixgbe on 4.11, and latest net/net-next (see [1]
> > > for more
> > > detail).
> > >
> > > After debugging for some time, we found that this issue is likely
> > > related
> > > to 39e6c8208d7b ("net: solve a NAPI race"). After reverting this
> > > commit,
> > > the steps described in [1] cannot reproduce the issue on ixgbe.
> > > Reverting
> > > this commit also reduces the chances we hit the issue with bnxt
> > > (it still
> > > happens with a lower rate).
> > >
> > > I tried to fix this issue with relaxed variant (or older version)
> > > of
> > > napi_schedule_prep() in netpoll, just like the one on
> > > napi_watchdog().
> > > However, my tests do not always go as expected.
> > >
> > > Please share your comments/suggestions on which direction shall
> > > we try
> > > to fix this.
> > >
> > > Thanks in advance!
> > > Song
> > >
> > >
> > > [1]
> > > https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_netdev_msg522328.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=iSaOapj1kxjhGYLgQr0Qd8mQCzVdobmgT1L4JwFvzxs&s=lCEhrz6wQJUUaJOkxFmtOszAgkf3Jh4reX_i1GbI5RI&e=
> >
> > You have not traced ixgbe to understand why driver hits
> > "clean_complete=false" all the time ?
>
> The trace showed that we got "clean_complete=false" because
> ixgbe_clean_rx_irq() used all budget (64). It feels like the driver
> is tricked to process old data on the rx_ring for one more time.
>
> Have you seen similar issue?
A quick reading of the code suggests that means
polling cannot keep up with the rate of incoming
packets.
That should not be a surprise, given that polling
appears to happen on just one CPU, while interrupt
driven packet delivery was fanned out across a
larger number of CPUs.
Does the NAPI code have any way in which it
periodically force-returns to IRQ mode, because
multiple CPUs in IRQ mode can keep up with packets
better than a single CPU in polling mode?
Alternatively, is NAPI with multi-queue network
adapters supposed to be polling on multiple CPUs,
but simply failing to do so in this case?
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 16:33 UTC (permalink / raw)
To: songliubraving
Cc: netdev, Jeff Kirsher, Alexander Duyck, michael.chan, kernel-team
In-Reply-To: <6FA4008E-CEEB-4EAB-BAD8-267D41574248@fb.com>
On Tue, Sep 18, 2018 at 9:19 AM Song Liu <songliubraving@fb.com> wrote:
>
>
>
> > On Sep 18, 2018, at 6:45 AM, Eric Dumazet <edumazet@google.com> wrote:
> >
> > On Tue, Sep 18, 2018 at 1:41 AM Song Liu <songliubraving@fb.com> wrote:
> >>
> >> We are debugging this issue that netconsole message triggers pegged softirq
> >> (ksoftirqd taking 100% CPU for many seconds). We found this issue in
> >> production with both bnxt and ixgbe, on a 4.11 based kernel. This is easily
> >> reproducible with ixgbe on 4.11, and latest net/net-next (see [1] for more
> >> detail).
> >>
> >> After debugging for some time, we found that this issue is likely related
> >> to 39e6c8208d7b ("net: solve a NAPI race"). After reverting this commit,
> >> the steps described in [1] cannot reproduce the issue on ixgbe. Reverting
> >> this commit also reduces the chances we hit the issue with bnxt (it still
> >> happens with a lower rate).
> >>
> >> I tried to fix this issue with relaxed variant (or older version) of
> >> napi_schedule_prep() in netpoll, just like the one on napi_watchdog().
> >> However, my tests do not always go as expected.
> >>
> >> Please share your comments/suggestions on which direction shall we try
> >> to fix this.
> >>
> >> Thanks in advance!
> >> Song
> >>
> >>
> >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_netdev_msg522328.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=iSaOapj1kxjhGYLgQr0Qd8mQCzVdobmgT1L4JwFvzxs&s=lCEhrz6wQJUUaJOkxFmtOszAgkf3Jh4reX_i1GbI5RI&e=
> >
> > You have not traced ixgbe to understand why driver hits
> > "clean_complete=false" all the time ?
>
> The trace showed that we got "clean_complete=false" because
> ixgbe_clean_rx_irq() used all budget (64). It feels like the driver
> is tricked to process old data on the rx_ring for one more time.
Process old data ???? That would be quite an horrible bug !
Probably ASAN would help here, detecting use-after-free or things like that.
^ permalink raw reply
* Re: [PATCH 1/2] netlink: add NLA_REJECT policy type
From: Johannes Berg @ 2018-09-18 16:42 UTC (permalink / raw)
To: Jamal Hadi Salim, Marcelo Ricardo Leitner, Michal Kubecek
Cc: linux-wireless, netdev, jbenc
In-Reply-To: <1a1e0b60-24d9-a313-f46e-afb590a99b3a@mojatatu.com>
On Tue, 2018-09-18 at 09:12 -0400, Jamal Hadi Salim wrote:
> Not very familiar with how wifi scan gets initiated. I am guessing
> you issue some GET or SET to start a scan - and you get an async
> response when it is complete (and it would include the time it took)?
> Or maybe you get an immediate response and event notification later
> and the time spent is in that notification?
There isn't really a GET or SET. It's just an arbitrary generic netlink
command that you send down. Not everything is a GET/SET model :-)
But yes, you issue a command via generic netlink to start a scan, with
some attributes, and then eventually get an async notification when it's
done (or was aborted for some reason.)
For purposes of my example, that time attribute would be in the
notification, yes.
> I would still see that as a read-only attribute.
I suppose you could, but you never really get to see it anywhere else.
> And the utility of "execute" bit is only in blocking another scan
> from being initiated when one is in progress, if that is a desired
> goal.
Not sure I understand that. I'm not really talking about some sort of
generic "SET_VALUE" command with a "SCAN_NOW" attribute?
> > I dunno. I think we already bloated the policies too much by including
> > the validation_data pointer, and would hate to add more to that :-)
>
> Your mileage may vary. NLA_REJECT may work acls offer more fine grained
> controls.
Fair point. We do have padding in the policy (at least on 64-bit
platforms) that we could use for more bits ;-)
johannes
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Song Liu @ 2018-09-18 16:49 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev, Jeff Kirsher, Alexander Duyck, michael.chan@broadcom.com,
Kernel Team
In-Reply-To: <CANn89iK4O+4RgFkaF0b9N6AFzZCGL8FjsNWBYNcr6MA2CaSRXw@mail.gmail.com>
> On Sep 18, 2018, at 9:33 AM, Eric Dumazet <edumazet@google.com> wrote:
>
> On Tue, Sep 18, 2018 at 9:19 AM Song Liu <songliubraving@fb.com> wrote:
>>
>>
>>
>>> On Sep 18, 2018, at 6:45 AM, Eric Dumazet <edumazet@google.com> wrote:
>>>
>>> On Tue, Sep 18, 2018 at 1:41 AM Song Liu <songliubraving@fb.com> wrote:
>>>>
>>>> We are debugging this issue that netconsole message triggers pegged softirq
>>>> (ksoftirqd taking 100% CPU for many seconds). We found this issue in
>>>> production with both bnxt and ixgbe, on a 4.11 based kernel. This is easily
>>>> reproducible with ixgbe on 4.11, and latest net/net-next (see [1] for more
>>>> detail).
>>>>
>>>> After debugging for some time, we found that this issue is likely related
>>>> to 39e6c8208d7b ("net: solve a NAPI race"). After reverting this commit,
>>>> the steps described in [1] cannot reproduce the issue on ixgbe. Reverting
>>>> this commit also reduces the chances we hit the issue with bnxt (it still
>>>> happens with a lower rate).
>>>>
>>>> I tried to fix this issue with relaxed variant (or older version) of
>>>> napi_schedule_prep() in netpoll, just like the one on napi_watchdog().
>>>> However, my tests do not always go as expected.
>>>>
>>>> Please share your comments/suggestions on which direction shall we try
>>>> to fix this.
>>>>
>>>> Thanks in advance!
>>>> Song
>>>>
>>>>
>>>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_netdev_msg522328.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=iSaOapj1kxjhGYLgQr0Qd8mQCzVdobmgT1L4JwFvzxs&s=lCEhrz6wQJUUaJOkxFmtOszAgkf3Jh4reX_i1GbI5RI&e=
>>>
>>> You have not traced ixgbe to understand why driver hits
>>> "clean_complete=false" all the time ?
>>
>> The trace showed that we got "clean_complete=false" because
>> ixgbe_clean_rx_irq() used all budget (64). It feels like the driver
>> is tricked to process old data on the rx_ring for one more time.
>
> Process old data ???? That would be quite an horrible bug !
>
> Probably ASAN would help here, detecting use-after-free or things like that.
I have tried KASAN, unfortunately, it doesn't yield any useful data. I think
it is not a use-after-free of some skb. It is more like bugs in the handling
of the rx/tx ring.
Thanks,
Song
^ permalink raw reply
* Re: [PATCH v3 net-next] ravb: do not write 1 to reserved bits
From: Sergei Shtylyov @ 2018-09-18 16:54 UTC (permalink / raw)
To: Simon Horman, David Miller; +Cc: Magnus Damm, netdev, linux-renesas-soc
In-Reply-To: <20180918102226.8017-1-horms+renesas@verge.net.au>
On 09/18/2018 01:22 PM, Simon Horman wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> EtherAVB hardware requires 0 to be written to status register bits in
> order to clear them, however, care must be taken not to:
>
> 1. Clear other bits, by writing zero to them
> 2. Write one to reserved bits
>
> This patch corrects the ravb driver with respect to the second point above.
> This is done by defining reserved bit masks for the affected registers and,
> after auditing the code, ensure all sites that may write a one to a
> reserved bit use are suitably masked.
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
[...]
Reviewed-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index 1470fc12282b..9b6bf557a2f5 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -428,6 +428,7 @@ enum EIS_BIT {
> EIS_CULF1 = 0x00000080,
> EIS_TFFF = 0x00000100,
> EIS_QFS = 0x00010000,
> + EIS_RESERVED = (GENMASK(31, 17) | GENMASK(15, 11)),
Well, I'm not a big fan of BIT() and GENMASK() -- they still lack a macro
to #define a bit/field value. But if you prefer to use them, OK, let's be so...
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH v3 net-next] ravb: do not write 1 to reserved bits
From: Sergei Shtylyov @ 2018-09-18 16:56 UTC (permalink / raw)
To: Simon Horman, David Miller; +Cc: Magnus Damm, netdev, linux-renesas-soc
In-Reply-To: <20180918102226.8017-1-horms+renesas@verge.net.au>
On 09/18/2018 01:22 PM, Simon Horman wrote:
> From: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
>
> EtherAVB hardware requires 0 to be written to status register bits in
> order to clear them, however, care must be taken not to:
>
> 1. Clear other bits, by writing zero to them
> 2. Write one to reserved bits
>
> This patch corrects the ravb driver with respect to the second point above.
> This is done by defining reserved bit masks for the affected registers and,
> after auditing the code, ensure all sites that may write a one to a
> reserved bit use are suitably masked.
>
> Signed-off-by: Kazuya Mizuguchi <kazuya.mizuguchi.ks@renesas.com>
> Signed-off-by: Simon Horman <horms+renesas@verge.net.au>
BTW, perhaps this should be merged into net.git instead? DaveM, your call? :-)
MBR, Sergei
^ permalink raw reply
* Re: kernel 4.18.5 Realtek 8111G network adapter stops responding under high system load
From: Maciej S. Szmigiero @ 2018-09-18 22:30 UTC (permalink / raw)
To: David Arendt; +Cc: linux-kernel, nic_swsd, netdev, Heiner Kallweit
In-Reply-To: <20180918102349.Horde.yHctatUod-_SC696oYKmySa@cloud.luxmemories.lu>
Hi,
On 18.09.2018 12:23, David Arendt wrote:
> Hi,
>
> Today I had the network adapter problems again.
> So the patch doesn't seem to change anything regarding this problem.
> This week my time is unfortunately very limited, but I will try to
> find some time next weekend to look a bit more into the issue.
If the problem is caused by missing TXCFG_AUTO_FIFO bit in TxConfig,
as the register difference would suggest, then you can try applying
the following patch (hack) on top of 4.18.8 that is already patched
with commit f74dd480cf4e:
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -5043,7 +5043,8 @@
{
/* Set DMA burst size and Interframe Gap Time */
RTL_W32(tp, TxConfig, (TX_DMA_BURST << TxDMAShift) |
- (InterFrameGap << TxInterFrameGapShift));
+ (InterFrameGap << TxInterFrameGapShift)
+ | TXCFG_AUTO_FIFO);
}
static void rtl_set_rx_max_size(struct rtl8169_private *tp)
This hack will probably only work properly on RTL_GIGA_MAC_VER_40 or
later NICs.
Before running any tests please verify with "ethtool -d enp3s0" that
TxConfig register now contains 0x4f000f80, as it did in the old,
working driver version.
If this does not help then a bisection will most likely be needed.
> Thanks in advance,
> David Arendt
Maciej
^ permalink raw reply
* Re: [PATCH net-next v5 19/20] security/keys: rewrite big_key crypto to use Zinc
From: David Howells @ 2018-09-18 17:01 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: dhowells, linux-kernel, netdev, linux-crypto, davem, gregkh,
Samuel Neves, Andy Lutomirski, Jean-Philippe Aumasson,
Eric Biggers
In-Reply-To: <20180918161646.19105-20-Jason@zx2c4.com>
Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> A while back, I noticed that the crypto and crypto API usage in big_keys
> were entirely broken in multiple ways, so I rewrote it. Now, I'm
> rewriting it again, but this time using Zinc's ChaCha20Poly1305
> function.
warthog>git grep chacha20poly1305_decrypt net-next/master
warthog1>
Where do I find this?
David
^ permalink raw reply
* [PATCH bpf] tools: bpf: fix license for a compat header file
From: Jakub Kicinski @ 2018-09-18 17:13 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, oss-drivers, Jakub Kicinski
libc_compat.h is used by libbpf so make sure it's licensed under
LGPL or BSD license. The license change should be OK, I'm the only
author of the file.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
tools/include/tools/libc_compat.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/include/tools/libc_compat.h b/tools/include/tools/libc_compat.h
index 664ced8cb1b0..e907ba6f15e5 100644
--- a/tools/include/tools/libc_compat.h
+++ b/tools/include/tools/libc_compat.h
@@ -1,4 +1,4 @@
-// SPDX-License-Identifier: GPL-2.0+
+// SPDX-License-Identifier: (LGPL-2.0+ OR BSD-2-Clause)
/* Copyright (C) 2018 Netronome Systems, Inc. */
#ifndef __TOOLS_LIBC_COMPAT_H
--
2.17.1
^ permalink raw reply related
* Re: [RFC 2/5] netlink: set extack error message in nla_validate()
From: David Ahern @ 2018-09-18 17:18 UTC (permalink / raw)
To: Johannes Berg, netdev; +Cc: Johannes Berg
In-Reply-To: <20180918131212.20266-2-johannes@sipsolutions.net>
On 9/18/18 6:12 AM, Johannes Berg wrote:
> diff --git a/lib/nlattr.c b/lib/nlattr.c
> index 120ad569e13d..efbd6c1aff29 100644
> --- a/lib/nlattr.c
> +++ b/lib/nlattr.c
> @@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
> int rem;
>
> nla_for_each_attr(nla, head, len, rem) {
> - int err = validate_nla(nla, maxtype, policy, NULL);
> + static const char _msg[] = "Attribute failed policy validation";
> + const char *msg = _msg;
> + int err = validate_nla(nla, maxtype, policy, &msg);
>
> if (err < 0) {
> + if (extack)
> + extack->_msg = msg;
> NL_SET_BAD_ATTR(extack, nla);
> return err;
> }
>
I take it this set is on top of another set - the NLA_REJECT?
^ permalink raw reply
* Re: [RFC 1/5] netlink: remove NLA_NESTED_COMPAT
From: David Ahern @ 2018-09-18 17:18 UTC (permalink / raw)
To: Johannes Berg, netdev; +Cc: Johannes Berg
In-Reply-To: <20180918131212.20266-1-johannes@sipsolutions.net>
On 9/18/18 6:12 AM, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> This isn't used anywhere, so we might as well get rid of it.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> include/net/netlink.h | 2 --
> lib/nlattr.c | 11 -----------
> 2 files changed, 13 deletions(-)
>
Reviewed-by: David Ahern <dsahern@gmail.com>
^ permalink raw reply
* Re: [PATCH net-next v5 09/20] zinc: Poly1305 ARM and ARM64 implementations
From: Eric Biggers @ 2018-09-18 22:55 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: linux-kernel, netdev, linux-crypto, davem, gregkh, Samuel Neves,
Andy Lutomirski, Jean-Philippe Aumasson, Andy Polyakov,
Russell King, linux-arm-kernel
In-Reply-To: <20180918161646.19105-10-Jason@zx2c4.com>
On Tue, Sep 18, 2018 at 06:16:35PM +0200, Jason A. Donenfeld wrote:
> diff --git a/lib/zinc/poly1305/poly1305-arm-glue.h b/lib/zinc/poly1305/poly1305-arm-glue.h
> new file mode 100644
> index 000000000000..dd3fa5a38c62
> --- /dev/null
> +++ b/lib/zinc/poly1305/poly1305-arm-glue.h
> @@ -0,0 +1,65 @@
> +/* SPDX-License-Identifier: MIT
> + *
> + * Copyright (C) 2015-2018 Jason A. Donenfeld <Jason@zx2c4.com>. All Rights Reserved.
> + */
> +
> +#include <asm/hwcap.h>
> +#include <asm/neon.h>
> +
> +asmlinkage void poly1305_init_arm(void *ctx, const u8 key[16]);
> +asmlinkage void poly1305_blocks_arm(void *ctx, const u8 *inp, const size_t len,
> + const u32 padbit);
> +asmlinkage void poly1305_emit_arm(void *ctx, u8 mac[16], const u32 nonce[4]);
> +#if IS_ENABLED(CONFIG_KERNEL_MODE_NEON) && \
> + (defined(CONFIG_64BIT) || __LINUX_ARM_ARCH__ >= 7)
> +#define ARM_USE_NEON
> +asmlinkage void poly1305_blocks_neon(void *ctx, const u8 *inp, const size_t len,
> + const u32 padbit);
> +asmlinkage void poly1305_emit_neon(void *ctx, u8 mac[16], const u32 nonce[4]);
> +#endif
> +
> +static bool poly1305_use_neon __ro_after_init;
> +
> +static void __init poly1305_fpu_init(void)
> +{
> +#if defined(CONFIG_ARM64)
> + poly1305_use_neon = elf_hwcap & HWCAP_ASIMD;
> +#elif defined(CONFIG_ARM)
> + poly1305_use_neon = elf_hwcap & HWCAP_NEON;
> +#endif
> +}
> +
> +static inline bool poly1305_init_arch(void *ctx,
> + const u8 key[POLY1305_KEY_SIZE])
> +{
> + poly1305_init_arm(ctx, key);
> + return true;
> +}
> +
> +static inline bool poly1305_blocks_arch(void *ctx, const u8 *inp,
> + const size_t len, const u32 padbit,
> + simd_context_t *simd_context)
> +{
> +#if defined(ARM_USE_NEON)
> + if (poly1305_use_neon && simd_use(simd_context)) {
> + poly1305_blocks_neon(ctx, inp, len, padbit);
> + return true;
> + }
> +#endif
> + poly1305_blocks_arm(ctx, inp, len, padbit);
> + return true;
> +}
This will compute the wrong digest if called with simd_context=HAVE_FULL_SIMD
and then later with simd_context=HAVE_NO_SIMD, since poly1305_blocks_neon()
converts the accumulator from base 32 to base 26, whereas poly1305_blocks_arm()
assumes it is still in base 32. Is that intentional? I'm sure this is a rare
case, but my understanding is that the existing crypto API doesn't preclude
calling successive steps in different contexts. And I'm concerned that it could
be relevant in some cases, e.g. especially if people are importing a hash state
that was exported earlier. Handling it by silently computing the wrong digest
is not a great idea...
- Eric
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] xsk: fix bug when trying to use both copy and zero-copy on one queue id
From: Y Song @ 2018-09-18 17:22 UTC (permalink / raw)
To: magnus.karlsson
Cc: Björn Töpel, Alexei Starovoitov, Daniel Borkmann,
netdev
In-Reply-To: <1537265538-5882-3-git-send-email-magnus.karlsson@intel.com>
On Tue, Sep 18, 2018 at 3:13 AM Magnus Karlsson
<magnus.karlsson@intel.com> wrote:
>
> Previously, the xsk code did not record which umem was bound to a
> specific queue id. This was not required if all drivers were zero-copy
> enabled as this had to be recorded in the driver anyway. So if a user
> tried to bind two umems to the same queue, the driver would say
> no. But if copy-mode was first enabled and then zero-copy mode (or the
> reverse order), we mistakenly enabled both of them on the same umem
> leading to buggy behavior. The main culprit for this is that we did
> not store the association of umem to queue id in the copy case and
> only relied on the driver reporting this. As this relation was not
> stored in the driver for copy mode (it does not rely on the AF_XDP
> NDOs), this obviously could not work.
>
> This patch fixes the problem by always recording the umem to queue id
> relationship in the netdev_queue and netdev_rx_queue structs. This way
> we always know what kind of umem has been bound to a queue id and can
> act appropriately at bind time.
>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> ---
> net/xdp/xdp_umem.c | 87 +++++++++++++++++++++++++++++++++++++++++++-----------
> net/xdp/xdp_umem.h | 2 +-
> 2 files changed, 71 insertions(+), 18 deletions(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index b3b632c..12300b5 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -42,6 +42,41 @@ void xdp_del_sk_umem(struct xdp_umem *umem, struct xdp_sock *xs)
> }
> }
>
> +/* The umem is stored both in the _rx struct and the _tx struct as we do
> + * not know if the device has more tx queues than rx, or the opposite.
> + * This might also change during run time.
> + */
> +static void xdp_reg_umem_at_qid(struct net_device *dev, struct xdp_umem *umem,
> + u16 queue_id)
> +{
> + if (queue_id < dev->real_num_rx_queues)
> + dev->_rx[queue_id].umem = umem;
> + if (queue_id < dev->real_num_tx_queues)
> + dev->_tx[queue_id].umem = umem;
> +}
> +
> +static struct xdp_umem *xdp_get_umem_from_qid(struct net_device *dev,
> + u16 queue_id)
> +{
> + if (queue_id < dev->real_num_rx_queues)
> + return dev->_rx[queue_id].umem;
> + if (queue_id < dev->real_num_tx_queues)
> + return dev->_tx[queue_id].umem;
> +
> + return NULL;
> +}
> +
> +static void xdp_clear_umem_at_qid(struct net_device *dev, u16 queue_id)
> +{
> + /* Zero out the entry independent on how many queues are configured
> + * at this point in time, as it might be used in the future.
> + */
> + if (queue_id < dev->num_rx_queues)
> + dev->_rx[queue_id].umem = NULL;
> + if (queue_id < dev->num_tx_queues)
> + dev->_tx[queue_id].umem = NULL;
> +}
> +
I am sure whether the following scenario can happen or not.
Could you clarify?
1. suppose initially we have num_rx_queues = num_tx_queues = 10
xdp_reg_umem_at_qid() set umem1 to queue_id = 8
2. num_tx_queues is changed to 5
3. xdp_clear_umem_at_qid() is called for queue_id = 8,
and dev->_rx[8].umum = 0.
4. xdp_reg_umem_at_qid() is called gain to set for queue_id = 8
dev->_rx[8].umem = umem2
5. num_tx_queues is changed to 10
Now dev->_rx[8].umem != dev->_tx[8].umem, is this possible and is it
a problem?
> int xdp_umem_query(struct net_device *dev, u16 queue_id)
> {
> struct netdev_bpf bpf;
> @@ -58,11 +93,11 @@ int xdp_umem_query(struct net_device *dev, u16 queue_id)
> }
>
> int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> - u32 queue_id, u16 flags)
> + u16 queue_id, u16 flags)
> {
> bool force_zc, force_copy;
> struct netdev_bpf bpf;
> - int err;
> + int err = 0;
>
> force_zc = flags & XDP_ZEROCOPY;
> force_copy = flags & XDP_COPY;
> @@ -70,18 +105,28 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> if (force_zc && force_copy)
> return -EINVAL;
>
> + rtnl_lock();
> + if (xdp_get_umem_from_qid(dev, queue_id)) {
> + err = -EBUSY;
> + goto rtnl_unlock;
> + }
> +
> + xdp_reg_umem_at_qid(dev, umem, queue_id);
> + umem->dev = dev;
> + umem->queue_id = queue_id;
> if (force_copy)
> - return 0;
> + /* For copy-mode, we are done. */
> + goto rtnl_unlock;
>
> - if (!dev->netdev_ops->ndo_bpf || !dev->netdev_ops->ndo_xsk_async_xmit)
> - return force_zc ? -EOPNOTSUPP : 0; /* fail or fallback */
> + if (!dev->netdev_ops->ndo_bpf ||
> + !dev->netdev_ops->ndo_xsk_async_xmit) {
> + err = -EOPNOTSUPP;
> + goto err_unreg_umem;
> + }
>
> - rtnl_lock();
> err = xdp_umem_query(dev, queue_id);
> - if (err) {
> - err = err < 0 ? -EOPNOTSUPP : -EBUSY;
> - goto err_rtnl_unlock;
> - }
> + if (err)
> + goto err_unreg_umem;
>
> bpf.command = XDP_SETUP_XSK_UMEM;
> bpf.xsk.umem = umem;
> @@ -89,18 +134,20 @@ int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
>
> err = dev->netdev_ops->ndo_bpf(dev, &bpf);
> if (err)
> - goto err_rtnl_unlock;
> + goto err_unreg_umem;
> rtnl_unlock();
>
> dev_hold(dev);
> - umem->dev = dev;
> - umem->queue_id = queue_id;
> umem->zc = true;
> return 0;
>
> -err_rtnl_unlock:
> +err_unreg_umem:
> + xdp_clear_umem_at_qid(dev, queue_id);
You did not clear umem->dev and umem->queue_id,is a problem here?
For example in xdp_umem_clear_dev(), umem->dev is checked.
> + if (!force_zc)
> + err = 0; /* fallback to copy mode */
> +rtnl_unlock:
> rtnl_unlock();
> - return force_zc ? err : 0; /* fail or fallback */
> + return err;
> }
>
> static void xdp_umem_clear_dev(struct xdp_umem *umem)
> @@ -108,7 +155,7 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
> struct netdev_bpf bpf;
> int err;
>
> - if (umem->dev) {
> + if (umem->zc) {
> bpf.command = XDP_SETUP_XSK_UMEM;
> bpf.xsk.umem = NULL;
> bpf.xsk.queue_id = umem->queue_id;
> @@ -121,7 +168,13 @@ static void xdp_umem_clear_dev(struct xdp_umem *umem)
> WARN(1, "failed to disable umem!\n");
>
> dev_put(umem->dev);
> - umem->dev = NULL;
> + umem->zc = false;
> + }
> +
> + if (umem->dev) {
> + rtnl_lock();
> + xdp_clear_umem_at_qid(umem->dev, umem->queue_id);
> + rtnl_unlock();
Previously, umem->dev is reset to NULL. Now, it is left as is. I
assume it is not possible
that this function xdp_umem_clear_dev() is called again for this umem, right?
> }
> }
>
> diff --git a/net/xdp/xdp_umem.h b/net/xdp/xdp_umem.h
> index c8be1ad..2760322 100644
> --- a/net/xdp/xdp_umem.h
> +++ b/net/xdp/xdp_umem.h
> @@ -9,7 +9,7 @@
> #include <net/xdp_sock.h>
>
> int xdp_umem_assign_dev(struct xdp_umem *umem, struct net_device *dev,
> - u32 queue_id, u16 flags);
> + u16 queue_id, u16 flags);
> bool xdp_umem_validate_queues(struct xdp_umem *umem);
> void xdp_get_umem(struct xdp_umem *umem);
> void xdp_put_umem(struct xdp_umem *umem);
> --
> 2.7.4
>
^ permalink raw reply
* Re: [PATCH bpf] tools: bpf: fix license for a compat header file
From: Y Song @ 2018-09-18 17:24 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Alexei Starovoitov, Daniel Borkmann, netdev, oss-drivers
In-Reply-To: <20180918171359.18750-1-jakub.kicinski@netronome.com>
On Tue, Sep 18, 2018 at 10:15 AM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> libc_compat.h is used by libbpf so make sure it's licensed under
> LGPL or BSD license. The license change should be OK, I'm the only
> author of the file.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
Acked-by: Yonghong Song <yhs@fb.com>
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Lendacky, Thomas @ 2018-09-18 23:09 UTC (permalink / raw)
To: David Miller, ahs3@redhat.com
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <20180917.193531.1424852853827678152.davem@davemloft.net>
On 09/17/2018 09:35 PM, David Miller wrote:
> From: Al Stone <ahs3@redhat.com>
> Date: Mon, 17 Sep 2018 17:35:33 -0600
>
>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>> processed = xgene_enet_process_ring(ring, budget);
>>
>> if (processed != budget) {
>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>> +
>> napi_complete_done(napi, processed);
The problem could be that the driver isn't checking the
napi_complete_done() return code. It was changed to return a bool and
the check should be more like:
if ((processed != budget) && napi_complete_done(napi, processed)) {
If it returns false, then the driver will get called for polling again
after having issued enable_irq() and it well then issue the enable_irq()
a second (or more) time without having the matching diable_irq().
Thanks,
Tom
>> - enable_irq(ring->irq);
>> + if (desc && desc->depth > 0)
>> + enable_irq(ring->irq);
>
> I really can't accept a patch that grovels into IRQ layer internals
> to work around a driver's IRQ enable/disable usage problem.
>
> Sorry.
>
^ permalink raw reply
* Re: [RFC 2/5] netlink: set extack error message in nla_validate()
From: Johannes Berg @ 2018-09-18 17:36 UTC (permalink / raw)
To: David Ahern, netdev
In-Reply-To: <f21246a8-6c0a-e99d-96fb-f6e549f78563@gmail.com>
On Tue, 2018-09-18 at 10:18 -0700, David Ahern wrote:
> On 9/18/18 6:12 AM, Johannes Berg wrote:
> > diff --git a/lib/nlattr.c b/lib/nlattr.c
> > index 120ad569e13d..efbd6c1aff29 100644
> > --- a/lib/nlattr.c
> > +++ b/lib/nlattr.c
> > @@ -181,9 +181,13 @@ int nla_validate(const struct nlattr *head, int len, int maxtype,
> > int rem;
> >
> > nla_for_each_attr(nla, head, len, rem) {
> > - int err = validate_nla(nla, maxtype, policy, NULL);
> > + static const char _msg[] = "Attribute failed policy validation";
> > + const char *msg = _msg;
> > + int err = validate_nla(nla, maxtype, policy, &msg);
> >
> > if (err < 0) {
> > + if (extack)
> > + extack->_msg = msg;
> > NL_SET_BAD_ATTR(extack, nla);
> > return err;
> > }
> >
>
> I take it this set is on top of another set - the NLA_REJECT?
Yeah, these two:
https://patchwork.ozlabs.org/project/netdev/list/?series=65982
johannes
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Al Stone @ 2018-09-18 23:15 UTC (permalink / raw)
To: Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <a5adee73-dde7-d00f-e7ef-1f81dc7c813a@amd.com>
On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>
>
> On 09/17/2018 09:35 PM, David Miller wrote:
>> From: Al Stone <ahs3@redhat.com>
>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>
>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>> processed = xgene_enet_process_ring(ring, budget);
>>>
>>> if (processed != budget) {
>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>> +
>>> napi_complete_done(napi, processed);
>
> The problem could be that the driver isn't checking the
> napi_complete_done() return code. It was changed to return a bool and
> the check should be more like:
>
> if ((processed != budget) && napi_complete_done(napi, processed)) {
>
> If it returns false, then the driver will get called for polling again
> after having issued enable_irq() and it well then issue the enable_irq()
> a second (or more) time without having the matching diable_irq().
>
> Thanks,
> Tom
Aha, that might be. My apologies -- I play in ACPI but seldom in the network
drivers, so was not fully aware of that change. I can give that a try.
Thanks for the pointer.
--
ciao,
al
-----------------------------------
Al Stone
Software Engineer
Red Hat, Inc.
ahs3@redhat.com
-----------------------------------
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Florian Fainelli @ 2018-09-18 23:21 UTC (permalink / raw)
To: ahs3, Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <4929e93d-508a-4670-9450-66e4fc85be7e@redhat.com>
On 09/18/2018 04:15 PM, Al Stone wrote:
> On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>>
>>
>> On 09/17/2018 09:35 PM, David Miller wrote:
>>> From: Al Stone <ahs3@redhat.com>
>>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>>
>>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>>> processed = xgene_enet_process_ring(ring, budget);
>>>>
>>>> if (processed != budget) {
>>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>>> +
>>>> napi_complete_done(napi, processed);
>>
>> The problem could be that the driver isn't checking the
>> napi_complete_done() return code. It was changed to return a bool and
>> the check should be more like:
>>
>> if ((processed != budget) && napi_complete_done(napi, processed)) {
>>
>> If it returns false, then the driver will get called for polling again
>> after having issued enable_irq() and it well then issue the enable_irq()
>> a second (or more) time without having the matching diable_irq().
>>
>> Thanks,
>> Tom
>
> Aha, that might be. My apologies -- I play in ACPI but seldom in the network
> drivers, so was not fully aware of that change. I can give that a try.
>
> Thanks for the pointer.
FWIW this is being discussed in this thread as well:
https://www.spinics.net/lists/netdev/msg523760.html
there should be an update to drivers that have a ndo_poll_controller()
and don't check napi_complete_done(), though I am not clear who is doing
that yet.
--
Florian
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Alexei Starovoitov @ 2018-09-18 17:51 UTC (permalink / raw)
To: Eric Dumazet, Song Liu
Cc: netdev, Jeff Kirsher, Alexander Duyck, michael.chan@broadcom.com,
Kernel Team
In-Reply-To: <CANn89iLAPvPn2C9J+sSHsQ5EeF0hq47iMiiAfwoQkdZurQiyzA@mail.gmail.com>
On 9/18/18 6:45 AM, Eric Dumazet wrote:
> On Tue, Sep 18, 2018 at 1:41 AM Song Liu <songliubraving@fb.com> wrote:
>>
>> We are debugging this issue that netconsole message triggers pegged softirq
>> (ksoftirqd taking 100% CPU for many seconds). We found this issue in
>> production with both bnxt and ixgbe, on a 4.11 based kernel. This is easily
>> reproducible with ixgbe on 4.11, and latest net/net-next (see [1] for more
>> detail).
>>
>> After debugging for some time, we found that this issue is likely related
>> to 39e6c8208d7b ("net: solve a NAPI race"). After reverting this commit,
>> the steps described in [1] cannot reproduce the issue on ixgbe. Reverting
>> this commit also reduces the chances we hit the issue with bnxt (it still
>> happens with a lower rate).
>>
>> I tried to fix this issue with relaxed variant (or older version) of
>> napi_schedule_prep() in netpoll, just like the one on napi_watchdog().
>> However, my tests do not always go as expected.
>>
>> Please share your comments/suggestions on which direction shall we try
>> to fix this.
>>
>> Thanks in advance!
>> Song
>>
>>
>> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_netdev_msg522328.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=iSaOapj1kxjhGYLgQr0Qd8mQCzVdobmgT1L4JwFvzxs&s=lCEhrz6wQJUUaJOkxFmtOszAgkf3Jh4reX_i1GbI5RI&e=
>
> You have not traced ixgbe to understand why driver hits
> "clean_complete=false" all the time ?
Eric,
I'm looking at commit 39e6c8208d7b and wondering that it's doing
clear_bit(NAPI_STATE_MISSED,..);
for busy_poll_stop(), but not for netpoll.
Can that be an issue?
and then something like below is needed:
diff --git a/net/core/netpoll.c b/net/core/netpoll.c
index 57557a6a950c..a848be6b503c 100644
--- a/net/core/netpoll.c
+++ b/net/core/netpoll.c
@@ -172,6 +172,7 @@ static void poll_one_napi(struct napi_struct *napi)
trace_napi_poll(napi, work, 0);
clear_bit(NAPI_STATE_NPSVC, &napi->state);
+ clear_bit(NAPI_STATE_MISSED, &napi->state);
}
^ permalink raw reply related
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Eric Dumazet @ 2018-09-18 23:25 UTC (permalink / raw)
To: ahs3, Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <4929e93d-508a-4670-9450-66e4fc85be7e@redhat.com>
On 09/18/2018 04:15 PM, Al Stone wrote:
> On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>>
>>
>> On 09/17/2018 09:35 PM, David Miller wrote:
>>> From: Al Stone <ahs3@redhat.com>
>>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>>
>>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>>> processed = xgene_enet_process_ring(ring, budget);
>>>>
>>>> if (processed != budget) {
>>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>>> +
>>>> napi_complete_done(napi, processed);
>>
>> The problem could be that the driver isn't checking the
>> napi_complete_done() return code. It was changed to return a bool and
>> the check should be more like:
>>
>> if ((processed != budget) && napi_complete_done(napi, processed)) {
>>
>> If it returns false, then the driver will get called for polling again
>> after having issued enable_irq() and it well then issue the enable_irq()
>> a second (or more) time without having the matching diable_irq().
>>
>> Thanks,
>> Tom
>
> Aha, that might be. My apologies -- I play in ACPI but seldom in the network
> drivers, so was not fully aware of that change. I can give that a try.
>
> Thanks for the pointer.
>
Yes, please look at commit d7aba644ffdebf756e51e26a2229055211838e89
("amd-xgbe: Enable IRQs only if napi_complete_done() is true")
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Eric Dumazet @ 2018-09-18 23:27 UTC (permalink / raw)
To: Florian Fainelli, ahs3, Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <cdb8a00c-a3bf-72e0-38b2-3c4b2427270a@gmail.com>
On 09/18/2018 04:21 PM, Florian Fainelli wrote:
> On 09/18/2018 04:15 PM, Al Stone wrote:
>> On 09/18/2018 05:09 PM, Lendacky, Thomas wrote:
>>>
>>>
>>> On 09/17/2018 09:35 PM, David Miller wrote:
>>>> From: Al Stone <ahs3@redhat.com>
>>>> Date: Mon, 17 Sep 2018 17:35:33 -0600
>>>>
>>>>> @@ -866,8 +866,11 @@ static int xgene_enet_napi(struct napi_struct *napi, const int budget)
>>>>> processed = xgene_enet_process_ring(ring, budget);
>>>>>
>>>>> if (processed != budget) {
>>>>> + struct irq_desc *desc = irq_to_desc(ring->irq);
>>>>> +
>>>>> napi_complete_done(napi, processed);
>>>
>>> The problem could be that the driver isn't checking the
>>> napi_complete_done() return code. It was changed to return a bool and
>>> the check should be more like:
>>>
>>> if ((processed != budget) && napi_complete_done(napi, processed)) {
>>>
>>> If it returns false, then the driver will get called for polling again
>>> after having issued enable_irq() and it well then issue the enable_irq()
>>> a second (or more) time without having the matching diable_irq().
>>>
>>> Thanks,
>>> Tom
>>
>> Aha, that might be. My apologies -- I play in ACPI but seldom in the network
>> drivers, so was not fully aware of that change. I can give that a try.
>>
>> Thanks for the pointer.
>
> FWIW this is being discussed in this thread as well:
>
> https://www.spinics.net/lists/netdev/msg523760.html
>
> there should be an update to drivers that have a ndo_poll_controller()
> and don't check napi_complete_done(), though I am not clear who is doing
> that yet.
That might be tricky.
I remember one of the napi_complete_done() change had to be reverted,
for some obscure reason.
So clearly, doing a mass update (without being able to test the driver on real hardware) might be risky.
^ permalink raw reply
* Re: [PATCH net-next v5 20/20] net: WireGuard secure network tunnel
From: Andrew Lunn @ 2018-09-18 23:34 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: linux-kernel, netdev, linux-crypto, davem, gregkh
In-Reply-To: <20180918161646.19105-21-Jason@zx2c4.com>
> +#define push_rcu(stack, p, len) ({ \
> + if (rcu_access_pointer(p)) { \
> + BUG_ON(len >= 128); \
> + stack[len++] = rcu_dereference_raw(p); \
> + } \
> + true; \
> + })
> +static void root_free_rcu(struct rcu_head *rcu)
> +{
> + struct allowedips_node *node, *stack[128] = {
> + container_of(rcu, struct allowedips_node, rcu) };
> + unsigned int len = 1;
> +
> + while (len > 0 && (node = stack[--len]) &&
> + push_rcu(stack, node->bit[0], len) &&
> + push_rcu(stack, node->bit[1], len))
> + kfree(node);
> +}
Hi Jason
I see this BUG_ON() is still here. It really needs to be removed. It
does not look like you need to crash the kernel here. Can you add in a
test of len >= 128, do a WARN and then return. I think you then leak
some memory, but i would much prefer that to a crashed machine.
Andrew
^ permalink raw reply
* Re: [bpf-next, v4 1/5] flow_dissector: implements flow dissector BPF hook
From: Eric Dumazet @ 2018-09-18 18:07 UTC (permalink / raw)
To: Petar Penkov, netdev
Cc: davem, ast, daniel, simon.horman, ecree, songliubraving, tom,
Petar Penkov, Willem de Bruijn
In-Reply-To: <20180914144622.16436-2-peterpenkov96@gmail.com>
On 09/14/2018 07:46 AM, Petar Penkov wrote:
> From: Petar Penkov <ppenkov@google.com>
>
> Adds a hook for programs of type BPF_PROG_TYPE_FLOW_DISSECTOR and
> attach type BPF_FLOW_DISSECTOR that is executed in the flow dissector
> path. The BPF program is per-network namespace
...
>
> + rcu_read_lock();
> + attached = skb ? rcu_dereference(dev_net(skb->dev)->flow_dissector_prog)
> + : NULL;
Some skbs have a NULL skb->dev, so we are going to crash here.
(AF_UNIX generates skbs)
> + if (attached) {
> + /* Note that even though the const qualifier is discarded
> + * throughout the execution of the BPF program, all changes(the
> + * control block) are reverted after the BPF program returns.
> + * Therefore, __skb_flow_dissect does not alter the skb.
> + */
^ permalink raw reply
* Re: pegged softirq and NAPI race (?)
From: Eric Dumazet @ 2018-09-18 18:17 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: songliubraving, netdev, Jeff Kirsher, Alexander Duyck,
michael.chan, kernel-team
In-Reply-To: <a946c4d2-5e29-b1d4-3af6-8657caeebc11@fb.com>
On Tue, Sep 18, 2018 at 10:51 AM Alexei Starovoitov <ast@fb.com> wrote:
>
> On 9/18/18 6:45 AM, Eric Dumazet wrote:
> > On Tue, Sep 18, 2018 at 1:41 AM Song Liu <songliubraving@fb.com> wrote:
> >>
> >> We are debugging this issue that netconsole message triggers pegged softirq
> >> (ksoftirqd taking 100% CPU for many seconds). We found this issue in
> >> production with both bnxt and ixgbe, on a 4.11 based kernel. This is easily
> >> reproducible with ixgbe on 4.11, and latest net/net-next (see [1] for more
> >> detail).
> >>
> >> After debugging for some time, we found that this issue is likely related
> >> to 39e6c8208d7b ("net: solve a NAPI race"). After reverting this commit,
> >> the steps described in [1] cannot reproduce the issue on ixgbe. Reverting
> >> this commit also reduces the chances we hit the issue with bnxt (it still
> >> happens with a lower rate).
> >>
> >> I tried to fix this issue with relaxed variant (or older version) of
> >> napi_schedule_prep() in netpoll, just like the one on napi_watchdog().
> >> However, my tests do not always go as expected.
> >>
> >> Please share your comments/suggestions on which direction shall we try
> >> to fix this.
> >>
> >> Thanks in advance!
> >> Song
> >>
> >>
> >> [1] https://urldefense.proofpoint.com/v2/url?u=https-3A__www.spinics.net_lists_netdev_msg522328.html&d=DwIBaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=i6WobKxbeG3slzHSIOxTVtYIJw7qjCE6S0spDTKL-J4&m=iSaOapj1kxjhGYLgQr0Qd8mQCzVdobmgT1L4JwFvzxs&s=lCEhrz6wQJUUaJOkxFmtOszAgkf3Jh4reX_i1GbI5RI&e=
> >
> > You have not traced ixgbe to understand why driver hits
> > "clean_complete=false" all the time ?
>
> Eric,
>
> I'm looking at commit 39e6c8208d7b and wondering that it's doing
> clear_bit(NAPI_STATE_MISSED,..);
> for busy_poll_stop(), but not for netpoll.
> Can that be an issue?
>
> and then something like below is needed:
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 57557a6a950c..a848be6b503c 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -172,6 +172,7 @@ static void poll_one_napi(struct napi_struct *napi)
> trace_napi_poll(napi, work, 0);
>
> clear_bit(NAPI_STATE_NPSVC, &napi->state);
> + clear_bit(NAPI_STATE_MISSED, &napi->state);
> }
NAPI_STATE_MISSED should only be cleared under strict circumstances.
The clear in busy_poll_stop() is an optimization really (as explained
in the comment)
It is cleared when napi_complete_done() is eventually called, but if
ixgbe always handle 64 RX frames in its poll function,
napi_complete_done() will not be called. The bug is in ixgbe,
pretending its poll function should be called forever.
^ permalink raw reply
* Re: [PATCH] net: apm: xgene: force XGene enet driver to re-balance IRQ usage
From: Eric Dumazet @ 2018-09-18 23:56 UTC (permalink / raw)
To: Florian Fainelli, ahs3, Lendacky, Thomas, David Miller
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
isubramanian@apm.com, kchudgar@apm.com, qnguyen@apm.com
In-Reply-To: <c32598b4-3f80-f83f-9b86-1c168752d1e2@gmail.com>
On 09/18/2018 04:27 PM, Eric Dumazet wrote:
>
> I remember one of the napi_complete_done() change had to be reverted,
> for some obscure reason.
That was not exactly a revert, :
commit 129c6cda2de2a8ac44fab096152469999b727faf
Author: Eric Dumazet <edumazet@google.com>
Date: Mon Sep 18 13:03:43 2017 -0700
8139too: revisit napi_complete_done() usage
It seems we have to be more careful in napi_complete_done()
use. This patch is not a revert, as it seems we can
avoid bug that Ville reported by moving the napi_complete_done()
test in the spinlock section.
Many thanks to Ville for detective work and all tests.
Fixes: 617f01211baf ("8139too: use napi_complete_done()")
Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Tested-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: David S. Miller <davem@davemloft.net>
^ 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