* Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations
From: Philippe Ombredanne @ 2017-12-07 23:00 UTC (permalink / raw)
To: Roman Gushchin
Cc: Jakub Kicinski, netdev, LKML, kernel-team, ast, daniel, kafai,
Quentin Monnet, David Ahern
In-Reply-To: <20171207142306.285be14c@cakuba.netronome.com>
Roman,
On Thu, Dec 7, 2017 at 11:23 PM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote:
>> This patch adds basic cgroup bpf operations to bpftool:
>> cgroup list, attach and detach commands.
>>
>> Usage is described in the corresponding man pages,
>> and examples are provided.
>>
>> Syntax:
>> $ bpftool cgroup list CGROUP
>> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
>> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
>>
>> Signed-off-by: Roman Gushchin <guro@fb.com>
>
> Looks good, a few very minor nits/questions below.
>
>> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
>> new file mode 100644
>> index 000000000000..88d67f74313f
>> --- /dev/null
>> +++ b/tools/bpf/bpftool/cgroup.c
>> @@ -0,0 +1,305 @@
>> +/*
>> + * Copyright (C) 2017 Facebook
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * as published by the Free Software Foundation; either version
>> + * 2 of the License, or (at your option) any later version.
>> + *
>> + * Author: Roman Gushchin <guro@fb.com>
>> + */
Have you considered using the new SPDX ids instead?
e.g. may be something like:
// SPDX-License-Identifier: GPL-2.0+
// Copyright (C) 2017 Facebook
// Author: Roman Gushchin <guro@fb.com>
Don't you love it with less boilerplate and a better code/comments ratio?
BTW the comment style may surprise you here: this is a suggestion, but
not just. Check the posts from Linus on this topic and Thomas's doc
patches for the rationale.
Thank you for your kind consideration!
--
Cordially
Philippe Ombredanne
^ permalink raw reply
* [PATCH net-next] net: stmmac: fix broken dma_interrupt handling for multi-queues
From: Niklas Cassel @ 2017-12-07 22:56 UTC (permalink / raw)
To: Joao.Pinto, Giuseppe Cavallaro, Alexandre Torgue
Cc: Niklas Cassel, netdev, linux-kernel
There is nothing that says that number of TX queues == number of RX
queues. E.g. the ARTPEC-6 SoC has 2 TX queues and 1 RX queue.
This code is obviously wrong:
for (chan = 0; chan < tx_channel_count; chan++) {
struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
priv->rx_queue has size MTL_MAX_RX_QUEUES, so this will send an
uninitialized napi_struct to __napi_schedule(), causing us to
crash in net_rx_action(), because napi_struct->poll is zero.
[12846.759880] Unable to handle kernel NULL pointer dereference at virtual address 00000000
[12846.768014] pgd = (ptrval)
[12846.770742] [00000000] *pgd=39ec7831, *pte=00000000, *ppte=00000000
[12846.777023] Internal error: Oops: 80000007 [#1] PREEMPT SMP ARM
[12846.782942] Modules linked in:
[12846.785998] CPU: 0 PID: 161 Comm: dropbear Not tainted 4.15.0-rc2-00285-gf5fb5f2f39a7 #36
[12846.794177] Hardware name: Axis ARTPEC-6 Platform
[12846.798879] task: (ptrval) task.stack: (ptrval)
[12846.803407] PC is at 0x0
[12846.805942] LR is at net_rx_action+0x274/0x43c
[12846.810383] pc : [<00000000>] lr : [<80bff064>] psr: 200e0113
[12846.816648] sp : b90d9ae8 ip : b90d9ae8 fp : b90d9b44
[12846.821871] r10: 00000008 r9 : 0013250e r8 : 00000100
[12846.827094] r7 : 0000012c r6 : 00000000 r5 : 00000001 r4 : bac84900
[12846.833619] r3 : 00000000 r2 : b90d9b08 r1 : 00000000 r0 : bac84900
Since each DMA channel can be used for rx and tx simultaneously,
the current code should probably be rewritten so that napi_struct is
embedded in a new struct stmmac_channel.
That way, stmmac_poll() can call stmmac_tx_clean() on just the tx queue
where we got the IRQ, instead of looping through all tx queues.
This is also how the xgbe driver does it (another driver for this IP).
Fixes: c22a3f48ef99 ("net: stmmac: adding multiple napi mechanism")
Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
---
drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 54 +++++++++++++++++++----
1 file changed, 46 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index d7250539d0bd..c52a9963c19d 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -1997,22 +1997,60 @@ static void stmmac_set_dma_operation_mode(struct stmmac_priv *priv, u32 txmode,
static void stmmac_dma_interrupt(struct stmmac_priv *priv)
{
u32 tx_channel_count = priv->plat->tx_queues_to_use;
- int status;
+ u32 rx_channel_count = priv->plat->rx_queues_to_use;
+ u32 channels_to_check = tx_channel_count > rx_channel_count ?
+ tx_channel_count : rx_channel_count;
u32 chan;
+ bool poll_scheduled = false;
+ int status[channels_to_check];
+
+ /* Each DMA channel can be used for rx and tx simultaneously, yet
+ * napi_struct is embedded in struct stmmac_rx_queue rather than in a
+ * stmmac_channel struct.
+ * Because of this, stmmac_poll currently checks (and possibly wakes)
+ * all tx queues rather than just a single tx queue.
+ */
+ for (chan = 0; chan < channels_to_check; chan++)
+ status[chan] = priv->hw->dma->dma_interrupt(priv->ioaddr,
+ &priv->xstats,
+ chan);
- for (chan = 0; chan < tx_channel_count; chan++) {
- struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
+ for (chan = 0; chan < rx_channel_count; chan++) {
+ if (likely(status[chan] & handle_rx)) {
+ struct stmmac_rx_queue *rx_q = &priv->rx_queue[chan];
- status = priv->hw->dma->dma_interrupt(priv->ioaddr,
- &priv->xstats, chan);
- if (likely((status & handle_rx)) || (status & handle_tx)) {
if (likely(napi_schedule_prep(&rx_q->napi))) {
stmmac_disable_dma_irq(priv, chan);
__napi_schedule(&rx_q->napi);
+ poll_scheduled = true;
+ }
+ }
+ }
+
+ /* If we scheduled poll, we already know that tx queues will be checked.
+ * If we didn't schedule poll, see if any DMA channel (used by tx) has a
+ * completed transmission, if so, call stmmac_poll (once).
+ */
+ if (!poll_scheduled) {
+ for (chan = 0; chan < tx_channel_count; chan++) {
+ if (status[chan] & handle_tx) {
+ /* It doesn't matter what rx queue we choose
+ * here. We use 0 since it always exists.
+ */
+ struct stmmac_rx_queue *rx_q =
+ &priv->rx_queue[0];
+
+ if (likely(napi_schedule_prep(&rx_q->napi))) {
+ stmmac_disable_dma_irq(priv, chan);
+ __napi_schedule(&rx_q->napi);
+ }
+ break;
}
}
+ }
- if (unlikely(status & tx_hard_error_bump_tc)) {
+ for (chan = 0; chan < tx_channel_count; chan++) {
+ if (unlikely(status[chan] & tx_hard_error_bump_tc)) {
/* Try to bump up the dma threshold on this failure */
if (unlikely(priv->xstats.threshold != SF_DMA_MODE) &&
(tc <= 256)) {
@@ -2029,7 +2067,7 @@ static void stmmac_dma_interrupt(struct stmmac_priv *priv)
chan);
priv->xstats.threshold = tc;
}
- } else if (unlikely(status == tx_hard_error)) {
+ } else if (unlikely(status[chan] == tx_hard_error)) {
stmmac_tx_err(priv, chan);
}
}
--
2.14.2
^ permalink raw reply related
* Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO_HW.
From: Alexander Duyck @ 2017-12-07 22:43 UTC (permalink / raw)
To: Michael Chan
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <CACKFLimOTo9ZQG1uQDgAKjxBX_YkpKp5D1c0j=pfCS09MoPYMg@mail.gmail.com>
On Thu, Dec 7, 2017 at 2:08 PM, Michael Chan <michael.chan@broadcom.com> wrote:
> On Thu, Dec 7, 2017 at 1:35 PM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Thu, Dec 7, 2017 at 10:44 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>> On Thu, Dec 7, 2017 at 10:13 AM, Alexander Duyck
>>> <alexander.duyck@gmail.com> wrote:
>>>> On Thu, Dec 7, 2017 at 12:03 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>>>> @@ -7405,6 +7405,23 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>>> features &= ~dev->gso_partial_features;
>>>>> }
>>>>>
>>>>> + if (features & NETIF_F_GRO_HW) {
>>>>> + /* Hardware GRO depends on GRO and RXCSUM. */
>>>>> + if (!(features & NETIF_F_GRO)) {
>>>>> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
>>>>> + features &= ~NETIF_F_GRO_HW;
>>>>> + }
>>>>
>>>> I'm not sure I agree with this part. The hardware offload should not
>>>> be dependent on the software offload.
>>>
>>> As I said before, GRO_HW is basically hardware accelerated GRO. To me,
>>> it makes perfect sense that GRO_HW depends on GRO.
>>>
>>>> If you are concerned with the
>>>> XDP case and such you should probably just look at handling it more
>>>> like how TSO is handled with a group of flags that aggregate GRO, LRO,
>>>> and GRO_HW into a group of features that must be disabled to support
>>>> XDP.
>>>>
>>>>> + if (!(features & NETIF_F_RXCSUM)) {
>>>>> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
>>>>> + features &= ~NETIF_F_GRO_HW;
>>>>> + }
>>>>> + /* Hardware GRO and LRO are mutually exclusive. */
>>>>> + if (features & NETIF_F_LRO) {
>>>>> + netdev_dbg(dev, "Dropping NETIF_F_LRO since GRO_HW is set.\n");
>>>>> + features &= ~NETIF_F_LRO;
>>>>> + }
>>>>
>>>> This is going to be problematic. What if you bond one interface that
>>>> has LRO and one that has GRO_HW? It seems like this is going to cause
>>>> the bond interface to lie and say LRO isn't there when it actually is,
>>>> or worse yet it will force features off when they shouldn't be.
>>>
>>> This is just dropping incompatible features similar to how other
>>> incompatible feature flags are dropped in netdev_fix_features().
>>> Hardware cannot aggregate in both LRO and GRO_HW modes at the same
>>> time. It's one or the other.
>>
>> I'm not saying this is on a single device. The problem is in the case
>> of bonding you have combined 2 devices. We don't want to have an upper
>> device saying LRO isn't there when it is. A bonding setup is supposed
>> to advertise LRO as being enabled if a lower device has it present.
>> You are now making this an either/or thing at the driver level
>> generically when that isn't the case due to things like bridging and
>> bonding. This is why I was arguing that it would be easier to just
>> make this a super-set of LRO with one form that is reversible and one
>> that isn't. As is you are introducing all sorts of regressions that
>> are going to be problematic in any sort of mixed environment.
>
> Are you saying that if LRO is dropped on a upper device (by the new
> code added in this patch), all lower devices will need to drop it too?
>
> If that's what you are saying, it's already taken care of. There is
> netdev_sync_upper_features() and netdev_sync_lower_features() that
> will automatically do that as part of __netdev_update_features().
>
>>
>>> LRO and GRO_HW are different. We may never agree on this but they are
>>> different. If a bond needs to disable LRO, it will be propagated to
>>> all slaves. But each slave can have GRO enabled. If GRO is enabled,
>>> GRO_HW, being a subset of GRO, can be enabled.
>>
>> I get that they are very different. At a driver level it makes sense
>> to say you get one or the other at a physical driver level. The point
>> I was trying to make is that the bond is only able to set one or the
>> other when you might have a mix of devices. On the bonding device you
>> should be able to have both LRO and GRO_HW enabled.
>
> On the bond, you can have LRO enabled and it is propagated to lower
> devices so that lower devices will enable LRO if it is supported. If
> LRO is disabled on the bond (e.g. due to bridging), It will be
> propagated so all lower devices will disable LRO if it is enabled.
>
> GRO/GRO_HW is different and it is not propagated from upper devices to
> lower devices like LRO. Lower devices can have GRO/GRO_HW
> enabled/disabled regardless of upper device. This is no different
> from say RXCSUM. Lower devices can have RXCSUM on/off regardless.
So on the bond device you should be able to have LRO, GRO, and GRO_HW
enabled all at the same time and that means devices below it can have
any combination of those enabled. If I recall enabling it on the lower
device shouldn't be allowed if an upper device has it disabled. If it
is enabled on the upper device it can be either enabled or disabled on
the lower device depending on the state the device prefers.
The GRO and GRO_HW should be propagated just like RXCSUM and
everything else. Basically if I turn it off on the bond all of the
lower devices should have it disabled. Only if it is enabled should
lower devices be allowed to have it either enabled or disabled. The
easiest way to think of it is on the bond the features are like a mask
of what can be enabled/disabled for the lower devices. If a feature is
not present in the bond it should not be present on any of the devices
below it. That is why I am saying you cannot say LRO and GRO_HW are
mutually exclusive for all devices. For physical devices I would say
yes it can be mutually exclusive, but for things like bond it cannot
be since you can have one device doing LRO and one device doing GRO_HW
getting added to a bond and both features should be advertised there.
My concern is if I have a bond with both GRO_HW and LRO enabled on
lower devices, and then disable something like TSO we are going to see
LRO disabled on the bond and propagated to all the lower devices. That
is a bad behavior and shouldn't happen. In addition I would want to be
able to disable GRO_HW from the bond should I encounter a buggy
implementation of the GRO hardware offload at some point in the
future. I don't like it being tied so closely with GRO since all it
takes is one hardware failure and then we are dealing with people
disabling GRO due to reports of it being buggy instead of it being
tied specifically to GRO_HW.
- Alex
^ permalink raw reply
* [PATCH] net: intel: e100: Fix warning of passing zero to 'PTR_ERR'
From: Vasyl Gomonovych @ 2017-12-07 22:39 UTC (permalink / raw)
To: jeffrey.t.kirsher, intel-wired-lan, gomonovych; +Cc: netdev, linux-kernel
fw could be NULL and put into PTR_ERR
also zero in PTR_ERR will be translated into success return statement
Signed-off-by: Vasyl Gomonovych <gomonovych@gmail.com>
---
drivers/net/ethernet/intel/e100.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/e100.c b/drivers/net/ethernet/intel/e100.c
index 4d10270..55e399d 100644
--- a/drivers/net/ethernet/intel/e100.c
+++ b/drivers/net/ethernet/intel/e100.c
@@ -1370,7 +1370,9 @@ static inline int e100_load_ucode_wait(struct nic *nic)
fw = e100_request_firmware(nic);
/* If it's NULL, then no ucode is required */
- if (!fw || IS_ERR(fw))
+ if (!fw)
+ return -EINVAL;
+ else if (IS_ERR(fw))
return PTR_ERR(fw);
if ((err = e100_exec_cb(nic, (void *)fw, e100_setup_ucode)))
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] netfilter: fix clusterip_net_exit build regression
From: Pablo Neira Ayuso @ 2017-12-07 22:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: Jozsef Kadlecsik, Florian Westphal, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Xin Long, Vasily Averin,
netfilter-devel, coreteam, netdev, linux-kernel
In-Reply-To: <20171207132636.259066-1-arnd@arndb.de>
On Thu, Dec 07, 2017 at 02:26:09PM +0100, Arnd Bergmann wrote:
> The added check produces a build error when CONFIG_PROC_FS is
> disabled:
>
> net/ipv4/netfilter/ipt_CLUSTERIP.c: In function 'clusterip_net_exit':
> net/ipv4/netfilter/ipt_CLUSTERIP.c:822:28: error: 'cn' undeclared (first use in this function)
>
> This moves the variable declaration out of the #ifdef to make it
> available to the WARN_ON_ONCE().
Applied, thanks Arnd.
^ permalink raw reply
* Re: [PATCH v2 net-next 4/4] bpftool: implement cgroup bpf operations
From: Jakub Kicinski @ 2017-12-07 22:23 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, linux-kernel, kernel-team, ast, daniel, kafai,
Quentin Monnet, David Ahern
In-Reply-To: <20171207183909.16240-5-guro@fb.com>
On Thu, 7 Dec 2017 18:39:09 +0000, Roman Gushchin wrote:
> This patch adds basic cgroup bpf operations to bpftool:
> cgroup list, attach and detach commands.
>
> Usage is described in the corresponding man pages,
> and examples are provided.
>
> Syntax:
> $ bpftool cgroup list CGROUP
> $ bpftool cgroup attach CGROUP ATTACH_TYPE PROG [ATTACH_FLAGS]
> $ bpftool cgroup detach CGROUP ATTACH_TYPE PROG
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
Looks good, a few very minor nits/questions below.
> diff --git a/tools/bpf/bpftool/cgroup.c b/tools/bpf/bpftool/cgroup.c
> new file mode 100644
> index 000000000000..88d67f74313f
> --- /dev/null
> +++ b/tools/bpf/bpftool/cgroup.c
> @@ -0,0 +1,305 @@
> +/*
> + * Copyright (C) 2017 Facebook
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + *
> + * Author: Roman Gushchin <guro@fb.com>
> + */
> +
> +#include <fcntl.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +
> +#include <bpf.h>
> +
> +#include "main.h"
> +
> +static const char * const attach_type_strings[] = {
> + [BPF_CGROUP_INET_INGRESS] = "ingress",
> + [BPF_CGROUP_INET_EGRESS] = "egress",
> + [BPF_CGROUP_INET_SOCK_CREATE] = "sock_create",
> + [BPF_CGROUP_SOCK_OPS] = "sock_ops",
> + [BPF_CGROUP_DEVICE] = "device",
> + [__MAX_BPF_ATTACH_TYPE] = NULL,
> +};
> +
> +static enum bpf_attach_type parse_attach_type(const char *str)
> +{
> + enum bpf_attach_type type;
> +
> + for (type = 0; type < __MAX_BPF_ATTACH_TYPE; type++) {
> + if (attach_type_strings[type] &&
> + strcmp(str, attach_type_strings[type]) == 0)
Here and for matching flags you use straight strcmp(), not our locally
defined is_prefix(), is this intentional? is_prefix() allows
abbreviations, like in iproute2 commands. E.g. this would work:
# bpftool cg att /sys/fs/cgroup/test.slice/ dev id 1 allow_multi
> + return type;
> + }
> +
> + return __MAX_BPF_ATTACH_TYPE;
> +}
> +static int list_attached_bpf_progs(int cgroup_fd, enum bpf_attach_type type)
> +{
> + __u32 attach_flags;
> + __u32 prog_ids[1024] = {0};
> + __u32 prog_cnt, iter;
> + char *attach_flags_str;
> + int ret;
nit: could you reorder the variables so they're listed longest to
shortest (reverse christmas tree)?
> + prog_cnt = ARRAY_SIZE(prog_ids);
> + ret = bpf_prog_query(cgroup_fd, type, 0, &attach_flags, prog_ids,
> + &prog_cnt);
> + if (ret)
> + return ret;
> +
> + if (prog_cnt == 0)
> + return 0;
> +
> + switch (attach_flags) {
> + case BPF_F_ALLOW_MULTI:
> + attach_flags_str = "allow_multi";
> + break;
> + case BPF_F_ALLOW_OVERRIDE:
> + attach_flags_str = "allow_override";
> + break;
> + case 0:
> + attach_flags_str = "";
> + break;
> + default:
> + attach_flags_str = "unknown";
nit: would it make sense to perhaps print flags in hex format in this
case?
> + }
> +
> + for (iter = 0; iter < prog_cnt; iter++)
> + list_bpf_prog(prog_ids[iter], attach_type_strings[type],
> + attach_flags_str);
> +
> + return 0;
> +}
> +static int do_attach(int argc, char **argv)
> +{
> + int cgroup_fd, prog_fd;
> + enum bpf_attach_type attach_type;
> + int attach_flags = 0;
> + int i;
> + int ret = -1;
> +
> + if (argc < 4) {
> + p_err("too few parameters for cgroup attach\n");
> + goto exit;
> + }
> +
> + cgroup_fd = open(argv[0], O_RDONLY);
> + if (cgroup_fd < 0) {
> + p_err("can't open cgroup %s\n", argv[1]);
> + goto exit;
> + }
> +
> + attach_type = parse_attach_type(argv[1]);
> + if (attach_type == __MAX_BPF_ATTACH_TYPE) {
> + p_err("invalid attach type\n");
> + goto exit_cgroup;
> + }
> +
> + argc -= 2;
> + argv = &argv[2];
> + prog_fd = prog_parse_fd(&argc, &argv);
> + if (prog_fd < 0)
> + goto exit_cgroup;
> +
> + for (i = 0; i < argc; i++) {
> + if (strcmp(argv[i], "allow_multi") == 0) {
> + attach_flags |= BPF_F_ALLOW_MULTI;
> + } else if (strcmp(argv[i], "allow_override") == 0) {
> + attach_flags |= BPF_F_ALLOW_OVERRIDE;
This is the other potential place for is_prefix() I referred to.
That reminds me - would you care to also update Quentin's bash
completions (tools/bpf/bpftool/bash-completion/bpftool)? They
are _very_ nice to have in day to day use!
> + } else {
> + p_err("unknown option: %s\n", argv[i]);
> + goto exit_cgroup;
> + }
> + }
> +
> + if (bpf_prog_attach(prog_fd, cgroup_fd, attach_type, attach_flags)) {
> + p_err("failed to attach program");
> + goto exit_prog;
> + }
> +
> + if (json_output)
> + jsonw_null(json_wtr);
> +
> + ret = 0;
> +
> +exit_prog:
> + close(prog_fd);
> +exit_cgroup:
> + close(cgroup_fd);
> +exit:
> + return ret;
> +}
Otherwise looks really nice, thanks for working on it!
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 06/10] ixgbe: restore offloaded SAs after a reset
From: Shannon Nelson @ 2017-12-07 22:19 UTC (permalink / raw)
To: Alexander Duyck
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <CAKgT0UfDnHnPCM8Px-TCbiB4wBQGN9CskftyF+4V=5g2F72QVw@mail.gmail.com>
On 12/7/2017 1:52 PM, Alexander Duyck wrote:
>
> The reads/writes themselves should be cheap. These kind of things only
> get to be really expensive when you start looking at adding delays in
> between the writes/reads polling on things. As long as we aren't
> waiting milliseconds on things you can write/read thousands of
> registers and not even notice it.
>
> One thing you might look at doing in order to speed some of this up a
> bit would be to also combine updating the Tx SA and Rx SA in your
> clear_hw_tables loop so that you could do them in parallel in your
> loop instead of having to do them in series. Anyway it is just a
> thought. If nothing else you might look at timing the function to see
> how long it actually takes. I suspect it shouldn't be too long since
> the turnaround time on the PCIe bus should be in microseconds so odds
> are reading/writing 35K registers might ovinly add a few milliseconds
> to total reset time.
>
Good ideas - thanks,
sln
^ permalink raw reply
* Re: [PATCH net-next V3] tun: add eBPF based queue selection method
From: Eric Dumazet @ 2017-12-07 22:11 UTC (permalink / raw)
To: Jason Wang, netdev, linux-kernel
Cc: mst, willemdebruijn.kernel, tom, aconole, wexu
In-Reply-To: <1512379883-11887-1-git-send-email-jasowang@redhat.com>
On Mon, 2017-12-04 at 17:31 +0800, Jason Wang wrote:
> This patch introduces an eBPF based queue selection method. With
> this,
> the policy could be offloaded to userspace completely through a new
> ioctl TUNSETSTEERINGEBPF.
Sorry for the delay, I see this patch was merged already.
...
> static void tun_free_netdev(struct net_device *dev)
> {
> struct tun_struct *tun = netdev_priv(dev);
> @@ -1996,6 +2068,9 @@ static void tun_free_netdev(struct net_device
> *dev)
> free_percpu(tun->pcpu_stats);
> tun_flow_uninit(tun);
> security_tun_dev_free_security(tun->security);
> + rtnl_lock();
> + __tun_set_steering_ebpf(tun, NULL);
> + rtnl_unlock();
> }
I am pretty sure tun_free_netdev() (aka ->priv_destructor()) can be
called under RTNL (say from register_netdevice())
So this will dead lock badly ?
^ permalink raw reply
* Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO_HW.
From: Michael Chan @ 2017-12-07 22:08 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <CAKgT0Ue9h8Pdb4OkBiWdL5bSNHPUkbZfBosrtZ4gQOAnuGiwRQ@mail.gmail.com>
On Thu, Dec 7, 2017 at 1:35 PM, Alexander Duyck
<alexander.duyck@gmail.com> wrote:
> On Thu, Dec 7, 2017 at 10:44 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>> On Thu, Dec 7, 2017 at 10:13 AM, Alexander Duyck
>> <alexander.duyck@gmail.com> wrote:
>>> On Thu, Dec 7, 2017 at 12:03 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>>> @@ -7405,6 +7405,23 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>>> features &= ~dev->gso_partial_features;
>>>> }
>>>>
>>>> + if (features & NETIF_F_GRO_HW) {
>>>> + /* Hardware GRO depends on GRO and RXCSUM. */
>>>> + if (!(features & NETIF_F_GRO)) {
>>>> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
>>>> + features &= ~NETIF_F_GRO_HW;
>>>> + }
>>>
>>> I'm not sure I agree with this part. The hardware offload should not
>>> be dependent on the software offload.
>>
>> As I said before, GRO_HW is basically hardware accelerated GRO. To me,
>> it makes perfect sense that GRO_HW depends on GRO.
>>
>>> If you are concerned with the
>>> XDP case and such you should probably just look at handling it more
>>> like how TSO is handled with a group of flags that aggregate GRO, LRO,
>>> and GRO_HW into a group of features that must be disabled to support
>>> XDP.
>>>
>>>> + if (!(features & NETIF_F_RXCSUM)) {
>>>> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
>>>> + features &= ~NETIF_F_GRO_HW;
>>>> + }
>>>> + /* Hardware GRO and LRO are mutually exclusive. */
>>>> + if (features & NETIF_F_LRO) {
>>>> + netdev_dbg(dev, "Dropping NETIF_F_LRO since GRO_HW is set.\n");
>>>> + features &= ~NETIF_F_LRO;
>>>> + }
>>>
>>> This is going to be problematic. What if you bond one interface that
>>> has LRO and one that has GRO_HW? It seems like this is going to cause
>>> the bond interface to lie and say LRO isn't there when it actually is,
>>> or worse yet it will force features off when they shouldn't be.
>>
>> This is just dropping incompatible features similar to how other
>> incompatible feature flags are dropped in netdev_fix_features().
>> Hardware cannot aggregate in both LRO and GRO_HW modes at the same
>> time. It's one or the other.
>
> I'm not saying this is on a single device. The problem is in the case
> of bonding you have combined 2 devices. We don't want to have an upper
> device saying LRO isn't there when it is. A bonding setup is supposed
> to advertise LRO as being enabled if a lower device has it present.
> You are now making this an either/or thing at the driver level
> generically when that isn't the case due to things like bridging and
> bonding. This is why I was arguing that it would be easier to just
> make this a super-set of LRO with one form that is reversible and one
> that isn't. As is you are introducing all sorts of regressions that
> are going to be problematic in any sort of mixed environment.
Are you saying that if LRO is dropped on a upper device (by the new
code added in this patch), all lower devices will need to drop it too?
If that's what you are saying, it's already taken care of. There is
netdev_sync_upper_features() and netdev_sync_lower_features() that
will automatically do that as part of __netdev_update_features().
>
>> LRO and GRO_HW are different. We may never agree on this but they are
>> different. If a bond needs to disable LRO, it will be propagated to
>> all slaves. But each slave can have GRO enabled. If GRO is enabled,
>> GRO_HW, being a subset of GRO, can be enabled.
>
> I get that they are very different. At a driver level it makes sense
> to say you get one or the other at a physical driver level. The point
> I was trying to make is that the bond is only able to set one or the
> other when you might have a mix of devices. On the bonding device you
> should be able to have both LRO and GRO_HW enabled.
On the bond, you can have LRO enabled and it is propagated to lower
devices so that lower devices will enable LRO if it is supported. If
LRO is disabled on the bond (e.g. due to bridging), It will be
propagated so all lower devices will disable LRO if it is enabled.
GRO/GRO_HW is different and it is not propagated from upper devices to
lower devices like LRO. Lower devices can have GRO/GRO_HW
enabled/disabled regardless of upper device. This is no different
from say RXCSUM. Lower devices can have RXCSUM on/off regardless.
^ permalink raw reply
* Re: [PATCH v2 net-next 3/4] bpftool: implement prog load command
From: Jakub Kicinski @ 2017-12-07 21:55 UTC (permalink / raw)
To: Roman Gushchin
Cc: netdev, linux-kernel, kernel-team, ast, daniel, kafai,
Quentin Monnet, David Ahern
In-Reply-To: <20171207183909.16240-4-guro@fb.com>
On Thu, 7 Dec 2017 18:39:08 +0000, Roman Gushchin wrote:
> Add the prog load command to load a bpf program from a specified
> binary file and pin it to bpffs.
>
> Usage description and examples are given in the corresponding man
> page.
>
> Syntax:
> $ bpftool prog load SOURCE_FILE FILE
>
> FILE is a non-existing file on bpffs.
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
Reviewed-by: Jakub Kicinski <jakub.kicinski@netronome.com>
The only reservation I have is that SRC may confuse people, would it
make sense to call the compiled code OBJ? I'm afraid someone may try
to load a C source...
^ permalink raw reply
* Re: [Intel-wired-lan] [next-queue 06/10] ixgbe: restore offloaded SAs after a reset
From: Alexander Duyck @ 2017-12-07 21:52 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, Jeff Kirsher, Steffen Klassert, Sowmini Varadhan,
Netdev
In-Reply-To: <f8f1d753-3f29-6aa7-326c-423f38036877@oracle.com>
On Thu, Dec 7, 2017 at 10:47 AM, Shannon Nelson
<shannon.nelson@oracle.com> wrote:
> On 12/7/2017 9:16 AM, Alexander Duyck wrote:
>>
>> On Wed, Dec 6, 2017 at 9:43 PM, Shannon Nelson
>> <shannon.nelson@oracle.com> wrote:
>>>
>>> On 12/5/2017 9:30 AM, Alexander Duyck wrote:
>>>>
>>>>
>>>> On Mon, Dec 4, 2017 at 9:35 PM, Shannon Nelson
>>>> <shannon.nelson@oracle.com> wrote:
>>>>>
>>>>>
>>>>> On a chip reset most of the table contents are lost, so must be
>
>
> <snip>
>
>>>>> + /* reload the IP addrs */
>>>>> + for (i = 0; i < IXGBE_IPSEC_MAX_RX_IP_COUNT; i++) {
>>>>> + struct rx_ip_sa *ipsa = &ipsec->ip_tbl[i];
>>>>> +
>>>>> + if (ipsa->used)
>>>>> + ixgbe_ipsec_set_rx_ip(hw, i, ipsa->ipaddr);
>>>>> + else
>>>>> + ixgbe_ipsec_set_rx_ip(hw, i, zbuf);
>>>>
>>>>
>>>>
>>>> If we are doing a restore do we actually need to write the zero
>>>> values? If we did a reset I thought you had a function that was going
>>>> through and zeroing everything out. If so this now becomes redundant.
>>>
>>>
>>>
>>> Currently ixgbe_ipsec_clear_hw_tables() only gets run at at probe. It
>>> should probably get run at remove as well. Doing this is a bit of safety
>>> paranoia, and making sure the CAM memory structures that don't get
>>> cleared
>>> on reset have exactly what I expect in them.
>>
>>
>> You might just move ixgbe_ipsec_clear_hw_tables into the rest logic
>> itself. Then it covers all cases where you would be resetting the
>> hardware and expecting a consistent state. It will mean writing some
>> registers twice during the reset but it is probably better just to
>> make certain everything stays in a known good state after a reset.
>
>
> If it is a small number, e.g. 10 or 20, then you may be right. However,
> given we have table space for 2k different SAs, at 6 writes per Tx SA and 10
> writes per Rx SA, plus 128 IP address with 4 writes each, we are already
> looking at 17K writes already to be sure the tables are clean.
>
> Unfortunately, I don't really know what a "typical" case will be, so I don't
> know how many SA we may be offloading at any one time. But in a busy cloud
> support server, we might have nearly full tables. If we do the full clean
> first, then have to fill all the tables, we're now looking at up to 35k
> writes slowing down the reset process.
>
> I'd rather keep it to the constant 17K writes for now, and look later at
> using the VALID bit in the IPSRXMOD to see if we can at least cut down on
> the Rx writes.
>
> sln
The reads/writes themselves should be cheap. These kind of things only
get to be really expensive when you start looking at adding delays in
between the writes/reads polling on things. As long as we aren't
waiting milliseconds on things you can write/read thousands of
registers and not even notice it.
One thing you might look at doing in order to speed some of this up a
bit would be to also combine updating the Tx SA and Rx SA in your
clear_hw_tables loop so that you could do them in parallel in your
loop instead of having to do them in series. Anyway it is just a
thought. If nothing else you might look at timing the function to see
how long it actually takes. I suspect it shouldn't be too long since
the turnaround time on the PCIe bus should be in microseconds so odds
are reading/writing 35K registers might ovinly add a few milliseconds
to total reset time.
^ permalink raw reply
* Re: [PATCH 2/3] bluetooth: hci_ll: add constant for vendor-specific command
From: Marcel Holtmann @ 2017-12-07 21:43 UTC (permalink / raw)
To: David Lechner
Cc: devicetree, open list:BLUETOOTH DRIVERS, Rob Herring,
Mark Rutland, Gustavo F. Padovan, Johan Hedberg,
Network Development, linux-kernel
In-Reply-To: <55fff19a-c30f-88f0-afb2-6c0b9bad25a7@lechnology.com>
Hi David,
>>> This adds a #define for the vendor-specific HCI command to set the
>>> baudrate instead of using the bare 0xff36 multiple times.
>>>
>>> Signed-off-by: David Lechner <david@lechnology.com>
>>> ---
>>> drivers/bluetooth/hci_ll.c | 10 ++++++++--
>>> 1 file changed, 8 insertions(+), 2 deletions(-)
>> patch has been applied to bluetooth-next tree.
>
> I am new to the bluetooth tree, so just to be sure... does this mean I should not include this patch in v2 of this series since this patch has been applied?
please don’t include already applied patches. Base everything against bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH net] tcp: invalidate rate samples during SACK reneging
From: Yousuk Seung @ 2017-12-07 21:41 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Yousuk Seung, Neal Cardwell, Yuchung Cheng
Mark tcp_sock during a SACK reneging event and invalidate rate samples
while marked. Such rate samples may overestimate bw by including packets
that were SACKed before reneging.
< ack 6001 win 10000 sack 7001:38001
< ack 7001 win 0 sack 8001:38001 // Reneg detected
> seq 7001:8001 // RTO, SACK cleared.
< ack 38001 win 10000
In above example the rate sample taken after the last ack will count
7001-38001 as delivered while the actual delivery rate likely could
be much lower i.e. 7001-8001.
This patch adds a new field tcp_sock.sack_reneg and marks it when we
declare SACK reneging and entering TCP_CA_Loss, and unmarks it after
the last rate sample was taken before moving back to TCP_CA_Open. This
patch also invalidates rate samples taken while tcp_sock.is_sack_reneg
is set.
Fixes: b9f64820fb22 ("tcp: track data delivery rate for a TCP connection")
Signed-off-by: Yousuk Seung <ysseung@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Eric Dumazet <edumazet@google.com>
Acked-by: Priyaranjan Jha <priyarjha@google.com>
---
include/linux/tcp.h | 3 ++-
include/net/tcp.h | 2 +-
net/ipv4/tcp.c | 1 +
net/ipv4/tcp_input.c | 10 ++++++++--
net/ipv4/tcp_rate.c | 10 +++++++---
5 files changed, 19 insertions(+), 7 deletions(-)
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index df5d97a85e1a..ca4a6361389b 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -224,7 +224,8 @@ struct tcp_sock {
rate_app_limited:1, /* rate_{delivered,interval_us} limited? */
fastopen_connect:1, /* FASTOPEN_CONNECT sockopt */
fastopen_no_cookie:1, /* Allow send/recv SYN+data without a cookie */
- unused:3;
+ is_sack_reneg:1, /* in recovery from loss with SACK reneg? */
+ unused:2;
u8 nonagle : 4,/* Disable Nagle algorithm? */
thin_lto : 1,/* Use linear timeouts for thin streams */
unused1 : 1,
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 6998707e81f3..6da880d2f022 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -1055,7 +1055,7 @@ void tcp_rate_skb_sent(struct sock *sk, struct sk_buff *skb);
void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb,
struct rate_sample *rs);
void tcp_rate_gen(struct sock *sk, u32 delivered, u32 lost,
- struct rate_sample *rs);
+ bool is_sack_reneg, struct rate_sample *rs);
void tcp_rate_check_app_limited(struct sock *sk);
/* These functions determine how the current flow behaves in respect of SACK
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index bf97317e6c97..f08eebe60446 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2412,6 +2412,7 @@ int tcp_disconnect(struct sock *sk, int flags)
tp->snd_cwnd_cnt = 0;
tp->window_clamp = 0;
tcp_set_ca_state(sk, TCP_CA_Open);
+ tp->is_sack_reneg = 0;
tcp_clear_retrans(tp);
inet_csk_delack_init(sk);
/* Initialize rcv_mss to TCP_MIN_MSS to avoid division by 0
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index 734cfc8ff76e..7b9e8b99b28d 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -1941,6 +1941,8 @@ void tcp_enter_loss(struct sock *sk)
if (is_reneg) {
NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPSACKRENEGING);
tp->sacked_out = 0;
+ /* Mark SACK reneging until we recover from this loss event. */
+ tp->is_sack_reneg = 1;
}
tcp_clear_all_retrans_hints(tp);
@@ -2364,6 +2366,7 @@ static bool tcp_try_undo_recovery(struct sock *sk)
return true;
}
tcp_set_ca_state(sk, TCP_CA_Open);
+ tp->is_sack_reneg = 0;
return false;
}
@@ -2397,8 +2400,10 @@ static bool tcp_try_undo_loss(struct sock *sk, bool frto_undo)
NET_INC_STATS(sock_net(sk),
LINUX_MIB_TCPSPURIOUSRTOS);
inet_csk(sk)->icsk_retransmits = 0;
- if (frto_undo || tcp_is_sack(tp))
+ if (frto_undo || tcp_is_sack(tp)) {
tcp_set_ca_state(sk, TCP_CA_Open);
+ tp->is_sack_reneg = 0;
+ }
return true;
}
return false;
@@ -3495,6 +3500,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
struct tcp_sacktag_state sack_state;
struct rate_sample rs = { .prior_delivered = 0 };
u32 prior_snd_una = tp->snd_una;
+ bool is_sack_reneg = tp->is_sack_reneg;
u32 ack_seq = TCP_SKB_CB(skb)->seq;
u32 ack = TCP_SKB_CB(skb)->ack_seq;
bool is_dupack = false;
@@ -3611,7 +3617,7 @@ static int tcp_ack(struct sock *sk, const struct sk_buff *skb, int flag)
delivered = tp->delivered - delivered; /* freshly ACKed or SACKed */
lost = tp->lost - lost; /* freshly marked lost */
- tcp_rate_gen(sk, delivered, lost, sack_state.rate);
+ tcp_rate_gen(sk, delivered, lost, is_sack_reneg, sack_state.rate);
tcp_cong_control(sk, ack, delivered, flag, sack_state.rate);
tcp_xmit_recovery(sk, rexmit);
return 1;
diff --git a/net/ipv4/tcp_rate.c b/net/ipv4/tcp_rate.c
index 3330a370d306..c61240e43923 100644
--- a/net/ipv4/tcp_rate.c
+++ b/net/ipv4/tcp_rate.c
@@ -106,7 +106,7 @@ void tcp_rate_skb_delivered(struct sock *sk, struct sk_buff *skb,
/* Update the connection delivery information and generate a rate sample. */
void tcp_rate_gen(struct sock *sk, u32 delivered, u32 lost,
- struct rate_sample *rs)
+ bool is_sack_reneg, struct rate_sample *rs)
{
struct tcp_sock *tp = tcp_sk(sk);
u32 snd_us, ack_us;
@@ -124,8 +124,12 @@ void tcp_rate_gen(struct sock *sk, u32 delivered, u32 lost,
rs->acked_sacked = delivered; /* freshly ACKed or SACKed */
rs->losses = lost; /* freshly marked lost */
- /* Return an invalid sample if no timing information is available. */
- if (!rs->prior_mstamp) {
+ /* Return an invalid sample if no timing information is available or
+ * in recovery from loss with SACK reneging. Rate samples taken during
+ * a SACK reneging event may overestimate bw by including packets that
+ * were SACKed before the reneg.
+ */
+ if (!rs->prior_mstamp || is_sack_reneg) {
rs->delivered = -1;
rs->interval_us = -1;
return;
--
2.15.1.424.g9478a66081-goog
^ permalink raw reply related
* Re: [PATCH net-next v2 1/5] net: Introduce NETIF_F_GRO_HW.
From: Alexander Duyck @ 2017-12-07 21:35 UTC (permalink / raw)
To: Michael Chan
Cc: David Miller, Netdev, Andrew Gospodarek, Ariel Elior,
everest-linux-l2
In-Reply-To: <CACKFLimmyQEDs8Ti3SqVD2Tq2uEjQv7En+BORgQSj8tqh0mx3A@mail.gmail.com>
On Thu, Dec 7, 2017 at 10:44 AM, Michael Chan <michael.chan@broadcom.com> wrote:
> On Thu, Dec 7, 2017 at 10:13 AM, Alexander Duyck
> <alexander.duyck@gmail.com> wrote:
>> On Thu, Dec 7, 2017 at 12:03 AM, Michael Chan <michael.chan@broadcom.com> wrote:
>>> @@ -7405,6 +7405,23 @@ static netdev_features_t netdev_fix_features(struct net_device *dev,
>>> features &= ~dev->gso_partial_features;
>>> }
>>>
>>> + if (features & NETIF_F_GRO_HW) {
>>> + /* Hardware GRO depends on GRO and RXCSUM. */
>>> + if (!(features & NETIF_F_GRO)) {
>>> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no GRO feature.\n");
>>> + features &= ~NETIF_F_GRO_HW;
>>> + }
>>
>> I'm not sure I agree with this part. The hardware offload should not
>> be dependent on the software offload.
>
> As I said before, GRO_HW is basically hardware accelerated GRO. To me,
> it makes perfect sense that GRO_HW depends on GRO.
>
>> If you are concerned with the
>> XDP case and such you should probably just look at handling it more
>> like how TSO is handled with a group of flags that aggregate GRO, LRO,
>> and GRO_HW into a group of features that must be disabled to support
>> XDP.
>>
>>> + if (!(features & NETIF_F_RXCSUM)) {
>>> + netdev_dbg(dev, "Dropping NETIF_F_GSO_HW since no RXCSUM feature.\n");
>>> + features &= ~NETIF_F_GRO_HW;
>>> + }
>>> + /* Hardware GRO and LRO are mutually exclusive. */
>>> + if (features & NETIF_F_LRO) {
>>> + netdev_dbg(dev, "Dropping NETIF_F_LRO since GRO_HW is set.\n");
>>> + features &= ~NETIF_F_LRO;
>>> + }
>>
>> This is going to be problematic. What if you bond one interface that
>> has LRO and one that has GRO_HW? It seems like this is going to cause
>> the bond interface to lie and say LRO isn't there when it actually is,
>> or worse yet it will force features off when they shouldn't be.
>
> This is just dropping incompatible features similar to how other
> incompatible feature flags are dropped in netdev_fix_features().
> Hardware cannot aggregate in both LRO and GRO_HW modes at the same
> time. It's one or the other.
I'm not saying this is on a single device. The problem is in the case
of bonding you have combined 2 devices. We don't want to have an upper
device saying LRO isn't there when it is. A bonding setup is supposed
to advertise LRO as being enabled if a lower device has it present.
You are now making this an either/or thing at the driver level
generically when that isn't the case due to things like bridging and
bonding. This is why I was arguing that it would be easier to just
make this a super-set of LRO with one form that is reversible and one
that isn't. As is you are introducing all sorts of regressions that
are going to be problematic in any sort of mixed environment.
> LRO and GRO_HW are different. We may never agree on this but they are
> different. If a bond needs to disable LRO, it will be propagated to
> all slaves. But each slave can have GRO enabled. If GRO is enabled,
> GRO_HW, being a subset of GRO, can be enabled.
I get that they are very different. At a driver level it makes sense
to say you get one or the other at a physical driver level. The point
I was trying to make is that the bond is only able to set one or the
other when you might have a mix of devices. On the bonding device you
should be able to have both LRO and GRO_HW enabled.
^ permalink raw reply
* Re: [PATCH v5 2/2] sock: Move the socket inuse to namespace.
From: Cong Wang @ 2017-12-07 21:28 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tonghao Zhang, David Miller, Eric Dumazet, Willem de Bruijn,
Linux Kernel Network Developers
In-Reply-To: <1512667208.25033.13.camel@gmail.com>
On Thu, Dec 7, 2017 at 9:20 AM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Thu, 2017-12-07 at 08:45 -0800, Tonghao Zhang wrote:
>> In some case, we want to know how many sockets are in use in
>> different _net_ namespaces. It's a key resource metric.
>>
>
> ...
>
>> +static void sock_inuse_add(struct net *net, int val)
>> +{
>> + if (net->core.prot_inuse)
>> + this_cpu_add(*net->core.sock_inuse, val);
>> +}
>
> This is very confusing.
>
> Why testing net->core.prot_inuse for NULL is needed at all ?
>
> Why not testing net->core.sock_inuse instead ?
I bet that is copy-n-paste error given that sock_inuse_exit_net()
has a similar typo.
^ permalink raw reply
* Re: [PATCH 2/3] bluetooth: hci_ll: add constant for vendor-specific command
From: David Lechner @ 2017-12-07 21:29 UTC (permalink / raw)
To: Marcel Holtmann
Cc: devicetree, open list:BLUETOOTH DRIVERS, Rob Herring,
Mark Rutland, Gustavo F. Padovan, Johan Hedberg, netdev,
linux-kernel
In-Reply-To: <5C65FB74-D7FD-4653-993A-5A68AD844A41@holtmann.org>
On 12/04/2017 12:23 AM, Marcel Holtmann wrote:
> Hi David,
>
>> This adds a #define for the vendor-specific HCI command to set the
>> baudrate instead of using the bare 0xff36 multiple times.
>>
>> Signed-off-by: David Lechner <david@lechnology.com>
>> ---
>> drivers/bluetooth/hci_ll.c | 10 ++++++++--
>> 1 file changed, 8 insertions(+), 2 deletions(-)
>
> patch has been applied to bluetooth-next tree.
>
I am new to the bluetooth tree, so just to be sure... does this mean I
should not include this patch in v2 of this series since this patch has
been applied?
^ permalink raw reply
* Re: [PATCH net-next] netdevsim: check return value of debugfs_create_dir
From: Jakub Kicinski @ 2017-12-07 21:20 UTC (permalink / raw)
To: Prashant Bhole; +Cc: 'David S . Miller', netdev
In-Reply-To: <001001d36f39$4a81b5b0$df852110$@lab.ntt.co.jp>
On Thu, 7 Dec 2017 17:56:37 +0900, Prashant Bhole wrote:
> > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> >
> > On Thu, 7 Dec 2017 13:10:39 +0900, Prashant Bhole wrote:
> > > > From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > > >
> > > > On Thu, 7 Dec 2017 10:02:13 +0900, Prashant Bhole wrote:
> > > > > - Handled debugfs_create_dir failure in nsim_init()
> > > > > - Fixed return value of nsim_module_init() when debugfs_create_dir
> > > > > fails
> > > > >
> > > > > Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
> > > >
> > > > Why? Failing to expose the state via DebugFS is not fatal to the driver.
> > >
> > > Ok, my intention was to handle the return code properly, which is not
> > > needed as per your comment.
> > > Shall I remove the existing handling in nsim_module_init() in separate
> > > patch?
> >
> > I was going back and forth on the error handling quite a bit writing that code. In
> > the end I decided to leave the module_init check and check for bpf prog
> > directory. Former one is mostly useful to make sure the is no duplicate directory
> > with the same name, the latter to limit possible false positive in the selftest..
>
> Ok. Currently return value is checked with IS_ERR(). But when Debug FS is
> enabled, debugfs_create_dir will never return error value. It returns either
> NULL or a valid pointer. Shall I replace IS_ERR with NULL check or
> IS_ERR_OR_NULL check?
Sure.
^ permalink raw reply
* Re: [PATCH net-next 1/1] net: dsa: microchip: Add Microchip KSZ8895 DSA driver
From: Pavel Machek @ 2017-12-07 21:04 UTC (permalink / raw)
To: Tristram.Ha
Cc: andrew, f.fainelli, muvarov, nathan.leigh.conrad, vivien.didelot,
UNGLinuxDriver, netdev
In-Reply-To: <93AF473E2DA327428DE3D46B72B1E9FD4113DCE0@CHN-SV-EXMX02.mchp-main.com>
[-- Attachment #1: Type: text/plain, Size: 5138 bytes --]
Hi!
> > I went back to my version of dsa patches, and test above works as
> > expected.
>
> Sorry to be this late for the reply. I finally got hold of a KSZ8895 board that
> works with my SoC board to confirm the network communication.
>
> As expected the KSZ8895 board works correctly as the chip uses the same
> tail tagging feature in KSZ8795, and I did verify that board is
> working.
Ok, let me retry:
> One thing to debug this problem is to dump the MIB counters. Use the ethtool
> utility to show MIB counters of both ports:
>
> ethtool -S lan3
> ethtool -S eth0
>
> Assuming eth0 is the MAC controller that drives the switch, the receive counters of
> the host port of the switch should match the transmit counters of
> lan3, and vice versa.
Hmm. I'm getting some "interesting" results from mii-tool:
root@miro:~# mii-tool lan3
lan3: negotiated 1000baseT-HD flow-control, link ok
But IIRC the switch is 100mbit? And dmesg does get it right. Its just
mii-tool that is confused.
Link detection seems to work
root@miro:/sys/bus/spi/devices/spi2.0# mii-tool lan1
lan1: negotiated 1000baseT-HD flow-control, link ok
root@miro:/sys/bus/spi/devices/spi2.0# mii-tool lan1
lan1: no link
(But that really should be 100baseT, not 1000baseT).
Is there register dump available somewhere? I was using
/sys/bus/spi/devices/spi32766.0/registers but this does not seem to be
available.
I tried that ethtool -S, and counters do not seem to match:
Help welcome.
Best regards,
Pavel
root@miro:~# ifconfig eth0 up
root@miro:~# ifconfig lan3 192.168.20.103 netmask 255.255.0.0 up
[ 180.903675] IPv6: ADDRCONF(NETDEV_UP): lan3: link is not ready
root@miro:~# [ 181.913284] ksz8895-switch spi2.0 lan3: Link is Up - 100Mbps/Full - flow control rx/tx
[ 181.921358] IPv6: ADDRCONF(NETDEV_CHANGE): lan3: link becomes ready
root@miro:~# route add default gw 192.168.1.1
root@miro:~# ping 192.168.1.1
PING 192.168.1.1 (192.168.1.1): 56 data bytes
^C
--- 192.168.1.1 ping statistics ---
17 packets transmitted, 0 packets received, 100% packet loss
root@miro:~# ethtool -S eth0
NIC statistics:
tx_dropped: 0
tx_packets: 55
tx_broadcast: 35
tx_multicast: 20
tx_crc_errors: 0
tx_undersize: 0
tx_oversize: 0
tx_fragment: 0
tx_jabber: 0
tx_collision: 0
tx_64byte: 0
tx_65to127byte: 35
tx_128to255byte: 0
tx_256to511byte: 20
tx_512to1023byte: 0
tx_1024to2047byte: 0
tx_GTE2048byte: 0
tx_octets: 9576
IEEE_tx_drop: 0
IEEE_tx_frame_ok: 55
IEEE_tx_1col: 0
IEEE_tx_mcol: 0
IEEE_tx_def: 0
IEEE_tx_lcol: 0
IEEE_tx_excol: 0
IEEE_tx_macerr: 0
IEEE_tx_cserr: 0
IEEE_tx_sqe: 0
IEEE_tx_fdxfc: 0
IEEE_tx_octets_ok: 9576
rx_packets: 0
rx_broadcast: 0
rx_multicast: 0
rx_crc_errors: 0
rx_undersize: 0
rx_oversize: 0
rx_fragment: 0
rx_jabber: 0
rx_64byte: 0
rx_65to127byte: 0
rx_128to255byte: 0
rx_256to511byte: 0
rx_512to1023byte: 0
rx_1024to2047byte: 0
rx_GTE2048byte: 0
rx_octets: 0
IEEE_rx_drop: 0
IEEE_rx_frame_ok: 0
IEEE_rx_crc: 0
IEEE_rx_align: 0
IEEE_rx_macerr: 0
IEEE_rx_fdxfc: 0
IEEE_rx_octets_ok: 0
p04_rx: 660
p04_rx_hi: 0
p04_rx_undersize: 0
p04_rx_fragments: 20
p04_rx_oversize: 0
p04_rx_jabbers: 0
p04_rx_symbol_err: 0
p04_rx_crc_err: 0
p04_rx_align_err: 0
p04_rx_mac_ctrl: 0
p04_rx_pause: 0
p04_rx_bcast: 0
p04_rx_mcast: 0
p04_rx_ucast: 0
p04_rx_64_or_less: 0
p04_rx_65_127: 0
p04_rx_128_255: 0
p04_rx_256_511: 0
p04_rx_512_1023: 0
p04_rx_1024_1522: 0
p04_tx: 388
p04_tx_hi: 0
p04_tx_late_col: 0
p04_tx_pause: 0
p04_tx_bcast: 0
p04_tx_mcast: 3
p04_tx_ucast: 0
p04_tx_deferred: 0
p04_tx_total_col: 0
p04_tx_exc_col: 0
p04_tx_single_col: 0
p04_tx_mult_col: 0
p04_rx_discards: 0
p04_tx_discards: 0
root@miro:~# ethtool -S lan3
NIC statistics:
tx_packets: 24
tx_bytes: 1356
rx_packets: 0
rx_bytes: 0
rx: 566
rx_hi: 0
rx_undersize: 0
rx_fragments: 0
rx_oversize: 0
rx_jabbers: 0
rx_symbol_err: 0
rx_crc_err: 0
rx_align_err: 0
rx_mac_ctrl: 0
rx_pause: 0
rx_bcast: 0
rx_mcast: 4
rx_ucast: 0
rx_64_or_less: 0
rx_65_127: 1
rx_128_255: 3
rx_256_511: 0
rx_512_1023: 0
rx_1024_1522: 0
tx: 0
tx_hi: 0
tx_late_col: 0
tx_pause: 0
tx_bcast: 0
tx_mcast: 0
tx_ucast: 0
tx_deferred: 0
tx_total_col: 0
tx_exc_col: 0
tx_single_col: 0
tx_mult_col: 0
rx_discards: 0
tx_discards: 0
root@miro:~#
--
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 181 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] net: dsa: lan9303: Protect ALR operations with mutex
From: Andrew Lunn @ 2017-12-07 20:58 UTC (permalink / raw)
To: Egil Hjelmeland; +Cc: vivien.didelot, f.fainelli, netdev, linux-kernel
In-Reply-To: <20171207185604.30745-1-privat@egil-hjelmeland.no>
On Thu, Dec 07, 2017 at 07:56:04PM +0100, Egil Hjelmeland wrote:
> ALR table operations are a sequence of related register operations which
> should be protected from concurrent access. The alr_cache should also be
> protected. Add alr_mutex doing that.
>
> Signed-off-by: Egil Hjelmeland <privat@egil-hjelmeland.no>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* [PATCH] xfrm: fix xfrm_do_migrate() with AEAD e.g(AES-GCM)
From: Antony Antony @ 2017-12-07 20:54 UTC (permalink / raw)
To: Steffen Klassert, Herbert Xu; +Cc: netdev, David S . Miller, Antony Antony
copy geniv when cloning the xfrm state.
x->geniv was not copied to the new state and migration would fail.
xfrm_do_migrate
..
xfrm_state_clone()
..
..
esp_init_aead()
crypto_alloc_aead()
crypto_alloc_tfm()
crypto_find_alg() return EAGAIN and failed
Signed-off-by: Antony Antony <antony@phenome.org>
---
net/xfrm/xfrm_state.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/xfrm/xfrm_state.c b/net/xfrm/xfrm_state.c
index 065d89606888..500b3391f474 100644
--- a/net/xfrm/xfrm_state.c
+++ b/net/xfrm/xfrm_state.c
@@ -1343,6 +1343,7 @@ static struct xfrm_state *xfrm_state_clone(struct xfrm_state *orig,
if (orig->aead) {
x->aead = xfrm_algo_aead_clone(orig->aead);
+ x->geniv = orig->geniv;
if (!x->aead)
goto error;
}
--
2.13.6
^ permalink raw reply related
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case
From: Ivan Khoronzhuk @ 2017-12-07 20:21 UTC (permalink / raw)
To: David Miller; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel
In-Reply-To: <20171207.151315.1535972470633975595.davem@davemloft.net>
On Thu, Dec 07, 2017 at 03:13:15PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Date: Thu, 7 Dec 2017 22:10:06 +0200
>
> > On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> Date: Thu, 7 Dec 2017 21:48:56 +0200
> >>
> >> > On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
> >> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> Date: Wed, 6 Dec 2017 16:41:18 +0200
> >> >>
> >> >> > If rate is the same as set it's correct case.
> >> >> >
> >> >> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> > ---
> >> >> > Based on net-next/master
> >> >> >
> >> >> > drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> >> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >> >
> >> >> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> >> > index e4d6edf..dbe9167 100644
> >> >> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> >> >> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> >> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
> >> >> > return -EINVAL;
> >> >> >
> >> >> > if (ch->rate == rate)
> >> >> > - return rate;
> >> >> > + return 0;
> >> >>
> >> >> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
> >> >> makes sure this can never, ever, happen.
> >> > In current circumstances yes, it will never happen.
> >> > But I caught it while adding related code and better return 0 if upper caller
> >> > doesn't have such check. Suppose that cpdma module is responsible for itself
> >> > and if it's critical I can send this patch along with whole related series.
> >>
> >> You have to decide one way or the other, who is responsible.
> >>
> >> I think checking higher up is better because it's cheaper at that point to
> >> look at the per-netdev queue rate setting before moving down deeper into the
> >> driver specific data-structures.
> >
> > No objection, but upper caller not always knows current rate and for doing like
> > this it needs read it first, and this is also some redundancy.
>
> How can the upper caller not know the current rate? The rate is
> always stored in the generic netdev per-queue datastructure.
>
> And that's what existing code checks right now.
Right now, when generic netdev only caller - yes.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case
From: David Miller @ 2017-12-07 20:13 UTC (permalink / raw)
To: ivan.khoronzhuk; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel
In-Reply-To: <20171207201005.GC3022@khorivan>
From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
Date: Thu, 7 Dec 2017 22:10:06 +0200
> On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
>> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> Date: Thu, 7 Dec 2017 21:48:56 +0200
>>
>> > On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
>> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >> Date: Wed, 6 Dec 2017 16:41:18 +0200
>> >>
>> >> > If rate is the same as set it's correct case.
>> >> >
>> >> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >> > ---
>> >> > Based on net-next/master
>> >> >
>> >> > drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
>> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >> >
>> >> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
>> >> > index e4d6edf..dbe9167 100644
>> >> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
>> >> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
>> >> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
>> >> > return -EINVAL;
>> >> >
>> >> > if (ch->rate == rate)
>> >> > - return rate;
>> >> > + return 0;
>> >>
>> >> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
>> >> makes sure this can never, ever, happen.
>> > In current circumstances yes, it will never happen.
>> > But I caught it while adding related code and better return 0 if upper caller
>> > doesn't have such check. Suppose that cpdma module is responsible for itself
>> > and if it's critical I can send this patch along with whole related series.
>>
>> You have to decide one way or the other, who is responsible.
>>
>> I think checking higher up is better because it's cheaper at that point to
>> look at the per-netdev queue rate setting before moving down deeper into the
>> driver specific data-structures.
>
> No objection, but upper caller not always knows current rate and for doing like
> this it needs read it first, and this is also some redundancy.
How can the upper caller not know the current rate? The rate is
always stored in the generic netdev per-queue datastructure.
And that's what existing code checks right now.
^ permalink raw reply
* Re: [PATCH net-next] net: ethernet: ti: cpdma: rate is not changed - correct case
From: Ivan Khoronzhuk @ 2017-12-07 20:10 UTC (permalink / raw)
To: David Miller; +Cc: grygorii.strashko, netdev, linux-omap, linux-kernel
In-Reply-To: <20171207.145024.1434883857028947517.davem@davemloft.net>
On Thu, Dec 07, 2017 at 02:50:24PM -0500, David Miller wrote:
> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Date: Thu, 7 Dec 2017 21:48:56 +0200
>
> > On Wed, Dec 06, 2017 at 04:35:45PM -0500, David Miller wrote:
> >> From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> Date: Wed, 6 Dec 2017 16:41:18 +0200
> >>
> >> > If rate is the same as set it's correct case.
> >> >
> >> > Signed-off-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> > ---
> >> > Based on net-next/master
> >> >
> >> > drivers/net/ethernet/ti/davinci_cpdma.c | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/net/ethernet/ti/davinci_cpdma.c b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> > index e4d6edf..dbe9167 100644
> >> > --- a/drivers/net/ethernet/ti/davinci_cpdma.c
> >> > +++ b/drivers/net/ethernet/ti/davinci_cpdma.c
> >> > @@ -841,7 +841,7 @@ int cpdma_chan_set_rate(struct cpdma_chan *ch, u32 rate)
> >> > return -EINVAL;
> >> >
> >> > if (ch->rate == rate)
> >> > - return rate;
> >> > + return 0;
> >>
> >> Looking at the one and only caller of this function, cpsw_ndo_set_tx_maxrate, it
> >> makes sure this can never, ever, happen.
> > In current circumstances yes, it will never happen.
> > But I caught it while adding related code and better return 0 if upper caller
> > doesn't have such check. Suppose that cpdma module is responsible for itself
> > and if it's critical I can send this patch along with whole related series.
>
> You have to decide one way or the other, who is responsible.
>
> I think checking higher up is better because it's cheaper at that point to
> look at the per-netdev queue rate setting before moving down deeper into the
> driver specific data-structures.
No objection, but upper caller not always knows current rate and for doing like
this it needs read it first, and this is also some redundancy.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net-next 0/6] smc: fixes 2017-12-07
From: David Miller @ 2017-12-07 20:06 UTC (permalink / raw)
To: ubraun; +Cc: netdev, linux-s390, jwi, schwidefsky, heiko.carstens, raspl
In-Reply-To: <20171207123849.52910-1-ubraun@linux.vnet.ibm.com>
From: Ursula Braun <ubraun@linux.vnet.ibm.com>
Date: Thu, 7 Dec 2017 13:38:43 +0100
> here are some smc-patches. The initial 4 patches are cleanups.
> Patch 5 gets rid of ib_post_sends in tasklet context to avoid peer drops due
> to out-of-order receivals.
> Patch 6 makes sure, the Linux SMC code understands variable sized CLC proposal
> messages built according to RFC7609.
Series applied to net-next.
^ permalink raw reply
* Re: [PATCH v5 net-next] net/tcp: trace all TCP/IP state transition with tcp_set_state tracepoint
From: David Miller @ 2017-12-07 20:04 UTC (permalink / raw)
To: marcelo.leitner
Cc: laoar.shao, songliubraving, kuznet, yoshfuji, rostedt, bgregg,
netdev, linux-kernel
In-Reply-To: <20171207200245.GQ13341@localhost.localdomain>
From: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
Date: Thu, 7 Dec 2017 18:02:46 -0200
> Dave?
Yeah that will untangle this mess much more nicely.
^ 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