* Re: [PATCH REPOST net 0/2] Setup mlx4 user space Ethernet QPs to properly handle VXLAN
From: David Miller @ 2014-08-30 3:13 UTC (permalink / raw)
To: ogerlitz; +Cc: netdev, amirv
In-Reply-To: <1409147269-21472-1-git-send-email-ogerlitz@mellanox.com>
From: Or Gerlitz <ogerlitz@mellanox.com>
Date: Wed, 27 Aug 2014 16:47:47 +0300
> This short series fixes the mlx4 driver setting of user space Ethernet QPs
> (e.g those opened by DPDK applications) such that they will properly handle
> VXLAN traffic/offloads
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH 1/4] net: stmmac: enhance to support multiple device instances
From: David Miller @ 2014-08-30 3:06 UTC (permalink / raw)
To: hock.leong.kweh; +Cc: peppe.cavallaro, netdev, linux-kernel, boon.leong.ong
In-Reply-To: <cf29eb9e793cf67b99b455f3a4a55e7aca5d826c.1409123378.git.hock.leong.kweh@intel.com>
From: Kweh Hock Leong <hock.leong.kweh@intel.com>
Date: Wed, 27 Aug 2014 18:32:26 +0800
> @@ -26,27 +26,22 @@
> #include <linux/pci.h>
> #include "stmmac.h"
>
> -static struct plat_stmmacenet_data plat_dat;
> -static struct stmmac_mdio_bus_data mdio_data;
> -static struct stmmac_dma_cfg dma_cfg;
> +static int instance_id = 1;
Don't do this instance stuff. Instead pull in some identifier that
can come from elsewhere.
> + plat_dat->mdio_bus_data = devm_kzalloc(&pdev->dev,
> + sizeof(*plat_dat->mdio_bus_data),
> + GFP_KERNEL);
This is not indented properly.
On the second and subsequent lines of a multi-line function call,
the lines should start exactly at the first column after the openning
parenthesis of the first line.
You must use the correct number of TAB and SPACE characters necessary
to do so. Generally speaking, if you are indenting using only TAB
characters, odds are you are doing it wrong.
Please audit for, and fix this, in your entire patch series.
^ permalink raw reply
* Re: [PATCH net-next v2] net: add skb_get_tx_queue() helper
From: David Miller @ 2014-08-30 3:02 UTC (permalink / raw)
To: dborkman; +Cc: therbert, netdev
In-Reply-To: <1409130687-30871-1-git-send-email-dborkman@redhat.com>
From: Daniel Borkmann <dborkman@redhat.com>
Date: Wed, 27 Aug 2014 11:11:27 +0200
> Replace occurences of skb_get_queue_mapping() and follow-up
> netdev_get_tx_queue() with an actual helper function.
>
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> ---
> v1->v2:
> - Lets keep skb_get_tx_queue() a one-liner
Applied, thanks Daniel.
^ permalink raw reply
* Re: net_ns cleanup / RCU overhead
From: Paul E. McKenney @ 2014-08-30 2:56 UTC (permalink / raw)
To: Florian Westphal
Cc: Eric W. Biederman, Julian Anastasov, Simon Kirby, linux-kernel,
netdev, Pablo Neira Ayuso
In-Reply-To: <20140829235206.GA15853@breakpoint.cc>
On Sat, Aug 30, 2014 at 01:52:06AM +0200, Florian Westphal wrote:
> Eric W. Biederman <ebiederm@xmission.com> wrote:
> > Julian Anastasov <ja@ssi.bg> writes:
> >
> > > Hello,
> > >
> > > On Thu, 28 Aug 2014, Simon Kirby wrote:
> > >
> > >> I noticed that [kworker/u16:0]'s stack is often:
> > >>
> > >> [<ffffffff810942a6>] wait_rcu_gp+0x46/0x50
> > >> [<ffffffff8109607e>] synchronize_sched+0x2e/0x50
> > >> [<ffffffffa00385ac>] nf_nat_net_exit+0x2c/0x50 [nf_nat]
> > >
> > > I guess the problem is in nf_nat_net_exit,
> > > may be other nf exit handlers too. pernet-exit handlers
> > > should avoid synchronize_rcu and rcu_barrier.
> > > A RCU callback and rcu_barrier in module-exit is the way
> > > to go. cleanup_net includes rcu_barrier, so pernet-exit
> > > does not need such calls.
> >
> > In principle I agree, however in this particular case it looks a bit
> > tricky because a separate hash table to track nat state per network
> > namespace.
> >
> > At the same time all of the packets should be drained before
> > we get to nf_nat_net_exit so it doesn't look the synchronize_rcu
> > in nf_nat_exit is actually protecting anything.
>
> Hmm, the problem is with the conntrack entries living in the netns being
> destroyed.
>
> I don't think they are guaranteed to be removed by the time
> the nat netns exit function runs.
>
> > Further calling a rcu delay function in net_exit methods largely
> > destroys the batched cleanup of network namespaces, so it is very
> > unpleasant.
> >
> > Could someone who knows nf_nat_core.c better than I do look and
> > see if we can just remove the synchronize_rcu in nf_nat_exit?
>
> If I remember correctly its needed to ensure that
> all conntracks with nat extensions that might still be referenced
> on other cpu have finished (i.e., nf_conntrack_destroy() has been
> called, which calls nf_nat_cleanup_conntrack() which deletes
> the extension from the hash table).
>
> As we remove the ct from that table ourselves EXCEPT in the
> case where we cannot steal the timers' reference we should
> be able to avoid that call virtually every time.
>
> Perhaps this is worth a shot (not even compile tested):
>
> diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
> index 4e0b478..80cfe10 100644
> --- a/net/netfilter/nf_nat_core.c
> +++ b/net/netfilter/nf_nat_core.c
> @@ -508,6 +508,7 @@ EXPORT_SYMBOL_GPL(nf_nat_packet);
> struct nf_nat_proto_clean {
> u8 l3proto;
> u8 l4proto;
> + bool need_sync_rcu;
> };
>
> /* kill conntracks with affected NAT section */
> @@ -528,23 +529,32 @@ static int nf_nat_proto_remove(struct nf_conn *i, void *data)
>
> static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
> {
> + struct nf_nat_proto_clean *clean = data;
> struct nf_conn_nat *nat = nfct_nat(ct);
>
> - if (nf_nat_proto_remove(ct, data))
> - return 1;
> -
> if (!nat || !nat->ct)
> return 0;
>
> - /* This netns is being destroyed, and conntrack has nat null binding.
> + /* This netns is being destroyed, and conntrack has nat binding.
> * Remove it from bysource hash, as the table will be freed soon.
> *
> - * Else, when the conntrack is destoyed, nf_nat_cleanup_conntrack()
> + * Else, when the conntrack is destroyed, nf_nat_cleanup_conntrack()
> * will delete entry from already-freed table.
> */
> - if (!del_timer(&ct->timeout))
> + if (!del_timer(&ct->timeout)) {
> + /* We have nat binding, but destruction
> + * might already be in progress.
> + *
> + * nat entry is removed only after last
> + * nf_ct_put().
> + */
> + clean->need_sync_rcu = true;
So this happens only if we race with the timer handler? If so, this
patch might give good speedups. (Can't comment on any other correctness
issues due to unfamiliarity with the code and what it is trying to do.)
Thanx, Paul
> return 1;
> + }
>
> + /* We stole refcount owned by timer;
> + * conntrack cannot go away.
> + */
> spin_lock_bh(&nf_nat_lock);
> hlist_del_rcu(&nat->bysource);
> ct->status &= ~IPS_NAT_DONE_MASK;
> @@ -553,6 +563,9 @@ static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
>
> add_timer(&ct->timeout);
>
> + if (nf_nat_proto_remove(ct, data))
> + return 1;
> +
> /* don't delete conntrack. Although that would make things a lot
> * simpler, we'd end up flushing all conntracks on nat rmmod.
> */
> @@ -830,7 +843,8 @@ static void __net_exit nf_nat_net_exit(struct net *net)
> struct nf_nat_proto_clean clean = {};
>
> nf_ct_iterate_cleanup(net, nf_nat_proto_clean, &clean, 0, 0);
> - synchronize_rcu();
> + if (clean.need_sync_rcu)
> + synchronize_rcu();
> nf_ct_free_hashtable(net->ct.nat_bysource, net->ct.nat_htable_size);
> }
>
>
^ permalink raw reply
* Re: [PATCH (net.git)] stmmac: fix dma api misuse
From: David Miller @ 2014-08-30 2:56 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev, andre.draszik, hdegoede
In-Reply-To: <1409131620-16888-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Wed, 27 Aug 2014 11:27:00 +0200
> Enabling DMA_API_DEBUG, warnings are reported at runtime
> because the device driver frees DMA memory with wrong functions
> and it does not call dma_mapping_error after mapping dma memory.
>
> The first problem is fixed by of introducing a flag that helps us
> keeping track which mapping technique was used, so that we can use
> the right API for unmap.
> This approach was inspired by the e1000 driver, which uses a similar
> technique.
>
> Signed-off-by: Andre Draszik <andre.draszik@st.com>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Reviewed-by: Denis Kirjanov <kda@linux-powerpc.org>
> Cc: Hans de Goede <hdegoede@redhat.com>
Applied.
^ permalink raw reply
* Re: [PATCH (net.git) (v2)] stmmac: ptp: fix the reference clock
From: David Miller @ 2014-08-30 2:55 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1409128669-14276-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Wed, 27 Aug 2014 10:37:49 +0200
> The PTP reference clock, used for setting the addend in the Timestamp Addend
> Register, was erroneously hard-coded (as reported in the databook just as
> example).
>
> The patch removes the macro named: STMMAC_SYSCLOCK and allows to use a
> reference clock (clk_ptp_ref_i) that can be passed from the platform.
>
> If not passed, the main driver clock will be used as default; note that
> this can be fine on some platforms.
>
> Note that, prior this patch, using the old STMMAC_SYSCLOCK on some platforms,
> as side effect, the ptp clock can move faster/slower than the system clock.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> ---
> v2: fixed the release function in case of the new clock is not provided by the
> platform.
Applied.
^ permalink raw reply
* Re: [PATCH (net.git)] stmmac: fix tipo on mmc crc error
From: David Miller @ 2014-08-30 2:55 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1409120773-28003-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Wed, 27 Aug 2014 08:26:13 +0200
> From: Giuseppe CAVALLARO <peppe.cavallaro@st.com>
>
> This patch is to fix a typo on mmc rx crc error when reported by ethtool.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Applied.
^ permalink raw reply
* Re: [PATCH (net.git)] stmmac: prevent false carrier sense detection
From: David Miller @ 2014-08-30 2:55 UTC (permalink / raw)
To: peppe.cavallaro; +Cc: netdev
In-Reply-To: <1409120404-20414-1-git-send-email-peppe.cavallaro@st.com>
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Wed, 27 Aug 2014 08:20:04 +0200
> This patch is to w/a a problem that happens on some boxes when run at 10Mbps
> Half duplex mode.
>
> During the transmission the CSR signal is asserted for some time and the frames
> aborted because of carrier sense error.
> This is reported by MMC HW counter: txcarrier signal.
> This actually is a false carrier so the frames are good and there is no reason
> to ask for dropping them.
>
> This patch so disables the Carrier Sense During Transmission
> and this means that the MAC transmitter ignore the CRS signal
> during frame transmission in Half-Duplex mode.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Acked-by: Vince Bridgers <vbridgers2013@gmail.com>
> Acked-by: Ley Foon Tan <lftan@altera.com>
> Acked-by: Chen-Yu Tsai <wens@csie.org>
Applied.
Although it would have made more sense to me if you only set the DCRS bit
in the case where the troubles occur, half-duplex 10Mbps, so that in other
modes real carrier loss during transmit would actually be detected.
^ permalink raw reply
* Re: net_ns cleanup / RCU overhead
From: Paul E. McKenney @ 2014-08-30 2:52 UTC (permalink / raw)
To: Simon Kirby; +Cc: Eric W. Biederman, linux-kernel, netdev
In-Reply-To: <20140829004029.GA18300@hostway.ca>
On Thu, Aug 28, 2014 at 05:40:29PM -0700, Simon Kirby wrote:
> On Thu, Aug 28, 2014 at 01:46:58PM -0700, Paul E. McKenney wrote:
>
> > On Thu, Aug 28, 2014 at 03:33:42PM -0500, Eric W. Biederman wrote:
> >
> > > I just want to add a little bit more analysis to this.
> > >
> > > What we desire to be fast is the copy_net_ns, cleanup_net is batched and
> > > asynchronous which nothing really cares how long it takes except that
> > > cleanup_net holds the net_mutex and thus blocks copy_net_ns.
> > >
> > > The puzzle is why and which rcu delays Simon is seeing in the network
> > > namespace cleanup path, as it seems like the synchronize_rcu is not
> > > the only one, and in the case of vsftp with trivail network namespaces
> > > where nothing has been done we should not need to delay.
> >
> > Indeed, given the version and .config, I can't see why any individual
> > RCU grace-period operation would be particularly slow.
> >
> > I suggest using ftrace on synchronize_rcu() and friends.
>
> I made a parallel net namespace create/destroy benchmark that prints the
> progress and time to create and cleanup 32 unshare()d child processes:
>
> http://0x.ca/sim/ref/tools/netnsbench.c
>
> I noticed that if I haven't run it for a while, the first batch often is
> fast, followed by slowness from then on:
>
> ++++++++++++++++++++++++++++++++-------------------------------- 0.039478s
> ++++++++++++++++++++-----+----------------+++++++++---------++-- 4.463837s
> +++++++++++++++++++++++++------+--------------------++++++------ 3.011882s
> +++++++++++++++---+-------------++++++++++++++++---------------- 2.283993s
>
> Fiddling around on a stock kernel, "echo 1 > /sys/kernel/rcu_expedited"
> makes behaviour change as it did with my patch:
>
> ++-++-+++-+-----+-+-++-+-++--++-+--+-+-++--++-+-+-+-++-+--++---- 0.801406s
> +-+-+-++-+-+-+-+-++--+-+-++-+--++-+-+-+-+-+-+-+-+-+-+-+--++-+--- 0.872011s
> ++--+-++--+-++--+-++--+-+-+-+-++-+--++--+-++-+-+-+-+--++-+-+-+-- 0.946745s
>
> How would I use ftrace on synchronize_rcu() here?
http://lwn.net/Articles/370423/ is your friend here. If your kernel
is built with the needed configuration, you give the command
"echo synchronize_rcu > set_ftrace_filter"
http://lwn.net/Articles/365835/ and http://lwn.net/Articles/366796/
have background info.
> As Eric said, cleanup_net() is batched, but while it is cleaning up,
> net_mutex is held. Isn't the issue just that net_mutex is held while
> some other things are going on that are meant to be lazy / batched?
>
> What is net_mutex protecting in cleanup_net()?
>
> I noticed that [kworker/u16:0]'s stack is often:
>
> [<ffffffff810942a6>] wait_rcu_gp+0x46/0x50
> [<ffffffff8109607e>] synchronize_sched+0x2e/0x50
> [<ffffffffa00385ac>] nf_nat_net_exit+0x2c/0x50 [nf_nat]
> [<ffffffff81720339>] ops_exit_list.isra.4+0x39/0x60
> [<ffffffff817209e0>] cleanup_net+0xf0/0x1a0
> [<ffffffff81062997>] process_one_work+0x157/0x440
> [<ffffffff81063303>] worker_thread+0x63/0x520
> [<ffffffff81068b96>] kthread+0xd6/0xf0
> [<ffffffff818d412c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> and
>
> [<ffffffff81095364>] _rcu_barrier+0x154/0x1f0
> [<ffffffff81095450>] rcu_barrier+0x10/0x20
> [<ffffffff81102c2c>] kmem_cache_destroy+0x6c/0xb0
> [<ffffffffa0089e97>] nf_conntrack_cleanup_net_list+0x167/0x1c0 [nf_conntrack]
> [<ffffffffa008aab5>] nf_conntrack_pernet_exit+0x65/0x70 [nf_conntrack]
> [<ffffffff81720353>] ops_exit_list.isra.4+0x53/0x60
> [<ffffffff817209e0>] cleanup_net+0xf0/0x1a0
> [<ffffffff81062997>] process_one_work+0x157/0x440
> [<ffffffff81063303>] worker_thread+0x63/0x520
> [<ffffffff81068b96>] kthread+0xd6/0xf0
> [<ffffffff818d412c>] ret_from_fork+0x7c/0xb0
> [<ffffffffffffffff>] 0xffffffffffffffff
>
> So I tried flushing iptables rules and rmmoding netfilter bits:
>
> ++++++++++++++++++++-+--------------------+++++++++++----------- 0.179940s
> ++++++++++++++--+-------------+++++++++++++++++----------------- 0.151988s
> ++++++++++++++++++++++++++++---+--------------------------+++--- 0.159967s
> ++++++++++++++++++++++----------------------++++++++++---------- 0.175964s
>
> Expedited:
>
> ++-+--++-+-+-+-+-+-+--++-+-+-++-+-+-+--++-+-+-+-+-+-+-+-+-+-+--- 0.079988s
> ++-+-+-+-+-+-+-+-+-+-+-+--++-+--++-+--+-++-+-+--++-+-+-+-+-+-+-- 0.089347s
> ++++--+++--++--+-+++++++-+++++--------------++-+-+--++-+-+--++-- 0.081566s
> +++++-+++-------++-+-+-+-+-+-+-+-+-+-+-++-+-+-+-+-+-+-+-+-+-+--- 0.089026s
>
> So, much faster. It seems that just loading nf_conntrack_ipv4 (like by
> running iptables -t nat -nvL) is enough to slow it way down. But it is
> still capable of being fast, as above.
My first guess is that this code sequence is calling synchronize_rcu()
quite often. Would it be possible to consolidate these?
Thanx, Paul
^ permalink raw reply
* [PATCH net-next 1/2] ipv6: add sysctl_mld_qrv to configure query robustness variable
From: Hannes Frederic Sowa @ 2014-08-30 2:42 UTC (permalink / raw)
To: netdev
This patch adds a new sysctl_mld_qrv knob to configure the mldv1/v2 query
robustness variable. It sepcifies how many retransmit of unsolicited mld
retransmit should happen. Admins might want to tune this on lossy links.
Also reset mld state on interface down/up, so we pick up new sysctl
settings during interface up event.
IPv6 certification requests this knob to be available.
I didn't make this knob netns specific, as it is mostly a setting in a
physical environment and should be per host.
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
Documentation/networking/ip-sysctl.txt | 3 +++
include/net/ipv6.h | 1 +
net/ipv6/mcast.c | 20 ++++++++++++--------
net/ipv6/sysctl_net_ipv6.c | 10 ++++++++++
4 files changed, 26 insertions(+), 8 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index 3cce8ea..b7fe844 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -1152,6 +1152,9 @@ anycast_src_echo_reply - BOOLEAN
FALSE: disabled
Default: FALSE
+mld_qrv - INTEGER
+ Controls the MLD query robustness variable (see RFC3810 9.1).
+
IPv6 Fragmentation:
ip6frag_high_thresh - INTEGER
diff --git a/include/net/ipv6.h b/include/net/ipv6.h
index a2db816..7e247e9 100644
--- a/include/net/ipv6.h
+++ b/include/net/ipv6.h
@@ -121,6 +121,7 @@ struct frag_hdr {
/* sysctls */
extern int sysctl_mld_max_msf;
+extern int sysctl_mld_qrv;
#define _DEVINC(net, statname, modifier, idev, field) \
({ \
diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
index 7088179..6efb0e5 100644
--- a/net/ipv6/mcast.c
+++ b/net/ipv6/mcast.c
@@ -121,6 +121,7 @@ static int ip6_mc_leave_src(struct sock *sk, struct ipv6_mc_socklist *iml,
#define IPV6_MLD_MAX_MSF 64
int sysctl_mld_max_msf __read_mostly = IPV6_MLD_MAX_MSF;
+int sysctl_mld_qrv __read_mostly = MLD_QRV_DEFAULT;
/*
* socket join on multicast group
@@ -1196,7 +1197,7 @@ static void mld_update_qrv(struct inet6_dev *idev,
if (mlh2->mld2q_qrv > 0)
idev->mc_qrv = mlh2->mld2q_qrv;
- if (unlikely(idev->mc_qrv < 2)) {
+ if (unlikely(idev->mc_qrv < MLD_QRV_DEFAULT)) {
net_warn_ratelimited("IPv6: MLD: clamping QRV from %u to %u!\n",
idev->mc_qrv, MLD_QRV_DEFAULT);
idev->mc_qrv = MLD_QRV_DEFAULT;
@@ -2478,6 +2479,14 @@ void ipv6_mc_down(struct inet6_dev *idev)
mld_clear_delrec(idev);
}
+static void ipv6_mc_reset(struct inet6_dev *idev)
+{
+ idev->mc_qrv = sysctl_mld_qrv;
+ idev->mc_qi = MLD_QI_DEFAULT;
+ idev->mc_qri = MLD_QRI_DEFAULT;
+ idev->mc_v1_seen = 0;
+ idev->mc_maxdelay = unsolicited_report_interval(idev);
+}
/* Device going up */
@@ -2488,6 +2497,7 @@ void ipv6_mc_up(struct inet6_dev *idev)
/* Install multicast list, except for all-nodes (already installed) */
read_lock_bh(&idev->lock);
+ ipv6_mc_reset(idev);
for (i = idev->mc_list; i; i = i->next)
igmp6_group_added(i);
read_unlock_bh(&idev->lock);
@@ -2508,13 +2518,7 @@ void ipv6_mc_init_dev(struct inet6_dev *idev)
(unsigned long)idev);
setup_timer(&idev->mc_dad_timer, mld_dad_timer_expire,
(unsigned long)idev);
-
- idev->mc_qrv = MLD_QRV_DEFAULT;
- idev->mc_qi = MLD_QI_DEFAULT;
- idev->mc_qri = MLD_QRI_DEFAULT;
-
- idev->mc_maxdelay = unsolicited_report_interval(idev);
- idev->mc_v1_seen = 0;
+ ipv6_mc_reset(idev);
write_unlock_bh(&idev->lock);
}
diff --git a/net/ipv6/sysctl_net_ipv6.c b/net/ipv6/sysctl_net_ipv6.c
index 0c56c93..c5c10fa 100644
--- a/net/ipv6/sysctl_net_ipv6.c
+++ b/net/ipv6/sysctl_net_ipv6.c
@@ -16,6 +16,8 @@
#include <net/addrconf.h>
#include <net/inet_frag.h>
+static int one = 1;
+
static struct ctl_table ipv6_table_template[] = {
{
.procname = "bindv6only",
@@ -63,6 +65,14 @@ static struct ctl_table ipv6_rotable[] = {
.mode = 0644,
.proc_handler = proc_dointvec
},
+ {
+ .procname = "mld_qrv",
+ .data = &sysctl_mld_qrv,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &one
+ },
{ }
};
--
1.9.3
^ permalink raw reply related
* [PATCH net-next 2/2] ipv4: implement igmp_qrv sysctl to tune igmp robustness variable
From: Hannes Frederic Sowa @ 2014-08-30 2:42 UTC (permalink / raw)
To: netdev
In-Reply-To: <1880a888c65dff7b7c73aa53eabbb841a65a74ce.1409366462.git.hannes@stressinduktion.org>
As in IPv6 people might increase the igmp query robustness variable to
make sure unsolicited state change reports aren't lost on the network. Add
and document this new knob to igmp code.
RFCs allow tuning this parameter back to first IGMP RFC, so we also use
this setting for all counters, including source specific multicast.
Also take over sysctl value when upping the interface and don't reuse
the last one seen on the interface.
Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
---
Documentation/networking/ip-sysctl.txt | 3 +++
include/linux/igmp.h | 1 +
net/ipv4/igmp.c | 27 +++++++++++----------------
net/ipv4/sysctl_net_ipv4.c | 8 ++++++++
4 files changed, 23 insertions(+), 16 deletions(-)
diff --git a/Documentation/networking/ip-sysctl.txt b/Documentation/networking/ip-sysctl.txt
index b7fe844..a5adddc 100644
--- a/Documentation/networking/ip-sysctl.txt
+++ b/Documentation/networking/ip-sysctl.txt
@@ -844,6 +844,9 @@ igmp_max_memberships - INTEGER
conf/all/* is special, changes the settings for all interfaces
+igmp_qrv - INTEGER
+ Controls the IGMP query robustness variable (see RFC2236 8.1).
+
log_martians - BOOLEAN
Log packets with impossible addresses to kernel log.
log_martians for the interface will be enabled if at least one of
diff --git a/include/linux/igmp.h b/include/linux/igmp.h
index f47550d..2c677af 100644
--- a/include/linux/igmp.h
+++ b/include/linux/igmp.h
@@ -39,6 +39,7 @@ static inline struct igmpv3_query *
extern int sysctl_igmp_max_memberships;
extern int sysctl_igmp_max_msf;
+extern int sysctl_igmp_qrv;
struct ip_sf_socklist {
unsigned int sl_max;
diff --git a/net/ipv4/igmp.c b/net/ipv4/igmp.c
index 890c425..1a38651 100644
--- a/net/ipv4/igmp.c
+++ b/net/ipv4/igmp.c
@@ -117,7 +117,7 @@
#define IGMP_V2_Unsolicited_Report_Interval (10*HZ)
#define IGMP_V3_Unsolicited_Report_Interval (1*HZ)
#define IGMP_Query_Response_Interval (10*HZ)
-#define IGMP_Unsolicited_Report_Count 2
+#define IGMP_Query_Robustness_Variable 2
#define IGMP_Initial_Report_Delay (1)
@@ -756,8 +756,7 @@ static void igmp_ifc_event(struct in_device *in_dev)
{
if (IGMP_V1_SEEN(in_dev) || IGMP_V2_SEEN(in_dev))
return;
- in_dev->mr_ifc_count = in_dev->mr_qrv ? in_dev->mr_qrv :
- IGMP_Unsolicited_Report_Count;
+ in_dev->mr_ifc_count = in_dev->mr_qrv ?: sysctl_igmp_qrv;
igmp_ifc_start_timer(in_dev, 1);
}
@@ -1086,8 +1085,7 @@ static void igmpv3_add_delrec(struct in_device *in_dev, struct ip_mc_list *im)
pmc->interface = im->interface;
in_dev_hold(in_dev);
pmc->multiaddr = im->multiaddr;
- pmc->crcount = in_dev->mr_qrv ? in_dev->mr_qrv :
- IGMP_Unsolicited_Report_Count;
+ pmc->crcount = in_dev->mr_qrv ?: sysctl_igmp_qrv;
pmc->sfmode = im->sfmode;
if (pmc->sfmode == MCAST_INCLUDE) {
struct ip_sf_list *psf;
@@ -1226,8 +1224,7 @@ static void igmp_group_added(struct ip_mc_list *im)
}
/* else, v3 */
- im->crcount = in_dev->mr_qrv ? in_dev->mr_qrv :
- IGMP_Unsolicited_Report_Count;
+ im->crcount = in_dev->mr_qrv ?: sysctl_igmp_qrv;
igmp_ifc_event(in_dev);
#endif
}
@@ -1322,7 +1319,7 @@ void ip_mc_inc_group(struct in_device *in_dev, __be32 addr)
spin_lock_init(&im->lock);
#ifdef CONFIG_IP_MULTICAST
setup_timer(&im->timer, igmp_timer_expire, (unsigned long)im);
- im->unsolicit_count = IGMP_Unsolicited_Report_Count;
+ im->unsolicit_count = sysctl_igmp_qrv;
#endif
im->next_rcu = in_dev->mc_list;
@@ -1460,7 +1457,7 @@ void ip_mc_init_dev(struct in_device *in_dev)
(unsigned long)in_dev);
setup_timer(&in_dev->mr_ifc_timer, igmp_ifc_timer_expire,
(unsigned long)in_dev);
- in_dev->mr_qrv = IGMP_Unsolicited_Report_Count;
+ in_dev->mr_qrv = sysctl_igmp_qrv;
#endif
spin_lock_init(&in_dev->mc_tomb_lock);
@@ -1474,6 +1471,7 @@ void ip_mc_up(struct in_device *in_dev)
ASSERT_RTNL();
+ in_dev->mr_qrv = sysctl_igmp_qrv;
ip_mc_inc_group(in_dev, IGMP_ALL_HOSTS);
for_each_pmc_rtnl(in_dev, pmc)
@@ -1540,7 +1538,7 @@ static struct in_device *ip_mc_find_dev(struct net *net, struct ip_mreqn *imr)
*/
int sysctl_igmp_max_memberships __read_mostly = IP_MAX_MEMBERSHIPS;
int sysctl_igmp_max_msf __read_mostly = IP_MAX_MSF;
-
+int sysctl_igmp_qrv __read_mostly = IGMP_Query_Robustness_Variable;
static int ip_mc_del1_src(struct ip_mc_list *pmc, int sfmode,
__be32 *psfsrc)
@@ -1575,8 +1573,7 @@ static int ip_mc_del1_src(struct ip_mc_list *pmc, int sfmode,
#ifdef CONFIG_IP_MULTICAST
if (psf->sf_oldin &&
!IGMP_V1_SEEN(in_dev) && !IGMP_V2_SEEN(in_dev)) {
- psf->sf_crcount = in_dev->mr_qrv ? in_dev->mr_qrv :
- IGMP_Unsolicited_Report_Count;
+ psf->sf_crcount = in_dev->mr_qrv ?: sysctl_igmp_qrv;
psf->sf_next = pmc->tomb;
pmc->tomb = psf;
rv = 1;
@@ -1639,8 +1636,7 @@ static int ip_mc_del_src(struct in_device *in_dev, __be32 *pmca, int sfmode,
/* filter mode change */
pmc->sfmode = MCAST_INCLUDE;
#ifdef CONFIG_IP_MULTICAST
- pmc->crcount = in_dev->mr_qrv ? in_dev->mr_qrv :
- IGMP_Unsolicited_Report_Count;
+ pmc->crcount = in_dev->mr_qrv ?: sysctl_igmp_qrv;
in_dev->mr_ifc_count = pmc->crcount;
for (psf = pmc->sources; psf; psf = psf->sf_next)
psf->sf_crcount = 0;
@@ -1818,8 +1814,7 @@ static int ip_mc_add_src(struct in_device *in_dev, __be32 *pmca, int sfmode,
#ifdef CONFIG_IP_MULTICAST
/* else no filters; keep old mode for reports */
- pmc->crcount = in_dev->mr_qrv ? in_dev->mr_qrv :
- IGMP_Unsolicited_Report_Count;
+ pmc->crcount = in_dev->mr_qrv ?: sysctl_igmp_qrv;
in_dev->mr_ifc_count = pmc->crcount;
for (psf = pmc->sources; psf; psf = psf->sf_next)
psf->sf_crcount = 0;
diff --git a/net/ipv4/sysctl_net_ipv4.c b/net/ipv4/sysctl_net_ipv4.c
index 79a007c..a942c48 100644
--- a/net/ipv4/sysctl_net_ipv4.c
+++ b/net/ipv4/sysctl_net_ipv4.c
@@ -451,6 +451,14 @@ static struct ctl_table ipv4_table[] = {
.proc_handler = proc_dointvec
},
{
+ .procname = "igmp_qrv",
+ .data = &sysctl_igmp_qrv,
+ .maxlen = sizeof(int),
+ .mode = 0644,
+ .proc_handler = proc_dointvec_minmax,
+ .extra1 = &one
+ },
+ {
.procname = "inet_peer_threshold",
.data = &inet_peer_threshold,
.maxlen = sizeof(int),
--
1.9.3
^ permalink raw reply related
* Re: RTNL: assertion failed at net/ipv6/addrconf.c (1699)
From: Hannes Frederic Sowa @ 2014-08-30 1:51 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: Cong Wang, Tommi Rantala, David S. Miller, Alexey Kuznetsov,
James Morris, Hideaki YOSHIFUJI, Patrick McHardy, netdev, LKML,
trinity, Dave Jones
In-Reply-To: <20140829195339.GA9780@kria>
Hi Sabrina,
On Fr, 2014-08-29 at 21:53 +0200, Sabrina Dubroca wrote:
> 2014-08-29, 11:14:48 -0700, Cong Wang wrote:
> > On Fri, Aug 29, 2014 at 8:26 AM, Tommi Rantala <tt.rantala@gmail.com> wrote:
> > > [ 77.297196] RTNL: assertion failed at net/ipv6/addrconf.c (1699)
> > > [ 77.298080] CPU: 0 PID: 4842 Comm: trinity-main Not tainted 3.17.0-rc2+ #30
> > > [ 77.299039] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
> > > [ 77.299789] ffff88003d76a618 ffff880026133c50 ffffffff8238ba79
> > > ffff880037c84520
> > > [ 77.300829] ffff880026133c90 ffffffff820bd52b 0000000000000000
> > > ffffffff82d86c40
> > > [ 77.301869] 0000000000000000 00000000f76fd1e1 ffff8800382d8000
> > > ffff8800382d8220
> > > [ 77.302906] Call Trace:
> > > [ 77.303246] [<ffffffff8238ba79>] dump_stack+0x4d/0x66
> > > [ 77.303928] [<ffffffff820bd52b>] addrconf_join_solict+0x4b/0xb0
> > > [ 77.304731] [<ffffffff820b031b>] ipv6_dev_ac_inc+0x2bb/0x330
> > > [ 77.305498] [<ffffffff820b0060>] ? ac6_seq_start+0x260/0x260
> > > [ 77.306257] [<ffffffff820b05fe>] ipv6_sock_ac_join+0x26e/0x360
> > > [ 77.307046] [<ffffffff820b0429>] ? ipv6_sock_ac_join+0x99/0x360
> > > [ 77.307798] [<ffffffff820cdd60>] do_ipv6_setsockopt.isra.5+0xa70/0xf20
> >
> >
> > I think we should just use rtnl_lock() instead of rcu_read_lock() there,
> > it is not a hot path worth optimization.
> >
> > Please try the attached patch.
>
> note: it doesn't build as it is now, it needs:
>
> -EXPORT_SYMBOL(dev_get_by_flags_rcu);
> +EXPORT_SYMBOL(dev_get_by_flags);
>
>
> I just tried your patch with a basic test program (open
> socket/join/leave/close and open socket/join/close).
>
> I think you need to modify ipv6_sock_ac_close as well, or you can still
> trigger the assertion when closing the socket without leaving first.
>
> Modified patch attached.
Sorry, just had time to look at this.
The reason is not to have list corruption but that the calls down to
ndo_set_rx_mode expect rtnl to be locked by the drivers. Filter lists
are locked by addr_list_lock and that's why I think we never saw any
problems with that, but drivers expect rtnl locked for those calls.
But this problem also affects multicast join, so patch seems incomplete
to me (and for that matter ssm multicast join, too).
Also rtnl_lock and rcu_read_lock compose in that order, so we don't need
to change dev_get_by_flags, but as this is the only user it sure is
possible. RCU locked version is just easier composeable, so I wouldn't
touch that if needed in future, just also take rcu lock as before.
So just adding rtnl_lock add appropriate places seems to be ok to me,
but still need to review parts of the ssm code.
Also we should move ASSERT_RTNL checks from addrconf_join_solict to
ipv6_dev_mc_inc/dec.
Thanks,
Hannes
^ permalink raw reply
* Re: net_ns cleanup / RCU overhead
From: Florian Westphal @ 2014-08-29 23:52 UTC (permalink / raw)
To: Eric W. Biederman
Cc: Julian Anastasov, Simon Kirby, Paul E. McKenney, linux-kernel,
netdev, Florian Westphal, Pablo Neira Ayuso
In-Reply-To: <87ppfjj6x6.fsf@x220.int.ebiederm.org>
Eric W. Biederman <ebiederm@xmission.com> wrote:
> Julian Anastasov <ja@ssi.bg> writes:
>
> > Hello,
> >
> > On Thu, 28 Aug 2014, Simon Kirby wrote:
> >
> >> I noticed that [kworker/u16:0]'s stack is often:
> >>
> >> [<ffffffff810942a6>] wait_rcu_gp+0x46/0x50
> >> [<ffffffff8109607e>] synchronize_sched+0x2e/0x50
> >> [<ffffffffa00385ac>] nf_nat_net_exit+0x2c/0x50 [nf_nat]
> >
> > I guess the problem is in nf_nat_net_exit,
> > may be other nf exit handlers too. pernet-exit handlers
> > should avoid synchronize_rcu and rcu_barrier.
> > A RCU callback and rcu_barrier in module-exit is the way
> > to go. cleanup_net includes rcu_barrier, so pernet-exit
> > does not need such calls.
>
> In principle I agree, however in this particular case it looks a bit
> tricky because a separate hash table to track nat state per network
> namespace.
>
> At the same time all of the packets should be drained before
> we get to nf_nat_net_exit so it doesn't look the synchronize_rcu
> in nf_nat_exit is actually protecting anything.
Hmm, the problem is with the conntrack entries living in the netns being
destroyed.
I don't think they are guaranteed to be removed by the time
the nat netns exit function runs.
> Further calling a rcu delay function in net_exit methods largely
> destroys the batched cleanup of network namespaces, so it is very
> unpleasant.
>
> Could someone who knows nf_nat_core.c better than I do look and
> see if we can just remove the synchronize_rcu in nf_nat_exit?
If I remember correctly its needed to ensure that
all conntracks with nat extensions that might still be referenced
on other cpu have finished (i.e., nf_conntrack_destroy() has been
called, which calls nf_nat_cleanup_conntrack() which deletes
the extension from the hash table).
As we remove the ct from that table ourselves EXCEPT in the
case where we cannot steal the timers' reference we should
be able to avoid that call virtually every time.
Perhaps this is worth a shot (not even compile tested):
diff --git a/net/netfilter/nf_nat_core.c b/net/netfilter/nf_nat_core.c
index 4e0b478..80cfe10 100644
--- a/net/netfilter/nf_nat_core.c
+++ b/net/netfilter/nf_nat_core.c
@@ -508,6 +508,7 @@ EXPORT_SYMBOL_GPL(nf_nat_packet);
struct nf_nat_proto_clean {
u8 l3proto;
u8 l4proto;
+ bool need_sync_rcu;
};
/* kill conntracks with affected NAT section */
@@ -528,23 +529,32 @@ static int nf_nat_proto_remove(struct nf_conn *i, void *data)
static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
{
+ struct nf_nat_proto_clean *clean = data;
struct nf_conn_nat *nat = nfct_nat(ct);
- if (nf_nat_proto_remove(ct, data))
- return 1;
-
if (!nat || !nat->ct)
return 0;
- /* This netns is being destroyed, and conntrack has nat null binding.
+ /* This netns is being destroyed, and conntrack has nat binding.
* Remove it from bysource hash, as the table will be freed soon.
*
- * Else, when the conntrack is destoyed, nf_nat_cleanup_conntrack()
+ * Else, when the conntrack is destroyed, nf_nat_cleanup_conntrack()
* will delete entry from already-freed table.
*/
- if (!del_timer(&ct->timeout))
+ if (!del_timer(&ct->timeout)) {
+ /* We have nat binding, but destruction
+ * might already be in progress.
+ *
+ * nat entry is removed only after last
+ * nf_ct_put().
+ */
+ clean->need_sync_rcu = true;
return 1;
+ }
+ /* We stole refcount owned by timer;
+ * conntrack cannot go away.
+ */
spin_lock_bh(&nf_nat_lock);
hlist_del_rcu(&nat->bysource);
ct->status &= ~IPS_NAT_DONE_MASK;
@@ -553,6 +563,9 @@ static int nf_nat_proto_clean(struct nf_conn *ct, void *data)
add_timer(&ct->timeout);
+ if (nf_nat_proto_remove(ct, data))
+ return 1;
+
/* don't delete conntrack. Although that would make things a lot
* simpler, we'd end up flushing all conntracks on nat rmmod.
*/
@@ -830,7 +843,8 @@ static void __net_exit nf_nat_net_exit(struct net *net)
struct nf_nat_proto_clean clean = {};
nf_ct_iterate_cleanup(net, nf_nat_proto_clean, &clean, 0, 0);
- synchronize_rcu();
+ if (clean.need_sync_rcu)
+ synchronize_rcu();
nf_ct_free_hashtable(net->ct.nat_bysource, net->ct.nat_htable_size);
}
^ permalink raw reply related
* Re: [PATCH v8 net-next 2/2] net: filter: split filter.h and expose eBPF to user space
From: Alexei Starovoitov @ 2014-08-29 23:01 UTC (permalink / raw)
To: Daniel Borkmann
Cc: Alexei Starovoitov, David S. Miller, Ingo Molnar, Linus Torvalds,
Andy Lutomirski, Steven Rostedt, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Brendan Gregg, Namhyung Kim, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <5400FDA0.7000704-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
On Fri, Aug 29, 2014 at 3:24 PM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>>
>> Also I really dislike 'ebpf' name in all lower case. If we make such
>> header
>> file name, we would need to rename all macros and function names
>> to EBPF_... which I find very ugly looking. I think all good abbreviations
>> are
>> three letters :)
>
>
> I don't think we would have to name defines that way, really, that would be
> terrible. We can keep them simply *as is*. Not sure though why bpf.h +
> ebpf.h
> would be that bad. ;) I haven't tried it out yet, but if we would indeed run
> into a name collision, above proposal would resolve that.
imo it's a consistency issue. If main uapi header is ebpf.h then
corresponding kernel internal header should be ebpf.h as well
and kernel/ebpf/ directory and so on.
That's why I insist on uapi/linux/bpf.h and no other name.
Note I didn't move any of the BPF_ALU64_REG, BPF_ALU32_IMM
macros from linux/filter.h. Without them my verifier testsuite
won't compile, so more lines would be added to bpf.h in the future.
At that time we can take 45 lines out of uapi/linux/filter.h and move
them into bpf_common.h. My request is let's not fight about it
right now. We didn't even cross the bridge yet and arguing
about beauty of user apps that come in 30 patches from now...
These two patches are about _intent_ of making eBPF usable
from userspace, so I can move along with llvm.
Also worth noting that llmv will not be including this uapi/linux/bpf.h
It has its own infra to generate instructions. Look at:
tools/bpf/llvm/lib/Target/BPF/BPFInstrInfo.td
it's a special 'table definition' language for describing bits and fields
of instructions.
So these two patches are mainly establishing _intent_ and bpf.h file
name. That's why I'm so paranoid about naming.
btw, I've spent last two days writing syscall manpage :(
What is the best way to present it for review?
If I just attach it raw, it's unreadable... I can include a link
to html page, but man2html produces ugly pages comparing
to what 'man' command shows. Any nice man converters
that generate stuff seen on man7.org ?
^ permalink raw reply
* Re: RTNL: assertion failed at net/ipv6/addrconf.c (1699)
From: Cong Wang @ 2014-08-29 22:54 UTC (permalink / raw)
To: Sabrina Dubroca
Cc: Tommi Rantala, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Hannes Frederic Sowa, netdev,
LKML, trinity, Dave Jones
In-Reply-To: <20140829195339.GA9780@kria>
On Fri, Aug 29, 2014 at 12:53 PM, Sabrina Dubroca <sd@queasysnail.net> wrote:
> 2014-08-29, 11:14:48 -0700, Cong Wang wrote:
>> On Fri, Aug 29, 2014 at 8:26 AM, Tommi Rantala <tt.rantala@gmail.com> wrote:
>> > [ 77.297196] RTNL: assertion failed at net/ipv6/addrconf.c (1699)
>> > [ 77.298080] CPU: 0 PID: 4842 Comm: trinity-main Not tainted 3.17.0-rc2+ #30
>> > [ 77.299039] Hardware name: Bochs Bochs, BIOS Bochs 01/01/2011
>> > [ 77.299789] ffff88003d76a618 ffff880026133c50 ffffffff8238ba79
>> > ffff880037c84520
>> > [ 77.300829] ffff880026133c90 ffffffff820bd52b 0000000000000000
>> > ffffffff82d86c40
>> > [ 77.301869] 0000000000000000 00000000f76fd1e1 ffff8800382d8000
>> > ffff8800382d8220
>> > [ 77.302906] Call Trace:
>> > [ 77.303246] [<ffffffff8238ba79>] dump_stack+0x4d/0x66
>> > [ 77.303928] [<ffffffff820bd52b>] addrconf_join_solict+0x4b/0xb0
>> > [ 77.304731] [<ffffffff820b031b>] ipv6_dev_ac_inc+0x2bb/0x330
>> > [ 77.305498] [<ffffffff820b0060>] ? ac6_seq_start+0x260/0x260
>> > [ 77.306257] [<ffffffff820b05fe>] ipv6_sock_ac_join+0x26e/0x360
>> > [ 77.307046] [<ffffffff820b0429>] ? ipv6_sock_ac_join+0x99/0x360
>> > [ 77.307798] [<ffffffff820cdd60>] do_ipv6_setsockopt.isra.5+0xa70/0xf20
>>
>>
>> I think we should just use rtnl_lock() instead of rcu_read_lock() there,
>> it is not a hot path worth optimization.
>>
>> Please try the attached patch.
>
> note: it doesn't build as it is now, it needs:
>
> -EXPORT_SYMBOL(dev_get_by_flags_rcu);
> +EXPORT_SYMBOL(dev_get_by_flags);
>
>
> I just tried your patch with a basic test program (open
> socket/join/leave/close and open socket/join/close).
>
> I think you need to modify ipv6_sock_ac_close as well, or you can still
> trigger the assertion when closing the socket without leaving first.
You are absolutely right here.
Can I have your Signed-off-by and Tested-by before sending the patch
formally?
Thanks!
^ permalink raw reply
* Re: [PATCH v8 net-next 2/2] net: filter: split filter.h and expose eBPF to user space
From: Daniel Borkmann @ 2014-08-29 22:24 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Alexei Starovoitov, David S. Miller, Ingo Molnar, Linus Torvalds,
Andy Lutomirski, Steven Rostedt, Chema Gonzalez, Eric Dumazet,
Peter Zijlstra, Brendan Gregg, Namhyung Kim, H. Peter Anvin,
Andrew Morton, Kees Cook, Linux API,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
In-Reply-To: <CAADnVQJbgiUK1vt_SDEG6Yee-Ht67e2M82PrHb3Kx533BOF-rg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 08/29/2014 08:02 PM, Alexei Starovoitov wrote:
> On Fri, Aug 29, 2014 at 10:39 AM, Daniel Borkmann <dborkman-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
>> On 08/27/2014 10:37 PM, Alexei Starovoitov wrote:
>>>
>>> allow user space to generate eBPF programs
>>>
>>> uapi/linux/bpf.h: eBPF instruction set definition
>>>
>>> linux/filter.h: the rest
>>
>> Very sorry for being late, but just a thought since we're touching user
>> space headers anyway ...
>>
>> Wouldn't it be more consistent to have it organized as follows ...
>>
>> - uapi/linux/bpf.h : classic BPF instruction set parts only
>> - uapi/linux/ebpf.h : eBPF instruction set definition (which also
>> includes uapi/linux/bpf.h though)
>> ... and have ...
>>
>> - uapi/linux/filter.h : just include uapi/linux/bpf.h but rest is empty
>>
>> That way, it would be more consistent ...
>>
>> Old legacy application can stay with linux/filter.h; new applications
>> based on their needs can choose between linux/{e,}bpf.h and in the kernel,
>> we can just include linux/ebpf.h.
>>
>> Right now, it seems, an eBPF user space program would need to include
>> 2 header files in user space (linux/filter.h, linux/bpf.h) which I find
>> a bit confusing.
>
> It's been bugging me as well, but I suspect having it the way you
> described won't work. Mainly because we cannot do include <uapi/..>
> inside uapi/*.h, so we would need to do include <linux/bpf.h>
> inside uapi/linux/filter.h, but that will cause serious include path
> confusion. That was the reason I didn't simply do include <linux/filter.h>
> inside uapi/linux/bpf.h
>
> Also I really dislike 'ebpf' name in all lower case. If we make such header
> file name, we would need to rename all macros and function names
> to EBPF_... which I find very ugly looking. I think all good abbreviations are
> three letters :)
I don't think we would have to name defines that way, really, that would be
terrible. We can keep them simply *as is*. Not sure though why bpf.h + ebpf.h
would be that bad. ;) I haven't tried it out yet, but if we would indeed run
into a name collision, above proposal would resolve that.
^ permalink raw reply
* Business Proposal From Tokyo
From: Itsuki_Kaito @ 2014-08-29 12:41 UTC (permalink / raw)
To: info
August 29, 2014
Konnichiwa ! It is with respect to directly write this proposal letter to
you, informing you of a potential business
proposal project that can be established from your country with your help,
which will mutually be profitable to us having no risk involved.
If you are agreeable to this business proposal, please indicate your
interest by giving me a direct response email. Feel free to contact me via
electronic mail or telephone for further discussion. I look forward to
hearing from you positively on this proposal.
Domo arigato !
Suki
^ permalink raw reply
* Re: net_ns cleanup / RCU overhead
From: Eric W. Biederman @ 2014-08-29 21:57 UTC (permalink / raw)
To: Julian Anastasov
Cc: Simon Kirby, Paul E. McKenney, linux-kernel, netdev,
Florian Westphal, Pablo Neira Ayuso
In-Reply-To: <alpine.LFD.2.11.1408290635530.1520@ja.home.ssi.bg>
Julian Anastasov <ja@ssi.bg> writes:
> Hello,
>
> On Thu, 28 Aug 2014, Simon Kirby wrote:
>
>> I noticed that [kworker/u16:0]'s stack is often:
>>
>> [<ffffffff810942a6>] wait_rcu_gp+0x46/0x50
>> [<ffffffff8109607e>] synchronize_sched+0x2e/0x50
>> [<ffffffffa00385ac>] nf_nat_net_exit+0x2c/0x50 [nf_nat]
>
> I guess the problem is in nf_nat_net_exit,
> may be other nf exit handlers too. pernet-exit handlers
> should avoid synchronize_rcu and rcu_barrier.
> A RCU callback and rcu_barrier in module-exit is the way
> to go. cleanup_net includes rcu_barrier, so pernet-exit
> does not need such calls.
In principle I agree, however in this particular case it looks a bit
tricky because a separate hash table to track nat state per network
namespace.
At the same time all of the packets should be drained before
we get to nf_nat_net_exit so it doesn't look the synchronize_rcu
in nf_nat_exit is actually protecting anything.
Further calling a rcu delay function in net_exit methods largely
destroys the batched cleanup of network namespaces, so it is very
unpleasant.
Could someone who knows nf_nat_core.c better than I do look and
see if we can just remove the synchronize_rcu in nf_nat_exit?
>> [<ffffffff81720339>] ops_exit_list.isra.4+0x39/0x60
>> [<ffffffff817209e0>] cleanup_net+0xf0/0x1a0
>> [<ffffffff81062997>] process_one_work+0x157/0x440
>> [<ffffffff81063303>] worker_thread+0x63/0x520
>> [<ffffffff81068b96>] kthread+0xd6/0xf0
>> [<ffffffff818d412c>] ret_from_fork+0x7c/0xb0
>> [<ffffffffffffffff>] 0xffffffffffffffff
Eric
^ permalink raw reply
* [PATCH net v5 4/4] tg3: Fix tx_pending checks for tg3_tso_bug
From: Benjamin Poirier @ 2014-08-29 20:46 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409345191-8819-1-git-send-email-bpoirier@suse.de>
In tg3_set_ringparam(), the tx_pending test to cover the cases where
tg3_tso_bug() is entered has two problems
1) the check is only done for certain hardware whereas the workaround
is now used more broadly. IOW, the check may not be performed when it
is needed.
2) the check is too optimistic.
For example, with a 5761 (SHORT_DMA_BUG), tg3_set_ringparam() skips over the
"tx_pending <= (MAX_SKB_FRAGS * 3)" check because TSO_BUG is false. Even if it
did do the check, with a full sized skb, frag_cnt_est = 135 but the check is
for <= MAX_SKB_FRAGS * 3 (= 17 * 3 = 51). So the check is insufficient. This
leads to the following situation: by setting, ex. tx_pending = 100, there can
be an skb that triggers tg3_tso_bug() and that is large enough to cause
tg3_tso_bug() to stop the queue even when it is empty. We then end up with a
netdev watchdog transmit timeout.
Given that 1) some of the conditions tested for in tg3_tx_frag_set() apply
regardless of the chipset flags and that 2) it is difficult to estimate ahead
of time the max possible number of frames that a large skb may be split into
by gso, we instead take the approach of adjusting dev->gso_max_segs according
to the requested tx_pending size.
This puts us in the exceptional situation that a single skb that triggers
tg3_tso_bug() may require the entire tx ring. Usually the tx queue is woken up
when at least a quarter of it is available (TG3_TX_WAKEUP_THRESH) but that
would be insufficient now. To avoid useless wakeups, the tx queue wake up
threshold is made dynamic. Likewise, usually the tx queue is stopped as soon
as an skb with max frags may overrun it. Since the skbs submitted from
tg3_tso_bug() use a controlled number of descriptors, the tx queue stop
threshold may be lowered.
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
Changes v1->v2
* in tg3_set_ringparam(), reduce gso_max_segs further to budget 3 descriptors
per gso seg instead of only 1 as in v1
* in tg3_tso_bug(), check that this estimation (3 desc/seg) holds, otherwise
linearize some skbs as needed
* in tg3_start_xmit(), make the queue stop threshold a parameter, for the
reason explained in the commit description
Changes v2->v3
* use tg3_maybe_stop_txq() instead of repeatedly open coding it
* add the requested tp->tx_dropped++ stat increase in tg3_tso_bug() if
skb_linearize() fails and we must abort
* in the same code block, add an additional check to stop the queue with the
default threshold. Otherwise, the netdev_err message at the start of
__tg3_start_xmit() could be triggered when the next frame is transmitted.
That is because the previous calls to __tg3_start_xmit() in tg3_tso_bug()
may have been using a stop_thresh=segs_remaining that is < MAX_SKB_FRAGS +
1.
Changes v3->v4
* in tg3_set_ringparam(), make sure that wakeup_thresh does not end up being
>= tx_pending. Identified by Prashant.
Changes v4->v5
* in tg3_set_ringparam(), use TG3_TX_WAKEUP_THRESH() and tp->txq_cnt instead
of tp->irq_max. Identified by Prashant.
I reproduced this bug using the same approach explained in patch 1.
The bug reproduces with tx_pending <= 135
---
drivers/net/ethernet/broadcom/tg3.c | 69 +++++++++++++++++++++++++++++--------
drivers/net/ethernet/broadcom/tg3.h | 1 +
2 files changed, 56 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index f706a1e..43feb18 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -204,6 +204,10 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS flag, unsigned long *bits)
/* minimum number of free TX descriptors required to wake up TX process */
#define TG3_TX_WAKEUP_THRESH(tnapi) max_t(u32, (tnapi)->tx_pending / 4, \
MAX_SKB_FRAGS + 1)
+/* estimate a certain number of descriptors per gso segment */
+#define TG3_TX_DESC_PER_SEG(seg_nb) ((seg_nb) * 3)
+#define TG3_TX_SEG_PER_DESC(desc_nb) ((desc_nb) / 3)
+
#define TG3_TX_BD_DMA_MAX_2K 2048
#define TG3_TX_BD_DMA_MAX_4K 4096
@@ -6609,10 +6613,10 @@ static void tg3_tx(struct tg3_napi *tnapi)
smp_mb();
if (unlikely(netif_tx_queue_stopped(txq) &&
- (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi)))) {
+ (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh))) {
__netif_tx_lock(txq, smp_processor_id());
if (netif_tx_queue_stopped(txq) &&
- (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi)))
+ (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh))
netif_tx_wake_queue(txq);
__netif_tx_unlock(txq);
}
@@ -7830,6 +7834,8 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi *tnapi,
}
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *, struct net_device *,
+ u32);
/* Returns true if the queue has been stopped. Note that it may have been
* restarted since.
@@ -7844,6 +7850,7 @@ static inline bool tg3_maybe_stop_txq(struct tg3_napi *tnapi,
if (!netif_tx_queue_stopped(txq)) {
stopped = true;
netif_tx_stop_queue(txq);
+ tnapi->wakeup_thresh = wakeup_thresh;
if (wakeup_thresh >= tnapi->tx_pending)
netdev_err(tnapi->tp->dev,
"BUG! wakeup_thresh too large (%u >= %u)\n",
@@ -7851,10 +7858,11 @@ static inline bool tg3_maybe_stop_txq(struct tg3_napi *tnapi,
}
/* netif_tx_stop_queue() must be done before checking tx index
* in tg3_tx_avail(), because in tg3_tx(), we update tx index
- * before checking for netif_tx_queue_stopped().
+ * before checking for netif_tx_queue_stopped(). The memory
+ * barrier also synchronizes wakeup_thresh changes.
*/
smp_mb();
- if (tg3_tx_avail(tnapi) > wakeup_thresh)
+ if (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh)
netif_tx_wake_queue(txq);
}
return stopped;
@@ -7867,10 +7875,10 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
struct netdev_queue *txq, struct sk_buff *skb)
{
struct sk_buff *segs, *nskb;
- u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
+ unsigned int segs_remaining = skb_shinfo(skb)->gso_segs;
+ u32 desc_cnt_est = TG3_TX_DESC_PER_SEG(segs_remaining);
- /* Estimate the number of fragments in the worst case */
- tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
+ tg3_maybe_stop_txq(tnapi, txq, desc_cnt_est, desc_cnt_est);
if (netif_tx_queue_stopped(txq))
return NETDEV_TX_BUSY;
@@ -7880,10 +7888,32 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
goto tg3_tso_bug_end;
do {
+ unsigned int desc_cnt = skb_shinfo(segs)->nr_frags + 1;
+
nskb = segs;
segs = segs->next;
nskb->next = NULL;
- tg3_start_xmit(nskb, tp->dev);
+
+ if (tg3_tx_avail(tnapi) <= segs_remaining - 1 + desc_cnt &&
+ skb_linearize(nskb)) {
+ tp->tx_dropped++;
+ nskb->next = segs;
+ segs = nskb;
+ do {
+ nskb = segs->next;
+
+ dev_kfree_skb_any(segs);
+ segs = nskb;
+ } while (segs);
+ tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ TG3_TX_WAKEUP_THRESH(tnapi));
+ goto tg3_tso_bug_end;
+ }
+ segs_remaining--;
+ if (segs_remaining)
+ __tg3_start_xmit(nskb, tp->dev, segs_remaining);
+ else
+ tg3_start_xmit(nskb, tp->dev);
} while (segs);
tg3_tso_bug_end:
@@ -7895,6 +7925,12 @@ tg3_tso_bug_end:
/* hard_start_xmit for all devices */
static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
{
+ return __tg3_start_xmit(skb, dev, MAX_SKB_FRAGS + 1);
+}
+
+static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
+ struct net_device *dev, u32 stop_thresh)
+{
struct tg3 *tp = netdev_priv(dev);
u32 len, entry, base_flags, mss, vlan = 0;
u32 budget;
@@ -8102,7 +8138,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
tw32_tx_mbox(tnapi->prodmbox, entry);
tnapi->tx_prod = entry;
- tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ tg3_maybe_stop_txq(tnapi, txq, stop_thresh,
TG3_TX_WAKEUP_THRESH(tnapi));
mmiowb();
@@ -12320,13 +12356,12 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
{
struct tg3 *tp = netdev_priv(dev);
int i, irq_sync = 0, err = 0;
+ struct tg3_napi *tnapi = &tp->napi[0];
if ((ering->rx_pending > tp->rx_std_ring_mask) ||
(ering->rx_jumbo_pending > tp->rx_jmb_ring_mask) ||
(ering->tx_pending > TG3_TX_RING_SIZE - 1) ||
- (ering->tx_pending <= MAX_SKB_FRAGS + 1) ||
- (tg3_flag(tp, TSO_BUG) &&
- (ering->tx_pending <= (MAX_SKB_FRAGS * 3))))
+ (ering->tx_pending <= MAX_SKB_FRAGS + 1))
return -EINVAL;
if (netif_running(dev)) {
@@ -12346,8 +12381,13 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
if (tg3_flag(tp, JUMBO_RING_ENABLE))
tp->rx_jumbo_pending = ering->rx_jumbo_pending;
- for (i = 0; i < tp->irq_max; i++)
- tp->napi[i].tx_pending = ering->tx_pending;
+ dev->gso_max_segs = TG3_TX_SEG_PER_DESC(ering->tx_pending - 1);
+ for (i = 0; i < tp->txq_cnt; i++, tnapi++) {
+ tnapi->tx_pending = ering->tx_pending;
+ if (netif_tx_queue_stopped(netdev_get_tx_queue(dev, i)) &&
+ tnapi->wakeup_thresh >= tnapi->tx_pending)
+ tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi);
+ }
if (netif_running(dev)) {
tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
@@ -17822,6 +17862,7 @@ static int tg3_init_one(struct pci_dev *pdev,
else
sndmbx += 0xc;
}
+ dev->gso_max_segs = TG3_TX_SEG_PER_DESC(TG3_DEF_TX_RING_PENDING - 1);
tg3_init_coal(tp);
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index 461acca..6a7e13d 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -3006,6 +3006,7 @@ struct tg3_napi {
u32 tx_pending;
u32 last_tx_cons;
u32 prodmbox;
+ u32 wakeup_thresh;
struct tg3_tx_buffer_desc *tx_ring;
struct tg3_tx_ring_info *tx_buffers;
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v5 3/4] tg3: Move tx queue stop logic to its own function
From: Benjamin Poirier @ 2014-08-29 20:46 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409345191-8819-1-git-send-email-bpoirier@suse.de>
It is duplicated. Also, the first instance in tg3_start_xmit() is racy.
Consider:
tg3_start_xmit()
if budget <= ...
tg3_tx()
(free up the entire ring)
tx_cons =
smp_mb
if queue_stopped and tx_avail, NO
if !queue_stopped
stop queue
return NETDEV_TX_BUSY
... tx queue stopped forever
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
Changes v2->v3
* new patch to avoid repeatedly open coding this block in the next patch.
Changes v3->v4
* added a comment to clarify the return value, as suggested
* replaced the BUG_ON with netdev_err(). No need to be so dramatic, this
situation will trigger a netdev watchdog anyways.
---
drivers/net/ethernet/broadcom/tg3.c | 75 ++++++++++++++++++++-----------------
1 file changed, 40 insertions(+), 35 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 0cecd6d..f706a1e 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7831,6 +7831,35 @@ static int tigon3_dma_hwbug_workaround(struct tg3_napi *tnapi,
static netdev_tx_t tg3_start_xmit(struct sk_buff *, struct net_device *);
+/* Returns true if the queue has been stopped. Note that it may have been
+ * restarted since.
+ */
+static inline bool tg3_maybe_stop_txq(struct tg3_napi *tnapi,
+ struct netdev_queue *txq,
+ u32 stop_thresh, u32 wakeup_thresh)
+{
+ bool stopped = false;
+
+ if (unlikely(tg3_tx_avail(tnapi) <= stop_thresh)) {
+ if (!netif_tx_queue_stopped(txq)) {
+ stopped = true;
+ netif_tx_stop_queue(txq);
+ if (wakeup_thresh >= tnapi->tx_pending)
+ netdev_err(tnapi->tp->dev,
+ "BUG! wakeup_thresh too large (%u >= %u)\n",
+ wakeup_thresh, tnapi->tx_pending);
+ }
+ /* netif_tx_stop_queue() must be done before checking tx index
+ * in tg3_tx_avail(), because in tg3_tx(), we update tx index
+ * before checking for netif_tx_queue_stopped().
+ */
+ smp_mb();
+ if (tg3_tx_avail(tnapi) > wakeup_thresh)
+ netif_tx_wake_queue(txq);
+ }
+ return stopped;
+}
+
/* Use GSO to workaround all TSO packets that meet HW bug conditions
* indicated in tg3_tx_frag_set()
*/
@@ -7841,20 +7870,9 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
/* Estimate the number of fragments in the worst case */
- if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
- netif_tx_stop_queue(txq);
-
- /* netif_tx_stop_queue() must be done before checking
- * checking tx index in tg3_tx_avail() below, because in
- * tg3_tx(), we update tx index before checking for
- * netif_tx_queue_stopped().
- */
- smp_mb();
- if (tg3_tx_avail(tnapi) <= frag_cnt_est)
- return NETDEV_TX_BUSY;
-
- netif_tx_wake_queue(txq);
- }
+ tg3_maybe_stop_txq(tnapi, txq, frag_cnt_est, frag_cnt_est);
+ if (netif_tx_queue_stopped(txq))
+ return NETDEV_TX_BUSY;
segs = skb_gso_segment(skb, tp->dev->features &
~(NETIF_F_TSO | NETIF_F_TSO6));
@@ -7902,16 +7920,13 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
* interrupt. Furthermore, IRQ processing runs lockless so we have
* no IRQ context deadlocks to worry about either. Rejoice!
*/
- if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
- if (!netif_tx_queue_stopped(txq)) {
- netif_tx_stop_queue(txq);
-
- /* This is a hard error, log it. */
- netdev_err(dev,
- "BUG! Tx Ring full when queue awake!\n");
- }
- return NETDEV_TX_BUSY;
+ if (tg3_maybe_stop_txq(tnapi, txq, skb_shinfo(skb)->nr_frags + 1,
+ TG3_TX_WAKEUP_THRESH(tnapi))) {
+ /* This is a hard error, log it. */
+ netdev_err(dev, "BUG! Tx Ring full when queue awake!\n");
}
+ if (netif_tx_queue_stopped(txq))
+ return NETDEV_TX_BUSY;
entry = tnapi->tx_prod;
base_flags = 0;
@@ -8087,18 +8102,8 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
tw32_tx_mbox(tnapi->prodmbox, entry);
tnapi->tx_prod = entry;
- if (unlikely(tg3_tx_avail(tnapi) <= (MAX_SKB_FRAGS + 1))) {
- netif_tx_stop_queue(txq);
-
- /* netif_tx_stop_queue() must be done before checking
- * checking tx index in tg3_tx_avail() below, because in
- * tg3_tx(), we update tx index before checking for
- * netif_tx_queue_stopped().
- */
- smp_mb();
- if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
- netif_tx_wake_queue(txq);
- }
+ tg3_maybe_stop_txq(tnapi, txq, MAX_SKB_FRAGS + 1,
+ TG3_TX_WAKEUP_THRESH(tnapi));
mmiowb();
return NETDEV_TX_OK;
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v5 2/4] tg3: Fix tx_pending check for MAX_SKB_FRAGS
From: Benjamin Poirier @ 2014-08-29 20:46 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1409345191-8819-1-git-send-email-bpoirier@suse.de>
The rest of the driver assumes at least one free descriptor in the tx ring.
Therefore, since an skb with max frags takes up (MAX_SKB_FRAGS + 1)
descriptors, tx_pending must be > (MAX_SKB_FRAGS + 1).
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
Changes v1->v2
Moved ahead in the series from 3/3 to 2/3, no functionnal change
I reproduced this bug using the same approach explained in patch 1.
The bug reproduces with tx_pending = 18
---
drivers/net/ethernet/broadcom/tg3.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index b11c0fd..0cecd6d 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -12319,7 +12319,7 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
if ((ering->rx_pending > tp->rx_std_ring_mask) ||
(ering->rx_jumbo_pending > tp->rx_jmb_ring_mask) ||
(ering->tx_pending > TG3_TX_RING_SIZE - 1) ||
- (ering->tx_pending <= MAX_SKB_FRAGS) ||
+ (ering->tx_pending <= MAX_SKB_FRAGS + 1) ||
(tg3_flag(tp, TSO_BUG) &&
(ering->tx_pending <= (MAX_SKB_FRAGS * 3))))
return -EINVAL;
--
1.8.4.5
^ permalink raw reply related
* [PATCH net v5 1/4] tg3: Limit minimum tx queue wakeup threshold
From: Benjamin Poirier @ 2014-08-29 20:46 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
tx_pending may be set by the user (via ethtool -G) to a low enough value that
TG3_TX_WAKEUP_THRESH becomes smaller than MAX_SKB_FRAGS + 1. This may cause
the tx queue to be waked when there are in fact not enough descriptors to
handle an skb with max frags. This in turn causes tg3_start_xmit() to return
NETDEV_TX_BUSY and print error messages. Fix the problem by putting a limit to
how low TG3_TX_WAKEUP_THRESH can go.
Signed-off-by: Benjamin Poirier <bpoirier@suse.de>
---
I noticed the problem in a 3.0 kernel when setting `ethtool eth0 -G tx 50` and
running a netperf TCP_STREAM test. The console fills up with
[10597.596155] tg3 0000:06:00.0: eth0: BUG! Tx Ring full when queue awake!
The problem in tg3 remains in current kernels though it does not reproduce as
easily since "5640f76 net: use a per task frag allocator (v3.7-rc1)". I
reproduced on current kernels by using the fail_page_alloc fault injection
mechanism to force the creation of skbs with many order-0 frags. Note that the
following script may also trigger another bug (NETDEV WATCHDOG), which is
fixed in the next patch.
$ cat /tmp/doit.sh
F="/sys/kernel/debug/fail_page_alloc"
echo -1 > "$F/times"
echo 0 > "$F/verbose"
echo 0 > "$F/ignore-gfp-wait"
echo 1 > "$F/task-filter"
echo 100 > "$F/probability"
netperf -H 192.168.9.30 -l100 -t omni -- -d send &
n=$!
sleep 0.3
echo 1 > "/proc/$n/make-it-fail"
sleep 10
kill "$n"
---
drivers/net/ethernet/broadcom/tg3.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 3ac5d23..b11c0fd 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -202,7 +202,8 @@ static inline void _tg3_flag_clear(enum TG3_FLAGS flag, unsigned long *bits)
#endif
/* minimum number of free TX descriptors required to wake up TX process */
-#define TG3_TX_WAKEUP_THRESH(tnapi) ((tnapi)->tx_pending / 4)
+#define TG3_TX_WAKEUP_THRESH(tnapi) max_t(u32, (tnapi)->tx_pending / 4, \
+ MAX_SKB_FRAGS + 1)
#define TG3_TX_BD_DMA_MAX_2K 2048
#define TG3_TX_BD_DMA_MAX_4K 4096
--
1.8.4.5
^ permalink raw reply related
* [PATCH net-next 1/2] sunvnet: Avoid sending superfluous LDC messages.
From: Sowmini Varadhan @ 2014-08-29 20:21 UTC (permalink / raw)
To: davem, sowmini.varadhan, raghuram.kothakota; +Cc: netdev
When sending out a burst of packets across multiple descriptors,
it is sufficient to send one LDC "start" trigger for
the first descriptor, so do not send an LDC "start" for every
pass through vnet_start_xmit. Similarly, it is sufficient to send
one "DRING_STOPPED" trigger for the last dring (and if that
fails, hold off and send the trigger later).
Optimizations to the number of LDC messages helps avoid
filling up the LDC channel with superfluous LDC messages
that risk triggering flow-control on the channel,
and also boosts performance.
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Acked-by: Raghuram Kothakota <raghuram.kothakota@oracle.com>
---
drivers/net/ethernet/sun/sunvnet.c | 73 +++++++++++++++++++++++++++++++++-----
drivers/net/ethernet/sun/sunvnet.h | 4 +++
2 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index 23c89ab..fc13b9c 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -37,6 +37,8 @@ MODULE_VERSION(DRV_MODULE_VERSION);
*/
#define VNET_MAX_RETRIES 10
+static int __vnet_tx_trigger(struct vnet_port *port, u32 start);
+
/* Ordered from largest major to lowest */
static struct vio_version vnet_versions[] = {
{ .major = 1, .minor = 0 },
@@ -283,10 +285,18 @@ static int vnet_send_ack(struct vnet_port *port, struct vio_dring_state *dr,
port->raddr[0], port->raddr[1],
port->raddr[2], port->raddr[3],
port->raddr[4], port->raddr[5]);
- err = -ECONNRESET;
+ break;
}
} while (err == -EAGAIN);
+ if (err <= 0 && vio_dring_state == VIO_DRING_STOPPED) {
+ port->stop_rx_idx = end;
+ port->stop_rx = true;
+ } else {
+ port->stop_rx_idx = 0;
+ port->stop_rx = false;
+ }
+
return err;
}
@@ -380,11 +390,14 @@ static int vnet_walk_rx(struct vnet_port *port, struct vio_dring_state *dr,
viodbg(DATA, "vnet_walk_rx start[%08x] end[%08x]\n", start, end);
while (start != end) {
- int ack = 0, err = vnet_walk_rx_one(port, dr, start, &ack);
+ int retries;
+ int ack = 0, err;
+
+ retries = 0;
+again:
+ err = vnet_walk_rx_one(port, dr, start, &ack);
if (err == -ECONNRESET)
return err;
- if (err != 0)
- break;
if (ack_start == -1)
ack_start = start;
ack_end = start;
@@ -448,7 +461,7 @@ static int vnet_ack(struct vnet_port *port, void *msgbuf)
struct net_device *dev;
struct vnet *vp;
u32 end;
-
+ struct vio_net_desc *desc;
if (unlikely(pkt->tag.stype_env != VIO_DRING_DATA))
return 0;
@@ -456,7 +469,24 @@ static int vnet_ack(struct vnet_port *port, void *msgbuf)
if (unlikely(!idx_is_pending(dr, end)))
return 0;
+ /* sync for race conditions with vnet_start_xmit() and tell xmit it
+ * is time to send a trigger.
+ */
dr->cons = next_idx(end, dr);
+ desc = vio_dring_entry(dr, dr->cons);
+ if (desc->hdr.state == VIO_DESC_READY && port->start_cons) {
+ /* vnet_start_xmit() just populated this dring but missed
+ * sending the "start" LDC message to the consumer.
+ * Send a "start" trigger on its behalf.
+ */
+ if (__vnet_tx_trigger(port, dr->cons) > 0)
+ port->start_cons = false;
+ else
+ port->start_cons = true;
+ } else {
+ port->start_cons = true;
+ }
+
vp = port->vp;
dev = vp->dev;
@@ -597,7 +627,7 @@ static void vnet_event(void *arg, int event)
local_irq_restore(flags);
}
-static int __vnet_tx_trigger(struct vnet_port *port)
+static int __vnet_tx_trigger(struct vnet_port *port, u32 start)
{
struct vio_dring_state *dr = &port->vio.drings[VIO_DRIVER_TX_RING];
struct vio_dring_data hdr = {
@@ -608,12 +638,21 @@ static int __vnet_tx_trigger(struct vnet_port *port)
.sid = vio_send_sid(&port->vio),
},
.dring_ident = dr->ident,
- .start_idx = dr->prod,
+ .start_idx = start,
.end_idx = (u32) -1,
};
int err, delay;
int retries = 0;
+ if (port->stop_rx) {
+ err = vnet_send_ack(port,
+ &port->vio.drings[VIO_DRIVER_RX_RING],
+ port->stop_rx_idx, -1,
+ VIO_DRING_STOPPED);
+ if (err <= 0)
+ return err;
+ }
+
hdr.seq = dr->snd_nxt;
delay = 1;
do {
@@ -734,7 +773,21 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
d->hdr.state = VIO_DESC_READY;
- err = __vnet_tx_trigger(port);
+ /* Exactly one ldc "start" trigger (for dr->cons) needs to be sent
+ * to notify the consumer that some descriptors are READY.
+ * After that "start" trigger, no additional triggers are needed until
+ * a DRING_STOPPED is received from the consumer. The dr->cons field
+ * (set up by vnet_ack()) has the value of the next dring index
+ * that has not yet been ack-ed. We send a "start" trigger here
+ * if, and only if, start_cons is true (reset it afterward). Conversely,
+ * vnet_ack() should check if the dring corresponding to cons
+ * is marked READY, but start_cons was false.
+ * If so, vnet_ack() should send out the missed "start" trigger.
+ */
+ if (!port->start_cons)
+ goto ldc_start_done; /* previous trigger suffices */
+
+ err = __vnet_tx_trigger(port, dr->cons);
if (unlikely(err < 0)) {
netdev_info(dev, "TX trigger error %d\n", err);
d->hdr.state = VIO_DESC_FREE;
@@ -742,6 +795,9 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
goto out_dropped_unlock;
}
+ldc_start_done:
+ port->start_cons = false;
+
dev->stats.tx_packets++;
dev->stats.tx_bytes += skb->len;
@@ -1035,6 +1091,7 @@ static int vnet_port_alloc_tx_bufs(struct vnet_port *port)
(sizeof(struct ldc_trans_cookie) * 2));
dr->num_entries = VNET_TX_RING_SIZE;
dr->prod = dr->cons = 0;
+ port->start_cons = true; /* need an initial trigger */
dr->pending = VNET_TX_RING_SIZE;
dr->ncookies = ncookies;
diff --git a/drivers/net/ethernet/sun/sunvnet.h b/drivers/net/ethernet/sun/sunvnet.h
index de5c2c6..da49337 100644
--- a/drivers/net/ethernet/sun/sunvnet.h
+++ b/drivers/net/ethernet/sun/sunvnet.h
@@ -40,6 +40,10 @@ struct vnet_port {
struct vnet_tx_entry tx_bufs[VNET_TX_RING_SIZE];
struct list_head list;
+
+ u32 stop_rx_idx;
+ bool stop_rx;
+ bool start_cons;
};
static inline struct vnet_port *to_vnet_port(struct vio_driver_state *vio)
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next 2/2] sunvnet: Re-check for a VIO_DESC_READY data descriptor after short udelay()
From: Sowmini Varadhan @ 2014-08-29 20:18 UTC (permalink / raw)
To: davem, sowmini.varadhan, raghuram.kothakota; +Cc: netdev
Upon encountering the first !VIO_DESC_READY in vnet_walk_rx(),
it is frequently worthwhile to re-check the descriptor status
after a short microsecond delay, as a bursty sender could
be actively populating the descriptors, and the short udelay()
is far less expensive than rolling back to ldc_rx() and having
to wake up and read data on another LDC message.
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
Acked-by: Raghuram Kothakota <raghuram.kothakota@oracle.com>
---
drivers/net/ethernet/sun/sunvnet.c | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index fc13b9c..7b1f320 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -398,6 +398,20 @@ again:
err = vnet_walk_rx_one(port, dr, start, &ack);
if (err == -ECONNRESET)
return err;
+ if (err != 0) {
+ /* The descriptor was not READY. Retry with a
+ * small delay, in case we have a bursty sender
+ * that is actively populating the descriptors, to
+ * reduce the overhead of stopping and re-entering
+ * which would involve expensive LDC messages.
+ */
+ if (retries++ < 3) {
+ udelay(4);
+ goto again;
+ } else {
+ break;
+ }
+ }
if (ack_start == -1)
ack_start = start;
ack_end = start;
--
1.8.4.2
^ permalink raw reply related
* [PATCH net-next 0/2] sunvnet: Reduce LDC message overhead.
From: Sowmini Varadhan @ 2014-08-29 20:16 UTC (permalink / raw)
To: davem, sowmini.varadhan, raghuram.kothakota; +Cc: netdev
This patch series has 2 sets of changes to reduce the overhead
from LDC messages.
- the current linux sunvnet driver sends out many more LDC start and stop
triggers than it needs to: when we are sending a burst of packets across
multiple descriptor rings, we only need to send the "start" trigger for
the first dring (we currently send one for every dring). And we only need
to send the "STOPPED" trigger for the last dring (and if that fails, hold
off and send the trigger later).
- When reading the descriptor rings, don't give up as soon as a dring
that is not VIO_DESC_READY is found- especially when the peer is sending
a burst of packets, it is worthwhile to recheck the descriptor status
after a small microsecond delay, as the likelihood of finding this
descriptor READY is high, and it is cheaper to just retry with a delay
than to return from the current context, read another LDC message,
and then come back to read this descriptor.
Sowmini Varadhan (2):
Avoid sending superfluous LDC messages.
Re-check for a VIO_DESC_READY data descriptor after short udelay()
drivers/net/ethernet/sun/sunvnet.c | 87 ++++++++++++++++++++++++++++++++++----
drivers/net/ethernet/sun/sunvnet.h | 4 ++
2 files changed, 83 insertions(+), 8 deletions(-)
--
1.8.4.2
^ 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