* Re: [RFC PATCH 00/24] Introducing AF_XDP support
From: Jesper Dangaard Brouer @ 2018-03-26 16:38 UTC (permalink / raw)
To: 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, brouer
In-Reply-To: <CALDO+SYK_4RfRBe7Z0n00wkPhKc1HwGmcGT34W0tVMVZFLqYpw@mail.gmail.com>
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 ?
> 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.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [net PATCH v2] net: sched, fix OOO packets with pfifo_fast
From: David Miller @ 2018-03-26 16:36 UTC (permalink / raw)
To: john.fastabend; +Cc: eric.dumazet, xiyou.wangcong, jiri, netdev
In-Reply-To: <20180325052505.4098.36912.stgit@john-Precision-Tower-5810>
From: John Fastabend <john.fastabend@gmail.com>
Date: Sat, 24 Mar 2018 22:25:06 -0700
> After the qdisc lock was dropped in pfifo_fast we allow multiple
> enqueue threads and dequeue threads to run in parallel. On the
> enqueue side the skb bit ooo_okay is used to ensure all related
> skbs are enqueued in-order. On the dequeue side though there is
> no similar logic. What we observe is with fewer queues than CPUs
> it is possible to re-order packets when two instances of
> __qdisc_run() are running in parallel. Each thread will dequeue
> a skb and then whichever thread calls the ndo op first will
> be sent on the wire. This doesn't typically happen because
> qdisc_run() is usually triggered by the same core that did the
> enqueue. However, drivers will trigger __netif_schedule()
> when queues are transitioning from stopped to awake using the
> netif_tx_wake_* APIs. When this happens netif_schedule() calls
> qdisc_run() on the same CPU that did the netif_tx_wake_* which
> is usually done in the interrupt completion context. This CPU
> is selected with the irq affinity which is unrelated to the
> enqueue operations.
>
> To resolve this we add a RUNNING bit to the qdisc to ensure
> only a single dequeue per qdisc is running. Enqueue and dequeue
> operations can still run in parallel and also on multi queue
> NICs we can still have a dequeue in-flight per qdisc, which
> is typically per CPU.
>
> Fixes: c5ad119fb6c0 ("net: sched: pfifo_fast use skb_array")
> Reported-by: Jakob Unterwurzacher <jakob.unterwurzacher@theobroma-systems.com>
> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
Applied, thanks John.
^ permalink raw reply
* Re: [PATCH] net: qmi_wwan: add BroadMobi BM806U 2020:2033
From: David Miller @ 2018-03-26 16:35 UTC (permalink / raw)
To: paweldembicki; +Cc: linux-usb, bjorn, netdev, linux-kernel
In-Reply-To: <1521925694-15433-1-git-send-email-paweldembicki@gmail.com>
From: Pawel Dembicki <paweldembicki@gmail.com>
Date: Sat, 24 Mar 2018 22:08:14 +0100
> BroadMobi BM806U is an Qualcomm MDM9225 based 3G/4G modem.
> Tested hardware BM806U is mounted on D-Link DWR-921-C3 router.
> The USB id is added to qmi_wwan.c to allow QMI communication with
> the BM806U.
>
> Tested on 4.14 kernel and OpenWRT.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 16:35 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Mathieu Desnoyers, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api,
Frank Ch. Eigler
In-Reply-To: <89fbc745-c290-c82c-a837-8998cf2988e7@fb.com>
On Mon, 26 Mar 2018 09:25:07 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> commit log of patch 6 states:
>
> "for_each_tracepoint_range() api has no users inside the kernel.
> Make it more useful with ability to stop for_each() loop depending
> via callback return value.
> In such form it's used in subsequent patch."
>
> and in patch 7:
>
> +static void *__find_tp(struct tracepoint *tp, void *priv)
> +{
> + char *name = priv;
> +
> + if (!strcmp(tp->name, name))
> + return tp;
> + return NULL;
> +}
> ...
> + struct tracepoint *tp;
> ...
> + tp = for_each_kernel_tracepoint(__find_tp, tp_name);
> + if (!tp)
> + return -ENOENT;
>
> still not obvious?
Please just create a new function called tracepoint_find_by_name(), and
use that. I don't see any benefit in using a for_each* function for
such a simple routine. Not to mention, you then don't need to know the
internals of a tracepoint in kernel/bpf/syscall.c.
-- Steve
^ permalink raw reply
* Re: [PATCH 0/2] sh_eth: unify the SoC feature checks
From: David Miller @ 2018-03-26 16:34 UTC (permalink / raw)
To: sergei.shtylyov; +Cc: netdev, linux-renesas-soc, linux-sh
In-Reply-To: <b3706a51-27a4-d78b-998d-9440637ac216@cogentembedded.com>
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Sat, 24 Mar 2018 23:04:53 +0300
> Here's a set of 5 patches against DaveM's 'net-next.git' repo.
>
> The Ether driver sometimes uses the bit fields in 'struct sh_eth_cpu_data'
> to check which Ether registers exist in a certain SoC and sometimes it uses
> sh_eth_is_{gether|rz_fast_ether}() which basically compares 2 pointers (1 of
> them being constant) -- the latter is definitely not a strongest feature of
> the RISC CPUs (be it SH or ARM), so I decided to get rid of this type of
> the feature checks in favour of the bit fields (I've also made use of a
> 32-bit value and method pointer where appropriate)...
Series applied with patch #4 subject fixed up.
Thank you.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 16:31 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api
In-Reply-To: <20180326115615.0ca53410@gandalf.local.home>
On Mon, 26 Mar 2018 11:56:15 -0400
Steven Rostedt <rostedt@goodmis.org> wrote:
> On Fri, 23 Mar 2018 19:30:34 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
> > +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;
> > - for (iter = begin; iter < end; iter++)
> > - fct(*iter, priv);
> > + return NULL;
> > + for (iter = begin; iter < end; iter++) {
> > + ret = fct(*iter, priv);
> > + if (ret)
> > + return ret;
>
> So you just stopped the loop here. You have an inconsistent state. What
> about the functions that were called before. How do you undo them? Or
> what about the rest that haven't been touched. This function gives no
> feedback to the caller.
>
OK, I see my confusion with this patch. I much rather have a new
function, and this isn't about being nice to out of tree modules. We
can keep this function as is (to be nice), but my biggest squabble
about this patch is that the function name is inconsistent to what its
doing. When I have a function that says "for_each" I expect it to go
through each function and not exit out. This is because of what I said
above. When you have a "for_each" function that stops in the middle, you
have a state that you may need to deal with. How would another use of
that function clean up the mess if it expected to fail at some random
location.
I see in the next patch that you are using it simply to find a
tracepoint with the given name. I'd be much happier to add a new
function called:
tracepoint_find_by_name(const char *name)
Because using a "for_each" to implement such a simple function seems
more of a hack.
-- Steve
^ permalink raw reply
* Re: [PATCH] Documentation/isdn: check and fix dead links ...
From: David Miller @ 2018-03-26 16:31 UTC (permalink / raw)
To: ghane0; +Cc: corbet, isdn, netdev, pebolle
In-Reply-To: <20180324050742.7072-1-ghane0@gmail.com>
From: Sanjeev Gupta <ghane0@gmail.com>
Date: Sat, 24 Mar 2018 13:07:42 +0800
> and switch to https where possible.
>
> All links have been eyeballed to verify that the domains have
> not changed, etc.
>
> Signed-off-by: Sanjeev Gupta <ghane0@gmail.com>
Applied, thank you.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 16:25 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <1055377367.195.1522081045131.JavaMail.zimbra@efficios.com>
On 3/26/18 9:17 AM, Mathieu Desnoyers wrote:
>>
>> re: 'added complexity'...
>> - for (iter = begin; iter < end; iter++)
>> - fct(*iter, priv);
>> + return NULL;
>> + for (iter = begin; iter < end; iter++) {
>> + ret = fct(*iter, priv);
>> + if (ret)
>> + return ret;
>> + }
>> + return NULL;
>>
>> where do you see 'added complexity' ?
>> Isn't the above diff self-explanatory that for_each_tracepoint_range()
>> can be used not only to iterate over all tracepoints
>> (just do 'return NULL') from callback _and_ to find one particular
>> tracepoint as patch 7 does ?
>
> I am not arguing about your proposed implementation. I am arguing about
> the lack of justification behind this change. Why is this change needed ?
> What is it allowing you to do that cannot be done using the private data
> pointer ?
commit log of patch 6 states:
"for_each_tracepoint_range() api has no users inside the kernel.
Make it more useful with ability to stop for_each() loop depending
via callback return value.
In such form it's used in subsequent patch."
and in patch 7:
+static void *__find_tp(struct tracepoint *tp, void *priv)
+{
+ char *name = priv;
+
+ if (!strcmp(tp->name, name))
+ return tp;
+ return NULL;
+}
...
+ struct tracepoint *tp;
...
+ tp = for_each_kernel_tracepoint(__find_tp, tp_name);
+ if (!tp)
+ return -ENOENT;
still not obvious?
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 16:17 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <f39181e0-3d46-2154-044c-dfc985053b3a@fb.com>
----- On Mar 26, 2018, at 12:08 PM, Alexei Starovoitov ast@fb.com wrote:
> On 3/26/18 8:55 AM, Mathieu Desnoyers wrote:
>> ----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov ast@fb.com wrote:
>>
>>> On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
>>>> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>>>>
>>>>> On Fri, 23 Mar 2018 19:30:34 -0700
>>>>> Alexei Starovoitov <ast@fb.com> wrote:
>>>>>
>>>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>>>
>>>>>> add fancy macro to compute number of arguments passed into tracepoint
>>>>>> at compile time and store it as part of 'struct tracepoint'.
>>>>>> The number is necessary to check safety of bpf program access that
>>>>>> is coming in subsequent patch.
>>>>>>
>>>>>> for_each_tracepoint_range() api has no users inside the kernel.
>>>>>> Make it more useful with ability to stop for_each() loop depending
>>>>>> via callback return value.
>>>>>> In such form it's used in subsequent patch.
>>>>>
>>>>> I believe this is used by LTTng.
>>>>
>>>> Indeed, and by SystemTAP as well.
>>>>
>>>> What justifies the need to stop mid-iteration ? A less intrusive alternative
>>>> would be to use the "priv" data pointer to keep state telling further calls
>>>> to return immediately. Does performance of iteration over tracepoints really
>>>> matter here so much that stopping iteration immediately is worth it ?
>>>
>>> I'm sure both you and Steven are not serious when you object
>>> to _in-tree_ change to for_each_kernel_tracepoint() that
>>> affects _out-of_tree_ modules?
>>>
>>> Just change your module to 'return NULL' instead of plain 'return'.
>>
>> I never said I objected to adapt the LTTng out of tree code. If there is a
>> solid reason for changing the kernel API, I will adapt my code to those
>> changes.
>>
>> What I'm trying to understand here is whether there is solid ground for
>> the added complexity you are proposing. Is it a performance enhancement ?
>> If so, explanation of the use cases targeted, and numbers that measure
>> performance improvements are needed.
>>
>> How is your patch making tracepoints "more useful" ?
>
> are you arguing about the whole set overall or about a change
> to for_each_kernel_tracepoint() ?
I'm perfectly fine with adding the "num_args" stuff. I think it's
really useful. It's only the for_each_kernel_tracepoint change for
which I'm trying to understand the rationale.
> I'm hearing your arguments as that now changes to all exported functions
> need to be "solid" (not sure what exactly means 'solid' here) to
> justify breakage of out-of-tree modules?
No. Any added complexity to tracepoint.c needs to be justified
appropriately.
>
> re: 'added complexity'...
> - for (iter = begin; iter < end; iter++)
> - fct(*iter, priv);
> + return NULL;
> + for (iter = begin; iter < end; iter++) {
> + ret = fct(*iter, priv);
> + if (ret)
> + return ret;
> + }
> + return NULL;
>
> where do you see 'added complexity' ?
> Isn't the above diff self-explanatory that for_each_tracepoint_range()
> can be used not only to iterate over all tracepoints
> (just do 'return NULL') from callback _and_ to find one particular
> tracepoint as patch 7 does ?
I am not arguing about your proposed implementation. I am arguing about
the lack of justification behind this change. Why is this change needed ?
What is it allowing you to do that cannot be done using the private data
pointer ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Steven Rostedt @ 2018-03-26 16:16 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Daniel Borkmann, davem, torvalds, peterz, netdev, kernel-team,
linux-api
In-Reply-To: <3967d838-e737-ec44-d03b-54f11f85d21b@fb.com>
On Mon, 26 Mar 2018 09:00:33 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> On 3/26/18 8:47 AM, Steven Rostedt wrote:
> > On Mon, 26 Mar 2018 17:32:02 +0200
> > Daniel Borkmann <daniel@iogearbox.net>
> >> On 03/26/2018 05:04 PM, Steven Rostedt wrote:
> >>> On Mon, 26 Mar 2018 10:28:03 +0200
> >>> Daniel Borkmann <daniel@iogearbox.net> wrote:
> >>>
> >>>>> tracepoint base kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
> >>>>> task_rename 1.1M 769K 947K 1.0M
> >>>>> urandom_read 789K 697K 750K 755K
> >>>>
> >>>> Applied to bpf-next, thanks Alexei!
> >>>
> >>> Please wait till you have the proper acks. Some of this affects
> >>> tracing.
> >>
> >> Ok, I thought time up to v5 was long enough. Anyway, in case there are
> >> objections I can still toss out the series from bpf-next tree worst case
> >> should e.g. follow-up fixups not be appropriate.
> >
> > Yeah, I've been traveling a bit which slowed down my review process
> > (trying to catch up).
>
> v1 of this set was posted Feb 28.
Yep, Where I traveled to the West coast 2/26 - 3/1 (but due to snow
storms, I didn't get home till late 3/2). Then I went back 3/6 and came
home 3/8 (again due to another snow storm, it was 3/9). Then I went to
ELC from 3/11 to 3/15 (Luckily, the third snow storm hit 3/14, and
didn't affect my return trip).
> imo one month is not an acceptable delay for maintainer to review
> the patches. You really need to consider group maintainership as
> we do with Daniel for bpf tree.
Perhaps, (which I talked to Masami about, just need to go through
logistics). But the tracing code isn't high volume, and the three weeks
of traveling for me was a fluke (didn't look at my schedule when I
agreed to make that second one).
>
> > My main concern is with patch 6, as there are
> > external users of those functions. Although, we generally don't cater
> > to out of tree code, we play nice with LTTng, and I don't want to break
> > it.
>
> out-of-tree module is out of tree. I'm beyond surprised that you
> propose to keep for_each_kernel_tracepoint() as-is with zero in-tree
> users in order to keep lttng working.
I'm nice.
>
> > I also should probably pull in the patches and run them through my
> > tests to make sure they don't have any other side effects.
>
> so let me rephrase.
> You're saying that a change to a function with zero in-tree users
> can somehow break your tests?
> How is that possible?
> Does it mean you also have some out-of-tree modules that will break?
> and that _is_ the real reason for objection?
That function isn't what I'm worried about. You changed much more than
that.
-- Steve
^ permalink raw reply
* Re: [PATCH net-next] ptp: Fix documentation to match code.
From: David Miller @ 2018-03-26 16:13 UTC (permalink / raw)
To: richardcochran; +Cc: netdev
In-Reply-To: <20180324042402.9705-1-richardcochran@gmail.com>
From: Richard Cochran <richardcochran@gmail.com>
Date: Fri, 23 Mar 2018 21:24:02 -0700
> Ever since commit 3a06c7ac24f9 ("posix-clocks: Remove interval timer
> facility and mmap/fasync callbacks") the possibility of PHC based
> posix timers has been removed. In addition it will probably never
> make sense to implement this functionality.
>
> This patch removes the misleading text which seems to suggest that
> posix timers for PHC devices will ever be a thing.
>
> Signed-off-by: Richard Cochran <richardcochran@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-next 0/5] fix some bugs for HNS3
From: David Miller @ 2018-03-26 16:13 UTC (permalink / raw)
To: lipeng321; +Cc: netdev, linux-kernel, linuxarm, salil.mehta
In-Reply-To: <1521862367-111399-1-git-send-email-lipeng321@huawei.com>
From: Peng Li <lipeng321@huawei.com>
Date: Sat, 24 Mar 2018 11:32:42 +0800
> This patchset fixes some bugs for HNS3 driver:
> [Patch 1/5 - 2/5] fix 2 return vlaue issues.
> [Patch 3/5 - 4/5] fix 2 comments reported by code review.
> [Ptach 5/5] avoid sending message to IMP because IMP will not
> handle any message when it is resetting.
Series applied, thank you.
^ permalink raw reply
* Re: [PATCH] net: qualcomm: rmnet: check for null ep to avoid null pointer dereference
From: David Miller @ 2018-03-26 16:11 UTC (permalink / raw)
To: colin.king; +Cc: subashab, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20180323235157.8129-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Fri, 23 Mar 2018 23:51:57 +0000
> From: Colin Ian King <colin.king@canonical.com>
>
> The call to rmnet_get_endpoint can potentially return NULL so check
> for this to avoid any subsequent null pointer dereferences on a NULL
> ep.
>
> Detected by CoverityScan, CID#1465385 ("Dereference null return value")
>
> Fixes: 23790ef12082 ("net: qualcomm: rmnet: Allow to configure flags for existing devices")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Alexei Starovoitov @ 2018-03-26 16:08 UTC (permalink / raw)
To: Mathieu Desnoyers
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <523311773.184.1522079745421.JavaMail.zimbra@efficios.com>
On 3/26/18 8:55 AM, Mathieu Desnoyers wrote:
> ----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov ast@fb.com wrote:
>
>> On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
>>> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>>>
>>>> On Fri, 23 Mar 2018 19:30:34 -0700
>>>> Alexei Starovoitov <ast@fb.com> wrote:
>>>>
>>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>>
>>>>> add fancy macro to compute number of arguments passed into tracepoint
>>>>> at compile time and store it as part of 'struct tracepoint'.
>>>>> The number is necessary to check safety of bpf program access that
>>>>> is coming in subsequent patch.
>>>>>
>>>>> for_each_tracepoint_range() api has no users inside the kernel.
>>>>> Make it more useful with ability to stop for_each() loop depending
>>>>> via callback return value.
>>>>> In such form it's used in subsequent patch.
>>>>
>>>> I believe this is used by LTTng.
>>>
>>> Indeed, and by SystemTAP as well.
>>>
>>> What justifies the need to stop mid-iteration ? A less intrusive alternative
>>> would be to use the "priv" data pointer to keep state telling further calls
>>> to return immediately. Does performance of iteration over tracepoints really
>>> matter here so much that stopping iteration immediately is worth it ?
>>
>> I'm sure both you and Steven are not serious when you object
>> to _in-tree_ change to for_each_kernel_tracepoint() that
>> affects _out-of_tree_ modules?
>>
>> Just change your module to 'return NULL' instead of plain 'return'.
>
> I never said I objected to adapt the LTTng out of tree code. If there is a
> solid reason for changing the kernel API, I will adapt my code to those
> changes.
>
> What I'm trying to understand here is whether there is solid ground for
> the added complexity you are proposing. Is it a performance enhancement ?
> If so, explanation of the use cases targeted, and numbers that measure
> performance improvements are needed.
>
> How is your patch making tracepoints "more useful" ?
are you arguing about the whole set overall or about a change
to for_each_kernel_tracepoint() ?
I'm hearing your arguments as that now changes to all exported functions
need to be "solid" (not sure what exactly means 'solid' here) to
justify breakage of out-of-tree modules?
re: 'added complexity'...
- for (iter = begin; iter < end; iter++)
- fct(*iter, priv);
+ return NULL;
+ for (iter = begin; iter < end; iter++) {
+ ret = fct(*iter, priv);
+ if (ret)
+ return ret;
+ }
+ return NULL;
where do you see 'added complexity' ?
Isn't the above diff self-explanatory that for_each_tracepoint_range()
can be used not only to iterate over all tracepoints
(just do 'return NULL') from callback _and_ to find one particular
tracepoint as patch 7 does ?
^ permalink raw reply
* Re: [PATCH 4/4] drivers/net: Use octal not symbolic permissions
From: David Miller @ 2018-03-26 16:08 UTC (permalink / raw)
To: joe
Cc: j.vosburgh, vfalico, andy, dmitry.tarnyagin, wg, mkl,
nicolas.ferre, alexandre.belloni, jpr, kys, haiyangz, sthemmin,
alex.aring, stefan, andrew, f.fainelli, paulus, mostrows, oliver,
wei.liu2, paul.durrant, boris.ostrovsky, jgross, netdev,
linux-kernel, linux-can, linux-arm-kernel, linux-hams, devel,
linux-wpan, linux-ppp, linux-usb, xen-devel
In-Reply-To: <8f24a711e6b8ec7b41356c378140fb54d510205c.1521845248.git.joe@perches.com>
From: Joe Perches <joe@perches.com>
Date: Fri, 23 Mar 2018 15:54:39 -0700
> Prefer the direct use of octal for permissions.
>
> Done with checkpatch -f --types=SYMBOLIC_PERMS --fix-inplace
> and some typing.
>
> Miscellanea:
>
> o Whitespace neatening around these conversions.
>
> Signed-off-by: Joe Perches <joe@perches.com>
Applied.
^ permalink raw reply
* Re: [PATCH 3/4] net: Use octal not symbolic permissions
From: David Miller @ 2018-03-26 16:08 UTC (permalink / raw)
To: joe
Cc: linux-wireless, jlayton, trond.myklebust, dhowells, linux-sctp,
linux-afs, steffen.klassert, herbert, sage, bridge, jchapman,
coreteam, kadlec, kuznet, idryomov, pablo, johan.hedberg, marcel,
linux-can, mkl, linux-hams, ceph-devel, linux-arm-kernel, bfields,
linux-nfs, linux-x25, nhorman, yoshfuji, socketcan, vyasevich,
linux-decnet-user, fw, linux-kernel, ralf, afaerbe
In-Reply-To: <738a74ff57f993d6b8f249425673413672d3fe70.1521845242.git.joe@perches.com>
Applied.
^ permalink raw reply
* Re: [PATCH 1/4] ethernet: Use octal not symbolic permissions
From: David Miller @ 2018-03-26 16:08 UTC (permalink / raw)
To: joe; +Cc: netdev
In-Reply-To: <31c47a8bb2af3f01178d83e890d2600f049ad709.1521845235.git.joe@perches.com>
Applied.
^ permalink raw reply
* Re: [RFC PATCH 00/24] Introducing AF_XDP support
From: William Tu @ 2018-03-26 16:06 UTC (permalink / raw)
To: Björn Töpel
Cc: magnus.karlsson, Alexander Duyck, Alexander Duyck, John Fastabend,
Alexei Starovoitov, Jesper Dangaard Brouer, 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: <20180131135356.19134-1-bjorn.topel@gmail.com>
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
AF_XDP performance:
Benchmark XDP_SKB
rxdrop 1.27 Mpps
txpush 0.99 Mpps
l2fwd 0.85 Mpps
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
For l2fwd:
20.81% xdpsock xdpsock [.] main
10.64% xdpsock [kernel.vmlinux] [k] clflush_cache_range
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
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?
Thanks
William
^ permalink raw reply
* Re: [PATCH net 3/3] bonding: process the err returned by dev_set_allmulti properly in bond_enslave
From: Andy Gospodarek @ 2018-03-26 16:07 UTC (permalink / raw)
To: Xin Long
Cc: network dev, davem, Jiri Pirko, Wang Chen, Veaceslav Falico,
Nikolay Aleksandrov
In-Reply-To: <5695ce418ff6d7a2755053d6c7c35dfd3ebf2d6b.1521997984.git.lucien.xin@gmail.com>
On Mon, Mar 26, 2018 at 01:16:47AM +0800, Xin Long wrote:
> When dev_set_promiscuity(1) succeeds but dev_set_allmulti(1) fails,
> dev_set_promiscuity(-1) should be done before going to the err path.
> Otherwise, dev->promiscuity will leak.
>
> Fixes: 7e1a1ac1fbaa ("bonding: Check return of dev_set_promiscuity/allmulti")
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Andy Gospodarek <andy@greyhouse.net>
> ---
> drivers/net/bonding/bond_main.c | 5 ++++-
> 1 file changed, 4 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 55e1985..b7b1130 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -1706,8 +1706,11 @@ int bond_enslave(struct net_device *bond_dev, struct net_device *slave_dev,
> /* set allmulti level to new slave */
> if (bond_dev->flags & IFF_ALLMULTI) {
> res = dev_set_allmulti(slave_dev, 1);
> - if (res)
> + if (res) {
> + if (bond_dev->flags & IFF_PROMISC)
> + dev_set_promiscuity(slave_dev, -1);
> goto err_sysfs_del;
> + }
> }
>
> netif_addr_lock_bh(bond_dev);
^ permalink raw reply
* Re: [PATCH v5 bpf-next 00/10] bpf, tracing: introduce bpf raw tracepoints
From: Alexei Starovoitov @ 2018-03-26 16:00 UTC (permalink / raw)
To: Steven Rostedt, Daniel Borkmann
Cc: davem, torvalds, peterz, netdev, kernel-team, linux-api
In-Reply-To: <20180326114725.1999288a@gandalf.local.home>
On 3/26/18 8:47 AM, Steven Rostedt wrote:
> On Mon, 26 Mar 2018 17:32:02 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>
>> On 03/26/2018 05:04 PM, Steven Rostedt wrote:
>>> On Mon, 26 Mar 2018 10:28:03 +0200
>>> Daniel Borkmann <daniel@iogearbox.net> wrote:
>>>
>>>>> tracepoint base kprobe+bpf tracepoint+bpf raw_tracepoint+bpf
>>>>> task_rename 1.1M 769K 947K 1.0M
>>>>> urandom_read 789K 697K 750K 755K
>>>>
>>>> Applied to bpf-next, thanks Alexei!
>>>
>>> Please wait till you have the proper acks. Some of this affects
>>> tracing.
>>
>> Ok, I thought time up to v5 was long enough. Anyway, in case there are
>> objections I can still toss out the series from bpf-next tree worst case
>> should e.g. follow-up fixups not be appropriate.
>
> Yeah, I've been traveling a bit which slowed down my review process
> (trying to catch up).
v1 of this set was posted Feb 28.
imo one month is not an acceptable delay for maintainer to review
the patches. You really need to consider group maintainership as
we do with Daniel for bpf tree.
> My main concern is with patch 6, as there are
> external users of those functions. Although, we generally don't cater
> to out of tree code, we play nice with LTTng, and I don't want to break
> it.
out-of-tree module is out of tree. I'm beyond surprised that you
propose to keep for_each_kernel_tracepoint() as-is with zero in-tree
users in order to keep lttng working.
> I also should probably pull in the patches and run them through my
> tests to make sure they don't have any other side effects.
so let me rephrase.
You're saying that a change to a function with zero in-tree users
can somehow break your tests?
How is that possible?
Does it mean you also have some out-of-tree modules that will break?
and that _is_ the real reason for objection?
^ permalink raw reply
* [PATCH bpf-next]: add sock_ops R/W access to ipv4 tos
From: Nikita V. Shirokov @ 2018-03-26 15:36 UTC (permalink / raw)
To: brakmo, ast, daniel, netdev; +Cc: kernel-team, tehnerd
bpf: Add sock_ops R/W access to ipv4 tos
Sample usage for tos:
bpf_getsockopt(skops, SOL_IP, IP_TOS, &v, sizeof(v))
where skops is a pointer to the ctx (struct bpf_sock_ops).
Signed-off-by: Nikita V. Shirokov <tehnerd@fb.com>
---
net/core/filter.c | 35 +++++++++++++++++++++++++++++++++++
1 file changed, 35 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index 00c711c..afd8255 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -3462,6 +3462,27 @@ BPF_CALL_5(bpf_setsockopt, struct bpf_sock_ops_kern *, bpf_sock,
ret = -EINVAL;
}
#ifdef CONFIG_INET
+ } else if (level == SOL_IP) {
+ if (optlen != sizeof(int) || sk->sk_family != AF_INET)
+ return -EINVAL;
+
+ val = *((int *)optval);
+ /* Only some options are supported */
+ switch (optname) {
+ case IP_TOS:
+ if (val < -1 || val > 0xff) {
+ ret = -EINVAL;
+ } else {
+ struct inet_sock *inet = inet_sk(sk);
+
+ if (val == -1)
+ val = 0;
+ inet->tos = val;
+ }
+ break;
+ default:
+ ret = -EINVAL;
+ }
#if IS_ENABLED(CONFIG_IPV6)
} else if (level == SOL_IPV6) {
if (optlen != sizeof(int) || sk->sk_family != AF_INET6)
@@ -3561,6 +3582,20 @@ BPF_CALL_5(bpf_getsockopt, struct bpf_sock_ops_kern *, bpf_sock,
} else {
goto err_clear;
}
+ } else if (level == SOL_IP) {
+ struct inet_sock *inet = inet_sk(sk);
+
+ if (optlen != sizeof(int) || sk->sk_family != AF_INET)
+ goto err_clear;
+
+ /* Only some options are supported */
+ switch (optname) {
+ case IP_TOS:
+ *((int *)optval) = (int)inet->tos;
+ break;
+ default:
+ goto err_clear;
+ }
#if IS_ENABLED(CONFIG_IPV6)
} else if (level == SOL_IPV6) {
struct ipv6_pinfo *np = inet6_sk(sk);
--
2.9.5
^ permalink raw reply related
* Re: [PATCH v5 2/2] net: ethernet: nixge: Add support for National Instruments XGE netdev
From: Moritz Fischer @ 2018-03-26 15:56 UTC (permalink / raw)
To: David Miller
Cc: mdf, linux-kernel, devicetree, netdev, robh+dt, andrew,
f.fainelli
In-Reply-To: <20180326.113830.932262386304702367.davem@davemloft.net>
Hi David,
On Mon, Mar 26, 2018 at 11:38:30AM -0400, David Miller wrote:
> From: Moritz Fischer <mdf@kernel.org>
> Date: Fri, 23 Mar 2018 13:41:28 -0700
>
> > +static void nixge_hw_dma_bd_release(struct net_device *ndev)
> > +{
> > + int i;
> > + struct nixge_priv *priv = netdev_priv(ndev);
>
> Please order local variables from longest to shortest line (ie. reverse
> christmas tree layout).
Sure.
>
> > +static int nixge_hw_dma_bd_init(struct net_device *ndev)
> > +{
> > + u32 cr;
> > + int i;
> > + struct sk_buff *skb;
> > + struct nixge_priv *priv = netdev_priv(ndev);
>
> Likewise.
Sure.
>
> > +static void __nixge_device_reset(struct nixge_priv *priv, off_t offset)
> > +{
> > + u32 status;
> > + int err;
> > + /* Reset Axi DMA. This would reset NIXGE Ethernet core as well.
> > + * The reset process of Axi DMA takes a while to complete as all
> > + * pending commands/transfers will be flushed or completed during
> > + * this reset process.
> > + */
>
> Please put an empty line between the local variable declarations
> and this comment.
Will do in v6
Thanks for your review!
Cheers Moritz
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Steven Rostedt @ 2018-03-26 15:56 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api
In-Reply-To: <20180324023038.938665-7-ast@fb.com>
On Fri, 23 Mar 2018 19:30:34 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> +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;
> - for (iter = begin; iter < end; iter++)
> - fct(*iter, priv);
> + return NULL;
> + for (iter = begin; iter < end; iter++) {
> + ret = fct(*iter, priv);
> + if (ret)
> + return ret;
So you just stopped the loop here. You have an inconsistent state. What
about the functions that were called before. How do you undo them? Or
what about the rest that haven't been touched. This function gives no
feedback to the caller.
-- Steve
> + }
> + return NULL;
> }
^ permalink raw reply
* Re: [PATCH v5 bpf-next 06/10] tracepoint: compute num_args at build time
From: Mathieu Desnoyers @ 2018-03-26 15:55 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: rostedt, David S. Miller, Daniel Borkmann, Linus Torvalds,
Peter Zijlstra, netdev, kernel-team, linux-api, Frank Ch. Eigler
In-Reply-To: <5bcacdb5-e72f-b67a-4884-61fcedf0938a@fb.com>
----- On Mar 26, 2018, at 11:42 AM, Alexei Starovoitov ast@fb.com wrote:
> On 3/26/18 8:14 AM, Mathieu Desnoyers wrote:
>> ----- On Mar 26, 2018, at 11:02 AM, rostedt rostedt@goodmis.org wrote:
>>
>>> On Fri, 23 Mar 2018 19:30:34 -0700
>>> Alexei Starovoitov <ast@fb.com> wrote:
>>>
>>>> From: Alexei Starovoitov <ast@kernel.org>
>>>>
>>>> add fancy macro to compute number of arguments passed into tracepoint
>>>> at compile time and store it as part of 'struct tracepoint'.
>>>> The number is necessary to check safety of bpf program access that
>>>> is coming in subsequent patch.
>>>>
>>>> for_each_tracepoint_range() api has no users inside the kernel.
>>>> Make it more useful with ability to stop for_each() loop depending
>>>> via callback return value.
>>>> In such form it's used in subsequent patch.
>>>
>>> I believe this is used by LTTng.
>>
>> Indeed, and by SystemTAP as well.
>>
>> What justifies the need to stop mid-iteration ? A less intrusive alternative
>> would be to use the "priv" data pointer to keep state telling further calls
>> to return immediately. Does performance of iteration over tracepoints really
>> matter here so much that stopping iteration immediately is worth it ?
>
> I'm sure both you and Steven are not serious when you object
> to _in-tree_ change to for_each_kernel_tracepoint() that
> affects _out-of_tree_ modules?
>
> Just change your module to 'return NULL' instead of plain 'return'.
I never said I objected to adapt the LTTng out of tree code. If there is a
solid reason for changing the kernel API, I will adapt my code to those
changes.
What I'm trying to understand here is whether there is solid ground for
the added complexity you are proposing. Is it a performance enhancement ?
If so, explanation of the use cases targeted, and numbers that measure
performance improvements are needed.
How is your patch making tracepoints "more useful" ?
Thanks,
Mathieu
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* Re: [PATCH v2] of_net: Implement of_get_nvmem_mac_address helper
From: Andrew Lunn @ 2018-03-26 15:50 UTC (permalink / raw)
To: Mike Looijmans
Cc: netdev, linux-kernel, devicetree, f.fainelli, robh+dt,
frowand.list
In-Reply-To: <1522046489-19652-1-git-send-email-mike.looijmans@topic.nl>
On Mon, Mar 26, 2018 at 08:41:29AM +0200, Mike Looijmans wrote:
> It's common practice to store MAC addresses for network interfaces into
> nvmem devices. However the code to actually do this in the kernel lacks,
> so this patch adds of_get_nvmem_mac_address() for drivers to obtain the
> address from an nvmem cell provider.
>
> This is particulary useful on devices where the ethernet interface cannot
> be configured by the bootloader, for example because it's in an FPGA.
>
> Tested by adapting the cadence macb driver to call this instead of
> of_get_mac_address().
Hi Mike
I can understand you not wanting to modify all the call sites for
of_get_mac_address().
However, the name of_get_nvmem_mac_address() suggests it gets the MAC
address from NVMEM. I think people are going to be surprised when they
find it first tries for a MAC address directly in device tree. I would
drop the call to of_get_mac_address(), and have the MAC driver call
both.
You could also maybe take a look at fwnode_get_mac_address(). It
should work for both OF and ACPI. It fits better because is passes a
char * for the address. You could make that do both, and call it from
the macb driver. dev_fwnode() probably does what you want.
Andrew
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox