* Re: [PATCH] cpsw: ethtool: add support for nway reset
From: David Miller @ 2016-11-30 0:41 UTC (permalink / raw)
To: yegorslists; +Cc: netdev, linux-omap, grygorii.strashko, mugunthanvnm
In-Reply-To: <1480326472-5849-1-git-send-email-yegorslists@googlemail.com>
From: yegorslists@googlemail.com
Date: Mon, 28 Nov 2016 10:47:52 +0100
> From: Yegor Yefremov <yegorslists@googlemail.com>
>
> This patch adds support for ethtool's '-r' command. Restarting
> N-WAY negotiation can be useful to activate newly changed EEE
> settings etc.
>
> Signed-off-by: Yegor Yefremov <yegorslists@googlemail.com>
This doesn't apply cleanly to net-next.
^ permalink raw reply
* Re: [PATCH net-next v3 0/4] Fix OdroidC2 Gigabit Tx link issue
From: David Miller @ 2016-11-30 0:38 UTC (permalink / raw)
To: jbrunet-rdvid1DuHRBWk0Htik3J/w
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, devicetree-u79uwXL29TY76Z2rM5mHXA,
f.fainelli-Re5JQEeQqe8AvxtiuMwx3w, carlo-KA+7E9HrN00dnm+yROfE0A,
khilman-rdvid1DuHRBWk0Htik3J/w, peppe.cavallaro-qxv4g6HH51o,
alexandre.torgue-qxv4g6HH51o,
martin.blumenstingl-gM/Ye1E23mwN+BqQ9rBEUg,
neolynx-Re5JQEeQqe8AvxtiuMwx3w, andrew-g2DYL2Zd6BY,
narmstrong-rdvid1DuHRBWk0Htik3J/w,
linux-amlogic-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1480326409-25419-1-git-send-email-jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
From: Jerome Brunet <jbrunet-rdvid1DuHRBWk0Htik3J/w@public.gmane.org>
Date: Mon, 28 Nov 2016 10:46:45 +0100
> This patchset fixes an issue with the OdroidC2 board (DWMAC + RTL8211F).
> The platform seems to enter LPI on the Rx path too often while performing
> relatively high TX transfer. This eventually break the link (both Tx and
> Rx), and require to bring the interface down and up again to get the Rx
> path working again.
>
> The root cause of this issue is not fully understood yet but disabling EEE
> advertisement on the PHY prevent this feature to be negotiated.
> With this change, the link is stable and reliable, with the expected
> throughput performance.
>
> The patchset adds options in the generic phy driver to disable EEE
> advertisement, through device tree. The way it is done is very similar
> to the handling of the max-speed property.
Patches 1-3 applied to net-next, thanks.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [net-next PATCH v3 6/6] virtio_net: xdp, add slowpath case for non contiguous buffers
From: Alexei Starovoitov @ 2016-11-30 0:37 UTC (permalink / raw)
To: John Fastabend
Cc: eric.dumazet, daniel, shm, davem, tgraf, john.r.fastabend, netdev,
bblanco, brouer
In-Reply-To: <20161129201133.26851.31803.stgit@john-Precision-Tower-5810>
On Tue, Nov 29, 2016 at 12:11:33PM -0800, John Fastabend wrote:
> virtio_net XDP support expects receive buffers to be contiguous.
> If this is not the case we enable a slowpath to allow connectivity
> to continue but at a significan performance overhead associated with
> linearizing data. To make it painfully aware to users that XDP is
> running in a degraded mode we throw an xdp buffer error.
>
> To linearize packets we allocate a page and copy the segments of
> the data, including the header, into it. After this the page can be
> handled by XDP code flow as normal.
>
> Then depending on the return code the page is either freed or sent
> to the XDP xmit path. There is no attempt to optimize this path.
>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
...
> +/* The conditions to enable XDP should preclude the underlying device from
> + * sending packets across multiple buffers (num_buf > 1). However per spec
> + * it does not appear to be illegal to do so but rather just against convention.
> + * So in order to avoid making a system unresponsive the packets are pushed
> + * into a page and the XDP program is run. This will be extremely slow and we
> + * push a warning to the user to fix this as soon as possible. Fixing this may
> + * require resolving the underlying hardware to determine why multiple buffers
> + * are being received or simply loading the XDP program in the ingress stack
> + * after the skb is built because there is no advantage to running it here
> + * anymore.
> + */
...
> if (num_buf > 1) {
> bpf_warn_invalid_xdp_buffer();
> - goto err_xdp;
> +
> + /* linearize data for XDP */
> + xdp_page = xdp_linearize_page(rq, num_buf,
> + page, offset, &len);
> + if (!xdp_page)
> + goto err_xdp;
in case when we're 'lucky' the performance will silently be bad.
Can we do warn_once here? so at least something in dmesg points out
that performance is not as expected. Am I reading it correctly that
you had to do a special kernel hack to trigger this situation and
in all normal cases it's not the case?
^ permalink raw reply
* Re: bpf debug info
From: Alexei Starovoitov @ 2016-11-30 0:28 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Jakub Kicinski, netdev, Brenden Blanco, Thomas Graf, Wangnan,
He Kuang, kernel-team
In-Reply-To: <583DE3AE.8030103@iogearbox.net>
On Tue, Nov 29, 2016 at 09:23:10PM +0100, Daniel Borkmann wrote:
> On 11/29/2016 07:51 PM, Alexei Starovoitov wrote:
> >On Tue, Nov 29, 2016 at 03:38:18PM +0000, Jakub Kicinski wrote:
> [...]
> >>>>So next step is to improve verifier messages to be more human friendly.
> >>>>The step after is to introduce BPF_COMMENT pseudo instruction
> >>>>that will be ignored by the interpreter yet it will contain the text
> >>>>of original source code. Then llvm-objdump step won't be necessary.
> >>>>The bpf loader will load both instructions and pieces of C sources.
> >>>>Then verifier errors should be even easier to read and humans
> >>>>can easily understand the purpose of the program.
> >>>
> >>>So the BPF_COMMENT pseudo insn will get stripped away from the insn array
> >>>after verification step, so we don't need to hold/account for this mem? I
> >>>assume in it's ->imm member it will just hold offset into text blob?
> >>
> >>Associating any form of opaque data with programs always makes me
> >>worried about opening a side channel of communication with a specialized
> >>user space implementations/compilers. But I guess if the BPF_COMMENTs
> >>are stripped in the verifier as Daniel assumes drivers and JITs will
> >>never see it.
> >
> >yes. the idea that it's a comment. It can contain any text,
> >not only C code, but any other language.
> >It's definitely going to be stripped before JITs and kernel will
> >not make any safety or translation decisions based on such comment.
> >
> >>Just to clarify, however - is there any reason why pushing the source
> >>code into the kernel is necessary? Or is it just for convenience?
> >>Provided the user space loader has access to the debug info it should
> >>have no problems matching the verifier output to code lines?
> >
> >correct. just for convenience. The user space has to keep .o around,
> >since it can crash, would have to reload and so on.
> >Only for some script that ssh-es into servers and wants to see
> >what is being loaded, it might help to dump full asm and these comments
> >along with prog_digest that Daniel is working on in parallel.
>
> Which would mean we'd need to keep it around somewhere (prog aux data?)
> in post-verification time (so potentially drivers/JITs could see it, too,
> just not inside insn stream). Some API glue code could probably blind
> this information for the JITing time to stop incentive of playing side
> channel games (e.g. core code could encrypt the pointer value and only
> core kernel knows how to access that data, no modules, no out-of-tree
> code). The other thing I'm wondering is, when we strip this info anyway
> from the insn stream to keep it in aux data (so it can later be reconstructed
> on a dump), then perhaps that is best done before prog loading time? It
> would then allow to keep complexity with stripping that insns out of the
> verifier. If semantics are that these comments are acting as a hole/gap
> (in a similar sense of what we have with cBPF today), then it can never
> become a jmp target and loaders could strip it out already (instead of
> teaching DFS, etc about it), and prepare a meta data structure in bpf_attr
> for bpf(2), and verifier works based on that one. What makes this problematic
> however is when you have rewrites in the kernel (ctx access, constant
> blinding, etc), but perhaps they could just adjust the offsets from that
> meta data thing as well?
yes. all correct.
if we keep comment==nop instructions as part of the program, it's not great,
since we'll be wasting performance for no strong reason.
If we remove them from instruction stream after the verifier then they
can only be useful in verifier messages and that's not much better
than existing 'llvm-objdump -S file.o' approach.
Hmm.
> >Alternatively instead of doing BPF_COMMENT we can load the whole .o
> >as-is into bpffs as a blob. Later (based on digest) the kernel can
> >dump such .o back for user space to run objdump on. It all can be
> >done without kernel involvement. Like tc command can copy .o and so on.
> >But not everything is using tc.
>
> That means kernel must ensure/verify that loaded insns also come from
> that claimed object file; not sure if easily possible w/o parsing elf.
> It could work if the kernel loads everything based on the content of
> the object file itself,
it will only check that program section of file contain valid insns.
the user may still cheat with junk in dwarf section of such elf.
Sounds more and more that this should really be solved by user space
and correlation to be done via prog_digest.
^ permalink raw reply
* Re: [PATCH net-next 2/2] tcp: allow to turn tcp timestamp randomization off
From: Florian Westphal @ 2016-11-30 0:21 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Florian Westphal, netdev
In-Reply-To: <1480442832.18162.148.camel@edumazet-glaptop3.roam.corp.google.com>
Eric Dumazet <eric.dumazet@gmail.com> wrote:
> On Tue, 2016-11-29 at 16:45 +0100, Florian Westphal wrote:
> > Eric says: "By looking at tcpdump, and TS val of xmit packets of multiple
> > flows, we can deduct the relative qdisc delays (think of fq pacing).
> > This should work even if we have one flow per remote peer."
> >
> > Having random per flow (or host) offsets doesn't allow that anymore so add
> > a way to turn this off.
> >
> > Suggested-by: Eric Dumazet <edumazet@google.com>
> > Signed-off-by: Florian Westphal <fw@strlen.de>
> > ---
>
> Excellent, thanks !
>
> Acked-by: Eric Dumazet <edumazet@google.com>
Thanks for the ack, I missed connect() side though so this doesn't work
for outgoing connections, sorry :-/
I will send a v2.
^ permalink raw reply
* Re: [PATCH net-next 5/5] udp: add recvmmsg implementation
From: David Miller @ 2016-11-30 0:22 UTC (permalink / raw)
To: hannes; +Cc: pabeni, netdev, edumazet, brouer, sd
In-Reply-To: <1165706e-b828-cb12-4bea-b77ccca1cb95@stressinduktion.org>
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Fri, 25 Nov 2016 18:09:00 +0100
> During review we discussed on how to handle major errors in the kernel:
>
> The old code and the new code still can report back success even though
> the kernel got back an EFAULT while copying from kernel space to user
> space (due to bad pointers).
>
> I favor that we drop all packets (also the already received batches) in
> this case and let the code report -EFAULT and increase sk_drops for all
> dropped packets from the queue.
>
> Currently sk_err is set so the next syscall would get an -EFAULT, which
> seems very bad and can also be overwritten by incoming icmp packets, so
> we never get a notification that we actually had a bad pointer somewhere
> in the mmsghdr. Also delivering -EFAULT on the follow-up syscalls really
> will make people confused that use strace.
>
> If people would like to know the amount of packets dropped we can make
> sk_drops readable by an getsockopt.
>
> Thoughts?
>
> Unfortunately the interface doesn't allow for better error handling.
I think this is a major problem.
If, as a side effect of batch dequeueing the SKBs from the socket,
you cannot stop properly mid-transfer if an error occurs, well then
you simply cannot batch like that.
You have to stop the exact byte where an error occurs mid-stream,
return the successful amount of bytes transferred, and then return
the error on the next recvmmsg call.
There is no other sane error reporting strategy.
If I get 4 frames, and the kernel can successfully copy the first
three and get an -EFAULT on the 4th. Dammit you better tell the
application this so it can properly process the first 3 packets and
then determine how it is going to error out and recover for the 4th
one.
If we need to add prioritized sk_err stuff, or another value like
"sk_app_err" to handle the ICMP vs. -EFAULT issue, so be it.
I know what you guys are thinking, in that you can't figure out a
way to avoid the transactional overhead if it is necessary to
"put back" some SKBs if one of them in the batch gets a fault.
That's too bad, we need a proper implementation and proper error
reporting. Those performance numbers are useless if we effectively
lose error notifications.
^ permalink raw reply
* Re: [PATCH] net: ethernet: ti: cpsw: fix ASSERT_RTNL() warning during resume
From: Ivan Khoronzhuk @ 2016-11-30 0:18 UTC (permalink / raw)
To: Grygorii Strashko
Cc: David S. Miller, netdev, Mugunthan V N, Sekhar Nori, linux-kernel,
linux-omap, Dave Gerlach
In-Reply-To: <20161129222703.10908-1-grygorii.strashko@ti.com>
On Tue, Nov 29, 2016 at 04:27:03PM -0600, Grygorii Strashko wrote:
> netif_set_real_num_tx/rx_queues() are required to be called with rtnl_lock
> taken, otherwise ASSERT_RTNL() warning will be triggered - which happens
> now during System resume from suspend:
> cpsw_resume()
> |- cpsw_ndo_open()
> |- netif_set_real_num_tx/rx_queues()
> |- ASSERT_RTNL();
>
> Hence, fix it by surrounding cpsw_ndo_open() by rtnl_lock/unlock() calls.
>
> Cc: Dave Gerlach <d-gerlach@ti.com>
> Cc: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> Fixes: commit e05107e6b747 ("net: ethernet: ti: cpsw: add multi queue support")
> Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
Reviewed-by: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> ---
> drivers/net/ethernet/ti/cpsw.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/ethernet/ti/cpsw.c b/drivers/net/ethernet/ti/cpsw.c
> index ae1ec6a..fd6c03b 100644
> --- a/drivers/net/ethernet/ti/cpsw.c
> +++ b/drivers/net/ethernet/ti/cpsw.c
> @@ -2944,6 +2944,8 @@ static int cpsw_resume(struct device *dev)
> /* Select default pin state */
> pinctrl_pm_select_default_state(dev);
>
> + /* shut up ASSERT_RTNL() warning in netif_set_real_num_tx/rx_queues */
> + rtnl_lock();
> if (cpsw->data.dual_emac) {
> int i;
>
> @@ -2955,6 +2957,8 @@ static int cpsw_resume(struct device *dev)
> if (netif_running(ndev))
> cpsw_ndo_open(ndev);
> }
> + rtnl_unlock();
> +
> return 0;
> }
> #endif
> --
> 2.10.1
>
^ permalink raw reply
* Re: [PATCH net-next v3 4/4] bpf: Add tests and samples for LWT-BPF
From: Alexei Starovoitov @ 2016-11-30 0:17 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, netdev, daniel, tom, roopa, hannes
In-Reply-To: <25a6f8d0d56175bb27a05c18eb54bc3bc5c09fa1.1480424542.git.tgraf@suug.ch>
On Tue, Nov 29, 2016 at 02:21:23PM +0100, Thomas Graf wrote:
> Adds a series of test to verify the functionality of attaching
> BPF programs at LWT hooks.
>
> Also adds a sample which collects a histogram of packet sizes which
> pass through an LWT hook.
>
> $ ./lwt_len_hist.sh
> Starting netserver with host 'IN(6)ADDR_ANY' port '12865' and family AF_UNSPEC
> MIGRATED TCP STREAM TEST from 0.0.0.0 (0.0.0.0) port 0 AF_INET to 192.168.253.2 () port 0 AF_INET : demo
> Recv Send Send
> Socket Socket Message Elapsed
> Size Size Size Time Throughput
> bytes bytes bytes secs. 10^6bits/sec
>
> 87380 16384 16384 10.00 39857.69
Nice!
> + ret = bpf_redirect(ifindex, 0);
> + if (ret < 0) {
> + printk("bpf_redirect() failed: %d\n", ret);
> + return BPF_DROP;
> + }
this 'if' looks a bit weird. You're passing 0 as flags,
so this helper will always succeed.
Other sample code often does 'return bpf_redirect(...)'
due to this reasoning.
^ permalink raw reply
* Re: [PATCH net-next v3 3/4] bpf: BPF for lightweight tunnel infrastructure
From: Alexei Starovoitov @ 2016-11-30 0:15 UTC (permalink / raw)
To: Thomas Graf; +Cc: davem, netdev, daniel, tom, roopa, hannes
In-Reply-To: <7cc79a82e49996a9ebbff861af471018fdf118fb.1480424542.git.tgraf@suug.ch>
On Tue, Nov 29, 2016 at 02:21:22PM +0100, Thomas Graf wrote:
> Registers new BPF program types which correspond to the LWT hooks:
> - BPF_PROG_TYPE_LWT_IN => dst_input()
> - BPF_PROG_TYPE_LWT_OUT => dst_output()
> - BPF_PROG_TYPE_LWT_XMIT => lwtunnel_xmit()
>
> The separate program types are required to differentiate between the
> capabilities each LWT hook allows:
>
> * Programs attached to dst_input() or dst_output() are restricted and
> may only read the data of an skb. This prevent modification and
> possible invalidation of already validated packet headers on receive
> and the construction of illegal headers while the IP headers are
> still being assembled.
>
> * Programs attached to lwtunnel_xmit() are allowed to modify packet
> content as well as prepending an L2 header via a newly introduced
> helper bpf_skb_push(). This is safe as lwtunnel_xmit() is invoked
> after the IP header has been assembled completely.
>
> All BPF programs receive an skb with L3 headers attached and may return
> one of the following error codes:
>
> BPF_OK - Continue routing as per nexthop
> BPF_DROP - Drop skb and return EPERM
> BPF_REDIRECT - Redirect skb to device as per redirect() helper.
> (Only valid in lwtunnel_xmit() context)
>
> The return codes are binary compatible with their TC_ACT_
> relatives to ease compatibility.
>
> Signed-off-by: Thomas Graf <tgraf@suug.ch>
...
> +#define LWT_BPF_MAX_HEADROOM 128
why 128?
btw I'm thinking for XDP to use 256, so metadata can be stored in there.
> +static int run_lwt_bpf(struct sk_buff *skb, struct bpf_lwt_prog *lwt,
> + struct dst_entry *dst, bool can_redirect)
> +{
> + int ret;
> +
> + /* Preempt disable is needed to protect per-cpu redirect_info between
> + * BPF prog and skb_do_redirect(). The call_rcu in bpf_prog_put() and
> + * access to maps strictly require a rcu_read_lock() for protection,
> + * mixing with BH RCU lock doesn't work.
> + */
> + preempt_disable();
> + rcu_read_lock();
> + bpf_compute_data_end(skb);
> + ret = BPF_PROG_RUN(lwt->prog, skb);
> + rcu_read_unlock();
> +
> + switch (ret) {
> + case BPF_OK:
> + break;
> +
> + case BPF_REDIRECT:
> + if (!can_redirect) {
> + WARN_ONCE(1, "Illegal redirect return code in prog %s\n",
> + lwt->name ? : "<unknown>");
> + ret = BPF_OK;
> + } else {
> + ret = skb_do_redirect(skb);
I think this assumes that program did bpf_skb_push and L2 header is present.
Would it make sense to check that mac_header < network_header here to make
sure that it actually happened? I think the cost of single 'if' isn't much.
Also skb_do_redirect() can redirect to l3 tunnels like ipip ;)
so program shouldn't be doing bpf_skb_push in such case...
May be rename bpf_skb_push to bpf_skb_push_l2 ?
since it's doing skb_reset_mac_header(skb); at the end of it?
Or it's probably better to use 'flags' argument to tell whether
bpf_skb_push() should set mac_header or not ? Then this bit:
> + case BPF_OK:
> + /* If the L3 header was expanded, headroom might be too
> + * small for L2 header now, expand as needed.
> + */
> + ret = xmit_check_hhlen(skb);
will work fine as well...
which probably needs "mac_header wasn't set" check? or it's fine?
All bpf bits look great. Thanks!
^ permalink raw reply
* Re: [PATCH] net: stmmac: enable tx queue 0 for gmac4 IPs synthesized with multiple TX queues
From: David Miller @ 2016-11-30 0:11 UTC (permalink / raw)
To: niklas.cassel
Cc: peppe.cavallaro, alexandre.torgue, niklass, netdev, linux-kernel
In-Reply-To: <1479998194-7113-1-git-send-email-niklass@axis.com>
From: Niklas Cassel <niklas.cassel@axis.com>
Date: Thu, 24 Nov 2016 15:36:33 +0100
> From: Niklas Cassel <niklas.cassel@axis.com>
>
> The dwmac4 IP can synthesized with 1-8 number of tx queues.
> On an IP synthesized with DWC_EQOS_NUM_TXQ > 1, all txqueues are disabled
> by default. For these IPs, the bitfield TXQEN is R/W.
>
> Always enable tx queue 0. The write will have no effect on IPs synthesized
> with DWC_EQOS_NUM_TXQ == 1.
>
> The driver does still not utilize more than one tx queue in the IP.
>
> Signed-off-by: Niklas Cassel <niklas.cassel@axis.com>
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH] net: macb: Write only necessary bits in NCR in macb reset
From: David Miller @ 2016-11-30 0:05 UTC (permalink / raw)
To: harini.katakam
Cc: nicolas.ferre, harinikatakamlinux, netdev, linux-kernel, harinik,
michals
In-Reply-To: <1480325029-39224-1-git-send-email-harinik@xilinx.com>
From: Harini Katakam <harini.katakam@xilinx.com>
Date: Mon, 28 Nov 2016 14:53:49 +0530
> In macb_reset_hw, use read-modify-write to disable RX and TX.
> This way exiting settings and reserved bits wont be disturbed.
> Use the same method for clearing statistics as well.
>
> Signed-off-by: Harini Katakam <harinik@xilinx.com>
This doesn't make much sense to me.
Consider the two callers of this function.
macb_init_hw() is going to do a non-masking write to the NCR
register:
/* Enable TX and RX */
macb_writel(bp, NCR, MACB_BIT(RE) | MACB_BIT(TE) | MACB_BIT(MPE));
So obviously no other writable fields matter at all for programming
the chip properly, otherwise macb_init_hw() would "or" in the bits
after a read of NCR. But that's not what this code does, it
writes "RE | TE | MPE" directly.
And the other caller is macb_close() which is shutting down the
chip so can zero out all the other bits and it can't possibly
matter, also due to the assertion above about macb_init_hw()
showing that only the RE, TE, and MPE bits matter for proper
functioning of the chip.
You haven't shown a issue caused by the way the code works now, so
this patch isn't fixing a bug. In fact, the "bit preserving" would
even be misleading to someone reading the code. They will ask
themselves what bits need to be preserved, and as shown above none of
them need to be.
I'm not applying this, sorry.
^ permalink raw reply
* RE: [PATCH net-netx] net: lan78xx: add LAN7801 MAC-only support
From: Woojung.Huh @ 2016-11-30 0:01 UTC (permalink / raw)
To: f.fainelli, davem, andrew; +Cc: netdev, UNGLinuxDriver
In-Reply-To: <b0dea0b1-11d1-3d58-c055-41824eaf1d1f@gmail.com>
> > It seems phy_unregister_fixup() will be needed for module network driver.
> > phy_fixup_list keeps the list even after unloading module.
> > Do you know any update is waiting for submission? If not, I'll make patch.
>
> Oh, yes, that's a good point, we need such a thing, so far fixups have
> been exclusively used by code that is built-in, but there really is not
> a reason for that. Please go ahead and cook a patch for this, thanks!
OK. Will do it.
Thanks.
- Woojung
^ permalink raw reply
* Re: [PATCH] net: arc_emac: add dependencies on associated arches and compile test
From: David Miller @ 2016-11-29 23:57 UTC (permalink / raw)
To: pbrobinson; +Cc: zhengxing, al.kochet, tremyfr, netdev
In-Reply-To: <20161128071237.9016-1-pbrobinson@gmail.com>
From: Peter Robinson <pbrobinson@gmail.com>
Date: Mon, 28 Nov 2016 07:12:37 +0000
> Add dependencies on the architectures that support these devices and
> add compile test to ensure ongoing code build coverage.
>
> Signed-off-by: Peter Robinson <pbrobinson@gmail.com>
Applied.
^ permalink raw reply
* Re: [PATCH net-netx] net: lan78xx: add LAN7801 MAC-only support
From: Florian Fainelli @ 2016-11-29 23:57 UTC (permalink / raw)
To: Woojung.Huh, davem, andrew; +Cc: netdev, UNGLinuxDriver
In-Reply-To: <9235D6609DB808459E95D78E17F2E43D409700F2@CHN-SV-EXMX02.mchp-main.com>
On 11/29/2016 03:55 PM, Woojung.Huh@microchip.com wrote:
>> There are two ways to get these settings propagated to the PHY driver:
>>
>> - using a board fixup which is going to be invoked during
>> drv->config_init() time
>>
>> - specifying a phydev->dev_flags and reading it from the PHY driver to
>> act upon and configure the PHY based on that value, there are only
>> 32-bits available though, and you need to make sure they are not
>> conflicting with other potential users in tree
>>
>> My preference would go with 1, since you could just register it in your
>> PHY driver and re-use the code you are proposing to include here.
>
> Florian,
>
> It seems phy_unregister_fixup() will be needed for module network driver.
> phy_fixup_list keeps the list even after unloading module.
> Do you know any update is waiting for submission? If not, I'll make patch.
Oh, yes, that's a good point, we need such a thing, so far fixups have
been exclusively used by code that is built-in, but there really is not
a reason for that. Please go ahead and cook a patch for this, thanks!
--
Florian
^ permalink raw reply
* RE: [PATCH net-netx] net: lan78xx: add LAN7801 MAC-only support
From: Woojung.Huh @ 2016-11-29 23:55 UTC (permalink / raw)
To: f.fainelli, davem, andrew; +Cc: netdev, UNGLinuxDriver
In-Reply-To: <dcabc07f-ab5f-ad25-5beb-09f717c06e4e@gmail.com>
> There are two ways to get these settings propagated to the PHY driver:
>
> - using a board fixup which is going to be invoked during
> drv->config_init() time
>
> - specifying a phydev->dev_flags and reading it from the PHY driver to
> act upon and configure the PHY based on that value, there are only
> 32-bits available though, and you need to make sure they are not
> conflicting with other potential users in tree
>
> My preference would go with 1, since you could just register it in your
> PHY driver and re-use the code you are proposing to include here.
Florian,
It seems phy_unregister_fixup() will be needed for module network driver.
phy_fixup_list keeps the list even after unloading module.
Do you know any update is waiting for submission? If not, I'll make patch.
Thanks.
Woojung
^ permalink raw reply
* Re: [PATCH v2 net-next 2/2] openvswitch: Fix skb->protocol for vlan frames.
From: Jarno Rajahalme @ 2016-11-29 23:32 UTC (permalink / raw)
To: Pravin Shelar; +Cc: Linux Kernel Network Developers, Jiri Benc
In-Reply-To: <CAOrHB_B15uBYL-nVhsCoRKzZ1Akmn0ZDcee57VUXGAjMNEAAWQ@mail.gmail.com>
> On Nov 28, 2016, at 11:21 PM, Pravin Shelar <pshelar@ovn.org> wrote:
>
> On Mon, Nov 28, 2016 at 6:41 PM, Jarno Rajahalme <jarno@ovn.org> wrote:
>> Do not set skb->protocol to be the ethertype of the L3 header, unless
>> the packet only has the L3 header. For a non-hardware offloaded VLAN
>> frame skb->protocol needs to be one of the VLAN ethertypes.
>>
>> Any VLAN offloading is undone on the OVS netlink interface. Also any
>> VLAN tags added by userspace are non-offloaded.
>>
>> Incorrect skb->protocol value on a full-size non-offloaded VLAN skb
>> causes packet drop due to failing MTU check, as the VLAN header should
>> not be counted in when considering MTU in ovs_vport_send().
>>
> I think we should move to is_skb_forwardable() type of packet length
> check in vport-send and get rid of skb-protocol checks altogether.
>
I added a new patch to do this, thanks for the suggestion.
>> Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
>> Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
>> ---
>> v2: Set skb->protocol when an ETH_P_TEB frame is received via ARPHRD_NONE
>> interface.
>>
>> net/openvswitch/datapath.c | 1 -
>> net/openvswitch/flow.c | 30 ++++++++++++++++++++++--------
>> 2 files changed, 22 insertions(+), 9 deletions(-)
> ...
> ...
>> @@ -531,15 +538,22 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
>> if (unlikely(parse_vlan(skb, key)))
>> return -ENOMEM;
>>
>> - skb->protocol = parse_ethertype(skb);
>> - if (unlikely(skb->protocol == htons(0)))
>> + key->eth.type = parse_ethertype(skb);
>> + if (unlikely(key->eth.type == htons(0)))
>> return -ENOMEM;
>>
>> + if (skb->protocol == htons(ETH_P_TEB)) {
>> + if (key->eth.vlan.tci & htons(VLAN_TAG_PRESENT)
>> + && !skb_vlan_tag_present(skb))
>> + skb->protocol = key->eth.vlan.tpid;
>> + else
>> + skb->protocol = key->eth.type;
>> + }
>> +
>
> I am not sure if this work in case of nested vlans.
> Can we move skb-protocol assignment to parse_vlan() to avoid checking
> for non-accelerated vlan case again here?
I did this for the v3 that I sent out a moment ago.
Thanks for the review,
Jarno
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/mlx5e: Implement Fragmented Work Queue (WQ)
From: Eric Dumazet @ 2016-11-29 23:31 UTC (permalink / raw)
To: Saeed Mahameed
Cc: David S. Miller, netdev, Tariq Toukan, Or Gerlitz, Roi Dayan,
Sebastian Ott
In-Reply-To: <1480457972-31286-2-git-send-email-saeedm@mellanox.com>
On Wed, 2016-11-30 at 00:19 +0200, Saeed Mahameed wrote:
> From: Tariq Toukan <tariqt@mellanox.com>
>
> Add new type of struct mlx5_frag_buf which is used to allocate fragmented
> buffers rather than contiguous, and make the Completion Queues (CQs) use
> it as they are big (default of 2MB per CQ in Striding RQ).
>
> This fixes the failures of type:
> "mlx5e_open_locked: mlx5e_open_channels failed, -12"
> due to dma_zalloc_coherent insufficient contiguous coherent memory to
> satisfy the driver's request when the user tries to setup more or larger
> rings.
>
> Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
> Reported-by: Sebastian Ott <sebott@linux.vnet.ibm.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
> ---
> drivers/net/ethernet/mellanox/mlx5/core/alloc.c | 66 +++++++++++++++++++++++
> drivers/net/ethernet/mellanox/mlx5/core/en.h | 2 +-
> drivers/net/ethernet/mellanox/mlx5/core/en_main.c | 10 ++--
> drivers/net/ethernet/mellanox/mlx5/core/wq.c | 26 ++++++---
> drivers/net/ethernet/mellanox/mlx5/core/wq.h | 18 +++++--
> include/linux/mlx5/driver.h | 11 ++++
> 6 files changed, 116 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/alloc.c b/drivers/net/ethernet/mellanox/mlx5/core/alloc.c
> index 2c6e3c7..bc8357d 100644
> --- a/drivers/net/ethernet/mellanox/mlx5/core/alloc.c
> +++ b/drivers/net/ethernet/mellanox/mlx5/core/alloc.c
> @@ -106,6 +106,63 @@ void mlx5_buf_free(struct mlx5_core_dev *dev, struct mlx5_buf *buf)
> }
> EXPORT_SYMBOL_GPL(mlx5_buf_free);
>
> +int mlx5_frag_buf_alloc_node(struct mlx5_core_dev *dev, int size,
> + struct mlx5_frag_buf *buf, int node)
> +{
> + int i;
> +
> + buf->size = size;
> + buf->npages = 1 << get_order(size);
> + buf->page_shift = PAGE_SHIFT;
> + buf->frags = kcalloc(buf->npages, sizeof(struct mlx5_buf_list),
> + GFP_KERNEL);
> + if (!buf->frags)
> + goto err_out;
> +
> + for (i = 0; i < buf->npages; i++) {
> + struct mlx5_buf_list *frag = &buf->frags[i];
> + int frag_sz = min_t(int, size, PAGE_SIZE);
> +
> + frag->buf = mlx5_dma_zalloc_coherent_node(dev, frag_sz,
> + &frag->map, node);
> + if (!frag->buf)
> + goto err_free_buf;
> + if (frag->map & ((1 << buf->page_shift) - 1)) {
> + dma_free_coherent(&dev->pdev->dev, frag_sz,
> + buf->frags[i].buf, buf->frags[i].map);
There is a bug if this happens with i = 0
> + mlx5_core_warn(dev, "unexpected map alignment: 0x%p, page_shift=%d\n",
> + (void *)frag->map, buf->page_shift);
> + goto err_free_buf;
> + }
> + size -= frag_sz;
> + }
> +
> + return 0;
> +
> +err_free_buf:
> + while (--i)
Because this loop will be done about 2^32 times.
> + dma_free_coherent(&dev->pdev->dev, PAGE_SIZE, buf->frags[i].buf,
> + buf->frags[i].map);
> + kfree(buf->frags);
> +err_out:
> + return -ENOMEM;
> +}
^ permalink raw reply
* [PATCH v3 net-next 3/3] openvswitch: Fix skb->protocol for vlan frames.
From: Jarno Rajahalme @ 2016-11-29 23:30 UTC (permalink / raw)
To: netdev; +Cc: jarno, jbenc, pshelar, e
In-Reply-To: <1480462253-114713-1-git-send-email-jarno@ovn.org>
Do not always set skb->protocol to be the ethertype of the L3 header.
For a packet with non-accelerated VLAN tags skb->protocol needs to be
the ethertype of the outermost non-accelerated VLAN ethertype.
Any VLAN offloading is undone on the OVS netlink interface, and any
VLAN tags added by userspace are non-accelerated, as are double tagged
VLAN packets.
Fixes: 018c1dda5f ("openvswitch: 802.1AD Flow handling, actions, vlan parsing, netlink attributes")
Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
v3: Set skb->protocol properly also for double tagged packets, as suggested
by Pravin. This patch is no longer needed for the MTU test failure, as
the new patch 2/3 addresses that.
net/openvswitch/datapath.c | 1 -
net/openvswitch/flow.c | 62 +++++++++++++++++++++++-----------------------
2 files changed, 31 insertions(+), 32 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 2d4c4d3..9c62b63 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -606,7 +606,6 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, struct genl_info *info)
rcu_assign_pointer(flow->sf_acts, acts);
packet->priority = flow->key.phy.priority;
packet->mark = flow->key.phy.skb_mark;
- packet->protocol = flow->key.eth.type;
rcu_read_lock();
dp = get_dp_rcu(net, ovs_header->dp_ifindex);
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 08aa926..e2fe2e5 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -354,6 +354,7 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
res = parse_vlan_tag(skb, &key->eth.vlan);
if (res <= 0)
return res;
+ skb->protocol = key->eth.vlan.tpid;
}
/* Parse inner vlan tag. */
@@ -361,6 +362,11 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
if (res <= 0)
return res;
+ /* If the outer vlan tag was accelerated, skb->protocol should
+ * refelect the inner vlan type. */
+ if (!eth_type_vlan(skb->protocol))
+ skb->protocol = key->eth.cvlan.tpid;
+
return 0;
}
@@ -477,12 +483,18 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
}
/**
- * key_extract - extracts a flow key from an Ethernet frame.
+ * key_extract - extracts a flow key from a packet with or without an
+ * Ethernet header.
* @skb: sk_buff that contains the frame, with skb->data pointing to the
- * Ethernet header
+ * beginning of the packet.
* @key: output flow key
*
- * The caller must ensure that skb->len >= ETH_HLEN.
+ * 'key->mac_proto' must be initialized to indicate the frame type. For an L3
+ * frame 'key->mac_proto' must equal 'MAC_PROTO_NONE', and the caller must
+ * ensure that 'skb->protocol' is set to the ethertype of the L3 header.
+ * Otherwise the presence of an Ethernet header is assumed and the caller must
+ * ensure that skb->len >= ETH_HLEN and that 'skb->protocol' is initialized to
+ * zero.
*
* Returns 0 if successful, otherwise a negative errno value.
*
@@ -498,8 +510,9 @@ static int parse_icmpv6(struct sk_buff *skb, struct sw_flow_key *key,
* of a correct length, otherwise the same as skb->network_header.
* For other key->eth.type values it is left untouched.
*
- * - skb->protocol: the type of the data starting at skb->network_header.
- * Equals to key->eth.type.
+ * - skb->protocol: For non-accelerated VLAN, one of the VLAN ether types,
+ * otherwise the same as key->eth.type, the ether type of the payload
+ * starting at skb->network_header.
*/
static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
{
@@ -531,15 +544,16 @@ static int key_extract(struct sk_buff *skb, struct sw_flow_key *key)
if (unlikely(parse_vlan(skb, key)))
return -ENOMEM;
- skb->protocol = parse_ethertype(skb);
- if (unlikely(skb->protocol == htons(0)))
+ key->eth.type = parse_ethertype(skb);
+ if (unlikely(key->eth.type == htons(0)))
return -ENOMEM;
skb_reset_network_header(skb);
__skb_push(skb, skb->data - skb_mac_header(skb));
}
skb_reset_mac_len(skb);
- key->eth.type = skb->protocol;
+ if (!eth_type_vlan(skb->protocol))
+ skb->protocol = key->eth.type;
/* Network layer. */
if (key->eth.type == htons(ETH_P_IP)) {
@@ -800,29 +814,15 @@ int ovs_flow_key_extract_userspace(struct net *net, const struct nlattr *attr,
if (err)
return err;
- if (ovs_key_mac_proto(key) == MAC_PROTO_NONE) {
- /* key_extract assumes that skb->protocol is set-up for
- * layer 3 packets which is the case for other callers,
- * in particular packets recieved from the network stack.
- * Here the correct value can be set from the metadata
- * extracted above.
- */
- skb->protocol = key->eth.type;
- } else {
- struct ethhdr *eth;
-
- skb_reset_mac_header(skb);
- eth = eth_hdr(skb);
-
- /* Normally, setting the skb 'protocol' field would be
- * handled by a call to eth_type_trans(), but it assumes
- * there's a sending device, which we may not have.
- */
- if (eth_proto_is_802_3(eth->h_proto))
- skb->protocol = eth->h_proto;
- else
- skb->protocol = htons(ETH_P_802_2);
- }
+ /* key_extract assumes that skb->protocol is set-up for
+ * layer 3 packets which is the case for other callers,
+ * in particular packets recieved from the network stack.
+ * Here the correct value can be set from the metadata
+ * extracted above. For layer 2 packets we initialize
+ * skb->protocol to zero and set it in key_extract() while
+ * parsing the L2 headers.
+ */
+ skb->protocol = key->eth.type;
return key_extract(skb, key);
}
--
2.1.4
^ permalink raw reply related
* [PATCH v3 net-next 2/3] openvswitch: Use is_skb_forwardable() for length check.
From: Jarno Rajahalme @ 2016-11-29 23:30 UTC (permalink / raw)
To: netdev; +Cc: jarno, jbenc, pshelar, e
In-Reply-To: <1480462253-114713-1-git-send-email-jarno@ovn.org>
Use is_skb_forwardable() instead of an explicit length check. This
gets around the apparent MTU check failure in OVS test cases when
skb->protocol is not properly set in case of non-accelerated VLAN
skbs.
Suggested-by: Pravin Shelar <pshelar@ovn.org>
Fixes: 5108bbaddc ("openvswitch: add processing of L3 packets")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
---
v3: New patch suggested by Pravin.
net/openvswitch/vport.c | 38 ++++++++++++++------------------------
1 file changed, 14 insertions(+), 24 deletions(-)
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index b6c8524..076b39f 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -464,27 +464,8 @@ int ovs_vport_receive(struct vport *vport, struct sk_buff *skb,
return 0;
}
-static unsigned int packet_length(const struct sk_buff *skb,
- struct net_device *dev)
-{
- unsigned int length = skb->len - dev->hard_header_len;
-
- if (!skb_vlan_tag_present(skb) &&
- eth_type_vlan(skb->protocol))
- length -= VLAN_HLEN;
-
- /* Don't subtract for multiple VLAN tags. Most (all?) drivers allow
- * (ETH_LEN + VLAN_HLEN) in addition to the mtu value, but almost none
- * account for 802.1ad. e.g. is_skb_forwardable().
- */
-
- return length;
-}
-
void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
{
- int mtu = vport->dev->mtu;
-
switch (vport->dev->type) {
case ARPHRD_NONE:
if (mac_proto == MAC_PROTO_ETHERNET) {
@@ -504,11 +485,20 @@ void ovs_vport_send(struct vport *vport, struct sk_buff *skb, u8 mac_proto)
goto drop;
}
- if (unlikely(packet_length(skb, vport->dev) > mtu &&
- !skb_is_gso(skb))) {
- net_warn_ratelimited("%s: dropped over-mtu packet: %d > %d\n",
- vport->dev->name,
- packet_length(skb, vport->dev), mtu);
+ if (unlikely(!is_skb_forwardable(vport->dev, skb))) {
+ /* Log only if the device is up. */
+ if (vport->dev->flags & IFF_UP) {
+ unsigned int length = skb->len
+ - vport->dev->hard_header_len;
+
+ if (!skb_vlan_tag_present(skb)
+ && eth_type_vlan(skb->protocol))
+ length -= VLAN_HLEN;
+
+ net_warn_ratelimited("%s: dropped over-mtu packet %d > %d\n",
+ vport->dev->name, length,
+ vport->dev->mtu);
+ }
vport->dev->stats.tx_errors++;
goto drop;
}
--
2.1.4
^ permalink raw reply related
* [PATCH v3 net-next 1/3] openvswitch: Add a missing break statement.
From: Jarno Rajahalme @ 2016-11-29 23:30 UTC (permalink / raw)
To: netdev; +Cc: jarno, jbenc, pshelar, e
Add a break statement to prevent fall-through from
OVS_KEY_ATTR_ETHERNET to OVS_KEY_ATTR_TUNNEL. Without the break
actions setting ethernet addresses fail to validate with log messages
complaining about invalid tunnel attributes.
Fixes: 0a6410fbde ("openvswitch: netlink: support L3 packets")
Signed-off-by: Jarno Rajahalme <jarno@ovn.org>
Acked-by: Pravin B Shelar <pshelar@ovn.org>
Acked-by: Jiri Benc <jbenc@redhat.com>
---
v3: No change.
net/openvswitch/flow_netlink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index d19044f..c87d359 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -2195,6 +2195,7 @@ static int validate_set(const struct nlattr *a,
case OVS_KEY_ATTR_ETHERNET:
if (mac_proto != MAC_PROTO_ETHERNET)
return -EINVAL;
+ break;
case OVS_KEY_ATTR_TUNNEL:
if (masked)
--
2.1.4
^ permalink raw reply related
* Re: bnx2 breaks Dell R815 BMC IPMI since 4.8
From: Gavin Shan @ 2016-11-29 23:28 UTC (permalink / raw)
To: Brice Goglin; +Cc: Linux Network Development list, Baoquan He
In-Reply-To: <583D26EF.60207@inria.fr>
On Tue, Nov 29, 2016 at 07:57:51AM +0100, Brice Goglin wrote:
>Hello
>
>My Dell PowerEdge R815 doesn't have IPMI anymore when I boot a 4.8
>kernel, the BMC doesn't even ping anymore. Its Ethernet devices are 4 of
>those:
>
>01:00.0 Ethernet controller: Broadcom Corporation NetXtreme II BCM5709 Gigabit Ethernet (rev 20)
> DeviceName: Embedded NIC 1
> Subsystem: Dell NetXtreme II BCM5709 Gigabit Ethernet
> Control: I/O- Mem+ BusMaster+ SpecCycle- MemWINV- VGASnoop- ParErr- Stepping- SERR- FastB2B- DisINTx+
> Status: Cap+ 66MHz- UDF- FastB2B- ParErr- DEVSEL=fast >TAbort- <TAbort- <MAbort- >SERR- <PERR- INTx-
> Latency: 0, Cache Line Size: 64 bytes
> Interrupt: pin A routed to IRQ 42
> Region 0: Memory at e6000000 (64-bit, non-prefetchable) [size=32M]
> Capabilities: <access denied>
> Kernel driver in use: bnx2
> Kernel modules: bnx2
>
>The only change in bnx2 between 4.7 and 4.8 appears to be this one:
>
>commit 3e1be7ad2d38c6bd6aeef96df9bd0a7822f4e51c
>Author: Baoquan He <bhe@redhat.com>
>Date: Fri Sep 9 22:43:12 2016 +0800
>
> bnx2: Reset device during driver initialization
>
>Could you patch actually break the BMC? What do I need to further debug
>this issue?
>
Brice, could you please confirm NCSI is enabled on BMC? It seems NCSI
AEN pakets aren't handled by NCSI stack on BMC side when resetting NIC
on host side. Those AEN packets usually help to reconfigure the (active)
NCSI channel and bring it back to service after the reset.
Thanks,
Gavin
^ permalink raw reply
* RE: [PATCH net-netx] net: lan78xx: add LAN7801 MAC-only support
From: Woojung.Huh @ 2016-11-29 23:20 UTC (permalink / raw)
To: f.fainelli, davem, andrew; +Cc: netdev, UNGLinuxDriver
In-Reply-To: <dcabc07f-ab5f-ad25-5beb-09f717c06e4e@gmail.com>
> There are two ways to get these settings propagated to the PHY driver:
>
> - using a board fixup which is going to be invoked during
> drv->config_init() time
>
> - specifying a phydev->dev_flags and reading it from the PHY driver to
> act upon and configure the PHY based on that value, there are only
> 32-bits available though, and you need to make sure they are not
> conflicting with other potential users in tree
>
> My preference would go with 1, since you could just register it in your
> PHY driver and re-use the code you are proposing to include here.
fixup looks good to me too. Will submit revision soon.
Thanks.
- Woojung
^ permalink raw reply
* Re: [PATCH net] fib_trie: Avoid expensive update of suffix length when not required
From: Alexander Duyck @ 2016-11-29 23:14 UTC (permalink / raw)
To: Robert Shearman; +Cc: David Miller, Netdev, Alexander Duyck
In-Reply-To: <1480441605-27855-1-git-send-email-rshearma@brocade.com>
On Tue, Nov 29, 2016 at 9:46 AM, Robert Shearman <rshearma@brocade.com> wrote:
> With certain distributions of routes it can take a long time to add
> and delete further routes at scale. For example, with a route for
> 10.37.96.0/20 present it takes 47s to add ~200k contiguous /24 routes
> from 8.0.0.0/24 through to 11.138.207.0/24. Perf shows the bottleneck
> is update_suffix:
>
> 40.39% [kernel] [k] update_suffix
> 8.02% libnl-3.so.200.19.0 [.] nl_hash_table_lookup
Well yeah, it will be expensive when you have over 512K entries in a
single node. I'm assuming that is the case based on the fact that
200K routes will double up in the trie node due to broadcast and the
route ending up in adjacent key vectors.
> With these changes, the time is reduced to 4s for the same scale and
> distribution of routes.
>
> The issue is that update_suffix does an O(n) walk on the children of a
> node and the with a dense distribtion of routes the number of children
> in a node tends towards the number of nodes in the tree.
Other than the performance I was just wondering if you did any other
validation to confirm that nothing else differs. Basically it would
just be a matter of verifying that /proc/net/fib_trie is the same
before and after your patch.
> In the add case it isn't necessary to walk all the other children to
> update the largest suffix length of the node (which is what
> update_suffix is doing) since we already know what the largest suffix
> length of all the other children is and we only need to update it if
> the new node/leaf has a larger suffix. However, it is currently called
> from resize which is called on any update to rebalance the trie.
>
> Therefore improve the scaling by moving the responsibility of
> recalculating the node's largest suffix length out of the resize
> function into its callers. For fib_insert_node this can be taken care
> of by extending put_child to not only update the largest suffix length
> in the parent node, but to propagate it all the way up the trie as
> required, using a new function, node_push_suffix, based on
> leaf_push_suffix, but renamed to reflect its purpose and made safe if
> the node has no parent.
>
> No changes are required to inflate/halve since the maximum suffix
> length of the sub-trie starting from the node's parent isn't changed,
> and the suffix lengths of the halved/inflated nodes are updated
> through the creation of the new nodes and put_child called to add the
> children to them.
>
> In both fib_table_flush and fib_table_flush_external, given that a
> walk of the entire trie is being done there's a choice of optimising
> either for the case of a small number of leafs being flushed by
> updating the suffix on a change and propagating it up, or optimising
> for large numbers of leafs being flushed by deferring the updating of
> the largest suffix length to the walk up to the parent node. I opted
> for the latter to keep the algorithm linear in complexity in the case
> of flushing all leafs and because it is close to the status quo.
>
> Fixes: 5405afd1a306 ("fib_trie: Add tracking value for suffix length")
> Signed-off-by: Robert Shearman <rshearma@brocade.com>
So I am not entirely convinced this is a regression, I was wondering
what your numbers looked like before the patch you are saying this
fixes? I recall I fixed a number of issues leading up to this so I am
just curious.
My thought is this seems more like a performance optimization than a
fix. If that is the case this probably belongs in net-next.
It seems to me we might be able to simplify update_suffix rather than
going through all this change. That might be something that is more
acceptable for net. Have you looked at simply adding code so that
there is a break inside update_suffix if (slen == tn->slen)? We don't
have to call it for if a leaf is added so it might make sense to add
that check.
> ---
> net/ipv4/fib_trie.c | 86 ++++++++++++++++++++++++++++++++++++++---------------
> 1 file changed, 62 insertions(+), 24 deletions(-)
>
> diff --git a/net/ipv4/fib_trie.c b/net/ipv4/fib_trie.c
> index 026f309c51e9..701cae8af44a 100644
> --- a/net/ipv4/fib_trie.c
> +++ b/net/ipv4/fib_trie.c
> @@ -421,8 +421,22 @@ static inline int tnode_full(struct key_vector *tn, struct key_vector *n)
> return n && ((n->pos + n->bits) == tn->pos) && IS_TNODE(n);
> }
>
> +static void node_push_suffix(struct key_vector *tn, struct key_vector *l)
> +{
> + while (tn->slen < l->slen) {
> + tn->slen = l->slen;
> + tn = node_parent(tn);
> + if (!tn)
> + break;
I don't think the NULL check is necessary, at least it wasn't with the old code.
> + }
> +}
> +
> /* Add a child at position i overwriting the old value.
> * Update the value of full_children and empty_children.
> + *
> + * The suffix length of the parent node and the rest of the tree is
> + * updated (if required) when adding/replacing a node, but is caller's
> + * responsibility on removal.
> */
> static void put_child(struct key_vector *tn, unsigned long i,
> struct key_vector *n)
> @@ -447,8 +461,8 @@ static void put_child(struct key_vector *tn, unsigned long i,
> else if (!wasfull && isfull)
> tn_info(tn)->full_children++;
>
> - if (n && (tn->slen < n->slen))
> - tn->slen = n->slen;
> + if (n)
> + node_push_suffix(tn, n);
This is just creating redundant work if we have to perform a resize.
> rcu_assign_pointer(tn->tnode[i], n);
> }
> @@ -919,34 +933,35 @@ static struct key_vector *resize(struct trie *t, struct key_vector *tn)
> if (max_work != MAX_WORK)
> return tp;
>
> - /* push the suffix length to the parent node */
> - if (tn->slen > tn->pos) {
> - unsigned char slen = update_suffix(tn);
> -
> - if (slen > tp->slen)
> - tp->slen = slen;
> - }
> -
> return tp;
> }
>
So it seems like the goal here is to move update_suffix out of the
resize code. I'm mostly okay with that. I don't recall why I was
doing it as a part of the resize instead of just doing it whenever a
longer suffix was added.
> -static void leaf_pull_suffix(struct key_vector *tp, struct key_vector *l)
> +static void node_set_suffix(struct key_vector *tp, unsigned char slen)
> {
> - while ((tp->slen > tp->pos) && (tp->slen > l->slen)) {
> - if (update_suffix(tp) > l->slen)
> + if (slen > tp->slen)
> + tp->slen = slen;
> +}
> +
> +static void node_pull_suffix(struct key_vector *tn)
> +{
> + struct key_vector *tp;
> + unsigned char slen;
> +
> + slen = update_suffix(tn);
> + tp = node_parent(tn);
> + while ((tp->slen > tp->pos) && (tp->slen > slen)) {
> + if (update_suffix(tp) > slen)
> break;
> tp = node_parent(tp);
> }
> }
>
I really hate all the renaming. Can't you just leave these as
leaf_pull_suffix and leaf_push _suffix. I'm fine with us dropping the
leaf of the suffix but the renaming just makes adds a bunch of noise.
Really it might work better if you broke the replacement of the
key_vector as a leaf with the suffix length into a separate patch,
then you could do the rename as a part of that.
> -static void leaf_push_suffix(struct key_vector *tn, struct key_vector *l)
> +static void leaf_pull_suffix(struct key_vector *tp, struct key_vector *l)
> {
> - /* if this is a new leaf then tn will be NULL and we can sort
> - * out parent suffix lengths as a part of trie_rebalance
> - */
> - while (tn->slen < l->slen) {
> - tn->slen = l->slen;
> - tn = node_parent(tn);
> + while ((tp->slen > tp->pos) && (tp->slen > l->slen)) {
> + if (update_suffix(tp) > l->slen)
> + break;
> + tp = node_parent(tp);
> }
> }
If possible it would work better if you could avoid moving functions
around as a result of your changes.
> @@ -1107,7 +1122,7 @@ static int fib_insert_alias(struct trie *t, struct key_vector *tp,
> /* if we added to the tail node then we need to update slen */
> if (l->slen < new->fa_slen) {
> l->slen = new->fa_slen;
> - leaf_push_suffix(tp, l);
> + node_push_suffix(tp, l);
> }
>
> return 0;
> @@ -1495,12 +1510,15 @@ static void fib_remove_alias(struct trie *t, struct key_vector *tp,
> /* remove the fib_alias from the list */
> hlist_del_rcu(&old->fa_list);
>
> - /* if we emptied the list this leaf will be freed and we can sort
> - * out parent suffix lengths as a part of trie_rebalance
> - */
> + /* if we emptied the list this leaf will be freed */
> if (hlist_empty(&l->leaf)) {
> put_child_root(tp, l->key, NULL);
> node_free(l);
> + /* only need to update suffixes if this alias was
> + * possibly the one with the largest suffix in the parent
> + */
> + if (tp->slen == old->fa_slen)
> + node_pull_suffix(tp);
> trie_rebalance(t, tp);
> return;
> }
> @@ -1783,6 +1801,16 @@ void fib_table_flush_external(struct fib_table *tb)
> if (IS_TRIE(pn))
> break;
>
> + /* push the suffix length to the parent node,
> + * since a previous leaf removal may have
> + * caused it to change
> + */
> + if (pn->slen > pn->pos) {
> + unsigned char slen = update_suffix(pn);
> +
> + node_set_suffix(node_parent(pn), slen);
> + }
> +
Why bother with the local variable? You could just pass
update_suffix(pn) as the parameter to your node_set_suffix function.
> /* resize completed node */
> pn = resize(t, pn);
> cindex = get_index(pkey, pn);
> @@ -1849,6 +1877,16 @@ int fib_table_flush(struct net *net, struct fib_table *tb)
> if (IS_TRIE(pn))
> break;
>
> + /* push the suffix length to the parent node,
> + * since a previous leaf removal may have
> + * caused it to change
> + */
> + if (pn->slen > pn->pos) {
> + unsigned char slen = update_suffix(pn);
> +
> + node_set_suffix(node_parent(pn), slen);
> + }
> +
Same here.
> /* resize completed node */
> pn = resize(t, pn);
> cindex = get_index(pkey, pn);
> --
> 2.1.4
>
^ permalink raw reply
* Re: netlink: GPF in sock_sndtimeo
From: Cong Wang @ 2016-11-29 23:13 UTC (permalink / raw)
To: Richard Guy Briggs
Cc: Dmitry Vyukov, David Miller, Johannes Berg, Florian Westphal,
Eric Dumazet, Herbert Xu, netdev, LKML, syzkaller
In-Reply-To: <20161129164859.GD26673@madcap2.tricolour.ca>
On Tue, Nov 29, 2016 at 8:48 AM, Richard Guy Briggs <rgb@redhat.com> wrote:
> On 2016-11-26 17:11, Cong Wang wrote:
>> It is racy on audit_sock, especially on the netns exit path.
>
> I think that is the only place it is racy. The other places audit_sock
> is set is when the socket failure has just triggered a reset.
>
> Is there a notifier callback for failed or reaped sockets?
Is NETLINK_URELEASE event what you are looking for?
^ permalink raw reply
* [PATCH for-next 5/6] IB/hns: Fix the bug when free cq
From: Salil Mehta @ 2016-11-29 23:10 UTC (permalink / raw)
To: dledford
Cc: salil.mehta, xavier.huwei, oulijun, xushaobo2, mehta.salil.lnk,
lijun_nudt, linux-rdma, netdev, linux-kernel, linuxarm
In-Reply-To: <20161129231030.1105600-1-salil.mehta@huawei.com>
From: Shaobo Xu <xushaobo2@huawei.com>
If the resources of cq are freed while executing the user case, hardware
can not been notified in hip06 SoC. Then hardware will hold on when it
writes the cq buffer which has been released.
In order to slove this problem, RoCE driver checks the CQE counter, and
ensure that the outstanding CQE have been written. Then the cq buffer
can be released.
Signed-off-by: Shaobo Xu <xushaobo2@huawei.com>
Reviewed-by: Wei Hu (Xavier) <xavier.huwei@huawei.com>
Signed-off-by: Salil Mehta <salil.mehta@huawei.com>
---
drivers/infiniband/hw/hns/hns_roce_common.h | 2 +
drivers/infiniband/hw/hns/hns_roce_cq.c | 27 ++++++++------
drivers/infiniband/hw/hns/hns_roce_device.h | 8 ++++
drivers/infiniband/hw/hns/hns_roce_hw_v1.c | 53 +++++++++++++++++++++++++++
4 files changed, 79 insertions(+), 11 deletions(-)
diff --git a/drivers/infiniband/hw/hns/hns_roce_common.h b/drivers/infiniband/hw/hns/hns_roce_common.h
index a055632..4af403e 100644
--- a/drivers/infiniband/hw/hns/hns_roce_common.h
+++ b/drivers/infiniband/hw/hns/hns_roce_common.h
@@ -354,6 +354,8 @@
#define ROCEE_SDB_ISSUE_PTR_REG 0x758
#define ROCEE_SDB_SEND_PTR_REG 0x75C
+#define ROCEE_CAEP_CQE_WCMD_EMPTY 0x850
+#define ROCEE_SCAEP_WR_CQE_CNT 0x8D0
#define ROCEE_SDB_INV_CNT_REG 0x9A4
#define ROCEE_SDB_RETRY_CNT_REG 0x9AC
#define ROCEE_TSP_BP_ST_REG 0x9EC
diff --git a/drivers/infiniband/hw/hns/hns_roce_cq.c b/drivers/infiniband/hw/hns/hns_roce_cq.c
index c9f6c3d..ff9a6a3 100644
--- a/drivers/infiniband/hw/hns/hns_roce_cq.c
+++ b/drivers/infiniband/hw/hns/hns_roce_cq.c
@@ -179,8 +179,7 @@ static int hns_roce_hw2sw_cq(struct hns_roce_dev *dev,
HNS_ROCE_CMD_TIMEOUT_MSECS);
}
-static void hns_roce_free_cq(struct hns_roce_dev *hr_dev,
- struct hns_roce_cq *hr_cq)
+void hns_roce_free_cq(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq)
{
struct hns_roce_cq_table *cq_table = &hr_dev->cq_table;
struct device *dev = &hr_dev->pdev->dev;
@@ -392,19 +391,25 @@ int hns_roce_ib_destroy_cq(struct ib_cq *ib_cq)
{
struct hns_roce_dev *hr_dev = to_hr_dev(ib_cq->device);
struct hns_roce_cq *hr_cq = to_hr_cq(ib_cq);
+ int ret = 0;
- hns_roce_free_cq(hr_dev, hr_cq);
- hns_roce_mtt_cleanup(hr_dev, &hr_cq->hr_buf.hr_mtt);
+ if (hr_dev->hw->destroy_cq) {
+ ret = hr_dev->hw->destroy_cq(ib_cq);
+ } else {
+ hns_roce_free_cq(hr_dev, hr_cq);
+ hns_roce_mtt_cleanup(hr_dev, &hr_cq->hr_buf.hr_mtt);
- if (ib_cq->uobject)
- ib_umem_release(hr_cq->umem);
- else
- /* Free the buff of stored cq */
- hns_roce_ib_free_cq_buf(hr_dev, &hr_cq->hr_buf, ib_cq->cqe);
+ if (ib_cq->uobject)
+ ib_umem_release(hr_cq->umem);
+ else
+ /* Free the buff of stored cq */
+ hns_roce_ib_free_cq_buf(hr_dev, &hr_cq->hr_buf,
+ ib_cq->cqe);
- kfree(hr_cq);
+ kfree(hr_cq);
+ }
- return 0;
+ return ret;
}
void hns_roce_cq_completion(struct hns_roce_dev *hr_dev, u32 cqn)
diff --git a/drivers/infiniband/hw/hns/hns_roce_device.h b/drivers/infiniband/hw/hns/hns_roce_device.h
index 1050829..d4f0fce 100644
--- a/drivers/infiniband/hw/hns/hns_roce_device.h
+++ b/drivers/infiniband/hw/hns/hns_roce_device.h
@@ -56,6 +56,12 @@
#define HNS_ROCE_MAX_INNER_MTPT_NUM 0x7
#define HNS_ROCE_MAX_MTPT_PBL_NUM 0x100000
+#define HNS_ROCE_EACH_FREE_CQ_WAIT_MSECS 20
+#define HNS_ROCE_MAX_FREE_CQ_WAIT_CNT \
+ (5000 / HNS_ROCE_EACH_FREE_CQ_WAIT_MSECS)
+#define HNS_ROCE_CQE_WCMD_EMPTY_BIT 0x2
+#define HNS_ROCE_MIN_CQE_CNT 16
+
#define HNS_ROCE_MAX_IRQ_NUM 34
#define HNS_ROCE_COMP_VEC_NUM 32
@@ -528,6 +534,7 @@ struct hns_roce_hw {
int (*req_notify_cq)(struct ib_cq *ibcq, enum ib_cq_notify_flags flags);
int (*poll_cq)(struct ib_cq *ibcq, int num_entries, struct ib_wc *wc);
int (*dereg_mr)(struct hns_roce_dev *hr_dev, struct hns_roce_mr *mr);
+ int (*destroy_cq)(struct ib_cq *ibcq);
void *priv;
};
@@ -734,6 +741,7 @@ struct ib_cq *hns_roce_ib_create_cq(struct ib_device *ib_dev,
struct ib_udata *udata);
int hns_roce_ib_destroy_cq(struct ib_cq *ib_cq);
+void hns_roce_free_cq(struct hns_roce_dev *hr_dev, struct hns_roce_cq *hr_cq);
void hns_roce_cq_completion(struct hns_roce_dev *hr_dev, u32 cqn);
void hns_roce_cq_event(struct hns_roce_dev *hr_dev, u32 cqn, int event_type);
diff --git a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
index f67a3bf..b8111b0 100644
--- a/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
+++ b/drivers/infiniband/hw/hns/hns_roce_hw_v1.c
@@ -3763,6 +3763,58 @@ int hns_roce_v1_destroy_qp(struct ib_qp *ibqp)
return 0;
}
+int hns_roce_v1_destroy_cq(struct ib_cq *ibcq)
+{
+ struct hns_roce_dev *hr_dev = to_hr_dev(ibcq->device);
+ struct hns_roce_cq *hr_cq = to_hr_cq(ibcq);
+ struct device *dev = &hr_dev->pdev->dev;
+ u32 cqe_cnt_ori;
+ u32 cqe_cnt_cur;
+ u32 cq_buf_size;
+ int wait_time = 0;
+ int ret = 0;
+
+ hns_roce_free_cq(hr_dev, hr_cq);
+
+ /*
+ * Before freeing cq buffer, we need to ensure that the outstanding CQE
+ * have been written by checking the CQE counter.
+ */
+ cqe_cnt_ori = roce_read(hr_dev, ROCEE_SCAEP_WR_CQE_CNT);
+ while (1) {
+ if (roce_read(hr_dev, ROCEE_CAEP_CQE_WCMD_EMPTY) &
+ HNS_ROCE_CQE_WCMD_EMPTY_BIT)
+ break;
+
+ cqe_cnt_cur = roce_read(hr_dev, ROCEE_SCAEP_WR_CQE_CNT);
+ if ((cqe_cnt_cur - cqe_cnt_ori) >= HNS_ROCE_MIN_CQE_CNT)
+ break;
+
+ msleep(HNS_ROCE_EACH_FREE_CQ_WAIT_MSECS);
+ if (wait_time > HNS_ROCE_MAX_FREE_CQ_WAIT_CNT) {
+ dev_warn(dev, "Destroy cq 0x%lx timeout!\n",
+ hr_cq->cqn);
+ ret = -ETIMEDOUT;
+ break;
+ }
+ wait_time++;
+ }
+
+ hns_roce_mtt_cleanup(hr_dev, &hr_cq->hr_buf.hr_mtt);
+
+ if (ibcq->uobject)
+ ib_umem_release(hr_cq->umem);
+ else {
+ /* Free the buff of stored cq */
+ cq_buf_size = (ibcq->cqe + 1) * hr_dev->caps.cq_entry_sz;
+ hns_roce_buf_free(hr_dev, cq_buf_size, &hr_cq->hr_buf.hr_buf);
+ }
+
+ kfree(hr_cq);
+
+ return ret;
+}
+
struct hns_roce_v1_priv hr_v1_priv;
struct hns_roce_hw hns_roce_hw_v1 = {
@@ -3784,5 +3836,6 @@ struct hns_roce_hw hns_roce_hw_v1 = {
.req_notify_cq = hns_roce_v1_req_notify_cq,
.poll_cq = hns_roce_v1_poll_cq,
.dereg_mr = hns_roce_v1_dereg_mr,
+ .destroy_cq = hns_roce_v1_destroy_cq,
.priv = &hr_v1_priv,
};
--
1.7.9.5
^ 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