* [patch net-next 0/6] net_sched: cls_*: couple of fixes
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
Jiri Pirko (6):
net_sched: cls_basic: remove unnecessary iteration and use passed arg
net_sched: cls_bpf: remove unnecessary iteration and use passed arg
net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu
net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu
net_sched: cls_flow: remove duplicate assignments
net_sched: cls_cgroup: remove unnecessary if
net/sched/cls_basic.c | 16 +++++-----------
net/sched/cls_bpf.c | 21 +++++++--------------
net/sched/cls_cgroup.c | 6 +-----
net/sched/cls_flow.c | 7 ++-----
4 files changed, 15 insertions(+), 35 deletions(-)
--
1.9.3
^ permalink raw reply
* [patch net-next 1/6] net_sched: cls_basic: remove unnecessary iteration and use passed arg
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
In-Reply-To: <1417539636-12710-1-git-send-email-jiri@resnulli.us>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/sched/cls_basic.c | 16 +++++-----------
1 file changed, 5 insertions(+), 11 deletions(-)
diff --git a/net/sched/cls_basic.c b/net/sched/cls_basic.c
index cd61280..1c122c7 100644
--- a/net/sched/cls_basic.c
+++ b/net/sched/cls_basic.c
@@ -113,18 +113,12 @@ static void basic_destroy(struct tcf_proto *tp)
static int basic_delete(struct tcf_proto *tp, unsigned long arg)
{
- struct basic_head *head = rtnl_dereference(tp->root);
- struct basic_filter *t, *f = (struct basic_filter *) arg;
-
- list_for_each_entry(t, &head->flist, link)
- if (t == f) {
- list_del_rcu(&t->link);
- tcf_unbind_filter(tp, &t->res);
- call_rcu(&t->rcu, basic_delete_filter);
- return 0;
- }
+ struct basic_filter *f = (struct basic_filter *) arg;
- return -ENOENT;
+ list_del_rcu(&f->link);
+ tcf_unbind_filter(tp, &f->res);
+ call_rcu(&f->rcu, basic_delete_filter);
+ return 0;
}
static const struct nla_policy basic_policy[TCA_BASIC_MAX + 1] = {
--
1.9.3
^ permalink raw reply related
* [patch net-next 4/6] net_sched: cls_flow: remove faulty use of list_for_each_entry_rcu
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
In-Reply-To: <1417539636-12710-1-git-send-email-jiri@resnulli.us>
rcu variant is not correct here. The code is called by updater (rtnl
lock is held), not by reader (no rcu_read_lock is held).
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/sched/cls_flow.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index 4ac515f..a605739 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -578,7 +578,7 @@ static unsigned long flow_get(struct tcf_proto *tp, u32 handle)
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f;
- list_for_each_entry_rcu(f, &head->filters, list)
+ list_for_each_entry(f, &head->filters, list)
if (f->handle == handle)
return (unsigned long)f;
return 0;
@@ -654,7 +654,7 @@ static void flow_walk(struct tcf_proto *tp, struct tcf_walker *arg)
struct flow_head *head = rtnl_dereference(tp->root);
struct flow_filter *f;
- list_for_each_entry_rcu(f, &head->filters, list) {
+ list_for_each_entry(f, &head->filters, list) {
if (arg->count < arg->skip)
goto skip;
if (arg->fn(tp, (unsigned long)f, arg) < 0) {
--
1.9.3
^ permalink raw reply related
* [patch net-next 3/6] net_sched: cls_bpf: remove faulty use of list_for_each_entry_rcu
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
In-Reply-To: <1417539636-12710-1-git-send-email-jiri@resnulli.us>
rcu variant is not correct here. The code is called by updater (rtnl
lock is held), not by reader (no rcu_read_lock is held).
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/sched/cls_bpf.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index cbfaf6f..d0de979 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -141,7 +141,7 @@ static unsigned long cls_bpf_get(struct tcf_proto *tp, u32 handle)
if (head == NULL)
return 0UL;
- list_for_each_entry_rcu(prog, &head->plist, link) {
+ list_for_each_entry(prog, &head->plist, link) {
if (prog->handle == handle) {
ret = (unsigned long) prog;
break;
@@ -337,7 +337,7 @@ static void cls_bpf_walk(struct tcf_proto *tp, struct tcf_walker *arg)
struct cls_bpf_head *head = rtnl_dereference(tp->root);
struct cls_bpf_prog *prog;
- list_for_each_entry_rcu(prog, &head->plist, link) {
+ list_for_each_entry(prog, &head->plist, link) {
if (arg->count < arg->skip)
goto skip;
if (arg->fn(tp, (unsigned long) prog, arg) < 0) {
--
1.9.3
^ permalink raw reply related
* [patch net-next 6/6] net_sched: cls_cgroup: remove unnecessary if
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
In-Reply-To: <1417539636-12710-1-git-send-email-jiri@resnulli.us>
since head->handle == handle (checked before), just assign handle.
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/sched/cls_cgroup.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/net/sched/cls_cgroup.c b/net/sched/cls_cgroup.c
index d61a801..dbee65e 100644
--- a/net/sched/cls_cgroup.c
+++ b/net/sched/cls_cgroup.c
@@ -117,11 +117,7 @@ static int cls_cgroup_change(struct net *net, struct sk_buff *in_skb,
return -ENOBUFS;
tcf_exts_init(&new->exts, TCA_CGROUP_ACT, TCA_CGROUP_POLICE);
- if (head)
- new->handle = head->handle;
- else
- new->handle = handle;
-
+ new->handle = handle;
new->tp = tp;
err = nla_parse_nested(tb, TCA_CGROUP_MAX, tca[TCA_OPTIONS],
cgroup_policy);
--
1.9.3
^ permalink raw reply related
* [patch net-next 2/6] net_sched: cls_bpf: remove unnecessary iteration and use passed arg
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
In-Reply-To: <1417539636-12710-1-git-send-email-jiri@resnulli.us>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/sched/cls_bpf.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index eed49d1..cbfaf6f 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -109,19 +109,12 @@ static void __cls_bpf_delete_prog(struct rcu_head *rcu)
static int cls_bpf_delete(struct tcf_proto *tp, unsigned long arg)
{
- struct cls_bpf_head *head = rtnl_dereference(tp->root);
- struct cls_bpf_prog *prog, *todel = (struct cls_bpf_prog *) arg;
-
- list_for_each_entry(prog, &head->plist, link) {
- if (prog == todel) {
- list_del_rcu(&prog->link);
- tcf_unbind_filter(tp, &prog->res);
- call_rcu(&prog->rcu, __cls_bpf_delete_prog);
- return 0;
- }
- }
+ struct cls_bpf_prog *prog = (struct cls_bpf_prog *) arg;
- return -ENOENT;
+ list_del_rcu(&prog->link);
+ tcf_unbind_filter(tp, &prog->res);
+ call_rcu(&prog->rcu, __cls_bpf_delete_prog);
+ return 0;
}
static void cls_bpf_destroy(struct tcf_proto *tp)
--
1.9.3
^ permalink raw reply related
* [patch net-next 5/6] net_sched: cls_flow: remove duplicate assignments
From: Jiri Pirko @ 2014-12-02 17:00 UTC (permalink / raw)
To: netdev; +Cc: davem, jhs
In-Reply-To: <1417539636-12710-1-git-send-email-jiri@resnulli.us>
Signed-off-by: Jiri Pirko <jiri@resnulli.us>
---
net/sched/cls_flow.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/net/sched/cls_flow.c b/net/sched/cls_flow.c
index a605739..819c230 100644
--- a/net/sched/cls_flow.c
+++ b/net/sched/cls_flow.c
@@ -426,10 +426,7 @@ static int flow_change(struct net *net, struct sk_buff *in_skb,
goto err2;
/* Copy fold into fnew */
- fnew->handle = fold->handle;
- fnew->keymask = fold->keymask;
fnew->tp = fold->tp;
-
fnew->handle = fold->handle;
fnew->nkeys = fold->nkeys;
fnew->keymask = fold->keymask;
--
1.9.3
^ permalink raw reply related
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Thomas Graf @ 2014-12-02 17:09 UTC (permalink / raw)
To: Du, Fan, 'Jason Wang', netdev@vger.kernel.org,
davem@davemloft.net, fw@strlen.de, dev, mst, jesse, pshelar
In-Reply-To: <20141202154839.GB5344@t520.home>
On 12/02/14 at 01:48pm, Flavio Leitner wrote:
> What about containers or any other virtualization environment that
> doesn't use Virtio?
The host can dictate the MTU in that case for both veth or OVS
internal which would be primary container plumbing techniques.
ivshmem would need it's own fix.
^ permalink raw reply
* Re: [PATCH] net: mvneta: fix Tx interrupt delay
From: Ezequiel Garcia @ 2014-12-02 17:12 UTC (permalink / raw)
To: Eric Dumazet, Willy Tarreau
Cc: netdev, Maggie Mae Roxas, Thomas Petazzoni, Gregory CLEMENT
In-Reply-To: <1417522688.5303.35.camel@edumazet-glaptop2.roam.corp.google.com>
Eric,
On 12/02/2014 09:18 AM, Eric Dumazet wrote:
[..]
>
> I am surprised TCP even worked correctly with this problem.
>
> I highly suggest BQL for this driver, now this issue is fixed.
>
Implementing BQL for the mvneta driver was something I wanted to do a
while ago, but you explained that these kind drivers (i.e. those with
software TSO) didn't need BQL, because the latency that resulted from
the ring was too small.
Quoting (http://www.spinics.net/lists/netdev/msg284439.html):
""
Note that a full size TSO packet (44 or 45 MSS) requires about 88 or 90
descriptors.
So I do not think BQL is really needed, because a 512 slots TX ring wont
add a big latency : About 5 ms max.
BQL is generally nice for NIC supporting hardware TSO, where a 64KB TSO
packet consumes 3 or 4 descriptors.
Also note that TCP Small Queues should limit TX ring occupancy of a
single bulk flow anyway.
""
Maybe I misunderstood something?
--
Ezequiel García, Free Electrons
Embedded Linux, Kernel and Android Engineering
http://free-electrons.com
^ permalink raw reply
* Re: [PATCH net-next] rtnetlink: delay RTM_DELLINK notification until after ndo_uninit()
From: Roopa Prabhu @ 2014-12-02 17:19 UTC (permalink / raw)
To: Eric Dumazet
Cc: Mahesh Bandewar, netdev, David Miller, Eric Dumazet,
Toshiaki Makita
In-Reply-To: <1417539160.5303.73.camel@edumazet-glaptop2.roam.corp.google.com>
On 12/2/14, 8:52 AM, Eric Dumazet wrote:
> On Tue, 2014-12-02 at 08:41 -0800, Roopa Prabhu wrote:
>
>> fair point. But the commit that moved things around was done to handle
>> cases where,
>> the ndo_uninit() already sends some notifications to userspace for the
>> changes
>> during uninit (example bond driver).
>>
>> The only point i was making was that the dellink after the ndo_uninit in
>> your
>> case now contains state that was prior to uninit for these drivers.
> I think Mahesh forgot to mention your patch probably broke some drivers.
>
> calling rtmsg_ifinfo() after uninit() is probably breaking dummy device,
> as it does :
>
> static void dummy_dev_uninit(struct net_device *dev)
> {
> free_percpu(dev->dstats);
> }
>
> It looks like 'fixing' ipvlan is not going to help.
>
> Instead of checking all drivers for such interesting side effects,
> and revert your patch, we had the idea of this solution.
ok fair enough. I could go through all drivers again and check and fix them.
but dont want to miss any such cases.
The patch idea is good.
Worth a comment in the source regarding the state in the dellink.
Hopefully the drivers that are affected by this are very few.
Acked-by: Roopa Prabhu <roopa@cumulusnetworks.com>
Thanks!.
^ permalink raw reply
* Re: [PATCH] net: mvneta: fix Tx interrupt delay
From: Dave Taht @ 2014-12-02 17:31 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Eric Dumazet, Willy Tarreau, netdev@vger.kernel.org,
Maggie Mae Roxas, Thomas Petazzoni, Gregory CLEMENT
In-Reply-To: <547DF2EA.2020908@free-electrons.com>
On Tue, Dec 2, 2014 at 9:12 AM, Ezequiel Garcia
<ezequiel.garcia@free-electrons.com> wrote:
> Eric,
>
> On 12/02/2014 09:18 AM, Eric Dumazet wrote:
> [..]
>>
>> I am surprised TCP even worked correctly with this problem.
>>
>> I highly suggest BQL for this driver, now this issue is fixed.
>>
>
> Implementing BQL for the mvneta driver was something I wanted to do a
> while ago, but you explained that these kind drivers (i.e. those with
> software TSO) didn't need BQL, because the latency that resulted from
> the ring was too small.
>
> Quoting (http://www.spinics.net/lists/netdev/msg284439.html):
> ""
> Note that a full size TSO packet (44 or 45 MSS) requires about 88 or 90
> descriptors.
>
> So I do not think BQL is really needed, because a 512 slots TX ring wont
> add a big latency : About 5 ms max.
at a gigabit.
Wildly variable with pause frames. And: 50ms at 100mbit. 500ms at 10mbit.
Secondly, why accept any latency in the driver when you can get it well below
2ms in most circumstances with a couple lines of BQL code?
>
> BQL is generally nice for NIC supporting hardware TSO, where a 64KB TSO
> packet consumes 3 or 4 descriptors.
BQL is nice at any speed, and darn useful at lower speeds,
as well.
>
> Also note that TCP Small Queues should limit TX ring occupancy of a
> single bulk flow anyway.
We only do one upload or one download at a time on our systems,
followed by a ping measurement... never do more than one flow
at the same time, right?
> ""
>
> Maybe I misunderstood something?
Try it, test it, you'll like it.
> --
> Ezequiel García, Free Electrons
> Embedded Linux, Kernel and Android Engineering
> http://free-electrons.com
> --
> 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
--
Dave Täht
http://www.bufferbloat.net/projects/bloat/wiki/Upcoming_Talks
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Michael S. Tsirkin @ 2014-12-02 17:34 UTC (permalink / raw)
To: Thomas Graf
Cc: dev-yBygre7rU0TnMu66kgdUjQ,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
'Jason Wang', Du, Fan,
fw-HFFVJYpyMKqzQB+pC5nmwQ@public.gmane.org,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20141202170927.GA9457-FZi0V3Vbi30CUdFEqe4BF2D2FQJk+8+b@public.gmane.org>
On Tue, Dec 02, 2014 at 05:09:27PM +0000, Thomas Graf wrote:
> On 12/02/14 at 01:48pm, Flavio Leitner wrote:
> > What about containers or any other virtualization environment that
> > doesn't use Virtio?
>
> The host can dictate the MTU in that case for both veth or OVS
> internal which would be primary container plumbing techniques.
It typically can't do this easily for VMs with emulated devices:
real ethernet uses a fixed MTU.
IMHO it's confusing to suggest MTU as a fix for this bug, it's
an unrelated optimization.
ICMP_DEST_UNREACH/ICMP_FRAG_NEEDED is the right fix here.
> ivshmem would need it's own fix.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Fw: [Bug 89161] New: Regression in bonding driver with devices that have no MAC address
From: Stephen Hemminger @ 2014-12-02 17:34 UTC (permalink / raw)
To: netdev
Begin forwarded message:
Date: Tue, 2 Dec 2014 02:15:05 -0800
From: "bugzilla-daemon@bugzilla.kernel.org" <bugzilla-daemon@bugzilla.kernel.org>
To: "stephen@networkplumber.org" <stephen@networkplumber.org>
Subject: [Bug 89161] New: Regression in bonding driver with devices that have no MAC address
https://bugzilla.kernel.org/show_bug.cgi?id=89161
Bug ID: 89161
Summary: Regression in bonding driver with devices that have no
MAC address
Product: Networking
Version: 2.5
Kernel Version: 3.17.4
Hardware: All
OS: Linux
Tree: Mainline
Status: NEW
Severity: normal
Priority: P1
Component: Other
Assignee: shemminger@linux-foundation.org
Reporter: tjc@wintrmute.net
Regression: No
In kernel 3.13 and earlier, a user could use the "bonding" network module to
enslave devices without MAC addresses, such as ppp devices, in certain modes
(such as balance-rr).
At some point between 3.13 and 3.17, this ability was removed -- even though
apparently a commit was made to specifically ALLOW this to happen:
https://github.com/torvalds/linux/commit/f54424412b6b2f64cae4d7c39d981ca14ce0052c
On kernel 3.13, this was printed to syslog upon enslaving ppp0:
bonding: bond0: Warning: The first slave device specified does not support
setting the MAC address. Setting fail_over_mac to active.
bonding: bond0: enslaving ppp0 as an active interface with an up link.
On kernel 3.17.2 and 3.17.4 (and probably others) instead this error comes up:
bond0: Adding slave ppp0
bond0: The slave device specified does not support setting the MAC address
And the slave is not added.
In case it was relevant, I resorted to manually creating the bond0 with
appropriate options (including fail_over_mac) preset, and yet the problem
persists.
To replicate the problem, setup at least one ppp connection, and then follow
these instructions:
modprobe bonding
echo '+bond0' > /sys/class/net/bonding_masters
echo 'active' > /sys/class/net/bond0/bonding/fail_over_mac
ifconfig bond0 down
echo 'balance-rr' > /sys/class/net/bond0/bonding/mode
ifconfig bond0 202.xx.xx.xx netmask 255.255.255.255 mtu 1492 up
echo '500' > /sys/class/net/bond0/bonding/miimon
ifconfig ppp0 down
echo '+ppp0' > /sys/class/net/bond0/bonding/slaves
Under earlier kernel versions, that would work -- but current stable kernels
fail.
--
You are receiving this mail because:
You are the assignee for the bug.
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Thomas Graf @ 2014-12-02 17:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Du, Fan, 'Jason Wang', netdev@vger.kernel.org,
davem@davemloft.net, fw@strlen.de, dev, jesse, pshelar
In-Reply-To: <20141202173401.GB4126@redhat.com>
On 12/02/14 at 07:34pm, Michael S. Tsirkin wrote:
> On Tue, Dec 02, 2014 at 05:09:27PM +0000, Thomas Graf wrote:
> > On 12/02/14 at 01:48pm, Flavio Leitner wrote:
> > > What about containers or any other virtualization environment that
> > > doesn't use Virtio?
> >
> > The host can dictate the MTU in that case for both veth or OVS
> > internal which would be primary container plumbing techniques.
>
> It typically can't do this easily for VMs with emulated devices:
> real ethernet uses a fixed MTU.
>
> IMHO it's confusing to suggest MTU as a fix for this bug, it's
> an unrelated optimization.
> ICMP_DEST_UNREACH/ICMP_FRAG_NEEDED is the right fix here.
PMTU discovery only resolves the issue if an actual IP stack is
running inside the VM. This may not be the case at all.
I agree that exposing an MTU towards the guest is not applicable
in all situations, in particular because it is difficult to decide
what MTU to expose. It is a relatively elegant solution in a lot
of virtualization host cases hooked up to an orchestration system
though.
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Jesse Gross @ 2014-12-02 18:06 UTC (permalink / raw)
To: Du, Fan, Jason Wang, netdev@vger.kernel.org, davem@davemloft.net,
fw@strlen.de
In-Reply-To: <20141202154425.GA5344@t520.home>
On Tue, Dec 2, 2014 at 7:44 AM, Flavio Leitner <fbl@redhat.com> wrote:
> On Sun, Nov 30, 2014 at 10:08:32AM +0000, Du, Fan wrote:
>>
>>
>> >-----Original Message-----
>> >From: Jason Wang [mailto:jasowang@redhat.com]
>> >Sent: Friday, November 28, 2014 3:02 PM
>> >To: Du, Fan
>> >Cc: netdev@vger.kernel.org; davem@davemloft.net; fw@strlen.de; Du, Fan
>> >Subject: Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
>> >
>> >
>> >
>> >On Fri, Nov 28, 2014 at 2:33 PM, Fan Du <fan.du@intel.com> wrote:
>> >> Test scenario: two KVM guests sitting in different hosts communicate
>> >> to each other with a vxlan tunnel.
>> >>
>> >> All interface MTU is default 1500 Bytes, from guest point of view, its
>> >> skb gso_size could be as bigger as 1448Bytes, however after guest skb
>> >> goes through vxlan encapuslation, individual segments length of a gso
>> >> packet could exceed physical NIC MTU 1500, which will be lost at
>> >> recevier side.
>> >>
>> >> So it's possible in virtualized environment, locally created skb len
>> >> after encapslation could be bigger than underlayer MTU. In such case,
>> >> it's reasonable to do GSO first, then fragment any packet bigger than
>> >> MTU as possible.
>> >>
>> >> +---------------+ TX RX +---------------+
>> >> | KVM Guest | -> ... -> | KVM Guest |
>> >> +-+-----------+-+ +-+-----------+-+
>> >> |Qemu/VirtIO| |Qemu/VirtIO|
>> >> +-----------+ +-----------+
>> >> | |
>> >> v tap0 tap0 v
>> >> +-----------+ +-----------+
>> >> | ovs bridge| | ovs bridge|
>> >> +-----------+ +-----------+
>> >> | vxlan vxlan |
>> >> v v
>> >> +-----------+ +-----------+
>> >> | NIC | <------> | NIC |
>> >> +-----------+ +-----------+
>> >>
>> >> Steps to reproduce:
>> >> 1. Using kernel builtin openvswitch module to setup ovs bridge.
>> >> 2. Runing iperf without -M, communication will stuck.
>> >
>> >Is this issue specific to ovs or ipv4? Path MTU discovery should help in this case I
>> >believe.
>>
>> Problem here is host stack push local over-sized gso skb down to NIC, and perform GSO there
>> without any further ip segmentation.
>>
>> Reasonable behavior is do gso first at ip level, if gso-ed skb is bigger than MTU && df is set,
>> Then push ICMP_DEST_UNREACH/ICMP_FRAG_NEEDED message back to sender to adjust mtu.
>>
>> For PMTU to work, that's another issue I will try to address later on.
>>
>> >>
>> >>
>> >> Signed-off-by: Fan Du <fan.du@intel.com>
>> >> ---
>> >> net/ipv4/ip_output.c | 7 ++++---
>> >> 1 files changed, 4 insertions(+), 3 deletions(-)
>> >>
>> >> diff --git a/net/ipv4/ip_output.c b/net/ipv4/ip_output.c index
>> >> bc6471d..558b5f8 100644
>> >> --- a/net/ipv4/ip_output.c
>> >> +++ b/net/ipv4/ip_output.c
>> >> @@ -217,9 +217,10 @@ static int ip_finish_output_gso(struct sk_buff
>> >> *skb)
>> >> struct sk_buff *segs;
>> >> int ret = 0;
>> >>
>> >> - /* common case: locally created skb or seglen is <= mtu */
>> >> - if (((IPCB(skb)->flags & IPSKB_FORWARDED) == 0) ||
>> >> - skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb))
>> >> + /* Both locally created skb and forwarded skb could exceed
>> >> + * MTU size, so make a unified rule for them all.
>> >> + */
>> >> + if (skb_gso_network_seglen(skb) <= ip_skb_dst_mtu(skb))
>> >> return ip_finish_output2(skb);
>
>
> Are you using kernel's vxlan device or openvswitch's vxlan device?
>
> Because for kernel's vxlan devices the MTU accounts for the header
> overhead so I believe your patch would work. However, the MTU is
> not visible for the ovs's vxlan devices, so that wouldn't work.
This is being called after the tunnel code, so the MTU that is being
looked at in all cases is the physical device's. Since the packet has
already been encapsulated, tunnel header overhead is already accounted
for in skb_gso_network_seglen() and this should be fine for both OVS
and non-OVS cases.
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Jesse Gross @ 2014-12-02 18:12 UTC (permalink / raw)
To: Thomas Graf
Cc: Michael S. Tsirkin, Du, Fan, Jason Wang, netdev@vger.kernel.org,
davem@davemloft.net, fw@strlen.de, dev@openvswitch.org,
Pravin Shelar
In-Reply-To: <20141202174158.GB9457@casper.infradead.org>
On Tue, Dec 2, 2014 at 9:41 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 12/02/14 at 07:34pm, Michael S. Tsirkin wrote:
>> On Tue, Dec 02, 2014 at 05:09:27PM +0000, Thomas Graf wrote:
>> > On 12/02/14 at 01:48pm, Flavio Leitner wrote:
>> > > What about containers or any other virtualization environment that
>> > > doesn't use Virtio?
>> >
>> > The host can dictate the MTU in that case for both veth or OVS
>> > internal which would be primary container plumbing techniques.
>>
>> It typically can't do this easily for VMs with emulated devices:
>> real ethernet uses a fixed MTU.
>>
>> IMHO it's confusing to suggest MTU as a fix for this bug, it's
>> an unrelated optimization.
>> ICMP_DEST_UNREACH/ICMP_FRAG_NEEDED is the right fix here.
>
> PMTU discovery only resolves the issue if an actual IP stack is
> running inside the VM. This may not be the case at all.
It's also only really a correct thing to do if the ICMP packet is
coming from an L3 node. If you are doing straight bridging then you
have to resort to hacks like OVS had before, which I agree are not
particularly desirable.
> I agree that exposing an MTU towards the guest is not applicable
> in all situations, in particular because it is difficult to decide
> what MTU to expose. It is a relatively elegant solution in a lot
> of virtualization host cases hooked up to an orchestration system
> though.
I also think this is the right thing to do as a common case
optimization and I know other platforms (such as Hyper-V) do it. It's
not a complete solution so we still need the original patch in this
thread to handle things transparently.
^ permalink raw reply
* Re: [PATCHv2 net] i40e: Implement ndo_gso_check()
From: Jesse Gross @ 2014-12-02 18:26 UTC (permalink / raw)
To: Tom Herbert
Cc: Joe Stringer, netdev, Shannon Nelson, Brandeburg, Jesse,
Jeff Kirsher, linux.nics, Linux Kernel Mailing List
In-Reply-To: <CA+mtBx9RwkJ9b84d_OkCtOunFBsuqw276=5E+Qhoqq75utCR4w@mail.gmail.com>
On Mon, Dec 1, 2014 at 4:09 PM, Tom Herbert <therbert@google.com> wrote:
> On Mon, Dec 1, 2014 at 3:53 PM, Jesse Gross <jesse@nicira.com> wrote:
>> On Mon, Dec 1, 2014 at 3:47 PM, Tom Herbert <therbert@google.com> wrote:
>>> On Mon, Dec 1, 2014 at 3:35 PM, Joe Stringer <joestringer@nicira.com> wrote:
>>>> On 21 November 2014 at 09:59, Joe Stringer <joestringer@nicira.com> wrote:
>>>>> On 20 November 2014 16:19, Jesse Gross <jesse@nicira.com> wrote:
>>>>>> I don't know if we need to have the check at all for IPIP though -
>>>>>> after all the driver doesn't expose support for it all (actually it
>>>>>> doesn't expose GRE either). This raises kind of an interesting
>>>>>> question about the checks though - it's pretty easy to add support to
>>>>>> the driver for a new GSO type (and I imagine that people will be
>>>>>> adding GRE soon) and forget to update the check.
>>>>>
>>>>> If the check is more conservative, then testing would show that it's
>>>>> not working and lead people to figure out why (and update the check).
>>>>
>>>> More concretely, one suggestion would be something like following at
>>>> the start of each gso_check():
>>>>
>>>> + const int supported = SKB_GSO_TCPV4 | SKB_GSO_TCPV6 | SKB_GSO_FCOE |
>>>> + SKB_GSO_UDP | SKB_GSO_UDP_TUNNEL;
>>>> +
>>>> + if (skb_shinfo(skb)->gso_type & ~supported)
>>>> + return false;
>>>
>>> This should already be handled by net_gso_ok.
>>
>> My original point wasn't so much that this isn't handled at the moment
>> but that it's easy to add a supported GSO type but then forget to
>> update this check - i.e. if a driver already supports UDP_TUNNEL and
>> adds support for GRE with the same constraints. It seems not entirely
>> ideal that this function is acting as a blacklist rather than a
>> whitelist.
>
> Agreed, it would be nice to have all the checking logic in one place.
> If all the drivers end up implementing ndo_gso_check then we could
> potentially get rid of the GSO types as features. This probably
> wouldn't be a bad thing since we already know that the features
> mechanism doesn't scale (for instance there's no way to indicate that
> certain combinations of GSO types are supported by a device).
This crossed my mind and I agree that it's pretty clear that the
features mechanism isn't scaling very well. Presumably, the logical
extension of this is that each driver would have a function that looks
at a packet and returns a set of offload operations that it can
support rather than exposing a set of protocols. However, it seems
like it would probably result in a bunch of duplicate code in each
driver.
^ permalink raw reply
* Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"
From: Dan Carpenter @ 2014-12-02 18:35 UTC (permalink / raw)
To: Johannes Berg
Cc: Julia Lawall, SF Markus Elfring, Lino Sanfilippo, Olof Johansson,
netdev-u79uwXL29TY76Z2rM5mHXA, backports-u79uwXL29TY76Z2rM5mHXA,
LKML, kernel-janitors-u79uwXL29TY76Z2rM5mHXA, Luis R. Rodriguez
In-Reply-To: <1417539208.1841.1.camel-cdvu00un1VgdHxzADdlk8Q@public.gmane.org>
On Tue, Dec 02, 2014 at 05:53:28PM +0100, Johannes Berg wrote:
> On Mon, 2014-12-01 at 21:34 +0100, Julia Lawall wrote:
>
> > > So this kind of evolution is no problem for the (automated) backports
> > > using the backports project - although it can be difficult to detect
> > > such a thing is needed.
> >
> > That is exactly the problem...
>
> I'm not convinced though that it should stop such progress in mainline.
Is it progress? These patches match the code look simpler by passing
hiding the NULL check inside a function call. Calling pci_dev_put(NULL)
doesn't make sense. Just because a sanity check exists doesn't mean we
should do insane things.
It's easy enough to store which functions have a sanity check in a
database, but to rememember all that as a human being trying to read the
code is impossible.
If we really wanted to make this code cleaner we would introduce more
error labels with better names.
regards,
dan carpenter
^ permalink raw reply
* Re: [PATCH] net: mvneta: fix Tx interrupt delay
From: Eric Dumazet @ 2014-12-02 18:39 UTC (permalink / raw)
To: Ezequiel Garcia
Cc: Willy Tarreau, netdev, Maggie Mae Roxas, Thomas Petazzoni,
Gregory CLEMENT
In-Reply-To: <547DF2EA.2020908@free-electrons.com>
On Tue, 2014-12-02 at 14:12 -0300, Ezequiel Garcia wrote:
> Eric,
>
> On 12/02/2014 09:18 AM, Eric Dumazet wrote:
> [..]
> >
> > I am surprised TCP even worked correctly with this problem.
> >
> > I highly suggest BQL for this driver, now this issue is fixed.
> >
>
> Implementing BQL for the mvneta driver was something I wanted to do a
> while ago, but you explained that these kind drivers (i.e. those with
> software TSO) didn't need BQL, because the latency that resulted from
> the ring was too small.
>
> Quoting (http://www.spinics.net/lists/netdev/msg284439.html):
> ""
> Note that a full size TSO packet (44 or 45 MSS) requires about 88 or 90
> descriptors.
>
> So I do not think BQL is really needed, because a 512 slots TX ring wont
> add a big latency : About 5 ms max.
>
> BQL is generally nice for NIC supporting hardware TSO, where a 64KB TSO
> packet consumes 3 or 4 descriptors.
>
> Also note that TCP Small Queues should limit TX ring occupancy of a
> single bulk flow anyway.
> ""
>
> Maybe I misunderstood something?
This was indeed the case, but we added recently xmit_more support, and
it uses BQL information. So you might add BQL anyway, if xmit_more
support is useful for this hardware.
^ permalink raw reply
* [net_test_tools] udpflood: Add IPv6 support
From: Martin KaFai Lau @ 2014-12-02 18:41 UTC (permalink / raw)
To: davem; +Cc: netdev
In-Reply-To: <1413837765-5446-1-git-send-email-kafai@fb.com>
This patch:
1. Add IPv6 support
2. Print timing for every 65536 fib insert operations to observe
the gc effect (mostly for IPv6 fib).
---
udpflood.c | 125 +++++++++++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 101 insertions(+), 24 deletions(-)
diff --git a/udpflood.c b/udpflood.c
index 6e658f7..5855012 100644
--- a/udpflood.c
+++ b/udpflood.c
@@ -6,7 +6,9 @@
#include <string.h>
#include <errno.h>
#include <unistd.h>
+#include <stdint.h>
+#include <sys/time.h>
#include <sys/types.h>
#include <sys/socket.h>
#include <netinet/in.h>
@@ -15,57 +17,121 @@
#define _GNU_SOURCE
#include <getopt.h>
+static int debug = 0;
+
+typedef union sa_u {
+ struct sockaddr_in a4;
+ struct sockaddr_in6 a6;
+} sa_u;
+
static int usage(void)
{
printf("usage: udpflood [ -l count ] [ -m message_size ] [ -c num_ip_addrs ] IP_ADDRESS\n");
return -1;
}
-static int send_packets(in_addr_t start_addr, in_addr_t end_addr,
- int port, int count, int msg_sz)
+static uint32_t get_last32h(const sa_u *sa)
+{
+ if (sa->a4.sin_family == PF_INET)
+ return ntohl(sa->a4.sin_addr.s_addr);
+ else
+ return ntohl(sa->a6.sin6_addr.s6_addr32[3]);
+}
+
+static void set_last32h(sa_u *sa, uint32_t last32h)
+{
+ if (sa->a4.sin_family == PF_INET)
+ sa->a4.sin_addr.s_addr = htonl(last32h);
+ else
+ sa->a6.sin6_addr.s6_addr32[3] = htonl(last32h);
+}
+
+static void print_sa(const sa_u *sa, const char *msg)
+{
+ char buf[sizeof("xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx:xxxx")];
+
+ if (!debug)
+ return;
+
+ switch (sa->a4.sin_family) {
+ case PF_INET:
+ inet_ntop(PF_INET, &(sa->a4.sin_addr.s_addr), buf,
+ sizeof(buf));
+ break;
+ case PF_INET6:
+ inet_ntop(PF_INET6, sa->a6.sin6_addr.s6_addr, buf, sizeof(buf));
+ break;
+ }
+
+ printf("%s: %s\n", msg, buf);
+}
+
+static long get_diff_ms(const struct timeval *now,
+ const struct timeval *start)
+{
+ long start_ms, now_ms;
+ start_ms = start->tv_sec * 1000 + (start->tv_usec / 1000);
+ now_ms = now->tv_sec * 1000 + (now->tv_usec / 1000);
+ return now_ms - start_ms;
+}
+
+static int send_packets(const sa_u *start_sa, size_t num_addrs, int count,
+ int msg_sz)
{
char *msg = malloc(msg_sz);
- struct sockaddr_in saddr;
- in_addr_t addr;
+ sa_u cur_sa;
+ uint32_t start_addr32h, end_addr32h, cur_addr32h;
int fd, i, err;
+ struct timeval last, now;
if (!msg)
return -ENOMEM;
memset(msg, 0, msg_sz);
- addr = start_addr;
-
- memset(&saddr, 0, sizeof(saddr));
- saddr.sin_family = AF_INET;
- saddr.sin_port = port;
- saddr.sin_addr.s_addr = addr;
+ memcpy(&cur_sa, start_sa, sizeof(cur_sa));
+ cur_addr32h = start_addr32h = get_last32h(&cur_sa);
+ end_addr32h = start_addr32h + num_addrs;
- fd = socket(PF_INET, SOCK_DGRAM, IPPROTO_IP);
+ fd = socket(cur_sa.a4.sin_family, SOCK_DGRAM, IPPROTO_IP);
if (fd < 0) {
perror("socket");
err = fd;
goto out_nofd;
}
- err = connect(fd, (struct sockaddr *) &saddr, sizeof(saddr));
+ err = connect(fd, (struct sockaddr *) &cur_sa, sizeof(cur_sa));
if (err < 0) {
perror("connect");
- close(fd);
goto out;
}
+ print_sa(start_sa, "start_addr");
+ gettimeofday(&last, NULL);
for (i = 0; i < count; i++) {
- saddr.sin_addr.s_addr = addr;
-
+ print_sa(&cur_sa, "sendto");
err = sendto(fd, msg, msg_sz, 0,
- (struct sockaddr *) &saddr, sizeof(saddr));
+ (struct sockaddr *) &cur_sa, sizeof(cur_sa));
if (err < 0) {
perror("sendto");
goto out;
}
- if (++addr >= end_addr)
- addr = start_addr;
+ if (++cur_addr32h >= end_addr32h)
+ cur_addr32h = start_addr32h;
+ set_last32h(&cur_sa, cur_addr32h);
+
+ /*
+ * print timing info for every 65536 fib inserts to
+ * observe the gc effect (mostly for IPv6 fib).
+ */
+ if (i && (i & 0xFFFF) == 0) {
+ long diff_ms;
+ gettimeofday(&now, NULL);
+ diff_ms = get_diff_ms(&now, &last);
+ printf("%d %ld.%ld\n", i >> 16,
+ diff_ms / 1000, diff_ms % 1000);
+ memcpy(&last, &now, sizeof(last));
+ }
}
err = 0;
@@ -79,14 +145,14 @@ out_nofd:
int main(int argc, char **argv, char **envp)
{
int port, msg_sz, count, num_addrs, ret;
- in_addr_t start_addr, end_addr;
+ sa_u start_sa;
port = 6000;
msg_sz = 32;
count = 10000000;
num_addrs = 1;
- while ((ret = getopt(argc, argv, "l:s:p:c:")) >= 0) {
+ while ((ret = getopt(argc, argv, "dl:s:p:c:")) >= 0) {
switch (ret) {
case 'l':
sscanf(optarg, "%d", &count);
@@ -100,18 +166,29 @@ int main(int argc, char **argv, char **envp)
case 'c':
sscanf(optarg, "%d", &num_addrs);
break;
+ case 'd':
+ debug = 1;
+ break;
case '?':
return usage();
}
}
+ if (num_addrs < 1 || count < 1)
+ return usage();
+
if (!argv[optind])
return usage();
- start_addr = inet_addr(argv[optind]);
- if (start_addr == INADDR_NONE)
+ memset(&start_sa, 0, sizeof(start_sa));
+ start_sa.a4.sin_port = htons(port);
+ if (inet_pton(PF_INET, argv[optind], &start_sa.a4.sin_addr))
+ start_sa.a4.sin_family = PF_INET;
+ else if (inet_pton(PF_INET6, argv[optind],
+ start_sa.a6.sin6_addr.s6_addr))
+ start_sa.a6.sin6_family = PF_INET6;
+ else
return usage();
- end_addr = start_addr + num_addrs;
- return send_packets(start_addr, end_addr, port, count, msg_sz);
+ return send_packets(&start_sa, num_addrs, count, msg_sz);
}
--
1.8.1
^ permalink raw reply related
* Re: net-PA Semi: Deletion of unnecessary checks before the function call "pci_dev_put"
From: Luis R. Rodriguez @ 2014-12-02 18:45 UTC (permalink / raw)
To: Johannes Berg
Cc: Julia Lawall, SF Markus Elfring, Lino Sanfilippo, Olof Johansson,
netdev@vger.kernel.org, backports@vger.kernel.org, LKML,
kernel-janitors@vger.kernel.org
In-Reply-To: <1417539208.1841.1.camel@sipsolutions.net>
On Tue, Dec 2, 2014 at 11:53 AM, Johannes Berg
<johannes@sipsolutions.net> wrote:
> On Mon, 2014-12-01 at 21:34 +0100, Julia Lawall wrote:
>
>> > So this kind of evolution is no problem for the (automated) backports
>> > using the backports project - although it can be difficult to detect
>> > such a thing is needed.
>>
>> That is exactly the problem...
>
> I'm not convinced though that it should stop such progress in mainline.
I believe this case requires a bit more information explained as to
why it was explained. The "form" of change this patch has is of the
type that can crash systems if the NULL pointer check on the caller
implementation was only added later. We might be able to grammatically
check for this situation in the future if we had a white list / black
list / kernel revision where the NULL check was added but for now we
don't have that and as such care is just required on the developer in
consideration for backports.
It should be up to the maintainer to appreciate the gains of doing
something differently to make it easier for backporting. I obviously
think its a good thing to consider, its extra work though, so only if
the maintainer has some appreciation for backporting would this make
sense.
In this particular case I've reviewed Julia's concern and I've
determined that the patch is safe up to at least v2.6.12-rc2 (which is
where our git history begins on Linus' tree), this is because the
check for NULL has been there since then:
git show 1da177e drivers/pci/pci-driver.c
+void pci_dev_put(struct pci_dev *dev)
+{
+ if (dev)
+ put_device(&dev->dev);
+}
So this type of wide collateral evolution should not cause panics.
Because of this:
Acked-by: Luis R. Rodriguez <mcgrof@suse.com>
But note -- I still think its only good for us to vet these, if we
can't why not? If the maintainer doesn't give a shit that's different,
but if there are folks out there willing to help with vetting then
well, why not :)
PS. Including something like historical vetting as I did above on the
commit log should help folks.
Luis
^ permalink raw reply
* [net PATCH] fib_trie: Fix /proc/net/fib_trie when CONFIG_IP_MULTIPLE_TABLES is not defined
From: Alexander Duyck @ 2014-12-02 18:58 UTC (permalink / raw)
To: netdev; +Cc: davem
In recent testing I had disabled CONFIG_IP_MULTIPLE_TABLES and as a result
when I ran "cat /proc/net/fib_trie" the main trie was displayed multiple
times. I found that the problem line of code was in the function
fib_trie_seq_next. Specifically the line below caused the indexes to go in
the opposite direction of our traversal:
h = tb->tb_id & (FIB_TABLE_HASHSZ - 1);
This issue was that the RT tables are defined such that RT_TABLE_LOCAL is ID
255, while it is located at TABLE_LOCAL_INDEX of 0, and RT_TABLE_MAIN is 254
with a TABLE_MAIN_INDEX of 1. This means that the above line will return 1
for the local table and 0 for main. The result is that fib_trie_seq_next
will return NULL at the end of the local table, fib_trie_seq_start will
return the start of the main table, and then fib_trie_seq_next will loop on
main forever as h will always return 0.
The fix for this is to reverse the ordering of the two tables. It has the
advantage of making it so that the tables now print in the same order
regardless of if multiple tables are enabled or not. In order to make the
definition consistent with the multiple tables case I simply masked the to
RT_TABLE_XXX values by (FIB_TABLE_HASHSZ - 1). This way the two table
layouts should always stay consistent.
Fixes: 93456b6 ("[IPV4]: Unify access to the routing tables")
Signed-off-by: Alexander Duyck <alexander.h.duyck@redhat.com>
---
include/net/ip_fib.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/include/net/ip_fib.h b/include/net/ip_fib.h
index dc9d2a2..09a819e 100644
--- a/include/net/ip_fib.h
+++ b/include/net/ip_fib.h
@@ -201,8 +201,8 @@ void fib_free_table(struct fib_table *tb);
#ifndef CONFIG_IP_MULTIPLE_TABLES
-#define TABLE_LOCAL_INDEX 0
-#define TABLE_MAIN_INDEX 1
+#define TABLE_LOCAL_INDEX (RT_TABLE_LOCAL & (FIB_TABLE_HASHSZ - 1))
+#define TABLE_MAIN_INDEX (RT_TABLE_MAIN & (FIB_TABLE_HASHSZ - 1))
static inline struct fib_table *fib_get_table(struct net *net, u32 id)
{
^ permalink raw reply related
* Re: [PATCH net-next] rtnetlink: delay RTM_DELLINK notification until after ndo_uninit()
From: Alexei Starovoitov @ 2014-12-02 19:53 UTC (permalink / raw)
To: Thomas Graf
Cc: Mahesh Bandewar, netdev, David Miller, Eric Dumazet, Roopa Prabhu,
Toshiaki Makita
In-Reply-To: <20141202100746.GA13717@casper.infradead.org>
On Tue, Dec 2, 2014 at 2:07 AM, Thomas Graf <tgraf@suug.ch> wrote:
> On 12/01/14 at 09:54pm, Mahesh Bandewar wrote:
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -2220,8 +2220,16 @@ static int rtnl_dump_all(struct sk_buff *skb, struct netlink_callback *cb)
>> return skb->len;
>> }
>>
>> -void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
>> - gfp_t flags)
>> +void rtmsg_ifinfo_send(struct sk_buff *skb, struct net_device *dev, gfp_t flags)
>> +{
>> + struct net *net = dev_net(dev);
>> +
>> + rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, flags);
>> +}
>> +EXPORT_SYMBOL(rtmsg_ifinfo_send);
>> +
>> +struct sk_buff *rtmsg_ifinfo(int type, struct net_device *dev,
>> + unsigned int change, gfp_t flags, bool fill_only)
>> {
>> struct net *net = dev_net(dev);
>> struct sk_buff *skb;
>> @@ -2239,11 +2247,15 @@ void rtmsg_ifinfo(int type, struct net_device *dev, unsigned int change,
>> kfree_skb(skb);
>> goto errout;
>> }
>> + if (fill_only)
>> + return skb;
>> +
>> rtnl_notify(skb, net, 0, RTNLGRP_LINK, NULL, flags);
>> - return;
>> + return NULL;
>> errout:
>> if (err < 0)
>> rtnl_set_sk_err(net, RTNLGRP_LINK, err);
>> + return NULL;
>> }
>
> I think it would be cleaner to introduce a new function, for example
> rtmsg_ifinfo_build_skb() which is called from rtmsg_ifinfo(). The
> single caller that requires delayed sending can use the build skb
> function directly and then send it off.
+1
that would make patch much smaller.
^ permalink raw reply
* Re: [PATCH] SSB / B44: fix WOL for BCM4401
From: Andrey Skvortsov @ 2014-12-02 20:01 UTC (permalink / raw)
To: Michael Büsch
Cc: Rafael J. Wysocki, Gary.Zambrano, netdev, linux-kernel, b43-dev,
Rafał Miłecki, Larry Finger
In-Reply-To: <20141201221023.79ffb40d@wiggum>
[-- Attachment #1: Type: text/plain, Size: 1965 bytes --]
On Mon, Dec 01, 2014 at 10:10:23PM +0100, Michael Büsch wrote:
> On Mon, 1 Dec 2014 23:46:38 +0300
> Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
>
> > Wake On Lan was not working on laptop DELL Vostro 1500.
> > If WOL was turned on, BCM4401 was powered up in suspend mode. LEDs blinked.
> > But the laptop could not be woken up with the Magic Packet. The reason for
> > that was that PCIE was not enabled as a system wakeup source and
> > therefore the host PCI bridge was not powered up in suspend mode.
> > PCIE was not enabled in suspend by PM because no child devices were
> > registered as wakeup source during suspend process.
> > On laptop BCM4401 is connected through the SSB bus, that is connected to the
> > PCI-Express bus. SSB and B44 did not use standard PM wakeup functions
> > and did not forward wakeup settings to their parents.
> > To fix that B44 driver enables PM wakeup and registers new wakeup source
> > using device_set_wakeup_enable(). Wakeup is automatically reported to the parent SSB
> > bus via power.wakeup_path. SSB bus enables wakeup for the parent PCI bridge, if there is any
> > child devices with enabled wakeup functionality. All other steps are
> > done by PM core code.
>
> Thanks, this looks good.
> I assume you tested this (I currently don't have a device to test this).
Sure, I've tested it. WOL from suspend is working and after resume from hibernate Ethernet is working too.
> Larry, Rafał, any other b43 user:
> Can you please test whether this doesn't cause regressions for suspend/resume on b43?
> (Patch is attached as reference)
>
>
> > Signed-off-by: Andrey Skvortsov <Andrej.Skvortzov@gmail.com>
> > ---
> > drivers/net/ethernet/broadcom/b44.c | 2 ++
> > drivers/ssb/pcihost_wrapper.c | 33 ++++++++++++++++++++++-----------
> > 2 files changed, 24 insertions(+), 11 deletions(-)
--
Best regards,
Andrey Skvortsov
PGP Key ID: 0x57A3AEAD
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH] SSB / B44: fix WOL for BCM4401
From: Michael Büsch @ 2014-12-02 20:12 UTC (permalink / raw)
To: Andrey Skvortsov
Cc: Rafael J. Wysocki, Gary.Zambrano, netdev, linux-kernel, b43-dev,
Rafał Miłecki, Larry Finger
In-Reply-To: <20141202200129.GA4580@crion89>
[-- Attachment #1: Type: text/plain, Size: 1711 bytes --]
On Tue, 2 Dec 2014 23:01:29 +0300
Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> On Mon, Dec 01, 2014 at 10:10:23PM +0100, Michael Büsch wrote:
> > On Mon, 1 Dec 2014 23:46:38 +0300
> > Andrey Skvortsov <andrej.skvortzov@gmail.com> wrote:
> >
> > > Wake On Lan was not working on laptop DELL Vostro 1500.
> > > If WOL was turned on, BCM4401 was powered up in suspend mode. LEDs blinked.
> > > But the laptop could not be woken up with the Magic Packet. The reason for
> > > that was that PCIE was not enabled as a system wakeup source and
> > > therefore the host PCI bridge was not powered up in suspend mode.
> > > PCIE was not enabled in suspend by PM because no child devices were
> > > registered as wakeup source during suspend process.
> > > On laptop BCM4401 is connected through the SSB bus, that is connected to the
> > > PCI-Express bus. SSB and B44 did not use standard PM wakeup functions
> > > and did not forward wakeup settings to their parents.
> > > To fix that B44 driver enables PM wakeup and registers new wakeup source
> > > using device_set_wakeup_enable(). Wakeup is automatically reported to the parent SSB
> > > bus via power.wakeup_path. SSB bus enables wakeup for the parent PCI bridge, if there is any
> > > child devices with enabled wakeup functionality. All other steps are
> > > done by PM core code.
> >
> > Thanks, this looks good.
> > I assume you tested this (I currently don't have a device to test this).
>
> Sure, I've tested it. WOL from suspend is working and after resume from hibernate Ethernet is working too.
That sounds good, indeed.
I'd still prefer, if someone with b43 (wireless) would test it, too.
--
Michael
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ 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