* Re: Alignment issues with freescale FEC driver
From: Eric Nelson @ 2016-09-23 18:26 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Eric Dumazet, linux-arm-kernel@lists.infradead.org,
netdev@vger.kernel.org, Fugang Duan, Troy Kisky, Otavio Salvador,
Simone
In-Reply-To: <20160923173751.GA1041@n2100.armlinux.org.uk>
Thanks Russell,
On 09/23/2016 10:37 AM, Russell King - ARM Linux wrote:
> On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote:
>> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
>>
>> I believe this is because the compiler is generating an ldm instruction
>> when the ntohl() call is used, but I'm stumped about why these aren't
>> generating faults:
After looking at it, I have to think that the code that reads iph->id
is just hit more frequently than the other code in this routine.
>
> ldm generates alignment faults when the address is not aligned to a
> 32-bit boundary. ldr on ARMv6+ does not.
>
>> I don't think that's the case.
>>
>> # CONFIG_IPV6_GRE is not set
>>
>> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
>> a 4-byte boundary.
>>
>> Does the ldm instruction require 8-byte alignment?
>>
>> There's definitely a compiler-version dependency involved here,
>> since using gcc 4.9 also reduced the number of faults dramatically.
>
> Well, I don't think it's that gcc related:
>
I can only say that I noticed a dramatic drop in the number of faults, and
didn't see the inet_gro_receive reported in /proc/cpu/alignment with gcc 4.9
when trying to identify the issue.
> User: 0
> System: 312855 (ip6_route_input+0x6c/0x1e0)
> Skipped: 0
> Half: 0
> Word: 0
> DWord: 2
> Multi: 312853
>
> c06d8998 <ip6_route_input>:
> c06d89ac: e1a04000 mov r4, r0
> c06d89b0: e1d489b4 ldrh r8, [r4, #148] ; 0x94
> c06d89b8: e594a0a0 ldr sl, [r4, #160] ; 0xa0
> c06d89cc: e08ac008 add ip, sl, r8
> c06d89d4: e28c3018 add r3, ip, #24
> c06d89dc: e28c7008 add r7, ip, #8
> c06d89e4: e893000f ldm r3, {r0, r1, r2, r3}
> c06d89ec: e24be044 sub lr, fp, #68 ; 0x44
> c06d89f4: e24b5054 sub r5, fp, #84 ; 0x54
> c06d89fc: e885000f stm r5, {r0, r1, r2, r3}
> c06d8a04: e897000f ldm r7, {r0, r1, r2, r3}
> c06d8a10: e88e000f stm lr, {r0, r1, r2, r3}
>
> This is from:
>
> struct flowi6 fl6 = {
> .flowi6_iif = l3mdev_fib_oif(skb->dev),
> .daddr = iph->daddr,
> .saddr = iph->saddr,
> .flowlabel = ip6_flowinfo(iph),
> .flowi6_mark = skb->mark,
> .flowi6_proto = iph->nexthdr,
> };
>
> specifically, I suspect, the saddr and daddr initialisations.
>
> There's not much to get away from this - the FEC on iMX requires a
> 16-byte alignment for DMA addresses, which violates the network
> stack's requirement for the ethernet packet to be received with a
> two byte offset. So the IP header (and IPv6 headers) will always
> be mis-aligned in memory, which leads to a huge number of alignment
> faults.
>
> There's not much getting away from this - the problem is not in the
> networking stack, but the FEC hardware/network driver. See:
>
> struct fec_enet_private *fep = netdev_priv(ndev);
> int off;
>
> off = ((unsigned long)skb->data) & fep->rx_align;
> if (off)
> skb_reserve(skb, fep->rx_align + 1 - off);
>
> bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));
>
> in fec_enet_new_rxbdp().
>
So the question is: should we just live with this and acknowledge a
performance penalty of bad alignment or do something about it?
I'm not sure the cost (or the details) of Eric's proposed fix of allocating
and copying the header to another skb.
The original report was of bad network performance, but I haven't
been able to see an impact doing some simple tests using wget
and SSH.
^ permalink raw reply
* [PATCH] Net Driver: Add Cypress GX3 VID=04b4 PID=3610.
From: chris.roth @ 2016-09-23 18:24 UTC (permalink / raw)
To: linux-usb, netdev, linux-kernel; +Cc: Chris Roth
From: Chris Roth <chris.roth@usask.ca>
Add support for Cypress GX3 SuperSpeed to Gigabit Ethernet
Bridge Controller (Vendor=04b4 ProdID=3610).
Patch verified on x64 linux kernel 4.7.4 system with the
Kensington SD4600P USB-C Universal Dock with Power, which uses the
Cypress GX3 SuperSpeed to Gigabit Ethernet Bridge Controller.
A similar patch was signed-off and tested-by Allan Chou
<allan@asix.com.tw> on 2015-12-01.
Allan verified his similar patch on x86 Linux kernel 4.1.6 system
with Cypress GX3 SuperSpeed to Gigabit Ethernet Bridge Controller.
Signed-off-by: Chris Roth <chris.roth@usask.ca>
---
drivers/net/usb/ax88179_178a.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
index e6338c1..8a6675d 100644
--- a/drivers/net/usb/ax88179_178a.c
+++ b/drivers/net/usb/ax88179_178a.c
@@ -1656,6 +1656,19 @@ static const struct driver_info ax88178a_info = {
.tx_fixup = ax88179_tx_fixup,
};
+static const struct driver_info cypress_GX3_info = {
+ .description = "Cypress GX3 SuperSpeed to Gigabit Ethernet Controller",
+ .bind = ax88179_bind,
+ .unbind = ax88179_unbind,
+ .status = ax88179_status,
+ .link_reset = ax88179_link_reset,
+ .reset = ax88179_reset,
+ .stop = ax88179_stop,
+ .flags = FLAG_ETHER | FLAG_FRAMING_AX,
+ .rx_fixup = ax88179_rx_fixup,
+ .tx_fixup = ax88179_tx_fixup,
+};
+
static const struct driver_info dlink_dub1312_info = {
.description = "D-Link DUB-1312 USB 3.0 to Gigabit Ethernet Adapter",
.bind = ax88179_bind,
@@ -1718,6 +1731,10 @@ static const struct usb_device_id products[] = {
USB_DEVICE(0x0b95, 0x178a),
.driver_info = (unsigned long)&ax88178a_info,
}, {
+ /* Cypress GX3 SuperSpeed to Gigabit Ethernet Bridge Controller */
+ USB_DEVICE(0x04b4, 0x3610),
+ .driver_info = (unsigned long)&cypress_GX3_info,
+}, {
/* D-Link DUB-1312 USB 3.0 to Gigabit Ethernet Adapter */
USB_DEVICE(0x2001, 0x4a00),
.driver_info = (unsigned long)&dlink_dub1312_info,
--
2.7.4
^ permalink raw reply related
* Re: [PATCH net-next 08/15] rxrpc: Fix call timer
From: David Howells @ 2016-09-23 18:21 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: dhowells, netdev, linux-afs, linux-kernel
In-Reply-To: <9efafc3e-5f19-598c-ee0c-def748eeb78a@cogentembedded.com>
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:
> > + if (call->timer.expires != t || !timer_pending(&call->timer)) {
> > mod_timer(&call->timer, t);
> > }
>
> CodingStyle: {} not needed now.
See patch 11.
David
^ permalink raw reply
* Re: [PATCH net-next 07/15] rxrpc: Fix accidental cancellation of scheduled resend by ACK parser
From: David Howells @ 2016-09-23 18:20 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: dhowells, netdev, linux-afs, linux-kernel
In-Reply-To: <c868ca22-fed0-67f7-7471-165b3e5a9edd@cogentembedded.com>
Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:
> > case RXRPC_ACK_TYPE_NACK:
> > if (anno_type == RXRPC_TX_ANNO_NAK)
> > continue;
> > + if (anno_type == RXRPC_TX_ANNO_RETRANS)
> > + continue;
>
> Why not fold the above 2 *if*s together? Or use *else if* at least?
I have a pending patch that adds something between them.
David
^ permalink raw reply
* Re: Alignment issues with freescale FEC driver
From: Andrew Lunn @ 2016-09-23 18:13 UTC (permalink / raw)
To: Eric Nelson
Cc: Eric Dumazet, Fugang Duan, Otavio Salvador,
netdev@vger.kernel.org, Troy Kisky, rmk+kernel, Simone,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <0fe7a310-2d2f-4fca-d698-85d66122d91c@nelint.com>
> Since the hardware requires longword alignment for its' DMA transfers,
> aligning the IP header will require a memcpy, right?
The vf610 FEC has an SHIFT16 bit in register ENETx_TACC, which inserts
two padding bits on transmit. ENETx_RACC has the same.
What about your hardware?
Andrew
^ permalink raw reply
* Re: [PATCH RFC 0/2] ila: ilarouter bpf code for tc and xdp
From: Jesper Dangaard Brouer @ 2016-09-23 18:06 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: David S . Miller, Daniel Borkmann, Tom Herbert, Jamal Hadi Salim,
Thomas Graf, netdev, brouer
In-Reply-To: <1474650995-2031928-1-git-send-email-ast@fb.com>
On Fri, 23 Sep 2016 10:16:33 -0700 Alexei Starovoitov <ast@fb.com> wrote:
> From: Aaron Yue <haoxuany@andrew.cmu.edu>
>
> Jesper,
>
> here is old email and cover letter that didn't make it to the list
> due to vger outage (I guess).
> The verifier patch that Aaron is talking about has landed long ago.
>
> The dataplane of ILA router is very short and simple.
Yes, looks very simple indeed! Cool! :-)
> Control plane is very different matter. It's not ready for prime time yet.
>
> ----------
>
> This patch contains the tc and xdp implementation of kernelspace bpf code.
> It requires userspace to insert to the ILA bpf maps, in tc's case, the
> precomputed ILA mappings, and in xdp's case, both the precomputed ILA
> mappings and the MAC address.
>
> The xdp bpf code also requires a verifier patch to allow direct map access
> from the packet (will be patched in by Alexei).
>
> Aaron Yue (2):
> samples/bpf: ilarouter for tc
> samples/bpf: ilarouter for xdp
>
> samples/bpf/Makefile | 2 +
> samples/bpf/ila.h | 80 ++++++++++++++++++++++++++++
> samples/bpf/ilarouter_tc.c | 124 ++++++++++++++++++++++++++++++++++++++++++++
> samples/bpf/ilarouter_xdp.c | 88 +++++++++++++++++++++++++++++++
> samples/bpf/inet_helper.h | 38 ++++++++++++++
> 5 files changed, 332 insertions(+)
> create mode 100644 samples/bpf/ila.h
> create mode 100644 samples/bpf/ilarouter_tc.c
> create mode 100644 samples/bpf/ilarouter_xdp.c
> create mode 100644 samples/bpf/inet_helper.h
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH net-next 08/15] rxrpc: Fix call timer
From: Sergei Shtylyov @ 2016-09-23 18:04 UTC (permalink / raw)
To: David Howells, netdev; +Cc: linux-afs, linux-kernel
In-Reply-To: <147464377267.5090.12222734463447533990.stgit@warthog.procyon.org.uk>
On 09/23/2016 06:16 PM, David Howells wrote:
> Fix the call timer in the following ways:
>
> (1) If call->resend_at or call->ack_at are before or equal to the current
> time, then ignore that timeout.
>
> (2) If call->expire_at is before or equal to the current time, then don't
> set the timer at all (possibly we should queue the call).
>
> (3) Don't skip modifying the timer if timer_pending() is true. This
> indicates that the timer is working, not that it has expired and is
> running/waiting to run its expiry handler.
>
> Also call rxrpc_set_timer() to start the call timer going rather than
> calling add_timer().
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> net/rxrpc/call_event.c | 25 ++++++++++++++-----------
> net/rxrpc/call_object.c | 4 ++--
> 2 files changed, 16 insertions(+), 13 deletions(-)
>
> diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
> index 3a7f90a2659c..8bc5c8e37ab4 100644
> --- a/net/rxrpc/call_event.c
> +++ b/net/rxrpc/call_event.c
> @@ -28,24 +28,27 @@ void rxrpc_set_timer(struct rxrpc_call *call)
> {
> unsigned long t, now = jiffies;
>
> - _enter("{%ld,%ld,%ld:%ld}",
> - call->ack_at - now, call->resend_at - now, call->expire_at - now,
> - call->timer.expires - now);
> -
> read_lock_bh(&call->state_lock);
>
> if (call->state < RXRPC_CALL_COMPLETE) {
> - t = call->ack_at;
> - if (time_before(call->resend_at, t))
> + t = call->expire_at;
> + if (time_before_eq(t, now))
> + goto out;
> +
> + if (time_after(call->resend_at, now) &&
> + time_before(call->resend_at, t))
> t = call->resend_at;
> - if (time_before(call->expire_at, t))
> - t = call->expire_at;
> - if (!timer_pending(&call->timer) ||
> - time_before(t, call->timer.expires)) {
> - _debug("set timer %ld", t - now);
> +
> + if (time_after(call->ack_at, now) &&
> + time_before(call->ack_at, t))
> + t = call->ack_at;
> +
> + if (call->timer.expires != t || !timer_pending(&call->timer)) {
> mod_timer(&call->timer, t);
> }
CodingStyle: {} not needed now.
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH net-next 07/15] rxrpc: Fix accidental cancellation of scheduled resend by ACK parser
From: Sergei Shtylyov @ 2016-09-23 18:02 UTC (permalink / raw)
To: David Howells, netdev; +Cc: linux-afs, linux-kernel
In-Reply-To: <147464376586.5090.14901936645927156006.stgit@warthog.procyon.org.uk>
Hello.
On 09/23/2016 06:16 PM, David Howells wrote:
> When rxrpc_input_soft_acks() is parsing the soft-ACKs from an ACK packet,
> it updates the Tx packet annotations in the annotation buffer. If a
> soft-ACK is an ACK, then we overwrite unack'd, nak'd or to-be-retransmitted
> states and that is fine; but if the soft-ACK is an NACK, we overwrite the
> to-be-retransmitted with a nak - which isn't.
>
> Instead, we need to let any scheduled retransmission stand if the packet
> was NAK'd.
>
> Note that we don't reissue a resend if the annotation is in the
> to-be-retransmitted state because someone else must've scheduled the
> resend already.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
> net/rxrpc/input.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/net/rxrpc/input.c b/net/rxrpc/input.c
> index 06027b6d9c19..d3d69ab1f0a1 100644
> --- a/net/rxrpc/input.c
> +++ b/net/rxrpc/input.c
> @@ -479,6 +479,8 @@ static void rxrpc_input_soft_acks(struct rxrpc_call *call, u8 *acks,
> case RXRPC_ACK_TYPE_NACK:
> if (anno_type == RXRPC_TX_ANNO_NAK)
> continue;
> + if (anno_type == RXRPC_TX_ANNO_RETRANS)
> + continue;
Why not fold the above 2 *if*s together? Or use *else if* at least?
MBR, Sergei
^ permalink raw reply
* Re: device-tree support for writing to phy registers?
From: Florian Fainelli @ 2016-09-23 17:40 UTC (permalink / raw)
To: Tim Harvey; +Cc: netdev
In-Reply-To: <CAJ+vNU3_48OVVpT33Jj1cFGqpUYJXY7T=OwR1Se6bYyY=zrH-w@mail.gmail.com>
On 09/23/2016 10:36 AM, Tim Harvey wrote:
> On Fri, Sep 23, 2016 at 9:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> On 09/23/2016 08:40 AM, Tim Harvey wrote:
>>> Greetings,
>>>
>>> I've got a TI DP83867 GbE phy that requires some register writes to
>>> configure its refclock output. Is there a generic device-tree API for
>>> writing to raw registers or is that something that would be need to be
>>> added to a specific phy driver with a device-tree binding?
>>
>> There are no standard properties that indicate how to write to register
>> from Device Tree (unfortunately there are non standard that allow this
>> to happen, e.g: marvell,reg-init), because that would mean that Device
>> Tree acts as some kind of firmware/binary interface, which is a bit of
>> stretch. Some bindings may indicate how to write to registers in a way
>> that accepts a address = value pair, but quite frankly, this is
>> absolutely horrible and not controllable nor easily transferable from
>> one model of device to the other, strongly discouraged.
>>
>>> There is a
>>> DP83867 phy driver but it doesn't contain anything related to
>>> configuring its CLKOUT via register 0x170.
>>
>> Then, I guess you should add a set of properties and corresponding code
>> reading these properties that would result in getting the register
>> programmed with the values you need.
>>
>
> Florian,
>
> agreed - this seems like the right thing to do and takes care of the
> important detail about power-management you mention below.
>
> Are there any phy drivers you know of that do and CLKOUT configuration
> that I could use as inspiration for dt prop names?
The micrel binding has some clock related configuration:
Documentation/devicetree/bindings/net/micrel.txt
could be used as an inspirational source ;)
>
> Thanks,
>
> Tim
>
>>>
>>> Alternatively, is it generally considered 'ok' to take care of this in
>>> the bootloader and not provide the MAC driver the gpio for phy-reset
>>> so that bootloader configuration persists through the kernel?
>>
>> It depends on what your platform does, punting on the bootloader is
>> usually fine, but also breaks nicely when you start implementing power
>> management in the kernel properly (e.g: deep sleep states) and you are
>> not calling back into the bootloader, yet your hardware lost its state
>> between power transitions.
>>
>> --
>> Florian
--
Florian
^ permalink raw reply
* Re: Alignment issues with freescale FEC driver
From: Russell King - ARM Linux @ 2016-09-23 17:37 UTC (permalink / raw)
To: Eric Nelson
Cc: Eric Dumazet, linux-arm-kernel@lists.infradead.org,
netdev@vger.kernel.org, Fugang Duan, Troy Kisky, Otavio Salvador,
Simone
In-Reply-To: <5ee28ee0-cf0c-bdab-1271-f17755365c13@nelint.com>
On Fri, Sep 23, 2016 at 10:19:50AM -0700, Eric Nelson wrote:
> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
>
> I believe this is because the compiler is generating an ldm instruction
> when the ntohl() call is used, but I'm stumped about why these aren't
> generating faults:
ldm generates alignment faults when the address is not aligned to a
32-bit boundary. ldr on ARMv6+ does not.
> I don't think that's the case.
>
> # CONFIG_IPV6_GRE is not set
>
> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
> a 4-byte boundary.
>
> Does the ldm instruction require 8-byte alignment?
>
> There's definitely a compiler-version dependency involved here,
> since using gcc 4.9 also reduced the number of faults dramatically.
Well, I don't think it's that gcc related:
User: 0
System: 312855 (ip6_route_input+0x6c/0x1e0)
Skipped: 0
Half: 0
Word: 0
DWord: 2
Multi: 312853
c06d8998 <ip6_route_input>:
c06d89ac: e1a04000 mov r4, r0
c06d89b0: e1d489b4 ldrh r8, [r4, #148] ; 0x94
c06d89b8: e594a0a0 ldr sl, [r4, #160] ; 0xa0
c06d89cc: e08ac008 add ip, sl, r8
c06d89d4: e28c3018 add r3, ip, #24
c06d89dc: e28c7008 add r7, ip, #8
c06d89e4: e893000f ldm r3, {r0, r1, r2, r3}
c06d89ec: e24be044 sub lr, fp, #68 ; 0x44
c06d89f4: e24b5054 sub r5, fp, #84 ; 0x54
c06d89fc: e885000f stm r5, {r0, r1, r2, r3}
c06d8a04: e897000f ldm r7, {r0, r1, r2, r3}
c06d8a10: e88e000f stm lr, {r0, r1, r2, r3}
This is from:
struct flowi6 fl6 = {
.flowi6_iif = l3mdev_fib_oif(skb->dev),
.daddr = iph->daddr,
.saddr = iph->saddr,
.flowlabel = ip6_flowinfo(iph),
.flowi6_mark = skb->mark,
.flowi6_proto = iph->nexthdr,
};
specifically, I suspect, the saddr and daddr initialisations.
There's not much to get away from this - the FEC on iMX requires a
16-byte alignment for DMA addresses, which violates the network
stack's requirement for the ethernet packet to be received with a
two byte offset. So the IP header (and IPv6 headers) will always
be mis-aligned in memory, which leads to a huge number of alignment
faults.
There's not much getting away from this - the problem is not in the
networking stack, but the FEC hardware/network driver. See:
struct fec_enet_private *fep = netdev_priv(ndev);
int off;
off = ((unsigned long)skb->data) & fep->rx_align;
if (off)
skb_reserve(skb, fep->rx_align + 1 - off);
bdp->cbd_bufaddr = cpu_to_fec32(dma_map_single(&fep->pdev->dev, skb->data, FEC_ENET_RX_FRSIZE - fep->rx_align, DMA_FROM_DEVICE));
in fec_enet_new_rxbdp().
--
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.
^ permalink raw reply
* Re: [PATCH net-next] net/vxlan: Avoid unaligned access in vxlan_build_skb()
From: Alexander Duyck @ 2016-09-23 17:38 UTC (permalink / raw)
To: Sowmini Varadhan
Cc: David Miller, Jiri Benc, Netdev, Hannes Frederic Sowa,
Alexander Duyck, Daniel Borkmann, Paolo Abeni
In-Reply-To: <20160923172059.GB2484@oracle.com>
On Fri, Sep 23, 2016 at 10:20 AM, Sowmini Varadhan
<sowmini.varadhan@oracle.com> wrote:
> On (09/23/16 07:17), Alexander Duyck wrote:
>> >> Is this basically about, e.g., putting the vxlanhdr in its own
>> >> skb_frag_t, or something else?
>> >
>> > Yes, and this way skb_header_pointer() is forced to do a memcpy.
> :
>> For Tx it all becomes a bit trickier since it would likely require us
>> to shift the frags up by 1 when we go from outer headers to inner
>> headers.
>
> here's how I thought through this so far, based on what I'm seeing for
> mld_newpack/vxlan (not sure if this can be extended for all the
> other tunnelling cases as well)..
>
> today the skb is set up so that we reserve LL_RESERVED_SPACE
> in the headroom, and vxlan sets up needed headroom for
> (outer_ether + ip + udp + vxlan + inner_ether). Insterad, if it
> set up the needed_headroom for just (outer_ether, ip, udp) and
> we had something like a "needed_fragroom" in the net_device,
> maybe we could set up the skb so that we dont have to shift the frags
> by 1.
>
> Drawbacks: this ends up with every skb going through vxlan etc being
> non-linear, so it is a lot of churn for several functions (e.g.,
> even mld_newpack() cannot just skb_put() things around). Also
> this probably gets quickly messy if we are dealing with multiple
> encaapsulations (even in the simple vxlan case we have
> vxlan + inner mac/ip/etc)
>
> BTW, I wonder if there is a small vxlan bug here- are we
> accounting for the outer_ether twice in LL_RESERVED_SPACE: once in
> ->hard_header_len, and once in ->needed_headroom?
>
>> One thought I had on that is that we could possibly avoid
>> having to do any allocation and could just take a reference on the
>> head_frag if that is what we are using. Then we just add a 2 byte pad
>> and resume writing headers in place and the pointer offsets for the
>> inner headers would remain valid, though they would be past the point
>> of skb->tail.
>
> I am not sure I follow, can you elaborate? Doesnt this also assume
> that every skb is necessarily non-linear?
So basically what I was thinking is we do something like reserving
NET_IP_ALIGN and continue writing headers to skb->data, but we force
the tracking for the inner headers into frag[0] so that we can keep
the inner headers aligned without messing up the alignment for outer
headers. In theory the inner offset and all that would still be
functional but might need a few tweaks. You could probably even use
the skb->encapsulation bit to indicate you are doing this. You could
almost think of it as us doing something like the inverse of
pskb_pull_tail. The general idea here is we want to actually leave
the data in skb->data, but just reference it from frag[0] so that we
don't accidentally pull in the 2 byte padding for alignment when
transmitting the frame.
^ permalink raw reply
* Re: device-tree support for writing to phy registers?
From: Tim Harvey @ 2016-09-23 17:36 UTC (permalink / raw)
To: Florian Fainelli; +Cc: netdev
In-Reply-To: <6acbbdca-d492-2efb-a0da-bd23d38085f8@gmail.com>
On Fri, Sep 23, 2016 at 9:29 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 09/23/2016 08:40 AM, Tim Harvey wrote:
>> Greetings,
>>
>> I've got a TI DP83867 GbE phy that requires some register writes to
>> configure its refclock output. Is there a generic device-tree API for
>> writing to raw registers or is that something that would be need to be
>> added to a specific phy driver with a device-tree binding?
>
> There are no standard properties that indicate how to write to register
> from Device Tree (unfortunately there are non standard that allow this
> to happen, e.g: marvell,reg-init), because that would mean that Device
> Tree acts as some kind of firmware/binary interface, which is a bit of
> stretch. Some bindings may indicate how to write to registers in a way
> that accepts a address = value pair, but quite frankly, this is
> absolutely horrible and not controllable nor easily transferable from
> one model of device to the other, strongly discouraged.
>
>> There is a
>> DP83867 phy driver but it doesn't contain anything related to
>> configuring its CLKOUT via register 0x170.
>
> Then, I guess you should add a set of properties and corresponding code
> reading these properties that would result in getting the register
> programmed with the values you need.
>
Florian,
agreed - this seems like the right thing to do and takes care of the
important detail about power-management you mention below.
Are there any phy drivers you know of that do and CLKOUT configuration
that I could use as inspiration for dt prop names?
Thanks,
Tim
>>
>> Alternatively, is it generally considered 'ok' to take care of this in
>> the bootloader and not provide the MAC driver the gpio for phy-reset
>> so that bootloader configuration persists through the kernel?
>
> It depends on what your platform does, punting on the bootloader is
> usually fine, but also breaks nicely when you start implementing power
> management in the kernel properly (e.g: deep sleep states) and you are
> not calling back into the bootloader, yet your hardware lost its state
> between power transitions.
>
> --
> Florian
^ permalink raw reply
* Re: Alignment issues with freescale FEC driver
From: Eric Nelson @ 2016-09-23 17:33 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
rmk+kernel, Fugang Duan, Troy Kisky, Otavio Salvador, Simone
In-Reply-To: <5ee28ee0-cf0c-bdab-1271-f17755365c13@nelint.com>
Hi Eric,
On 09/23/2016 10:19 AM, Eric Nelson wrote:
> Thanks Eric,
>
> On 09/23/2016 09:54 AM, Eric Dumazet wrote:
>> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>>>
>>> Hello all,
>>>
>>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>>>
>>>
>
> <snip>
>
>>>
>>> - id = ntohl(*(__be32 *)&iph->id);
>>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
>>> - id >>= 16;
>>> + id = ntohs(*(__be16 *)&iph->id);
>>> + frag = ntohs(*(__be16 *)&iph->frag_off);
>>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
>>> ~IP_DF));
>>>
>>> for (p = *head; p; p = p->next) {
>>> struct iphdr *iph2;
>>>
>>
>> This solves nothing, because a few lines after you'll have yet another
>> unaligned access :
>>
>
> Oddly, it does prevent the vast majority (90%+) of the alignment errors.
>
> I believe this is because the compiler is generating an ldm instruction
> when the ntohl() call is used, but I'm stumped about why these aren't
> generating faults:
>
>> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
>> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
>>
>> So you might have one less problematic access, out of hundreds of them
>> all over the places.
>>
>> Really the problem is that whole stack depends on the assumption that
>> IP headers are aligned on arches that care
>> (ie where NET_IP_ALIGN == 2)
>>
>> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
>> might be because of a buggy driver.
>>
>
> NET_IP_ALIGN is set to 2.
>
>> The other known case is some GRE encapsulations that break the
>> assumption, and this is discussed somewhere else.
>>
> I don't think that's the case.
>
> # CONFIG_IPV6_GRE is not set
>
> Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
> a 4-byte boundary.
>
No. That was wrong.
The iphdr is aligned at offsets of 14 from the ethernet frame, which itself
is longword aligned.
I mistakenly tested before the call to skb_gro_header_slow(), when
iph was NULL.
After putting a test in the right place, I'm seeing an address of
888a364e for the first un-aligned packet.
Since the hardware requires longword alignment for its' DMA transfers,
aligning the IP header will require a memcpy, right?
You hinted at a solution in this post:
http://www.spinics.net/lists/netdev/msg213166.html
Are you aware of another driver doing this that could be used as
a reference?
Please advise,
Eric
^ permalink raw reply
* Re: [RFC] net: store port/representative id in metadata_dst
From: Samudrala, Sridhar @ 2016-09-23 17:22 UTC (permalink / raw)
To: Jakub Kicinski, John Fastabend
Cc: Jiri Benc, Jiri Pirko, netdev, Thomas Graf, Roopa Prabhu,
ogerlitz, ast, daniel, simon.horman, Paolo Abeni, Pravin B Shelar,
hannes, kubakici
In-Reply-To: <20160923162907.7fd5314e@jkicinski-Precision-T1700>
On 9/23/2016 8:29 AM, Jakub Kicinski wrote:
> On Fri, 23 Sep 2016 07:23:26 -0700, John Fastabend wrote:
>> On 16-09-23 05:55 AM, Jakub Kicinski wrote:
>>> On Fri, 23 Sep 2016 11:06:09 +0200, Jiri Benc wrote:
>>>> On Fri, 23 Sep 2016 08:34:29 +0200, Jiri Pirko wrote:
>>>>> So if I understand that correctly, this would need some "shared netdev"
>>>>> which would effectively serve only as a sink for all port netdevices to
>>>>> tx packets to. On RX, this would be completely avoided. This lower
>>>>> device looks like half zombie to me.
>>>> Looks more like a quarter zombie. Even tx would not be allowed unless
>>>> going through one of the ports, as all skbs without
>>>> METADATA_HW_PORT_MUX metadata_dst would be dropped. But it would be
>>>> possible to attach qdisc to the "lower" netdevice and it would actually
>>>> have an effect. On rx this netdevice would be ignored completely. This
>>>> is very weird behavior.
>>>>
>>>>> I don't like it :( I wonder if the
>>>>> solution would not be possible without this lower netdev.
>>>> I agree. This approach doesn't sound correct. The skbs should not be
>>>> requeued.
>>> Thanks for the responses!
>> Nice timing we were just thinking about this.
>>
>>> I think SR-IOV NICs are coming at this problem from a different angle,
>>> we already have a big, feature-full per-port netdevs and now we want to
>>> spawn representators for VFs to handle fallback traffic. This patch
>>> would help us mux VFR traffic on all the queues of the physical port
>>> netdevs (the ones which were already present in legacy mode, that's the
>>> lower device).
>> Yep, I like the idea in general. I had a slightly different approach in
>> mind though. If you look at __dev_queue_xmit() there is a void
>> accel_priv pointer (gather you found this based on your commit note).
>> My take was we could extend this a bit so it can be used by the VFR
>> devices and they could do a dev_queue_xmit_accel(). In this way there is
>> no need to touch /net/core/{filter, dst, ip_tunnel}.c etc. Maybe the
>> accel logic needs to be extended to push the priv pointer all the way
>> through the xmit routine of the target netdev though. This should look
>> a lot like the macvlan accelerated xmit device path without the
>> switching logic.
>>
>> Of course maybe the name would be extended to dev_queue_xmit_extended()
>> or something.
>>
>> So the flow on ingress would be,
>>
>> 1. pkt_received_by_PF_netdev
>> 2. PF_netdev reads some tag off packet/descriptor and sets correct
>> skb->dev field. This is needed so stack "sees" packets from
>> correct VF ports.
>> 3. packet passed up to stack.
>>
>> I guess it is a bit "zombie" like on the receive path because the packet
>> is never actually handled by VF netdev code per se and on egress can
>> traverse both the VFR and PF netdevs qdiscs. But on the other hand the
>> VFR netdevs and PF netdevs are all in the same driver. Plus using a
>> queue per VFR is a bit of a waste as its not needed and also hardware
>> may not have any mechanism to push VF traffic onto a rx queue.
>>
>> On egress,
>>
>> 1. VFR xmit is called
>> 2. VFR xmit calls dev_queue_xmit_accel() with some meta-data if needed
>> for the lower netdev
>> 3. lower netdev sends out the packet.
>>
>> Again we don't need to waste any queues for each VFR and the VFR can be
>> a LLTX device. In this scheme I think you avoid much of the changes in
>> your patch and keep it all contained in the driver. Any thoughts?
The 'accel' parameter in dev_queue_xmit_accel() is currently only passed
to ndo_select_queue() via netdev_pick_tx() and is used to select the tx
queue.
Also, it is not passed all the way to the driver specific xmit routine.
Doesn't it require
changing all the driver xmit routines if we want to pass this parameter?
> Goes without saying that you have a much better understanding of packet
> scheduling so please bear with me :) My target model is that I have
> n_cpus x "n_tc/prio" queues on the PF and I want to transmit the
> fallback traffic over those same queues. So no new HW queues are used
> for VFRs at all. This is a reverse of macvlan offload which AFAICT has
> "bastard hw queues" which actually TX for a separate software device.
>
> My understanding was that I can rework this model to have software
> queues for VFRs (#sw queues == #PF queues + #VFRs) but no extra HW
> queues (#hw queues == #PF queues) but then when the driver sees a
> packet on sw-only VFR queue it has to pick one of the PF queues (which
> one?), lock PF software queue to own it, and only then can it
> transmit. With the dst_metadata there is no need for extra locking or
> queue selection.
Yes. The VFPR netdevs don't have any HW queues associated with them and
we would like
to use the PF queues for the xmit.
I was also looking into some way of passing the port id via skb
parameter to the
dev_queue_xmit() call so that the PF xmit routine can do a directed
transmit to a specifc VF.
Is skb->cb an option to pass this info?
dst_metadata approach would work too if it is acceptable.
>
>> To address 'I wonder if the solution can be done without this lower
>> netdev' I think it can be but it creates two issues which I'm not sure
>> have a good solution.
>>
>> Without a lowerdev we either (a) give each VFR its own queue which I
>> don't like because it complicates mgmt and uses resources or (b) we
>> implicitly share queues. The later could be fine it just looks a bit
>> cleaner IMO to make it explicit.
>>
>> With regard to VF-PF flow rules if we allow matching on ingress port
>> then can all your flow rules be pushed through the PF netdevices or
>> if you want any of the VFR netdevs? After all I expsect the flow rule
>> table is actually a shared resource between all attached ports.
> With the VF-PF forwarding rules I was just inching towards re-opening
> the discussion on whether there should be an CPU port netdev. I guess
> there are good reasons why there isn't so maybe let's not go there :)
> The meaning of PF netdevs in SR-IOV switchdev mode is "external ports"
> AFAICT which could make it cumbersome to reach the host.
^ permalink raw reply
* Re: [PATCH net-next] net/vxlan: Avoid unaligned access in vxlan_build_skb()
From: Sowmini Varadhan @ 2016-09-23 17:20 UTC (permalink / raw)
To: Alexander Duyck
Cc: David Miller, Jiri Benc, Netdev, Hannes Frederic Sowa,
Alexander Duyck, Daniel Borkmann, Paolo Abeni
In-Reply-To: <CAKgT0UcoDb5T9xwfAq83cNwU=XAKZxf0TmPiJZQyHFPr82zggg@mail.gmail.com>
On (09/23/16 07:17), Alexander Duyck wrote:
> >> Is this basically about, e.g., putting the vxlanhdr in its own
> >> skb_frag_t, or something else?
> >
> > Yes, and this way skb_header_pointer() is forced to do a memcpy.
:
> For Tx it all becomes a bit trickier since it would likely require us
> to shift the frags up by 1 when we go from outer headers to inner
> headers.
here's how I thought through this so far, based on what I'm seeing for
mld_newpack/vxlan (not sure if this can be extended for all the
other tunnelling cases as well)..
today the skb is set up so that we reserve LL_RESERVED_SPACE
in the headroom, and vxlan sets up needed headroom for
(outer_ether + ip + udp + vxlan + inner_ether). Insterad, if it
set up the needed_headroom for just (outer_ether, ip, udp) and
we had something like a "needed_fragroom" in the net_device,
maybe we could set up the skb so that we dont have to shift the frags
by 1.
Drawbacks: this ends up with every skb going through vxlan etc being
non-linear, so it is a lot of churn for several functions (e.g.,
even mld_newpack() cannot just skb_put() things around). Also
this probably gets quickly messy if we are dealing with multiple
encaapsulations (even in the simple vxlan case we have
vxlan + inner mac/ip/etc)
BTW, I wonder if there is a small vxlan bug here- are we
accounting for the outer_ether twice in LL_RESERVED_SPACE: once in
->hard_header_len, and once in ->needed_headroom?
> One thought I had on that is that we could possibly avoid
> having to do any allocation and could just take a reference on the
> head_frag if that is what we are using. Then we just add a 2 byte pad
> and resume writing headers in place and the pointer offsets for the
> inner headers would remain valid, though they would be past the point
> of skb->tail.
I am not sure I follow, can you elaborate? Doesnt this also assume
that every skb is necessarily non-linear?
--Sowmini
^ permalink raw reply
* Re: Alignment issues with freescale FEC driver
From: Eric Nelson @ 2016-09-23 17:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
rmk+kernel, Fugang Duan, Troy Kisky, Otavio Salvador, Simone
In-Reply-To: <CANn89iLb+QJu+MYh859oAAjcLCsKRpBbzVf43yOdq+HJVtZDpQ@mail.gmail.com>
Thanks Eric,
On 09/23/2016 09:54 AM, Eric Dumazet wrote:
> On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>>
>> Hello all,
>>
>> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>>
>>
<snip>
>>
>> - id = ntohl(*(__be32 *)&iph->id);
>> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
>> - id >>= 16;
>> + id = ntohs(*(__be16 *)&iph->id);
>> + frag = ntohs(*(__be16 *)&iph->frag_off);
>> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
>> ~IP_DF));
>>
>> for (p = *head; p; p = p->next) {
>> struct iphdr *iph2;
>>
>
> This solves nothing, because a few lines after you'll have yet another
> unaligned access :
>
Oddly, it does prevent the vast majority (90%+) of the alignment errors.
I believe this is because the compiler is generating an ldm instruction
when the ntohl() call is used, but I'm stumped about why these aren't
generating faults:
> ((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
> ((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
>
> So you might have one less problematic access, out of hundreds of them
> all over the places.
>
> Really the problem is that whole stack depends on the assumption that
> IP headers are aligned on arches that care
> (ie where NET_IP_ALIGN == 2)
>
> If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
> might be because of a buggy driver.
>
NET_IP_ALIGN is set to 2.
> The other known case is some GRE encapsulations that break the
> assumption, and this is discussed somewhere else.
>
I don't think that's the case.
# CONFIG_IPV6_GRE is not set
Hmm... Instrumenting the kernel, it seems that iphdr **is** aligned on
a 4-byte boundary.
Does the ldm instruction require 8-byte alignment?
There's definitely a compiler-version dependency involved here,
since using gcc 4.9 also reduced the number of faults dramatically.
^ permalink raw reply
* [PATCH RFC 1/2] samples/bpf: ilarouter for tc
From: Alexei Starovoitov @ 2016-09-23 17:16 UTC (permalink / raw)
To: David S . Miller
Cc: Daniel Borkmann, Jesper Dangaard Brouer, Tom Herbert,
Jamal Hadi Salim, Thomas Graf, netdev
In-Reply-To: <1474650995-2031928-1-git-send-email-ast@fb.com>
From: Aaron Yue <haoxuany@andrew.cmu.edu>
From: Aaron Yue <haoxuany@fb.com>
Requires a userspace program to insert ila mappings to the ila map.
Signed-off-by: Aaron Yue <haoxuany@fb.com>
Signed-off-by: Aaron Yue <haoxuany@andrew.cmu.edu>
---
samples/bpf/Makefile | 1 +
samples/bpf/ila.h | 80 +++++++++++++++++++++++++++++
samples/bpf/ilarouter_tc.c | 124 +++++++++++++++++++++++++++++++++++++++++++++
samples/bpf/inet_helper.h | 38 ++++++++++++++
4 files changed, 243 insertions(+)
create mode 100644 samples/bpf/ila.h
create mode 100644 samples/bpf/ilarouter_tc.c
create mode 100644 samples/bpf/inet_helper.h
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 90ebf7d..15e19bb 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -74,6 +74,7 @@ always += parse_varlen.o parse_simple.o parse_ldabs.o
always += test_cgrp2_tc_kern.o
always += xdp1_kern.o
always += xdp2_kern.o
+always += ilarouter_tc.o
HOSTCFLAGS += -I$(objtree)/usr/include
diff --git a/samples/bpf/ila.h b/samples/bpf/ila.h
new file mode 100644
index 0000000..39a11f8
--- /dev/null
+++ b/samples/bpf/ila.h
@@ -0,0 +1,80 @@
+#ifndef _SIR_H
+#define _SIR_H
+
+#include <linux/types.h>
+#include <linux/in6.h>
+#include <asm/byteorder.h>
+
+#define SIR_T_LOCAL 0x1
+#define SIR_T_VIRTUAL 0x3
+
+struct in6_addr_sir {
+ __be64 prefix;
+ __be64 identifier_c_type;
+} __packed;
+
+struct in6_addr_ila {
+ __be64 locator;
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ __u8 identifier:4,
+ c:1,
+ type:3;
+ __u8 identifier2;
+ __be16 identifier3;
+ __be16 identifier4;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ __be32 type:3,
+ c:1,
+ identifier:28;
+ __be16 identifier2;
+#else
+#error "Fix asm/byteorder.h"
+#endif
+ __be16 checksum;
+} __packed;
+
+struct sirhdr {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ __u16 traffic_class:4,
+ version:4,
+ flow_label:4,
+ traffic_class2:4;
+ __be16 flow_label2;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ __u32 version:4,
+ traffic_class:8,
+ flow_label:20;
+#else
+#error "Fix asm/byteorder.h"
+#endif
+ __be16 payload_length;
+ __u8 next_header;
+ __u8 hop_limit;
+
+ struct in6_addr source_address;
+ struct in6_addr_sir destination_address;
+} __packed;
+
+struct ilahdr {
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+ __u16 traffic_class:4,
+ version:4,
+ flow_label:4,
+ traffic_class2:4;
+ __be16 flow_label2;
+#elif defined(__BIG_ENDIAN_BITFIELD)
+ __u32 version:4,
+ traffic_class:8,
+ flow_label:20;
+#else
+#error "Fix asm/byteorder.h"
+#endif
+ __be16 payload_length;
+ __u8 next_header;
+ __u8 hop_limit;
+
+ struct in6_addr source_address;
+ struct in6_addr_ila destination_address;
+} __packed;
+
+#endif
diff --git a/samples/bpf/ilarouter_tc.c b/samples/bpf/ilarouter_tc.c
new file mode 100644
index 0000000..277322e
--- /dev/null
+++ b/samples/bpf/ilarouter_tc.c
@@ -0,0 +1,124 @@
+/* Copyright (c) 2016 Facebook
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+
+#define MAP_SIZE (1 << 20)
+
+#define KBUILD_MODNAME "ilarouter"
+#include <linux/if_ether.h>
+#include <linux/ipv6.h>
+#include <uapi/linux/bpf.h>
+#include "ila.h"
+#include "inet_helper.h"
+#include "bpf_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+unsigned int version SEC("version") = 1;
+
+struct bpf_map_def SEC("maps") ila_lookup_map = {
+ .type = BPF_MAP_TYPE_HASH,
+ .key_size = sizeof(struct in6_addr),
+ .value_size = sizeof(struct in6_addr),
+ .max_entries = MAP_SIZE,
+};
+
+#define IPV6_DEST_OFF (ETH_HLEN + offsetof(struct ipv6hdr, daddr))
+
+struct addr {
+ __u64 addr_hi;
+ __u64 addr_lo;
+} __packed;
+
+SEC("classifier")
+int ila_lookup(struct __sk_buff *skb)
+{
+ unsigned long dataptr = (unsigned long)skb->data;
+ struct ethhdr *eth;
+ struct ipv6hdr *sir;
+ struct addr *pkt_addr;
+ struct addr stack_addr;
+ struct addr *reply;
+#ifdef DEBUG
+ char lookup_request[] = "Lookup request for sir: %llx, iden: %llx\n";
+ char lookup_fail[] = "Lookup failed\n";
+ char lookup_success[] = "Lookup success. hi: %llx, lo: %llx\n";
+#endif
+
+ /* Invalid packet: length too short
+ * compiler optimization/verifier bypass: this way it won't assume
+ * that we copied over a pkt_ptr,
+ * which has register range of 0 (from (r1 + 0))
+ */
+ if (dataptr + sizeof(struct ethhdr) +
+ sizeof(struct ipv6hdr) > skb->data_end)
+ goto redirect;
+
+ /* Ethernet header */
+ eth = (struct ethhdr *)dataptr;
+
+ /* Irrelevant packet: not IPv6 */
+ if (eth->h_proto != htons(ETH_P_IPV6))
+ goto redirect;
+
+ /* SIR Address header */
+ sir = (struct ipv6hdr *)(dataptr + sizeof(struct ethhdr));
+#ifdef DEBUG
+ {
+ /* ILA Address header */
+ struct ilahdr *ila = (struct ilahdr *)sir;
+
+ /* For debugging purposes,
+ * we don't care about non-SIR/ILA addresses
+ */
+ if (ila->destination_address.c)
+ goto redirect;
+
+ switch (ila->destination_address.type) {
+ case SIR_T_LOCAL:
+ case SIR_T_VIRTUAL:
+ break;
+ default:
+ goto redirect;
+ }
+ }
+#endif
+
+ pkt_addr = (struct addr *)&(sir->daddr);
+
+ stack_addr.addr_hi = pkt_addr->addr_hi;
+ stack_addr.addr_lo = pkt_addr->addr_lo;
+
+ reply = bpf_map_lookup_elem(&ila_lookup_map, &stack_addr);
+ if (!reply) {
+#ifdef DEBUG
+ /* Comment out if too noisy */
+ bpf_trace_printk(lookup_request, sizeof(lookup_request),
+ _ntohll(pkt_addr->addr_hi),
+ _ntohll(pkt_addr->addr_lo));
+
+ bpf_trace_printk(lookup_fail, sizeof(lookup_fail));
+#endif
+ goto redirect;
+ }
+
+#ifdef DEBUG
+ bpf_trace_printk(lookup_request, sizeof(lookup_request),
+ _ntohll(pkt_addr->addr_hi),
+ _ntohll(pkt_addr->addr_lo));
+
+ bpf_trace_printk(lookup_success, sizeof(lookup_success),
+ _ntohll(reply->addr_hi), _ntohll(reply->addr_lo));
+#endif
+
+ stack_addr.addr_hi = reply->addr_hi;
+ stack_addr.addr_lo = reply->addr_lo;
+
+ bpf_skb_store_bytes(skb, IPV6_DEST_OFF, &stack_addr,
+ sizeof(struct in6_addr), 0);
+
+redirect:
+ return bpf_redirect(skb->ifindex, 1);
+}
diff --git a/samples/bpf/inet_helper.h b/samples/bpf/inet_helper.h
new file mode 100644
index 0000000..d5fc281
--- /dev/null
+++ b/samples/bpf/inet_helper.h
@@ -0,0 +1,38 @@
+#ifndef __INET_HELPER_H
+#define __INET_HELPER_H
+
+#include <linux/inet.h>
+
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+#define _htonl(A) __builtin_bswap32(A)
+#elif defined(__BIG_ENDIAN_BITFIELD)
+#define _htonl(A) (A)
+#else
+#error "Fix asm/byteorder.h"
+#endif
+
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+#define _ntohl(A) __builtin_bswap32(A)
+#elif defined(__BIG_ENDIAN_BITFIELD)
+#define _ntohl(A) (A)
+#else
+#error "Fix asm/byteorder.h"
+#endif
+
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+#define _htonll(A) __builtin_bswap64(A)
+#elif defined(__BIG_ENDIAN_BITFIELD)
+#define _htonll(A) (A)
+#else
+#error "Fix asm/byteorder.h"
+#endif
+
+#if defined(__LITTLE_ENDIAN_BITFIELD)
+#define _ntohll(A) __builtin_bswap64(A)
+#elif defined(__BIG_ENDIAN_BITFIELD)
+#define _ntohll(A) (A)
+#else
+#error "Fix asm/byteorder.h"
+#endif
+
+#endif
--
2.8.0.rc2
^ permalink raw reply related
* [PATCH RFC 2/2] samples/bpf: ilarouter for xdp
From: Alexei Starovoitov @ 2016-09-23 17:16 UTC (permalink / raw)
To: David S . Miller
Cc: Daniel Borkmann, Jesper Dangaard Brouer, Tom Herbert,
Jamal Hadi Salim, Thomas Graf, netdev
In-Reply-To: <1474650995-2031928-1-git-send-email-ast@fb.com>
From: Aaron Yue <haoxuany@andrew.cmu.edu>
From: Aaron Yue <haoxuany@fb.com>
Requires a userspace program to insert ila mappings and mac addresses to
the ila map. Needs a verifier patch to directly allow access to the pkt
from the bpf map.
Signed-off-by: Aaron Yue <haoxuany@fb.com>
Signed-off-by: Aaron Yue <haoxuany@andrew.cmu.edu>
---
samples/bpf/Makefile | 1 +
samples/bpf/ilarouter_xdp.c | 88 +++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 89 insertions(+)
create mode 100644 samples/bpf/ilarouter_xdp.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 15e19bb..827e6e8 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -75,6 +75,7 @@ always += test_cgrp2_tc_kern.o
always += xdp1_kern.o
always += xdp2_kern.o
always += ilarouter_tc.o
+always += ilarouter_xdp.o
HOSTCFLAGS += -I$(objtree)/usr/include
diff --git a/samples/bpf/ilarouter_xdp.c b/samples/bpf/ilarouter_xdp.c
new file mode 100644
index 0000000..24749c4
--- /dev/null
+++ b/samples/bpf/ilarouter_xdp.c
@@ -0,0 +1,88 @@
+/* Copyright (c) 2016 Facebook
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of version 2 of the GNU General Public
+ * License as published by the Free Software Foundation.
+ */
+
+#define MAP_SIZE (1 << 20)
+
+#define KBUILD_MODNAME "ilarouter"
+#include <uapi/linux/if_ether.h>
+#include <uapi/linux/ipv6.h>
+#include <uapi/linux/bpf.h>
+#include "bpf_helpers.h"
+
+struct ila_addr {
+ u64 addr_hi;
+ u64 addr_lo;
+} __packed;
+
+struct ila_info {
+ struct ila_addr addr;
+ u16 mac[3];
+} __packed;
+
+char _license[] SEC("license") = "GPL";
+unsigned int version SEC("version") = 1;
+
+struct bpf_map_def SEC("map_ila_lookup_map") ila_lookup_map = {
+ .type = BPF_MAP_TYPE_HASH,
+ .key_size = sizeof(struct in6_addr),
+ .value_size = sizeof(struct ila_info),
+ .max_entries = MAP_SIZE,
+};
+
+SEC("xdp_ila_lookup")
+int ila_lookup(struct xdp_md *ctx)
+{
+ unsigned long dataptr = (unsigned long)ctx->data;
+ struct ethhdr *eth;
+ struct ipv6hdr *sir;
+ struct ila_addr *pkt_addr;
+ struct ila_info *reply;
+ u16 *dst_mac;
+
+ /* Invalid packet: length too short
+ * compiler optimization/verifier bypass:
+ * this way it won't assume that we copied over a pkt_ptr,
+ * which has register range of 0 (from (r1 + 0))
+ */
+ if (dataptr + sizeof(struct ethhdr) + sizeof(struct ipv6hdr) >
+ (unsigned long)ctx->data_end)
+ return XDP_PASS;
+
+ /* Ethernet header */
+ eth = (struct ethhdr *)dataptr;
+
+ /* Irrelevant packet: not IPv6 */
+ if (eth->h_proto != htons(ETH_P_IPV6))
+ return XDP_PASS;
+
+ /* Sir Address header */
+ sir = (struct ipv6hdr *)(dataptr + sizeof(struct ethhdr));
+
+ /* We don't have to check for C bit or Type, since
+ * userspace mapping inserts guarantees that only valid values
+ * will be inserted into the map in network byte-order.
+ * Hence, a lookup fail implies either C bit/Type is invalid,
+ * or mapping does not exist, in both cases we pass the packet without
+ * modifications.
+ */
+ pkt_addr = (struct ila_addr *)&(sir->daddr);
+ reply = bpf_map_lookup_elem(&ila_lookup_map, pkt_addr);
+
+ if (!reply)
+ return XDP_PASS;
+
+ pkt_addr->addr_hi = reply->addr.addr_hi;
+ pkt_addr->addr_lo = reply->addr.addr_lo;
+
+ dst_mac = (u16 *)eth;
+ dst_mac[0] = reply->mac[0];
+ dst_mac[1] = reply->mac[1];
+ dst_mac[2] = reply->mac[2];
+
+ return XDP_TX;
+}
+
--
2.8.0.rc2
^ permalink raw reply related
* [PATCH RFC 0/2] ila: ilarouter bpf code for tc and xdp
From: Alexei Starovoitov @ 2016-09-23 17:16 UTC (permalink / raw)
To: David S . Miller
Cc: Daniel Borkmann, Jesper Dangaard Brouer, Tom Herbert,
Jamal Hadi Salim, Thomas Graf, netdev
From: Aaron Yue <haoxuany@andrew.cmu.edu>
Jesper,
here is old email and cover letter that didn't make it to the list
due to vger outage (I guess).
The verifier patch that Aaron is talking about has landed long ago.
The dataplane of ILA router is very short and simple.
Control plane is very different matter. It's not ready for prime time yet.
----------
This patch contains the tc and xdp implementation of kernelspace bpf code.
It requires userspace to insert to the ILA bpf maps, in tc's case, the
precomputed ILA mappings, and in xdp's case, both the precomputed ILA
mappings and the MAC address.
The xdp bpf code also requires a verifier patch to allow direct map access
from the packet (will be patched in by Alexei).
Aaron Yue (2):
samples/bpf: ilarouter for tc
samples/bpf: ilarouter for xdp
samples/bpf/Makefile | 2 +
samples/bpf/ila.h | 80 ++++++++++++++++++++++++++++
samples/bpf/ilarouter_tc.c | 124 ++++++++++++++++++++++++++++++++++++++++++++
samples/bpf/ilarouter_xdp.c | 88 +++++++++++++++++++++++++++++++
samples/bpf/inet_helper.h | 38 ++++++++++++++
5 files changed, 332 insertions(+)
create mode 100644 samples/bpf/ila.h
create mode 100644 samples/bpf/ilarouter_tc.c
create mode 100644 samples/bpf/ilarouter_xdp.c
create mode 100644 samples/bpf/inet_helper.h
--
2.8.0.rc2
^ permalink raw reply
* [PATCH net] ip6_gre: fix flowi6_proto value in ip6gre_xmit_other()
From: Lance Richardson @ 2016-09-23 16:54 UTC (permalink / raw)
To: netdev
Similar to commit 3be07244b733 ("ip6_gre: fix flowi6_proto value in
xmit path"), set flowi6_proto to IPPROTO_GRE for output route lookup.
Since the correct proto is already set in the tunnel flowi6 template via
commit 252f3f5a1189 ("ip6_gre: Set flowi6_proto as IPPROTO_GRE in xmit
path."), simply delete the line setting the incorrect flowi6_proto value.
Suggested-by: Jiri Benc <jbenc@redhat.com>
Fixes: commit c12b395a4664 ("gre: Support GRE over IPv6")
Signed-off-by: Lance Richardson <lrichard@redhat.com>
---
net/ipv6/ip6_gre.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/net/ipv6/ip6_gre.c b/net/ipv6/ip6_gre.c
index 704274c..edc3daa 100644
--- a/net/ipv6/ip6_gre.c
+++ b/net/ipv6/ip6_gre.c
@@ -648,7 +648,6 @@ static int ip6gre_xmit_other(struct sk_buff *skb, struct net_device *dev)
encap_limit = t->parms.encap_limit;
memcpy(&fl6, &t->fl.u.ip6, sizeof(fl6));
- fl6.flowi6_proto = skb->protocol;
err = gre_handle_offloads(skb, !!(t->parms.o_flags & TUNNEL_CSUM));
if (err)
--
2.5.5
^ permalink raw reply related
* Re: Alignment issues with freescale FEC driver
From: Eric Dumazet @ 2016-09-23 16:54 UTC (permalink / raw)
To: Eric Nelson
Cc: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
rmk+kernel, Fugang Duan, Troy Kisky, Otavio Salvador, Simone
In-Reply-To: <02afb707-65de-5101-a79b-355929c4e00b@nelint.com>
On Fri, Sep 23, 2016 at 9:43 AM, Eric Nelson <eric@nelint.com> wrote:
>
> Hello all,
>
> We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
>
> root@mx6ul:~# cat /proc/cpu/alignment
> User: 0
> System: 470010 (inet_gro_receive+0x104/0x278)
>
> This seems to be related to the ip header alignment, and there
> was much discussion in mailing list threads [1] and [2].
>
> In particular, Russell referred to a patch here, but I haven't been
> able to find it:
> https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html
>
> Eric Dumazet also suggested a path toward fixing it, but I don't quite
> understand the suggestion:
> http://www.spinics.net/lists/netdev/msg213166.htm
>
> The immediate problem is addressed by just reading the id and frag_offs
> fields in the iphdr structure as shown in this patch:
>
> commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
> Author: Eric Nelson <eric@nelint.com>
> Date: Fri Sep 23 08:26:03 2016 -0700
>
> net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit
> value
>
> Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807
> Signed-off-by: Eric Nelson <eric@nelint.com>
>
> diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
> index 0cc98b1..c17ef6e 100644
> --- a/net/ipv4/af_inet.c
> +++ b/net/ipv4/af_inet.c
> @@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
> sk_buff **head,
> unsigned int hlen;
> unsigned int off;
> unsigned int id;
> + unsigned int frag;
> int flush = 1;
> int proto;
>
> @@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
> sk_buff **head,
> if (unlikely(ip_fast_csum((u8 *)iph, 5)))
> goto out_unlock;
>
> - id = ntohl(*(__be32 *)&iph->id);
> - flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
> - id >>= 16;
> + id = ntohs(*(__be16 *)&iph->id);
> + frag = ntohs(*(__be16 *)&iph->frag_off);
> + flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
> ~IP_DF));
>
> for (p = *head; p; p = p->next) {
> struct iphdr *iph2;
>
This solves nothing, because a few lines after you'll have yet another
unaligned access :
((__force u32)iph->saddr ^ (__force u32)iph2->saddr) |
((__force u32)iph->daddr ^ (__force u32)iph2->daddr)) {
So you might have one less problematic access, out of hundreds of them
all over the places.
Really the problem is that whole stack depends on the assumption that
IP headers are aligned on arches that care
(ie where NET_IP_ALIGN == 2)
If your build does have NET_IP_ALIGN = 2 and you get a fault here, it
might be because of a buggy driver.
The other known case is some GRE encapsulations that break the
assumption, and this is discussed somewhere else.
^ permalink raw reply
* Re: [PATCH] net: bcmgenet: Fix EPHY reset in power up
From: Florian Fainelli @ 2016-09-23 16:54 UTC (permalink / raw)
To: Jaedon Shin, Andrew Lunn; +Cc: David Miller, Philippe Reynes, netdev
In-Reply-To: <5A71930B-ED19-4690-942D-779F1454C77A@gmail.com>
On 09/23/2016 08:04 AM, Jaedon Shin wrote:
> Hi Andrew,
>
> On 23 Sep 2016, at 11:06 PM, Andrew Lunn <andrew@lunn.ch> wrote:
>>
>> On Fri, Sep 23, 2016 at 10:20:04PM +0900, Jaedon Shin wrote:
>>> The bcmgenet_mii_reset() is always not running in power up sequence
>>> after 'commit 62469c76007e ("net: ethernet: bcmgenet: use phydev from
>>> struct net_device")'. This'll show extremely high latency and duplicate
>>> packets while interface down and up repeatedly.
>>>
>>> For now, adds again a private phydev for mii reset when runs power up to
>>> open interface.
>>
>> Hi Jaedon
>>
>> How does this fix the issue? It sounds like you are papering over the
>> crack, not truly fixing it.
>>
>> Andrew
>
> Yes, It feel like a workaround, but I think it must need v4.8 stable
> version. If we find better way that fixes internal PHY to initialize
> after re-open interface, this patch will be dropped.
I can observe the faulting behavior with 4.8-rc7 that the link below
fixed initially:
# ping fainelli-linux
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.352 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.472 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.496 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.517 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.536 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=1.557 ms (DUP!)
64 bytes from 10.112.156.244: seq=1 ttl=61 time=752.448 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.291 ms
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.421 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.444 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.464 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.483 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=1.505 ms (DUP!)
64 bytes from 10.112.156.244: seq=2 ttl=61 time=24.964 ms (DUP!)
If we revert this patch, we indeed get the normal and expected behavior
back:
# ping fainelli-linux
PING fainelli-linux (10.112.156.244): 56 data bytes
64 bytes from 10.112.156.244: seq=0 ttl=61 time=0.417 ms
64 bytes from 10.112.156.244: seq=1 ttl=61 time=0.415 ms
64 bytes from 10.112.156.244: seq=2 ttl=61 time=0.424 ms
Actually, the key thing is this:
- without Philippe's patch we call twice bcmgenet_mii_reset, and that is
intended:
- first time from bcmgenet_power_up() to make sure the PHY is
initialized *before* we get to initialize the UniMAC, this is critical
- second time from bcmgenet_mii_probe(), through the normal phy_init_hw()
- with Philippe's patch, we only get to call bcmgenet_mii_reset once, in
bcmgenet_mii_probe() because the first time in bcmgenet_power_up(),
dev->phydev is NULL, because of a prior call to phy_disconnect() in
bcmgenet_close(), unfortunately, there has been MAC activity, so the PHY
gets in a bad state
Jaedon, feel free to use the explanation above, and send a plain revert
of commit 62469c76007e11428e2ee3c6de90cbe74b588d44.
Thanks!
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH] softirq: let ksoftirqd do its job
From: Jesper Dangaard Brouer @ 2016-09-23 16:51 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Daniel Borkmann, David Miller, eric.dumazet, riel, pabeni, hannes,
linux-kernel, netdev, corbet, Ingo Molnar
In-Reply-To: <20160923115333.GJ5008@twins.programming.kicks-ass.net>
On Fri, 23 Sep 2016 13:53:33 +0200
Peter Zijlstra <peterz@infradead.org> wrote:
> On Fri, Sep 23, 2016 at 01:35:59PM +0200, Daniel Borkmann wrote:
> > On 09/02/2016 08:39 AM, David Miller wrote:
> > >
> > >I'm just kind of assuming this won't go through my tree, but I can take
> > >it if that's what everyone agrees to.
> >
> > Was this actually picked up somewhere in the mean time?
>
> I can queue it for tip. In fact, I've just done so to avoid loosing it.
> If anybody else wants it holler.
Good that you are picking this up! It is a very important fix, as least
for networking.
This is your git tree, right:
https://git.kernel.org/cgit/linux/kernel/git/peterz/queue.git/
Doesn't look like you pushed it yet, or do I need to look at a specific
branch?
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* Re: [PATCH] fs/select: add vmalloc fallback for select(2)
From: Jason Baron @ 2016-09-23 16:47 UTC (permalink / raw)
To: Nicholas Piggin, Hillf Danton
Cc: 'Vlastimil Babka', 'Alexander Viro',
linux-fsdevel, linux-kernel, linux-mm, 'Michal Hocko',
netdev, Eric Dumazet
In-Reply-To: <20160923172434.7ad8f2e0@roar.ozlabs.ibm.com>
Hi,
On 09/23/2016 03:24 AM, Nicholas Piggin wrote:
> On Fri, 23 Sep 2016 14:42:53 +0800
> "Hillf Danton" <hillf.zj@alibaba-inc.com> wrote:
>
>>>
>>> The select(2) syscall performs a kmalloc(size, GFP_KERNEL) where size grows
>>> with the number of fds passed. We had a customer report page allocation
>>> failures of order-4 for this allocation. This is a costly order, so it might
>>> easily fail, as the VM expects such allocation to have a lower-order fallback.
>>>
>>> Such trivial fallback is vmalloc(), as the memory doesn't have to be
>>> physically contiguous. Also the allocation is temporary for the duration of the
>>> syscall, so it's unlikely to stress vmalloc too much.
>>>
>>> Note that the poll(2) syscall seems to use a linked list of order-0 pages, so
>>> it doesn't need this kind of fallback.
>
> How about something like this? (untested)
>
> Eric isn't wrong about vmalloc sucking :)
>
> Thanks,
> Nick
>
>
> ---
> fs/select.c | 57 +++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 43 insertions(+), 14 deletions(-)
>
> diff --git a/fs/select.c b/fs/select.c
> index 8ed9da5..3b4834c 100644
> --- a/fs/select.c
> +++ b/fs/select.c
> @@ -555,6 +555,7 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
> void *bits;
> int ret, max_fds;
> unsigned int size;
> + size_t nr_bytes;
> struct fdtable *fdt;
> /* Allocate small arguments on the stack to save memory and be faster */
> long stack_fds[SELECT_STACK_ALLOC/sizeof(long)];
> @@ -576,21 +577,39 @@ int core_sys_select(int n, fd_set __user *inp, fd_set __user *outp,
> * since we used fdset we need to allocate memory in units of
> * long-words.
> */
> - size = FDS_BYTES(n);
> + ret = -ENOMEM;
> bits = stack_fds;
> - if (size > sizeof(stack_fds) / 6) {
> - /* Not enough space in on-stack array; must use kmalloc */
> + size = FDS_BYTES(n);
> + nr_bytes = 6 * size;
> +
> + if (unlikely(nr_bytes > PAGE_SIZE)) {
> + /* Avoid multi-page allocation if possible */
> ret = -ENOMEM;
> - bits = kmalloc(6 * size, GFP_KERNEL);
> - if (!bits)
> - goto out_nofds;
> + fds.in = kmalloc(size, GFP_KERNEL);
> + fds.out = kmalloc(size, GFP_KERNEL);
> + fds.ex = kmalloc(size, GFP_KERNEL);
> + fds.res_in = kmalloc(size, GFP_KERNEL);
> + fds.res_out = kmalloc(size, GFP_KERNEL);
> + fds.res_ex = kmalloc(size, GFP_KERNEL);
> +
> + if (!(fds.in && fds.out && fds.ex &&
> + fds.res_in && fds.res_out && fds.res_ex))
> + goto out;
> + } else {
> + if (nr_bytes > sizeof(stack_fds)) {
> + /* Not enough space in on-stack array */
> + if (nr_bytes > PAGE_SIZE * 2)
The 'if' looks extraneous?
Also, I wonder if we can just avoid some allocations altogether by
checking by if the user fd_set pointers are NULL? That can avoid failures :)
Thanks,
-Jason
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Alignment issues with freescale FEC driver
From: Eric Nelson @ 2016-09-23 16:43 UTC (permalink / raw)
To: linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
rmk+kernel, edumazet, Fugang Duan, Troy Kisky
Cc: Otavio Salvador, Simone
Hello all,
We're seeing alignment issues from the ethernet stack on an i.MX6UL board:
root@mx6ul:~# cat /proc/cpu/alignment
User: 0
System: 470010 (inet_gro_receive+0x104/0x278)
This seems to be related to the ip header alignment, and there
was much discussion in mailing list threads [1] and [2].
In particular, Russell referred to a patch here, but I haven't been
able to find it:
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002844.html
Eric Dumazet also suggested a path toward fixing it, but I don't quite
understand the suggestion:
http://www.spinics.net/lists/netdev/msg213166.htm
The immediate problem is addressed by just reading the id and frag_offs
fields in the iphdr structure as shown in this patch:
commit 98810abc911b1286a7e4a2ebdfbad66f12fae19d
Author: Eric Nelson <eric@nelint.com>
Date: Fri Sep 23 08:26:03 2016 -0700
net: ipv4: af_inet: don't read multiple 16-bit iphdr fields as a 32-bit
value
Change-Id: Idc7122c22c13ca078be31907d30ab1c3148ba807
Signed-off-by: Eric Nelson <eric@nelint.com>
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 0cc98b1..c17ef6e 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1301,6 +1301,7 @@ static struct sk_buff **inet_gro_receive(struct
sk_buff **head,
unsigned int hlen;
unsigned int off;
unsigned int id;
+ unsigned int frag;
int flush = 1;
int proto;
@@ -1326,9 +1327,9 @@ static struct sk_buff **inet_gro_receive(struct
sk_buff **head,
if (unlikely(ip_fast_csum((u8 *)iph, 5)))
goto out_unlock;
- id = ntohl(*(__be32 *)&iph->id);
- flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (id & ~IP_DF));
- id >>= 16;
+ id = ntohs(*(__be16 *)&iph->id);
+ frag = ntohs(*(__be16 *)&iph->frag_off);
+ flush = (u16)((ntohl(*(__be32 *)iph) ^ skb_gro_len(skb)) | (frag &
~IP_DF));
for (p = *head; p; p = p->next) {
struct iphdr *iph2;
The reading of both fields in one "ntohl" seems obfuscated at best and
certainly worthy of a comment about the optimization but I understand
from other notes that the fundamental problem is that the IP header should
be aligned on a 4-byte boundary and that's not possible without a memcpy.
I'd like to hear suggestions about how we can address this.
Regards,
Eric
[1] - http://www.spinics.net/lists/netdev/msg213114.html
[2] -
https://lists.linaro.org/pipermail/linaro-toolchain/2012-October/002828.html
^ 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