* Re: [PATCH v2] tcp: tsq: restore minimal amount of queueing
From: Arnaud Ebalard @ 2013-11-13 21:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: David Miller, Sujith Manoharan, Cong Wang, netdev, Felix Fietkau
In-Reply-To: <1384353174.28458.110.camel@edumazet-glaptop2.roam.corp.google.com>
Hi Eric,
Eric Dumazet <eric.dumazet@gmail.com> writes:
> From: Eric Dumazet <edumazet@google.com>
>
> After commit c9eeec26e32e ("tcp: TSQ can use a dynamic limit"), several
> users reported throughput regressions, notably on mvneta and wifi
> adapters.
>
> 802.11 AMPDU requires a fair amount of queueing to be effective.
>
> This patch partially reverts the change done in tcp_write_xmit()
> so that the minimal amount is sysctl_tcp_limit_output_bytes.
I just tested the fix on current linux tree with the same setup
on which I observed the regression: it also fixes it on my side
on my RN102. Thanks for the quick fix, Eric.
Cheers,
a+
^ permalink raw reply
* [PATCH 2/2] net: phy: at803x: soft-reset PHY when link goes down
From: Daniel Mack @ 2013-11-13 21:07 UTC (permalink / raw)
To: netdev; +Cc: davem, marek.belisko, ujhelyi.m, Daniel Mack
In-Reply-To: <1384376870-7810-1-git-send-email-zonque@gmail.com>
When the link goes down and up again quickly and multiple times in a
row, the AT803x PHY gets stuck and won't recover unless it is soft-reset
via the BMCR register.
Unfortunately, there is no way to detect this state, as all registers
contain perfectly sane values in such cases. Hence, the only option is
to always soft-reset the PHY when the link goes down.
Reset logic is based on a patch from Marek Belisko.
Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-and-tested-by: Marek Belisko <marek.belisko@gmail.com>
---
drivers/net/phy/at803x.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 52 insertions(+)
diff --git a/drivers/net/phy/at803x.c b/drivers/net/phy/at803x.c
index bc71947..303c5ae 100644
--- a/drivers/net/phy/at803x.c
+++ b/drivers/net/phy/at803x.c
@@ -32,10 +32,56 @@
#define AT803X_DEBUG_SYSTEM_MODE_CTRL 0x05
#define AT803X_DEBUG_RGMII_TX_CLK_DLY BIT(8)
+#define AT803X_BMCR_SOFTRESET BIT(15)
+
MODULE_DESCRIPTION("Atheros 803x PHY driver");
MODULE_AUTHOR("Matus Ujhelyi");
MODULE_LICENSE("GPL");
+struct at803x_priv {
+ bool phy_reset;
+};
+
+static void at803x_adjust_state(struct phy_device *phydev)
+{
+ struct at803x_priv *priv = phydev->priv;
+
+ if (phydev->state == PHY_NOLINK) {
+ unsigned long timeout;
+ unsigned int val;
+
+ if (priv->phy_reset)
+ return;
+
+ val = phy_read(phydev, MII_BMCR);
+ val |= AT803X_BMCR_SOFTRESET;
+ phy_write(phydev, MII_BMCR, val);
+
+ timeout = jiffies + HZ/10;
+ do {
+ cpu_relax();
+ } while ((phy_read(phydev, MII_BMCR) & AT803X_BMCR_SOFTRESET) &&
+ time_after(timeout, jiffies));
+
+ WARN(phy_read(phydev, MII_BMCR) & AT803X_BMCR_SOFTRESET,
+ "failed to soft-reset phy\n");
+
+ priv->phy_reset = true;
+ } else {
+ priv->phy_reset = false;
+ }
+}
+
+static int at803x_probe(struct phy_device *phydev)
+{
+ phydev->priv = devm_kzalloc(&phydev->dev, sizeof(struct at803x_priv),
+ GFP_KERNEL);
+ if (!phydev->priv)
+ return -ENOMEM;
+
+ return 0;
+}
+
static int at803x_set_wol(struct phy_device *phydev,
struct ethtool_wolinfo *wol)
{
@@ -197,6 +243,7 @@ static struct phy_driver at803x_driver[] = {
.phy_id = 0x004dd072,
.name = "Atheros 8035 ethernet",
.phy_id_mask = 0xffffffef,
+ .probe = at803x_probe,
.config_init = at803x_config_init,
.set_wol = at803x_set_wol,
.get_wol = at803x_get_wol,
@@ -206,6 +253,7 @@ static struct phy_driver at803x_driver[] = {
.flags = PHY_HAS_INTERRUPT,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
+ .adjust_state = at803x_adjust_state,
.driver = {
.owner = THIS_MODULE,
},
@@ -214,6 +262,7 @@ static struct phy_driver at803x_driver[] = {
.phy_id = 0x004dd076,
.name = "Atheros 8030 ethernet",
.phy_id_mask = 0xffffffef,
+ .probe = at803x_probe,
.config_init = at803x_config_init,
.set_wol = at803x_set_wol,
.get_wol = at803x_get_wol,
@@ -223,6 +272,7 @@ static struct phy_driver at803x_driver[] = {
.flags = PHY_HAS_INTERRUPT,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
+ .adjust_state = at803x_adjust_state,
.driver = {
.owner = THIS_MODULE,
},
@@ -231,6 +281,7 @@ static struct phy_driver at803x_driver[] = {
.phy_id = 0x004dd074,
.name = "Atheros 8031 ethernet",
.phy_id_mask = 0xffffffef,
+ .probe = at803x_probe,
.config_init = at803x_config_init,
.set_wol = at803x_set_wol,
.get_wol = at803x_get_wol,
@@ -240,6 +291,7 @@ static struct phy_driver at803x_driver[] = {
.flags = PHY_HAS_INTERRUPT,
.config_aneg = genphy_config_aneg,
.read_status = genphy_read_status,
+ .adjust_state = at803x_adjust_state,
.driver = {
.owner = THIS_MODULE,
},
--
1.8.4.2
^ permalink raw reply related
* [PATCH 1/2] net: phylib: add adjust_state callback to phy device
From: Daniel Mack @ 2013-11-13 21:07 UTC (permalink / raw)
To: netdev; +Cc: davem, marek.belisko, ujhelyi.m, Daniel Mack
Allow phy drivers to take action when the core does its link adjustment.
No change for drivers that do not implement this callback.
Signed-off-by: Daniel Mack <zonque@gmail.com>
---
drivers/net/phy/phy.c | 3 +++
include/linux/phy.h | 2 ++
2 files changed, 5 insertions(+)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 36c6994..240e33f 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -748,6 +748,9 @@ void phy_state_machine(struct work_struct *work)
if (phydev->adjust_state)
phydev->adjust_state(phydev->attached_dev);
+ if (phydev->drv->adjust_state)
+ phydev->drv->adjust_state(phydev);
+
switch(phydev->state) {
case PHY_DOWN:
case PHY_STARTING:
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 64ab823..85826eb 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -467,6 +467,8 @@ struct phy_driver {
/* See set_wol, but for checking whether Wake on LAN is enabled. */
void (*get_wol)(struct phy_device *dev, struct ethtool_wolinfo *wol);
+ void (*adjust_state)(struct phy_device *dev);
+
struct device_driver driver;
};
#define to_phy_driver(d) container_of(d, struct phy_driver, driver)
--
1.8.4.2
^ permalink raw reply related
* Re: [PATCH 1/1] net: sctp: bug fixing when sctp path recovers
From: Chang @ 2013-11-13 20:48 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Vlad Yasevich, nhorman, davem, linux-sctp, netdev, linux-kernel,
dreibh
In-Reply-To: <5283CEC3.4050507@redhat.com>
On 11/13/2013 08:10 PM, Daniel Borkmann wrote:
> On 11/13/2013 08:06 PM, Chang wrote:
>> On 11/13/2013 09:44 AM, Daniel Borkmann wrote:
>>> On 11/13/2013 03:54 AM, Chang wrote:
>>>> On 11/13/2013 03:37 AM, Vlad Yasevich wrote:
>>>>> On 11/12/2013 08:34 PM, Chang Xiangzhong wrote:
>>>>>> Look for the __two__ most recently used path/transport and set to
>>>>>> active_path
>>>>>> and retran_path respectively
>>>
>>> Please also for the log, elaborate a bit more, explaining what
>>> currently
>>> happens, and what the effects of this bug are, so that later when
>>> people
>>> are looking through the Git log they can easily get what problem you
>>> are
>>> trying to fix; and if possible, add:
>>>
>>> Fixes: <12 digits SHA1> ("<commit title>")
>>>
>> Yeah, sure, I'll elaborate that more specifically.
>
> Thanks !
>
>> I assume the 12-digit SHA1 is the revision number. But may I ask
>> where and how shall I add the tag "Fixes" tag? The revision number is
>> generated after "git commit", how can I know that in advance?
>
> Nope, it's the affected commit id from the current git log that
> your patch fixes.
>
> Have a look for example at commit:
>
> 98bbc06aabac5a2 ("net: x86: bpf: don't forget to free sk_filter (v2)")
Thank you for your quick response. I'm quite green on kernel programming
and git. So here's one question:
To find the the revision that **caused** the bug, I could use gitk to
trace the changing of the file(s) history. Is that correct?
^ permalink raw reply
* oops in tcp_get_metrics, followed by lockup.
From: Dave Jones @ 2013-11-13 20:45 UTC (permalink / raw)
To: netdev
My fuzzer just hit this on v3.12-7033-g42a2d923cc34
Oops: 0000 [#1] PREEMPT SMP DEBUG_PAGEALLOC
Modules linked in: fuse hidp tun snd_seq_dummy bnep nfnetlink rfcomm ipt_ULOG can_bcm nfc caif_socket caif af_802154 phonet af_rxrpc bluetooth rfkill can_raw can llc2 pppoe pppox ppp
_generic slhc irda crc_ccitt rds scsi_transport_iscsi af_key rose x25 atm netrom appletalk ipx p8023 psnap p8022 llc ax25 xfs libcrc32c coretemp hwmon x86_pkg_temp_thermal kvm_intel kvm crct10dif_p
clmul crc32c_intel ghash_clmulni_intel usb_debug snd_hda_codec_realtek snd_hda_codec_hdmi microcode pcspkr snd_hda_intel snd_hda_codec snd_hwdep snd_seq snd_seq_device snd_pcm snd_page_alloc snd_ti
mer e1000e snd ptp shpchp soundcore pps_core serio_raw
CPU: 1 PID: 16002 Comm: trinity-child1 Not tainted 3.12.0+ #2
task: ffff88023cd75580 ti: ffff88009ee26000 task.ti: ffff88009ee26000
RIP: 0010:[<ffffffff81658dd2>] [<ffffffff81658dd2>] tcp_get_metrics+0x62/0x420
RSP: 0018:ffff880244a03d28 EFLAGS: 00010246
RAX: 0000000000000002 RBX: ffff88009c77a4c0 RCX: 0000000000000001
RDX: 0000000000000001 RSI: 0000000000000000 RDI: ffff88009c77a4c0
RBP: ffff880244a03d78 R08: 0000000000000001 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000000 R12: 0000000000000000
R13: 0000000000000000 R14: 00000000000010ac R15: 0000000000000000
FS: 0000000000000000(0000) GS:ffff880244a00000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 0000000000000018 CR3: 0000000001c0b000 CR4: 00000000001407e0
DR0: 0000000000000000 DR1: 0000000000000000 DR2: 0000000000000000
DR3: 0000000000000000 DR6: 00000000fffe0ff0 DR7: 0000000000000400
Stack:
000000018165a6b5 ffffffff000010ac 0000000000000246 0000000044a00002
ffffffff81c480a0 ffff88009c77a4c0 0000000000000000 0000000000000000
0000000000000001 0000000000000000 ffff880244a03db8 ffffffff8165a740
Call Trace:
<IRQ>
[<ffffffff8165a740>] tcp_fastopen_cache_set+0x90/0x280
[<ffffffff8165a6b5>] ? tcp_fastopen_cache_set+0x5/0x280
[<ffffffff8164f3a7>] tcp_retransmit_timer+0x1d7/0x930
[<ffffffff8164fcb0>] ? tcp_write_timer_handler+0x1b0/0x1b0
[<ffffffff8164fba0>] tcp_write_timer_handler+0xa0/0x1b0
[<ffffffff8164fd2c>] tcp_write_timer+0x7c/0x80
[<ffffffff81063c1a>] call_timer_fn+0x8a/0x340
[<ffffffff81063b95>] ? call_timer_fn+0x5/0x340
[<ffffffff8164fcb0>] ? tcp_write_timer_handler+0x1b0/0x1b0
[<ffffffff81064114>] run_timer_softirq+0x244/0x3a0
[<ffffffff8105aa9c>] __do_softirq+0xfc/0x490
[<ffffffff8105b28d>] irq_exit+0x13d/0x160
[<ffffffff8172fe25>] smp_apic_timer_interrupt+0x45/0x60
[<ffffffff8172eaaf>] apic_timer_interrupt+0x6f/0x80
<EOI>
[<ffffffff810d559d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff811560af>] ? free_hot_cold_page+0xff/0x180
[<ffffffff81156176>] free_hot_cold_page_list+0x46/0x160
[<ffffffff8115c21e>] release_pages+0x8e/0x1f0
[<ffffffff8118c135>] free_pages_and_swap_cache+0x95/0xb0
[<ffffffff81175acc>] tlb_flush_mmu.part.73+0x4c/0x90
[<ffffffff81176115>] tlb_finish_mmu+0x55/0x60
[<ffffffff81180d84>] exit_mmap+0xf4/0x170
[<ffffffff8105108b>] mmput+0x6b/0x100
[<ffffffff810559e8>] do_exit+0x278/0xcb0
[<ffffffff817250e1>] ? _raw_spin_unlock+0x31/0x50
[<ffffffff810d53c6>] ? trace_hardirqs_on_caller+0x16/0x1e0
[<ffffffff810d559d>] ? trace_hardirqs_on+0xd/0x10
[<ffffffff810577ec>] do_group_exit+0x4c/0xc0
[<ffffffff81057874>] SyS_exit_group+0x14/0x20
[<ffffffff8172e064>] tracesys+0xdd/0xe2
Code: 0a 0f 85 c2 01 00 00 48 8b 47 38 48 8b 57 40 48 89 44 24 08 48 8b 47 40 48 89 54 24 10 48 33 47 38 49 89 c6 49 c1 ee 20 41 31 c6 <49> 8b 45 18 b9 20 00 00 00 45 69 f6 01 00 37 9e 48 8b 80 d8 04
RIP [<ffffffff81658dd2>] tcp_get_metrics+0x62/0x420
RSP <ffff880244a03d28>
CR2: 0000000000000018
---[ end trace c25bf4de9744120a ]---
The disassembly looks like it happened here :-
static inline u32 ipv6_addr_hash(const struct in6_addr *a)
{
#if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) && BITS_PER_LONG == 64
const unsigned long *ul = (const unsigned long *)a;
unsigned long x = ul[0] ^ ul[1];
10db: 48 8b 47 40 mov 0x40(%rdi),%rax
10df: 48 89 54 24 10 mov %rdx,0x10(%rsp)
10e4: 48 33 47 38 xor 0x38(%rdi),%rax
return (u32)(x ^ (x >> 32));
10e8: 49 89 c6 mov %rax,%r14
10eb: 49 c1 ee 20 shr $0x20,%r14
10ef: 41 31 c6 xor %eax,%r14d
10f2: 49 8b 45 18 mov 0x18(%r13),%rax <<<< Faulting instruction.
10f6: b9 20 00 00 00 mov $0x20,%ecx
}
^ permalink raw reply
* Re: [PATCH net] netlink: fix netlink_ack with large messages
From: David Miller @ 2013-11-13 20:43 UTC (permalink / raw)
To: jbenc; +Cc: pablo, jhs, tgraf, netdev
In-Reply-To: <20131113122504.0ddcbf89@griffin>
From: Jiri Benc <jbenc@redhat.com>
Date: Wed, 13 Nov 2013 12:25:04 +0100
> I completely agree with this, sorry for not being clear. I just
> understood from the thread that the way to go is to do both, in order
> to not generate too large ACKs for the _new_ code (i.e. for the
> messages that were not plausible before "netlink: allow large data
> transfers from user-space"). I don't know what the "too large" should
> be, though, hence the question.
>
> But then, if we don't do any capping, the only outcome of a failed
> allocation is the ACK won't be sent and it's clearly stated that
> netlink does not provide reliability. Works for me.
Of course, we should meanwhile add the large SKB handling to the
netlink ACK code.
Therefore, please resubmit your original patch, but modify it as
I asked such that only the ACK code path gets the new large SKB
call.
Thanks.
^ permalink raw reply
* Re: [PATCH RFC] netlink: a flag requesting header w/o data in error ACK
From: David Miller @ 2013-11-13 20:31 UTC (permalink / raw)
To: jbenc; +Cc: netdev, tgraf, jhs, pablo
In-Reply-To: <231e025015eee115182368b34333b38579e3112d.1384357520.git.jbenc@redhat.com>
From: Jiri Benc <jbenc@redhat.com>
Date: Wed, 13 Nov 2013 16:46:09 +0100
> Currently, ACK in case of error contains a full copy of the originating
> message, although many applications do not need it. This can cause lost ACKs
> with large netlink messages, especially after commit c05cdb1b864f ("netlink:
> allow large data transfers from user-space").
>
> Introduce a new message flag, NLM_F_SHORT_NACK, which requests only message
> header to be included in ACK regardless of the error code. This flag has no
> effect if NLM_F_ACK is not set.
>
> Signed-off-by: Jiri Benc <jbenc@redhat.com>
> ---
> I'm not entirely happy with taking a bit in netlink message header for this.
> Alternative approach would be to make this a socket option (as suggested by
> David) but that would mean a socket lookup in netlink_ack for each and every
> ACK, which doesn't sound nice, either. If this is preferred, I'll rework the
> patch.
It's not really all that difficult, you can either attack the sender socket
to in_skb->sk, or pass a new boolean around in the various code paths that
lead to the ACK.
^ permalink raw reply
* Re: [PATCH] net/7990: Make lance_private.name const
From: David Miller @ 2013-11-13 20:13 UTC (permalink / raw)
To: geert; +Cc: sergei.shtylyov, netdev, linux-m68k
In-Reply-To: <CAMuHMdXyqOATvE8e=3Ed17bwiDRE8w=B=xKuYwd5qorxqA0BJg@mail.gmail.com>
From: Geert Uytterhoeven <geert@linux-m68k.org>
Date: Wed, 13 Nov 2013 09:54:18 +0100
> On Wed, Nov 13, 2013 at 12:16 AM, David Miller <davem@davemloft.net> wrote:
>>>> And remember about checkpatch.pl which was hardly content with the patch.
>>>
>>> Only because checkpatch looks at the _new_ lines, and doesn't compare the
>>> styles of the old and new lines.
>>
>> You really should fix the line you are changing to use tabs, please respin
>> with this in mind, thanks.
>
> As that would add more to the TAB/space mess^H^H^Hix, I prepended a
> patch to fix all whitespace errors.
I implicitly asked you not to do this, now things are more difficult
and your original change will take longer to integrate.
Becuase now it isn't a patch set I can easily just apply quickly in
the current merge window, and it's therefore a series you'll have to
resubmit later when the merge window closes and the net-next tree
opens up again.
Sorry.
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Stefan Priebe @ 2013-11-13 20:09 UTC (permalink / raw)
To: Vlad Yasevich, Veaceslav Falico; +Cc: Linux Netdev List
In-Reply-To: <5283B500.9040408@gmail.com>
Am 13.11.2013 18:21, schrieb Vlad Yasevich:
> On 11/13/2013 11:21 AM, Veaceslav Falico wrote:
>> On Wed, Nov 13, 2013 at 04:17:33PM +0100, Stefan Priebe - Profihost AG
>> wrote:
>>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>>> Hi Falico,
>>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe -
>>>>>> Profihost AG
>>>>>> wrote:
>>>>>>> Hello,
>>>>>>>
>>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>>> never
>>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>>
>>>>>>> I've this:
>>>>>>>
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> /
>>>>>>> eth3
>>>>>>>
>>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>>> bonding and using a bridge.
>>>>>>>
>>>>>>> I the past i had this:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 \ vmbr1.3000
>>>>>>> \ ---- tap114i1
>>>>>>>
>>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-(
>>>>>>> this
>>>>>>> is bad!
>>>>>>
>>>>>> As a guess - do you use arp monitoring for bonding? Try using
>>>>>> miimon -
>>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>>> patchsets that will never hit 3.10 stable.
>>>>>> Also, the bonding configuration would be welcome.
>>>>>
>>>>> Debian Bonding konfiguration looks like this:
>>>>> auto bond1
>>>>> iface bond1 inet manual
>>>>> slaves eth2 eth3
>>>>> bond-mode 802.3ad
>>>>> bond_miimon 100
>>>>> bond_updelay 200
>>>>> bond_downdelay 0
>>>>>
>>>>> This should be miimon using lacp and not arp isn't it?
>>>>> Anything more needed?
>>>>>
>>>>
>>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>>> bond should place all of its ports into promiscuous mode. Do you see
>>>> the the kernel messages that say that?
>>>
>>> No it does not - i only see:
>>> # dmesg -c|egrep "promiscuous|forward"
>>> [ 5.445161] device bond0 entered promiscuous mode
>>> [ 7.670701] device bond1 entered promiscuous mode
>>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>>>
>>> Now adding variant 1:
>>> # dmesg -c|egrep "promiscuous|forward"
>>> [ 85.919382] device tap113i0 entered promiscuous mode
>>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>>> [ 86.263292] device tap113i1 entered promiscuous mode
>>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>>> [ 86.314153] device vmbr1 entered promiscuous mode
>>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>
>>> Now it looks like this:
>>> # ip a l|grep PROMISC
>>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>> htb master vmbr0 state UNKNOWN qlen 500
>>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>> htb master vmbr1v3000 state UNKNOWN qlen 500
>>
>> eth* should get into forwarding mode cause bond0 is a port of the bridge
>> and should propagate its state towards its slaves. Something is wrong
>> here.
>>
>> Maybe we're looking at the wrong direction - and the promisc for the
>> ethernet drivers got broken?
>
> I was able to duplicate Stefans results only when I turn off the link to
> the underlying devices when building the bridge. When the link is off,
> the rx_flags do not propagate down to the lower level devices.
>
> What about something like this (completely untested)
>
> diff --git a/drivers/net/bonding/bond_main.c
> b/drivers/net/bonding/bond_main.c
> index 4dd5ee2..3051744 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2863,6 +2863,17 @@ static int bond_slave_netdev_event(unsigned long
> event,
> bond_release(bond_dev, slave_dev);
> break;
> case NETDEV_UP:
> + /* If the bond was set to primisc, but slave has not due to
> + * slave being down when the command was issued, sync the
> + * state when the slave comes up.
> + */
> + if (bond_dev->flags & IFF_PROMISC &&
> !slave_dev->promiscuity) {
> + if (!USES_PRIMARY(bond))
> + dev_set_promiscuity(slave_dev, 1);
> + else if (slave == bond->curr_active_slave)
> + dev_set_promiscuity(slave_dev, 1);
> + }
> +
> case NETDEV_CHANGE:
> old_speed = slave->speed;
> old_duplex = slave->duplex;
>
> -vlad
>>
I tried that one but it still looks like this:
[ 173.266915] device tap113i1 entered promiscuous mode
[ 173.305617] 8021q: adding VLAN 3000 to HW filter on device bond1.3000
[ 173.315881] device bond1.3000 entered promiscuous mode
[ 173.315926] vmbr1v3000: port 1(bond1.3000) entered forwarding state
[ 173.315929] vmbr1v3000: port 1(bond1.3000) entered forwarding state
[ 173.319844] vmbr1v3000: port 2(tap113i1) entered forwarding state
[ 173.319847] vmbr1v3000: port 2(tap113i1) entered forwarding state
[ 188.344076] vmbr1v3000: port 1(bond1.3000) entered forwarding state
# ip a l|grep PROMISC
13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
htb master vmbr0 state UNKNOWN qlen 500
14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
htb master vmbr1v3000 state UNKNOWN qlen 500
>> What ethernet cards/driver do you use for eth*?
>>
>>>
>>> Greets,
>>> Stefan
>>>
>>>
>>> Main question is - is this one correct:
>>
>> Both are correct. Here's my setup (sorry for stretching):
>>
>> +---------------+ +------------+
>> +-------------+ +---------+ +------+
>> | bond1 | | | | bridge0
>> | | | | |
>> | 192.168.2.1 | master | bridge0.15 | neighbour | 192.168.3.1 |
>> master | bond0 | master | eth2 |
>> | | --------> | | ------------ | 192.168.4.1 |
>> --------> | | --------> | |
>> +---------------+ +------------+
>> +-------------+ +---------+ +------+
>>
>> |
>>
>> | master
>>
>> v
>> +---------------+
>> +---------+
>> | dummy0
>> | |
>> eth0 |
>> +---------------+
>> +---------+
>>
>> (disregard that dummy0).
>>
>> All 192.168.X.1 ips are pingable (via the correct vlans) on both
>> net-next and stable 3.10.19.
>>
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 \ vmbr1.3000
>>>>>>> \ ---- tap114i1
>>>
>>> <= does not work at all
>>>
>>> or this one?:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>> \ ---- tap114i1
>>>
>>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>>
>>>> -vlad
>>>>
>>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben
>>>>> not
>>>>> with promisc mode. The one below works fine if i set eth2 and eth3
>>>>> into
>>>>> promisc mode.
>>>>>
>>>>> Stefan
>>>>>
>>>>>>> I also tried this one without success:
>>>>>>> eth2
>>>>>>> \
>>>>>>> -- bond1 -- vmbr1
>>>>>>> / \
>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>> \ ---- tap114i1
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> Greets,
>>>>>>> Stefan
>>>>>>> --
>>>>>>> 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
>>>>> --
>>>>> 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
* Fwd: Cross thread shutdown of connected UDP socket, unexpected recvfrom behavior
From: mpb @ 2013-11-13 20:07 UTC (permalink / raw)
To: netdev
In-Reply-To: <CAApbN=Jm=zuDaVYALmGg5z_QfJJ3q13e9tshb7OpAtPQ8Ema1w@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1910 bytes --]
Hi netdev@vger.kernel.org,
Someone on LKML suggested that I forward this issue to the netdev list.
In a nutshell, recvfrom returns ambiguous results, and it is
impossible to distinguish between these two situations:
a) calling recvfrom and receiving a zero length UDP message
b) calling recvfrom and blocking until the socket is shutdown (locally)
More details below.
Thanks!
-mpb
---------- Forwarded message ----------
From: mpb <mpb.mail@gmail.com>
Date: Sat, Nov 9, 2013 at 5:32 PM
Subject: Cross thread shutdown of connected UDP socket, unexpected
recvfrom behavior
To: linux-kernel@vger.kernel.org
Hi LKML,
I have a C/pthreads program with two threads ("main" and "thread").
"thread" calls recvfrom on a connected UDP socket and blocks, waiting
for input. "main" then calls shutdown SHUT_RD on the socket. In
"thread", recvfrom returns, apparently successfully, returning a zero
length string, and setting src_addr to a bogus(?) address family,
port, and source address.
Is the above the correct behavior for recvfrom?
Here is output from my program. "main" sends "hello\0", then "" (the
empty string), then calls shutdown. "thread" receives "hello\0", "",
and then finally receives an empty string that was never sent!
thread recvfrom: Success
rv 6 addrlen 16 fam 2 port 8000 addr 100007f
thread recvfrom: Success
rv 0 addrlen 16 fam 2 port 8000 addr 100007f
main shutdown: Success
rv 0
thread recvfrom: Success
rv 0 addrlen 16 fam 59060 port 44237 addr deaadef0
The source code (2k) for the porgram is attached. I'm running Ubuntu
13.04, kernel 3.8.0-19-generic #30-Ubuntu, on 32-bit i686.
For reference, this June 2000 LKML thread discusses calling close in a
similar situation.
http://www.gossamer-threads.com/lists/linux/kernel/144379
Please CC me if you have questions, otherwise I'll try to watch for
answers in the list archives.
Thanks!
-mpb
[-- Attachment #2: test.c --]
[-- Type: text/x-csrc, Size: 1861 bytes --]
#include <netinet/in.h>
#include <pthread.h>
#include <stdio.h>
#include <sys/socket.h>
#include <sys/types.h>
struct sockaddr_in addr_0, addr_1, addr_2 ;
int fd0, fd1, rv0, rv1, addrlen;
char buf_0[1024], buf_1[1024];
pthread_t thread;
void* thread_rv;
void* thread_main (void* arg) {
int i; for ( i=0; i<3; i++ ) {
addr_2.sin_family = 0;
addr_2.sin_port = 0;
addr_2.sin_addr.s_addr = 0;
addrlen = sizeof addr_2;
rv1 = recvfrom ( fd1, buf_0, sizeof buf_0, 0,
(struct sockaddr*) &addr_2, &addrlen );
int fam = addr_2.sin_family;
int port = addr_2.sin_port;
int addr = addr_2.sin_addr.s_addr;
printf ("\n");
perror ("thread recvfrom");
printf ( " rv %d addrlen %d fam %d port %d addr %x\n",
rv1, addrlen, fam, ntohs (port), addr );
;;; }
return NULL; }
void main_main () {
send ( fd0, "hello", 6, 0);
send ( fd0, "", 0, 0);
usleep (100000);
rv0 = shutdown ( fd1, SHUT_RD );
printf ("\n");
perror ("main shutdown");
printf (" %d\n", rv0)
;;; }
int main () {
addr_0.sin_family = AF_INET;
addr_0.sin_port = htons (8000);
inet_pton ( AF_INET, "127.0.0.1", &addr_0.sin_addr );
addr_1.sin_family = AF_INET;
addr_1.sin_port = htons (8001);
inet_pton ( AF_INET, "127.0.0.1", &addr_1.sin_addr );
fd0 = socket ( AF_INET, SOCK_DGRAM, 0 );
bind ( fd0, (struct sockaddr*) &addr_0, sizeof addr_0 );
connect ( fd0, (struct sockaddr*) &addr_1, sizeof addr_1 );
fd1 = socket ( AF_INET, SOCK_DGRAM, 0 );
bind ( fd1, (struct sockaddr*) &addr_1, sizeof addr_1 );
connect ( fd1, (struct sockaddr*) &addr_0, sizeof addr_0 );
pthread_create ( &thread, NULL, thread_main, NULL );
main_main ();
pthread_join ( thread, &thread_rv );
return 0; }
^ permalink raw reply
* Re: __refrigerator() && saved task->state
From: Oleg Nesterov @ 2013-11-13 19:40 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Tejun Heo, David Rientjes, David Laight, Geert Uytterhoeven,
Ingo Molnar, netdev, linux-kernel
In-Reply-To: <20131113191449.GF16796@laptop.programming.kicks-ass.net>
On 11/13, Peter Zijlstra wrote:
>
> On Wed, Nov 13, 2013 at 08:11:43PM +0100, Oleg Nesterov wrote:
> > At first glance it would be better to simply kill this logic? If
> > it was called with ->state != 0, the caller is going to schedule()
> > and it probably executes the wait_event-like code, in this case
> > it would me more safe to pretend the task got a spurious wakeup?
>
> Note that in general the kernel cannot deal with spurious wakeups :/
>
> Most proper locks and wait primitives can, but there's enough open-coded
> crap out there that can not.
Oh yes, I understand.
My point is, "restore the old state" in this case looks worse simply
because you miss any wakeup in between which was going to clear that
state. And afaics only kthreads can call __refrigerator() in !RUNNING.
But let me repeat, I am almost sure I missed something else.
Oleg.
^ permalink raw reply
* Re: __refrigerator() && saved task->state
From: Peter Zijlstra @ 2013-11-13 19:14 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Tejun Heo, David Rientjes, David Laight, Geert Uytterhoeven,
Ingo Molnar, netdev, linux-kernel
In-Reply-To: <20131113191143.GA24005@redhat.com>
On Wed, Nov 13, 2013 at 08:11:43PM +0100, Oleg Nesterov wrote:
> At first glance it would be better to simply kill this logic? If
> it was called with ->state != 0, the caller is going to schedule()
> and it probably executes the wait_event-like code, in this case
> it would me more safe to pretend the task got a spurious wakeup?
Note that in general the kernel cannot deal with spurious wakeups :/
Most proper locks and wait primitives can, but there's enough open-coded
crap out there that can not.
I tried..
^ permalink raw reply
* Re: [PATCH 1/1] net: sctp: bug fixing when sctp path recovers
From: Daniel Borkmann @ 2013-11-13 19:10 UTC (permalink / raw)
To: Chang
Cc: Vlad Yasevich, nhorman, davem, linux-sctp, netdev, linux-kernel,
dreibh
In-Reply-To: <5283CDD1.3090309@gmail.com>
On 11/13/2013 08:06 PM, Chang wrote:
> On 11/13/2013 09:44 AM, Daniel Borkmann wrote:
>> On 11/13/2013 03:54 AM, Chang wrote:
>>> On 11/13/2013 03:37 AM, Vlad Yasevich wrote:
>>>> On 11/12/2013 08:34 PM, Chang Xiangzhong wrote:
>>>>> Look for the __two__ most recently used path/transport and set to active_path
>>>>> and retran_path respectively
>>
>> Please also for the log, elaborate a bit more, explaining what currently
>> happens, and what the effects of this bug are, so that later when people
>> are looking through the Git log they can easily get what problem you are
>> trying to fix; and if possible, add:
>>
>> Fixes: <12 digits SHA1> ("<commit title>")
>>
> Yeah, sure, I'll elaborate that more specifically.
Thanks !
> I assume the 12-digit SHA1 is the revision number. But may I ask where and how shall I add the tag "Fixes" tag? The revision number is generated after "git commit", how can I know that in advance?
Nope, it's the affected commit id from the current git log that
your patch fixes.
Have a look for example at commit:
98bbc06aabac5a2 ("net: x86: bpf: don't forget to free sk_filter (v2)")
^ permalink raw reply
* __refrigerator() && saved task->state
From: Oleg Nesterov @ 2013-11-13 19:11 UTC (permalink / raw)
To: Tejun Heo
Cc: Peter Zijlstra, David Rientjes, David Laight, Geert Uytterhoeven,
Ingo Molnar, netdev, linux-kernel
In-Reply-To: <20131113170724.GA17739@redhat.com>
Sorry for noise, but I am totally confused.
Could you please remind why __refrigerator() saves/restores
task->state?
I can see only one reason: set_freezable() kernel threads which
check kthread_should_stop() and do try_to_freeze() by hand.
But does this save/restore actually help?
For example kauditd_thread() looks obviously racy exactly because
try_to_freeze() can return in TASK_INTERRUPTIBLE state after
wake_up(kauditd_wait) was already called, we can miss an event.
At first glance it would be better to simply kill this logic? If
it was called with ->state != 0, the caller is going to schedule()
and it probably executes the wait_event-like code, in this case
it would me more safe to pretend the task got a spurious wakeup?
(as for kauditd_thread() in particular, it looks wrong in any
case, even kthread_should_stop() check doesn't look right, it
needs kthread_freezable_should_stop() afaics).
But I guess I missed something else...
Oleg.
^ permalink raw reply
* Re: [PATCH 1/1] net: sctp: bug fixing when sctp path recovers
From: Chang @ 2013-11-13 19:06 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Vlad Yasevich, nhorman, davem, linux-sctp, netdev, linux-kernel,
dreibh
In-Reply-To: <52833BD3.4040003@redhat.com>
On 11/13/2013 09:44 AM, Daniel Borkmann wrote:
> On 11/13/2013 03:54 AM, Chang wrote:
>> On 11/13/2013 03:37 AM, Vlad Yasevich wrote:
>>> On 11/12/2013 08:34 PM, Chang Xiangzhong wrote:
>>>> Look for the __two__ most recently used path/transport and set to
>>>> active_path
>>>> and retran_path respectively
>
> Please also for the log, elaborate a bit more, explaining what currently
> happens, and what the effects of this bug are, so that later when people
> are looking through the Git log they can easily get what problem you are
> trying to fix; and if possible, add:
>
> Fixes: <12 digits SHA1> ("<commit title>")
>
Yeah, sure, I'll elaborate that more specifically.
I assume the 12-digit SHA1 is the revision number. But may I ask where
and how shall I add the tag "Fixes" tag? The revision number is
generated after "git commit", how can I know that in advance?
Best Regards!
>>>> Signed-off-by: changxiangzhong@gmail.com
>>>> ---
>>>> net/sctp/associola.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/net/sctp/associola.c b/net/sctp/associola.c
>>>> index ab67efc..070011a 100644
>>>> --- a/net/sctp/associola.c
>>>> +++ b/net/sctp/associola.c
>>>> @@ -913,11 +913,15 @@ void sctp_assoc_control_transport(struct
>>>> sctp_association *asoc,
>>>> if (!first || t->last_time_heard > first->last_time_heard) {
>>>> second = first;
>>>> first = t;
>>>> + continue;
>>>> }
>>>> if (!second || t->last_time_heard > second->last_time_heard)
>>>> second = t;
>>>
>>> You might as well remove this bit and then you don't need a continue.
>> I don't think we could remove this bit. My understanding of these
>> algorithms are to find the 1st recently used path and the 2nd,
>> assigning to active_path and retran_path respectively. If we remove
>> the looking-for-second block, how are we suppose to find the 2nd?
>> I think we can remove the continue and use else-if in the
>> 2nd-assignment-block.
>>>
>>>> }
>>>>
>>>> + if (!second)
>>>> + second = first;
>>>> +
>>>
>>> This needs to move down 1 more block. Set the second transport
>>> after we
>>> check to see if the primary is back up and we need to go back to
>>> using it.
>>>
>>> -vlad
>>>
>> I agree with this change
>>>> /* RFC 2960 6.4 Multi-Homed SCTP Endpoints
>>>> *
>>>> * By default, an endpoint should always transmit to the
>>>>
>>>
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-sctp" 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] mac80211: minstrels: spare numerous useless calls to get_random_bytes
From: Johannes Berg @ 2013-11-13 18:34 UTC (permalink / raw)
To: Karl Beldan
Cc: Felix Fietkau, linux-wireless, Hannes Frederic Sowa, netdev,
Karl Beldan
In-Reply-To: <1384336459-5322-1-git-send-email-karl.beldan@gmail.com>
On Wed, 2013-11-13 at 10:54 +0100, Karl Beldan wrote:
> From: Karl Beldan <karl.beldan@rivierawaves.com>
>
> ATM, only the first array value returned by get_random_bytes is used.
> This change moves the call to get_random_bytes from the nested loop it
> is in to its parent.
> While at it, replace get_random_bytes with prandom_bytes since PRNs are
> way enough for the selection process.
> After this, minstrel_ht reclaims 80 PR-bytes instead of 640 R-bytes.
Applied.
johannes
^ permalink raw reply
* Re: [PATCH v2] mac80211: add assoc beacon timeout logic
From: Johannes Berg @ 2013-11-13 18:30 UTC (permalink / raw)
To: Felipe Contreras
Cc: linux-wireless Mailing List, netdev, John W. Linville,
David S. Miller
In-Reply-To: <CAMP44s1yJMd+9G5yu988GqafRYnAteN-y-WWDe-rJh_qQpSPkQ@mail.gmail.com>
On Mon, 2013-11-11 at 12:06 -0600, Felipe Contreras wrote:
> > The not receiving part is a bug. I think you're probably receiving
> > beacons once associated though?
>
> Nope. Never.
That's odd, firmware bug? But it's still odd since windows works?
> Moreover, if continuing the association without beacons has a legal a
> problem, that problem would exist for drivers that don't have the
> IEEE80211_HW_NEED_DTIM_BEFORE_ASSOC flag, wouldn't it? How exactly
> would trying to associate with need_beacon break the law, but not if
> !need_beacon?
Well, it's actually somewhat unlikely you'd break the law, although
possible, since eventually you'd get disconnected from the AP anyway?
In any case - your argumentation above isn't true because every device
listens for beacons, and if none are received then it should eventually
disconnect. That should be true even with your hack here.
In any case, I'm not really comfortable connecting to an AP that we
never ever receive beacons from. I can try to investigate why this
doesn't happen on windows and what we should be doing though.
johannes
^ permalink raw reply
* [PATCH RFC] net: error on trying to add a duplicate ipip tunnel
From: Andreas Henriksson @ 2013-11-13 18:21 UTC (permalink / raw)
To: netdev
After patch:
$ sudo ip tunnel add test1 mode ipip remote 1.2.3.4
$ sudo ip tunnel add test2 mode ipip remote 1.2.3.4
add tunnel "tunl0" failed: File exists
Before the patch, there would be no error and
"test2" (silently) not added.
Originally reported at http://bugs.debian.org/508450
The originally reported problem with sit tunnel seems
to have been resolved since then:
$ sudo ip tun add test3 mode sit
add tunnel "sit0" failed: No buffer space available
The problem still exists for (atleast) ipip tunnels though.
Reported-by: martin f krafft <madduck@debian.org>
Signed-off-by: Andreas Henriksson <andreas@fatal.se>
---
net/ipv4/ip_tunnel.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
Maybe also sit tunnels should explicitly
use EEXISTS instead of ENOBUFS?
Maybe ipip tunnel code should be refactored
to be similar to sit tunnel addition code?
Which other tunnel types should be checked
for how they behave in similar situations?
Other comments?
diff --git a/net/ipv4/ip_tunnel.c b/net/ipv4/ip_tunnel.c
index 63a6d6d..1dc4e41 100644
--- a/net/ipv4/ip_tunnel.c
+++ b/net/ipv4/ip_tunnel.c
@@ -732,8 +732,14 @@ int ip_tunnel_ioctl(struct net_device *dev, struct ip_tunnel_parm *p, int cmd)
t = ip_tunnel_find(itn, p, itn->fb_tunnel_dev->type);
- if (!t && (cmd == SIOCADDTUNNEL))
- t = ip_tunnel_create(net, itn, p);
+ if (cmd == SIOCADDTUNNEL) {
+ if (!t) {
+ t = ip_tunnel_create(net, itn, p);
+ } else {
+ err = -EEXIST;
+ break;
+ }
+ }
if (dev != itn->fb_tunnel_dev && cmd == SIOCCHGTUNNEL) {
if (t != NULL) {
--
1.8.4.3
^ permalink raw reply related
* Re: oom-kill && frozen()
From: Oleg Nesterov @ 2013-11-13 18:15 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Tejun Heo, David Rientjes, David Laight, Geert Uytterhoeven,
Ingo Molnar, netdev, linux-kernel
In-Reply-To: <20131113174220.GQ21461@twins.programming.kicks-ass.net>
On 11/13, Peter Zijlstra wrote:
>
> On Wed, Nov 13, 2013 at 06:07:24PM +0100, Oleg Nesterov wrote:
> > 4. Finally, change try_to_wake_up() path to do
> >
> > - p->state = TASK_WAKING;
> > + p->state &= ~state;
> > + if (p->state & ~(TASK_DEAD | TASK_WAKEKILL | TASK_PARKED))
> > + return;
> > + else
> > + p->state = TASK_WAKING;
> >
> > IOW, if the task sleeps in, say, TASK_INTERRUPTIBLE | __TASK_FROZEN
> > then it need both try_to_wake_up(TASK_INTERRUPTIBLE) and
> > try_to_wake_up(__TASK_FROZEN) to wake up.
>
> > Tejun, Peter, do you think this makes any sense? I am just curious, but
> > "selective wakeup" looks potentially useful.
>
> I've never looked at any of this freeze stuff, so I cannot comment too
> much. However we should be very careful not to add too much to relative
> hot paths for the relative rare case of freezing stuff.
Yes, yes, sure.
Plus my description was confusing and incomplete (and I am sure I missed
something more). I forgot to mention that freeze_task() should also add
the FROZEN state if it sees FROZEN qualifier, this needs more changes.
So please forget. May be I'll _try_ to make something working (at least
for discussion), but I am not sure.
Oleg.
^ permalink raw reply
* Re: [patch] net: mv643xx_eth: potential NULL dereference in probe()
From: Jason Gunthorpe @ 2013-11-13 18:00 UTC (permalink / raw)
To: Sebastian Hesselbarth; +Cc: Dan Carpenter, netdev, kernel-janitors
In-Reply-To: <52833734.5000507@gmail.com>
On Wed, Nov 13, 2013 at 09:24:20AM +0100, Sebastian Hesselbarth wrote:
> On 11/13/13 08:52, Dan Carpenter wrote:
> >We assume that "mp->phy" can be NULL a couple lines before the
> >dereference.
> >
> >Fixes: 1cce16d37d0f ('net: mv643xx_eth: Add missing phy_addr_set in DT mode')
> >Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> Acked-by: Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>
Acked-by: Jason Gunthorpe <jgunthorpe@obsidianresearch.com>
My bad, I missed this possibility.
Jason
^ permalink raw reply
* Re: [PATCH 1/3] can: mcp251x: Convert driver to use common clk framework
From: Dmitry Eremin-Solenikov @ 2013-11-13 17:59 UTC (permalink / raw)
To: netdev; +Cc: linux-can, linux-arm-kernel
In-Reply-To: <1384332144-15211-1-git-send-email-shc_work@mail.ru>
On Wed, 13 Nov 2013 12:42:24 +0400, Alexander Shiyan wrote:
> This patch converts mcp251x driver to using common clk framework for
> getting input frequency. The change was made for the preparation of the
> driver work with the devicetree. As a result private header for the
> driver is eliminated.
> Additionally, all existing users of the driver was converted to use this
> feature.
PXA does not employ COMMON_CLK yet (WIP locally, but it is not finished
yet), so clk_register_fixed() will probably fail.
--
With best wishes
Dmitry
^ permalink raw reply
* Re: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Vlad Yasevich @ 2013-11-13 17:49 UTC (permalink / raw)
To: Stefan Priebe - Profihost AG; +Cc: Veaceslav Falico, Linux Netdev List
In-Reply-To: <96EAD9BC-FA9E-4EAC-BDC5-C3DA6DE9EF48@profihost.ag>
On 11/13/2013 12:46 PM, Stefan Priebe - Profihost AG wrote:
>
>> Am 13.11.2013 um 18:37 schrieb Vlad Yasevich <vyasevich@gmail.com>:
>>
>>> On 11/13/2013 12:22 PM, Stefan Priebe - Profihost AG wrote:
>>>
>>>>> Am 13.11.2013 um 17:44 schrieb Vlad Yasevich <vyasevich@gmail.com>:
>>>>>
>>>>> On 11/13/2013 10:17 AM, Stefan Priebe - Profihost AG wrote:
>>>>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>>>>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>>>>> Hi Falico,
>>>>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>>>>>> wrote:
>>>>>>>>> Hello,
>>>>>>>>>
>>>>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>>>>> never
>>>>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>>>>
>>>>>>>>> I've this:
>>>>>>>>>
>>>>>>>>> eth2
>>>>>>>>> \
>>>>>>>>> -- bond1 -- vmbr1
>>>>>>>>> /
>>>>>>>>> eth3
>>>>>>>>>
>>>>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>>>>> bonding and using a bridge.
>>>>>>>>>
>>>>>>>>> I the past i had this:
>>>>>>>>> eth2
>>>>>>>>> \
>>>>>>>>> -- bond1 -- vmbr1
>>>>>>>>> / \
>>>>>>>>> eth3 \ vmbr1.3000
>>>>>>>>> \ ---- tap114i1
>>>>>>>>>
>>>>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>>>>>> is bad!
>>>>>>>>
>>>>>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>>>>> patchsets that will never hit 3.10 stable.
>>>>>>>> Also, the bonding configuration would be welcome.
>>>>>>>
>>>>>>> Debian Bonding konfiguration looks like this:
>>>>>>> auto bond1
>>>>>>> iface bond1 inet manual
>>>>>>> slaves eth2 eth3
>>>>>>> bond-mode 802.3ad
>>>>>>> bond_miimon 100
>>>>>>> bond_updelay 200
>>>>>>> bond_downdelay 0
>>>>>>>
>>>>>>> This should be miimon using lacp and not arp isn't it?
>>>>>>> Anything more needed?
>>>>>>
>>>>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>>>>> bond should place all of its ports into promiscuous mode. Do you see
>>>>>> the the kernel messages that say that?
>>>>>
>>>>> No it does not - i only see:
>>>>> # dmesg -c|egrep "promiscuous|forward"
>>>>> [ 5.445161] device bond0 entered promiscuous mode
>>>>> [ 7.670701] device bond1 entered promiscuous mode
>>>>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>>>>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>>>>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>>>>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>>>>
>>>> So this can happen if bond interfaces are down at the time of joining
>>>> the bridge.
>>>
>>> About which of my tried configuration do you speak? The bond itself is always up it's the internet connection. Or do you mean the vlan bond?
>>
>> The issue is in the formation for vmbr0 and vmbr1. When bond0 and
>> bond1 enter promisc mode as part of becoming a bridge port, they should
>> put some subset of their ports into promisc mode as well (in 802.3ad
>> mode it should happen on all bond slaves).
>>
>> What I see in my config is something like:
>>
>> [ 2640.725700] device bond0 entered promiscuous mode
>> [ 2640.726623] device eth0 entered promiscuous mode
>> [ 2640.727655] br0: port 1(bond0) entered forwarding state
>> [ 2640.728718] br0: port 1(bond0) entered forwarding state
>>
>> This is my quick configuration:
>>
>> br0 ----> bond0 -------> eth0
>>
>> I wanted to make sure in this case eth0 is properly configured in
>> promisc mode.
>>
>> If eth0 is somehow DOWN, when bond0 is added to the bridge, then it
>> will never get promisc mode set (at least in my quick test).
>> I am not sure if this what's happening in your case, but that's
>> the only explanation I can come up with for the above log you provided.
>
> But this one works fine it only does not work with vlans.
>
The fact that it does not make lower level devices enter promisc mode
indicates that it may not be working correctly.
> Is there an easy way to check where those packages get dropped.
>
dropwatch should be able to help.
-vlad
> Stefan
>
>>
>>
>> -vlad
>>
>>>
>>>> The promisc flag propagation to lower interfaces only happens when those interfaces are up. So if for whatever reason,
>>>> the lower interfaces happen to be down during the crating of this
>>>> hierarchy, the promiscuity flag may not get set on the lower level port
>>>> device an we may end up in this situation.
>>>>
>>>> -vlad
>>>>
>>>>> Now adding variant 1:
>>>>> # dmesg -c|egrep "promiscuous|forward"
>>>>> [ 85.919382] device tap113i0 entered promiscuous mode
>>>>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>>>>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>>>>> [ 86.263292] device tap113i1 entered promiscuous mode
>>>>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>>>>> [ 86.314153] device vmbr1 entered promiscuous mode
>>>>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>>>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>>>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>>>
>>>>> Now it looks like this:
>>>>> # ip a l|grep PROMISC
>>>>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>>>> htb master vmbr0 state UNKNOWN qlen 500
>>>>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>>>> htb master vmbr1v3000 state UNKNOWN qlen 500
>>>>>
>>>>> Greets,
>>>>> Stefan
>>>>>
>>>>>
>>>>> Main question is - is this one correct:
>>>>>>>>> eth2
>>>>>>>>> \
>>>>>>>>> -- bond1 -- vmbr1
>>>>>>>>> / \
>>>>>>>>> eth3 \ vmbr1.3000
>>>>>>>>> \ ---- tap114i1
>>>>>
>>>>> <= does not work at all
>>>>>
>>>>> or this one?:
>>>>>>>>> eth2
>>>>>>>>> \
>>>>>>>>> -- bond1 -- vmbr1
>>>>>>>>> / \
>>>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>>>> \ ---- tap114i1
>>>>>
>>>>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>>>>
>>>>>> -vlad
>>>>>>
>>>>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>>>>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>>>>>> promisc mode.
>>>>>>>
>>>>>>> Stefan
>>>>>>>
>>>>>>>>> I also tried this one without success:
>>>>>>>>> eth2
>>>>>>>>> \
>>>>>>>>> -- bond1 -- vmbr1
>>>>>>>>> / \
>>>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>>>> \ ---- tap114i1
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Greets,
>>>>>>>>> Stefan
>>>>>>>>> --
>>>>>>>>> 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
>>>>>>> --
>>>>>>> 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: how to mix bridges and bonding inc. vlans correctly on Kernel > 3.10
From: Stefan Priebe - Profihost AG @ 2013-11-13 17:46 UTC (permalink / raw)
To: Vlad Yasevich; +Cc: Veaceslav Falico, Linux Netdev List
In-Reply-To: <5283B8EF.8060902@gmail.com>
> Am 13.11.2013 um 18:37 schrieb Vlad Yasevich <vyasevich@gmail.com>:
>
>> On 11/13/2013 12:22 PM, Stefan Priebe - Profihost AG wrote:
>>
>>>> Am 13.11.2013 um 17:44 schrieb Vlad Yasevich <vyasevich@gmail.com>:
>>>>
>>>> On 11/13/2013 10:17 AM, Stefan Priebe - Profihost AG wrote:
>>>> Am 13.11.2013 16:05, schrieb Vlad Yasevich:
>>>>>> On 11/13/2013 09:20 AM, Stefan Priebe - Profihost AG wrote:
>>>>>> Hi Falico,
>>>>>> Am 13.11.2013 15:12, schrieb Veaceslav Falico:
>>>>>>> On Wed, Nov 13, 2013 at 02:58:40PM +0100, Stefan Priebe - Profihost AG
>>>>>>> wrote:
>>>>>>>> Hello,
>>>>>>>>
>>>>>>>> while my vlans, bridging and bonding stuff was working until 3.9 i
>>>>>>>> never
>>>>>>>> thought about how it is right. So maybe i was always wrong.
>>>>>>>>
>>>>>>>> I've this:
>>>>>>>>
>>>>>>>> eth2
>>>>>>>> \
>>>>>>>> -- bond1 -- vmbr1
>>>>>>>> /
>>>>>>>> eth3
>>>>>>>>
>>>>>>>> This works fine and as expected now i want to have a vlan using the
>>>>>>>> bonding and using a bridge.
>>>>>>>>
>>>>>>>> I the past i had this:
>>>>>>>> eth2
>>>>>>>> \
>>>>>>>> -- bond1 -- vmbr1
>>>>>>>> / \
>>>>>>>> eth3 \ vmbr1.3000
>>>>>>>> \ ---- tap114i1
>>>>>>>>
>>>>>>>> This was working fine until 3.9.X since 3.10. Right now using 3.10 i
>>>>>>>> need to put eth2 and eth3 into promisc mode to get it working ;-( this
>>>>>>>> is bad!
>>>>>>>
>>>>>>> As a guess - do you use arp monitoring for bonding? Try using miimon -
>>>>>>> there were some issues with it in 3.10, which were fixed by some huge
>>>>>>> patchsets that will never hit 3.10 stable.
>>>>>>> Also, the bonding configuration would be welcome.
>>>>>>
>>>>>> Debian Bonding konfiguration looks like this:
>>>>>> auto bond1
>>>>>> iface bond1 inet manual
>>>>>> slaves eth2 eth3
>>>>>> bond-mode 802.3ad
>>>>>> bond_miimon 100
>>>>>> bond_updelay 200
>>>>>> bond_downdelay 0
>>>>>>
>>>>>> This should be miimon using lacp and not arp isn't it?
>>>>>> Anything more needed?
>>>>>
>>>>> Hmm.. With 802.3ad mode, when the bond is a port on the bridge, the
>>>>> bond should place all of its ports into promiscuous mode. Do you see
>>>>> the the kernel messages that say that?
>>>>
>>>> No it does not - i only see:
>>>> # dmesg -c|egrep "promiscuous|forward"
>>>> [ 5.445161] device bond0 entered promiscuous mode
>>>> [ 7.670701] device bond1 entered promiscuous mode
>>>> [ 7.845472] vmbr0: port 1(bond0) entered forwarding state
>>>> [ 7.845474] vmbr0: port 1(bond0) entered forwarding state
>>>> [ 8.269769] vmbr1: port 1(bond1) entered forwarding state
>>>> [ 8.269771] vmbr1: port 1(bond1) entered forwarding state
>>>
>>> So this can happen if bond interfaces are down at the time of joining
>>> the bridge.
>>
>> About which of my tried configuration do you speak? The bond itself is always up it's the internet connection. Or do you mean the vlan bond?
>
> The issue is in the formation for vmbr0 and vmbr1. When bond0 and
> bond1 enter promisc mode as part of becoming a bridge port, they should
> put some subset of their ports into promisc mode as well (in 802.3ad
> mode it should happen on all bond slaves).
>
> What I see in my config is something like:
>
> [ 2640.725700] device bond0 entered promiscuous mode
> [ 2640.726623] device eth0 entered promiscuous mode
> [ 2640.727655] br0: port 1(bond0) entered forwarding state
> [ 2640.728718] br0: port 1(bond0) entered forwarding state
>
> This is my quick configuration:
>
> br0 ----> bond0 -------> eth0
>
> I wanted to make sure in this case eth0 is properly configured in
> promisc mode.
>
> If eth0 is somehow DOWN, when bond0 is added to the bridge, then it
> will never get promisc mode set (at least in my quick test).
> I am not sure if this what's happening in your case, but that's
> the only explanation I can come up with for the above log you provided.
But this one works fine it only does not work with vlans.
Is there an easy way to check where those packages get dropped.
Stefan
>
>
> -vlad
>
>>
>>> The promisc flag propagation to lower interfaces only happens when those interfaces are up. So if for whatever reason,
>>> the lower interfaces happen to be down during the crating of this
>>> hierarchy, the promiscuity flag may not get set on the lower level port
>>> device an we may end up in this situation.
>>>
>>> -vlad
>>>
>>>> Now adding variant 1:
>>>> # dmesg -c|egrep "promiscuous|forward"
>>>> [ 85.919382] device tap113i0 entered promiscuous mode
>>>> [ 85.965018] vmbr0: port 2(tap113i0) entered forwarding state
>>>> [ 85.965023] vmbr0: port 2(tap113i0) entered forwarding state
>>>> [ 86.263292] device tap113i1 entered promiscuous mode
>>>> [ 86.314151] device vmbr1.3000 entered promiscuous mode
>>>> [ 86.314153] device vmbr1 entered promiscuous mode
>>>> [ 86.314192] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>> [ 86.314196] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>> [ 86.318116] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>>> [ 86.318120] vmbr1v3000: port 2(tap113i1) entered forwarding state
>>>> [ 101.382129] vmbr1v3000: port 1(vmbr1.3000) entered forwarding state
>>>>
>>>> Now it looks like this:
>>>> # ip a l|grep PROMISC
>>>> 13: tap113i0: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>>> htb master vmbr0 state UNKNOWN qlen 500
>>>> 14: tap113i1: <BROADCAST,MULTICAST,PROMISC,UP,LOWER_UP> mtu 1500 qdisc
>>>> htb master vmbr1v3000 state UNKNOWN qlen 500
>>>>
>>>> Greets,
>>>> Stefan
>>>>
>>>>
>>>> Main question is - is this one correct:
>>>>>>>> eth2
>>>>>>>> \
>>>>>>>> -- bond1 -- vmbr1
>>>>>>>> / \
>>>>>>>> eth3 \ vmbr1.3000
>>>>>>>> \ ---- tap114i1
>>>>
>>>> <= does not work at all
>>>>
>>>> or this one?:
>>>>>>>> eth2
>>>>>>>> \
>>>>>>>> -- bond1 -- vmbr1
>>>>>>>> / \
>>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>>> \ ---- tap114i1
>>>>
>>>> <= works if i manually put eth2 and eth3 into promiscous mode.
>>>>
>>>>> -vlad
>>>>>
>>>>>> One thing i forgot the one with vmbr1.3000 does not work at all eben not
>>>>>> with promisc mode. The one below works fine if i set eth2 and eth3 into
>>>>>> promisc mode.
>>>>>>
>>>>>> Stefan
>>>>>>
>>>>>>>> I also tried this one without success:
>>>>>>>> eth2
>>>>>>>> \
>>>>>>>> -- bond1 -- vmbr1
>>>>>>>> / \
>>>>>>>> eth3 ----- bond1.3000 --- vmbr1v3000
>>>>>>>> \ ---- tap114i1
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> Greets,
>>>>>>>> Stefan
>>>>>>>> --
>>>>>>>> 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
>>>>>> --
>>>>>> 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 1/4] virtio-net: mergeable buffer size should include virtio-net header
From: Michael S. Tsirkin @ 2013-11-13 17:43 UTC (permalink / raw)
To: Michael Dalton
Cc: netdev, Daniel Borkmann, virtualization, Eric Dumazet,
David S. Miller
In-Reply-To: <1384294885-6444-1-git-send-email-mwdalton@google.com>
On Tue, Nov 12, 2013 at 02:21:22PM -0800, Michael Dalton wrote:
> Commit 2613af0ed18a ("virtio_net: migrate mergeable rx buffers to page
> frag allocators") changed the mergeable receive buffer size from PAGE_SIZE
> to MTU-size. However, the merge buffer size does not take into account the
> size of the virtio-net header. Consequently, packets that are MTU-size
> will take two buffers intead of one (to store the virtio-net header),
> substantially decreasing the throughput of MTU-size traffic due to TCP
> window / SKB truesize effects.
>
> This commit changes the mergeable buffer size to include the virtio-net
> header. The buffer size is cacheline-aligned because skb_page_frag_refill
> will not automatically align the requested size.
>
> Benchmarks taken from an average of 5 netperf 30-second TCP_STREAM runs
> between two QEMU VMs on a single physical machine. Each VM has two VCPUs and
> vhost enabled. All VMs and vhost threads run in a single 4 CPU cgroup
> cpuset, using cgroups to ensure that other processes in the system will not
> be scheduled on the benchmark CPUs. Transmit offloads and mergeable receive
> buffers are enabled, but guest_tso4 / guest_csum are explicitly disabled to
> force MTU-sized packets on the receiver.
>
> next-net trunk before 2613af0ed18a (PAGE_SIZE buf): 3861.08Gb/s
> net-next trunk (MTU 1500- packet uses two buf due to size bug): 4076.62Gb/s
> net-next trunk (MTU 1480- packet fits in one buf): 6301.34Gb/s
> net-next trunk w/ size fix (MTU 1500 - packet fits in one buf): 6445.44Gb/s
>
> Suggested-by: Eric Northup <digitaleric@google.com>
> Signed-off-by: Michael Dalton <mwdalton@google.com>
Please note this is a bugfix - useful by itself.
> ---
> drivers/net/virtio_net.c | 30 ++++++++++++++++--------------
> 1 file changed, 16 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 01f4eb5..69fb225 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -36,7 +36,10 @@ module_param(csum, bool, 0444);
> module_param(gso, bool, 0444);
>
> /* FIXME: MTU in config. */
> -#define MAX_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> +#define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> +#define MERGE_BUFFER_LEN (ALIGN(GOOD_PACKET_LEN + \
> + sizeof(struct virtio_net_hdr_mrg_rxbuf), \
> + L1_CACHE_BYTES))
> #define GOOD_COPY_LEN 128
>
> #define VIRTNET_DRIVER_VERSION "1.0.0"
> @@ -314,10 +317,10 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> head_skb->dev->stats.rx_length_errors++;
> return -EINVAL;
> }
> - if (unlikely(len > MAX_PACKET_LEN)) {
> + if (unlikely(len > MERGE_BUFFER_LEN)) {
> pr_debug("%s: rx error: merge buffer too long\n",
> head_skb->dev->name);
> - len = MAX_PACKET_LEN;
> + len = MERGE_BUFFER_LEN;
> }
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> @@ -336,18 +339,17 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> if (curr_skb != head_skb) {
> head_skb->data_len += len;
> head_skb->len += len;
> - head_skb->truesize += MAX_PACKET_LEN;
> + head_skb->truesize += MERGE_BUFFER_LEN;
> }
> page = virt_to_head_page(buf);
> offset = buf - (char *)page_address(page);
> if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
> put_page(page);
> skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> - len, MAX_PACKET_LEN);
> + len, MERGE_BUFFER_LEN);
> } else {
> skb_add_rx_frag(curr_skb, num_skb_frags, page,
> - offset, len,
> - MAX_PACKET_LEN);
> + offset, len, MERGE_BUFFER_LEN);
> }
> --rq->num;
> }
> @@ -383,7 +385,7 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> struct page *page = virt_to_head_page(buf);
> skb = page_to_skb(rq, page,
> (char *)buf - (char *)page_address(page),
> - len, MAX_PACKET_LEN);
> + len, MERGE_BUFFER_LEN);
> if (unlikely(!skb)) {
> dev->stats.rx_dropped++;
> put_page(page);
> @@ -471,11 +473,11 @@ static int add_recvbuf_small(struct receive_queue *rq, gfp_t gfp)
> struct skb_vnet_hdr *hdr;
> int err;
>
> - skb = __netdev_alloc_skb_ip_align(vi->dev, MAX_PACKET_LEN, gfp);
> + skb = __netdev_alloc_skb_ip_align(vi->dev, GOOD_PACKET_LEN, gfp);
> if (unlikely(!skb))
> return -ENOMEM;
>
> - skb_put(skb, MAX_PACKET_LEN);
> + skb_put(skb, GOOD_PACKET_LEN);
>
> hdr = skb_vnet_hdr(skb);
> sg_set_buf(rq->sg, &hdr->hdr, sizeof hdr->hdr);
> @@ -542,20 +544,20 @@ static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> int err;
>
> if (gfp & __GFP_WAIT) {
> - if (skb_page_frag_refill(MAX_PACKET_LEN, &vi->alloc_frag,
> + if (skb_page_frag_refill(MERGE_BUFFER_LEN, &vi->alloc_frag,
> gfp)) {
> buf = (char *)page_address(vi->alloc_frag.page) +
> vi->alloc_frag.offset;
> get_page(vi->alloc_frag.page);
> - vi->alloc_frag.offset += MAX_PACKET_LEN;
> + vi->alloc_frag.offset += MERGE_BUFFER_LEN;
> }
> } else {
> - buf = netdev_alloc_frag(MAX_PACKET_LEN);
> + buf = netdev_alloc_frag(MERGE_BUFFER_LEN);
> }
> if (!buf)
> return -ENOMEM;
>
> - sg_init_one(rq->sg, buf, MAX_PACKET_LEN);
> + sg_init_one(rq->sg, buf, MERGE_BUFFER_LEN);
> err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
> if (err < 0)
> put_page(virt_to_head_page(buf));
> --
> 1.8.4.1
^ permalink raw reply
* Re: [PATCH net-next 4/4] virtio-net: auto-tune mergeable rx buffer size for improved performance
From: Michael S. Tsirkin @ 2013-11-13 17:42 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, Daniel Borkmann, virtualization,
Eric Dumazet, David S. Miller
In-Reply-To: <528325DC.3050801@redhat.com>
On Wed, Nov 13, 2013 at 03:10:20PM +0800, Jason Wang wrote:
> On 11/13/2013 06:21 AM, Michael Dalton wrote:
> > Commit 2613af0ed18a ("virtio_net: migrate mergeable rx buffers to page frag
> > allocators") changed the mergeable receive buffer size from PAGE_SIZE to
> > MTU-size, introducing a single-stream regression for benchmarks with large
> > average packet size. There is no single optimal buffer size for all workloads.
> > For workloads with packet size <= MTU bytes, MTU + virtio-net header-sized
> > buffers are preferred as larger buffers reduce the TCP window due to SKB
> > truesize. However, single-stream workloads with large average packet sizes
> > have higher throughput if larger (e.g., PAGE_SIZE) buffers are used.
> >
> > This commit auto-tunes the mergeable receiver buffer packet size by choosing
> > the packet buffer size based on an EWMA of the recent packet sizes for the
> > receive queue. Packet buffer sizes range from MTU_SIZE + virtio-net header
> > len to PAGE_SIZE. This improves throughput for large packet workloads, as
> > any workload with average packet size >= PAGE_SIZE will use PAGE_SIZE
> > buffers.
>
> Hi Michael:
>
> There's one concern with EWMA. How well does it handle multiple streams
> each with different packet size? E.g there may be two flows, one with
> 256 bytes each packet another is 64K. Looks like it can result we
> allocate PAGE_SIZE buffer for 256 (which is bad since the
> payload/truesize is low) bytes or 1500+ for 64K buffer (which is ok
> since we can do coalescing).
> >
> > These optimizations interact positively with recent commit
> > ba275241030c ("virtio-net: coalesce rx frags when possible during rx"),
> > which coalesces adjacent RX SKB fragments in virtio_net. The coalescing
> > optimizations benefit buffers of any size.
> >
> > Benchmarks taken from an average of 5 netperf 30-second TCP_STREAM runs
> > between two QEMU VMs on a single physical machine. Each VM has two VCPUs
> > with all offloads & vhost enabled. All VMs and vhost threads run in a
> > single 4 CPU cgroup cpuset, using cgroups to ensure that other processes
> > in the system will not be scheduled on the benchmark CPUs. Trunk includes
> > SKB rx frag coalescing.
> >
> > net-next trunk w/ virtio_net before 2613af0ed18a (PAGE_SIZE bufs): 14642.85Gb/s
> > net-next trunk (MTU-size bufs): 13170.01Gb/s
> > net-next trunk + auto-tune: 14555.94Gb/s
>
> Do you have perf numbers that just without this patch? We need to know
> how much EWMA help exactly.
Yes I'm curious too.
> >
> > Signed-off-by: Michael Dalton <mwdalton@google.com>
> > ---
> > drivers/net/virtio_net.c | 73 +++++++++++++++++++++++++++++++++++-------------
> > 1 file changed, 53 insertions(+), 20 deletions(-)
> >
> > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > index 0c93054..b1086e0 100644
> > --- a/drivers/net/virtio_net.c
> > +++ b/drivers/net/virtio_net.c
> > @@ -27,6 +27,7 @@
> > #include <linux/if_vlan.h>
> > #include <linux/slab.h>
> > #include <linux/cpu.h>
> > +#include <linux/average.h>
> >
> > static int napi_weight = NAPI_POLL_WEIGHT;
> > module_param(napi_weight, int, 0444);
> > @@ -37,10 +38,8 @@ module_param(gso, bool, 0444);
> >
> > /* FIXME: MTU in config. */
> > #define GOOD_PACKET_LEN (ETH_HLEN + VLAN_HLEN + ETH_DATA_LEN)
> > -#define MERGE_BUFFER_LEN (ALIGN(GOOD_PACKET_LEN + \
> > - sizeof(struct virtio_net_hdr_mrg_rxbuf), \
> > - L1_CACHE_BYTES))
> > #define GOOD_COPY_LEN 128
> > +#define RECEIVE_AVG_WEIGHT 64
>
> Maybe we can make this as a module parameter.
I'm not sure it's useful - no one is likely to tune it in practice.
But how about a comment explaining how was the number chosen?
> >
> > #define VIRTNET_DRIVER_VERSION "1.0.0"
> >
> > @@ -79,6 +78,9 @@ struct receive_queue {
> > /* Chain pages by the private ptr. */
> > struct page *pages;
> >
> > + /* Average packet length for mergeable receive buffers. */
> > + struct ewma mrg_avg_pkt_len;
> > +
> > /* Page frag for GFP_ATOMIC packet buffer allocation. */
> > struct page_frag atomic_frag;
> >
> > @@ -302,14 +304,17 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
> > return skb;
> > }
> >
> > -static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> > +static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb,
> > + struct page *head_page)
> > {
> > struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
> > struct sk_buff *curr_skb = head_skb;
> > + struct page *page = head_page;
> > char *buf;
> > - struct page *page;
> > - int num_buf, len, offset, truesize;
> > + int num_buf, len, offset;
> > + u32 est_buffer_len;
> >
> > + len = head_skb->len;
> > num_buf = hdr->mhdr.num_buffers;
> > while (--num_buf) {
> > int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> > @@ -320,7 +325,6 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> > head_skb->dev->stats.rx_length_errors++;
> > return -EINVAL;
> > }
> > - truesize = max_t(int, len, MERGE_BUFFER_LEN);
> > if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> > struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> > if (unlikely(!nskb)) {
> > @@ -338,20 +342,38 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
> > if (curr_skb != head_skb) {
> > head_skb->data_len += len;
> > head_skb->len += len;
> > - head_skb->truesize += truesize;
> > + head_skb->truesize += len;
> > }
> > page = virt_to_head_page(buf);
> > offset = buf - (char *)page_address(page);
> > if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
> > put_page(page);
> > skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> > - len, truesize);
> > + len, len);
> > } else {
> > skb_add_rx_frag(curr_skb, num_skb_frags, page,
> > - offset, len, truesize);
> > + offset, len, len);
> > }
> > --rq->num;
> > }
> > + /* All frags before the last frag are fully used -- for those frags,
> > + * truesize = len. Use the size of the most recent buffer allocation
> > + * from the last frag's page to estimate the truesize of the last frag.
> > + * EWMA with a weight of 64 makes the size adjustments quite small in
> > + * the frags allocated on one page (even a order-3 one), and truesize
> > + * doesn't need to be 100% accurate.
> > + */
> > + if (page) {
> > + est_buffer_len = page_private(page);
> > + if (est_buffer_len > len) {
> > + u32 truesize_delta = est_buffer_len - len;
> > +
> > + curr_skb->truesize += truesize_delta;
> > + if (curr_skb != head_skb)
> > + head_skb->truesize += truesize_delta;
> > + }
>
> Is there a chance that est_buffer_len was smaller than or equal with len?
> > + }
> > + ewma_add(&rq->mrg_avg_pkt_len, head_skb->len);
> > return 0;
> > }
> >
> > @@ -382,16 +404,21 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
> > skb_trim(skb, len);
> > } else if (vi->mergeable_rx_bufs) {
> > struct page *page = virt_to_head_page(buf);
> > - int truesize = max_t(int, len, MERGE_BUFFER_LEN);
> > + /* Use an initial truesize of 'len' bytes for page_to_skb --
> > + * receive_mergeable will fixup the truesize of the last page
> > + * frag if the packet is non-linear (> GOOD_COPY_LEN bytes).
> > + */
> > skb = page_to_skb(rq, page,
> > (char *)buf - (char *)page_address(page),
> > - len, truesize);
> > + len, len);
> > if (unlikely(!skb)) {
> > dev->stats.rx_dropped++;
> > put_page(page);
> > return;
> > }
> > - if (receive_mergeable(rq, skb)) {
> > + if (!skb_is_nonlinear(skb))
> > + page = NULL;
> > + if (receive_mergeable(rq, skb, page)) {
> > dev_kfree_skb(skb);
> > return;
> > }
> > @@ -540,24 +567,29 @@ static int add_recvbuf_big(struct receive_queue *rq, gfp_t gfp)
> > static int add_recvbuf_mergeable(struct receive_queue *rq, gfp_t gfp)
> > {
> > struct virtnet_info *vi = rq->vq->vdev->priv;
> > + const size_t hdr_len = sizeof(struct virtio_net_hdr_mrg_rxbuf);
> > struct page_frag *alloc_frag;
> > char *buf;
> > - int err, len, hole;
> > + int err, hole;
> > + u32 buflen;
> >
> > + buflen = hdr_len + clamp_t(u32, ewma_read(&rq->mrg_avg_pkt_len),
> > + GOOD_PACKET_LEN, PAGE_SIZE - hdr_len);
> > + buflen = ALIGN(buflen, L1_CACHE_BYTES);
> > alloc_frag = (gfp & __GFP_WAIT) ? &vi->sleep_frag : &rq->atomic_frag;
> > - if (unlikely(!skb_page_frag_refill(MERGE_BUFFER_LEN, alloc_frag, gfp)))
> > + if (unlikely(!skb_page_frag_refill(buflen, alloc_frag, gfp)))
> > return -ENOMEM;
> > buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> > get_page(alloc_frag->page);
> > - len = MERGE_BUFFER_LEN;
> > - alloc_frag->offset += len;
> > + alloc_frag->offset += buflen;
> > + set_page_private(alloc_frag->page, buflen);
>
> Not sure this is accurate, since buflen may change and several frags may
> share a single page. So the est_buffer_len we get in receive_mergeable()
> may not be the correct value.
> > hole = alloc_frag->size - alloc_frag->offset;
> > - if (hole < MERGE_BUFFER_LEN) {
> > - len += hole;
> > + if (hole < buflen) {
> > + buflen += hole;
> > alloc_frag->offset += hole;
> > }
> >
> > - sg_init_one(rq->sg, buf, len);
> > + sg_init_one(rq->sg, buf, buflen);
> > err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
> > if (err < 0)
> > put_page(virt_to_head_page(buf));
> > @@ -1475,6 +1507,7 @@ static int virtnet_alloc_queues(struct virtnet_info *vi)
> > napi_weight);
> >
> > sg_init_table(vi->rq[i].sg, ARRAY_SIZE(vi->rq[i].sg));
> > + ewma_init(&vi->rq[i].mrg_avg_pkt_len, 1, RECEIVE_AVG_WEIGHT);
> > sg_init_table(vi->sq[i].sg, ARRAY_SIZE(vi->sq[i].sg));
> > }
> >
^ 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