* [PATCH net] phylink: ensure AN is enabled
From: Russell King @ 2017-12-20 23:21 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
Ensure that we mark AN as enabled at boot time, rather than leaving
it disabled. This is noticable if your SFP module is fiber, and you
it supports faster speeds than 1G with 2.5G support in place.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index e30339fca5cf..db5d5726ced9 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -567,6 +567,7 @@ struct phylink *phylink_create(struct net_device *ndev,
pl->link_config.pause = MLO_PAUSE_AN;
pl->link_config.speed = SPEED_UNKNOWN;
pl->link_config.duplex = DUPLEX_UNKNOWN;
+ pl->link_config.an_enabled = true;
pl->ops = ops;
__set_bit(PHYLINK_DISABLE_STOPPED, &pl->phylink_disable_state);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next] phylink: avoid attaching more than one PHY
From: Russell King @ 2017-12-20 23:23 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli; +Cc: netdev
Attaching more than one PHY to phylink is bad news, as we store a
pointer to the PHY in a single location. Error out if more than one
PHY is attempted to be attached.
Signed-off-by: Russell King <rmk+kernel@armlinux.org.uk>
---
drivers/net/phy/phylink.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index db5d5726ced9..82166a26f5c6 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -726,6 +726,9 @@ int phylink_connect_phy(struct phylink *pl, struct phy_device *phy)
phy_interface_mode_is_8023z(pl->link_interface))))
return -EINVAL;
+ if (pl->phydev)
+ return -EBUSY;
+
/* Use PHY device/driver interface */
if (pl->link_interface == PHY_INTERFACE_MODE_NA) {
pl->link_interface = phy->interface;
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] net: Revert "net_sched: no need to free qdisc in RCU callback"
From: Cong Wang @ 2017-12-20 23:23 UTC (permalink / raw)
To: John Fastabend
Cc: Jiri Pirko, David Miller, Jakub Kicinski,
Linux Kernel Network Developers, Eric Dumazet
In-Reply-To: <d6892431-21bc-254f-cc29-acf28d00af20@gmail.com>
On Wed, Dec 20, 2017 at 3:05 PM, John Fastabend
<john.fastabend@gmail.com> wrote:
> On 12/20/2017 02:41 PM, Cong Wang wrote:
>> On Wed, Dec 20, 2017 at 12:09 PM, John Fastabend
>> <john.fastabend@gmail.com> wrote:
>>> RCU grace period is needed for lockless qdiscs added in the commit
>>> c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array").
>>>
>>> It is needed now that qdiscs may be lockless otherwise we risk
>>> free'ing a qdisc that is still in use from datapath. Additionally,
>>> push list cleanup into RCU callback. Otherwise we risk the datapath
>>> adding skbs during removal.
>>
>> What about qdisc_graft() -> dev_deactivate() -> synchronize_net() ?
>> It doesn't work with your "lockless" patches?
>>
>
> Well this is only in the 'parent == NULL' case otherwise we call
> cops->graft(). Most sch_* seem to use qdisc_replace and this uses
> sch_tree_lock().
>
> The only converted qdisc mq and mqprio at this point don't care
> though and do their own dev_deactivate/activate. So its not fixing
> anything in the above mentioned commit.
Sure, removing a class does not impact the whole device,
but removing the root qdisc does.
After your "lockless", skb_array_consume_bh() is called in
pfifo_fast_reset() and ptr_ring_cleanup() is called in
pfifo_fast_destroy(), assuming skb_array is not buggy, what race
do we have here with datapath?
>
> I still think it will need to be done eventually. If it resolves
> the miniq case it seems like a good idea. Although per Jakub's comment
> perhaps I pulled too much into the RCU handler.
The case Jakub reported is a RCU callback missing a rcu
barrier. I don't understand why you keep believing it is RCU
readers on datapath.
Not even to mention ingress is not affected by your "lockless"
thing.
^ permalink raw reply
* Re: [PATCH] net: Revert "net_sched: no need to free qdisc in RCU callback"
From: John Fastabend @ 2017-12-20 23:34 UTC (permalink / raw)
To: Cong Wang
Cc: Jiri Pirko, David Miller, Jakub Kicinski,
Linux Kernel Network Developers, Eric Dumazet
In-Reply-To: <CAM_iQpVQF5MosmXqhfxtnH9kh96MEHd0-kO8SO3TbCpywOzv+g@mail.gmail.com>
On 12/20/2017 03:23 PM, Cong Wang wrote:
> On Wed, Dec 20, 2017 at 3:05 PM, John Fastabend
> <john.fastabend@gmail.com> wrote:
>> On 12/20/2017 02:41 PM, Cong Wang wrote:
>>> On Wed, Dec 20, 2017 at 12:09 PM, John Fastabend
>>> <john.fastabend@gmail.com> wrote:
>>>> RCU grace period is needed for lockless qdiscs added in the commit
>>>> c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array").
>>>>
>>>> It is needed now that qdiscs may be lockless otherwise we risk
>>>> free'ing a qdisc that is still in use from datapath. Additionally,
>>>> push list cleanup into RCU callback. Otherwise we risk the datapath
>>>> adding skbs during removal.
>>>
>>> What about qdisc_graft() -> dev_deactivate() -> synchronize_net() ?
>>> It doesn't work with your "lockless" patches?
>>>
>>
>> Well this is only in the 'parent == NULL' case otherwise we call
>> cops->graft(). Most sch_* seem to use qdisc_replace and this uses
>> sch_tree_lock().
>>
>> The only converted qdisc mq and mqprio at this point don't care
>> though and do their own dev_deactivate/activate. So its not fixing
>> anything in the above mentioned commit.
>
> Sure, removing a class does not impact the whole device,
> but removing the root qdisc does.
>
> After your "lockless", skb_array_consume_bh() is called in
> pfifo_fast_reset() and ptr_ring_cleanup() is called in
> pfifo_fast_destroy(), assuming skb_array is not buggy, what race
> do we have here with datapath?
>
None at the moment.
>
>>
>> I still think it will need to be done eventually. If it resolves
>> the miniq case it seems like a good idea. Although per Jakub's comment
>> perhaps I pulled too much into the RCU handler.
>
> The case Jakub reported is a RCU callback missing a rcu
> barrier. I don't understand why you keep believing it is RCU
> readers on datapath.>
> Not even to mention ingress is not affected by your "lockless"
> thing.
>
I was thinking about the case where we want a lockless qdisc
with classes. Doing the qdisc destroy after a grace period would
solve this. Also we could start to cleanup a lot of the locking
and extra bits around 'running' qdisc and such by doing a clean
xchg on the qdisc layer. It seems that a dev_activate/deactivate
just to install a new qdisc is not needed.
Anyways future work. However if it resolves the miniq issue, as
Jiri indicated, seems like a clean fix. Although Jakub's issue
with the patch would need to be addressed. Seems he gets a WARN_ON
if the offload is not disabled but the device is unitialized.
.John
^ permalink raw reply
* Re: [net] Revert "net: core: maybe return -EEXIST in __dev_alloc_name"
From: Rasmus Villemoes @ 2017-12-20 23:37 UTC (permalink / raw)
To: Michael Ellerman
Cc: Johannes Berg, netdev@vger.kernel.org, Jouni Malinen,
Johannes Berg, linuxppc-dev@lists.ozlabs.org
In-Reply-To: <87vah29a1m.fsf@concordia.ellerman.id.au>
On Tue, Dec 19 2017, Michael Ellerman <michael@concordia.ellerman.id.au> wrote:
> Hi Johannes,
>
>> From: Johannes Berg <johannes.berg@intel.com>
>>
>> This reverts commit d6f295e9def0; some userspace (in the case
>
> This revert seems to have broken networking on one of my powerpc
> machines, according to git bisect.
>
> The symptom is DHCP fails and I don't get a link, I didn't dig any
> further than that. I can if it's helpful.
>
> I think the problem is that 87c320e51519 ("net: core: dev_get_valid_name
> is now the same as dev_alloc_name_ns") only makes sense while
> d6f295e9def0 remains in the tree.
I'm sorry about all of this, I really didn't think there would be such
consequences of changing an errno return. Indeed, d6f29 was preparation
for unifying the two functions that do the exact same thing (and how we
ever got into that situation is somewhat unclear), except for
their behaviour in the case the requested name already exists. So one of
the two interfaces had to change its return value, and as I wrote, I
thought EEXIST was the saner choice when an explicit name (no %d) had
been requested.
> ie. before the entire series, dev_get_valid_name() would return EEXIST,
> and that was retained when 87c320e51519 was merged, but now that
> d6f295e9def0 has been reverted dev_get_valid_name() is returning ENFILE.
>
> I can get the network up again if I also revert 87c320e51519 ("net:
> core: dev_get_valid_name is now the same as dev_alloc_name_ns"), or with
> the gross patch below.
I don't think changing -ENFILE to -EEXIST would be right either, since
dev_get_valid_name() used to be able to return both (-EEXIST in the case
where there's no %d, -ENFILE in the case where we end up calling
dev_alloc_name_ns()). If anything, we could do the check for the old
-EEXIST condition first, and then call dev_alloc_name_ns(). But I'm also
fine with reverting.
Again, sorry :(
Rasmus
^ permalink raw reply
* Re: [PATCH] net: Revert "net_sched: no need to free qdisc in RCU callback"
From: John Fastabend @ 2017-12-20 23:40 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: xiyou.wangcong, jiri, davem, netdev, eric.dumazet
In-Reply-To: <20171220135959.3ff075ac@cakuba.netronome.com>
On 12/20/2017 01:59 PM, Jakub Kicinski wrote:
> On Wed, 20 Dec 2017 12:09:19 -0800, John Fastabend wrote:
>> RCU grace period is needed for lockless qdiscs added in the commit
>> c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array").
>>
>> It is needed now that qdiscs may be lockless otherwise we risk
>> free'ing a qdisc that is still in use from datapath. Additionally,
>> push list cleanup into RCU callback. Otherwise we risk the datapath
>> adding skbs during removal.
>>
>> Fixes: c5ad119fb6c09 ("net: sched: pfifo_fast use skb_array")
>> Signed-off-by: John Fastabend <john.fastabend@gmail.com>
>
> Seems like this revert may be too heavy handed:
>
> # ./tools/testing/selftests/bpf/test_offload.py --log /tmp/log
> Test destruction of generic XDP...
> Test TC non-offloaded...
> Test TC non-offloaded isn't getting bound...
> Test TC offloads are off by default...
> Test TC offload by default...
> Test TC cBPF bytcode tries offload by default...
> Test TC cBPF unbound bytecode doesn't offload...
> Test TC offloads work...
> FAIL: TC filter did not load with TC offloads enabled
>
> And it's triggering:
>
> WARNING: CPU: 15 PID: 1853 at ../drivers/net/netdevsim/bpf.c:372 nsim_bpf_uninit+0x2e/0x41 [netdevsim]
>
> Which is:
>
> 368 void nsim_bpf_uninit(struct netdevsim *ns)
> 369 {
> 370 WARN_ON(!list_empty(&ns->bpf_bound_progs));
> 371 WARN_ON(ns->xdp_prog);
>>> 372 WARN_ON(ns->bpf_offloaded);
> 373 }
>
> (Meaning the offload was not stopped by the stack before ndo_uninit.)
>
Dang. So offload code depends on destroy being called on a qdisc
to in turn destroy the filters and unbind any offloads.
I was hoping I could get away with tearing down live qdiscs without
too much work. Looks like not.
Note the fixes tag was bogus nothing is actually broken in current
code until a lockless qdisc with classes shows up.
.John
^ permalink raw reply
* linux-next: build failure after merge of the net-next tree
From: Stephen Rothwell @ 2017-12-20 23:43 UTC (permalink / raw)
To: David Miller, Networking
Cc: Linux-Next Mailing List, Linux Kernel Mailing List,
Jakub Kicinski, Daniel Borkmann
Hi all,
After merging the net-next tree, today's linux-next build (x86_64
allmodconfig) failed like this:
drivers/net/netdevsim/bpf.c: In function 'nsim_bpf_setup_tc_block_cb':
drivers/net/netdevsim/bpf.c:127:7: error: 'TC_CLSBPF_REPLACE' undeclared (first use in this function)
case TC_CLSBPF_REPLACE:
^
drivers/net/netdevsim/bpf.c:129:7: error: 'TC_CLSBPF_ADD' undeclared (first use in this function)
case TC_CLSBPF_ADD:
^
drivers/net/netdevsim/bpf.c:131:7: error: 'TC_CLSBPF_DESTROY' undeclared (first use in this function)
case TC_CLSBPF_DESTROY:
^
Caused by commit
31d3ad832948 ("netdevsim: add bpf offload support")
interacting with commit
102740bd9436 ("cls_bpf: fix offload assumptions after callback conversion")
from the net tree.
I applied the following merge fix patch:
From: Stephen Rothwell <sfr@canb.auug.org.au>
Date: Thu, 21 Dec 2017 10:18:46 +1100
Subject: [PATCH] netdevsim: fix up for "cls_bpf: fix offload assumptions after
callback conversion"
Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
---
drivers/net/netdevsim/bpf.c | 12 +++---------
1 file changed, 3 insertions(+), 9 deletions(-)
diff --git a/drivers/net/netdevsim/bpf.c b/drivers/net/netdevsim/bpf.c
index 7da814686ad9..afaf980bbbe7 100644
--- a/drivers/net/netdevsim/bpf.c
+++ b/drivers/net/netdevsim/bpf.c
@@ -123,16 +123,10 @@ int nsim_bpf_setup_tc_block_cb(enum tc_setup_type type,
if (prog && !prog->aux->offload && !ns->bpf_tc_non_bound_accept)
return -EOPNOTSUPP;
- switch (cls_bpf->command) {
- case TC_CLSBPF_REPLACE:
- return nsim_bpf_offload(ns, prog, true);
- case TC_CLSBPF_ADD:
- return nsim_bpf_offload(ns, prog, false);
- case TC_CLSBPF_DESTROY:
- return nsim_bpf_offload(ns, NULL, true);
- default:
+ if (cls_bpf->command != TC_CLSBPF_OFFLOAD)
return -EOPNOTSUPP;
- }
+
+ return nsim_bpf_offload(ns, prog, cls_bpf->oldprog);
}
int nsim_bpf_disable_tc(struct netdevsim *ns)
--
2.15.0
--
Cheers,
Stephen Rothwell
^ permalink raw reply related
* Re: linux-next: build failure after merge of the net-next tree
From: Jakub Kicinski @ 2017-12-20 23:59 UTC (permalink / raw)
To: Stephen Rothwell
Cc: David Miller, Networking, Linux-Next Mailing List,
Linux Kernel Mailing List, Daniel Borkmann
In-Reply-To: <20171221104304.38f94a00@canb.auug.org.au>
On Thu, 21 Dec 2017 10:43:04 +1100, Stephen Rothwell wrote:
> Hi all,
>
> After merging the net-next tree, today's linux-next build (x86_64
> allmodconfig) failed like this:
>
> drivers/net/netdevsim/bpf.c: In function 'nsim_bpf_setup_tc_block_cb':
> drivers/net/netdevsim/bpf.c:127:7: error: 'TC_CLSBPF_REPLACE' undeclared (first use in this function)
> case TC_CLSBPF_REPLACE:
> ^
> drivers/net/netdevsim/bpf.c:129:7: error: 'TC_CLSBPF_ADD' undeclared (first use in this function)
> case TC_CLSBPF_ADD:
> ^
> drivers/net/netdevsim/bpf.c:131:7: error: 'TC_CLSBPF_DESTROY' undeclared (first use in this function)
> case TC_CLSBPF_DESTROY:
> ^
>
> Caused by commit
>
> 31d3ad832948 ("netdevsim: add bpf offload support")
>
> interacting with commit
>
> 102740bd9436 ("cls_bpf: fix offload assumptions after callback conversion")
>
> from the net tree.
>
> I applied the following merge fix patch:
>
> From: Stephen Rothwell <sfr@canb.auug.org.au>
> Date: Thu, 21 Dec 2017 10:18:46 +1100
> Subject: [PATCH] netdevsim: fix up for "cls_bpf: fix offload assumptions after
> callback conversion"
>
> Signed-off-by: Stephen Rothwell <sfr@canb.auug.org.au>
Hi Stephen, sorry about those merges today. The proper fix is queued
up in patchwork for when net-next is merged in to net:
http://patchwork.ozlabs.org/patch/851063/
I will CC you on patches which may cause/fix merge trouble in the
future, often the information about how to resolve the conflict is
not part of the commit message.
Sorry I didn't think about CCing you earlier!
^ permalink raw reply
* Re: RCU callback crashes
From: Cong Wang @ 2017-12-21 0:03 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jiri Pirko, netdev@vger.kernel.org
In-Reply-To: <CAM_iQpVPUifm3rcXu8SP9ShSmm7z9z+8UjppdY_AxMYQwHE9YQ@mail.gmail.com>
On Wed, Dec 20, 2017 at 10:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Dec 20, 2017 at 10:17 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
>>
>> I guess it is q->miniqp which is freed in qdisc_graft() without properly
>> waiting for rcu readers?
>
> It is probably so, the call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func)
> in the end of mini_qdisc_pair_swap() is invoked on miniq_old->rcu,
> but miniq is being freed, no rcu barrier waits for it...
>
> You can try to add a rcu_barrier_bh() at the end to see if this crash
> is gone, but I don't think people like adding yet another rcu barrier...
Hi, Jakub
Can you test the following fix? I am not a fan of rcu barrier but we
already have one so...
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 876fab2604b8..1b68fedea124 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1240,6 +1240,8 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
if (!tp_head) {
RCU_INIT_POINTER(*miniqp->p_miniq, NULL);
+ /* Wait for existing flying RCU callback before being freed. */
+ rcu_barrier_bh();
return;
}
^ permalink raw reply related
* Re: RCU callback crashes
From: Jakub Kicinski @ 2017-12-21 0:08 UTC (permalink / raw)
To: Cong Wang; +Cc: Jiri Pirko, netdev@vger.kernel.org
In-Reply-To: <CAM_iQpUngX+oSDiforfZceqMZrg=jDJnNf3QFF9WFQdHrU9o-g@mail.gmail.com>
On Wed, 20 Dec 2017 16:03:49 -0800, Cong Wang wrote:
> On Wed, Dec 20, 2017 at 10:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Wed, Dec 20, 2017 at 10:17 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >>
> >> I guess it is q->miniqp which is freed in qdisc_graft() without properly
> >> waiting for rcu readers?
> >
> > It is probably so, the call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func)
> > in the end of mini_qdisc_pair_swap() is invoked on miniq_old->rcu,
> > but miniq is being freed, no rcu barrier waits for it...
> >
> > You can try to add a rcu_barrier_bh() at the end to see if this crash
> > is gone, but I don't think people like adding yet another rcu barrier...
>
> Hi, Jakub
>
> Can you test the following fix? I am not a fan of rcu barrier but we
> already have one so...
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 876fab2604b8..1b68fedea124 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1240,6 +1240,8 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
>
> if (!tp_head) {
> RCU_INIT_POINTER(*miniqp->p_miniq, NULL);
> + /* Wait for existing flying RCU callback before being freed. */
> + rcu_barrier_bh();
> return;
> }
Mm.. I was running with this hack for the last two hours and it was OK:
diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
index 876fab2604b8..d7e0c3ad0a1c 100644
--- a/net/sched/sch_generic.c
+++ b/net/sched/sch_generic.c
@@ -1260,6 +1260,7 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
* are not seeing it.
*/
call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func);
+ rcu_barrier_bh();
}
EXPORT_SYMBOL(mini_qdisc_pair_swap);
Let me try to move the barrier...
^ permalink raw reply related
* Re: [PATCH net-next v4 0/6] net: tcp: sctp: dccp: Replace jprobe usage with trace events
From: Masami Hiramatsu @ 2017-12-21 0:10 UTC (permalink / raw)
To: David Miller
Cc: mingo, ian.mcdonald, vyasevich, stephen, rostedt, peterz, tglx,
linux-kernel, hpa, gerrit, nhorman, dccp, netdev, linux-sctp, sfr
In-Reply-To: <20171220.142424.1214953044518214833.davem@davemloft.net>
On Wed, 20 Dec 2017 14:24:24 -0500 (EST)
David Miller <davem@davemloft.net> wrote:
> From: David Miller <davem@davemloft.net>
> Date: Wed, 20 Dec 2017 14:20:40 -0500 (EST)
>
> > From: Masami Hiramatsu <mhiramat@kernel.org>
> > Date: Wed, 20 Dec 2017 13:14:11 +0900
> >
> >> This series is v4 of the replacement of jprobe usage with trace
> >> events. This version is rebased on net-next, fixes a build warning
> >> and moves a temporal variable definition in a block.
> >>
> >> Previous version is here;
> >> https://lkml.org/lkml/2017/12/19/153
> >>
> >> Changes from v3:
> >> All: Rebased on net-next
> >> [3/6]: fixes a build warning for i386 by casting pointer unsigned
> >> long instead of __u64, and moves a temporal variable
> >> definition in a block.
> >
> > Looks good, series applied to net-next, thanks.
>
> Actually, this doesn't even compile, so I've reverted:
>
> [davem@dhcp-10-15-49-227 net-next]$ make -s -j16
> In file included from net/dccp/trace.h:105:0,
> from net/dccp/proto.c:42:
> ./include/trace/define_trace.h:89:42: fatal error: ./trace.h: No such file or directory
> #include TRACE_INCLUDE(TRACE_INCLUDE_FILE)
> ^
> compilation terminated.
Hmm, strange.
I could compile it on x86-64 and i386. Let me check what was wrong.
Thanks,
--
Masami Hiramatsu <mhiramat@kernel.org>
^ permalink raw reply
* Re: [patch iproute2] tc: add -bs option for batch mode
From: David Ahern @ 2017-12-21 0:13 UTC (permalink / raw)
To: Stephen Hemminger, Chris Mi; +Cc: netdev@vger.kernel.org, gerlitz.or@gmail.com
In-Reply-To: <20171220071744.25f9dd41@xeon-e3>
On 12/20/17 8:17 AM, Stephen Hemminger wrote:
> On Wed, 20 Dec 2017 09:23:34 +0000
> Chris Mi <chrism@mellanox.com> wrote:
>
>>> Your real performance win is just not asking for ACK for every rule.
>> No. Even if batch_size > 1, we ack every rule. The real performance win is
>> to send multiple rules in one system call. If we are not asking for ACK for every rule,
>> the performance will be improved further.
>
> Try the no ACK method.
>
> When we were optimizing routing daemons like Quagga, it was discovered
> that an ACK for every route insert was the main bottleneck. Doing asynchronous
> error handling got a bigger win than your batching.
>
> Please try that, doing multiple messages using iov is not necessary.
>
FWIW, I plan to look at batching routes in a similar fashion.
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Michael S. Tsirkin @ 2017-12-21 0:14 UTC (permalink / raw)
To: Sridhar Samudrala; +Cc: stephen, netdev, virtualization, alexander.duyck
In-Reply-To: <1513644036-45230-1-git-send-email-sridhar.samudrala@intel.com>
On Mon, Dec 18, 2017 at 04:40:36PM -0800, Sridhar Samudrala wrote:
> This patch enables virtio to switch over to a VF datapath when a VF netdev
> is present with the same MAC address.
I prefer saying "a passthrough device" here. Does not have to be a VF at
all.
> It allows live migration of a VM
> with a direct attached VF without the need to setup a bond/team between a
> VF and virtio net device in the guest.
>
> The hypervisor needs to unplug the VF device from the guest on the source
> host and reset the MAC filter of the VF to initiate failover of datapath to
> virtio before starting the migration. After the migration is completed, the
> destination hypervisor sets the MAC filter on the VF and plugs it back to
> the guest to switch over to VF datapath.
>
> It is entirely based on netvsc implementation and it should be possible to
> make this code generic and move it to a common location that can be shared
> by netvsc and virtio.
>
> Also, i think we should make this a negotiated feature that is off by
> default via a new feature bit.
So please include this. A copy needs to go to virtio TC
to reserve the bit. Enabling this by default risks breaking
too many configurations.
>
> This patch is based on the discussion initiated by Jesse on this thread.
> https://marc.info/?l=linux-virtualization&m=151189725224231&w=2
>
> Signed-off-by: Sridhar Samudrala <sridhar.samudrala@intel.com>
> Reviewed-by: Jesse Brandeburg <jesse.brandeburg@intel.com>
> ---
> drivers/net/virtio_net.c | 341 ++++++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 339 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 559b215c0169..a34c717bb15b 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -31,6 +31,8 @@
> #include <linux/average.h>
> #include <linux/filter.h>
> #include <net/route.h>
> +#include <linux/netdevice.h>
> +#include <linux/netpoll.h>
>
> static int napi_weight = NAPI_POLL_WEIGHT;
> module_param(napi_weight, int, 0444);
> @@ -56,6 +58,8 @@ module_param(napi_tx, bool, 0644);
> */
> DECLARE_EWMA(pkt_len, 0, 64)
>
> +#define VF_TAKEOVER_INT (HZ / 10)
> +
> #define VIRTNET_DRIVER_VERSION "1.0.0"
>
> static const unsigned long guest_offloads[] = {
> @@ -117,6 +121,15 @@ struct receive_queue {
> char name[40];
> };
>
> +struct virtnet_vf_pcpu_stats {
> + u64 rx_packets;
> + u64 rx_bytes;
> + u64 tx_packets;
> + u64 tx_bytes;
> + struct u64_stats_sync syncp;
> + u32 tx_dropped;
> +};
> +
> struct virtnet_info {
> struct virtio_device *vdev;
> struct virtqueue *cvq;
> @@ -179,6 +192,11 @@ struct virtnet_info {
> u32 speed;
>
> unsigned long guest_offloads;
> +
> + /* State to manage the associated VF interface. */
> + struct net_device __rcu *vf_netdev;
> + struct virtnet_vf_pcpu_stats __percpu *vf_stats;
> + struct delayed_work vf_takeover;
> };
>
> struct padded_vnet_hdr {
> @@ -1300,16 +1318,51 @@ static int xmit_skb(struct send_queue *sq, struct sk_buff *skb)
> return virtqueue_add_outbuf(sq->vq, sq->sg, num_sg, skb, GFP_ATOMIC);
> }
>
> +/* Send skb on the slave VF device. */
> +static int virtnet_vf_xmit(struct net_device *dev, struct net_device *vf_netdev,
> + struct sk_buff *skb)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + unsigned int len = skb->len;
> + int rc;
> +
> + skb->dev = vf_netdev;
> + skb->queue_mapping = qdisc_skb_cb(skb)->slave_dev_queue_mapping;
> +
> + rc = dev_queue_xmit(skb);
> + if (likely(rc == NET_XMIT_SUCCESS || rc == NET_XMIT_CN)) {
> + struct virtnet_vf_pcpu_stats *pcpu_stats
> + = this_cpu_ptr(vi->vf_stats);
> +
> + u64_stats_update_begin(&pcpu_stats->syncp);
> + pcpu_stats->tx_packets++;
> + pcpu_stats->tx_bytes += len;
> + u64_stats_update_end(&pcpu_stats->syncp);
> + } else {
> + this_cpu_inc(vi->vf_stats->tx_dropped);
> + }
> +
> + return rc;
> +}
> +
> static netdev_tx_t start_xmit(struct sk_buff *skb, struct net_device *dev)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> int qnum = skb_get_queue_mapping(skb);
> struct send_queue *sq = &vi->sq[qnum];
> + struct net_device *vf_netdev;
> int err;
> struct netdev_queue *txq = netdev_get_tx_queue(dev, qnum);
> bool kick = !skb->xmit_more;
> bool use_napi = sq->napi.weight;
>
> + /* if VF is present and up then redirect packets
> + * called with rcu_read_lock_bh
> + */
> + vf_netdev = rcu_dereference_bh(vi->vf_netdev);
> + if (vf_netdev && netif_running(vf_netdev) && !netpoll_tx_running(dev))
> + return virtnet_vf_xmit(dev, vf_netdev, skb);
> +
> /* Free up any pending old buffers before queueing new ones. */
> free_old_xmit_skbs(sq);
>
> @@ -1456,10 +1509,41 @@ static int virtnet_set_mac_address(struct net_device *dev, void *p)
> return ret;
> }
>
> +static void virtnet_get_vf_stats(struct net_device *dev,
> + struct virtnet_vf_pcpu_stats *tot)
> +{
> + struct virtnet_info *vi = netdev_priv(dev);
> + int i;
> +
> + memset(tot, 0, sizeof(*tot));
> +
> + for_each_possible_cpu(i) {
> + const struct virtnet_vf_pcpu_stats *stats
> + = per_cpu_ptr(vi->vf_stats, i);
> + u64 rx_packets, rx_bytes, tx_packets, tx_bytes;
> + unsigned int start;
> +
> + do {
> + start = u64_stats_fetch_begin_irq(&stats->syncp);
> + rx_packets = stats->rx_packets;
> + tx_packets = stats->tx_packets;
> + rx_bytes = stats->rx_bytes;
> + tx_bytes = stats->tx_bytes;
> + } while (u64_stats_fetch_retry_irq(&stats->syncp, start));
> +
> + tot->rx_packets += rx_packets;
> + tot->tx_packets += tx_packets;
> + tot->rx_bytes += rx_bytes;
> + tot->tx_bytes += tx_bytes;
> + tot->tx_dropped += stats->tx_dropped;
> + }
> +}
> +
> static void virtnet_stats(struct net_device *dev,
> struct rtnl_link_stats64 *tot)
> {
> struct virtnet_info *vi = netdev_priv(dev);
> + struct virtnet_vf_pcpu_stats vf_stats;
> int cpu;
> unsigned int start;
>
> @@ -1490,6 +1574,13 @@ static void virtnet_stats(struct net_device *dev,
> tot->rx_dropped = dev->stats.rx_dropped;
> tot->rx_length_errors = dev->stats.rx_length_errors;
> tot->rx_frame_errors = dev->stats.rx_frame_errors;
> +
> + virtnet_get_vf_stats(dev, &vf_stats);
> + tot->rx_packets += vf_stats.rx_packets;
> + tot->tx_packets += vf_stats.tx_packets;
> + tot->rx_bytes += vf_stats.rx_bytes;
> + tot->tx_bytes += vf_stats.tx_bytes;
> + tot->tx_dropped += vf_stats.tx_dropped;
> }
>
> #ifdef CONFIG_NET_POLL_CONTROLLER
> @@ -2508,6 +2599,47 @@ static int virtnet_validate(struct virtio_device *vdev)
> return 0;
> }
>
> +static void __virtnet_vf_setup(struct net_device *ndev,
> + struct net_device *vf_netdev)
> +{
> + int ret;
> +
> + /* Align MTU of VF with master */
> + ret = dev_set_mtu(vf_netdev, ndev->mtu);
> + if (ret)
> + netdev_warn(vf_netdev,
> + "unable to change mtu to %u\n", ndev->mtu);
> +
> + if (netif_running(ndev)) {
> + ret = dev_open(vf_netdev);
> + if (ret)
> + netdev_warn(vf_netdev,
> + "unable to open: %d\n", ret);
> + }
> +}
> +
> +/* Setup VF as slave of the virtio device.
> + * Runs in workqueue to avoid recursion in netlink callbacks.
> + */
> +static void virtnet_vf_setup(struct work_struct *w)
> +{
> + struct virtnet_info *vi
> + = container_of(w, struct virtnet_info, vf_takeover.work);
> + struct net_device *ndev = vi->dev;
> + struct net_device *vf_netdev;
> +
> + if (!rtnl_trylock()) {
> + schedule_delayed_work(&vi->vf_takeover, 0);
> + return;
> + }
> +
> + vf_netdev = rtnl_dereference(vi->vf_netdev);
> + if (vf_netdev)
> + __virtnet_vf_setup(ndev, vf_netdev);
> +
> + rtnl_unlock();
> +}
> +
> static int virtnet_probe(struct virtio_device *vdev)
> {
> int i, err;
> @@ -2600,6 +2732,11 @@ static int virtnet_probe(struct virtio_device *vdev)
> }
>
> INIT_WORK(&vi->config_work, virtnet_config_changed_work);
> + INIT_DELAYED_WORK(&vi->vf_takeover, virtnet_vf_setup);
> +
> + vi->vf_stats = netdev_alloc_pcpu_stats(struct virtnet_vf_pcpu_stats);
> + if (!vi->vf_stats)
> + goto free_stats;
>
> /* If we can receive ANY GSO packets, we must allocate large ones. */
> if (virtio_has_feature(vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> @@ -2634,7 +2771,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> */
> dev_err(&vdev->dev, "device MTU appears to have changed "
> "it is now %d < %d", mtu, dev->min_mtu);
> - goto free_stats;
> + goto free_vf_stats;
> }
>
> dev->mtu = mtu;
> @@ -2658,7 +2795,7 @@ static int virtnet_probe(struct virtio_device *vdev)
> /* Allocate/initialize the rx/tx queues, and invoke find_vqs */
> err = init_vqs(vi);
> if (err)
> - goto free_stats;
> + goto free_vf_stats;
>
> #ifdef CONFIG_SYSFS
> if (vi->mergeable_rx_bufs)
> @@ -2712,6 +2849,8 @@ static int virtnet_probe(struct virtio_device *vdev)
> cancel_delayed_work_sync(&vi->refill);
> free_receive_page_frags(vi);
> virtnet_del_vqs(vi);
> +free_vf_stats:
> + free_percpu(vi->vf_stats);
> free_stats:
> free_percpu(vi->stats);
> free:
> @@ -2733,19 +2872,178 @@ static void remove_vq_common(struct virtnet_info *vi)
> virtnet_del_vqs(vi);
> }
>
> +static struct net_device *get_virtio_bymac(const u8 *mac)
> +{
> + struct net_device *dev;
> +
> + ASSERT_RTNL();
> +
> + for_each_netdev(&init_net, dev) {
> + if (dev->netdev_ops != &virtnet_netdev)
> + continue; /* not a virtio_net device */
> +
> + if (ether_addr_equal(mac, dev->perm_addr))
> + return dev;
> + }
> +
> + return NULL;
> +}
> +
> +static struct net_device *get_virtio_byref(struct net_device *vf_netdev)
> +{
> + struct net_device *dev;
> +
> + ASSERT_RTNL();
> +
> + for_each_netdev(&init_net, dev) {
> + struct virtnet_info *vi;
> +
> + if (dev->netdev_ops != &virtnet_netdev)
> + continue; /* not a virtio_net device */
> +
> + vi = netdev_priv(dev);
> + if (rtnl_dereference(vi->vf_netdev) == vf_netdev)
> + return dev; /* a match */
> + }
> +
> + return NULL;
> +}
> +
> +/* Called when VF is injecting data into network stack.
> + * Change the associated network device from VF to virtio.
> + * note: already called with rcu_read_lock
> + */
> +static rx_handler_result_t virtnet_vf_handle_frame(struct sk_buff **pskb)
> +{
> + struct sk_buff *skb = *pskb;
> + struct net_device *ndev = rcu_dereference(skb->dev->rx_handler_data);
> + struct virtnet_info *vi = netdev_priv(ndev);
> + struct virtnet_vf_pcpu_stats *pcpu_stats =
> + this_cpu_ptr(vi->vf_stats);
> +
> + skb->dev = ndev;
> +
> + u64_stats_update_begin(&pcpu_stats->syncp);
> + pcpu_stats->rx_packets++;
> + pcpu_stats->rx_bytes += skb->len;
> + u64_stats_update_end(&pcpu_stats->syncp);
> +
> + return RX_HANDLER_ANOTHER;
> +}
> +
> +static int virtnet_vf_join(struct net_device *vf_netdev,
> + struct net_device *ndev)
> +{
> + struct virtnet_info *vi = netdev_priv(ndev);
> + int ret;
> +
> + ret = netdev_rx_handler_register(vf_netdev,
> + virtnet_vf_handle_frame, ndev);
> + if (ret != 0) {
> + netdev_err(vf_netdev,
> + "can not register virtio VF receive handler (err = %d)\n",
> + ret);
> + goto rx_handler_failed;
> + }
> +
> + ret = netdev_upper_dev_link(vf_netdev, ndev, NULL);
> + if (ret != 0) {
> + netdev_err(vf_netdev,
> + "can not set master device %s (err = %d)\n",
> + ndev->name, ret);
> + goto upper_link_failed;
> + }
> +
> + /* set slave flag before open to prevent IPv6 addrconf */
> + vf_netdev->flags |= IFF_SLAVE;
> +
> + schedule_delayed_work(&vi->vf_takeover, VF_TAKEOVER_INT);
> +
> + call_netdevice_notifiers(NETDEV_JOIN, vf_netdev);
> +
> + netdev_info(vf_netdev, "joined to %s\n", ndev->name);
> + return 0;
> +
> +upper_link_failed:
> + netdev_rx_handler_unregister(vf_netdev);
> +rx_handler_failed:
> + return ret;
> +}
> +
> +static int virtnet_register_vf(struct net_device *vf_netdev)
> +{
> + struct net_device *ndev;
> + struct virtnet_info *vi;
> +
> + if (vf_netdev->addr_len != ETH_ALEN)
> + return NOTIFY_DONE;
> +
> + /* We will use the MAC address to locate the virtio_net interface to
> + * associate with the VF interface. If we don't find a matching
> + * virtio interface, move on.
> + */
> + ndev = get_virtio_bymac(vf_netdev->perm_addr);
> + if (!ndev)
> + return NOTIFY_DONE;
> +
> + vi = netdev_priv(ndev);
> + if (rtnl_dereference(vi->vf_netdev))
> + return NOTIFY_DONE;
> +
> + if (virtnet_vf_join(vf_netdev, ndev) != 0)
> + return NOTIFY_DONE;
> +
> + netdev_info(ndev, "VF registering %s\n", vf_netdev->name);
> +
> + dev_hold(vf_netdev);
> + rcu_assign_pointer(vi->vf_netdev, vf_netdev);
> +
> + return NOTIFY_OK;
> +}
> +
> +static int virtnet_unregister_vf(struct net_device *vf_netdev)
> +{
> + struct net_device *ndev;
> + struct virtnet_info *vi;
> +
> + ndev = get_virtio_byref(vf_netdev);
> + if (!ndev)
> + return NOTIFY_DONE;
> +
> + vi = netdev_priv(ndev);
> + cancel_delayed_work_sync(&vi->vf_takeover);
> +
> + netdev_info(ndev, "VF unregistering %s\n", vf_netdev->name);
> +
> + netdev_rx_handler_unregister(vf_netdev);
> + netdev_upper_dev_unlink(vf_netdev, ndev);
> + RCU_INIT_POINTER(vi->vf_netdev, NULL);
> + dev_put(vf_netdev);
> +
> + return NOTIFY_OK;
> +}
> +
> static void virtnet_remove(struct virtio_device *vdev)
> {
> struct virtnet_info *vi = vdev->priv;
> + struct net_device *vf_netdev;
>
> virtnet_cpu_notif_remove(vi);
>
> /* Make sure no work handler is accessing the device. */
> flush_work(&vi->config_work);
>
> + rtnl_lock();
> + vf_netdev = rtnl_dereference(vi->vf_netdev);
> + if (vf_netdev)
> + virtnet_unregister_vf(vf_netdev);
> + rtnl_unlock();
> +
> unregister_netdev(vi->dev);
>
> remove_vq_common(vi);
>
> + free_percpu(vi->vf_stats);
> free_percpu(vi->stats);
> free_netdev(vi->dev);
> }
> @@ -2823,6 +3121,42 @@ static struct virtio_driver virtio_net_driver = {
> #endif
> };
>
> +static int virtio_netdev_event(struct notifier_block *this,
> + unsigned long event, void *ptr)
> +{
> + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> +
> + /* Skip our own events */
> + if (event_dev->netdev_ops == &virtnet_netdev)
> + return NOTIFY_DONE;
> +
> + /* Avoid non-Ethernet type devices */
> + if (event_dev->type != ARPHRD_ETHER)
> + return NOTIFY_DONE;
> +
> + /* Avoid Vlan dev with same MAC registering as VF */
> + if (is_vlan_dev(event_dev))
> + return NOTIFY_DONE;
> +
> + /* Avoid Bonding master dev with same MAC registering as VF */
> + if ((event_dev->priv_flags & IFF_BONDING) &&
> + (event_dev->flags & IFF_MASTER))
> + return NOTIFY_DONE;
> +
> + switch (event) {
> + case NETDEV_REGISTER:
> + return virtnet_register_vf(event_dev);
> + case NETDEV_UNREGISTER:
> + return virtnet_unregister_vf(event_dev);
> + default:
> + return NOTIFY_DONE;
> + }
> +}
> +
> +static struct notifier_block virtio_netdev_notifier = {
> + .notifier_call = virtio_netdev_event,
> +};
> +
> static __init int virtio_net_driver_init(void)
> {
> int ret;
> @@ -2841,6 +3175,8 @@ static __init int virtio_net_driver_init(void)
> ret = register_virtio_driver(&virtio_net_driver);
> if (ret)
> goto err_virtio;
> +
> + register_netdevice_notifier(&virtio_netdev_notifier);
> return 0;
> err_virtio:
> cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> @@ -2853,6 +3189,7 @@ module_init(virtio_net_driver_init);
>
> static __exit void virtio_net_driver_exit(void)
> {
> + unregister_netdevice_notifier(&virtio_netdev_notifier);
> unregister_virtio_driver(&virtio_net_driver);
> cpuhp_remove_multi_state(CPUHP_VIRT_NET_DEAD);
> cpuhp_remove_multi_state(virtionet_online);
> --
> 2.14.3
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Michael S. Tsirkin @ 2017-12-21 0:15 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Sridhar Samudrala, stephen, netdev, virtualization,
alexander.duyck
In-Reply-To: <20171220143140.0a3dc7f1@cakuba.netronome.com>
On Wed, Dec 20, 2017 at 02:33:34PM -0800, Jakub Kicinski wrote:
> On Mon, 18 Dec 2017 16:40:36 -0800, Sridhar Samudrala wrote:
> > +static int virtio_netdev_event(struct notifier_block *this,
> > + unsigned long event, void *ptr)
> > +{
> > + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > +
> > + /* Skip our own events */
> > + if (event_dev->netdev_ops == &virtnet_netdev)
> > + return NOTIFY_DONE;
>
> I wonder how does this work WRT loop prevention. What if I have two
> virtio devices with the same MAC, what is preventing them from claiming
> each other? Is it only the check above (not sure what is "own" in
> comment referring to)?
I expect we will add a feature bit (VIRTIO_NET_F_MASTER) and it will
be host's responsibility not to add more than 1 such device.
> I'm worried the check above will not stop virtio from enslaving hyperv
> interfaces and vice versa, potentially leading to a loop, no? There is
> also the fact that it would be preferable to share the code between
> paravirt drivers, to avoid duplicated bugs.
>
> My suggestion during the previous discussion was to create a paravirt
> bond device, which will explicitly tell the OS which interfaces to
> bond, regardless of which driver they're using. Could be some form of
> ACPI/FW driver too, I don't know enough about x86 FW to suggest
> something fully fleshed :(
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Jakub Kicinski @ 2017-12-21 0:29 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Sridhar Samudrala, stephen, netdev, virtualization,
alexander.duyck
In-Reply-To: <20171221021450-mutt-send-email-mst@kernel.org>
On Thu, 21 Dec 2017 02:15:31 +0200, Michael S. Tsirkin wrote:
> On Wed, Dec 20, 2017 at 02:33:34PM -0800, Jakub Kicinski wrote:
> > On Mon, 18 Dec 2017 16:40:36 -0800, Sridhar Samudrala wrote:
> > > +static int virtio_netdev_event(struct notifier_block *this,
> > > + unsigned long event, void *ptr)
> > > +{
> > > + struct net_device *event_dev = netdev_notifier_info_to_dev(ptr);
> > > +
> > > + /* Skip our own events */
> > > + if (event_dev->netdev_ops == &virtnet_netdev)
> > > + return NOTIFY_DONE;
> >
> > I wonder how does this work WRT loop prevention. What if I have two
> > virtio devices with the same MAC, what is preventing them from claiming
> > each other? Is it only the check above (not sure what is "own" in
> > comment referring to)?
>
> I expect we will add a feature bit (VIRTIO_NET_F_MASTER) and it will
> be host's responsibility not to add more than 1 such device.
Do you mean a virtio_net feature bit? That won't stop the loop with
a hyperv device. Unless we want every paravirt device to have such
bit and enforce there can be only one magic bond enslavement active in
the system.
Making the bonding information separate from the slaves seems so much
cleaner... No limitation on device types, or how many bonds user
chooses to create. No MAC matching...
> > I'm worried the check above will not stop virtio from enslaving hyperv
> > interfaces and vice versa, potentially leading to a loop, no? There is
> > also the fact that it would be preferable to share the code between
> > paravirt drivers, to avoid duplicated bugs.
> >
> > My suggestion during the previous discussion was to create a paravirt
> > bond device, which will explicitly tell the OS which interfaces to
> > bond, regardless of which driver they're using. Could be some form of
> > ACPI/FW driver too, I don't know enough about x86 FW to suggest
> > something fully fleshed :(
^ permalink raw reply
* Re: RCU callback crashes
From: Jakub Kicinski @ 2017-12-21 0:37 UTC (permalink / raw)
To: Cong Wang; +Cc: Jiri Pirko, netdev@vger.kernel.org
In-Reply-To: <CAM_iQpUngX+oSDiforfZceqMZrg=jDJnNf3QFF9WFQdHrU9o-g@mail.gmail.com>
On Wed, 20 Dec 2017 16:03:49 -0800, Cong Wang wrote:
> On Wed, Dec 20, 2017 at 10:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Wed, Dec 20, 2017 at 10:17 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >>
> >> I guess it is q->miniqp which is freed in qdisc_graft() without properly
> >> waiting for rcu readers?
> >
> > It is probably so, the call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func)
> > in the end of mini_qdisc_pair_swap() is invoked on miniq_old->rcu,
> > but miniq is being freed, no rcu barrier waits for it...
> >
> > You can try to add a rcu_barrier_bh() at the end to see if this crash
> > is gone, but I don't think people like adding yet another rcu barrier...
>
> Hi, Jakub
>
> Can you test the following fix? I am not a fan of rcu barrier but we
> already have one so...
>
> diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> index 876fab2604b8..1b68fedea124 100644
> --- a/net/sched/sch_generic.c
> +++ b/net/sched/sch_generic.c
> @@ -1240,6 +1240,8 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
>
> if (!tp_head) {
> RCU_INIT_POINTER(*miniqp->p_miniq, NULL);
> + /* Wait for existing flying RCU callback before being freed. */
> + rcu_barrier_bh();
> return;
> }
Looks good after 30 minutes, feel free to add if you post officially:
Tested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
^ permalink raw reply
* Re: RCU callback crashes
From: Jakub Kicinski @ 2017-12-21 0:41 UTC (permalink / raw)
To: Cong Wang, John Fastabend; +Cc: Jiri Pirko, netdev@vger.kernel.org
In-Reply-To: <20171220163710.7a5f06e5@cakuba.netronome.com>
On Wed, 20 Dec 2017 16:37:10 -0800, Jakub Kicinski wrote:
> On Wed, 20 Dec 2017 16:03:49 -0800, Cong Wang wrote:
> > On Wed, Dec 20, 2017 at 10:31 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > > On Wed, Dec 20, 2017 at 10:17 AM, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > >>
> > >> I guess it is q->miniqp which is freed in qdisc_graft() without properly
> > >> waiting for rcu readers?
> > >
> > > It is probably so, the call_rcu_bh(&miniq_old->rcu, mini_qdisc_rcu_func)
> > > in the end of mini_qdisc_pair_swap() is invoked on miniq_old->rcu,
> > > but miniq is being freed, no rcu barrier waits for it...
> > >
> > > You can try to add a rcu_barrier_bh() at the end to see if this crash
> > > is gone, but I don't think people like adding yet another rcu barrier...
> >
> > Hi, Jakub
> >
> > Can you test the following fix? I am not a fan of rcu barrier but we
> > already have one so...
> >
> > diff --git a/net/sched/sch_generic.c b/net/sched/sch_generic.c
> > index 876fab2604b8..1b68fedea124 100644
> > --- a/net/sched/sch_generic.c
> > +++ b/net/sched/sch_generic.c
> > @@ -1240,6 +1240,8 @@ void mini_qdisc_pair_swap(struct mini_Qdisc_pair *miniqp,
> >
> > if (!tp_head) {
> > RCU_INIT_POINTER(*miniqp->p_miniq, NULL);
> > + /* Wait for existing flying RCU callback before being freed. */
> > + rcu_barrier_bh();
> > return;
> > }
>
> Looks good after 30 minutes, feel free to add if you post officially:
>
> Tested-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Just as I hit send... :) but this looks unrelated, "Comm: sshd" -
so probably from the management interface.
[ 154.604041] ==================================================================
[ 154.612245] BUG: KASAN: slab-out-of-bounds in pfifo_fast_dequeue+0x140/0x2d0
[ 154.620219] Read of size 8 at addr ffff88086bb64040 by task sshd/983
[ 154.627403]
[ 154.629161] CPU: 10 PID: 983 Comm: sshd Not tainted 4.15.0-rc3-perf-00984-g82d3fc87a4aa-dirty #13
[ 154.639190] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.3.4 11/08/2016
[ 154.647665] Call Trace:
[ 154.650494] dump_stack+0xa6/0x118
[ 154.654387] ? _atomic_dec_and_lock+0xe8/0xe8
[ 154.659355] ? trace_event_raw_event_rcu_torture_read+0x190/0x190
[ 154.666263] ? rcu_segcblist_enqueue+0xe9/0x120
[ 154.671422] ? _raw_spin_unlock_bh+0x91/0xc0
[ 154.676286] ? pfifo_fast_dequeue+0x140/0x2d0
[ 154.681251] print_address_description+0x6a/0x270
[ 154.686601] ? pfifo_fast_dequeue+0x140/0x2d0
[ 154.691565] kasan_report+0x23f/0x350
[ 154.695752] pfifo_fast_dequeue+0x140/0x2d0
[ 154.700523] __qdisc_run+0x264/0xa20
[ 154.704613] ? sch_direct_xmit+0x3d0/0x3d0
[ 154.709287] ? _raw_spin_unlock+0x73/0xc0
[ 154.713860] ? is_bpf_text_address+0x1e/0x30
[ 154.718724] ? kernel_text_address+0xec/0x100
[ 154.723687] ? __kernel_text_address+0xe/0x30
[ 154.728650] ? unwind_get_return_address+0x2f/0x50
[ 154.734099] ? pfifo_fast_enqueue+0x154/0x180
[ 154.739065] __dev_queue_xmit+0x5ae/0x1110
[ 154.743738] ? dst_alloc+0x8c/0xd0
[ 154.747633] ? netdev_pick_tx+0x150/0x150
[ 154.752206] ? ip_route_output_key_hash+0xee/0x130
[ 154.757654] ? ip_queue_xmit+0x7d0/0x830
[ 154.762131] ? tcp_transmit_skb+0xc52/0x15b0
[ 154.766994] ? tcp_write_xmit+0x425/0x2060
[ 154.771665] ? __tcp_push_pending_frames+0x56/0x110
[ 154.777209] ? tcp_push+0x2cf/0x360
[ 154.781200] ? tcp_sendmsg_locked+0xdb3/0x1cb0
[ 154.786259] ? tcp_sendmsg+0x27/0x40
[ 154.790347] ? inet_sendmsg+0xb3/0x1f0
[ 154.794629] ? sock_sendmsg+0x64/0x80
[ 154.798814] ? sock_write_iter+0x148/0x1f0
[ 154.803486] ? __vfs_write+0x26e/0x370
[ 154.807767] ? vfs_write+0xe9/0x240
[ 154.811747] ? SyS_write+0xa7/0x130
[ 154.815739] ? entry_SYSCALL_64_fastpath+0x1e/0x81
[ 154.821190] ? __alias_free_mem+0x20/0x20
[ 154.825766] ? rt_cache_route+0x143/0x170
[ 154.830342] ? find_busiest_group+0x12eb/0x1630
[ 154.835500] ? inet_lookup_ifaddr_rcu+0x126/0x170
[ 154.840852] ? percpu_counter_add_batch+0x24/0xa0
[ 154.846207] ? rt_cpu_seq_stop+0x10/0x10
[ 154.850684] ? dst_alloc+0xac/0xd0
[ 154.854579] ? rt_dst_alloc+0x1f0/0x250
[ 154.858958] ? ipv4_neigh_lookup+0x3a0/0x3a0
[ 154.863824] ? __rcu_read_unlock+0x6e/0x120
[ 154.868594] ? trace_event_raw_event_rcu_torture_read+0x190/0x190
[ 154.875502] ? ip_finish_output2+0x68d/0x7c0
[ 154.880366] ip_finish_output2+0x68d/0x7c0
[ 154.885040] ? ip_send_check+0x60/0x60
[ 154.889322] ? ip_route_input_noref+0xd0/0xd0
[ 154.894287] ? xfrm_lookup+0x888/0x10f0
[ 154.898668] ? ipv4_mtu+0x163/0x200
[ 154.902662] ? load_balance+0x108d/0x14a0
[ 154.907238] ? ip_finish_output+0x39a/0x4c0
[ 154.912004] ip_finish_output+0x39a/0x4c0
[ 154.916578] ? ip_fragment.constprop.5+0xf0/0xf0
[ 154.921832] ? find_busiest_group+0x1630/0x1630
[ 154.926991] ? check_cfs_rq_runtime+0x70/0x70
[ 154.931954] ? __rcu_read_unlock+0x6e/0x120
[ 154.936723] ? trace_event_raw_event_rcu_torture_read+0x190/0x190
[ 154.943630] ? unwind_get_return_address+0x2f/0x50
[ 154.949077] ? ip_send_check+0x20/0x60
[ 154.953360] ip_output+0x106/0x280
[ 154.957253] ? ip_mc_output+0x750/0x750
[ 154.961631] ? ip_route_output_key_hash_rcu+0x1240/0x1240
[ 154.967757] ? sk_setup_caps+0x180/0x180
[ 154.972236] ? __skb_clone+0x2f8/0x370
[ 154.976520] ip_queue_xmit+0x381/0x830
[ 154.980805] ? ip_build_and_send_pkt+0x420/0x420
[ 154.986060] ? trace_event_raw_event_bpf_obj_map+0x200/0x200
[ 154.992481] ? tcp_options_write+0xc3/0x360
[ 154.997248] ? tcp_established_options+0x122/0x190
[ 155.002697] tcp_transmit_skb+0xc52/0x15b0
[ 155.007374] ? __tcp_select_window+0x3c0/0x3c0
[ 155.012433] ? is_bpf_text_address+0x1e/0x30
[ 155.017296] ? kernel_text_address+0xec/0x100
[ 155.022259] ? __kernel_text_address+0xe/0x30
[ 155.027221] ? unwind_get_return_address+0x2f/0x50
[ 155.032670] ? __save_stack_trace+0x83/0xd0
[ 155.037437] ? memcmp+0x45/0x70
[ 155.041041] ? depot_save_stack+0x12d/0x470
[ 155.045811] ? tcp_small_queue_check.isra.4+0x10a/0x1f0
[ 155.051745] ? tcp_tso_segs+0xe0/0xe0
[ 155.055932] ? native_sched_clock+0xcc/0x130
[ 155.060799] ? cyc2ns_read_end+0x20/0x20
[ 155.065275] ? sock_sendmsg+0x64/0x80
[ 155.069460] ? vfs_write+0xe9/0x240
[ 155.073483] ? entry_SYSCALL_64_fastpath+0x1e/0x81
[ 155.078931] ? sock_sendmsg+0x64/0x80
[ 155.083116] ? sock_write_iter+0x148/0x1f0
[ 155.087790] ? sched_clock+0x5/0x10
[ 155.091780] ? deref_stack_reg+0x98/0xd0
[ 155.096257] ? sched_clock+0x5/0x10
[ 155.100248] ? sched_clock_cpu+0x14/0xf0
[ 155.104726] tcp_write_xmit+0x425/0x2060
[ 155.109209] ? memcg_kmem_get_cache+0x4e0/0x4e0
[ 155.114356] ? tcp_transmit_skb+0x15b0/0x15b0
[ 155.119318] ? memcg_kmem_put_cache+0x63/0x120
[ 155.124376] ? memcg_kmem_get_cache+0x4e0/0x4e0
[ 155.129536] ? __kmalloc_node_track_caller+0x1fe/0x2a0
[ 155.135371] ? __alloc_skb+0xed/0x390
[ 155.139558] ? __kmalloc_reserve.isra.7+0x43/0x80
[ 155.144908] ? memset+0x1f/0x40
[ 155.148510] ? __alloc_skb+0x302/0x390
[ 155.152792] ? __kmalloc_reserve.isra.7+0x80/0x80
[ 155.158142] ? ipv4_mtu+0x90/0x200
[ 155.162036] ? tcp_mtu_to_mss+0x155/0x1a0
[ 155.166610] ? ipv4_negative_advice+0x60/0x60
[ 155.171572] ? tcp_trim_head+0x260/0x260
[ 155.176048] ? SyS_read+0xa7/0x130
[ 155.179941] ? iov_iter_advance+0x16a/0x780
[ 155.184709] ? copyout+0x4f/0x60
[ 155.188410] ? tcp_established_options+0x122/0x190
[ 155.193858] ? import_single_range+0x110/0x110
[ 155.198918] __tcp_push_pending_frames+0x56/0x110
[ 155.204269] tcp_push+0x2cf/0x360
[ 155.208068] ? tcp_splice_data_recv+0xb0/0xb0
[ 155.213032] ? skb_entail+0x2e5/0x300
[ 155.217217] ? _copy_from_iter+0x680/0x680
[ 155.221890] ? _raw_spin_unlock_bh+0x91/0xc0
[ 155.226757] tcp_sendmsg_locked+0xdb3/0x1cb0
[ 155.231628] ? tcp_recvmsg+0x790/0x1420
[ 155.236001] ? tcp_sendpage+0x60/0x60
[ 155.240190] ? tcp_recv_timestamp+0x240/0x240
[ 155.245158] ? compat_poll_select_copy_remaining+0x310/0x310
[ 155.251582] ? compat_poll_select_copy_remaining+0x310/0x310
[ 155.258004] ? compat_poll_select_copy_remaining+0x310/0x310
[ 155.264427] ? __rcu_read_unlock+0xf8/0x120
[ 155.269197] ? trace_event_raw_event_rcu_torture_read+0x190/0x190
[ 155.276104] ? trace_event_raw_event_rcu_torture_read+0x190/0x190
[ 155.283000] ? _raw_spin_unlock+0x73/0xc0
[ 155.287573] ? _raw_spin_trylock+0xe0/0xe0
[ 155.292246] ? __release_sock+0xc0/0x140
[ 155.296727] tcp_sendmsg+0x27/0x40
[ 155.300621] inet_sendmsg+0xb3/0x1f0
[ 155.304709] ? aa_path_link+0x260/0x260
[ 155.309088] ? inet_recvmsg+0x210/0x210
[ 155.313469] ? fsnotify+0xae8/0xb30
[ 155.317462] ? inet_recvmsg+0x210/0x210
[ 155.332037] sock_sendmsg+0x64/0x80
[ 155.336029] sock_write_iter+0x148/0x1f0
[ 155.340506] ? sock_sendmsg+0x80/0x80
[ 155.344691] ? sock_recvmsg+0x90/0x90
[ 155.348881] ? tty_ldisc_deref+0x12/0x20
[ 155.353356] ? iov_iter_init+0x77/0xb0
[ 155.357639] __vfs_write+0x26e/0x370
[ 155.361727] ? kernel_read+0xa0/0xa0
[ 155.365816] ? _raw_spin_unlock_irq+0x73/0xc0
[ 155.370783] ? __fsnotify_update_child_dentry_flags.part.0+0x150/0x150
[ 155.378174] ? __fsnotify_parent+0x84/0x220
[ 155.382944] ? __fsnotify_update_child_dentry_flags.part.0+0x150/0x150
[ 155.390340] vfs_write+0xe9/0x240
[ 155.394137] SyS_write+0xa7/0x130
[ 155.397934] ? SyS_read+0x130/0x130
[ 155.401925] ? SyS_clock_settime+0x110/0x110
[ 155.406791] ? SyS_fcntl+0x82/0xb0
[ 155.410685] entry_SYSCALL_64_fastpath+0x1e/0x81
[ 155.415939] RIP: 0033:0x7fed3bbf4290
[ 155.420024] RSP: 002b:00007ffcffb69468 EFLAGS: 00000246 ORIG_RAX: 0000000000000001
[ 155.428600] RAX: ffffffffffffffda RBX: 00007fed3bec1b20 RCX: 00007fed3bbf4290
[ 155.436668] RDX: 0000000000000024 RSI: 0000559b52ddd308 RDI: 0000000000000003
[ 155.444735] RBP: 0000000000000021 R08: 0000559b52ddaf60 R09: 0000000000000014
[ 155.452803] R10: 0000000000000000 R11: 0000000000000246 R12: 00007fed3bec1b78
[ 155.460870] R13: 0000559b52ddb030 R14: 0000559b52ddaf50 R15: 0000559b52ddaf50
[ 155.468942]
[ 155.470697] Allocated by task 780:
[ 155.474589] __kmalloc+0xfa/0x230
[ 155.478377] pfifo_fast_init+0x69/0x160
[ 155.482757] qdisc_create_dflt+0x69/0xb0
[ 155.487232] mq_init+0x195/0x1e0
[ 155.490931] qdisc_create_dflt+0x69/0xb0
[ 155.495407] dev_activate+0x48a/0x4e0
[ 155.499593] __dev_open+0x19e/0x210
[ 155.503583] __dev_change_flags+0x3b5/0x3f0
[ 155.508351] dev_change_flags+0x50/0xa0
[ 155.512729] do_setlink+0x5eb/0x1cf0
[ 155.516817] rtnl_newlink+0x9d5/0xe40
[ 155.521002] rtnetlink_rcv_msg+0x37c/0x7e0
[ 155.525673] netlink_rcv_skb+0x122/0x230
[ 155.530149] netlink_unicast+0x2ae/0x360
[ 155.534624] netlink_sendmsg+0x5d5/0x620
[ 155.539100] sock_sendmsg+0x64/0x80
[ 155.543090] ___sys_sendmsg+0x4a8/0x500
[ 155.547467] __sys_sendmsg+0xa9/0x140
[ 155.551643] entry_SYSCALL_64_fastpath+0x1e/0x81
[ 155.556893]
[ 155.558646] Freed by task 0:
[ 155.561953] (stack is not available)
[ 155.566035]
[ 155.567791] The buggy address belongs to the object at ffff88086bb62100
[ 155.567791] which belongs to the cache kmalloc-8192 of size 8192
[ 155.582099] The buggy address is located 8000 bytes inside of
[ 155.582099] 8192-byte region [ffff88086bb62100, ffff88086bb64100)
[ 155.595529] The buggy address belongs to the page:
[ 155.600977] page:00000000a9a82c52 count:1 mapcount:0 mapping: (null) index:0x0 compound_mapcount: 0
[ 155.612081] flags: 0x6ffff0000008100(slab|head)
[ 155.617240] raw: 06ffff0000008100 0000000000000000 0000000000000000 0000000100030003
[ 155.626010] raw: dead000000000100 dead000000000200 ffff8803afc0e680 0000000000000000
[ 155.634776] page dumped because: kasan: bad access detected
[ 155.641094]
[ 155.642935] Memory state around the buggy address:
[ 155.648382] ffff88086bb63f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 155.656568] ffff88086bb63f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
[ 155.664756] >ffff88086bb64000: 00 00 00 00 00 00 00 00 fc fc fc fc fc fc fc fc
[ 155.672943] ^
[ 155.678972] ffff88086bb64080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 155.687160] ffff88086bb64100: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc
[ 155.695346] ==================================================================
^ permalink raw reply
* [PATCH] net: phy: micrel: ksz9031: reconfigure autoneg after phy autoneg workaround
From: Grygorii Strashko @ 2017-12-21 0:45 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, netdev
Cc: linux-kernel, Sekhar Nori, Grygorii Strashko
Under some circumstances driver will perform PHY reset in
ksz9031_read_status() to fix autoneg failure case (idle error count =
0xFF). When this happens ksz9031 will not detect link status change any
more when connecting to Netgear 1G switch (link can be recovered sometimes by
restarting netdevice "ifconfig down up"). Reproduced with TI am572x board
equipped with ksz9031 PHY while connecting to Netgear 1G switch.
Fix the issue by reconfiguring autonegotiation after PHY reset in
ksz9031_read_status().
Fixes: d2fd719bcb0e ("net/phy: micrel: Add workaround for bad autoneg")
Signed-off-by: Grygorii Strashko <grygorii.strashko@ti.com>
---
drivers/net/phy/micrel.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index ab46141..422ff63 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -624,6 +624,7 @@ static int ksz9031_read_status(struct phy_device *phydev)
phydev->link = 0;
if (phydev->drv->config_intr && phy_interrupt_is_valid(phydev))
phydev->drv->config_intr(phydev);
+ return genphy_config_aneg(phydev);
}
return 0;
--
2.10.5
^ permalink raw reply related
* Re: [-next PATCH 4/4] treewide: Use DEVICE_ATTR_WO
From: Zhang Rui @ 2017-12-21 0:50 UTC (permalink / raw)
To: Joe Perches, Borislav Petkov, Benjamin Herrenschmidt,
Paul Mackerras, Michael Ellerman, Thomas Falcon, John Allen,
Inaky Perez-Gonzalez, linux-wimax, James Smart, Dick Kennedy,
Eduardo Valentin
Cc: Martin Schwidefsky, Heiko Carstens, Thomas Gleixner,
H. Peter Anvin, x86, Dmitry Torokhov, James E.J. Bottomley,
Martin K. Petersen, linux-s390, linux-kernel, linux-input, netdev,
linuxppc-dev, linux-scsi, linux-pm
In-Reply-To: <fa30f1ad73f76dafff816df40cacffe613aa2f48.1513706702.git.joe@perches.com>
On Tue, 2017-12-19 at 10:15 -0800, Joe Perches wrote:
> Convert DEVICE_ATTR uses to DEVICE_ATTR_WO where possible.
>
> Done with perl script:
>
> $ git grep -w --name-only DEVICE_ATTR | \
> xargs perl -i -e 'local $/; while (<>) {
> s/\bDEVICE_ATTR\s*\(\s*(\w+)\s*,\s*\(?(?:\s*S_IWUSR\s*|\s*0200\s*)\)?
> \s*,\s*NULL\s*,\s*\s_store\s*\)/DEVICE_ATTR_WO(\1)/g; print;}'
>
> Signed-off-by: Joe Perches <joe@perches.com>
> ---
> arch/s390/kernel/smp.c | 2 +-
> arch/x86/kernel/cpu/microcode/core.c | 2 +-
> drivers/input/touchscreen/elants_i2c.c | 2 +-
> drivers/net/ethernet/ibm/ibmvnic.c | 2 +-
> drivers/net/wimax/i2400m/sysfs.c | 3 +--
> drivers/scsi/lpfc/lpfc_attr.c | 3 +--
> drivers/thermal/thermal_sysfs.c | 2 +-
For the thermal part,
Acked-by: Zhang Rui <rui.zhang@intel.com>
thanks,
rui
> 7 files changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/arch/s390/kernel/smp.c b/arch/s390/kernel/smp.c
> index b8c1a85bcf2d..a919b2f0141d 100644
> --- a/arch/s390/kernel/smp.c
> +++ b/arch/s390/kernel/smp.c
> @@ -1151,7 +1151,7 @@ static ssize_t __ref rescan_store(struct device
> *dev,
> rc = smp_rescan_cpus();
> return rc ? rc : count;
> }
> -static DEVICE_ATTR(rescan, 0200, NULL, rescan_store);
> +static DEVICE_ATTR_WO(rescan);
> #endif /* CONFIG_HOTPLUG_CPU */
>
> static int __init s390_smp_init(void)
> diff --git a/arch/x86/kernel/cpu/microcode/core.c
> b/arch/x86/kernel/cpu/microcode/core.c
> index c4fa4a85d4cb..09c74b0560dd 100644
> --- a/arch/x86/kernel/cpu/microcode/core.c
> +++ b/arch/x86/kernel/cpu/microcode/core.c
> @@ -560,7 +560,7 @@ static ssize_t pf_show(struct device *dev,
> return sprintf(buf, "0x%x\n", uci->cpu_sig.pf);
> }
>
> -static DEVICE_ATTR(reload, 0200, NULL, reload_store);
> +static DEVICE_ATTR_WO(reload);
> static DEVICE_ATTR(version, 0400, version_show, NULL);
> static DEVICE_ATTR(processor_flags, 0400, pf_show, NULL);
>
> diff --git a/drivers/input/touchscreen/elants_i2c.c
> b/drivers/input/touchscreen/elants_i2c.c
> index a458e5ec9e41..819213e88f32 100644
> --- a/drivers/input/touchscreen/elants_i2c.c
> +++ b/drivers/input/touchscreen/elants_i2c.c
> @@ -1000,7 +1000,7 @@ static ssize_t show_iap_mode(struct device
> *dev,
> "Normal" : "Recovery");
> }
>
> -static DEVICE_ATTR(calibrate, S_IWUSR, NULL, calibrate_store);
> +static DEVICE_ATTR_WO(calibrate);
> static DEVICE_ATTR(iap_mode, S_IRUGO, show_iap_mode, NULL);
> static DEVICE_ATTR(update_fw, S_IWUSR, NULL, write_update_fw);
>
> diff --git a/drivers/net/ethernet/ibm/ibmvnic.c
> b/drivers/net/ethernet/ibm/ibmvnic.c
> index 1dc4aef37d3a..42b96e1a1b13 100644
> --- a/drivers/net/ethernet/ibm/ibmvnic.c
> +++ b/drivers/net/ethernet/ibm/ibmvnic.c
> @@ -4411,7 +4411,7 @@ static ssize_t failover_store(struct device
> *dev, struct device_attribute *attr,
> return count;
> }
>
> -static DEVICE_ATTR(failover, 0200, NULL, failover_store);
> +static DEVICE_ATTR_WO(failover);
>
> static unsigned long ibmvnic_get_desired_dma(struct vio_dev *vdev)
> {
> diff --git a/drivers/net/wimax/i2400m/sysfs.c
> b/drivers/net/wimax/i2400m/sysfs.c
> index 1237109f251a..8c67df11105c 100644
> --- a/drivers/net/wimax/i2400m/sysfs.c
> +++ b/drivers/net/wimax/i2400m/sysfs.c
> @@ -65,8 +65,7 @@ ssize_t i2400m_idle_timeout_store(struct device
> *dev,
> }
>
> static
> -DEVICE_ATTR(i2400m_idle_timeout, S_IWUSR,
> - NULL, i2400m_idle_timeout_store);
> +DEVICE_ATTR_WO(i2400m_idle_timeout);
>
> static
> struct attribute *i2400m_dev_attrs[] = {
> diff --git a/drivers/scsi/lpfc/lpfc_attr.c
> b/drivers/scsi/lpfc/lpfc_attr.c
> index 517ff203cfde..6ddaf51a23f6 100644
> --- a/drivers/scsi/lpfc/lpfc_attr.c
> +++ b/drivers/scsi/lpfc/lpfc_attr.c
> @@ -2418,8 +2418,7 @@ lpfc_soft_wwn_enable_store(struct device *dev,
> struct device_attribute *attr,
>
> return count;
> }
> -static DEVICE_ATTR(lpfc_soft_wwn_enable, S_IWUSR, NULL,
> - lpfc_soft_wwn_enable_store);
> +static DEVICE_ATTR_WO(lpfc_soft_wwn_enable);
>
> /**
> * lpfc_soft_wwpn_show - Return the cfg soft ww port name of the
> adapter
> diff --git a/drivers/thermal/thermal_sysfs.c
> b/drivers/thermal/thermal_sysfs.c
> index 2bc964392924..ba81c9080f6e 100644
> --- a/drivers/thermal/thermal_sysfs.c
> +++ b/drivers/thermal/thermal_sysfs.c
> @@ -317,7 +317,7 @@ emul_temp_store(struct device *dev, struct
> device_attribute *attr,
>
> return ret ? ret : count;
> }
> -static DEVICE_ATTR(emul_temp, S_IWUSR, NULL, emul_temp_store);
> +static DEVICE_ATTR_WO(emul_temp);
> #endif
>
> static ssize_t
^ permalink raw reply
* Re: RCU callback crashes
From: Jakub Kicinski @ 2017-12-21 0:50 UTC (permalink / raw)
To: John Fastabend; +Cc: Cong Wang, Jiri Pirko, netdev@vger.kernel.org
In-Reply-To: <20171220164058.2a862e27@cakuba.netronome.com>
On Wed, 20 Dec 2017 16:41:14 -0800, Jakub Kicinski wrote:
> Just as I hit send... :) but this looks unrelated, "Comm: sshd" -
> so probably from the management interface.
>
> [ 154.604041] ==================================================================
> [ 154.612245] BUG: KASAN: slab-out-of-bounds in pfifo_fast_dequeue+0x140/0x2d0
> [ 154.620219] Read of size 8 at addr ffff88086bb64040 by task sshd/983
> [ 154.627403]
> [ 154.629161] CPU: 10 PID: 983 Comm: sshd Not tainted 4.15.0-rc3-perf-00984-g82d3fc87a4aa-dirty #13
> [ 154.639190] Hardware name: Dell Inc. PowerEdge R730/072T6D, BIOS 2.3.4 11/08/2016
> [ 154.647665] Call Trace:
> [ 154.650494] dump_stack+0xa6/0x118
> [ 154.654387] ? _atomic_dec_and_lock+0xe8/0xe8
> [ 154.659355] ? trace_event_raw_event_rcu_torture_read+0x190/0x190
> [ 154.666263] ? rcu_segcblist_enqueue+0xe9/0x120
> [ 154.671422] ? _raw_spin_unlock_bh+0x91/0xc0
> [ 154.676286] ? pfifo_fast_dequeue+0x140/0x2d0
> [ 154.681251] print_address_description+0x6a/0x270
> [ 154.686601] ? pfifo_fast_dequeue+0x140/0x2d0
> [ 154.691565] kasan_report+0x23f/0x350
> [ 154.695752] pfifo_fast_dequeue+0x140/0x2d0
If we trust stack decode it's:
615 static struct sk_buff *pfifo_fast_dequeue(struct Qdisc *qdisc)
616 {
617 struct pfifo_fast_priv *priv = qdisc_priv(qdisc);
618 struct sk_buff *skb = NULL;
619 int band;
620
621 for (band = 0; band < PFIFO_FAST_BANDS && !skb; band++) {
622 struct skb_array *q = band2list(priv, band);
623
>> 624 if (__skb_array_empty(q))
625 continue;
626
627 skb = skb_array_consume_bh(q);
628 }
629 if (likely(skb)) {
630 qdisc_qstats_cpu_backlog_dec(qdisc, skb);
631 qdisc_bstats_cpu_update(qdisc, skb);
632 qdisc_qstats_cpu_qlen_dec(qdisc);
633 }
634
635 return skb;
636 }
^ permalink raw reply
* Re: [PATCH v3 next-queue 05/10] ixgbe: add ipsec offload add and remove SA
From: Marcelo Ricardo Leitner @ 2017-12-21 1:17 UTC (permalink / raw)
To: Shannon Nelson
Cc: intel-wired-lan, jeffrey.t.kirsher, steffen.klassert,
sowmini.varadhan, netdev
In-Reply-To: <1513728002-7643-6-git-send-email-shannon.nelson@oracle.com>
Hi,
On Tue, Dec 19, 2017 at 03:59:57PM -0800, Shannon Nelson wrote:
> +}
> +
> +static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
> + .xdo_dev_state_add = ixgbe_ipsec_add_sa,
> + .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
> +};
> +
This struct is only declared if XFRM_OFFLOAD is selected. What is
selecting it for ixgbe driver?
mlx5 driver has an extra option for ipsec offload and it then does
'depends on XFRM_OFFLOAD'
Marcelo
^ permalink raw reply
* Re: [PATCH V3 net-next 00/17] add some features and fix some bugs for HNS3 driver
From: lipeng (Y) @ 2017-12-21 1:30 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, linuxarm, salil.mehta
In-Reply-To: <20171220.142843.568343410203899321.davem@davemloft.net>
On 2017/12/21 3:28, David Miller wrote:
> From: Lipeng <lipeng321@huawei.com>
> Date: Wed, 20 Dec 2017 16:43:02 +0800
>
>> This patchset adds some new feature support and fixes some bugs:
>> [Patch 1/17 - 5/17] add the support to modify/query the tqp number
>> through ethtool -L/l command, and also fix some related bugs for
>> change tqp number.
>> [Patch 6/17 - 9-17] add support vlan tag offload on tx&&rx direction
>> for pf, and fix some related bugs.
>> [patch 10/17 - 11/17] fix bugs for auto negotiation.
>> [patch 12/17] adds support for ethtool command set_pauseparam.
>> [patch 13/17 - 14/17] add support to update flow control settings after
>> autoneg.
>> [patch 15/17 - 17/17] fix some other bugs in net-next.
> In your From: email field, as well as you and your colleagues signoffs
> you are not using your full, real, name:
> ====================
> From: Lipeng <lipeng321@huawei.com>
My first name is 'Peng' and Surname is 'Li'.
But it is usually spelled as 'Lipeng' in one go when referring.
This is quite usual convention with full names originating from
China. Surnames comes first, followed by the first name and they
both are inseparable while they are written as well.
"Lipeng" is my full real name, and my sign-off's appear like above.
Thanks,
Lipeng
> ====================
> Signed-off-by: qumingguang <qumingguang@huawei.com>
> Signed-off-by: Lipeng <lipeng321@huawei.com>
> ====================
>
> Please fix this.
>
> .
>
^ permalink raw reply
* RE: [patch iproute2] tc: add -bs option for batch mode
From: Chris Mi @ 2017-12-21 1:30 UTC (permalink / raw)
To: Stephen Hemminger; +Cc: netdev@vger.kernel.org, gerlitz.or@gmail.com
In-Reply-To: <20171220071744.25f9dd41@xeon-e3>
> -----Original Message-----
> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Wednesday, December 20, 2017 11:18 PM
> To: Chris Mi <chrism@mellanox.com>
> Cc: netdev@vger.kernel.org; gerlitz.or@gmail.com
> Subject: Re: [patch iproute2] tc: add -bs option for batch mode
>
> On Wed, 20 Dec 2017 09:23:34 +0000
> Chris Mi <chrism@mellanox.com> wrote:
>
> > > Your real performance win is just not asking for ACK for every rule.
> > No. Even if batch_size > 1, we ack every rule. The real performance
> > win is to send multiple rules in one system call. If we are not asking
> > for ACK for every rule, the performance will be improved further.
>
> Try the no ACK method.
>
> When we were optimizing routing daemons like Quagga, it was discovered
> that an ACK for every route insert was the main bottleneck. Doing
> asynchronous error handling got a bigger win than your batching.
>
> Please try that, doing multiple messages using iov is not necessary.
This is my testing result to insert 1,000,000 rules:
1. batch_size = 1, acking
real 0m15.125s
user 0m6.982s
sys 0m8.080s
2. batch_size = 10, acking
real 0m12.772s
user 0m5.984s
sys 0m6.723s
3. batch_size = 1, no acking
real 0m14.484s
user 0m6.919s
sys 0m7.498s
4. batch_size = 10, no acking
real 0m11.664s
user 0m6.017s
sys 0m5.578s
As we can see from above test result, the bottleneck is not in acking.
Without acking or with asynchronous error handling, we can improve the performance further.
It is worth to do that. But I think that should be in another patch. This patch only adds
the -bs option.
^ permalink raw reply
* Re: [RFC PATCH] virtio_net: Extend virtio to use VF datapath when available
From: Siwei Liu @ 2017-12-21 1:31 UTC (permalink / raw)
To: Stephen Hemminger
Cc: mst, sridhar.samudrala, alexander.duyck, virtualization, netdev,
David Miller
In-Reply-To: <20171219104159.30d9caaf@xeon-e3>
[-- Attachment #1.1: Type: text/plain, Size: 1504 bytes --]
On Tue, Dec 19, 2017 at 10:41 AM, Stephen Hemminger <
stephen@networkplumber.org> wrote:
> On Tue, 19 Dec 2017 13:21:17 -0500 (EST)
> David Miller <davem@davemloft.net> wrote:
>
> > From: Stephen Hemminger <stephen@networkplumber.org>
> > Date: Tue, 19 Dec 2017 09:55:48 -0800
> >
> > > could be 10ms, just enough to let udev do its renaming
> >
> > Please, move to some kind of notification or event based handling of
> > this problem.
> >
> > No delay is safe, what if userspace gets swapped out or whatever
> > else might make userspace stall unexpectedly?
> >
>
> The plan is to remove the delay and do the naming in the kernel.
> This was suggested by Lennart since udev is only doing naming policy
> because kernel names were not repeatable.
>
> This makes the VF show up as "ethN_vf" on Hyper-V which is user friendly.
>
> Patch is pending.
>
While it's good to show VF with specific naming to indicate enslavement, I
wonder wouldn't it be better to hide this netdev at all from the user
space? IMHO this extra device is useless when being enslaved and we may
delegate controls (e.g. ethtool) over to the para-virtual device instead?
That way it's possible to eliminate the possibility of additional udev
setup or modification?
I'm not sure if this is consistent with Windows guest or not, but I don't
find it _Linux_ user friendly that ethtool doesn't work on the composite
interface any more, and I have to end up with finding out the correct
enslaved VF I must operate on.
Regards,
-Siwei
[-- Attachment #1.2: Type: text/html, Size: 2322 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v3 next-queue 05/10] ixgbe: add ipsec offload add and remove SA
From: Shannon Nelson @ 2017-12-21 1:39 UTC (permalink / raw)
To: Marcelo Ricardo Leitner
Cc: intel-wired-lan, jeffrey.t.kirsher, steffen.klassert,
sowmini.varadhan, netdev
In-Reply-To: <20171221010139.GM6122@localhost.localdomain>
On 12/20/2017 5:17 PM, Marcelo Ricardo Leitner wrote:
> Hi,
>
> On Tue, Dec 19, 2017 at 03:59:57PM -0800, Shannon Nelson wrote:
>> +}
>> +
>> +static const struct xfrmdev_ops ixgbe_xfrmdev_ops = {
>> + .xdo_dev_state_add = ixgbe_ipsec_add_sa,
>> + .xdo_dev_state_delete = ixgbe_ipsec_del_sa,
>> +};
>> +
>
> This struct is only declared if XFRM_OFFLOAD is selected. What is
> selecting it for ixgbe driver?
> mlx5 driver has an extra option for ipsec offload and it then does
> 'depends on XFRM_OFFLOAD'
>
> Marcelo
>
I didn't bother putting a 'depends' item in the ixgbe's Kconfig entry,
and I didn't create an extra CONFIG variable to enable ixgbe's support
of the offload. If CONFIG_XFRM_OFFLOAD is set, then ixgbe will support it.
sln
^ 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