* Re: [PATCH net-next 4/8] dt-bindings: net: add DT bindings for Microsemi Ocelot Switch
From: Rob Herring @ 2018-03-26 22:25 UTC (permalink / raw)
To: Alexandre Belloni
Cc: David S . Miller, Allan Nielsen, razvan.stefanescu, po.liu,
Thomas Petazzoni, Andrew Lunn, Florian Fainelli, netdev,
devicetree, linux-kernel, linux-mips
In-Reply-To: <20180323201117.8416-5-alexandre.belloni@bootlin.com>
On Fri, Mar 23, 2018 at 09:11:13PM +0100, Alexandre Belloni wrote:
> DT bindings for the Ethernet switch found on Microsemi Ocelot platforms.
>
> Cc: Rob Herring <robh+dt@kernel.org>
> Signed-off-by: Alexandre Belloni <alexandre.belloni@bootlin.com>
> ---
> .../devicetree/bindings/net/mscc-ocelot.txt | 62 ++++++++++++++++++++++
> 1 file changed, 62 insertions(+)
> create mode 100644 Documentation/devicetree/bindings/net/mscc-ocelot.txt
>
> diff --git a/Documentation/devicetree/bindings/net/mscc-ocelot.txt b/Documentation/devicetree/bindings/net/mscc-ocelot.txt
> new file mode 100644
> index 000000000000..ee092a85b5a0
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/net/mscc-ocelot.txt
> @@ -0,0 +1,62 @@
> +Microsemi Ocelot network Switch
> +===============================
> +
> +The Microsemi Ocelot network switch can be found on Microsemi SoCs (VSC7513,
> +VSC7514)
What's the difference in these SoCs? You should probably have the
part#'s in the compatible string.
> +
> +Required properties:
> +- compatible: Should be "mscc,ocelot-switch"
> +- reg: Must contain an (offset, length) pair of the register set for each
> + entry in reg-names.
> +- reg-names: Must include the following entries:
> + - "sys"
> + - "rew"
> + - "qs"
> + - "hsio"
> + - "qsys"
> + - "ana"
> + - "portX" with X from 0 to the number of last port index available on that
> + switch
> +- interrupts: Should contain the switch interrupts for frame extraction and
> + frame injection
> +- interrupt-names: should contain the interrupt names: "xtr", "inj"
> +
> +Example:
> +
> + switch@1010000 {
> + #address-cells = <1>;
> + #size-cells = <0>;
> + compatible = "mscc,ocelot-switch";
> + reg = <0x1010000 0x10000>,
> + <0x1030000 0x10000>,
> + <0x1080000 0x100>,
> + <0x10d0000 0x10000>,
> + <0x11e0000 0x100>,
> + <0x11f0000 0x100>,
> + <0x1200000 0x100>,
> + <0x1210000 0x100>,
> + <0x1220000 0x100>,
> + <0x1230000 0x100>,
> + <0x1240000 0x100>,
> + <0x1250000 0x100>,
> + <0x1260000 0x100>,
> + <0x1270000 0x100>,
> + <0x1280000 0x100>,
> + <0x1800000 0x80000>,
> + <0x1880000 0x10000>;
> + reg-names = "sys", "rew", "qs", "hsio", "port0",
> + "port1", "port2", "port3", "port4", "port5",
> + "port6", "port7", "port8", "port9", "port10",
> + "qsys", "ana";
> + interrupts = <21 22>;
> + interrupt-names = "xtr", "inj";
> +
> + port0: port@0 {
> + reg = <0>;
> + phy-handle = <&phy0>;
> + };
> + port1: port@1 {
> + reg = <1>;
> + phy-handle = <&phy1>;
> + };
> + };
> --
> 2.16.2
>
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, tracing: unbreak lttng
From: Alexei Starovoitov @ 2018-03-26 22:25 UTC (permalink / raw)
To: Steven Rostedt, Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, mathieu.desnoyers, netdev,
kernel-team, linux-api
In-Reply-To: <20180326181532.587e9e2b@gandalf.local.home>
On 3/26/18 3:15 PM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 15:08:45 -0700
> Alexei Starovoitov <ast@kernel.org> wrote:
>
>> for_each_kernel_tracepoint() is used by out-of-tree lttng module
>> and therefore cannot be changed.
>> Instead introduce kernel_tracepoint_find_by_name() to find
>> tracepoint by name.
>>
>> Fixes: 9e9afbae6514 ("tracepoint: compute num_args at build time")
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
> I'm curious, why can't you rebase? The first patch was never acked.
because I think it makes sense to keep such things in the commit log
and in the separate diff, so next developer is aware of what kind of
minefield the tracpoints are.
No wonder some maintainers refuse to add them.
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: broadcom: Adaptive interrupt coalescing
From: Florian Fainelli @ 2018-03-26 22:29 UTC (permalink / raw)
To: Tal Gilboa, netdev
Cc: davem, jaedon.shin, pgynther, opendmb, michael.chan, gospo,
saeedm
In-Reply-To: <4625ee80-9588-e39c-5add-3c57432c1141@gmail.com>
On 03/26/2018 03:04 PM, Florian Fainelli wrote:
> On 03/26/2018 02:16 PM, Tal Gilboa wrote:
>> On 3/23/2018 4:19 AM, Florian Fainelli wrote:
>>> Hi all,
>>>
>>> This patch series adds adaptive interrupt coalescing for the Gigabit
>>> Ethernet
>>> drivers SYSTEMPORT and GENET.
>>>
>>> This really helps lower the interrupt count and system load, as
>>> measured by
>>> vmstat for a Gigabit TCP RX session:
>>
>> I don't see an improvement in system load, the opposite - 42% vs. 100%
>> for SYSTEMPORT and 85% vs. 100% for GENET. Both with the same bandwidth.
>
> Looks like I did not extract the correct data the load could spike in
> both cases (with and without net_dim) up to 100, but averaged over the
> transmission I see the following:
>
> GENET without:
> 1 0 0 1169568 0 25556 0 0 0 0 130079 62795 2
> 86 13 0 0
>
> GENET with:
> 1 0 0 1169536 0 25556 0 0 0 0 10566 10869 1
> 21 78 0 0
>
>> Am I missing something? Talking about bandwidth, I would expect 941Mb/s
>> (assuming this is TCP over IPv4). Do you know why the reduced interrupt
>> rate doesn't improve bandwidth?
>
> I am assuming that this comes down to a latency, still capturing some
> pcap files to analyze the TCP session with wireshark and see if that is
> indeed what is going on. The test machine is actually not that great
>
>> Also, any effect on the client side (you
>> mentioned enabling TX moderation for SYSTEMPORT)?
>
> Yes, on SYSTEMPORT, being the TCP IPv4 client, I have the following:
>
> SYSTEMPORT without:
> 2 0 0 191428 0 25748 0 0 0 0 86254 264 0 41
> 59 0 0
>
> SYSTEMPORT with:
> 3 0 0 190176 0 25748 0 0 0 0 45485 31332 0
> 100 0 0 0
>
> I don't get top to agree with these load results though but it looks
> like we just have the CPU spinning more, does not look like a win.
The problem appears to be the timeout selection on TX, ignoring it
completely allows us to keep the load average down while maintaining the
bandwidth. Looks like NAPI on TX already does a good job, so interrupt
mitigation on TX is not such a great idea actually...
Also, doing UDP TX tests shows that we can lower the interrupt count by
setting an appropriate tx-frames (as expected), but we won't be lowering
the CPU load since that is inherently a CPU intensive work. Past
tx-frames=64, the bandwidth completely drops because that would be 1/2
of the ring size.
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, tracing: unbreak lttng
From: Mathieu Desnoyers @ 2018-03-26 22:30 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <20180326220845.678423-1-ast@kernel.org>
----- On Mar 26, 2018, at 6:08 PM, Alexei Starovoitov ast@kernel.org wrote:
[...]
>
> #ifdef CONFIG_TRACEPOINTS
> -void *
> -for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
> +void
> +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
> void *priv);
> +struct tracepoint *kernel_tracepoint_find_by_name(const char *name);
> #else
> -static inline void *
> -for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
> +static inline void
> +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
> void *priv)
> {
> return NULL;
> }
This patch is not reverting to the old code properly. It introduces a
static inline void function that returns NULL. Please compile-test
with CONFIG_TRACEPOINTS=n before submitting a patch involving tracepoints.
But this patch should not even be needed in the first place, because it
partially reverts changes that were introduced in the bpf-next tree without
any Acked-by from the tracing maintainers. I don't see any need to obfuscate
the git log of tracepoint.{c,h}.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v2 iproute2-next 3/6] rdma: Add CM_ID resource tracking information
From: Jason Gunthorpe @ 2018-03-26 22:30 UTC (permalink / raw)
To: Steve Wise; +Cc: David Ahern, leon, stephen, netdev, linux-rdma
In-Reply-To: <b5f651c2-8f71-bc1b-32b8-f4c5175fb4a0@opengridcomputing.com>
On Mon, Mar 26, 2018 at 04:34:44PM -0500, Steve Wise wrote:
>
> On 3/26/2018 4:15 PM, Jason Gunthorpe wrote:
> > On Mon, Mar 26, 2018 at 09:30:41AM -0500, Steve Wise wrote:
> >>
> >> On 3/26/2018 9:17 AM, David Ahern wrote:
> >>> On 2/27/18 9:07 AM, Steve Wise wrote:
> >>>> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >>>> index 5809f70..e55205b 100644
> >>>> +++ b/rdma/rdma.h
> >>>> @@ -18,10 +18,12 @@
> >>>> #include <libmnl/libmnl.h>
> >>>> #include <rdma/rdma_netlink.h>
> >>>> #include <time.h>
> >>>> +#include <net/if_arp.h>
> >>>>
> >>>> #include "list.h"
> >>>> #include "utils.h"
> >>>> #include "json_writer.h"
> >>>> +#include <rdma/rdma_cma.h>
> >>>>
> >>> did you forget to add rdma_cma.h? I don't see that file in my repo.
> >> It is provided by the rdma-core package, upon which rdma tool now
> >> depends for the rdma_port_space enum.
> > It is a kernel bug that enum is not in an include/uapi/rdma header
> >
> > Fix it there and don't try to use rdma-core headers to get kernel ABI.
> >
> > Jason
>
> I wish you'd commented on this just a little sooner. I just resent v3
> of this series... with rdma_cma.h included. :)
>
> How about the restrack/nldev code just translates the port space from
> enum rdma_port_space to a new ABI enum, say nldev_rdma_port_space, that
> i add to rdma_netlink.h? I'd hate to open the can of worms of trying to
> split rdma_cma.h into uabi and no uabi headers. :(
If port space is already part of the ABI there isn't much reason to
translate it.
You just need to pick the right header to put it in, since it is a verbs
define it doesn't belong in the netlink header.
Jason
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, tracing: unbreak lttng
From: Alexei Starovoitov @ 2018-03-26 22:35 UTC (permalink / raw)
To: Mathieu Desnoyers, Alexei Starovoitov
Cc: David S. Miller, Daniel Borkmann, Linus Torvalds, Peter Zijlstra,
rostedt, netdev, kernel-team, linux-api
In-Reply-To: <1523827268.612.1522103407744.JavaMail.zimbra@efficios.com>
On 3/26/18 3:30 PM, Mathieu Desnoyers wrote:
> ----- On Mar 26, 2018, at 6:08 PM, Alexei Starovoitov ast@kernel.org wrote:
> [...]
>>
>> #ifdef CONFIG_TRACEPOINTS
>> -void *
>> -for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
>> +void
>> +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
>> void *priv);
>> +struct tracepoint *kernel_tracepoint_find_by_name(const char *name);
>> #else
>> -static inline void *
>> -for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
>> +static inline void
>> +for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
>> void *priv)
>> {
>> return NULL;
>> }
>
> This patch is not reverting to the old code properly. It introduces a
> static inline void function that returns NULL. Please compile-test
> with CONFIG_TRACEPOINTS=n before submitting a patch involving tracepoints.
right. good catch. v2 is coming.
^ permalink raw reply
* Re: [PATCH bpf-next] bpf, tracing: unbreak lttng
From: Mathieu Desnoyers @ 2018-03-26 22:39 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: rostedt, Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api
In-Reply-To: <24d0ff40-c6fd-6349-4a89-dffda22cb596@fb.com>
----- On Mar 26, 2018, at 6:25 PM, Alexei Starovoitov ast@fb.com wrote:
> On 3/26/18 3:15 PM, Steven Rostedt wrote:
>> On Mon, 26 Mar 2018 15:08:45 -0700
>> Alexei Starovoitov <ast@kernel.org> wrote:
>>
>>> for_each_kernel_tracepoint() is used by out-of-tree lttng module
>>> and therefore cannot be changed.
>>> Instead introduce kernel_tracepoint_find_by_name() to find
>>> tracepoint by name.
>>>
>>> Fixes: 9e9afbae6514 ("tracepoint: compute num_args at build time")
>>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>>
>> I'm curious, why can't you rebase? The first patch was never acked.
>
> because I think it makes sense to keep such things in the commit log
> and in the separate diff, so next developer is aware of what kind of
> minefield the tracpoints are.
> No wonder some maintainers refuse to add them.
Since when has it become accepted to push commits into maintainer's
subsystems without their acknowledgment first ?
The minefield you are currently walking through appears to be of your
own making, so please just rework your initial patch before it reaches
upstream.
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH net] r8169: fix setting driver_data after register_netdev
From: Andrew Lunn @ 2018-03-26 22:40 UTC (permalink / raw)
To: Francois Romieu
Cc: Heiner Kallweit, Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20180326221840.GA32458@electric-eye.fr.zoreil.com>
On Tue, Mar 27, 2018 at 12:18:40AM +0200, Francois Romieu wrote:
> Andrew Lunn <andrew@lunn.ch> :
> [...]
> > How about rtl8169_get_wol() and rtl8169_set_wol(). And
> > rtl8169_get_ethtool_stats().
>
> rtl8169_get_wol does not depend on dev->driver_data. Neither does
> rtl8169_set_wol() nor rtl8169_get_ethtool_stats().
I don't know runtime pm very well, but these functions call
pm_runtime_get_noresume and pm_runtime_put_noidle. If they can result
in calls to any of the rtl8169_runtime_* functions, pci_get_drvdata()
is going to get called.
Andrew
^ permalink raw reply
* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
From: Florian Fainelli @ 2018-03-26 22:44 UTC (permalink / raw)
To: vicentiu.galanopulo, netdev, linux-kernel, robh+dt, mark.rutland,
davem, marcel, devicetree
Cc: madalin.bucur, alexandru.marginean
In-Reply-To: <20180323150522.9603-1-vicentiu.galanopulo@nxp.com>
On 03/23/2018 08:05 AM, Vicentiu Galanopulo wrote:
> Reason for this patch is that the Inphi PHY has a
> vendor specific address space for accessing the
> C45 MDIO registers - starting from 0x1e.
>
> A search of the dev-addr property is done in of_mdiobus_register.
> If the property is found in the PHY node,
> of_mdiobus_register_static_phy is called. This is a
> wrapper function for of_mdiobus_register_phy which finds the
> device in package based on dev-addr and fills devices_addrs:
> devices_addrs is a new field added to phy_c45_device_ids.
> This new field will store the dev-addr property on the same
> index where the device in package has been found.
> In order to have dev-addr in get_phy_c45_ids(), mdio_c45_ids is
> passed from of_mdio.c to phy_device.c as an external variable.
> In get_phy_device a copy of the mdio_c45_ids is done over the
> local c45_ids (wich are empty). After the copying, the c45_ids
> will also contain the static device found from dev-addr.
> Having dev-addr stored in devices_addrs, in get_phy_c45_ids(),
> when probing the identifiers, dev-addr can be extracted from
> devices_addrs and probed if devices_addrs[current_identifier]
> is not 0.
> This way changing the kernel API is avoided completely.
>
> As a plus to this patch, num_ids in get_phy_c45_ids,
> has the value 8 (ARRAY_SIZE(c45_ids->device_ids)),
> but the u32 *devs can store 32 devices in the bitfield.
> If a device is stored in *devs, in bits 32 to 9, it
> will not be found. This is the reason for changing
> in phy.h, the size of device_ids array.
>
> Signed-off-by: Vicentiu Galanopulo <vicentiu.galanopulo@nxp.com>
> ---
Correct me if I am completely misunderstanding the problem, but have
you considered doing the following:
- if all you need to is to replace instances of loops that do:
if (phydev->is_c45) {
for (i = 1; i < num_ids; i++) {
if (!(phydev->c45_ids.devices_in_package & (1 <<
i)))
continue;
with one that starts at dev-addr, as specified by Device Tree, then I
suspect there is an easier way to do what you want rather than your
fairly intrusive patch to do that:
- patch of_mdiobus_register_phy() to lookup both the c45 compatible
string as well as fetch the "dev-addr" property
- provide a PHY Device Tree node that has its OUI as a compatible string
(see of_get_phy_id() for details), or if it has a specified 'dev-addr'
property, use that in lieu of the default get_phy_device() logic
- pass both to phy_device_create() and eventually introduce a helper
function that lets you populate the c45_ids structure
Then for each function that does the loop above, as long as you have a
phydev reference, you can have phydev->dev_addr = 0x1e be where to start
from, if it is 0, then start at 1 (like it currently is). If you don't
have a phy device reference, which would be get_phy_c45_ids() then just
make sure you don't call that function :)
> struct phy_c45_device_ids {
> u32 devices_in_package;
> - u32 device_ids[8];
> + u32 device_ids[32];
> + u32 devices_addrs[32];
> };
This looks like a fix in itself, so it is worth a separate patch.
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 4/8] dt-bindings: net: add DT bindings for Microsemi Ocelot Switch
From: Andrew Lunn @ 2018-03-26 22:50 UTC (permalink / raw)
To: Rob Herring
Cc: Florian Fainelli, Alexandre Belloni, David S . Miller,
Allan Nielsen, razvan.stefanescu, po.liu, Thomas Petazzoni,
netdev, devicetree, linux-kernel, linux-mips
In-Reply-To: <20180326222514.4eciw66aihhcjgtw@rob-hp-laptop>
> ports and port collide with the OF graph binding. It would be good if
> this moved to ethernet-port(s) or similar.
Hi Rob
Well, we have been using port in DSA since March 2013. ports is a bit
newer, June 2016.
Changing DSA is not going to happen. But new switch bindings could use
ethernet-port(s). It just makes them inconsistent with existing switch
drivers.
Andrew
^ permalink raw reply
* Re: [net-next 00/15][pull request] 100GbE Intel Wired LAN Driver Updates 2018-03-26
From: David Miller @ 2018-03-26 22:54 UTC (permalink / raw)
To: jeffrey.t.kirsher
Cc: netdev, nhorman, sassmann, jogreene, tbogendoerfer, bpoirier
In-Reply-To: <20180326194619.1202-1-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 26 Mar 2018 12:46:04 -0700
> This patch series adds the ice driver, which will support the Intel(R)
> E800 Series of network devices.
Pulled, thanks Jeff.
^ permalink raw reply
* Re: [RFC PATCH 00/24] Introducing AF_XDP support
From: Tushar Dave @ 2018-03-26 22:54 UTC (permalink / raw)
To: Jesper Dangaard Brouer, William Tu
Cc: Björn Töpel, magnus.karlsson, Alexander Duyck,
Alexander Duyck, John Fastabend, Alexei Starovoitov,
willemdebruijn.kernel, Daniel Borkmann,
Linux Kernel Network Developers, Björn Töpel,
michael.lundkvist, jesse.brandeburg, anjali.singhai,
jeffrey.b.shaw, ferruh.yigit, qi.z.zhang
In-Reply-To: <20180326183810.2ef4e29f@redhat.com>
On 03/26/2018 09:38 AM, Jesper Dangaard Brouer wrote:
>
> On Mon, 26 Mar 2018 09:06:54 -0700 William Tu <u9012063@gmail.com> wrote:
>
>> On Wed, Jan 31, 2018 at 5:53 AM, Björn Töpel <bjorn.topel@gmail.com> wrote:
>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>
>>> This RFC introduces a new address family called AF_XDP that is
>>> optimized for high performance packet processing and zero-copy
>>> semantics. Throughput improvements can be up to 20x compared to V2 and
>>> V3 for the micro benchmarks included. Would be great to get your
>>> feedback on it. Note that this is the follow up RFC to AF_PACKET V4
>>> from November last year. The feedback from that RFC submission and the
>>> presentation at NetdevConf in Seoul was to create a new address family
>>> instead of building on top of AF_PACKET. AF_XDP is this new address
>>> family.
>>>
>>> The main difference between AF_XDP and AF_PACKET V2/V3 on a descriptor
>>> level is that TX and RX descriptors are separated from packet
>>> buffers. An RX or TX descriptor points to a data buffer in a packet
>>> buffer area. RX and TX can share the same packet buffer so that a
>>> packet does not have to be copied between RX and TX. Moreover, if a
>>> packet needs to be kept for a while due to a possible retransmit, then
>>> the descriptor that points to that packet buffer can be changed to
>>> point to another buffer and reused right away. This again avoids
>>> copying data.
>>>
>>> The RX and TX descriptor rings are registered with the setsockopts
>>> XDP_RX_RING and XDP_TX_RING, similar to AF_PACKET. The packet buffer
>>> area is allocated by user space and registered with the kernel using
>>> the new XDP_MEM_REG setsockopt. All these three areas are shared
>>> between user space and kernel space. The socket is then bound with a
>>> bind() call to a device and a specific queue id on that device, and it
>>> is not until bind is completed that traffic starts to flow.
>>>
>>> An XDP program can be loaded to direct part of the traffic on that
>>> device and queue id to user space through a new redirect action in an
>>> XDP program called bpf_xdpsk_redirect that redirects a packet up to
>>> the socket in user space. All the other XDP actions work just as
>>> before. Note that the current RFC requires the user to load an XDP
>>> program to get any traffic to user space (for example all traffic to
>>> user space with the one-liner program "return
>>> bpf_xdpsk_redirect();"). We plan on introducing a patch that removes
>>> this requirement and sends all traffic from a queue to user space if
>>> an AF_XDP socket is bound to it.
>>>
>>> AF_XDP can operate in three different modes: XDP_SKB, XDP_DRV, and
>>> XDP_DRV_ZC (shorthand for XDP_DRV with a zero-copy allocator as there
>>> is no specific mode called XDP_DRV_ZC). If the driver does not have
>>> support for XDP, or XDP_SKB is explicitly chosen when loading the XDP
>>> program, XDP_SKB mode is employed that uses SKBs together with the
>>> generic XDP support and copies out the data to user space. A fallback
>>> mode that works for any network device. On the other hand, if the
>>> driver has support for XDP (all three NDOs: ndo_bpf, ndo_xdp_xmit and
>>> ndo_xdp_flush), these NDOs, without any modifications, will be used by
>>> the AF_XDP code to provide better performance, but there is still a
>>> copy of the data into user space. The last mode, XDP_DRV_ZC, is XDP
>>> driver support with the zero-copy user space allocator that provides
>>> even better performance. In this mode, the networking HW (or SW driver
>>> if it is a virtual driver like veth) DMAs/puts packets straight into
>>> the packet buffer that is shared between user space and kernel
>>> space. The RX and TX descriptor queues of the networking HW are NOT
>>> shared to user space. Only the kernel can read and write these and it
>>> is the kernel driver's responsibility to translate these HW specific
>>> descriptors to the HW agnostic ones in the virtual descriptor rings
>>> that user space sees. This way, a malicious user space program cannot
>>> mess with the networking HW. This mode though requires some extensions
>>> to XDP.
>>>
>>> To get the XDP_DRV_ZC mode to work for RX, we chose to introduce a
>>> buffer pool concept so that the same XDP driver code can be used for
>>> buffers allocated using the page allocator (XDP_DRV), the user-space
>>> zero-copy allocator (XDP_DRV_ZC), or some internal driver specific
>>> allocator/cache/recycling mechanism. The ndo_bpf call has also been
>>> extended with two commands for registering and unregistering an XSK
>>> socket and is in the RX case mainly used to communicate some
>>> information about the user-space buffer pool to the driver.
>>>
>>> For the TX path, our plan was to use ndo_xdp_xmit and ndo_xdp_flush,
>>> but we run into problems with this (further discussion in the
>>> challenges section) and had to introduce a new NDO called
>>> ndo_xdp_xmit_xsk (xsk = XDP socket). It takes a pointer to a netdevice
>>> and an explicit queue id that packets should be sent out on. In
>>> contrast to ndo_xdp_xmit, it is asynchronous and pulls packets to be
>>> sent from the xdp socket (associated with the dev and queue
>>> combination that was provided with the NDO call) using a callback
>>> (get_tx_packet), and when they have been transmitted it uses another
>>> callback (tx_completion) to signal completion of packets. These
>>> callbacks are set via ndo_bpf in the new XDP_REGISTER_XSK
>>> command. ndo_xdp_xmit_xsk is exclusively used by the XDP socket code
>>> and thus does not clash with the XDP_REDIRECT use of
>>> ndo_xdp_xmit. This is one of the reasons that the XDP_DRV mode
>>> (without ZC) is currently not supported by TX. Please have a look at
>>> the challenges section for further discussions.
>>>
>>> The AF_XDP bind call acts on a queue pair (channel in ethtool speak),
>>> so the user needs to steer the traffic to the zero-copy enabled queue
>>> pair. Which queue to use, is up to the user.
>>>
>>> For an untrusted application, HW packet steering to a specific queue
>>> pair (the one associated with the application) is a requirement, as
>>> the application would otherwise be able to see other user space
>>> processes' packets. If the HW cannot support the required packet
>>> steering, XDP_DRV or XDP_SKB mode have to be used as they do not
>>> expose the NIC's packet buffer into user space as the packets are
>>> copied into user space from the NIC's packet buffer in the kernel.
>>>
>>> There is a xdpsock benchmarking/test application included. Say that
>>> you would like your UDP traffic from port 4242 to end up in queue 16,
>>> that we will enable AF_XDP on. Here, we use ethtool for this:
>>>
>>> ethtool -N p3p2 rx-flow-hash udp4 fn
>>> ethtool -N p3p2 flow-type udp4 src-port 4242 dst-port 4242 \
>>> action 16
>>>
>>> Running the l2fwd benchmark in XDP_DRV_ZC mode can then be done using:
>>>
>>> samples/bpf/xdpsock -i p3p2 -q 16 -l -N
>>>
>>> For XDP_SKB mode, use the switch "-S" instead of "-N" and all options
>>> can be displayed with "-h", as usual.
>>>
>>> We have run some benchmarks on a dual socket system with two Broadwell
>>> E5 2660 @ 2.0 GHz with hyperthreading turned off. Each socket has 14
>>> cores which gives a total of 28, but only two cores are used in these
>>> experiments. One for TR/RX and one for the user space application. The
>>> memory is DDR4 @ 2133 MT/s (1067 MHz) and the size of each DIMM is
>>> 8192MB and with 8 of those DIMMs in the system we have 64 GB of total
>>> memory. The compiler used is gcc version 5.4.0 20160609. The NIC is an
>>> Intel I40E 40Gbit/s using the i40e driver.
>>>
>>> Below are the results in Mpps of the I40E NIC benchmark runs for 64
>>> byte packets, generated by commercial packet generator HW that is
>>> generating packets at full 40 Gbit/s line rate.
>>>
>>> XDP baseline numbers without this RFC:
>>> xdp_rxq_info --action XDP_DROP 31.3 Mpps
>>> xdp_rxq_info --action XDP_TX 16.7 Mpps
>>>
>>> XDP performance with this RFC i.e. with the buffer allocator:
>>> XDP_DROP 21.0 Mpps
>>> XDP_TX 11.9 Mpps
>>>
>>> AF_PACKET V4 performance from previous RFC on 4.14-rc7:
>>> Benchmark V2 V3 V4 V4+ZC
>>> rxdrop 0.67 0.73 0.74 33.7
>>> txpush 0.98 0.98 0.91 19.6
>>> l2fwd 0.66 0.71 0.67 15.5
>>>
>>> AF_XDP performance:
>>> Benchmark XDP_SKB XDP_DRV XDP_DRV_ZC (all in Mpps)
>>> rxdrop 3.3 11.6 16.9
>>> txpush 2.2 NA* 21.8
>>> l2fwd 1.7 NA* 10.4
>>>
>>
>> Hi,
>> I also did an evaluation of AF_XDP, however the performance isn't as
>> good as above.
>> I'd like to share the result and see if there are some tuning suggestions.
>>
>> System:
>> 16 core, Intel(R) Xeon(R) CPU E5-2440 v2 @ 1.90GHz
>> Intel 10G X540-AT2 ---> so I can only run XDP_SKB mode
>
> Hmmm, why is X540-AT2 not able to use XDP natively?
>
>> AF_XDP performance:
>> Benchmark XDP_SKB
>> rxdrop 1.27 Mpps
>> txpush 0.99 Mpps
>> l2fwd 0.85 Mpps
>
> Definitely too low...
>
> What is the performance if you drop packets via iptables?
>
> Command:
> $ iptables -t raw -I PREROUTING -p udp --dport 9 --j DROP
>
>> NIC configuration:
>> the command
>> "ethtool -N p3p2 flow-type udp4 src-port 4242 dst-port 4242 action 16"
>> doesn't work on my ixgbe driver, so I use ntuple:
>>
>> ethtool -K enp10s0f0 ntuple on
>> ethtool -U enp10s0f0 flow-type udp4 src-ip 10.1.1.100 action 1
>> then
>> echo 1 > /proc/sys/net/core/bpf_jit_enable
>> ./xdpsock -i enp10s0f0 -r -S --queue=1
>>
>> I also take a look at perf result:
>> For rxdrop:
>> 86.56% xdpsock xdpsock [.] main
>> 9.22% xdpsock [kernel.vmlinux] [k] nmi
>> 4.23% xdpsock xdpsock [.] xq_enq
>
> It looks very strange that you see non-maskable interrupt's (NMI) being
> this high...
>
>
>> For l2fwd:
>> 20.81% xdpsock xdpsock [.] main
>> 10.64% xdpsock [kernel.vmlinux] [k] clflush_cache_range
>
> Oh, clflush_cache_range is being called!
> Do your system use an IOMMU ?
Whats the implication here. Should IOMMU be disabled?
I'm asking because I do see a huge difference while running pktgen test
for my performance benchmarks, with and without intel_iommu.
-Tushar
>
>> 8.46% xdpsock [kernel.vmlinux] [k] xsk_sendmsg
>> 6.72% xdpsock [kernel.vmlinux] [k] skb_set_owner_w
>> 5.89% xdpsock [kernel.vmlinux] [k] __domain_mapping
>> 5.74% xdpsock [kernel.vmlinux] [k] alloc_skb_with_frags
>> 4.62% xdpsock [kernel.vmlinux] [k] netif_skb_features
>> 3.96% xdpsock [kernel.vmlinux] [k] ___slab_alloc
>> 3.18% xdpsock [kernel.vmlinux] [k] nmi
>
> Again high count for NMI ?!?
>
> Maybe you just forgot to tell perf that you want it to decode the
> bpf_prog correctly?
>
> https://prototype-kernel.readthedocs.io/en/latest/bpf/troubleshooting.html#perf-tool-symbols
>
> Enable via:
> $ sysctl net/core/bpf_jit_kallsyms=1
>
> And use perf report (while BPF is STILL LOADED):
>
> $ perf report --kallsyms=/proc/kallsyms
>
> E.g. for emailing this you can use this command:
>
> $ perf report --sort cpu,comm,dso,symbol --kallsyms=/proc/kallsyms --no-children --stdio -g none | head -n 40
>
>
>> I observed that the i40e's XDP_SKB result is much better than my ixgbe's result.
>> I wonder in XDP_SKB mode, does the driver make performance difference?
>> Or my cpu (E5-2440 v2 @ 1.90GHz) is too old?
>
> I suspect some setup issue on your system.
>
^ permalink raw reply
* Re: Bug#892057: Fwd: Re: TS-x09 fails to boot when obtaining MAC
From: Andrew Lunn @ 2018-03-26 22:58 UTC (permalink / raw)
To: David Miller; +Cc: 892057, netdev, Martin Michlmayr
In-Reply-To: <20180326225003.m6gzmft27b4tyqk7@jirafa.cyrius.com>
On Tue, Mar 27, 2018 at 12:50:04AM +0200, Martin Michlmayr wrote:
> The fix is in Linus' tree since v.16-rc5:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=13a55372b64e00e564a08d785ca87bd9d454ba30
>
> I don't see it in 4.9 stable or the stable queue. Will Greg pick it
> up automatically because of the Fixes: info or do we have to let him
> know?
Hi Dave
This is one of your own patches. Please could you add it to stable, if
it is not already.
Thanks
Andrew
^ permalink raw reply
* [PATCH v2 bpf-next] bpf, tracing: unbreak lttng
From: Alexei Starovoitov @ 2018-03-26 23:02 UTC (permalink / raw)
To: davem
Cc: daniel, torvalds, peterz, rostedt, mathieu.desnoyers, netdev,
kernel-team, linux-api
for_each_kernel_tracepoint() is used by out-of-tree lttng module
and therefore cannot be changed.
Instead introduce kernel_tracepoint_find_by_name() to find
tracepoint by name.
Fixes: 9e9afbae6514 ("tracepoint: compute num_args at build time")
Signed-off-by: Alexei Starovoitov <ast@kernel.org>
---
v1->v2: fix 'undef CONFIG_TRACEPOINTS' build as spotted by Mathieu
---
include/linux/tracepoint.h | 13 ++++++++-----
kernel/bpf/syscall.c | 11 +----------
kernel/tracepoint.c | 36 ++++++++++++++++++++----------------
3 files changed, 29 insertions(+), 31 deletions(-)
diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
index 2194e7c31484..d578a962091e 100644
--- a/include/linux/tracepoint.h
+++ b/include/linux/tracepoint.h
@@ -42,13 +42,16 @@ extern int
tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data);
#ifdef CONFIG_TRACEPOINTS
-void *
-for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
+void
+for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
void *priv);
+struct tracepoint *kernel_tracepoint_find_by_name(const char *name);
#else
-static inline void *
-for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
- void *priv)
+static inline void
+for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
+ void *priv) {}
+static inline struct tracepoint *
+kernel_tracepoint_find_by_name(const char *name)
{
return NULL;
}
diff --git a/kernel/bpf/syscall.c b/kernel/bpf/syscall.c
index ae8b43f1cee3..644311777d8e 100644
--- a/kernel/bpf/syscall.c
+++ b/kernel/bpf/syscall.c
@@ -1334,15 +1334,6 @@ static const struct file_operations bpf_raw_tp_fops = {
.write = bpf_dummy_write,
};
-static void *__find_tp(struct tracepoint *tp, void *priv)
-{
- char *name = priv;
-
- if (!strcmp(tp->name, name))
- return tp;
- return NULL;
-}
-
#define BPF_RAW_TRACEPOINT_OPEN_LAST_FIELD raw_tracepoint.prog_fd
static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
@@ -1358,7 +1349,7 @@ static int bpf_raw_tracepoint_open(const union bpf_attr *attr)
return -EFAULT;
tp_name[sizeof(tp_name) - 1] = 0;
- tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+ tp = kernel_tracepoint_find_by_name(tp_name);
if (!tp)
return -ENOENT;
diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
index 3f2dc5738c2b..764d02fbe782 100644
--- a/kernel/tracepoint.c
+++ b/kernel/tracepoint.c
@@ -502,22 +502,17 @@ static __init int init_tracepoints(void)
__initcall(init_tracepoints);
#endif /* CONFIG_MODULES */
-static void *for_each_tracepoint_range(struct tracepoint * const *begin,
- struct tracepoint * const *end,
- void *(*fct)(struct tracepoint *tp, void *priv),
- void *priv)
+static void for_each_tracepoint_range(struct tracepoint * const *begin,
+ struct tracepoint * const *end,
+ void (*fct)(struct tracepoint *tp, void *priv),
+ void *priv)
{
struct tracepoint * const *iter;
- void *ret;
if (!begin)
- return NULL;
- for (iter = begin; iter < end; iter++) {
- ret = fct(*iter, priv);
- if (ret)
- return ret;
- }
- return NULL;
+ return;
+ for (iter = begin; iter < end; iter++)
+ fct(*iter, priv);
}
/**
@@ -525,14 +520,23 @@ static void *for_each_tracepoint_range(struct tracepoint * const *begin,
* @fct: callback
* @priv: private data
*/
-void *for_each_kernel_tracepoint(void *(*fct)(struct tracepoint *tp, void *priv),
- void *priv)
+void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
+ void *priv)
{
- return for_each_tracepoint_range(__start___tracepoints_ptrs,
- __stop___tracepoints_ptrs, fct, priv);
+ for_each_tracepoint_range(__start___tracepoints_ptrs,
+ __stop___tracepoints_ptrs, fct, priv);
}
EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
+struct tracepoint *kernel_tracepoint_find_by_name(const char *name)
+{
+ struct tracepoint * const *tp = __start___tracepoints_ptrs;
+
+ for (; tp < __stop___tracepoints_ptrs; tp++)
+ if (!strcmp((*tp)->name, name))
+ return *tp;
+ return NULL;
+}
#ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
/* NB: reg/unreg are called while guarded with the tracepoints_mutex */
--
2.9.5
^ permalink raw reply related
* Re: [RFC PATCH 00/24] Introducing AF_XDP support
From: Alexander Duyck @ 2018-03-26 23:03 UTC (permalink / raw)
To: Tushar Dave
Cc: Jesper Dangaard Brouer, William Tu, Björn Töpel,
Karlsson, Magnus, Alexander Duyck, John Fastabend,
Alexei Starovoitov, Willem de Bruijn, Daniel Borkmann,
Linux Kernel Network Developers, Björn Töpel,
michael.lundkvist, Brandeburg, Jesse, Anjali Singhai Jain,
jeffrey.b.shaw, ferruh.yigit, qi.z.zhang
In-Reply-To: <ad766caf-4656-befa-d55c-92d9c943fa15@oracle.com>
On Mon, Mar 26, 2018 at 3:54 PM, Tushar Dave <tushar.n.dave@oracle.com> wrote:
>
>
> On 03/26/2018 09:38 AM, Jesper Dangaard Brouer wrote:
>>
>>
>> On Mon, 26 Mar 2018 09:06:54 -0700 William Tu <u9012063@gmail.com> wrote:
>>
>>> On Wed, Jan 31, 2018 at 5:53 AM, Björn Töpel <bjorn.topel@gmail.com>
>>> wrote:
>>>>
>>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>>
>>>> This RFC introduces a new address family called AF_XDP that is
>>>> optimized for high performance packet processing and zero-copy
>>>> semantics. Throughput improvements can be up to 20x compared to V2 and
>>>> V3 for the micro benchmarks included. Would be great to get your
>>>> feedback on it. Note that this is the follow up RFC to AF_PACKET V4
>>>> from November last year. The feedback from that RFC submission and the
>>>> presentation at NetdevConf in Seoul was to create a new address family
>>>> instead of building on top of AF_PACKET. AF_XDP is this new address
>>>> family.
>>>>
>>>> The main difference between AF_XDP and AF_PACKET V2/V3 on a descriptor
>>>> level is that TX and RX descriptors are separated from packet
>>>> buffers. An RX or TX descriptor points to a data buffer in a packet
>>>> buffer area. RX and TX can share the same packet buffer so that a
>>>> packet does not have to be copied between RX and TX. Moreover, if a
>>>> packet needs to be kept for a while due to a possible retransmit, then
>>>> the descriptor that points to that packet buffer can be changed to
>>>> point to another buffer and reused right away. This again avoids
>>>> copying data.
>>>>
>>>> The RX and TX descriptor rings are registered with the setsockopts
>>>> XDP_RX_RING and XDP_TX_RING, similar to AF_PACKET. The packet buffer
>>>> area is allocated by user space and registered with the kernel using
>>>> the new XDP_MEM_REG setsockopt. All these three areas are shared
>>>> between user space and kernel space. The socket is then bound with a
>>>> bind() call to a device and a specific queue id on that device, and it
>>>> is not until bind is completed that traffic starts to flow.
>>>>
>>>> An XDP program can be loaded to direct part of the traffic on that
>>>> device and queue id to user space through a new redirect action in an
>>>> XDP program called bpf_xdpsk_redirect that redirects a packet up to
>>>> the socket in user space. All the other XDP actions work just as
>>>> before. Note that the current RFC requires the user to load an XDP
>>>> program to get any traffic to user space (for example all traffic to
>>>> user space with the one-liner program "return
>>>> bpf_xdpsk_redirect();"). We plan on introducing a patch that removes
>>>> this requirement and sends all traffic from a queue to user space if
>>>> an AF_XDP socket is bound to it.
>>>>
>>>> AF_XDP can operate in three different modes: XDP_SKB, XDP_DRV, and
>>>> XDP_DRV_ZC (shorthand for XDP_DRV with a zero-copy allocator as there
>>>> is no specific mode called XDP_DRV_ZC). If the driver does not have
>>>> support for XDP, or XDP_SKB is explicitly chosen when loading the XDP
>>>> program, XDP_SKB mode is employed that uses SKBs together with the
>>>> generic XDP support and copies out the data to user space. A fallback
>>>> mode that works for any network device. On the other hand, if the
>>>> driver has support for XDP (all three NDOs: ndo_bpf, ndo_xdp_xmit and
>>>> ndo_xdp_flush), these NDOs, without any modifications, will be used by
>>>> the AF_XDP code to provide better performance, but there is still a
>>>> copy of the data into user space. The last mode, XDP_DRV_ZC, is XDP
>>>> driver support with the zero-copy user space allocator that provides
>>>> even better performance. In this mode, the networking HW (or SW driver
>>>> if it is a virtual driver like veth) DMAs/puts packets straight into
>>>> the packet buffer that is shared between user space and kernel
>>>> space. The RX and TX descriptor queues of the networking HW are NOT
>>>> shared to user space. Only the kernel can read and write these and it
>>>> is the kernel driver's responsibility to translate these HW specific
>>>> descriptors to the HW agnostic ones in the virtual descriptor rings
>>>> that user space sees. This way, a malicious user space program cannot
>>>> mess with the networking HW. This mode though requires some extensions
>>>> to XDP.
>>>>
>>>> To get the XDP_DRV_ZC mode to work for RX, we chose to introduce a
>>>> buffer pool concept so that the same XDP driver code can be used for
>>>> buffers allocated using the page allocator (XDP_DRV), the user-space
>>>> zero-copy allocator (XDP_DRV_ZC), or some internal driver specific
>>>> allocator/cache/recycling mechanism. The ndo_bpf call has also been
>>>> extended with two commands for registering and unregistering an XSK
>>>> socket and is in the RX case mainly used to communicate some
>>>> information about the user-space buffer pool to the driver.
>>>>
>>>> For the TX path, our plan was to use ndo_xdp_xmit and ndo_xdp_flush,
>>>> but we run into problems with this (further discussion in the
>>>> challenges section) and had to introduce a new NDO called
>>>> ndo_xdp_xmit_xsk (xsk = XDP socket). It takes a pointer to a netdevice
>>>> and an explicit queue id that packets should be sent out on. In
>>>> contrast to ndo_xdp_xmit, it is asynchronous and pulls packets to be
>>>> sent from the xdp socket (associated with the dev and queue
>>>> combination that was provided with the NDO call) using a callback
>>>> (get_tx_packet), and when they have been transmitted it uses another
>>>> callback (tx_completion) to signal completion of packets. These
>>>> callbacks are set via ndo_bpf in the new XDP_REGISTER_XSK
>>>> command. ndo_xdp_xmit_xsk is exclusively used by the XDP socket code
>>>> and thus does not clash with the XDP_REDIRECT use of
>>>> ndo_xdp_xmit. This is one of the reasons that the XDP_DRV mode
>>>> (without ZC) is currently not supported by TX. Please have a look at
>>>> the challenges section for further discussions.
>>>>
>>>> The AF_XDP bind call acts on a queue pair (channel in ethtool speak),
>>>> so the user needs to steer the traffic to the zero-copy enabled queue
>>>> pair. Which queue to use, is up to the user.
>>>>
>>>> For an untrusted application, HW packet steering to a specific queue
>>>> pair (the one associated with the application) is a requirement, as
>>>> the application would otherwise be able to see other user space
>>>> processes' packets. If the HW cannot support the required packet
>>>> steering, XDP_DRV or XDP_SKB mode have to be used as they do not
>>>> expose the NIC's packet buffer into user space as the packets are
>>>> copied into user space from the NIC's packet buffer in the kernel.
>>>>
>>>> There is a xdpsock benchmarking/test application included. Say that
>>>> you would like your UDP traffic from port 4242 to end up in queue 16,
>>>> that we will enable AF_XDP on. Here, we use ethtool for this:
>>>>
>>>> ethtool -N p3p2 rx-flow-hash udp4 fn
>>>> ethtool -N p3p2 flow-type udp4 src-port 4242 dst-port 4242 \
>>>> action 16
>>>>
>>>> Running the l2fwd benchmark in XDP_DRV_ZC mode can then be done using:
>>>>
>>>> samples/bpf/xdpsock -i p3p2 -q 16 -l -N
>>>>
>>>> For XDP_SKB mode, use the switch "-S" instead of "-N" and all options
>>>> can be displayed with "-h", as usual.
>>>>
>>>> We have run some benchmarks on a dual socket system with two Broadwell
>>>> E5 2660 @ 2.0 GHz with hyperthreading turned off. Each socket has 14
>>>> cores which gives a total of 28, but only two cores are used in these
>>>> experiments. One for TR/RX and one for the user space application. The
>>>> memory is DDR4 @ 2133 MT/s (1067 MHz) and the size of each DIMM is
>>>> 8192MB and with 8 of those DIMMs in the system we have 64 GB of total
>>>> memory. The compiler used is gcc version 5.4.0 20160609. The NIC is an
>>>> Intel I40E 40Gbit/s using the i40e driver.
>>>>
>>>> Below are the results in Mpps of the I40E NIC benchmark runs for 64
>>>> byte packets, generated by commercial packet generator HW that is
>>>> generating packets at full 40 Gbit/s line rate.
>>>>
>>>> XDP baseline numbers without this RFC:
>>>> xdp_rxq_info --action XDP_DROP 31.3 Mpps
>>>> xdp_rxq_info --action XDP_TX 16.7 Mpps
>>>>
>>>> XDP performance with this RFC i.e. with the buffer allocator:
>>>> XDP_DROP 21.0 Mpps
>>>> XDP_TX 11.9 Mpps
>>>>
>>>> AF_PACKET V4 performance from previous RFC on 4.14-rc7:
>>>> Benchmark V2 V3 V4 V4+ZC
>>>> rxdrop 0.67 0.73 0.74 33.7
>>>> txpush 0.98 0.98 0.91 19.6
>>>> l2fwd 0.66 0.71 0.67 15.5
>>>>
>>>> AF_XDP performance:
>>>> Benchmark XDP_SKB XDP_DRV XDP_DRV_ZC (all in Mpps)
>>>> rxdrop 3.3 11.6 16.9
>>>> txpush 2.2 NA* 21.8
>>>> l2fwd 1.7 NA* 10.4
>>>>
>>>
>>>
>>> Hi,
>>> I also did an evaluation of AF_XDP, however the performance isn't as
>>> good as above.
>>> I'd like to share the result and see if there are some tuning
>>> suggestions.
>>>
>>> System:
>>> 16 core, Intel(R) Xeon(R) CPU E5-2440 v2 @ 1.90GHz
>>> Intel 10G X540-AT2 ---> so I can only run XDP_SKB mode
>>
>>
>> Hmmm, why is X540-AT2 not able to use XDP natively?
>>
>>> AF_XDP performance:
>>> Benchmark XDP_SKB
>>> rxdrop 1.27 Mpps
>>> txpush 0.99 Mpps
>>> l2fwd 0.85 Mpps
>>
>>
>> Definitely too low...
>>
>> What is the performance if you drop packets via iptables?
>>
>> Command:
>> $ iptables -t raw -I PREROUTING -p udp --dport 9 --j DROP
>>
>>> NIC configuration:
>>> the command
>>> "ethtool -N p3p2 flow-type udp4 src-port 4242 dst-port 4242 action 16"
>>> doesn't work on my ixgbe driver, so I use ntuple:
>>>
>>> ethtool -K enp10s0f0 ntuple on
>>> ethtool -U enp10s0f0 flow-type udp4 src-ip 10.1.1.100 action 1
>>> then
>>> echo 1 > /proc/sys/net/core/bpf_jit_enable
>>> ./xdpsock -i enp10s0f0 -r -S --queue=1
>>>
>>> I also take a look at perf result:
>>> For rxdrop:
>>> 86.56% xdpsock xdpsock [.] main
>>> 9.22% xdpsock [kernel.vmlinux] [k] nmi
>>> 4.23% xdpsock xdpsock [.] xq_enq
>>
>>
>> It looks very strange that you see non-maskable interrupt's (NMI) being
>> this high...
>>
>>
>>>
>>> For l2fwd:
>>> 20.81% xdpsock xdpsock [.] main
>>> 10.64% xdpsock [kernel.vmlinux] [k] clflush_cache_range
>>
>>
>> Oh, clflush_cache_range is being called!
>> Do your system use an IOMMU ?
>
>
> Whats the implication here. Should IOMMU be disabled?
> I'm asking because I do see a huge difference while running pktgen test for
> my performance benchmarks, with and without intel_iommu.
>
>
> -Tushar
For the Intel parts the IOMMU can be expensive primarily for Tx, since
it should have minimal impact if the Rx pages are pinned/recycled. I
am assuming the same is true here for AF_XDP, Bjorn can correct me if
I am wrong.
Basically the IOMMU can make creating/destroying a DMA mapping really
expensive. The easiest way to work around it in the case of the Intel
IOMMU is to boot with "iommu=pt" which will create an identity mapping
for the host. The downside is though that you then have the entire
system accessible to the device unless a new mapping is created for it
by assigning it to a new IOMMU domain.
Thanks.
- Alex
^ permalink raw reply
* Re: [PATCH net-next 1/2] net: systemport: Implement adaptive interrupt coalescing
From: Tal Gilboa @ 2018-03-26 23:07 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: davem, jaedon.shin, pgynther, opendmb, michal.chan, gospo, saeedm
In-Reply-To: <0056b0e3-7672-16ad-725b-8b2bc22b97fd@gmail.com>
On 3/27/2018 12:36 AM, Florian Fainelli wrote:
> On 03/26/2018 02:22 PM, Tal Gilboa wrote:
>> On 3/23/2018 4:19 AM, Florian Fainelli wrote:
>>> Implement support for adaptive RX and TX interrupt coalescing using
>>> net_dim. We have each of our TX ring and our single RX ring implement a
>>> bcm_sysport_net_dim structure which holds an interrupt counter, number
>>> of packets, bytes, and a container for a net_dim instance.
>>>
>>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>>> ---
>>> drivers/net/ethernet/broadcom/bcmsysport.c | 141
>>> ++++++++++++++++++++++++++---
>>> drivers/net/ethernet/broadcom/bcmsysport.h | 14 +++
>>> 2 files changed, 140 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/broadcom/bcmsysport.c
>>> b/drivers/net/ethernet/broadcom/bcmsysport.c
>>> index f15a8fc6dfc9..5a5a726bafa4 100644
>>> --- a/drivers/net/ethernet/broadcom/bcmsysport.c
>>> +++ b/drivers/net/ethernet/broadcom/bcmsysport.c
>>> @@ -15,6 +15,7 @@
>>> #include <linux/module.h>
>>> #include <linux/kernel.h>
>>> #include <linux/netdevice.h>
>>> +#include <linux/net_dim.h>
>>
>> I don't think you need this include. You already include net_dim in
>> bcmsysport.h and include the bcmsysport.h here.
>
> Indeed.
>
>>
>>> #include <linux/etherdevice.h>
>>> #include <linux/platform_device.h>
>>> #include <linux/of.h>
>>> @@ -574,21 +575,55 @@ static int bcm_sysport_set_wol(struct net_device
>>> *dev,
>>> return 0;
>>> }
>>> +static void bcm_sysport_set_rx_coalesce(struct bcm_sysport_priv *priv)
>>> +{
>>> + u32 reg;
>>> +
>>> + reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>>> + reg &= ~(RDMA_INTR_THRESH_MASK |
>>> + RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
>>> + reg |= priv->dim.coal_pkts;
>>> + reg |= DIV_ROUND_UP(priv->dim.coal_usecs * 1000, 8192) <<
>>> + RDMA_TIMEOUT_SHIFT;
>>> + rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>>> +}
>>> +
>>> +static void bcm_sysport_set_tx_coalesce(struct bcm_sysport_tx_ring
>>> *ring)
>>> +{
>>> + struct bcm_sysport_priv *priv = ring->priv;
>>> + u32 reg;
>>> +
>>> + reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(ring->index));
>>> + reg &= ~(RING_INTR_THRESH_MASK |
>>> + RING_TIMEOUT_MASK << RING_TIMEOUT_SHIFT);
>>> + reg |= ring->dim.coal_pkts;
>>> + reg |= DIV_ROUND_UP(ring->dim.coal_usecs * 1000, 8192) <<
>>> + RING_TIMEOUT_SHIFT;
>>> + tdma_writel(priv, reg, TDMA_DESC_RING_INTR_CONTROL(ring->index));
>>> +}
>>> +
>>
>> I wouldn't couple these functions with dim. This implies dim is always
>> used. IMO, would be more clear to use a generic method which takes usecs
>> and packets as an argument.
>
> I did not want to create an additional structure for storing coalescing
> parameters, but if you prefer I make this function take two parameters,
> that sounds entirely reasonable.
>
>>
>>> static int bcm_sysport_get_coalesce(struct net_device *dev,
>>> struct ethtool_coalesce *ec)
>>> {
>>> struct bcm_sysport_priv *priv = netdev_priv(dev);
>>> + struct bcm_sysport_tx_ring *ring;
>>> + unsigned int i;
>>> u32 reg;
>>> reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(0));
>>> ec->tx_coalesce_usecs = (reg >> RING_TIMEOUT_SHIFT) * 8192 /
>>> 1000;
>>> ec->tx_max_coalesced_frames = reg & RING_INTR_THRESH_MASK;
>>> + for (i = 0; i < dev->num_tx_queues; i++) {
>>> + ring = &priv->tx_rings[i];
>>> + ec->use_adaptive_tx_coalesce |= ring->dim.use_dim;
>>> + }
>>> reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>>> ec->rx_coalesce_usecs = (reg >> RDMA_TIMEOUT_SHIFT) * 8192 /
>>> 1000;
>>> ec->rx_max_coalesced_frames = reg & RDMA_INTR_THRESH_MASK;
>>> + ec->use_adaptive_rx_coalesce = priv->dim.use_dim;
>>> return 0;
>>> }
>>> @@ -597,8 +632,8 @@ static int bcm_sysport_set_coalesce(struct
>>> net_device *dev,
>>> struct ethtool_coalesce *ec)
>>> {
>>> struct bcm_sysport_priv *priv = netdev_priv(dev);
>>> + struct bcm_sysport_tx_ring *ring;
>>> unsigned int i;
>>> - u32 reg;
>>> /* Base system clock is 125Mhz, DMA timeout is this reference
>>> clock
>>> * divided by 1024, which yield roughly 8.192 us, our maximum
>>> value has
>>> @@ -615,22 +650,26 @@ static int bcm_sysport_set_coalesce(struct
>>> net_device *dev,
>>> return -EINVAL;
>>> for (i = 0; i < dev->num_tx_queues; i++) {
>>> - reg = tdma_readl(priv, TDMA_DESC_RING_INTR_CONTROL(i));
>>> - reg &= ~(RING_INTR_THRESH_MASK |
>>> - RING_TIMEOUT_MASK << RING_TIMEOUT_SHIFT);
>>> - reg |= ec->tx_max_coalesced_frames;
>>> - reg |= DIV_ROUND_UP(ec->tx_coalesce_usecs * 1000, 8192) <<
>>> - RING_TIMEOUT_SHIFT;
>>> - tdma_writel(priv, reg, TDMA_DESC_RING_INTR_CONTROL(i));
>>> + ring = &priv->tx_rings[i];
>>> + ring->dim.coal_pkts = ec->tx_max_coalesced_frames;
>>> + ring->dim.coal_usecs = ec->tx_coalesce_usecs;
>>> + if (!ec->use_adaptive_tx_coalesce && ring->dim.use_dim) {
>>> + ring->dim.coal_pkts = 1;
>>> + ring->dim.coal_usecs = 0;
>>> + }
>>> + ring->dim.use_dim = ec->use_adaptive_tx_coalesce;
>>> + bcm_sysport_set_tx_coalesce(ring);
>>> }
>>
>> If I understand correctly, if I disable dim, moderation is set to
>> {usecs,packets}={0,1} regardless of the input from ethtool right?
>
> Correct, these are the default coalescing parameters that the driver
> sets. As mentioned before, since I am not storing any coalescing
> parameters other than these two, there is no copy of what an user might
> have previously provided, falling back to the defaults seemed reasonable.
Consider this example: ethtool -C <int> adaptive-tx on; ethtool -C
<intf> adaptive-tx off tx-usecs 8 tx-frames 32;
In this case the actual moderation would be {0,1} instead of the
requested {8,32}. Setting default values is ok unless requested
otherwise. I would also use macros for default values.
>
>> Doesn't this break the wanted behavior? As mentioned above, I would
>> decouple dim from the set_tx/rx_coalesce() function. Also, when dim is
>> enabled, why change dim.coal_pkts/usecs? They would just be overwritten
>> in the next iteration of net_dim.
>
> Indeed, that is not necessary.
>
>>
>>> - reg = rdma_readl(priv, RDMA_MBDONE_INTR);
>>> - reg &= ~(RDMA_INTR_THRESH_MASK |
>>> - RDMA_TIMEOUT_MASK << RDMA_TIMEOUT_SHIFT);
>>> - reg |= ec->rx_max_coalesced_frames;
>>> - reg |= DIV_ROUND_UP(ec->rx_coalesce_usecs * 1000, 8192) <<
>>> - RDMA_TIMEOUT_SHIFT;
>>> - rdma_writel(priv, reg, RDMA_MBDONE_INTR);
>>> + priv->dim.coal_usecs = ec->rx_coalesce_usecs;
>>> + priv->dim.coal_pkts = ec->rx_max_coalesced_frames;
>>> +
>>> + if (!ec->use_adaptive_rx_coalesce && priv->dim.use_dim) {
>>> + priv->dim.coal_pkts = 1;
>>> + priv->dim.coal_usecs = 0;
>>> + }
>>> + priv->dim.use_dim = ec->use_adaptive_rx_coalesce;
>>> + bcm_sysport_set_rx_coalesce(priv);
>>
>> Same comment as above.
>>
>>> return 0;
>>> }
>>> @@ -709,6 +748,7 @@ static unsigned int bcm_sysport_desc_rx(struct
>>> bcm_sysport_priv *priv,
>>> struct bcm_sysport_stats64 *stats64 = &priv->stats64;
>>> struct net_device *ndev = priv->netdev;
>>> unsigned int processed = 0, to_process;
>>> + unsigned int processed_bytes = 0;
>>> struct bcm_sysport_cb *cb;
>>> struct sk_buff *skb;
>>> unsigned int p_index;
>>> @@ -800,6 +840,7 @@ static unsigned int bcm_sysport_desc_rx(struct
>>> bcm_sysport_priv *priv,
>>> */
>>> skb_pull(skb, sizeof(*rsb) + 2);
>>> len -= (sizeof(*rsb) + 2);
>>> + processed_bytes += len;
>>> /* UniMAC may forward CRC */
>>> if (priv->crc_fwd) {
>>> @@ -824,6 +865,9 @@ static unsigned int bcm_sysport_desc_rx(struct
>>> bcm_sysport_priv *priv,
>>> priv->rx_read_ptr = 0;
>>> }
>>> + priv->dim.packets = processed;
>>> + priv->dim.bytes = processed_bytes;
>>> +
>>> return processed;
>>> }
>>> @@ -900,6 +944,8 @@ static unsigned int
>>> __bcm_sysport_tx_reclaim(struct bcm_sysport_priv *priv,
>>> ring->packets += pkts_compl;
>>> ring->bytes += bytes_compl;
>>> u64_stats_update_end(&priv->syncp);
>>> + ring->dim.packets = pkts_compl;
>>> + ring->dim.bytes = bytes_compl;
>>> ring->c_index = c_index;
>>> @@ -945,6 +991,7 @@ static int bcm_sysport_tx_poll(struct
>>> napi_struct *napi, int budget)
>>> {
>>> struct bcm_sysport_tx_ring *ring =
>>> container_of(napi, struct bcm_sysport_tx_ring, napi);
>>> + struct net_dim_sample dim_sample;
>>> unsigned int work_done = 0;
>>> work_done = bcm_sysport_tx_reclaim(ring->priv, ring);
>>> @@ -961,6 +1008,12 @@ static int bcm_sysport_tx_poll(struct
>>> napi_struct *napi, int budget)
>>> return 0;
>>> }
>>> + if (ring->dim.use_dim) {
>>> + net_dim_sample(ring->dim.event_ctr, ring->dim.packets,
>>> + ring->dim.bytes, &dim_sample);
>>> + net_dim(&ring->dim.dim, dim_sample);
>>> + }
>>> +
>>> return budget;
>>> }
>>> @@ -976,6 +1029,7 @@ static int bcm_sysport_poll(struct napi_struct
>>> *napi, int budget)
>>> {
>>> struct bcm_sysport_priv *priv =
>>> container_of(napi, struct bcm_sysport_priv, napi);
>>> + struct net_dim_sample dim_sample;
>>> unsigned int work_done = 0;
>>> work_done = bcm_sysport_desc_rx(priv, budget);
>>> @@ -998,6 +1052,12 @@ static int bcm_sysport_poll(struct napi_struct
>>> *napi, int budget)
>>> intrl2_0_mask_clear(priv, INTRL2_0_RDMA_MBDONE);
>>> }
>>> + if (priv->dim.use_dim) {
>>> + net_dim_sample(priv->dim.event_ctr, priv->dim.packets,
>>> + priv->dim.bytes, &dim_sample);
>>> + net_dim(&priv->dim.dim, dim_sample);
>>> + }
>>> +
>>> return work_done;
>>> }
>>> @@ -1016,6 +1076,40 @@ static void
>>> bcm_sysport_resume_from_wol(struct bcm_sysport_priv *priv)
>>> netif_dbg(priv, wol, priv->netdev, "resumed from WOL\n");
>>> }
>>> +static void bcm_sysport_dim_work(struct work_struct *work)
>>> +{
>>> + struct net_dim *dim = container_of(work, struct net_dim, work);
>>> + struct bcm_sysport_net_dim *ndim =
>>> + container_of(dim, struct bcm_sysport_net_dim, dim);
>>> + struct bcm_sysport_priv *priv =
>>> + container_of(ndim, struct bcm_sysport_priv, dim);
>>> + struct net_dim_cq_moder cur_profile =
>>> + net_dim_get_profile(dim->mode, dim->profile_ix);
>>> +
>>> + priv->dim.coal_usecs = cur_profile.usec;
>>> + priv->dim.coal_pkts = cur_profile.pkts;
>>> +
>>> + bcm_sysport_set_rx_coalesce(priv);
>>> + dim->state = NET_DIM_START_MEASURE;
>>> +}
>>> +
>>> +static void bcm_sysport_dim_tx_work(struct work_struct *work)
>>> +{
>>> + struct net_dim *dim = container_of(work, struct net_dim, work);
>>> + struct bcm_sysport_net_dim *ndim =
>>> + container_of(dim, struct bcm_sysport_net_dim, dim);
>>> + struct bcm_sysport_tx_ring *ring =
>>> + container_of(ndim, struct bcm_sysport_tx_ring, dim);
>>> + struct net_dim_cq_moder cur_profile =
>>> + net_dim_get_profile(dim->mode, dim->profile_ix);
>>> +
>>> + ring->dim.coal_usecs = cur_profile.usec;
>>> + ring->dim.coal_pkts = cur_profile.pkts;
>>> +
>>> + bcm_sysport_set_tx_coalesce(ring);
>>> + dim->state = NET_DIM_START_MEASURE;
>>> +}
>>> +
>>> /* RX and misc interrupt routine */
>>> static irqreturn_t bcm_sysport_rx_isr(int irq, void *dev_id)
>>> {
>>> @@ -1034,6 +1128,7 @@ static irqreturn_t bcm_sysport_rx_isr(int irq,
>>> void *dev_id)
>>> }
>>> if (priv->irq0_stat & INTRL2_0_RDMA_MBDONE) {
>>> + priv->dim.event_ctr++;
>>> if (likely(napi_schedule_prep(&priv->napi))) {
>>> /* disable RX interrupts */
>>> intrl2_0_mask_set(priv, INTRL2_0_RDMA_MBDONE);
>>> @@ -1061,6 +1156,7 @@ static irqreturn_t bcm_sysport_rx_isr(int irq,
>>> void *dev_id)
>>> continue;
>>> txr = &priv->tx_rings[ring];
>>> + txr->dim.event_ctr++;
>>> if (likely(napi_schedule_prep(&txr->napi))) {
>>> intrl2_0_mask_set(priv, ring_bit);
>>> @@ -1093,6 +1189,7 @@ static irqreturn_t bcm_sysport_tx_isr(int irq,
>>> void *dev_id)
>>> continue;
>>> txr = &priv->tx_rings[ring];
>>> + txr->dim.event_ctr++;
>>> if (likely(napi_schedule_prep(&txr->napi))) {
>>> intrl2_1_mask_set(priv, BIT(ring));
>>> @@ -1358,6 +1455,16 @@ static void bcm_sysport_adj_link(struct
>>> net_device *dev)
>>> phy_print_status(phydev);
>>> }
>>> +static void bcm_sysport_init_dim(struct bcm_sysport_net_dim *dim,
>>> + void (*cb)(struct work_struct *work))
>>> +{
>>> + INIT_WORK(&dim->dim.work, cb);
>>> + dim->dim.mode = NET_DIM_CQ_PERIOD_MODE_START_FROM_EQE;
>>> + dim->event_ctr = 0;
>>> + dim->packets = 0;
>>> + dim->bytes = 0;
>>> +}
>>
>> What about default values for coal_usecs/pkts? dim supports it through
>> net_dim_get_def_profile(mode) function.
>
> OK, thanks I did not know that.
>
I'll add it to the documentation.
^ permalink raw reply
* Re: [RFC PATCH 00/24] Introducing AF_XDP support
From: Tushar Dave @ 2018-03-26 23:20 UTC (permalink / raw)
To: Alexander Duyck
Cc: Jesper Dangaard Brouer, William Tu, Björn Töpel,
Karlsson, Magnus, Alexander Duyck, John Fastabend,
Alexei Starovoitov, Willem de Bruijn, Daniel Borkmann,
Linux Kernel Network Developers, Björn Töpel,
michael.lundkvist, Brandeburg, Jesse, Anjali Singhai Jain,
jeffrey.b.shaw, ferruh.yigit, qi.z.zhang
In-Reply-To: <CAKgT0Ufya10bXa4BXFbR2GtwZM6i7x8LPYoodFPZ82=S+t7x2w@mail.gmail.com>
On 03/26/2018 04:03 PM, Alexander Duyck wrote:
> On Mon, Mar 26, 2018 at 3:54 PM, Tushar Dave <tushar.n.dave@oracle.com> wrote:
>>
>>
>> On 03/26/2018 09:38 AM, Jesper Dangaard Brouer wrote:
>>>
>>>
>>> On Mon, 26 Mar 2018 09:06:54 -0700 William Tu <u9012063@gmail.com> wrote:
>>>
>>>> On Wed, Jan 31, 2018 at 5:53 AM, Björn Töpel <bjorn.topel@gmail.com>
>>>> wrote:
>>>>>
>>>>> From: Björn Töpel <bjorn.topel@intel.com>
>>>>>
>>>>> This RFC introduces a new address family called AF_XDP that is
>>>>> optimized for high performance packet processing and zero-copy
>>>>> semantics. Throughput improvements can be up to 20x compared to V2 and
>>>>> V3 for the micro benchmarks included. Would be great to get your
>>>>> feedback on it. Note that this is the follow up RFC to AF_PACKET V4
>>>>> from November last year. The feedback from that RFC submission and the
>>>>> presentation at NetdevConf in Seoul was to create a new address family
>>>>> instead of building on top of AF_PACKET. AF_XDP is this new address
>>>>> family.
>>>>>
>>>>> The main difference between AF_XDP and AF_PACKET V2/V3 on a descriptor
>>>>> level is that TX and RX descriptors are separated from packet
>>>>> buffers. An RX or TX descriptor points to a data buffer in a packet
>>>>> buffer area. RX and TX can share the same packet buffer so that a
>>>>> packet does not have to be copied between RX and TX. Moreover, if a
>>>>> packet needs to be kept for a while due to a possible retransmit, then
>>>>> the descriptor that points to that packet buffer can be changed to
>>>>> point to another buffer and reused right away. This again avoids
>>>>> copying data.
>>>>>
>>>>> The RX and TX descriptor rings are registered with the setsockopts
>>>>> XDP_RX_RING and XDP_TX_RING, similar to AF_PACKET. The packet buffer
>>>>> area is allocated by user space and registered with the kernel using
>>>>> the new XDP_MEM_REG setsockopt. All these three areas are shared
>>>>> between user space and kernel space. The socket is then bound with a
>>>>> bind() call to a device and a specific queue id on that device, and it
>>>>> is not until bind is completed that traffic starts to flow.
>>>>>
>>>>> An XDP program can be loaded to direct part of the traffic on that
>>>>> device and queue id to user space through a new redirect action in an
>>>>> XDP program called bpf_xdpsk_redirect that redirects a packet up to
>>>>> the socket in user space. All the other XDP actions work just as
>>>>> before. Note that the current RFC requires the user to load an XDP
>>>>> program to get any traffic to user space (for example all traffic to
>>>>> user space with the one-liner program "return
>>>>> bpf_xdpsk_redirect();"). We plan on introducing a patch that removes
>>>>> this requirement and sends all traffic from a queue to user space if
>>>>> an AF_XDP socket is bound to it.
>>>>>
>>>>> AF_XDP can operate in three different modes: XDP_SKB, XDP_DRV, and
>>>>> XDP_DRV_ZC (shorthand for XDP_DRV with a zero-copy allocator as there
>>>>> is no specific mode called XDP_DRV_ZC). If the driver does not have
>>>>> support for XDP, or XDP_SKB is explicitly chosen when loading the XDP
>>>>> program, XDP_SKB mode is employed that uses SKBs together with the
>>>>> generic XDP support and copies out the data to user space. A fallback
>>>>> mode that works for any network device. On the other hand, if the
>>>>> driver has support for XDP (all three NDOs: ndo_bpf, ndo_xdp_xmit and
>>>>> ndo_xdp_flush), these NDOs, without any modifications, will be used by
>>>>> the AF_XDP code to provide better performance, but there is still a
>>>>> copy of the data into user space. The last mode, XDP_DRV_ZC, is XDP
>>>>> driver support with the zero-copy user space allocator that provides
>>>>> even better performance. In this mode, the networking HW (or SW driver
>>>>> if it is a virtual driver like veth) DMAs/puts packets straight into
>>>>> the packet buffer that is shared between user space and kernel
>>>>> space. The RX and TX descriptor queues of the networking HW are NOT
>>>>> shared to user space. Only the kernel can read and write these and it
>>>>> is the kernel driver's responsibility to translate these HW specific
>>>>> descriptors to the HW agnostic ones in the virtual descriptor rings
>>>>> that user space sees. This way, a malicious user space program cannot
>>>>> mess with the networking HW. This mode though requires some extensions
>>>>> to XDP.
>>>>>
>>>>> To get the XDP_DRV_ZC mode to work for RX, we chose to introduce a
>>>>> buffer pool concept so that the same XDP driver code can be used for
>>>>> buffers allocated using the page allocator (XDP_DRV), the user-space
>>>>> zero-copy allocator (XDP_DRV_ZC), or some internal driver specific
>>>>> allocator/cache/recycling mechanism. The ndo_bpf call has also been
>>>>> extended with two commands for registering and unregistering an XSK
>>>>> socket and is in the RX case mainly used to communicate some
>>>>> information about the user-space buffer pool to the driver.
>>>>>
>>>>> For the TX path, our plan was to use ndo_xdp_xmit and ndo_xdp_flush,
>>>>> but we run into problems with this (further discussion in the
>>>>> challenges section) and had to introduce a new NDO called
>>>>> ndo_xdp_xmit_xsk (xsk = XDP socket). It takes a pointer to a netdevice
>>>>> and an explicit queue id that packets should be sent out on. In
>>>>> contrast to ndo_xdp_xmit, it is asynchronous and pulls packets to be
>>>>> sent from the xdp socket (associated with the dev and queue
>>>>> combination that was provided with the NDO call) using a callback
>>>>> (get_tx_packet), and when they have been transmitted it uses another
>>>>> callback (tx_completion) to signal completion of packets. These
>>>>> callbacks are set via ndo_bpf in the new XDP_REGISTER_XSK
>>>>> command. ndo_xdp_xmit_xsk is exclusively used by the XDP socket code
>>>>> and thus does not clash with the XDP_REDIRECT use of
>>>>> ndo_xdp_xmit. This is one of the reasons that the XDP_DRV mode
>>>>> (without ZC) is currently not supported by TX. Please have a look at
>>>>> the challenges section for further discussions.
>>>>>
>>>>> The AF_XDP bind call acts on a queue pair (channel in ethtool speak),
>>>>> so the user needs to steer the traffic to the zero-copy enabled queue
>>>>> pair. Which queue to use, is up to the user.
>>>>>
>>>>> For an untrusted application, HW packet steering to a specific queue
>>>>> pair (the one associated with the application) is a requirement, as
>>>>> the application would otherwise be able to see other user space
>>>>> processes' packets. If the HW cannot support the required packet
>>>>> steering, XDP_DRV or XDP_SKB mode have to be used as they do not
>>>>> expose the NIC's packet buffer into user space as the packets are
>>>>> copied into user space from the NIC's packet buffer in the kernel.
>>>>>
>>>>> There is a xdpsock benchmarking/test application included. Say that
>>>>> you would like your UDP traffic from port 4242 to end up in queue 16,
>>>>> that we will enable AF_XDP on. Here, we use ethtool for this:
>>>>>
>>>>> ethtool -N p3p2 rx-flow-hash udp4 fn
>>>>> ethtool -N p3p2 flow-type udp4 src-port 4242 dst-port 4242 \
>>>>> action 16
>>>>>
>>>>> Running the l2fwd benchmark in XDP_DRV_ZC mode can then be done using:
>>>>>
>>>>> samples/bpf/xdpsock -i p3p2 -q 16 -l -N
>>>>>
>>>>> For XDP_SKB mode, use the switch "-S" instead of "-N" and all options
>>>>> can be displayed with "-h", as usual.
>>>>>
>>>>> We have run some benchmarks on a dual socket system with two Broadwell
>>>>> E5 2660 @ 2.0 GHz with hyperthreading turned off. Each socket has 14
>>>>> cores which gives a total of 28, but only two cores are used in these
>>>>> experiments. One for TR/RX and one for the user space application. The
>>>>> memory is DDR4 @ 2133 MT/s (1067 MHz) and the size of each DIMM is
>>>>> 8192MB and with 8 of those DIMMs in the system we have 64 GB of total
>>>>> memory. The compiler used is gcc version 5.4.0 20160609. The NIC is an
>>>>> Intel I40E 40Gbit/s using the i40e driver.
>>>>>
>>>>> Below are the results in Mpps of the I40E NIC benchmark runs for 64
>>>>> byte packets, generated by commercial packet generator HW that is
>>>>> generating packets at full 40 Gbit/s line rate.
>>>>>
>>>>> XDP baseline numbers without this RFC:
>>>>> xdp_rxq_info --action XDP_DROP 31.3 Mpps
>>>>> xdp_rxq_info --action XDP_TX 16.7 Mpps
>>>>>
>>>>> XDP performance with this RFC i.e. with the buffer allocator:
>>>>> XDP_DROP 21.0 Mpps
>>>>> XDP_TX 11.9 Mpps
>>>>>
>>>>> AF_PACKET V4 performance from previous RFC on 4.14-rc7:
>>>>> Benchmark V2 V3 V4 V4+ZC
>>>>> rxdrop 0.67 0.73 0.74 33.7
>>>>> txpush 0.98 0.98 0.91 19.6
>>>>> l2fwd 0.66 0.71 0.67 15.5
>>>>>
>>>>> AF_XDP performance:
>>>>> Benchmark XDP_SKB XDP_DRV XDP_DRV_ZC (all in Mpps)
>>>>> rxdrop 3.3 11.6 16.9
>>>>> txpush 2.2 NA* 21.8
>>>>> l2fwd 1.7 NA* 10.4
>>>>>
>>>>
>>>>
>>>> Hi,
>>>> I also did an evaluation of AF_XDP, however the performance isn't as
>>>> good as above.
>>>> I'd like to share the result and see if there are some tuning
>>>> suggestions.
>>>>
>>>> System:
>>>> 16 core, Intel(R) Xeon(R) CPU E5-2440 v2 @ 1.90GHz
>>>> Intel 10G X540-AT2 ---> so I can only run XDP_SKB mode
>>>
>>>
>>> Hmmm, why is X540-AT2 not able to use XDP natively?
>>>
>>>> AF_XDP performance:
>>>> Benchmark XDP_SKB
>>>> rxdrop 1.27 Mpps
>>>> txpush 0.99 Mpps
>>>> l2fwd 0.85 Mpps
>>>
>>>
>>> Definitely too low...
>>>
>>> What is the performance if you drop packets via iptables?
>>>
>>> Command:
>>> $ iptables -t raw -I PREROUTING -p udp --dport 9 --j DROP
>>>
>>>> NIC configuration:
>>>> the command
>>>> "ethtool -N p3p2 flow-type udp4 src-port 4242 dst-port 4242 action 16"
>>>> doesn't work on my ixgbe driver, so I use ntuple:
>>>>
>>>> ethtool -K enp10s0f0 ntuple on
>>>> ethtool -U enp10s0f0 flow-type udp4 src-ip 10.1.1.100 action 1
>>>> then
>>>> echo 1 > /proc/sys/net/core/bpf_jit_enable
>>>> ./xdpsock -i enp10s0f0 -r -S --queue=1
>>>>
>>>> I also take a look at perf result:
>>>> For rxdrop:
>>>> 86.56% xdpsock xdpsock [.] main
>>>> 9.22% xdpsock [kernel.vmlinux] [k] nmi
>>>> 4.23% xdpsock xdpsock [.] xq_enq
>>>
>>>
>>> It looks very strange that you see non-maskable interrupt's (NMI) being
>>> this high...
>>>
>>>
>>>>
>>>> For l2fwd:
>>>> 20.81% xdpsock xdpsock [.] main
>>>> 10.64% xdpsock [kernel.vmlinux] [k] clflush_cache_range
>>>
>>>
>>> Oh, clflush_cache_range is being called!
>>> Do your system use an IOMMU ?
>>
>>
>> Whats the implication here. Should IOMMU be disabled?
>> I'm asking because I do see a huge difference while running pktgen test for
>> my performance benchmarks, with and without intel_iommu.
>>
>>
>> -Tushar
>
> For the Intel parts the IOMMU can be expensive primarily for Tx, since
> it should have minimal impact if the Rx pages are pinned/recycled. I
> am assuming the same is true here for AF_XDP, Bjorn can correct me if
> I am wrong.
Indeed. Intel iommu has least effect on RX because of premap/recycle.
But TX dma map and unmap is really expensive!
>
> Basically the IOMMU can make creating/destroying a DMA mapping really
> expensive. The easiest way to work around it in the case of the Intel
> IOMMU is to boot with "iommu=pt" which will create an identity mapping
> for the host. The downside is though that you then have the entire
> system accessible to the device unless a new mapping is created for it
> by assigning it to a new IOMMU domain.
Yeah thats what I would say, If you really want to use intel iommu and
don't want to hit by performance , use 'iommu=pt'.
Good to have confirmation from you Alex. Thanks.
btw, I don't want to distract this thread on iommu discussion however
even using 'pt' doesn't give you the same performance numbers that you
rather get with intel iommu disabled!
-Tushar
>
> Thanks.
>
> - Alex
>
^ permalink raw reply
* Re: [PATCH net-next 0/2] net: broadcom: Adaptive interrupt coalescing
From: Tal Gilboa @ 2018-03-26 23:21 UTC (permalink / raw)
To: Florian Fainelli, netdev
Cc: davem, jaedon.shin, pgynther, opendmb, michael.chan, gospo,
saeedm
In-Reply-To: <3bacf19a-ae05-0410-f276-2b928b826af7@gmail.com>
On 3/27/2018 1:29 AM, Florian Fainelli wrote:
> On 03/26/2018 03:04 PM, Florian Fainelli wrote:
>> On 03/26/2018 02:16 PM, Tal Gilboa wrote:
>>> On 3/23/2018 4:19 AM, Florian Fainelli wrote:
>>>> Hi all,
>>>>
>>>> This patch series adds adaptive interrupt coalescing for the Gigabit
>>>> Ethernet
>>>> drivers SYSTEMPORT and GENET.
>>>>
>>>> This really helps lower the interrupt count and system load, as
>>>> measured by
>>>> vmstat for a Gigabit TCP RX session:
>>>
>>> I don't see an improvement in system load, the opposite - 42% vs. 100%
>>> for SYSTEMPORT and 85% vs. 100% for GENET. Both with the same bandwidth.
>>
>> Looks like I did not extract the correct data the load could spike in
>> both cases (with and without net_dim) up to 100, but averaged over the
>> transmission I see the following:
>>
>> GENET without:
>> 1 0 0 1169568 0 25556 0 0 0 0 130079 62795 2
>> 86 13 0 0
>>
>> GENET with:
>> 1 0 0 1169536 0 25556 0 0 0 0 10566 10869 1
>> 21 78 0 0
>>
>>> Am I missing something? Talking about bandwidth, I would expect 941Mb/s
>>> (assuming this is TCP over IPv4). Do you know why the reduced interrupt
>>> rate doesn't improve bandwidth?
>>
>> I am assuming that this comes down to a latency, still capturing some
>> pcap files to analyze the TCP session with wireshark and see if that is
>> indeed what is going on. The test machine is actually not that great
I would expect 1GbE full wire speed on almost any setup. I'll try
applying your code on my setup and see what I get.
>>
>>> Also, any effect on the client side (you
>>> mentioned enabling TX moderation for SYSTEMPORT)?
>>
>> Yes, on SYSTEMPORT, being the TCP IPv4 client, I have the following:
>>
>> SYSTEMPORT without:
>> 2 0 0 191428 0 25748 0 0 0 0 86254 264 0 41
>> 59 0 0
>>
>> SYSTEMPORT with:
>> 3 0 0 190176 0 25748 0 0 0 0 45485 31332 0
>> 100 0 0 0
>>
>> I don't get top to agree with these load results though but it looks
>> like we just have the CPU spinning more, does not look like a win.
>
> The problem appears to be the timeout selection on TX, ignoring it
> completely allows us to keep the load average down while maintaining the
> bandwidth. Looks like NAPI on TX already does a good job, so interrupt
> mitigation on TX is not such a great idea actually...
I saw a similar behavior for TX. For me the issue was too many
outstanding bytes without a completion (defined to be 256KB by sysctl
net.ipv4.tcp_limit_output_bytes). I tested on a 100GbE connection so
with reasonable timeout values I already waited too long (4 TSO
sessions). For the 1GbE case this might have no effect since you need a
very long timeout. I'm currently working on adding TX support for dim.
If you don't see a good benefit currently you might want to wait a
little with TX adaptive interrupt moderation. Maybe only adjust static
moderation for now?
>
> Also, doing UDP TX tests shows that we can lower the interrupt count by
> setting an appropriate tx-frames (as expected), but we won't be lowering
> the CPU load since that is inherently a CPU intensive work. Past
Do you see higher TX UDP bandwidth? If you are bounded by CPU on both
cases I would at least expect higher bandwidth with less interrupts
since you reduce work from the CPU.
> tx-frames=64, the bandwidth completely drops because that would be 1/2
> of the ring size.
>
^ permalink raw reply
* Re: [PATCH net-next V2] Documentation/networking: Add net DIM documentation
From: Tal Gilboa @ 2018-03-26 23:28 UTC (permalink / raw)
To: David Miller; +Cc: netdev, tariqt
In-Reply-To: <20180322.145110.563779296540463789.davem@davemloft.net>
On 3/22/2018 8:51 PM, David Miller wrote:
> From: Tal Gilboa <talgi@mellanox.com>
> Date: Wed, 21 Mar 2018 20:33:45 +0200
>
>> Net DIM is a generic algorithm, purposed for dynamically
>> optimizing network devices interrupt moderation. This
>> document describes how it works and how to use it.
>>
>> Signed-off-by: Tal Gilboa <talgi@mellanox.com>
>
> Applied, although several improvements have been suggested.
>
> Please handle that as follow-ups.
>
> Thank you.
>
Will do ASAP. Thanks all for the feedback.
^ permalink raw reply
* [PATCH 0/6] rhashtable: assorted fixes and enhancements
From: NeilBrown @ 2018-03-26 23:33 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, linux-kernel
Hi,
I'm hoping to use rhashtable in lustre, to replace the resizeable
hashtable implementation in libcfs.
While working through the conversion I found some minor bugs in the
rhashtable code and documentation, and some areas where enhancements
could make rhashtable a better fit for lustre.
Following 6 patches are the result. Please review.
It would help me if I could get an Ack for these patches, and could
then submit them through the drivers/staging tree together with the
lustre changes that make use to rhashtable. The first 2 are mostly
just fixes to comments and can go in through the netdev tree if you
prefer - the last 4 are needed for lustre to work
correctly/optimally.
Thanks,
NeilBrown
---
NeilBrown (6):
rhashtable: improve documentation for rhashtable_walk_peek()
rhashtable: remove outdated comments about grow_decision etc
rhashtable: reset iter when rhashtable_walk_start sees new table
rhashtable: allow a walk of the hash table without missing objects.
rhashtable: support guaranteed successful insertion.
rhashtable: allow element counting to be disabled.
include/linux/rhashtable.h | 89 ++++++++++++++++++++++++++++----------
lib/rhashtable.c | 102 +++++++++++++++++++++++++++++++-------------
2 files changed, 136 insertions(+), 55 deletions(-)
--
Signature
^ permalink raw reply
* [PATCH 1/6] rhashtable: improve documentation for rhashtable_walk_peek()
From: NeilBrown @ 2018-03-26 23:33 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, linux-kernel
In-Reply-To: <152210688405.11435.13010923693146415942.stgit@noble>
The documentation for rhashtable_walk_peek() wrong. It claims to
return the *next* entry, whereas it in fact returns the *previous*
entry.
However if no entries have yet been returned - or if the iterator
was reset due to a resize event, then rhashtable_walk_peek()
*does* return the next entry, but also advances the iterator.
I suspect that this interface should be discarded and the one user
should be changed to not require it. Possibly this patch should be
seen as a first step in that conversation.
This patch mostly corrects the documentation, but does make a
small code change so that the documentation can be correct without
listing too many special cases. I don't think the one user will
be affected by the code change.
Signed-off-by: NeilBrown <neilb@suse.com>
---
lib/rhashtable.c | 17 +++++++++++++----
1 file changed, 13 insertions(+), 4 deletions(-)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 3825c30aaa36..24a57ca494cb 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -853,13 +853,17 @@ void *rhashtable_walk_next(struct rhashtable_iter *iter)
EXPORT_SYMBOL_GPL(rhashtable_walk_next);
/**
- * rhashtable_walk_peek - Return the next object but don't advance the iterator
+ * rhashtable_walk_peek - Return the previously returned object without advancing the iterator
* @iter: Hash table iterator
*
- * Returns the next object or NULL when the end of the table is reached.
+ * Returns the last object returned, or NULL if no object has yet been returned.
+ * If the previously returned object has since been removed, then some other arbitrary
+ * object maybe returned, or possibly NULL will be returned. In that case, the
+ * iterator might be advanced.
*
* Returns -EAGAIN if resize event occurred. Note that the iterator
- * will rewind back to the beginning and you may continue to use it.
+ * will rewind back to the beginning and rhashtable_walk_next() should be
+ * used to get the next object.
*/
void *rhashtable_walk_peek(struct rhashtable_iter *iter)
{
@@ -880,7 +884,12 @@ void *rhashtable_walk_peek(struct rhashtable_iter *iter)
* the table hasn't changed.
*/
iter->skip--;
- }
+ } else
+ /* ->skip is only zero after rhashtable_walk_start()
+ * or when the iterator is reset. In this case there
+ * is no previous object to return.
+ */
+ return NULL;
return __rhashtable_walk_find_next(iter);
}
^ permalink raw reply related
* [PATCH 2/6] rhashtable: remove outdated comments about grow_decision etc
From: NeilBrown @ 2018-03-26 23:33 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, linux-kernel
In-Reply-To: <152210688405.11435.13010923693146415942.stgit@noble>
grow_decision and shink_decision no longer exist, so remove
the remaining references to them.
Signed-off-by: NeilBrown <neilb@suse.com>
---
include/linux/rhashtable.h | 33 ++++++++++++++-------------------
1 file changed, 14 insertions(+), 19 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index c9df2527e0cd..3bd19d29f46b 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -834,9 +834,8 @@ static inline void *__rhashtable_insert_fast(
*
* It is safe to call this function from atomic context.
*
- * Will trigger an automatic deferred table resizing if the size grows
- * beyond the watermark indicated by grow_decision() which can be passed
- * to rhashtable_init().
+ * Will trigger an automatic deferred table resizing if residency in the
+ * table grows beyond 70%.
*/
static inline int rhashtable_insert_fast(
struct rhashtable *ht, struct rhash_head *obj,
@@ -864,9 +863,8 @@ static inline int rhashtable_insert_fast(
*
* It is safe to call this function from atomic context.
*
- * Will trigger an automatic deferred table resizing if the size grows
- * beyond the watermark indicated by grow_decision() which can be passed
- * to rhashtable_init().
+ * Will trigger an automatic deferred table resizing if residency in the
+ * table grows beyond 70%.
*/
static inline int rhltable_insert_key(
struct rhltable *hlt, const void *key, struct rhlist_head *list,
@@ -888,9 +886,8 @@ static inline int rhltable_insert_key(
*
* It is safe to call this function from atomic context.
*
- * Will trigger an automatic deferred table resizing if the size grows
- * beyond the watermark indicated by grow_decision() which can be passed
- * to rhashtable_init().
+ * Will trigger an automatic deferred table resizing if residency in the
+ * table grows beyond 70%.
*/
static inline int rhltable_insert(
struct rhltable *hlt, struct rhlist_head *list,
@@ -920,9 +917,8 @@ static inline int rhltable_insert(
*
* It is safe to call this function from atomic context.
*
- * Will trigger an automatic deferred table resizing if the size grows
- * beyond the watermark indicated by grow_decision() which can be passed
- * to rhashtable_init().
+ * Will trigger an automatic deferred table resizing if residency in the
+ * table grows beyond 70%.
*/
static inline int rhashtable_lookup_insert_fast(
struct rhashtable *ht, struct rhash_head *obj,
@@ -979,9 +975,8 @@ static inline void *rhashtable_lookup_get_insert_fast(
*
* Lookups may occur in parallel with hashtable mutations and resizing.
*
- * Will trigger an automatic deferred table resizing if the size grows
- * beyond the watermark indicated by grow_decision() which can be passed
- * to rhashtable_init().
+ * Will trigger an automatic deferred table resizing if residency in the
+ * table grows beyond 70%.
*
* Returns zero on success.
*/
@@ -1132,8 +1127,8 @@ static inline int __rhashtable_remove_fast(
* walk the bucket chain upon removal. The removal operation is thus
* considerable slow if the hash table is not correctly sized.
*
- * Will automatically shrink the table via rhashtable_expand() if the
- * shrink_decision function specified at rhashtable_init() returns true.
+ * Will automatically shrink the table if permitted when residency drops
+ * below 30%.
*
* Returns zero on success, -ENOENT if the entry could not be found.
*/
@@ -1154,8 +1149,8 @@ static inline int rhashtable_remove_fast(
* walk the bucket chain upon removal. The removal operation is thus
* considerable slow if the hash table is not correctly sized.
*
- * Will automatically shrink the table via rhashtable_expand() if the
- * shrink_decision function specified at rhashtable_init() returns true.
+ * Will automatically shrink the table if permitted when residency drops
+ * below 30%
*
* Returns zero on success, -ENOENT if the entry could not be found.
*/
^ permalink raw reply related
* [PATCH 3/6] rhashtable: reset intr when rhashtable_walk_start sees new table
From: NeilBrown @ 2018-03-26 23:33 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, linux-kernel
In-Reply-To: <152210688405.11435.13010923693146415942.stgit@noble>
The documentation claims that when rhashtable_walk_start_check()
detects a resize event, it will rewind back to the beginning
of the table. This is not true. We need to set ->slot and
->skip to be zero for it to be true.
Signed-off-by: NeilBrown <neilb@suse.com>
---
lib/rhashtable.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 24a57ca494cb..08018198f045 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -733,6 +733,8 @@ int rhashtable_walk_start_check(struct rhashtable_iter *iter)
if (!iter->walker.tbl && !iter->end_of_table) {
iter->walker.tbl = rht_dereference_rcu(ht->tbl, ht);
+ iter->slot = 0;
+ iter->skip = 0;
return -EAGAIN;
}
^ permalink raw reply related
* [PATCH 4/6] rhashtable: allow a walk of the hash table without missing objects.
From: NeilBrown @ 2018-03-26 23:33 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, linux-kernel
In-Reply-To: <152210688405.11435.13010923693146415942.stgit@noble>
When a walk of the hashtable can be done entirely under RCU,
no objects will be missed - though seeing duplicates is possible.
This is because a cursor is kept in iter->p.
Without the cursor we depend on the ->skip counter. If an object
before the current location in hash chain is removed, the ->skip
counter will be too large and would could miss a later object.
In many cases where the walker needs to drop out of RCU protection,
it will take a reference to the object and this can prevent it from
being removed from the hash table. In those cases, the last-returned
object can still be used as a cursor. rhashtable cannot detect
these cases itself.
This patch adds a new rhashtable_walk_start_continue() interface which
is passed the last object returned. This can be used if the caller
knows that the object is still in the hash table. When it is used,
a walk of the hash table will return every object that was in the
hastable for the duration of the walk, at least once. This can be
used, for example, to selectively delete objects from the table.
Signed-off-by: NeilBrown <neilb@suse.com>
---
include/linux/rhashtable.h | 28 ++++++++++++++++++++++++++--
lib/rhashtable.c | 42 ++++++++++++++++++++++++++----------------
2 files changed, 52 insertions(+), 18 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 3bd19d29f46b..4ffd96949d4f 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -387,11 +387,35 @@ void *rhashtable_insert_slow(struct rhashtable *ht, const void *key,
void rhashtable_walk_enter(struct rhashtable *ht,
struct rhashtable_iter *iter);
void rhashtable_walk_exit(struct rhashtable_iter *iter);
-int rhashtable_walk_start_check(struct rhashtable_iter *iter) __acquires(RCU);
+int rhashtable_walk_start_continue(struct rhashtable_iter *iter,
+ struct rhash_head *obj) __acquires(RCU);
+
+/**
+ * rhashtable_walk_start_check - Start a hash table walk
+ * @iter: Hash table iterator
+ *
+ * Start a hash table walk at the current iterator position. Note that we take
+ * the RCU lock in all cases including when we return an error. So you must
+ * always call rhashtable_walk_stop to clean up.
+ *
+ * Returns zero if successful.
+ *
+ * Returns -EAGAIN if resize event occured. Note that the iterator
+ * will rewind back to the beginning and you may use it immediately
+ * by calling rhashtable_walk_next.
+ *
+ * rhashtable_walk_start is defined as an inline variant that returns
+ * void. This is preferred in cases where the caller would ignore
+ * resize events and always continue.
+ */
+static inline int rhashtable_walk_start_check(struct rhashtable_iter *iter)
+{
+ return rhashtable_walk_start_continue(iter, NULL);
+}
static inline void rhashtable_walk_start(struct rhashtable_iter *iter)
{
- (void)rhashtable_walk_start_check(iter);
+ (void)rhashtable_walk_start_continue(iter, NULL);
}
void *rhashtable_walk_next(struct rhashtable_iter *iter);
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index 08018198f045..fd6f320b9704 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -702,30 +702,41 @@ void rhashtable_walk_exit(struct rhashtable_iter *iter)
EXPORT_SYMBOL_GPL(rhashtable_walk_exit);
/**
- * rhashtable_walk_start_check - Start a hash table walk
- * @iter: Hash table iterator
+ * rhashtable_walk_start_continue - Restart a hash table walk from last object
+ * @iter: Hask table iterator
+ * @obj: pointer to rhash_head in last object returned.
+ *
+ * Restart a hash table walk, ensuring not to miss any objects. The
+ * previously returned object must still be in the hash table, and must be
+ * provided as an argument.
*
- * Start a hash table walk at the current iterator position. Note that we take
- * the RCU lock in all cases including when we return an error. So you must
- * always call rhashtable_walk_stop to clean up.
+ * When rhashtable_walk_start() or rhashtable_walk_start_check() is used,
+ * a deletion since the previous walk_start can result in objects being missed
+ * as a hash chain might be shorter than expected. This can be avoided by
+ * using the last returned object as a cursor.
*
- * Returns zero if successful.
+ * If the @obj passed is NULL, or not the most recently returned object,
+ * rhashtable_walk_start_continue() will act like rhashtable_walk_start_check();
*
- * Returns -EAGAIN if resize event occured. Note that the iterator
- * will rewind back to the beginning and you may use it immediately
- * by calling rhashtable_walk_next.
+ * Returns -EAGAIN if a resize event was detected. The iterator will
+ * rewind back to the beginning and can be used immediately. Seeing duplicates
+ * is possible but missing objects isn't.
+ * Returns zero if no resize event was detected. This does not guarantee
+ * that no duplicates will be seen.
*
- * rhashtable_walk_start is defined as an inline variant that returns
- * void. This is preferred in cases where the caller would ignore
- * resize events and always continue.
+ * Always takes the RCU read lock, so rhashtable_walk_stop() must always be called
+ * to clean up.
*/
-int rhashtable_walk_start_check(struct rhashtable_iter *iter)
+int rhashtable_walk_start_continue(struct rhashtable_iter *iter, struct rhash_head *obj)
__acquires(RCU)
{
struct rhashtable *ht = iter->ht;
rcu_read_lock();
+ if (!obj || iter->p != obj)
+ iter->p = NULL;
+
spin_lock(&ht->lock);
if (iter->walker.tbl)
list_del(&iter->walker.list);
@@ -733,6 +744,7 @@ int rhashtable_walk_start_check(struct rhashtable_iter *iter)
if (!iter->walker.tbl && !iter->end_of_table) {
iter->walker.tbl = rht_dereference_rcu(ht->tbl, ht);
+ iter->p = NULL;
iter->slot = 0;
iter->skip = 0;
return -EAGAIN;
@@ -740,7 +752,7 @@ int rhashtable_walk_start_check(struct rhashtable_iter *iter)
return 0;
}
-EXPORT_SYMBOL_GPL(rhashtable_walk_start_check);
+EXPORT_SYMBOL_GPL(rhashtable_walk_start_continue);
/**
* __rhashtable_walk_find_next - Find the next element in a table (or the first
@@ -922,8 +934,6 @@ void rhashtable_walk_stop(struct rhashtable_iter *iter)
iter->walker.tbl = NULL;
spin_unlock(&ht->lock);
- iter->p = NULL;
-
out:
rcu_read_unlock();
}
^ permalink raw reply related
* [PATCH 5/6] rhashtable: support guaranteed successful insertion.
From: NeilBrown @ 2018-03-26 23:33 UTC (permalink / raw)
To: Thomas Graf, Herbert Xu; +Cc: netdev, linux-kernel
In-Reply-To: <152210688405.11435.13010923693146415942.stgit@noble>
The current rhashtable will fail an insertion if the hashtable
it "too full", one of:
- table already has 2^31 elements (-E2BIG)
- a max_size was specified and table already has that
many elements (rounded up to power of 2) (-E2BIG)
- a single chain has more than 16 elements (-EBUSY)
- table has more elements than the current table size,
and allocating a new table fails (-ENOMEM)
- a new page needed to be allocated for a nested table,
and the memory allocation failed (-ENOMEM).
A traditional hash table does not have a concept of "too full", and
insertion only fails if the key already exists. Many users of hash
tables have separate means of limiting the total number of entries,
and are not susceptible to an attack which could cause unusually large
hash chains. For those users, the need to check for errors when
inserting objects to an rhashtable is an unnecessary burden and hence
a potential source of bugs (as these failures are likely to be rare).
This patch adds a "never_fail_insert" configuration parameter which
ensures that insertion will only fail if the key already exists.
When this option is in effect:
- nelems is capped at INT_MAX and will never decrease once it reaches
that value
- max_size is largely ignored
- elements will be added to a table that is nominally "full", though
a rehash will be scheduled
- a new table will never be allocated directly by the insert
function, that is always left for the worker.
For this to trigger a rehash when long chains are detected (possibly
still useful) an extra field in the table records if a long chain
has been seen. This shares a word with the 'nest' value. As
'nest' is never changed once the table is created, updating the
new ->long_chain without locking cannot cause any corruption.
Signed-off-by: NeilBrown <neilb@suse.com>
---
include/linux/rhashtable.h | 18 +++++++++++++++---
lib/rhashtable.c | 27 +++++++++++++++++++--------
2 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/include/linux/rhashtable.h b/include/linux/rhashtable.h
index 4ffd96949d4f..abdeb1f3f378 100644
--- a/include/linux/rhashtable.h
+++ b/include/linux/rhashtable.h
@@ -77,6 +77,7 @@ struct rhlist_head {
* struct bucket_table - Table of hash buckets
* @size: Number of hash buckets
* @nest: Number of bits of first-level nested table.
+ * @long_chain: %true when a chain longer than RHT_ELASTICITY seen.
* @rehash: Current bucket being rehashed
* @hash_rnd: Random seed to fold into hash
* @locks_mask: Mask to apply before accessing locks[]
@@ -89,7 +90,8 @@ struct rhlist_head {
*/
struct bucket_table {
unsigned int size;
- unsigned int nest;
+ unsigned short nest;
+ bool long_chain;
unsigned int rehash;
u32 hash_rnd;
unsigned int locks_mask;
@@ -129,6 +131,9 @@ struct rhashtable;
* @min_size: Minimum size while shrinking
* @locks_mul: Number of bucket locks to allocate per cpu (default: 32)
* @automatic_shrinking: Enable automatic shrinking of tables
+ * @never_fail_insert: Insert will always succeed, even if table will become
+ * unbalanced. Without this, -E2BIG, -EBUSY, and -ENOMEM are possible
+ * errors from rhashtable_*insert*()
* @nulls_base: Base value to generate nulls marker
* @hashfn: Hash function (default: jhash2 if !(key_len % 4), or jhash)
* @obj_hashfn: Function to hash object
@@ -142,6 +147,7 @@ struct rhashtable_params {
unsigned int max_size;
u16 min_size;
bool automatic_shrinking;
+ bool never_fail_insert;
u8 locks_mul;
u32 nulls_base;
rht_hashfn_t hashfn;
@@ -832,7 +838,10 @@ static inline void *__rhashtable_insert_fast(
rcu_assign_pointer(*pprev, obj);
- atomic_inc(&ht->nelems);
+ if (params.never_fail_insert)
+ atomic_add_unless(&ht->nelems, 1, INT_MAX);
+ else
+ atomic_inc(&ht->nelems);
if (rht_grow_above_75(ht, tbl))
schedule_work(&ht->run_work);
@@ -1104,7 +1113,10 @@ static inline int __rhashtable_remove_fast_one(
spin_unlock_bh(lock);
if (err > 0) {
- atomic_dec(&ht->nelems);
+ if (params.never_fail_insert)
+ atomic_add_unless(&ht->nelems, -1, INT_MAX);
+ else
+ atomic_dec(&ht->nelems);
if (unlikely(ht->p.automatic_shrinking &&
rht_shrink_below_30(ht, tbl)))
schedule_work(&ht->run_work);
diff --git a/lib/rhashtable.c b/lib/rhashtable.c
index fd6f320b9704..427836aace60 100644
--- a/lib/rhashtable.c
+++ b/lib/rhashtable.c
@@ -424,7 +424,7 @@ static void rht_deferred_worker(struct work_struct *work)
err = rhashtable_rehash_alloc(ht, tbl, tbl->size * 2);
else if (ht->p.automatic_shrinking && rht_shrink_below_30(ht, tbl))
err = rhashtable_shrink(ht);
- else if (tbl->nest)
+ else if (tbl->nest || tbl->long_chain)
err = rhashtable_rehash_alloc(ht, tbl, tbl->size);
if (!err)
@@ -549,14 +549,22 @@ static struct bucket_table *rhashtable_insert_one(struct rhashtable *ht,
if (new_tbl)
return new_tbl;
- if (PTR_ERR(data) != -ENOENT)
- return ERR_CAST(data);
+ if (ht->p.never_fail_insert) {
+ if (PTR_ERR(data) == -EAGAIN &&
+ atomic_read(&ht->nelems) != INT_MAX) {
+ tbl->long_chain = true;
+ schedule_work(&ht->run_work);
+ }
+ } else {
+ if (PTR_ERR(data) != -ENOENT)
+ return ERR_CAST(data);
- if (unlikely(rht_grow_above_max(ht, tbl)))
- return ERR_PTR(-E2BIG);
+ if (unlikely(rht_grow_above_max(ht, tbl)))
+ return ERR_PTR(-E2BIG);
- if (unlikely(rht_grow_above_100(ht, tbl)))
- return ERR_PTR(-EAGAIN);
+ if (unlikely(rht_grow_above_100(ht, tbl)))
+ return ERR_PTR(-EAGAIN);
+ }
pprev = rht_bucket_insert(ht, tbl, hash);
if (!pprev)
@@ -574,7 +582,10 @@ static struct bucket_table *rhashtable_insert_one(struct rhashtable *ht,
rcu_assign_pointer(*pprev, obj);
- atomic_inc(&ht->nelems);
+ if (ht->p.never_fail_insert)
+ atomic_add_unless(&ht->nelems, 1, INT_MAX);
+ else
+ atomic_inc(&ht->nelems);
if (rht_grow_above_75(ht, tbl))
schedule_work(&ht->run_work);
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox