* Re: [PATCH] ixgbe: Look up MAC address in Open Firmware
From: Florian Fainelli @ 2014-11-14 22:23 UTC (permalink / raw)
To: Martin K. Petersen, netdev; +Cc: linux.nics
In-Reply-To: <yq1r3x5vbh9.fsf@sermon.lab.mkp.net>
On 11/14/2014 11:16 AM, Martin K. Petersen wrote:
[snip]
> +#ifdef CONFIG_OF
> +/**
> + * ixgbe_of_mac_addr - Look up MAC address in Open Firmware
> + * @adapter: Pointer to adapter struct
> + */
> +static void ixgbe_of_mac_addr(struct ixgbe_adapter *adapter)
> +{
> + struct device_node *dp = pci_device_to_OF_node(adapter->pdev);
> + struct ixgbe_hw *hw = &adapter->hw;
> + const unsigned char *addr;
> + int len;
> +
> + addr = of_get_property(dp, "local-mac-address", &len);
> + if (addr && len == 6) {
> + e_dev_info("Using OpenPROM MAC address\n");
> + memcpy(hw->mac.perm_addr, addr, 6);
> + }
Cannot you use of_get_mac_address() here which does iterate through all
the OF standard ways to specify a MAC address: mac-address,
local-mac-address and address? Benefit is that this will work for all
DT-enabled platforms.
> +
> + if (!is_valid_ether_addr(hw->mac.perm_addr)) {
> + e_dev_info("Using IDPROM MAC address\n");
> + memcpy(hw->mac.perm_addr, idprom->id_ethaddr, 6);
> + }
> +}
> +#endif
> +
> /**
> * ixgbe_probe - Device Initialization Routine
> * @pdev: PCI device information struct
> @@ -8223,6 +8253,10 @@ skip_sriov:
> goto err_sw_init;
> }
>
> +#ifdef CONFIG_OF
> + ixgbe_of_mac_addr(adapter);
> +#endif
> +
> memcpy(netdev->dev_addr, hw->mac.perm_addr, netdev->addr_len);
>
> if (!is_valid_ether_addr(netdev->dev_addr)) {
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Eric Dumazet @ 2014-11-14 22:24 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tom Herbert, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CALCETrXf8j52nmpw-A2+bQt0yoz-fiD-4GP4Qf-afwH6UjCVnw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, 2014-11-14 at 14:10 -0800, Andy Lutomirski wrote:
> I have a bunch of threads that are pinned to various CPUs or groups of
> CPUs. Each thread is responsible for a fixed set of flows. I'd like
> those flows to go to those CPUs.
>
> RFS will eventually do it, but it would be nice if I could
> deterministically ask for a flow to be routed to the right CPU. Also,
> if my thread bounces temporarily to another CPU, I don't really need
> the flow to follow it -- I'd like it to stay put.
>
> This has a significant benefit over using automatic steering: with
> automatic steering, I have to make all of the hash tables have a size
> around the square of the total number of the flows in order to make it
> reliable.
>
> Something like SO_STEER_TO_THIS_CPU would be fine, as long as it
> reported whether it worked (for my diagnostics).
This requires some kind of hardware support, and unfortunately this is
not generic.
With SO_INCOMING_CPU, you simply can pass fd of sockets around threads,
so that a dumb RSS multiqueue NIC is OK (assuming you are not using some
encapsulation that NIC is not able to parse to find L4 information)
Steering is a dream, I really think its easier to build flows so that
their RX queue matches your requirements.
We usually can pick at least one element of the 4-tuple, so its actually
possible to get this before connect().
Two cases :
1) Passive connections.
After accept(), get SO_INCOMING_CPU, then pass the fd to appropriate
thread of your pool.
2) Active connections .
find a proper 4-tuple, bind() then connect(). Eventually check
SO_INCOMING_CPU to verify your expectations.
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Andy Lutomirski @ 2014-11-14 22:27 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tom Herbert, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <1416003883.17262.72.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Fri, Nov 14, 2014 at 2:24 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, 2014-11-14 at 14:10 -0800, Andy Lutomirski wrote:
>
>> I have a bunch of threads that are pinned to various CPUs or groups of
>> CPUs. Each thread is responsible for a fixed set of flows. I'd like
>> those flows to go to those CPUs.
>>
>> RFS will eventually do it, but it would be nice if I could
>> deterministically ask for a flow to be routed to the right CPU. Also,
>> if my thread bounces temporarily to another CPU, I don't really need
>> the flow to follow it -- I'd like it to stay put.
>>
>> This has a significant benefit over using automatic steering: with
>> automatic steering, I have to make all of the hash tables have a size
>> around the square of the total number of the flows in order to make it
>> reliable.
>>
>> Something like SO_STEER_TO_THIS_CPU would be fine, as long as it
>> reported whether it worked (for my diagnostics).
>
> This requires some kind of hardware support, and unfortunately this is
> not generic.
>
> With SO_INCOMING_CPU, you simply can pass fd of sockets around threads,
> so that a dumb RSS multiqueue NIC is OK (assuming you are not using some
> encapsulation that NIC is not able to parse to find L4 information)
I can't really do this. It means that the performance of my system
will be wildly different every time I restart it. I don't have enough
connections for everything to average out.
>
> Steering is a dream, I really think its easier to build flows so that
> their RX queue matches your requirements.
I have supporting hardware :) I just want it to work without
programming the ntuple table myself.
>
> We usually can pick at least one element of the 4-tuple, so its actually
> possible to get this before connect().
>
Hmm. An API for that would be quite nice :)
>
> Two cases :
>
> 1) Passive connections.
>
> After accept(), get SO_INCOMING_CPU, then pass the fd to appropriate
> thread of your pool.
>
> 2) Active connections .
> find a proper 4-tuple, bind() then connect(). Eventually check
> SO_INCOMING_CPU to verify your expectations.
The people at the other end will be really pissed if that results in
lots of reconnections.
--Andy
>
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH net-next] fast_hash: clobber registers correctly for inline function use
From: Hannes Frederic Sowa @ 2014-11-14 22:37 UTC (permalink / raw)
To: Jay Vosburgh
Cc: David Miller, eric.dumazet, netdev, ogerlitz, pshelar, jesse,
discuss
In-Reply-To: <18948.1416003002@famine>
Hi Jay,
On Fr, 2014-11-14 at 14:10 -0800, Jay Vosburgh wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> wrote:
> [...]
> >I created it via the function calling convention documented in
> >arch/x86/include/asm/calling.h, so I specified each register which a
> >function is allowed to clobber with.
> >
> >I currently cannot see how I can resolve the invalid constraints error
> >easily. :(
> >
> >So either go with my first patch, which I puts the alternative_call
> >switch point into its own function without ever inlining or the patch
> >needs to be reverted. :/
>
> As a data point, I tested the first patch as well, and the
> system does not panic with it in place. Inspection shows that it's
> using %r14 in place of %r8 in the prior (crashing) implementation.
Yes, I also could reproduce your oops and the first unoffical patch and
the first offical one both fixed it. After that, I thought that just
adding more clobbers cannot introduce bugs, so I only did compile
testing until I hit a window where gcc got mad with the excessive use of
clobbered registers but haven't tested the inline call sites that much
(sorry). :(
> Disassembly of the call site (on the non-sse4_1 system) in
> ovs_flow_tbl_insert with the first patch applied looks like this:
>
> 0xffffffffa00b6bb9 <ovs_flow_tbl_insert+0xb9>: mov %r15,0x348(%r14)
> 0xffffffffa00b6bc0 <ovs_flow_tbl_insert+0xc0>: movzwl 0x28(%r15),%ecx
> 0xffffffffa00b6bc5 <ovs_flow_tbl_insert+0xc5>: movzwl 0x2a(%r15),%esi
> 0xffffffffa00b6bca <ovs_flow_tbl_insert+0xca>: movzwl %cx,%eax
> 0xffffffffa00b6bcd <ovs_flow_tbl_insert+0xcd>: sub %ecx,%esi
> 0xffffffffa00b6bcf <ovs_flow_tbl_insert+0xcf>: lea 0x38(%r14,%rax,1),%rdi
> 0xffffffffa00b6bd4 <ovs_flow_tbl_insert+0xd4>: sar $0x2,%esi
> 0xffffffffa00b6bd7 <ovs_flow_tbl_insert+0xd7>: callq 0xffffffff813a7810 <__jhash2>
> 0xffffffffa00b6bdc <ovs_flow_tbl_insert+0xdc>: mov %eax,0x30(%r14)
> 0xffffffffa00b6be0 <ovs_flow_tbl_insert+0xe0>: mov (%rbx),%r13
> 0xffffffffa00b6be3 <ovs_flow_tbl_insert+0xe3>: mov %r14,%rsi
> 0xffffffffa00b6be6 <ovs_flow_tbl_insert+0xe6>: mov %r13,%rdi
> 0xffffffffa00b6be9 <ovs_flow_tbl_insert+0xe9>: callq 0xffffffffa00b61a0 <table_instance_insert>
>
> Compared to the panicking version's function:
>
> 0xffffffffa01a55c9 <ovs_flow_tbl_insert+0xb9>: mov %r15,0x348(%r8)
> 0xffffffffa01a55d0 <ovs_flow_tbl_insert+0xc0>: movzwl 0x28(%r15),%ecx
> 0xffffffffa01a55d5 <ovs_flow_tbl_insert+0xc5>: movzwl 0x2a(%r15),%esi
> 0xffffffffa01a55da <ovs_flow_tbl_insert+0xca>: movzwl %cx,%eax
> 0xffffffffa01a55dd <ovs_flow_tbl_insert+0xcd>: sub %ecx,%esi
> 0xffffffffa01a55df <ovs_flow_tbl_insert+0xcf>: lea 0x38(%r8,%rax,1),%rdi
> 0xffffffffa01a55e4 <ovs_flow_tbl_insert+0xd4>: sar $0x2,%esi
> 0xffffffffa01a55e7 <ovs_flow_tbl_insert+0xd7>: callq 0xffffffff813a75c0 <__jhash2>
> 0xffffffffa01a55ec <ovs_flow_tbl_insert+0xdc>: mov %eax,0x30(%r8)
> 0xffffffffa01a55f0 <ovs_flow_tbl_insert+0xe0>: mov (%rbx),%r13
> 0xffffffffa01a55f3 <ovs_flow_tbl_insert+0xe3>: mov %r8,%rsi
> 0xffffffffa01a55f6 <ovs_flow_tbl_insert+0xe6>: mov %r13,%rdi
> 0xffffffffa01a55f9 <ovs_flow_tbl_insert+0xe9>: callq 0xffffffffa01a4ba0 <table_instance_insert>
>
> It appears to generate the same instructions, but allocates
> registers differently (using %r14 instead of %r8).
Exactly and that makes sense. While %r8 must be available for the callee
to be clobbered with, %r14 must be saved by the callee and restored
before returning. So you pass the responsibility down to the other
functions, which tries not to touch %r14 because it knows it will have
to generate code for saving and restoring.
That's the reason why I actually like the the static inline clobbering
approach so much, it gives gcc possibilities to move around the
save/restore cycles and decide itself just by aligning which registers
to use.
Also the first version does work flawlessly (which I didn't send as a
patch but only as a diff in the mail). Here gcc synthesizes a full
function call which has the same effect as the long clobber list, only
it does chain two calls right behind each other.
> The __jhash2 disassembly appears to be unchanged between the two
> versions.
Thanks for looking into this!
It is actually pretty hairy to come up with a good solution for this,
because with the alternative interface you are only allowed to alter one
instruction. jump_tables also don't work because I currently have the
opinion that they do the switch way too late. I absolutely don't want to
have inserts into a hashtable with different hashing functions depending
how early during boot they took place.
Bye,
Hannes
^ permalink raw reply
* tcp_mark_head_lost warning in Linux kernel 3.10
From: Vinson Lee @ 2014-11-14 22:39 UTC (permalink / raw)
To: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy
Cc: netdev
Hi.
We hit this networking tcp_mark_head_lost warning in Linux kernel 3.10.
------------[ cut here ]------------
WARNING: at net/ipv4/tcp_input.c:2262 tcp_mark_head_lost+0x1aa/0x1dd()
Modules linked in: netconsole configfs xt_DSCP iptable_mangle
cpufreq_ondemand ipv6 ppdev parport_pc lp parport tcp_diag inet_diag
ipmi_si ipmi_devintf ipmi_msghandler iTCO_wdt iTCO_vendor_support
acpi_cpufreq freq_table mperf coretemp kvm_intel kvm crc32c_intel
ghash_clmulni_intel microcode serio_raw i2c_i801 lpc_ich mfd_core igb
i2c_algo_bit i2c_core ptp pps_core ioatdma dca wmi
CPU: 1 PID: 16561 Not tainted 3.10.50 #1
0000000000000000 ffff88085fc23948 ffffffff814a4094 ffff88085fc23980
ffffffff8103cbf9 0000000000000000 ffff8807b644b200 0000000000000001
ffff880764e1ca40 000000004e9c4970 ffff88085fc23990 ffffffff8103ccbf
Call Trace:
<IRQ> [<ffffffff814a4094>] dump_stack+0x19/0x1b
[<ffffffff8103cbf9>] warn_slowpath_common+0x65/0x7d
[<ffffffff8103ccbf>] warn_slowpath_null+0x1a/0x1c
[<ffffffff81434c61>] tcp_mark_head_lost+0x1aa/0x1dd
[<ffffffff814367fc>] tcp_update_scoreboard+0x4f/0x13f
[<ffffffff81438beb>] tcp_fastretrans_alert+0x736/0x7be
[<ffffffff81439644>] tcp_ack+0x95e/0xb47
[<ffffffff81439be6>] tcp_rcv_established+0xde/0x442
[<ffffffff81442cab>] tcp_v4_do_rcv+0x1e6/0x3e8
[<ffffffff8144371e>] tcp_v4_rcv+0x293/0x52c
[<ffffffff81425e6a>] ? NF_HOOK_THRESH.constprop.11+0x53/0x53
[<ffffffff8142d562>] ? __inet_lookup_listener+0xfa/0x17f
[<ffffffff81425f4a>] ip_local_deliver_finish+0xe0/0x155
[<ffffffff81425e6a>] ? NF_HOOK_THRESH.constprop.11+0x53/0x53
[<ffffffff81425e48>] NF_HOOK_THRESH.constprop.11+0x31/0x53
[<ffffffff814260ef>] ip_local_deliver+0x40/0x52
[<ffffffff81425d50>] ip_rcv_finish+0x274/0x2b6
[<ffffffff81425adc>] ? inet_del_offload+0x3d/0x3d
[<ffffffff81425e48>] NF_HOOK_THRESH.constprop.11+0x31/0x53
[<ffffffff81426324>] ip_rcv+0x223/0x267
[<ffffffff813f76b4>] __netif_receive_skb_core+0x435/0x4a7
[<ffffffff8107bab8>] ? timekeeping_get_ns.constprop.10+0x11/0x36
[<ffffffff813f773e>] __netif_receive_skb+0x18/0x5a
[<ffffffff813f8813>] netif_receive_skb+0x40/0x75
[<ffffffff813f8f94>] napi_gro_receive+0x3e/0x80
[<ffffffffa004a097>] igb_clean_rx_irq+0x67e/0x69e [igb]
[<ffffffffa004a425>] igb_poll+0x36e/0x5b0 [igb]
[<ffffffff813ea49a>] ? __sk_mem_reclaim+0x2f/0x84
[<ffffffff8143ef76>] ? tcp_delack_timer_handler+0x2e/0x19c
[<ffffffff813f8a61>] net_rx_action+0xcf/0x196
[<ffffffff8106870c>] ? sched_clock_cpu+0x42/0xc7
[<ffffffff8104371d>] __do_softirq+0xd5/0x1f4
[<ffffffff814b01bc>] call_softirq+0x1c/0x30
[<ffffffff81003ecd>] do_softirq+0x33/0x6e
[<ffffffff81043931>] irq_exit+0x51/0x93
[<ffffffff814b086e>] do_IRQ+0x8e/0xa5
[<ffffffff814a85aa>] common_interrupt+0x6a/0x6a
<EOI>
---[ end trace 8b2ed2c5c443e39d ]---
net/ipv4/tcp_input.c
2200 /* Detect loss in event "A" above by marking head of queue up as lost.
2201 * For FACK or non-SACK(Reno) senders, the first "packets"
number of segments
2202 * are considered lost. For RFC3517 SACK, a segment is
considered lost if it
2203 * has at least tp->reordering SACKed seqments above it;
"packets" refers to
2204 * the maximum SACKed segments to pass before reaching this limit.
2205 */
2206 static void tcp_mark_head_lost(struct sock *sk, int packets,
int mark_head)
2207 {
[...]
2262 tcp_verify_left_out(tp);
2263 }
include/net/tcp.h
936 /* Use define here intentionally to get WARN_ON location shown
at the caller */
937 #define tcp_verify_left_out(tp) WARN_ON(tcp_left_out(tp) >
tp->packets_out)
Cheers,
Vinson
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Eric Dumazet @ 2014-11-14 22:58 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tom Herbert, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CALCETrWhsj+byMeT=WHb9eLR_vgsUe58Li8JDLaTvsSPWp1DKQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, 2014-11-14 at 14:27 -0800, Andy Lutomirski wrote:
> The people at the other end will be really pissed if that results in
> lots of reconnections.
No reconnections necessary.
I believe you misunderstood : On the 4-tuple (SADDR,SPORT,DADDR,DPORT),
you can pick for example SPORT so that hash(SADDR,SPORT,DADDR,DPORT)
maps to a known and wanted RX queue number.
Once you know that, you use bind(SADDR, SPORT), then
connect(DADDR,DPORT).
Anyway, if your hardware is able to cope with the few number of flows,
just use the hardware and be happy.
Here we want about 10 millions sockets, there is little hope for
hardware being helpful.
^ permalink raw reply
* Re: tcp_mark_head_lost warning in Linux kernel 3.10
From: Eric Dumazet @ 2014-11-14 23:00 UTC (permalink / raw)
To: Vinson Lee, Neal Cardwell
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev
In-Reply-To: <CAHTgTXXqaVzVOyGuVWpdQokY8RmYRMriaLhBecLzfg=HF6V8dQ@mail.gmail.com>
On Fri, 2014-11-14 at 14:39 -0800, Vinson Lee wrote:
> Hi.
>
> We hit this networking tcp_mark_head_lost warning in Linux kernel 3.10.
>
CC Neal, because he is working on this
> ------------[ cut here ]------------
> WARNING: at net/ipv4/tcp_input.c:2262 tcp_mark_head_lost+0x1aa/0x1dd()
> Modules linked in: netconsole configfs xt_DSCP iptable_mangle
> cpufreq_ondemand ipv6 ppdev parport_pc lp parport tcp_diag inet_diag
> ipmi_si ipmi_devintf ipmi_msghandler iTCO_wdt iTCO_vendor_support
> acpi_cpufreq freq_table mperf coretemp kvm_intel kvm crc32c_intel
> ghash_clmulni_intel microcode serio_raw i2c_i801 lpc_ich mfd_core igb
> i2c_algo_bit i2c_core ptp pps_core ioatdma dca wmi
> CPU: 1 PID: 16561 Not tainted 3.10.50 #1
> 0000000000000000 ffff88085fc23948 ffffffff814a4094 ffff88085fc23980
> ffffffff8103cbf9 0000000000000000 ffff8807b644b200 0000000000000001
> ffff880764e1ca40 000000004e9c4970 ffff88085fc23990 ffffffff8103ccbf
> Call Trace:
> <IRQ> [<ffffffff814a4094>] dump_stack+0x19/0x1b
> [<ffffffff8103cbf9>] warn_slowpath_common+0x65/0x7d
> [<ffffffff8103ccbf>] warn_slowpath_null+0x1a/0x1c
> [<ffffffff81434c61>] tcp_mark_head_lost+0x1aa/0x1dd
> [<ffffffff814367fc>] tcp_update_scoreboard+0x4f/0x13f
> [<ffffffff81438beb>] tcp_fastretrans_alert+0x736/0x7be
> [<ffffffff81439644>] tcp_ack+0x95e/0xb47
> [<ffffffff81439be6>] tcp_rcv_established+0xde/0x442
> [<ffffffff81442cab>] tcp_v4_do_rcv+0x1e6/0x3e8
> [<ffffffff8144371e>] tcp_v4_rcv+0x293/0x52c
> [<ffffffff81425e6a>] ? NF_HOOK_THRESH.constprop.11+0x53/0x53
> [<ffffffff8142d562>] ? __inet_lookup_listener+0xfa/0x17f
> [<ffffffff81425f4a>] ip_local_deliver_finish+0xe0/0x155
> [<ffffffff81425e6a>] ? NF_HOOK_THRESH.constprop.11+0x53/0x53
> [<ffffffff81425e48>] NF_HOOK_THRESH.constprop.11+0x31/0x53
> [<ffffffff814260ef>] ip_local_deliver+0x40/0x52
> [<ffffffff81425d50>] ip_rcv_finish+0x274/0x2b6
> [<ffffffff81425adc>] ? inet_del_offload+0x3d/0x3d
> [<ffffffff81425e48>] NF_HOOK_THRESH.constprop.11+0x31/0x53
> [<ffffffff81426324>] ip_rcv+0x223/0x267
> [<ffffffff813f76b4>] __netif_receive_skb_core+0x435/0x4a7
> [<ffffffff8107bab8>] ? timekeeping_get_ns.constprop.10+0x11/0x36
> [<ffffffff813f773e>] __netif_receive_skb+0x18/0x5a
> [<ffffffff813f8813>] netif_receive_skb+0x40/0x75
> [<ffffffff813f8f94>] napi_gro_receive+0x3e/0x80
> [<ffffffffa004a097>] igb_clean_rx_irq+0x67e/0x69e [igb]
> [<ffffffffa004a425>] igb_poll+0x36e/0x5b0 [igb]
> [<ffffffff813ea49a>] ? __sk_mem_reclaim+0x2f/0x84
> [<ffffffff8143ef76>] ? tcp_delack_timer_handler+0x2e/0x19c
> [<ffffffff813f8a61>] net_rx_action+0xcf/0x196
> [<ffffffff8106870c>] ? sched_clock_cpu+0x42/0xc7
> [<ffffffff8104371d>] __do_softirq+0xd5/0x1f4
> [<ffffffff814b01bc>] call_softirq+0x1c/0x30
> [<ffffffff81003ecd>] do_softirq+0x33/0x6e
> [<ffffffff81043931>] irq_exit+0x51/0x93
> [<ffffffff814b086e>] do_IRQ+0x8e/0xa5
> [<ffffffff814a85aa>] common_interrupt+0x6a/0x6a
> <EOI>
> ---[ end trace 8b2ed2c5c443e39d ]---
>
>
> net/ipv4/tcp_input.c
> 2200 /* Detect loss in event "A" above by marking head of queue up as lost.
> 2201 * For FACK or non-SACK(Reno) senders, the first "packets"
> number of segments
> 2202 * are considered lost. For RFC3517 SACK, a segment is
> considered lost if it
> 2203 * has at least tp->reordering SACKed seqments above it;
> "packets" refers to
> 2204 * the maximum SACKed segments to pass before reaching this limit.
> 2205 */
> 2206 static void tcp_mark_head_lost(struct sock *sk, int packets,
> int mark_head)
> 2207 {
> [...]
> 2262 tcp_verify_left_out(tp);
> 2263 }
>
> include/net/tcp.h
> 936 /* Use define here intentionally to get WARN_ON location shown
> at the caller */
> 937 #define tcp_verify_left_out(tp) WARN_ON(tcp_left_out(tp) >
> tp->packets_out)
>
> Cheers,
> Vinson
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Andy Lutomirski @ 2014-11-14 23:03 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tom Herbert, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <1416005912.17262.76.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Fri, Nov 14, 2014 at 2:58 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, 2014-11-14 at 14:27 -0800, Andy Lutomirski wrote:
>
>> The people at the other end will be really pissed if that results in
>> lots of reconnections.
>
> No reconnections necessary.
>
> I believe you misunderstood : On the 4-tuple (SADDR,SPORT,DADDR,DPORT),
> you can pick for example SPORT so that hash(SADDR,SPORT,DADDR,DPORT)
> maps to a known and wanted RX queue number.
>
> Once you know that, you use bind(SADDR, SPORT), then
> connect(DADDR,DPORT).
If the kernel had an API for this, I'd be all for using it.
>
> Anyway, if your hardware is able to cope with the few number of flows,
> just use the hardware and be happy.
>
> Here we want about 10 millions sockets, there is little hope for
> hardware being helpful.
>
It's the intermediate numbers that are bad.
With ten flows, the current accelerated RFS works fine. With 10M
flows, RFS is a lost cause and this solution is much nicer. With,
say, 1k flows, accelerated RFS *deserves* to work perfectly, because
the hardware has enough filter slots. But making it work reliably
requires a ridiculously large hash table, and collisions cause silent
bad behavior.
--Andy
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* [PATCH] ieee802154: fix error handling in ieee802154fake_probe()
From: Alexey Khoroshilov @ 2014-11-14 23:11 UTC (permalink / raw)
To: Alexander Aring, David S . Miller
Cc: Alexey Khoroshilov, linux-wpan, netdev, linux-kernel, ldv-project
In case of any failure ieee802154fake_probe() just calls unregister_netdev().
But it does not look safe to unregister netdevice before it was registered.
The patch implements straightforward resource deallocation in case of
failure in ieee802154fake_probe().
Found by Linux Driver Verification project (linuxtesting.org).
Signed-off-by: Alexey Khoroshilov <khoroshilov@ispras.ru>
---
drivers/net/ieee802154/fakehard.c | 13 ++++++++-----
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ieee802154/fakehard.c b/drivers/net/ieee802154/fakehard.c
index 9ce854f43917..6cbc56ad9ff4 100644
--- a/drivers/net/ieee802154/fakehard.c
+++ b/drivers/net/ieee802154/fakehard.c
@@ -377,17 +377,20 @@ static int ieee802154fake_probe(struct platform_device *pdev)
err = wpan_phy_register(phy);
if (err)
- goto out;
+ goto err_phy_reg;
err = register_netdev(dev);
- if (err < 0)
- goto out;
+ if (err)
+ goto err_netdev_reg;
dev_info(&pdev->dev, "Added ieee802154 HardMAC hardware\n");
return 0;
-out:
- unregister_netdev(dev);
+err_netdev_reg:
+ wpan_phy_unregister(phy);
+err_phy_reg:
+ free_netdev(dev);
+ wpan_phy_free(phy);
return err;
}
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Eric Dumazet @ 2014-11-14 23:32 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Tom Herbert, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CALCETrUAd50g2mYEOKT-5pEqMvwSstfQcgU8=7GRsO1XcKBSFA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, 2014-11-14 at 15:03 -0800, Andy Lutomirski wrote:
> If the kernel had an API for this, I'd be all for using it.
It would be user land code, not kernel.
Why doing a system call when you can avoid it ? ;)
Doing it in the kernel would be quite complex actually.
In userland, you can even implement this by machine learning.
For every connection you made, you get the 4-tuple and INCOMING_CPU,
then you store it in a cache.
Next time you need to connect to same remote peer, you can lookup in the
cache to find a good candidate.
It would actually be good if we could do in a single socket verb a
bind_and_connect(fd, &source, &destination)
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Andy Lutomirski @ 2014-11-14 23:40 UTC (permalink / raw)
To: Eric Dumazet
Cc: Tom Herbert, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <1416007946.17262.84.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Fri, Nov 14, 2014 at 3:32 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, 2014-11-14 at 15:03 -0800, Andy Lutomirski wrote:
>
>> If the kernel had an API for this, I'd be all for using it.
>
> It would be user land code, not kernel.
>
> Why doing a system call when you can avoid it ? ;)
>
> Doing it in the kernel would be quite complex actually.
A semi-official library would work, too. As long as it kept working.
>
> In userland, you can even implement this by machine learning.
>
> For every connection you made, you get the 4-tuple and INCOMING_CPU,
> then you store it in a cache.
Eww :(
>
> Next time you need to connect to same remote peer, you can lookup in the
> cache to find a good candidate.
>
> It would actually be good if we could do in a single socket verb a
> bind_and_connect(fd, &source, &destination)
Fair enough. Although for my use case, the time it takes to connect
is completely irrelevant.
--Andy
>
>
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Tom Herbert @ 2014-11-15 0:06 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eric Dumazet, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CALCETrXf8j52nmpw-A2+bQt0yoz-fiD-4GP4Qf-afwH6UjCVnw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Nov 14, 2014 at 2:10 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Fri, Nov 14, 2014 at 1:36 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> On Fri, Nov 14, 2014 at 12:34 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> On Fri, Nov 14, 2014 at 12:25 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On Fri, Nov 14, 2014 at 12:16 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>> On Fri, Nov 14, 2014 at 11:52 AM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>> On Fri, Nov 14, 2014 at 11:33 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>> On Fri, 2014-11-14 at 09:17 -0800, Andy Lutomirski wrote:
>>>>>>>
>>>>>>>> As a heavy user of RFS (and finder of bugs in it, too), here's my
>>>>>>>> question about this API:
>>>>>>>>
>>>>>>>> How does an application tell whether the socket represents a
>>>>>>>> non-actively-steered flow? If the flow is subject to RFS, then moving
>>>>>>>> the application handling to the socket's CPU seems problematic, as the
>>>>>>>> socket's CPU might move as well. The current implementation in this
>>>>>>>> patch seems to tell me which CPU the most recent packet came in on,
>>>>>>>> which is not necessarily very useful.
>>>>>>>
>>>>>>> Its the cpu that hit the TCP stack, bringing dozens of cache lines in
>>>>>>> its cache. This is all that matters,
>>>>>>>
>>>>>>>>
>>>>>>>> Some possibilities:
>>>>>>>>
>>>>>>>> 1. Let SO_INCOMING_CPU fail if RFS or RPS are in play.
>>>>>>>
>>>>>>> Well, idea is to not use RFS at all. Otherwise, it is useless.
>>>>>
>>>>> Sure, but how do I know that it'll be the same CPU next time?
>>>>>
>>>>>>>
>>>>>> Bear in mind this is only an interface to report RX CPU and in itself
>>>>>> doesn't provide any functionality for changing scheduling, there is
>>>>>> obviously logic needed in user space that would need to do something.
>>>>>>
>>>>>> If we track the interrupting CPU in skb, the interface could be easily
>>>>>> extended to provide the interrupting CPU, the RPS CPU (calculated at
>>>>>> reported time), and the CPU processing transport (post steering which
>>>>>> is what is currently returned). That would provide the complete
>>>>>> picture to control scheduling a flow from userspace, and an interface
>>>>>> to selectively turn off RFS for a socket would make sense then.
>>>>>
>>>>> I think that a turn-off-RFS interface would also want a way to figure
>>>>> out where the flow would go without RFS. Can the network stack do
>>>>> that (e.g. evaluate the rx indirection hash or whatever happens these
>>>>> days)?
>>>>>
>>>> Yes,. We need the rxhash and the CPU that packets are received on from
>>>> the device for the socket. The former we already have, the latter
>>>> might be done by adding a field to skbuff to set received CPU. Given
>>>> the L4 hash and interrupting CPU we can calculated the RPS CPU which
>>>> is where packet would have landed with RFS off.
>>>
>>> Hmm. I think this would be useful for me. It would *definitely* be
>>> useful for me if I could pin an RFS flow to a cpu of my choice.
>>>
>> Andy, can you elaborate a little more on your use case. I've thought
>> several times about an interface to program the flow table from
>> userspace, but never quite came up with a compelling use case and
>> there is the security concern that a user could "steal" cycles from
>> arbitrary CPUs.
>
> I have a bunch of threads that are pinned to various CPUs or groups of
> CPUs. Each thread is responsible for a fixed set of flows. I'd like
> those flows to go to those CPUs.
>
> RFS will eventually do it, but it would be nice if I could
> deterministically ask for a flow to be routed to the right CPU. Also,
> if my thread bounces temporarily to another CPU, I don't really need
> the flow to follow it -- I'd like it to stay put.
>
Okay, how about it we have a SO_RFS_LOCK_FLOW sockopt. When this is
called on a socket we can lock the socket to CPU binding to the
current CPU it is called from. It could be unlocked at a later point.
Would this satisfy your requirements?
> This has a significant benefit over using automatic steering: with
> automatic steering, I have to make all of the hash tables have a size
> around the square of the total number of the flows in order to make it
> reliable.
>
> Something like SO_STEER_TO_THIS_CPU would be fine, as long as it
> reported whether it worked (for my diagnostics).
>
>>
>>> With SO_INCOMING_CPU as described, I'm worried that people will write
>>> programs that perform very well if RFS is off, but that once that code
>>> runs with RFS on, weird things could happen.
>>>
>>> (On a side note: the RFS flow hash stuff seems to be rather buggy.
>>> Some Solarflare engineers know about this, but a fix seems to be
>>> rather slow in the works. I think that some of the bugs are in core
>>> code, though.)
>>
>> This is problems with accelerated RFS or just getting the flow hash for packets?
>
> Accelerated RFS.
>
> Digging through my email, I thought that
> net.core.rps_sock_flow_entries != the per-queue rps_flow_cnt would
> make no sense, although I haven't configured it that way.
>
> More importantly, though, I think that some of the has table stuff is
> problematic. My understanding is:
>
> get_rps_cpu may call set_rps_cpu, passing rflow =
> flow_table->flows[hash & flow_table->mask];
>
> set_rps_cpu will compute flow_id = hash & flow_table->mask, which
> looks to me like it has the property that rflow == flow_table[hash]
> (unless we race with a hash table resize).
>
> Now set_rps_cpu tries to steer the new flow to the right CPU (all good
> so far), but then it gets weird. We have a very high probability of
> old_flow == rflow. rflow->filter gets overwritten with the filter id,
> the if condition doesn't execute, and nothing gets set to
> RPS_NO_FILTER.
>
> This is technically all correct, but if there are two active flows
> with the same hash, they'll each keep getting steered to the same
> place. This wastes cycles and seems to anger the sfc driver (the
> latter is presumably an sfc bug). It also means that some of the
> filters are likely to get expired for no good reason.
>
Yes, I could see where persistent hash collisions could cause an issue
with aRFS. IIRC, I saw programming the filters to be quite an
expensive operation. Minimally if seems like there some rate limiting
change to avoid hysteresis.
> Also, shouldn't ndo_rx_flow_steer be passed the full hash, not just
> the index into the hash table? What's the point of cutting off the
> high bits?
>
In order to make aRFS transparent to the user, it was implemented to
be driven from the RFS table which hashes based on the mask so that is
why it is limited. An alternative would be to program the filters
directly from a socket using the full hash, that interface might
already exist in ntuple filtering I suppose.
> --Andy
>
> --Andy
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Tom Herbert @ 2014-11-15 0:15 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eric Dumazet, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CALCETrVQqsJKqVU4ENEfi86wkQLQ=mqBif=HcQiKT2h_fj22xQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Nov 14, 2014 at 2:18 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Fri, Nov 14, 2014 at 2:16 PM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>> On Fri, 2014-11-14 at 12:16 -0800, Andy Lutomirski wrote:
>>
>>> Sure, but how do I know that it'll be the same CPU next time?
>>
>> Because the NIC always use same RX queue for a given flow.
>>
>> So if you setup your IRQ affinities properly, the same CPU will drain
>> packets from this RX queue. And since RFS is off, you have the guarantee
>> the same CPU will be used to process packets in TCP stack.
>
> Right. My concern is that if RFS is off, everything works fine, but
> if RFS is on (and the app has no good way to know), then silly results
> will happen. I think I'd rather have the getsockopt fail if RFS is on
> or at least give some indication so that the app can react
> accordingly.
>
As I mentioned, there is no material functionality in this patch and
it should be independent of RFS. It simply returns the CPU where the
stack processed the packet. Whether or not this is meaningful
information to the algorithm being implemented in userspace is
completely up to the caller to decide.
Tom
> --Andy
>
>>
>> This SO_INCOMING_CPU info is a hint, there is no guarantee eg if you use
>> bonding and some load balancer or switch decides to send packets on
>> different links.
>>
>> Most NIC use Toeplitz hash, so given the 4-tuple, and rss key (40
>> bytes), you can actually compute the hash in software and know on which
>> RX queue traffic should land.
>>
>>
>
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Andy Lutomirski @ 2014-11-15 0:24 UTC (permalink / raw)
To: Tom Herbert
Cc: Eric Dumazet, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CA+mtBx9Z33O0x4-MG=66Fb4qvBJfxV54xOyiYvWydr=4Bka2xw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Nov 14, 2014 at 4:06 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Nov 14, 2014 at 2:10 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Fri, Nov 14, 2014 at 1:36 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Fri, Nov 14, 2014 at 12:34 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>> On Fri, Nov 14, 2014 at 12:25 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On Fri, Nov 14, 2014 at 12:16 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>>> On Fri, Nov 14, 2014 at 11:52 AM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>> On Fri, Nov 14, 2014 at 11:33 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>> On Fri, 2014-11-14 at 09:17 -0800, Andy Lutomirski wrote:
>>>>>>>>
>>>>>>>>> As a heavy user of RFS (and finder of bugs in it, too), here's my
>>>>>>>>> question about this API:
>>>>>>>>>
>>>>>>>>> How does an application tell whether the socket represents a
>>>>>>>>> non-actively-steered flow? If the flow is subject to RFS, then moving
>>>>>>>>> the application handling to the socket's CPU seems problematic, as the
>>>>>>>>> socket's CPU might move as well. The current implementation in this
>>>>>>>>> patch seems to tell me which CPU the most recent packet came in on,
>>>>>>>>> which is not necessarily very useful.
>>>>>>>>
>>>>>>>> Its the cpu that hit the TCP stack, bringing dozens of cache lines in
>>>>>>>> its cache. This is all that matters,
>>>>>>>>
>>>>>>>>>
>>>>>>>>> Some possibilities:
>>>>>>>>>
>>>>>>>>> 1. Let SO_INCOMING_CPU fail if RFS or RPS are in play.
>>>>>>>>
>>>>>>>> Well, idea is to not use RFS at all. Otherwise, it is useless.
>>>>>>
>>>>>> Sure, but how do I know that it'll be the same CPU next time?
>>>>>>
>>>>>>>>
>>>>>>> Bear in mind this is only an interface to report RX CPU and in itself
>>>>>>> doesn't provide any functionality for changing scheduling, there is
>>>>>>> obviously logic needed in user space that would need to do something.
>>>>>>>
>>>>>>> If we track the interrupting CPU in skb, the interface could be easily
>>>>>>> extended to provide the interrupting CPU, the RPS CPU (calculated at
>>>>>>> reported time), and the CPU processing transport (post steering which
>>>>>>> is what is currently returned). That would provide the complete
>>>>>>> picture to control scheduling a flow from userspace, and an interface
>>>>>>> to selectively turn off RFS for a socket would make sense then.
>>>>>>
>>>>>> I think that a turn-off-RFS interface would also want a way to figure
>>>>>> out where the flow would go without RFS. Can the network stack do
>>>>>> that (e.g. evaluate the rx indirection hash or whatever happens these
>>>>>> days)?
>>>>>>
>>>>> Yes,. We need the rxhash and the CPU that packets are received on from
>>>>> the device for the socket. The former we already have, the latter
>>>>> might be done by adding a field to skbuff to set received CPU. Given
>>>>> the L4 hash and interrupting CPU we can calculated the RPS CPU which
>>>>> is where packet would have landed with RFS off.
>>>>
>>>> Hmm. I think this would be useful for me. It would *definitely* be
>>>> useful for me if I could pin an RFS flow to a cpu of my choice.
>>>>
>>> Andy, can you elaborate a little more on your use case. I've thought
>>> several times about an interface to program the flow table from
>>> userspace, but never quite came up with a compelling use case and
>>> there is the security concern that a user could "steal" cycles from
>>> arbitrary CPUs.
>>
>> I have a bunch of threads that are pinned to various CPUs or groups of
>> CPUs. Each thread is responsible for a fixed set of flows. I'd like
>> those flows to go to those CPUs.
>>
>> RFS will eventually do it, but it would be nice if I could
>> deterministically ask for a flow to be routed to the right CPU. Also,
>> if my thread bounces temporarily to another CPU, I don't really need
>> the flow to follow it -- I'd like it to stay put.
>>
> Okay, how about it we have a SO_RFS_LOCK_FLOW sockopt. When this is
> called on a socket we can lock the socket to CPU binding to the
> current CPU it is called from. It could be unlocked at a later point.
> Would this satisfy your requirements?
Yes, I think. Especially if it bypassed the hash table.
>
>> This has a significant benefit over using automatic steering: with
>> automatic steering, I have to make all of the hash tables have a size
>> around the square of the total number of the flows in order to make it
>> reliable.
>>
>> Something like SO_STEER_TO_THIS_CPU would be fine, as long as it
>> reported whether it worked (for my diagnostics).
>>
>>>
>>>> With SO_INCOMING_CPU as described, I'm worried that people will write
>>>> programs that perform very well if RFS is off, but that once that code
>>>> runs with RFS on, weird things could happen.
>>>>
>>>> (On a side note: the RFS flow hash stuff seems to be rather buggy.
>>>> Some Solarflare engineers know about this, but a fix seems to be
>>>> rather slow in the works. I think that some of the bugs are in core
>>>> code, though.)
>>>
>>> This is problems with accelerated RFS or just getting the flow hash for packets?
>>
>> Accelerated RFS.
>>
>> Digging through my email, I thought that
>> net.core.rps_sock_flow_entries != the per-queue rps_flow_cnt would
>> make no sense, although I haven't configured it that way.
>>
>> More importantly, though, I think that some of the has table stuff is
>> problematic. My understanding is:
>>
>> get_rps_cpu may call set_rps_cpu, passing rflow =
>> flow_table->flows[hash & flow_table->mask];
>>
>> set_rps_cpu will compute flow_id = hash & flow_table->mask, which
>> looks to me like it has the property that rflow == flow_table[hash]
>> (unless we race with a hash table resize).
>>
>> Now set_rps_cpu tries to steer the new flow to the right CPU (all good
>> so far), but then it gets weird. We have a very high probability of
>> old_flow == rflow. rflow->filter gets overwritten with the filter id,
>> the if condition doesn't execute, and nothing gets set to
>> RPS_NO_FILTER.
>>
>> This is technically all correct, but if there are two active flows
>> with the same hash, they'll each keep getting steered to the same
>> place. This wastes cycles and seems to anger the sfc driver (the
>> latter is presumably an sfc bug). It also means that some of the
>> filters are likely to get expired for no good reason.
>>
> Yes, I could see where persistent hash collisions could cause an issue
> with aRFS. IIRC, I saw programming the filters to be quite an
> expensive operation. Minimally if seems like there some rate limiting
> change to avoid hysteresis.
They're especially expensive, given that they often generate printks
for me due to the sfc bug.
> As I mentioned, there is no material functionality in this patch and
> it should be independent of RFS. It simply returns the CPU where the
> stack processed the packet. Whether or not this is meaningful
> information to the algorithm being implemented in userspace is
> completely up to the caller to decide.
Agreed.
My only concern is that writing that userspace algorithm might result
in surprises if RFS is on. Having the user program notice the problem
early and alert the admin might help keep Murphy's Law at bay here.
--Andy
--
Andy Lutomirski
AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH 1/1 net-next] Bluetooth: hidp: replace kzalloc/copy_from_user by memdup_user
From: Marcel Holtmann @ 2014-11-15 0:31 UTC (permalink / raw)
To: Fabian Frederick
Cc: linux-kernel, Gustavo F. Padovan, Johan Hedberg, David S. Miller,
linux-bluetooth, netdev
In-Reply-To: <1415990105-28609-1-git-send-email-fabf@skynet.be>
Hi Fabian,
> use memdup_user for rd_data import.
>
> Signed-off-by: Fabian Frederick <fabf@skynet.be>
> ---
> net/bluetooth/hidp/core.c | 10 +++-------
> 1 file changed, 3 insertions(+), 7 deletions(-)
patch has been applied to bluetooth-next tree.
Regards
Marcel
^ permalink raw reply
* [PATCH net] dcbnl : Disable software interrupts before taking dcb_lock
From: Anish Bhatt @ 2014-11-15 0:38 UTC (permalink / raw)
To: netdev; +Cc: davem, john.fastabend, neerav.parikh, Anish Bhatt
Solves possible lockup issues that can be seen from firmware DCB agents calling
into the DCB app api.
DCB firmware event queues can be tied in with NAPI so that dcb events are
generated in softIRQ context. This can results in calls to dcb_*app()
functions which try to take the dcb_lock.
If the the event triggers while we also have the dcb_lock because lldpad or
some other agent happened to be issuing a get/set command we could see a cpu
lockup.
This code was not originally written with firmware agents in mind, hence
grabbing dcb_lock from softIRQ context was not considered.
Signed-off-by: Anish Bhatt <anish@chelsio.com>
---
net/dcb/dcbnl.c | 36 ++++++++++++++++++------------------
1 file changed, 18 insertions(+), 18 deletions(-)
diff --git a/net/dcb/dcbnl.c b/net/dcb/dcbnl.c
index ca11d28..93ea801 100644
--- a/net/dcb/dcbnl.c
+++ b/net/dcb/dcbnl.c
@@ -1080,13 +1080,13 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
if (!app)
return -EMSGSIZE;
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
list_for_each_entry(itr, &dcb_app_list, list) {
if (itr->ifindex == netdev->ifindex) {
err = nla_put(skb, DCB_ATTR_IEEE_APP, sizeof(itr->app),
&itr->app);
if (err) {
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
return -EMSGSIZE;
}
}
@@ -1097,7 +1097,7 @@ static int dcbnl_ieee_fill(struct sk_buff *skb, struct net_device *netdev)
else
dcbx = -EOPNOTSUPP;
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
nla_nest_end(skb, app);
/* get peer info if available */
@@ -1234,7 +1234,7 @@ static int dcbnl_cee_fill(struct sk_buff *skb, struct net_device *netdev)
}
/* local app */
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
app = nla_nest_start(skb, DCB_ATTR_CEE_APP_TABLE);
if (!app)
goto dcb_unlock;
@@ -1271,7 +1271,7 @@ static int dcbnl_cee_fill(struct sk_buff *skb, struct net_device *netdev)
else
dcbx = -EOPNOTSUPP;
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
/* features flags */
if (ops->getfeatcfg) {
@@ -1326,7 +1326,7 @@ static int dcbnl_cee_fill(struct sk_buff *skb, struct net_device *netdev)
return 0;
dcb_unlock:
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
nla_put_failure:
return err;
}
@@ -1762,10 +1762,10 @@ u8 dcb_getapp(struct net_device *dev, struct dcb_app *app)
struct dcb_app_type *itr;
u8 prio = 0;
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
if ((itr = dcb_app_lookup(app, dev->ifindex, 0)))
prio = itr->app.priority;
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
return prio;
}
@@ -1789,7 +1789,7 @@ int dcb_setapp(struct net_device *dev, struct dcb_app *new)
if (dev->dcbnl_ops->getdcbx)
event.dcbx = dev->dcbnl_ops->getdcbx(dev);
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
/* Search for existing match and replace */
if ((itr = dcb_app_lookup(new, dev->ifindex, 0))) {
if (new->priority)
@@ -1804,7 +1804,7 @@ int dcb_setapp(struct net_device *dev, struct dcb_app *new)
if (new->priority)
err = dcb_app_add(new, dev->ifindex);
out:
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
if (!err)
call_dcbevent_notifiers(DCB_APP_EVENT, &event);
return err;
@@ -1823,10 +1823,10 @@ u8 dcb_ieee_getapp_mask(struct net_device *dev, struct dcb_app *app)
struct dcb_app_type *itr;
u8 prio = 0;
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
if ((itr = dcb_app_lookup(app, dev->ifindex, 0)))
prio |= 1 << itr->app.priority;
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
return prio;
}
@@ -1850,7 +1850,7 @@ int dcb_ieee_setapp(struct net_device *dev, struct dcb_app *new)
if (dev->dcbnl_ops->getdcbx)
event.dcbx = dev->dcbnl_ops->getdcbx(dev);
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
/* Search for existing match and abort if found */
if (dcb_app_lookup(new, dev->ifindex, new->priority)) {
err = -EEXIST;
@@ -1859,7 +1859,7 @@ int dcb_ieee_setapp(struct net_device *dev, struct dcb_app *new)
err = dcb_app_add(new, dev->ifindex);
out:
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
if (!err)
call_dcbevent_notifiers(DCB_APP_EVENT, &event);
return err;
@@ -1882,7 +1882,7 @@ int dcb_ieee_delapp(struct net_device *dev, struct dcb_app *del)
if (dev->dcbnl_ops->getdcbx)
event.dcbx = dev->dcbnl_ops->getdcbx(dev);
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
/* Search for existing match and remove it. */
if ((itr = dcb_app_lookup(del, dev->ifindex, del->priority))) {
list_del(&itr->list);
@@ -1890,7 +1890,7 @@ int dcb_ieee_delapp(struct net_device *dev, struct dcb_app *del)
err = 0;
}
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
if (!err)
call_dcbevent_notifiers(DCB_APP_EVENT, &event);
return err;
@@ -1902,12 +1902,12 @@ static void dcb_flushapp(void)
struct dcb_app_type *app;
struct dcb_app_type *tmp;
- spin_lock(&dcb_lock);
+ spin_lock_bh(&dcb_lock);
list_for_each_entry_safe(app, tmp, &dcb_app_list, list) {
list_del(&app->list);
kfree(app);
}
- spin_unlock(&dcb_lock);
+ spin_unlock_bh(&dcb_lock);
}
static int __init dcbnl_init(void)
--
2.1.3
^ permalink raw reply related
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Tom Herbert @ 2014-11-15 0:40 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Eric Dumazet, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CALCETrXnM5zCt651QWF0Z3c197gqzbLA29bQzwi64DnCvS48NQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Nov 14, 2014 at 4:24 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
> On Fri, Nov 14, 2014 at 4:06 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>> On Fri, Nov 14, 2014 at 2:10 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>> On Fri, Nov 14, 2014 at 1:36 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>> On Fri, Nov 14, 2014 at 12:34 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>> On Fri, Nov 14, 2014 at 12:25 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>> On Fri, Nov 14, 2014 at 12:16 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>>>> On Fri, Nov 14, 2014 at 11:52 AM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>>> On Fri, Nov 14, 2014 at 11:33 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>> On Fri, 2014-11-14 at 09:17 -0800, Andy Lutomirski wrote:
>>>>>>>>>
>>>>>>>>>> As a heavy user of RFS (and finder of bugs in it, too), here's my
>>>>>>>>>> question about this API:
>>>>>>>>>>
>>>>>>>>>> How does an application tell whether the socket represents a
>>>>>>>>>> non-actively-steered flow? If the flow is subject to RFS, then moving
>>>>>>>>>> the application handling to the socket's CPU seems problematic, as the
>>>>>>>>>> socket's CPU might move as well. The current implementation in this
>>>>>>>>>> patch seems to tell me which CPU the most recent packet came in on,
>>>>>>>>>> which is not necessarily very useful.
>>>>>>>>>
>>>>>>>>> Its the cpu that hit the TCP stack, bringing dozens of cache lines in
>>>>>>>>> its cache. This is all that matters,
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Some possibilities:
>>>>>>>>>>
>>>>>>>>>> 1. Let SO_INCOMING_CPU fail if RFS or RPS are in play.
>>>>>>>>>
>>>>>>>>> Well, idea is to not use RFS at all. Otherwise, it is useless.
>>>>>>>
>>>>>>> Sure, but how do I know that it'll be the same CPU next time?
>>>>>>>
>>>>>>>>>
>>>>>>>> Bear in mind this is only an interface to report RX CPU and in itself
>>>>>>>> doesn't provide any functionality for changing scheduling, there is
>>>>>>>> obviously logic needed in user space that would need to do something.
>>>>>>>>
>>>>>>>> If we track the interrupting CPU in skb, the interface could be easily
>>>>>>>> extended to provide the interrupting CPU, the RPS CPU (calculated at
>>>>>>>> reported time), and the CPU processing transport (post steering which
>>>>>>>> is what is currently returned). That would provide the complete
>>>>>>>> picture to control scheduling a flow from userspace, and an interface
>>>>>>>> to selectively turn off RFS for a socket would make sense then.
>>>>>>>
>>>>>>> I think that a turn-off-RFS interface would also want a way to figure
>>>>>>> out where the flow would go without RFS. Can the network stack do
>>>>>>> that (e.g. evaluate the rx indirection hash or whatever happens these
>>>>>>> days)?
>>>>>>>
>>>>>> Yes,. We need the rxhash and the CPU that packets are received on from
>>>>>> the device for the socket. The former we already have, the latter
>>>>>> might be done by adding a field to skbuff to set received CPU. Given
>>>>>> the L4 hash and interrupting CPU we can calculated the RPS CPU which
>>>>>> is where packet would have landed with RFS off.
>>>>>
>>>>> Hmm. I think this would be useful for me. It would *definitely* be
>>>>> useful for me if I could pin an RFS flow to a cpu of my choice.
>>>>>
>>>> Andy, can you elaborate a little more on your use case. I've thought
>>>> several times about an interface to program the flow table from
>>>> userspace, but never quite came up with a compelling use case and
>>>> there is the security concern that a user could "steal" cycles from
>>>> arbitrary CPUs.
>>>
>>> I have a bunch of threads that are pinned to various CPUs or groups of
>>> CPUs. Each thread is responsible for a fixed set of flows. I'd like
>>> those flows to go to those CPUs.
>>>
>>> RFS will eventually do it, but it would be nice if I could
>>> deterministically ask for a flow to be routed to the right CPU. Also,
>>> if my thread bounces temporarily to another CPU, I don't really need
>>> the flow to follow it -- I'd like it to stay put.
>>>
>> Okay, how about it we have a SO_RFS_LOCK_FLOW sockopt. When this is
>> called on a socket we can lock the socket to CPU binding to the
>> current CPU it is called from. It could be unlocked at a later point.
>> Would this satisfy your requirements?
>
> Yes, I think. Especially if it bypassed the hash table.
Unfortunately we can't easily bypass the hash table. The only way I
know of to to do that is to perform the socket lookup to do steering
(I tried that early on, but it was pretty costly).
>
>>
>>> This has a significant benefit over using automatic steering: with
>>> automatic steering, I have to make all of the hash tables have a size
>>> around the square of the total number of the flows in order to make it
>>> reliable.
>>>
>>> Something like SO_STEER_TO_THIS_CPU would be fine, as long as it
>>> reported whether it worked (for my diagnostics).
>>>
>>>>
>>>>> With SO_INCOMING_CPU as described, I'm worried that people will write
>>>>> programs that perform very well if RFS is off, but that once that code
>>>>> runs with RFS on, weird things could happen.
>>>>>
>>>>> (On a side note: the RFS flow hash stuff seems to be rather buggy.
>>>>> Some Solarflare engineers know about this, but a fix seems to be
>>>>> rather slow in the works. I think that some of the bugs are in core
>>>>> code, though.)
>>>>
>>>> This is problems with accelerated RFS or just getting the flow hash for packets?
>>>
>>> Accelerated RFS.
>>>
>>> Digging through my email, I thought that
>>> net.core.rps_sock_flow_entries != the per-queue rps_flow_cnt would
>>> make no sense, although I haven't configured it that way.
>>>
>>> More importantly, though, I think that some of the has table stuff is
>>> problematic. My understanding is:
>>>
>>> get_rps_cpu may call set_rps_cpu, passing rflow =
>>> flow_table->flows[hash & flow_table->mask];
>>>
>>> set_rps_cpu will compute flow_id = hash & flow_table->mask, which
>>> looks to me like it has the property that rflow == flow_table[hash]
>>> (unless we race with a hash table resize).
>>>
>>> Now set_rps_cpu tries to steer the new flow to the right CPU (all good
>>> so far), but then it gets weird. We have a very high probability of
>>> old_flow == rflow. rflow->filter gets overwritten with the filter id,
>>> the if condition doesn't execute, and nothing gets set to
>>> RPS_NO_FILTER.
>>>
>>> This is technically all correct, but if there are two active flows
>>> with the same hash, they'll each keep getting steered to the same
>>> place. This wastes cycles and seems to anger the sfc driver (the
>>> latter is presumably an sfc bug). It also means that some of the
>>> filters are likely to get expired for no good reason.
>>>
>> Yes, I could see where persistent hash collisions could cause an issue
>> with aRFS. IIRC, I saw programming the filters to be quite an
>> expensive operation. Minimally if seems like there some rate limiting
>> change to avoid hysteresis.
>
> They're especially expensive, given that they often generate printks
> for me due to the sfc bug.
>
:-)
>
>> As I mentioned, there is no material functionality in this patch and
>> it should be independent of RFS. It simply returns the CPU where the
>> stack processed the packet. Whether or not this is meaningful
>> information to the algorithm being implemented in userspace is
>> completely up to the caller to decide.
>
> Agreed.
>
> My only concern is that writing that userspace algorithm might result
> in surprises if RFS is on. Having the user program notice the problem
> early and alert the admin might help keep Murphy's Law at bay here.
>
By Murphy's law we'd also have to consider that the flow hash could
change after reading the results so that the scheduling done in
userspace is completely wrong until the CPU is read again.
Synchronizing kernel and device state with userspace state is not
always so easy. One way to mitigate is to use ancillary data which
would provide real time information and obviate the need for another
system call.
> --Andy
>
>
> --
> Andy Lutomirski
> AMA Capital Management, LLC
^ permalink raw reply
* Re: [PATCH net-next] net: introduce SO_INCOMING_CPU
From: Andy Lutomirski @ 2014-11-15 0:50 UTC (permalink / raw)
To: Tom Herbert
Cc: Eric Dumazet, Michael Kerrisk, David Miller, netdev, Ying Cai,
Willem de Bruijn, Neal Cardwell, Linux API
In-Reply-To: <CA+mtBx8uneXRoc5HcXED58zi44UyMZGJS6_sU3av8ST1ft7Diw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On Fri, Nov 14, 2014 at 4:40 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
> On Fri, Nov 14, 2014 at 4:24 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>> On Fri, Nov 14, 2014 at 4:06 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>> On Fri, Nov 14, 2014 at 2:10 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>> On Fri, Nov 14, 2014 at 1:36 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>> On Fri, Nov 14, 2014 at 12:34 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>>> On Fri, Nov 14, 2014 at 12:25 PM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>> On Fri, Nov 14, 2014 at 12:16 PM, Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org> wrote:
>>>>>>>> On Fri, Nov 14, 2014 at 11:52 AM, Tom Herbert <therbert-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org> wrote:
>>>>>>>>> On Fri, Nov 14, 2014 at 11:33 AM, Eric Dumazet <eric.dumazet-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>>>>>>>>>> On Fri, 2014-11-14 at 09:17 -0800, Andy Lutomirski wrote:
>>>>>>>>>>
>>>>>>>>>>> As a heavy user of RFS (and finder of bugs in it, too), here's my
>>>>>>>>>>> question about this API:
>>>>>>>>>>>
>>>>>>>>>>> How does an application tell whether the socket represents a
>>>>>>>>>>> non-actively-steered flow? If the flow is subject to RFS, then moving
>>>>>>>>>>> the application handling to the socket's CPU seems problematic, as the
>>>>>>>>>>> socket's CPU might move as well. The current implementation in this
>>>>>>>>>>> patch seems to tell me which CPU the most recent packet came in on,
>>>>>>>>>>> which is not necessarily very useful.
>>>>>>>>>>
>>>>>>>>>> Its the cpu that hit the TCP stack, bringing dozens of cache lines in
>>>>>>>>>> its cache. This is all that matters,
>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> Some possibilities:
>>>>>>>>>>>
>>>>>>>>>>> 1. Let SO_INCOMING_CPU fail if RFS or RPS are in play.
>>>>>>>>>>
>>>>>>>>>> Well, idea is to not use RFS at all. Otherwise, it is useless.
>>>>>>>>
>>>>>>>> Sure, but how do I know that it'll be the same CPU next time?
>>>>>>>>
>>>>>>>>>>
>>>>>>>>> Bear in mind this is only an interface to report RX CPU and in itself
>>>>>>>>> doesn't provide any functionality for changing scheduling, there is
>>>>>>>>> obviously logic needed in user space that would need to do something.
>>>>>>>>>
>>>>>>>>> If we track the interrupting CPU in skb, the interface could be easily
>>>>>>>>> extended to provide the interrupting CPU, the RPS CPU (calculated at
>>>>>>>>> reported time), and the CPU processing transport (post steering which
>>>>>>>>> is what is currently returned). That would provide the complete
>>>>>>>>> picture to control scheduling a flow from userspace, and an interface
>>>>>>>>> to selectively turn off RFS for a socket would make sense then.
>>>>>>>>
>>>>>>>> I think that a turn-off-RFS interface would also want a way to figure
>>>>>>>> out where the flow would go without RFS. Can the network stack do
>>>>>>>> that (e.g. evaluate the rx indirection hash or whatever happens these
>>>>>>>> days)?
>>>>>>>>
>>>>>>> Yes,. We need the rxhash and the CPU that packets are received on from
>>>>>>> the device for the socket. The former we already have, the latter
>>>>>>> might be done by adding a field to skbuff to set received CPU. Given
>>>>>>> the L4 hash and interrupting CPU we can calculated the RPS CPU which
>>>>>>> is where packet would have landed with RFS off.
>>>>>>
>>>>>> Hmm. I think this would be useful for me. It would *definitely* be
>>>>>> useful for me if I could pin an RFS flow to a cpu of my choice.
>>>>>>
>>>>> Andy, can you elaborate a little more on your use case. I've thought
>>>>> several times about an interface to program the flow table from
>>>>> userspace, but never quite came up with a compelling use case and
>>>>> there is the security concern that a user could "steal" cycles from
>>>>> arbitrary CPUs.
>>>>
>>>> I have a bunch of threads that are pinned to various CPUs or groups of
>>>> CPUs. Each thread is responsible for a fixed set of flows. I'd like
>>>> those flows to go to those CPUs.
>>>>
>>>> RFS will eventually do it, but it would be nice if I could
>>>> deterministically ask for a flow to be routed to the right CPU. Also,
>>>> if my thread bounces temporarily to another CPU, I don't really need
>>>> the flow to follow it -- I'd like it to stay put.
>>>>
>>> Okay, how about it we have a SO_RFS_LOCK_FLOW sockopt. When this is
>>> called on a socket we can lock the socket to CPU binding to the
>>> current CPU it is called from. It could be unlocked at a later point.
>>> Would this satisfy your requirements?
>>
>> Yes, I think. Especially if it bypassed the hash table.
>
> Unfortunately we can't easily bypass the hash table. The only way I
> know of to to do that is to perform the socket lookup to do steering
> (I tried that early on, but it was pretty costly).
What happens if you just call ndo_rx_flow_steer and do something to
keep the result from expiring?
>>
>>> As I mentioned, there is no material functionality in this patch and
>>> it should be independent of RFS. It simply returns the CPU where the
>>> stack processed the packet. Whether or not this is meaningful
>>> information to the algorithm being implemented in userspace is
>>> completely up to the caller to decide.
>>
>> Agreed.
>>
>> My only concern is that writing that userspace algorithm might result
>> in surprises if RFS is on. Having the user program notice the problem
>> early and alert the admin might help keep Murphy's Law at bay here.
>>
> By Murphy's law we'd also have to consider that the flow hash could
> change after reading the results so that the scheduling done in
> userspace is completely wrong until the CPU is read again.
> Synchronizing kernel and device state with userspace state is not
> always so easy. One way to mitigate is to use ancillary data which
> would provide real time information and obviate the need for another
> system call.
Hmm. That would work, too. I don't know how annoyed user code would
be at having to read ancillary data, though.
The flow hash really shouldn't change much, though, right?
--Andy
^ permalink raw reply
* [GIT net] Open vSwitch
From: Pravin B Shelar @ 2014-11-15 1:37 UTC (permalink / raw)
To: davem; +Cc: netdev
Following fixes are accumulated in ovs-repo.
Three of them are related to protocol processing, one is
related to memory leak in case of error and one is to
fix race.
Patch "Validate IPv6 flow key and mask values" has conflicts
with net-next, Let me know if you want me to send the patch
for net-next.
----------------------------------------------------------------
The following changes since commit b23dc5a7cc6ebc9a0d57351da7a0e8454c9ffea3:
Merge tag 'for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/mst/vhost (2014-11-13 18:07:52 -0800)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/pshelar/openvswitch.git net_ovs
for you to fetch changes up to fecaef85f7188ad1822210e2c7a7625c9a32a8e4:
openvswitch: Validate IPv6 flow key and mask values. (2014-11-14 15:13:26 -0800)
----------------------------------------------------------------
Daniele Di Proietto (1):
openvswitch: Fix NDP flow mask validation
Jarno Rajahalme (1):
openvswitch: Validate IPv6 flow key and mask values.
Jesse Gross (1):
openvswitch: Fix checksum calculation when modifying ICMPv6 packets.
Pravin B Shelar (2):
openvswitch: Fix memory leak.
openvswitch: Convert dp rcu read operation to locked operations
net/openvswitch/actions.c | 10 ++++++----
net/openvswitch/datapath.c | 14 +++++++-------
net/openvswitch/flow_netlink.c | 9 ++++++++-
3 files changed, 21 insertions(+), 12 deletions(-)
^ permalink raw reply
* [PATCH net 1/5] openvswitch: Fix memory leak.
From: Pravin B Shelar @ 2014-11-15 1:37 UTC (permalink / raw)
To: davem; +Cc: netdev, Pravin B Shelar
Need to free memory in case of sample action error.
Introduced by commit 651887b0c22cffcfce7eb9c ("openvswitch: Sample
action without side effects").
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
net/openvswitch/actions.c | 2 --
1 file changed, 2 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 006886d..00e447a 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -722,8 +722,6 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
case OVS_ACTION_ATTR_SAMPLE:
err = sample(dp, skb, key, a);
- if (unlikely(err)) /* skb already freed. */
- return err;
break;
}
--
1.9.3
^ permalink raw reply related
* [PATCH net 2/5] openvswitch: Fix checksum calculation when modifying ICMPv6 packets.
From: Pravin B Shelar @ 2014-11-15 1:37 UTC (permalink / raw)
To: davem; +Cc: netdev, Jesse Gross, Pravin B Shelar
From: Jesse Gross <jesse@nicira.com>
The checksum of ICMPv6 packets uses the IP pseudoheader as part of
the calculation, unlike ICMP in IPv4. This was not implemented,
which means that modifying the IP addresses of an ICMPv6 packet
would cause the checksum to no longer be correct as the psuedoheader
did not match.
Introduced by commit 3fdbd1ce11e5 ("openvswitch: add ipv6 'set' action").
Reported-by: Neal Shrader <icosahedral@gmail.com>
Signed-off-by: Jesse Gross <jesse@nicira.com>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
net/openvswitch/actions.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 00e447a..8c4229b 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -246,11 +246,11 @@ static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
{
int transport_len = skb->len - skb_transport_offset(skb);
- if (l4_proto == IPPROTO_TCP) {
+ if (l4_proto == NEXTHDR_TCP) {
if (likely(transport_len >= sizeof(struct tcphdr)))
inet_proto_csum_replace16(&tcp_hdr(skb)->check, skb,
addr, new_addr, 1);
- } else if (l4_proto == IPPROTO_UDP) {
+ } else if (l4_proto == NEXTHDR_UDP) {
if (likely(transport_len >= sizeof(struct udphdr))) {
struct udphdr *uh = udp_hdr(skb);
@@ -261,6 +261,10 @@ static void update_ipv6_checksum(struct sk_buff *skb, u8 l4_proto,
uh->check = CSUM_MANGLED_0;
}
}
+ } else if (l4_proto == NEXTHDR_ICMP) {
+ if (likely(transport_len >= sizeof(struct icmp6hdr)))
+ inet_proto_csum_replace16(&icmp6_hdr(skb)->icmp6_cksum,
+ skb, addr, new_addr, 1);
}
}
--
1.9.3
^ permalink raw reply related
* [PATCH net 3/5] openvswitch: Fix NDP flow mask validation
From: Pravin B Shelar @ 2014-11-15 1:37 UTC (permalink / raw)
To: davem; +Cc: netdev, Daniele Di Proietto, Pravin B Shelar
From: Daniele Di Proietto <ddiproietto@vmware.com>
match_validate() enforce that a mask matching on NDP attributes has also an
exact match on ICMPv6 type.
The ICMPv6 type, which is 8-bit wide, is stored in the 'tp.src' field of
'struct sw_flow_key', which is 16-bit wide.
Therefore, an exact match on ICMPv6 type should only check the first 8 bits.
This commit fixes a bug that prevented flows with an exact match on NDP field
from being installed
Introduced by commit 03f0d916aa03 ("openvswitch: Mega flow implementation").
Signed-off-by: Daniele Di Proietto <ddiproietto@vmware.com>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
net/openvswitch/flow_netlink.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 939bcb3..dda040e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -145,7 +145,7 @@ static bool match_validate(const struct sw_flow_match *match,
if (match->key->eth.type == htons(ETH_P_ARP)
|| match->key->eth.type == htons(ETH_P_RARP)) {
key_expected |= 1 << OVS_KEY_ATTR_ARP;
- if (match->mask && (match->mask->key.eth.type == htons(0xffff)))
+ if (match->mask && (match->mask->key.tp.src == htons(0xff)))
mask_allowed |= 1 << OVS_KEY_ATTR_ARP;
}
--
1.9.3
^ permalink raw reply related
* [PATCH net 4/5] openvswitch: Convert dp rcu read operation to locked operations
From: Pravin B Shelar @ 2014-11-15 1:38 UTC (permalink / raw)
To: davem; +Cc: netdev, Pravin B Shelar
dp read operations depends on ovs_dp_cmd_fill_info(). This API
needs to looup vport to find dp name, but vport lookup can
fail. Therefore to keep vport reference alive we need to
take ovs lock.
Introduced by commit 6093ae9abac1 ("openvswitch: Minimize
dp and vport critical sections").
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
Acked-by: Andy Zhou <azhou@nicira.com>
---
net/openvswitch/datapath.c | 14 +++++++-------
1 file changed, 7 insertions(+), 7 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index e6d7255..f9e556b 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -1265,7 +1265,7 @@ static size_t ovs_dp_cmd_msg_size(void)
return msgsize;
}
-/* Called with ovs_mutex or RCU read lock. */
+/* Called with ovs_mutex. */
static int ovs_dp_cmd_fill_info(struct datapath *dp, struct sk_buff *skb,
u32 portid, u32 seq, u32 flags, u8 cmd)
{
@@ -1555,7 +1555,7 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct genl_info *info)
if (!reply)
return -ENOMEM;
- rcu_read_lock();
+ ovs_lock();
dp = lookup_datapath(sock_net(skb->sk), info->userhdr, info->attrs);
if (IS_ERR(dp)) {
err = PTR_ERR(dp);
@@ -1564,12 +1564,12 @@ static int ovs_dp_cmd_get(struct sk_buff *skb, struct genl_info *info)
err = ovs_dp_cmd_fill_info(dp, reply, info->snd_portid,
info->snd_seq, 0, OVS_DP_CMD_NEW);
BUG_ON(err < 0);
- rcu_read_unlock();
+ ovs_unlock();
return genlmsg_reply(reply, info);
err_unlock_free:
- rcu_read_unlock();
+ ovs_unlock();
kfree_skb(reply);
return err;
}
@@ -1581,8 +1581,8 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
int skip = cb->args[0];
int i = 0;
- rcu_read_lock();
- list_for_each_entry_rcu(dp, &ovs_net->dps, list_node) {
+ ovs_lock();
+ list_for_each_entry(dp, &ovs_net->dps, list_node) {
if (i >= skip &&
ovs_dp_cmd_fill_info(dp, skb, NETLINK_CB(cb->skb).portid,
cb->nlh->nlmsg_seq, NLM_F_MULTI,
@@ -1590,7 +1590,7 @@ static int ovs_dp_cmd_dump(struct sk_buff *skb, struct netlink_callback *cb)
break;
i++;
}
- rcu_read_unlock();
+ ovs_unlock();
cb->args[0] = i;
--
1.9.3
^ permalink raw reply related
* [PATCH net 5/5] openvswitch: Validate IPv6 flow key and mask values.
From: Pravin B Shelar @ 2014-11-15 1:38 UTC (permalink / raw)
To: davem; +Cc: netdev, Jarno Rajahalme, Pravin B Shelar
From: Jarno Rajahalme <jrajahalme@nicira.com>
Reject flow label key and mask values with invalid bits set.
Introduced by commit 3fdbd1ce11e5 ("openvswitch: add ipv6 'set'
action").
Signed-off-by: Jarno Rajahalme <jrajahalme@nicira.com>
Acked-by: Jesse Gross <jesse@nicira.com>
Signed-off-by: Pravin B Shelar <pshelar@nicira.com>
---
net/openvswitch/flow_netlink.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index dda040e..fa4ec2e 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -689,6 +689,13 @@ static int ovs_key_from_nlattrs(struct sw_flow_match *match, u64 attrs,
ipv6_key->ipv6_frag, OVS_FRAG_TYPE_MAX);
return -EINVAL;
}
+
+ if (ipv6_key->ipv6_label & htonl(0xFFF00000)) {
+ OVS_NLERR("IPv6 flow label %x is out of range (max=%x).\n",
+ ntohl(ipv6_key->ipv6_label), (1 << 20) - 1);
+ return -EINVAL;
+ }
+
SW_FLOW_KEY_PUT(match, ipv6.label,
ipv6_key->ipv6_label, is_mask);
SW_FLOW_KEY_PUT(match, ip.proto,
--
1.9.3
^ permalink raw reply related
* Re: tcp_mark_head_lost warning in Linux kernel 3.10
From: Vinson Lee @ 2014-11-15 1:48 UTC (permalink / raw)
To: Neal Cardwell
Cc: Eric Dumazet, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Netdev
In-Reply-To: <CADVnQymD96k6pBmkYHb5G5MAWcRywdgEb2jo-BnNttWeii0u2Q@mail.gmail.com>
On Fri, Nov 14, 2014 at 5:36 PM, Neal Cardwell <ncardwell@google.com> wrote:
> Thanks, Vinson. The theory I'm working on involves SACK reneging. What do
> you get from:
>
> nstat -a -z | grep TcpExtTCPSACKReneging
>
$ nstat -a -z | grep TcpExtTCPSACKReneging
TcpExtTCPSACKReneging 184357 0.0
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox