* Re: [PATCH v1 net] packet: handle too big packets for PACKET_V3
From: David Miller @ 2014-08-21 23:45 UTC (permalink / raw)
To: eric.dumazet; +Cc: hannes, dborkman, nhorman, brouer, netdev, guy
In-Reply-To: <1408119364.6804.89.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Fri, 15 Aug 2014 09:16:04 -0700
> From: Eric Dumazet <edumazet@google.com>
>
> af_packet can currently overwrite kernel memory by out of bound
> accesses, because it assumed a [new] block can always hold one frame.
>
> This is not generally the case, even if most existing tools do it right.
>
> This patch clamps too long frames as API permits, and issue a one time
> error on syslog.
>
> [ 394.357639] tpacket_rcv: packet too big, clamped from 5042 to 3966. macoff=82
>
> In this example, packet header tp_snaplen was set to 3966,
> and tp_len was set to 5042 (skb->len)
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Fixes: f6fb8f100b80 ("af-packet: TPACKET_V3 flexible buffer implementation.")
Since both the skb->len and the snaplen are provided to the user in
the ring entry descriptor, it is correct to fix this problem by simply
truncating.
Applied and queued up for -stable, thanks a lot Eric.
^ permalink raw reply
* Re: [PATCH 2/2] MAINTAINERS: add entry for ec_bhf driver
From: David Miller @ 2014-08-21 23:39 UTC (permalink / raw)
To: reksio; +Cc: netdev, linux-kernel
In-Reply-To: <20140815155046.GA16712@newterm.pl>
From: Dariusz Marcinkiewicz <reksio@newterm.pl>
Date: Fri, 15 Aug 2014 17:50:47 +0200
> Added entry for ec_bhf driver.
>
> Signed-off-by: Dariusz Marcinkiewicz <reksio@newterm.pl>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH 1/2] net: ec_bhf: remove excessive debug messages
From: David Miller @ 2014-08-21 23:39 UTC (permalink / raw)
To: reksio; +Cc: netdev, linux-kernel
In-Reply-To: <20140815154941.GA16632@newterm.pl>
From: Dariusz Marcinkiewicz <reksio@newterm.pl>
Date: Fri, 15 Aug 2014 17:49:41 +0200
> This cuts down on the number of debug information spit out by
> the driver. Some of the potentially useful debug info gets exposed
> by debugfs.
>
> Signed-off-by: Dariusz Marcinkiewicz <reksio@newterm.pl>
I think you should just flat out remove a lot of this stuff:
> +static struct debugfs_reg32 ec_bhf_debugfs_mii_regs[] = {
> + {
> + .name = "link-status",
> + .offset = MII_LINK_STATUS
> + }
> +};
This is completely unnecessary, if you want to export MII register
values to the user, we have a mechanism for that, via the SIOCGMII*
ioctls.
> +static struct debugfs_reg32 ec_bhf_debugfs_fifo_regs[] = {
> + {
> + .name = "fifo-tx",
> + .offset = FIFO_TX_REG
> + },
> + {
> + .name = "fifo-rx",
> + .offset = FIFO_RX_REG
> + }
> +};
You can export chip register values via the ethtool register dump API.
Simply implement ethtool_ops->get_regs_len and ethtool_ops->get_regs
and off you go.
^ permalink raw reply
* Re: [PATCH v2 0/3] net: Add Keystone NetCP ethernet driver support
From: David Miller @ 2014-08-21 23:36 UTC (permalink / raw)
To: santosh.shilimkar
Cc: netdev, linux-arm-kernel, linux-kernel, robh+dt, grant.likely,
devicetree, sandeep_n
In-Reply-To: <1408115562-22487-1-git-send-email-santosh.shilimkar@ti.com>
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Fri, 15 Aug 2014 11:12:39 -0400
> Update version after incorporating David Miller's comment from earlier
> posting [1]. I would like to get these merged for upcoming 3.18 merge
> window if there are no concerns on this version.
>
> The network coprocessor (NetCP) is a hardware accelerator that processes
> Ethernet packets. NetCP has a gigabit Ethernet (GbE) subsystem with a ethernet
> switch sub-module to send and receive packets. NetCP also includes a packet
> accelerator (PA) module to perform packet classification operations such as
> header matching, and packet modification operations such as checksum
> generation. NetCP can also optionally include a Security Accelerator(SA)
> capable of performing IPSec operations on ingress/egress packets.
>
> Keystone SoC's also have a 10 Gigabit Ethernet Subsystem (XGbE) which
> includes a 3-port Ethernet switch sub-module capable of 10Gb/s and
> 1Gb/s rates per Ethernet port.
>
> NetCP driver has a plug-in module architecture where each of the NetCP
> sub-modules exist as a loadable kernel module which plug in to the netcp
> core. These sub-modules are represented as "netcp-devices" in the dts
> bindings. It is mandatory to have the ethernet switch sub-module for
> the ethernet interface to be operational. Any other sub-module like the
> PA is optional.
>
> Both GBE and XGBE network processors supported using common driver. It
> is also designed to handle future variants of NetCP.
I don't want to see an offload driver that doesn't plug into the existing
generic frameworks for configuration et al.
If no existing facility exists to support what you need, you must work
with the upstream maintainers to design and create one.
It is absolutely no reasonable for every "switch on a chip" driver to
export it's own configuration knob, we need a standard interface all
such drivers will plug into and provide.
^ permalink raw reply
* Re: [PATCH net] net: fix checksum features handling in netif_skb_features()
From: David Miller @ 2014-08-21 23:33 UTC (permalink / raw)
To: mkubecek; +Cc: netdev, linux-kernel
In-Reply-To: <20140815095617.3006CA0BED@unicorn.suse.cz>
From: Michal Kubecek <mkubecek@suse.cz>
Date: Fri, 15 Aug 2014 11:56:17 +0200 (CEST)
> @@ -2587,13 +2587,18 @@ netdev_features_t netif_skb_features(struct sk_buff *skb)
> return harmonize_features(skb, features);
> }
>
> - features &= (skb->dev->vlan_features | NETIF_F_HW_VLAN_CTAG_TX |
> - NETIF_F_HW_VLAN_STAG_TX);
> + features = netdev_intersect_features(features, skb->dev->vlan_features |
> + NETIF_F_HW_VLAN_CTAG_TX |
> + NETIF_F_HW_VLAN_STAG_TX);
This is not indented properly.
The second and subsequent lines of the function call should start precisely
at the first column after the openning parenthesis on the first line. You
should use the appropriate number of TAB and SPACE characters necessary
to do so.
Thanks.
^ permalink raw reply
* Re: [PATCH net-next] lec: Use rtnl lock/unlock when updating MTU
From: David Miller @ 2014-08-21 23:31 UTC (permalink / raw)
To: chas; +Cc: netdev, linux-atm-general
In-Reply-To: <201408150056.s7F0uRg7003712@thirdoffive.cmf.nrl.navy.mil>
From: "Chas Williams (CONTRACTOR)" <chas@cmf.nrl.navy.mil>
Date: Thu, 14 Aug 2014 20:56:27 -0400
> In message <20140814.143706.1833450188258591738.davem@redhat.com>,David Miller writes:
>>From: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
>>Date: Thu, 14 Aug 2014 09:19:47 -0400
>>
>>> The LECS response contains the MTU that should be used. Correctly
>>> synchronize with other layers when updating.
>>>
>>> Signed-off-by: Chas Williams - CONTRACTOR <chas@cmf.nrl.navy.mil>
>>
>>I don't think you can sleep from this function, which rtnl_lock() may
>>require. Look elsewhere in this routine, it's doing GFP_ATOMIC
>>allocations even.
>
> Its been a while but...
>
> This is the send routine for a virtual atm device that is the control
> interface bewteen the user space client and the kernel. The user space
> client creates an atm socket and uses an ioctl to connect that atm socket
> to the this virtual device. So the call path is something like:
>
> sendmsg() -> vcc_sendmsg() -> virtual atm device.send()
>
> Generally speaking, you can't sleep in the send routine of an atm device
> since there are other potential users besides sockets.
>
> The GFP_ATOMIC usage is probably a lack of understanding. The other
> IRQ level locks are necessary for coordination.
Ok, that makes sense, thanks for explaining the context in which this is
invoked.
Patch applied, thanks again.
^ permalink raw reply
* Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold
From: Michael Chan @ 2014-08-21 23:26 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: Prashant Sreedharan, netdev, linux-kernel
In-Reply-To: <20140821230607.GB7724@f1.synalogic.ca>
On Thu, 2014-08-21 at 16:06 -0700, Benjamin Poirier wrote:
> On 2014/08/21 15:32, Michael Chan wrote:
> > On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote:
> > > On 2014/08/19 15:00, Michael Chan wrote:
> > > > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote:
> > > > > 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)
> > > >
> > > > I think we should precompute this and store it in something like
> > > > tp->tx_wake_thresh.
> > >
> > > I've tried this by adding the following patch at the end of the v2
> > > series but I did not measure a significant latency improvement. Was
> > > there another reason for the change?
> >
> > Just performance. The wake up threshold is checked in the tx fast path
> > in both start_xmit() and tg3_tx(). I would optimize such code for speed
>
> I don't see what you mean. The code in those two functions that used to
> invoke TG3_TX_WAKEUP_THRESH is wrapped in unlikely() conditions. You
> can't tell me that's the fast path ;) It's only checked when the queue
> is stopped.
I missed the unlikely(). So you're right. It's not really in the fast
path.
>
> Moreover, the patches I've sent already add tg3_napi.wakeup_thresh. It
> is over those patches that I've made the measurements.
Right. But my original comment was over your original patch #1 which
was adding max_t() to the macro TG3_TX_WAKE_THRESH without adding
wakeup_thresh field. All my comments (performance and smaller code)
were based on your original patch #1. Later I did see that your patch 3
converted TG3_TX_WAKEUP_THRESH to a structure field so it's no longer an
issue.
>
> > as much as possible. In the current code, it was just a right shift
> > operation. Now, with max_t() added, I think I prefer having it
> > pre-computed. The performance difference may not be measurable, but I
> > think the compiled code size may be smaller too.
>
> Maybe in certain areas, but not overall:
>
> with v2 patches 1-3
> text data bss dec hex filename
> 149495 1247 0 150742 24cd6 drivers/net/ethernet/broadcom/tg3.o
> with v2 patches 1-3 + tx_wake_thresh_def
> text data bss dec hex filename
> 149524 1247 0 150771 24cf3 drivers/net/ethernet/broadcom/tg3.o
>
> I really don't see a gain.
>
Agreed. Once you have converted the TG3_TX_WAKEUP_THRESH to a structure
field, that's sufficient. No need to have multiple fields. Thanks.
^ permalink raw reply
* Re: Performance regression on kernels 3.10 and newer
From: David Miller @ 2014-08-21 23:24 UTC (permalink / raw)
To: therbert; +Cc: alexander.h.duyck, eric.dumazet, netdev
In-Reply-To: <CA+mtBx-c0HCAES9PCyGn7cChZsX0KaLgdziFL_e1iRmkdG0dsQ@mail.gmail.com>
From: Tom Herbert <therbert@google.com>
Date: Thu, 14 Aug 2014 16:25:53 -0700
> I don't know if it's the same problem, but I did post a patch back in
> January that would resolve false sharing of dst->__refcnt and rt_genid
> in the same cacheline. We could revisit that if it helps.
I think that regardless of what happens in the discussion here, you
should repost that patch.
^ permalink raw reply
* Re: [PATCH net-next v3 2/3] ethtool: Add generic options for tunables
From: David Miller @ 2014-08-21 23:18 UTC (permalink / raw)
To: _govind; +Cc: netdev, ben, stephen, ssujith, benve
In-Reply-To: <1408008560-1067-3-git-send-email-_govind@gmx.com>
From: Govindarajulu Varadarajan <_govind@gmx.com>
Date: Thu, 14 Aug 2014 14:59:19 +0530
> @@ -1621,6 +1621,50 @@ static int ethtool_get_module_eeprom(struct net_device *dev,
> modinfo.eeprom_len);
> }
>
> +static int ethtool_get_tunable(struct net_device *dev, void __user *useraddr)
> +{
> + int ret;
> + struct ethtool_tunable tuna;
> + const struct ethtool_tunable_ops *ops;
> +
> + if (copy_from_user(&tuna, useraddr, sizeof(tuna)))
> + return -EFAULT;
> + if (tuna.tcmd >= ETHTOOL_TUNABLE_MAX)
> + return -EOPNOTSUPP;
> + ops = &dev->ethtool_ops->tunable_ops[tuna.tcmd];
> + if (!ops->get)
> + return -EOPNOTSUPP;
> + ret = ops->get(dev, &tuna);
> + if (ret)
> + return ret;
> + if (copy_to_user(useraddr, &tuna, sizeof(tuna)))
> + return -EFAULT;
> +
> + return 0;
> +}
You should be validating tuna.len here, it should not be happening in the drivers
as I see you are doing in your enic implementation.
Also, having a seperate OP for each tunable might be overkill.
Consider instead one "get_tunable" and one "set_tunable" callback,
which contains a switch statement over 'tcmd'. The generic
ethtool_{g,s}et_tunable() would still validate the length, so that the
driver method does not need to do so.
^ permalink raw reply
* Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold
From: Benjamin Poirier @ 2014-08-21 23:06 UTC (permalink / raw)
To: Michael Chan; +Cc: Prashant Sreedharan, netdev, linux-kernel
In-Reply-To: <1408660355.7644.50.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
On 2014/08/21 15:32, Michael Chan wrote:
> On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote:
> > On 2014/08/19 15:00, Michael Chan wrote:
> > > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote:
> > > > 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)
> > >
> > > I think we should precompute this and store it in something like
> > > tp->tx_wake_thresh.
> >
> > I've tried this by adding the following patch at the end of the v2
> > series but I did not measure a significant latency improvement. Was
> > there another reason for the change?
>
> Just performance. The wake up threshold is checked in the tx fast path
> in both start_xmit() and tg3_tx(). I would optimize such code for speed
I don't see what you mean. The code in those two functions that used to
invoke TG3_TX_WAKEUP_THRESH is wrapped in unlikely() conditions. You
can't tell me that's the fast path ;) It's only checked when the queue
is stopped.
Moreover, the patches I've sent already add tg3_napi.wakeup_thresh. It
is over those patches that I've made the measurements.
> as much as possible. In the current code, it was just a right shift
> operation. Now, with max_t() added, I think I prefer having it
> pre-computed. The performance difference may not be measurable, but I
> think the compiled code size may be smaller too.
Maybe in certain areas, but not overall:
with v2 patches 1-3
text data bss dec hex filename
149495 1247 0 150742 24cd6 drivers/net/ethernet/broadcom/tg3.o
with v2 patches 1-3 + tx_wake_thresh_def
text data bss dec hex filename
149524 1247 0 150771 24cf3 drivers/net/ethernet/broadcom/tg3.o
I really don't see a gain.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
^ permalink raw reply
* Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold
From: Michael Chan @ 2014-08-21 22:32 UTC (permalink / raw)
To: Benjamin Poirier; +Cc: Prashant Sreedharan, netdev, linux-kernel
In-Reply-To: <20140821220424.GB7117@f1.synalogic.ca>
On Thu, 2014-08-21 at 15:04 -0700, Benjamin Poirier wrote:
> On 2014/08/19 15:00, Michael Chan wrote:
> > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote:
> > > 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)
> >
> > I think we should precompute this and store it in something like
> > tp->tx_wake_thresh.
>
> I've tried this by adding the following patch at the end of the v2
> series but I did not measure a significant latency improvement. Was
> there another reason for the change?
Just performance. The wake up threshold is checked in the tx fast path
in both start_xmit() and tg3_tx(). I would optimize such code for speed
as much as possible. In the current code, it was just a right shift
operation. Now, with max_t() added, I think I prefer having it
pre-computed. The performance difference may not be measurable, but I
think the compiled code size may be smaller too.
^ permalink raw reply
* Re: [PATCH net-next v3 3/3] enic: Add tunable_ops support for rx_copybreak
From: Michał Mirosław @ 2014-08-21 22:12 UTC (permalink / raw)
To: Govindarajulu Varadarajan
Cc: David Miller, netdev, ben, stephen, ssujith, benve
In-Reply-To: <1408008560-1067-4-git-send-email-_govind@gmx.com>
2014-08-14 11:29 GMT+02:00 Govindarajulu Varadarajan <_govind@gmx.com>:
> This patch adds support for setting/getting rx_copybreak using
> tunable_ops.
>
> Defines enic_get_rx_copybreak() & enic_set_rx_copybreak() tunable_ops.
>
> Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
> ---
> drivers/net/ethernet/cisco/enic/enic_ethtool.c | 30 ++++++++++++++++++++++++++
> 1 file changed, 30 insertions(+)
>
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index 523c9ce..9bf53fc 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -379,6 +379,30 @@ static int enic_get_rxnfc(struct net_device *dev, struct ethtool_rxnfc *cmd,
> return ret;
> }
>
> +static int enic_get_rx_copybreak(struct net_device *dev,
> + struct ethtool_tunable *tuna)
> +{
> + struct enic *enic = netdev_priv(dev);
> +
> + if (tuna->len < sizeof(tuna->data.rx_copybreak))
> + return -ENOSPC;
> + tuna->data.rx_copybreak = enic->rx_copybreak;
> +
> + return 0;
> +}
> +
> +static int enic_set_rx_copybreak(struct net_device *dev,
> + struct ethtool_tunable *tuna)
> +{
> + struct enic *enic = netdev_priv(dev);
> +
> + if (tuna->len != sizeof(tuna->data.rx_copybreak))
> + return -EINVAL;
> + enic->rx_copybreak = tuna->data.rx_copybreak;
> +
> + return 0;
> +}
> +
> static const struct ethtool_ops enic_ethtool_ops = {
> .get_settings = enic_get_settings,
> .get_drvinfo = enic_get_drvinfo,
> @@ -391,6 +415,12 @@ static const struct ethtool_ops enic_ethtool_ops = {
> .get_coalesce = enic_get_coalesce,
> .set_coalesce = enic_set_coalesce,
> .get_rxnfc = enic_get_rxnfc,
> + .tunable_ops = {
> + [ETHTOOL_RX_COPYBREAK] = {
> + .set = enic_set_rx_copybreak,
> + .get = enic_get_rx_copybreak,
> + },
> + },
> };
>
> void enic_set_ethtool_ops(struct net_device *netdev)
This is a lot of boilerplate code to write for such a simple variable.
BTW, there are a lot of drivers that use rx_copybreak idea. Some time
ago, I tried to unify rx packet handling, but got into too many leafs
of the theme.You could google up 'rx_copybreak handling' if you'd like
to revive the idea.
Best Regards,
Michał Mirosław
^ permalink raw reply
* Re: [PATCH v2] net: ipv6: fib: don't sleep inside atomic lock
From: Hannes Frederic Sowa @ 2014-08-21 22:12 UTC (permalink / raw)
To: Benjamin Block
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel
In-Reply-To: <1408642668-5808-1-git-send-email-bebl@mageta.org>
On Do, 2014-08-21 at 19:37 +0200, Benjamin Block wrote:
> The function fib6_commit_metrics() allocates a piece of memory in mode
> GFP_KERNEL while holding an atomic lock from higher up in the stack, in
> the function __ip6_ins_rt(). This produces the following BUG:
>
> > BUG: sleeping function called from invalid context at mm/slub.c:1250
> > in_atomic(): 1, irqs_disabled(): 0, pid: 2909, name: dhcpcd
> > 2 locks held by dhcpcd/2909:
> > #0: (rtnl_mutex){+.+.+.}, at: [<ffffffff81978e67>] rtnl_lock+0x17/0x20
> > #1: (&tb->tb6_lock){++--+.}, at: [<ffffffff81a6951a>] ip6_route_add+0x65a/0x800
> > CPU: 1 PID: 2909 Comm: dhcpcd Not tainted 3.17.0-rc1 #1
> > Hardware name: ASUS All Series/Q87T, BIOS 0216 10/16/2013
> > 0000000000000008 ffff8800c8f13858 ffffffff81af135a 0000000000000000
> > ffff880212202430 ffff8800c8f13878 ffffffff810f8d3a ffff880212202c98
> > 0000000000000010 ffff8800c8f138c8 ffffffff8121ad0e 0000000000000001
> > Call Trace:
> > [<ffffffff81af135a>] dump_stack+0x4e/0x68
> > [<ffffffff810f8d3a>] __might_sleep+0x10a/0x120
> > [<ffffffff8121ad0e>] kmem_cache_alloc_trace+0x4e/0x190
> > [<ffffffff81a6bcd6>] ? fib6_commit_metrics+0x66/0x110
> > [<ffffffff81a6bcd6>] fib6_commit_metrics+0x66/0x110
> > [<ffffffff81a6cbf3>] fib6_add+0x883/0xa80
> > [<ffffffff81a6951a>] ? ip6_route_add+0x65a/0x800
> > [<ffffffff81a69535>] ip6_route_add+0x675/0x800
> > [<ffffffff81a68f2a>] ? ip6_route_add+0x6a/0x800
> > [<ffffffff81a6990c>] inet6_rtm_newroute+0x5c/0x80
> > [<ffffffff8197cf01>] rtnetlink_rcv_msg+0x211/0x260
> > [<ffffffff81978e67>] ? rtnl_lock+0x17/0x20
> > [<ffffffff81119708>] ? lock_release_holdtime+0x28/0x180
> > [<ffffffff81978e67>] ? rtnl_lock+0x17/0x20
> > [<ffffffff8197ccf0>] ? __rtnl_unlock+0x20/0x20
> > [<ffffffff819a989e>] netlink_rcv_skb+0x6e/0xd0
> > [<ffffffff81978ee5>] rtnetlink_rcv+0x25/0x40
> > [<ffffffff819a8e59>] netlink_unicast+0xd9/0x180
> > [<ffffffff819a9600>] netlink_sendmsg+0x700/0x770
> > [<ffffffff81103735>] ? local_clock+0x25/0x30
> > [<ffffffff8194e83c>] sock_sendmsg+0x6c/0x90
> > [<ffffffff811f98e3>] ? might_fault+0xa3/0xb0
> > [<ffffffff8195ca6d>] ? verify_iovec+0x7d/0xf0
> > [<ffffffff8194ec3e>] ___sys_sendmsg+0x37e/0x3b0
> > [<ffffffff8111ef15>] ? trace_hardirqs_on_caller+0x185/0x220
> > [<ffffffff81af979e>] ? mutex_unlock+0xe/0x10
> > [<ffffffff819a55ec>] ? netlink_insert+0xbc/0xe0
> > [<ffffffff819a65e5>] ? netlink_autobind.isra.30+0x125/0x150
> > [<ffffffff819a6520>] ? netlink_autobind.isra.30+0x60/0x150
> > [<ffffffff819a84f9>] ? netlink_bind+0x159/0x230
> > [<ffffffff811f989a>] ? might_fault+0x5a/0xb0
> > [<ffffffff8194f25e>] ? SYSC_bind+0x7e/0xd0
> > [<ffffffff8194f8cd>] __sys_sendmsg+0x4d/0x80
> > [<ffffffff8194f912>] SyS_sendmsg+0x12/0x20
> > [<ffffffff81afc692>] system_call_fastpath+0x16/0x1b
>
> Fixing this by replacing the mode GFP_KERNEL with GFP_ATOMIC.
>
> Signed-off-by: Benjamin Block <bebl@mageta.org>
Acked-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Thanks,
Hannes
^ permalink raw reply
* Re: [PATCH 1/3] tg3: Limit minimum tx queue wakeup threshold
From: Benjamin Poirier @ 2014-08-21 22:04 UTC (permalink / raw)
To: Michael Chan; +Cc: Prashant Sreedharan, netdev, linux-kernel
In-Reply-To: <1408485647.7404.3.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
On 2014/08/19 15:00, Michael Chan wrote:
> On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote:
> > 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)
>
> I think we should precompute this and store it in something like
> tp->tx_wake_thresh.
I've tried this by adding the following patch at the end of the v2
series but I did not measure a significant latency improvement. Was
there another reason for the change?
Here are the performance results. The first set of numbers are the same
as those found in patch v2 3/3.
# perf stat -r10 -ad netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d rr
* with patches 1-3
rr values: 6732.65 6920.1 6909.46 7032.41 6864.43 6897.6 6815.19 6967.83 6849.23 6929.52
sample size: 10
mean: 6891.842
standard deviation: 82.91901
quantiles: 6732.65 6853.03 6903.53 6927.165 7032.41
6890±80
Performance counter stats for 'netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d rr' (10 runs):
480675.949728 task-clock # 8.001 CPUs utilized ( +- 0.01% ) [100.00%]
850,461 context-switches # 0.002 M/sec ( +- 0.37% ) [100.00%]
564 CPU-migrations # 0.000 M/sec ( +- 5.67% ) [100.00%]
417 page-faults # 0.000 M/sec ( +- 76.04% )
287,019,442,295 cycles # 0.597 GHz ( +- 7.16% ) [15.01%]
828,198,830,689 stalled-cycles-frontend # 288.55% frontend cycles idle ( +- 3.01% ) [25.01%]
718,230,307,166 stalled-cycles-backend # 250.24% backend cycles idle ( +- 3.53% ) [35.00%]
117,976,598,188 instructions # 0.41 insns per cycle
# 7.02 stalled cycles per insn ( +- 4.06% ) [45.00%]
26,715,853,108 branches # 55.580 M/sec ( +- 3.77% ) [50.00%]
198,787,673 branch-misses # 0.74% of all branches ( +- 0.86% ) [50.00%]
28,416,922,166 L1-dcache-loads # 59.119 M/sec ( +- 3.54% ) [50.00%]
367,613,007 L1-dcache-load-misses # 1.29% of all L1-dcache hits ( +- 0.47% ) [50.00%]
75,260,575 LLC-loads # 0.157 M/sec ( +- 2.24% ) [40.00%]
5,777 LLC-load-misses # 0.01% of all LL-cache hits ( +- 36.03% ) [ 5.00%]
60.077898757 seconds time elapsed ( +- 0.01% )
* with patches 1-3 + tx_wake_thresh_def
rr values: 6636.87 6874.05 6916.29 6961.68 6941.3 6841.44 6829.05 6806.55 6846.04 6958.39
sample size: 10
mean: 6861.166
standard deviation: 96.67967
quantiles: 6636.87 6832.148 6860.045 6935.048 6961.68
6900±100
Performance counter stats for 'netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d rr' (10 runs):
480688.653656 task-clock # 8.001 CPUs utilized ( +- 0.01% ) [100.00%]
846,980 context-switches # 0.002 M/sec ( +- 0.40% ) [100.00%]
524 CPU-migrations # 0.000 M/sec ( +- 11.82% ) [100.00%]
420 page-faults # 0.000 M/sec ( +- 75.31% )
275,602,421,981 cycles # 0.573 GHz ( +- 3.23% ) [15.01%]
806,335,406,844 stalled-cycles-frontend # 292.57% frontend cycles idle ( +- 2.16% ) [25.01%]
640,757,376,054 stalled-cycles-backend # 232.49% backend cycles idle ( +- 2.46% ) [35.00%]
113,241,018,220 instructions # 0.41 insns per cycle
# 7.12 stalled cycles per insn ( +- 1.93% ) [45.00%]
25,479,064,973 branches # 53.005 M/sec ( +- 1.96% ) [50.00%]
205,483,191 branch-misses # 0.81% of all branches ( +- 0.75% ) [50.00%]
27,209,883,125 L1-dcache-loads # 56.606 M/sec ( +- 1.87% ) [50.00%]
361,721,478 L1-dcache-load-misses # 1.33% of all L1-dcache hits ( +- 0.51% ) [50.00%]
80,669,260 LLC-loads # 0.168 M/sec ( +- 1.01% ) [40.00%]
8,846 LLC-load-misses # 0.01% of all LL-cache hits ( +- 34.01% ) [ 5.00%]
60.079761525 seconds time elapsed ( +- 0.01% )
---
drivers/net/ethernet/broadcom/tg3.c | 27 ++++++++++++++++-----------
drivers/net/ethernet/broadcom/tg3.h | 5 +++--
2 files changed, 19 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index c29f2e3..81e390b 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -6478,10 +6478,11 @@ static void tg3_dump_state(struct tg3 *tp)
tnapi->hw_status->idx[0].tx_consumer);
netdev_err(tp->dev,
- "%d: NAPI info [%08x:%08x:(%04x:%04x:%04x):%04x:(%04x:%04x:%04x:%04x)]\n",
+ "%d: NAPI info [%08x:%08x:(%04x:%04x:%04x:%04x):%04x:(%04x:%04x:%04x:%04x)]\n",
i,
tnapi->last_tag, tnapi->last_irq_tag,
tnapi->tx_prod, tnapi->tx_cons, tnapi->tx_pending,
+ tnapi->tx_wake_thresh_cur,
tnapi->rx_rcb_ptr,
tnapi->prodring.rx_std_prod_idx,
tnapi->prodring.rx_std_cons_idx,
@@ -6613,10 +6614,10 @@ static void tg3_tx(struct tg3_napi *tnapi)
smp_mb();
if (unlikely(netif_tx_queue_stopped(txq) &&
- (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh))) {
+ (tg3_tx_avail(tnapi) > tnapi->tx_wake_thresh_cur))) {
__netif_tx_lock(txq, smp_processor_id());
if (netif_tx_queue_stopped(txq) &&
- (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh))
+ (tg3_tx_avail(tnapi) > tnapi->tx_wake_thresh_cur))
netif_tx_wake_queue(txq);
__netif_tx_unlock(txq);
}
@@ -7849,8 +7850,8 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
if (unlikely(tg3_tx_avail(tnapi) <= desc_cnt_est)) {
netif_tx_stop_queue(txq);
- tnapi->wakeup_thresh = desc_cnt_est;
- BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending);
+ tnapi->tx_wake_thresh_cur = desc_cnt_est;
+ BUG_ON(tnapi->tx_wake_thresh_cur >= tnapi->tx_pending);
/* netif_tx_stop_queue() must be done before checking
* checking tx index in tg3_tx_avail() below, because in
@@ -7858,7 +7859,7 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
* netif_tx_queue_stopped().
*/
smp_mb();
- if (tg3_tx_avail(tnapi) <= tnapi->wakeup_thresh)
+ if (tg3_tx_avail(tnapi) <= tnapi->tx_wake_thresh_cur)
return NETDEV_TX_BUSY;
netif_tx_wake_queue(txq);
@@ -7938,14 +7939,14 @@ static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
if (!netif_tx_queue_stopped(txq)) {
netif_tx_stop_queue(txq);
- tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi);
+ tnapi->tx_wake_thresh_cur = tnapi->tx_wake_thresh_def;
/* This is a hard error, log it. */
netdev_err(dev,
"BUG! Tx Ring full when queue awake!\n");
}
smp_mb();
- if (tg3_tx_avail(tnapi) <= tnapi->wakeup_thresh)
+ if (tg3_tx_avail(tnapi) <= tnapi->tx_wake_thresh_cur)
return NETDEV_TX_BUSY;
netif_tx_wake_queue(txq);
@@ -8127,7 +8128,7 @@ static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
tnapi->tx_prod = entry;
if (unlikely(tg3_tx_avail(tnapi) <= stop_thresh)) {
netif_tx_stop_queue(txq);
- tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi);
+ tnapi->tx_wake_thresh_cur = tnapi->tx_wake_thresh_def;
/* netif_tx_stop_queue() must be done before checking
* checking tx index in tg3_tx_avail() below, because in
@@ -8135,7 +8136,7 @@ static netdev_tx_t __tg3_start_xmit(struct sk_buff *skb,
* netif_tx_queue_stopped().
*/
smp_mb();
- if (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh)
+ if (tg3_tx_avail(tnapi) > tnapi->tx_wake_thresh_cur)
netif_tx_wake_queue(txq);
}
@@ -12379,8 +12380,11 @@ static int tg3_set_ringparam(struct net_device *dev, struct ethtool_ringparam *e
tp->rx_jumbo_pending = ering->rx_jumbo_pending;
dev->gso_max_segs = TG3_TX_SEG_PER_DESC(ering->tx_pending - 1);
- for (i = 0; i < tp->irq_max; i++)
+ for (i = 0; i < tp->irq_max; i++) {
tp->napi[i].tx_pending = ering->tx_pending;
+ tp->napi[i].tx_wake_thresh_def =
+ TG3_TX_WAKEUP_THRESH(&tp->napi[i]);
+ }
if (netif_running(dev)) {
tg3_halt(tp, RESET_KIND_SHUTDOWN, 1);
@@ -17820,6 +17824,7 @@ static int tg3_init_one(struct pci_dev *pdev,
tnapi->tp = tp;
tnapi->tx_pending = TG3_DEF_TX_RING_PENDING;
+ tnapi->tx_wake_thresh_def = TG3_TX_WAKEUP_THRESH(tnapi);
tnapi->int_mbox = intmbx;
if (i <= 4)
diff --git a/drivers/net/ethernet/broadcom/tg3.h b/drivers/net/ethernet/broadcom/tg3.h
index 6a7e13d..44a21cb 100644
--- a/drivers/net/ethernet/broadcom/tg3.h
+++ b/drivers/net/ethernet/broadcom/tg3.h
@@ -3004,11 +3004,12 @@ struct tg3_napi {
u32 tx_prod ____cacheline_aligned;
u32 tx_cons;
u32 tx_pending;
- u32 last_tx_cons;
u32 prodmbox;
- u32 wakeup_thresh;
+ u32 tx_wake_thresh_cur;
+ u32 tx_wake_thresh_def;
struct tg3_tx_buffer_desc *tx_ring;
struct tg3_tx_ring_info *tx_buffers;
+ u32 last_tx_cons;
dma_addr_t status_mapping;
dma_addr_t rx_rcb_mapping;
^ permalink raw reply related
* Re: [PATCH 2/3] tg3: Fix tx_pending checks for tg3_tso_bug
From: Benjamin Poirier @ 2014-08-21 21:59 UTC (permalink / raw)
To: Michael Chan; +Cc: Prashant Sreedharan, netdev, linux-kernel
In-Reply-To: <1408614683.7644.19.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
On 2014/08/21 02:51, Michael Chan wrote:
> On Wed, 2014-08-20 at 18:23 -0700, Benjamin Poirier wrote:
> > On 2014/08/19 16:10, Michael Chan wrote:
> > > On Tue, 2014-08-19 at 11:52 -0700, Benjamin Poirier wrote:
> > > > @@ -7838,11 +7838,14 @@ 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;
> > > >
> > > > - /* Estimate the number of fragments in the worst case */
> > > > - if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
> > > > + if (unlikely(tg3_tx_avail(tnapi) <= skb_shinfo(skb)->gso_segs)) {
> > > > + trace_printk("stopping queue, %d <= %d\n",
> > > > + tg3_tx_avail(tnapi), skb_shinfo(skb)->gso_segs);
> > > > netif_tx_stop_queue(txq);
> > > > + trace_printk("stopped queue\n");
> > > > + tnapi->wakeup_thresh = skb_shinfo(skb)->gso_segs;
> > > > + BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending);
> > > >
> > > > /* netif_tx_stop_queue() must be done before checking
> > > > * checking tx index in tg3_tx_avail() below, because in
> > >
> > > I don't quite understand this logic and I must be missing something.
> > > gso_segs is the number of TCP segments the large packet will be broken
> > > up into. If it exceeds dev->gso_max_segs, it means it exceeds
> > > hardware's capabilty and it will do GSO instead of TSO. But in this
> > > case in tg3_tso_bug(), we are doing GSO and we may not have enough DMA
> > > descriptors to do GSO. Each gso_seg typically requires 2 DMA
> > > descriptors.
> >
> > You're right, I had wrongly assumed that the skbs coming out of
> > skb_gso_segment() were linear. I'll address that in v2 of the patch by masking
> > out NETIF_F_SG in tg3_tso_bug().
> >
>
> While masking out NETF_F_SG will work, it will also disable checksum
> offload for the whole device momentarily.
>
> > I noticed another issue that had not occurred to me: when tg3_tso_bug is
> > submitting a full gso segs sequence to tg3_start_xmit, the code at the end of
> > that function stops the queue before the end of the sequence because tx_avail
> > becomes smaller than (MAX_SKB_FRAGS + 1). The transmission actually proceeds
> > because tg3_tso_bug() does not honour the queue state but it seems rather
> > unsightly to me.
>
> That's why the number of DMA descriptors that we estimate has to be
> accurate. It's unfortunate that the various tg3 chips require so many
> different workarounds. The objective is to keep TSO and checksum
> enabled and workaround the occasional packets using GSO.
Ah, now I understand the reason for the * 3 in
u32 frag_cnt_est = skb_shinfo(skb)->gso_segs * 3;
/* Estimate the number of fragments in the worst case */
but that is not really the "worst case". It's not forbidden to have more than
two frags per skb output from skb_gso_segment(). I've kept this estimation
approach but I've added code to validate the estimation or else linearize the
skb.
>
> I believe that the boundary error conditions that you brought up can be
> addressed by enforcing some limits on the tx ring size and by reducing
> gso_max_size/gso_max_segs when necessary (for example when MTU and/or
> ring size is set very small).
^ permalink raw reply
* [PATCH v2 2/3] tg3: Fix tx_pending check for MAX_SKB_FRAGS
From: Benjamin Poirier @ 2014-08-21 21:57 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1408658240-6811-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 v2 3/3] tg3: Fix tx_pending checks for tg3_tso_bug
From: Benjamin Poirier @ 2014-08-21 21:57 UTC (permalink / raw)
To: Prashant Sreedharan, Michael Chan; +Cc: netdev, linux-kernel
In-Reply-To: <1408658240-6811-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
I was concerned that this last change, because of the extra call in the
default xmit path, may impact performance so I performed an rr latency test
but I did not measure a significant impact. That test was with default mtu and
ring size.
# perf stat -r10 -ad netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d rr
* without patches
rr values: 7039.63 6865.03 6939.21 6919.31 6931.88 6932.74 6925.1 6953.33 6868.43 6935.65
sample size: 10
mean: 6931.031
standard deviation: 48.10918
quantiles: 6865.03 6920.757 6932.31 6938.32 7039.63
6930±50
Performance counter stats for 'netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d rr' (10 runs):
480643.024723 task-clock # 8.001 CPUs utilized ( +- 0.00% ) [100.00%]
855,136 context-switches # 0.002 M/sec ( +- 0.23% ) [100.00%]
521 CPU-migrations # 0.000 M/sec ( +- 6.49% ) [100.00%]
104 page-faults # 0.000 M/sec ( +- 2.73% )
298,416,906,437 cycles # 0.621 GHz ( +- 4.08% ) [15.01%]
812,072,320,370 stalled-cycles-frontend # 272.13% frontend cycles idle ( +- 1.89% ) [25.01%]
685,633,562,247 stalled-cycles-backend # 229.76% backend cycles idle ( +- 2.50% ) [35.00%]
117,665,891,888 instructions # 0.39 insns per cycle
# 6.90 stalled cycles per insn ( +- 2.22% ) [45.00%]
26,158,399,505 branches # 54.424 M/sec ( +- 2.10% ) [50.00%]
205,688,614 branch-misses # 0.79% of all branches ( +- 0.78% ) [50.00%]
27,882,474,171 L1-dcache-loads # 58.011 M/sec ( +- 1.98% ) [50.00%]
369,911,372 L1-dcache-load-misses # 1.33% of all L1-dcache hits ( +- 0.62% ) [50.00%]
76,240,847 LLC-loads # 0.159 M/sec ( +- 1.04% ) [40.00%]
3,220 LLC-load-misses # 0.00% of all LL-cache hits ( +- 19.49% ) [ 5.00%]
60.074059340 seconds time elapsed ( +- 0.00% )
* with patches
rr values: 6732.65 6920.1 6909.46 7032.41 6864.43 6897.6 6815.19 6967.83 6849.23 6929.52
sample size: 10
mean: 6891.842
standard deviation: 82.91901
quantiles: 6732.65 6853.03 6903.53 6927.165 7032.41
6890±80
Performance counter stats for 'netperf -H 192.168.9.30 -l60 -T 0,0 -t omni -- -d rr' (10 runs):
480675.949728 task-clock # 8.001 CPUs utilized ( +- 0.01% ) [100.00%]
850,461 context-switches # 0.002 M/sec ( +- 0.37% ) [100.00%]
564 CPU-migrations # 0.000 M/sec ( +- 5.67% ) [100.00%]
417 page-faults # 0.000 M/sec ( +- 76.04% )
287,019,442,295 cycles # 0.597 GHz ( +- 7.16% ) [15.01%]
828,198,830,689 stalled-cycles-frontend # 288.55% frontend cycles idle ( +- 3.01% ) [25.01%]
718,230,307,166 stalled-cycles-backend # 250.24% backend cycles idle ( +- 3.53% ) [35.00%]
117,976,598,188 instructions # 0.41 insns per cycle
# 7.02 stalled cycles per insn ( +- 4.06% ) [45.00%]
26,715,853,108 branches # 55.580 M/sec ( +- 3.77% ) [50.00%]
198,787,673 branch-misses # 0.74% of all branches ( +- 0.86% ) [50.00%]
28,416,922,166 L1-dcache-loads # 59.119 M/sec ( +- 3.54% ) [50.00%]
367,613,007 L1-dcache-load-misses # 1.29% of all L1-dcache hits ( +- 0.47% ) [50.00%]
75,260,575 LLC-loads # 0.157 M/sec ( +- 2.24% ) [40.00%]
5,777 LLC-load-misses # 0.01% of all LL-cache hits ( +- 36.03% ) [ 5.00%]
60.077898757 seconds time elapsed ( +- 0.01% )
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 | 67 +++++++++++++++++++++++++++++--------
drivers/net/ethernet/broadcom/tg3.h | 1 +
2 files changed, 54 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/tg3.c b/drivers/net/ethernet/broadcom/tg3.c
index 0cecd6d..c29f2e3 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);
/* Use GSO to workaround all TSO packets that meet HW bug conditions
* indicated in tg3_tx_frag_set()
@@ -7838,11 +7844,13 @@ 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 */
- if (unlikely(tg3_tx_avail(tnapi) <= frag_cnt_est)) {
+ if (unlikely(tg3_tx_avail(tnapi) <= desc_cnt_est)) {
netif_tx_stop_queue(txq);
+ tnapi->wakeup_thresh = desc_cnt_est;
+ BUG_ON(tnapi->wakeup_thresh >= tnapi->tx_pending);
/* netif_tx_stop_queue() must be done before checking
* checking tx index in tg3_tx_avail() below, because in
@@ -7850,7 +7858,7 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
* netif_tx_queue_stopped().
*/
smp_mb();
- if (tg3_tx_avail(tnapi) <= frag_cnt_est)
+ if (tg3_tx_avail(tnapi) <= tnapi->wakeup_thresh)
return NETDEV_TX_BUSY;
netif_tx_wake_queue(txq);
@@ -7858,14 +7866,33 @@ static int tg3_tso_bug(struct tg3 *tp, struct tg3_napi *tnapi,
segs = skb_gso_segment(skb, tp->dev->features &
~(NETIF_F_TSO | NETIF_F_TSO6));
- if (IS_ERR(segs) || !segs)
+ if (IS_ERR_OR_NULL(segs))
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)) {
+ nskb->next = segs;
+ segs = nskb;
+ do {
+ nskb = segs->next;
+
+ dev_kfree_skb_any(segs);
+ segs = nskb;
+ } while (segs);
+ 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:
@@ -7877,6 +7904,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;
@@ -7905,12 +7938,17 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
if (unlikely(budget <= (skb_shinfo(skb)->nr_frags + 1))) {
if (!netif_tx_queue_stopped(txq)) {
netif_tx_stop_queue(txq);
+ tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi);
/* This is a hard error, log it. */
netdev_err(dev,
"BUG! Tx Ring full when queue awake!\n");
}
- return NETDEV_TX_BUSY;
+ smp_mb();
+ if (tg3_tx_avail(tnapi) <= tnapi->wakeup_thresh)
+ return NETDEV_TX_BUSY;
+
+ netif_tx_wake_queue(txq);
}
entry = tnapi->tx_prod;
@@ -8087,8 +8125,9 @@ 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))) {
+ if (unlikely(tg3_tx_avail(tnapi) <= stop_thresh)) {
netif_tx_stop_queue(txq);
+ tnapi->wakeup_thresh = TG3_TX_WAKEUP_THRESH(tnapi);
/* netif_tx_stop_queue() must be done before checking
* checking tx index in tg3_tx_avail() below, because in
@@ -8096,7 +8135,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb, struct net_device *dev)
* netif_tx_queue_stopped().
*/
smp_mb();
- if (tg3_tx_avail(tnapi) > TG3_TX_WAKEUP_THRESH(tnapi))
+ if (tg3_tx_avail(tnapi) > tnapi->wakeup_thresh)
netif_tx_wake_queue(txq);
}
@@ -12319,9 +12358,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 + 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)) {
@@ -12341,6 +12378,7 @@ 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;
+ dev->gso_max_segs = TG3_TX_SEG_PER_DESC(ering->tx_pending - 1);
for (i = 0; i < tp->irq_max; i++)
tp->napi[i].tx_pending = ering->tx_pending;
@@ -17817,6 +17855,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 v2 1/3] tg3: Limit minimum tx queue wakeup threshold
From: Benjamin Poirier @ 2014-08-21 21:57 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
#!/bin/bash
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
* Re: [PATCH v2] net: ipv6: fib: don't sleep inside atomic lock
From: David Rientjes @ 2014-08-21 21:57 UTC (permalink / raw)
To: Benjamin Block
Cc: David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, netdev, linux-kernel,
Hannes Frederic Sowa
In-Reply-To: <1408642668-5808-1-git-send-email-bebl@mageta.org>
On Thu, 21 Aug 2014, Benjamin Block wrote:
> The function fib6_commit_metrics() allocates a piece of memory in mode
> GFP_KERNEL while holding an atomic lock from higher up in the stack, in
> the function __ip6_ins_rt(). This produces the following BUG:
>
> > BUG: sleeping function called from invalid context at mm/slub.c:1250
> > in_atomic(): 1, irqs_disabled(): 0, pid: 2909, name: dhcpcd
> > 2 locks held by dhcpcd/2909:
> > #0: (rtnl_mutex){+.+.+.}, at: [<ffffffff81978e67>] rtnl_lock+0x17/0x20
> > #1: (&tb->tb6_lock){++--+.}, at: [<ffffffff81a6951a>] ip6_route_add+0x65a/0x800
> > CPU: 1 PID: 2909 Comm: dhcpcd Not tainted 3.17.0-rc1 #1
> > Hardware name: ASUS All Series/Q87T, BIOS 0216 10/16/2013
> > 0000000000000008 ffff8800c8f13858 ffffffff81af135a 0000000000000000
> > ffff880212202430 ffff8800c8f13878 ffffffff810f8d3a ffff880212202c98
> > 0000000000000010 ffff8800c8f138c8 ffffffff8121ad0e 0000000000000001
> > Call Trace:
> > [<ffffffff81af135a>] dump_stack+0x4e/0x68
> > [<ffffffff810f8d3a>] __might_sleep+0x10a/0x120
> > [<ffffffff8121ad0e>] kmem_cache_alloc_trace+0x4e/0x190
> > [<ffffffff81a6bcd6>] ? fib6_commit_metrics+0x66/0x110
> > [<ffffffff81a6bcd6>] fib6_commit_metrics+0x66/0x110
> > [<ffffffff81a6cbf3>] fib6_add+0x883/0xa80
> > [<ffffffff81a6951a>] ? ip6_route_add+0x65a/0x800
> > [<ffffffff81a69535>] ip6_route_add+0x675/0x800
> > [<ffffffff81a68f2a>] ? ip6_route_add+0x6a/0x800
> > [<ffffffff81a6990c>] inet6_rtm_newroute+0x5c/0x80
> > [<ffffffff8197cf01>] rtnetlink_rcv_msg+0x211/0x260
> > [<ffffffff81978e67>] ? rtnl_lock+0x17/0x20
> > [<ffffffff81119708>] ? lock_release_holdtime+0x28/0x180
> > [<ffffffff81978e67>] ? rtnl_lock+0x17/0x20
> > [<ffffffff8197ccf0>] ? __rtnl_unlock+0x20/0x20
> > [<ffffffff819a989e>] netlink_rcv_skb+0x6e/0xd0
> > [<ffffffff81978ee5>] rtnetlink_rcv+0x25/0x40
> > [<ffffffff819a8e59>] netlink_unicast+0xd9/0x180
> > [<ffffffff819a9600>] netlink_sendmsg+0x700/0x770
> > [<ffffffff81103735>] ? local_clock+0x25/0x30
> > [<ffffffff8194e83c>] sock_sendmsg+0x6c/0x90
> > [<ffffffff811f98e3>] ? might_fault+0xa3/0xb0
> > [<ffffffff8195ca6d>] ? verify_iovec+0x7d/0xf0
> > [<ffffffff8194ec3e>] ___sys_sendmsg+0x37e/0x3b0
> > [<ffffffff8111ef15>] ? trace_hardirqs_on_caller+0x185/0x220
> > [<ffffffff81af979e>] ? mutex_unlock+0xe/0x10
> > [<ffffffff819a55ec>] ? netlink_insert+0xbc/0xe0
> > [<ffffffff819a65e5>] ? netlink_autobind.isra.30+0x125/0x150
> > [<ffffffff819a6520>] ? netlink_autobind.isra.30+0x60/0x150
> > [<ffffffff819a84f9>] ? netlink_bind+0x159/0x230
> > [<ffffffff811f989a>] ? might_fault+0x5a/0xb0
> > [<ffffffff8194f25e>] ? SYSC_bind+0x7e/0xd0
> > [<ffffffff8194f8cd>] __sys_sendmsg+0x4d/0x80
> > [<ffffffff8194f912>] SyS_sendmsg+0x12/0x20
> > [<ffffffff81afc692>] system_call_fastpath+0x16/0x1b
>
> Fixing this by replacing the mode GFP_KERNEL with GFP_ATOMIC.
>
> Signed-off-by: Benjamin Block <bebl@mageta.org>
> ---
> net/ipv6/ip6_fib.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/ipv6/ip6_fib.c b/net/ipv6/ip6_fib.c
> index cb4459b..76b7f5e 100644
> --- a/net/ipv6/ip6_fib.c
> +++ b/net/ipv6/ip6_fib.c
> @@ -643,7 +643,7 @@ static int fib6_commit_metrics(struct dst_entry *dst,
> if (dst->flags & DST_HOST) {
> mp = dst_metrics_write_ptr(dst);
> } else {
> - mp = kzalloc(sizeof(u32) * RTAX_MAX, GFP_KERNEL);
> + mp = kzalloc(sizeof(u32) * RTAX_MAX, GFP_ATOMIC);
> if (!mp)
> return -ENOMEM;
> dst_init_metrics(dst, mp, 0);
Acked-by: David Rientjes <rientjes@google.com>
Your patch is obviously good, but this kzalloc() could also be turned into
kcalloc() if you're so inclined.
^ permalink raw reply
* Re: [PATCH v4 2/2] net: moxa: replace build_skb() with netdev_alloc_skb_ip_align() / memcpy()
From: Michał Mirosław @ 2014-08-21 21:43 UTC (permalink / raw)
To: Jonas Jensen
Cc: netdev, f.fainelli, Linux Kernel, Ben Hutchings, David Miller,
linux-arm-kernel@lists.infradead.org
In-Reply-To: <1408459784-9385-2-git-send-email-jonas.jensen@gmail.com>
2014-08-19 16:49 GMT+02:00 Jonas Jensen <jonas.jensen@gmail.com>:
[...]
> diff --git a/drivers/net/ethernet/moxa/moxart_ether.c b/drivers/net/ethernet/moxa/moxart_ether.c
> index aa45607..17c9f0e 100644
> --- a/drivers/net/ethernet/moxa/moxart_ether.c
> +++ b/drivers/net/ethernet/moxa/moxart_ether.c
> @@ -226,14 +226,21 @@ static int moxart_rx_poll(struct napi_struct *napi, int budget)
> if (len > RX_BUF_SIZE)
> len = RX_BUF_SIZE;
>
> - skb = build_skb(priv->rx_buf[rx_head], priv->rx_buf_size);
> + dma_sync_single_for_cpu(&ndev->dev,
> + priv->rx_mapping[rx_head],
> + priv->rx_buf_size, DMA_FROM_DEVICE);
> + skb = netdev_alloc_skb_ip_align(ndev, len);
> if (unlikely(!skb)) {
> - net_dbg_ratelimited("build_skb failed\n");
> + net_dbg_ratelimited("netdev_alloc_skb_ip_align failed\n");
> priv->stats.rx_dropped++;
> priv->stats.rx_errors++;
> }
> -
> + memcpy(skb->data, priv->rx_buf[rx_head], len);
This has implicit: if (!skb) BUG(); There should probably be a return
or continue inside the if (!skb).
> skb_put(skb, len);
> + dma_sync_single_for_device(&ndev->dev,
> + priv->rx_mapping[rx_head],
> + priv->rx_buf_size, DMA_FROM_DEVICE);
> +
dma_sync_single_for_device() is not needed here as CPU does not and
should not write to the DMA_FROM_DEVICE mapping.
> skb->protocol = eth_type_trans(skb, ndev);
> napi_gro_receive(&priv->napi, skb);
> rx++;
Best Regards,
Michał Mirosław
^ permalink raw reply
* order:1 failure in ipoib_cm_alloc_rx_skb
From: Venkat Venkatsubra @ 2014-08-21 21:29 UTC (permalink / raw)
To: netdev
Hello All,
Many of our customers have hit this order:1 failure in ipoib_cm_alloc_rx_skb causing softlockup:
....
skb = dev_alloc_skb(IPOIB_CM_HEAD_SIZE + 12);
....
IPOIB_CM_HEAD_SIZE is defined as
IPOIB_ENCAP_LEN = 4,
IPOIB_CM_MTU = 0x10000 - 0x10, /* padding to align header to 16 */
IPOIB_CM_BUF_SIZE = IPOIB_CM_MTU + IPOIB_ENCAP_LEN,
IPOIB_CM_HEAD_SIZE = IPOIB_CM_BUF_SIZE % PAGE_SIZE,
For a 4kb PAGE_SIZE IPOIB_CM_HEAD_SIZE ends up as
(65536 - 16 + 4) % 4096 = 4084 bytes for a dev_alloc_skb(4084 + 12) or dev_alloc_skb(4096).
Redefining IPOIB_CM_HEAD_SIZE and IPOIB_CM_RX_SG as
IPOIB_CM_HEAD_SIZE = SKB_MAX_HEAD(NET_SKB_PAD + 12),
IPOIB_CM_RX_SG = ALIGN(IPOIB_CM_BUF_SIZE-IPOIB_CM_HEAD_SIZE,PAGE_SIZE)/PAGE_SIZE + 1,
resolves the problem by avoiding the order:1 allocation.
But in releases where NET_SKB_PAD is defined with max() as
#define NET_SKB_PAD max(32, L1_CACHE_BYTES)
we get compilation error:
CC [M] drivers/infiniband/ulp/ipoib/ipoib_main.o
In file included from drivers/infiniband/ulp/ipoib/ipoib_main.c:35:
drivers/infiniband/ulp/ipoib/ipoib.h:76: error: braced-group within expression allowed only inside a function
Changing it to a #define moves the error to when IPOIB_CM_RX_SG is used.
#define IPOIB_CM_HEAD_SIZE SKB_MAX_HEAD(NET_SKB_PAD + 12)
#define IPOIB_CM_RX_SG ALIGN(IPOIB_CM_BUF_SIZE-IPOIB_CM_HEAD_SIZE,PAGE_SIZE)/PAGE_SIZE + 1
The same compilation error comes from the "u64 mapping[IPOIB_CM_RX_SG];" line in
struct ipoib_cm_rx_buf {
struct sk_buff *skb;
u64 mapping[IPOIB_CM_RX_SG];
};
How should I fix this compilation problem without having to make too many changes ?
I am including the stack trace of the order:1 failure:
kswapd0: page allocation failure. order:1, mode:0x20
Pid: 273, comm: kswapd0 Not tainted 2.6.32-400.11.1.el5uek #1
Call Trace:
<IRQ> [<ffffffff810ddf74>] __alloc_pages_nodemask+0x524/0x595
[<ffffffff8110da3f>] kmem_getpages+0x4f/0xf4
[<ffffffff8110dc3c>] fallback_alloc+0x158/0x1ce
[<ffffffff8110ddd3>] ____cache_alloc_node+0x121/0x134
[<ffffffff8110e3f3>] kmem_cache_alloc_node_notrace+0x84/0xb9
[<ffffffff8110e46e>] __kmalloc_node+0x46/0x73
[<ffffffff813b9aa8>] ? __alloc_skb+0x72/0x13d
[<ffffffff813b9aa8>] __alloc_skb+0x72/0x13d
[<ffffffff813b9bdb>] alloc_skb+0x13/0x15
[<ffffffff813b9f11>] dev_alloc_skb+0x1b/0x38
[<ffffffffa029e722>] ipoib_cm_alloc_rx_skb+0x31/0x1de [ib_ipoib]
[<ffffffffa029fd04>] ipoib_cm_handle_rx_wc+0x3a1/0x5b8 [ib_ipoib]
[<ffffffffa0191bdc>] ? mlx4_ib_free_srq_wqe+0x27/0x54 [mlx4_ib]
[<ffffffffa01894d4>] ? mlx4_ib_poll_cq+0x620/0x65e [mlx4_ib]
[<ffffffff813b9015>] ? __kfree_skb+0x79/0x7e
[<ffffffffa029e9f7>] ? netif_tx_lock+0x44/0x71 [ib_ipoib]
[<ffffffffa029ae97>] ipoib_poll+0x87/0x128 [ib_ipoib]
[<ffffffff813c4b69>] net_rx_action+0xc6/0x1cd
[<ffffffff8105e8cd>] __do_softirq+0xd7/0x19e
[<ffffffff810aefdc>] ? handle_IRQ_event+0x66/0x120
[<ffffffff81012eec>] call_softirq+0x1c/0x30
[<ffffffff81014695>] do_softirq+0x46/0x89
[<ffffffff8105e752>] irq_exit+0x3b/0x7a
[<ffffffff8145bea1>] do_IRQ+0x99/0xb0
[<ffffffff81012713>] ret_from_intr+0x0/0x11
<EOI> [<ffffffff812379a3>] ? radix_tree_delete+0x8f/0x194
[<ffffffffa03693b3>] ? __nfs_access_zap_cache+0x75/0xb0 [nfs]
[<ffffffff81207d05>] ? ima_inode_free+0x35/0x55
[<ffffffff8112fd9c>] ? __destroy_inode+0x26/0x66
[<ffffffff8112fdf2>] ? destroy_inode+0x16/0x44
[<ffffffff81130074>] ? dispose_list+0xb2/0xe1
[<ffffffff81130251>] ? shrink_icache_memory+0x1ae/0x1e0
[<ffffffff810e3f83>] ? shrink_slab+0xe1/0x153
[<ffffffff810e5063>] ? kswapd+0x3dd/0x516
[<ffffffff810e26f3>] ? isolate_pages_global+0x0/0x1ba
[<ffffffff810432be>] ? need_resched+0x23/0x2d
[<ffffffff81077030>] ? autoremove_wake_function+0x0/0x3d
[<ffffffff810e4c86>] ? kswapd+0x0/0x516
[<ffffffff81076c87>] ? kthread+0x6e/0x76
[<ffffffff81012dea>] ? child_rip+0xa/0x20
[<ffffffff81076c19>] ? kthread+0x0/0x76
[<ffffffff81012de0>] ? child_rip+0x0/0x20
Thanks.
Venkat
^ permalink raw reply
* [PATCH] net: xgene: fix possible NULL dereference in xgene_enet_free_desc_rings()
From: Iyappan Subramanian @ 2014-08-21 20:44 UTC (permalink / raw)
To: davem, netdev; +Cc: dan.carpenter, Iyappan Subramanian
A NULL pointer dereference is possible for the argument ring->buf_pool
which is passed to xgene_enet_free_desc_ring(), as ring could be NULL.
And now since NULL pointers are being checked for before the calls to
xgene_enet_free_desc_ring(), might as well take advantage of them and
not call the function if the argument would be NULL.
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Signed-off-by: Iyappan Subramanian <isubramanian@apm.com>
---
drivers/net/ethernet/apm/xgene/xgene_enet_main.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
index e1a8f4e..e4222af 100644
--- a/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
+++ b/drivers/net/ethernet/apm/xgene/xgene_enet_main.c
@@ -563,15 +563,21 @@ static void xgene_enet_free_desc_rings(struct xgene_enet_pdata *pdata)
struct xgene_enet_desc_ring *ring;
ring = pdata->tx_ring;
- if (ring && ring->cp_ring && ring->cp_ring->cp_skb)
- devm_kfree(dev, ring->cp_ring->cp_skb);
- xgene_enet_free_desc_ring(ring);
+ if (ring) {
+ if (ring->cp_ring && ring->cp_ring->cp_skb)
+ devm_kfree(dev, ring->cp_ring->cp_skb);
+ xgene_enet_free_desc_ring(ring);
+ }
ring = pdata->rx_ring;
- if (ring && ring->buf_pool && ring->buf_pool->rx_skb)
- devm_kfree(dev, ring->buf_pool->rx_skb);
- xgene_enet_free_desc_ring(ring->buf_pool);
- xgene_enet_free_desc_ring(ring);
+ if (ring) {
+ if (ring->buf_pool) {
+ if (ring->buf_pool->rx_skb)
+ devm_kfree(dev, ring->buf_pool->rx_skb);
+ xgene_enet_free_desc_ring(ring->buf_pool);
+ }
+ xgene_enet_free_desc_ring(ring);
+ }
}
static struct xgene_enet_desc_ring *xgene_enet_create_desc_ring(
--
1.9.1
^ permalink raw reply related
* Re: BUG: lockdep (inconsistent usage) in netlink
From: Benjamin Block @ 2014-08-21 20:43 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel, linux-acpi, Lan Tianyu
In-Reply-To: <20140821185209.GC25021@kalium.Speedport_W723_V_Typ_A_1_01_001>
[-- Attachment #1: Type: text/plain, Size: 5384 bytes --]
On 08/21/2014 08:52 PM, Benjamin Block wrote:
> Hello,
>
> while rebooting one of my dev-machines I stumbled over this
> lockdep-mess-up:
>
>> =================================
>> [ INFO: inconsistent lock state ]
>> 3.17.0-rc1-00001-gb83ca8c #2 Tainted: G O
>> ---------------------------------
>> inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.
>> swapper/0/0 [HC1[1]:SC0[0]:HE0:SE1] takes:
>> (&(&list->lock)->rlock#3){?.-...}, at: [<ffffffff819580db>] skb_queue_tail+0x2b/0x60
>> {HARDIRQ-ON-W} state was registered at:
>> [<ffffffff8111c9f7>] __lock_acquire+0x877/0x1c90
>> [<ffffffff8111e45a>] lock_acquire+0xca/0x120
>> [<ffffffff81afc744>] _raw_spin_lock_bh+0x44/0x80
>> [<ffffffff819a8918>] netlink_poll+0xf8/0x1c0
>> [<ffffffff8194e031>] sock_poll+0x161/0x190
>> [<ffffffff81271ffb>] SyS_epoll_ctl+0x51b/0xd10
>> [<ffffffff81afd452>] system_call_fastpath+0x16/0x1b
>> irq event stamp: 1699744
>> hardirqs last enabled at (1699741): [<ffffffff8189b1d4>] cpuidle_enter_state+0xc4/0x190
>> hardirqs last disabled at (1699742): [<ffffffff81afdfaa>] common_interrupt+0x6a/0x6f
>> softirqs last enabled at (1699744): [<ffffffff810d7fda>] _local_bh_enable+0x4a/0x50
>> softirqs last disabled at (1699743): [<ffffffff810d88f0>] irq_enter+0x30/0x70
>>
>> other info that might help us debug this:
>> Possible unsafe locking scenario:
>>
>> CPU0
>> ----
>> lock(&(&list->lock)->rlock#3);
>> <Interrupt>
>> lock(&(&list->lock)->rlock#3);
>>
>> *** DEADLOCK ***
>>
>> no locks held by swapper/0/0.
>>
>> stack backtrace:
>> CPU: 0 PID: 0 Comm: swapper/0 Tainted: G O 3.17.0-rc1-00001-gb83ca8c #2
>> Hardware name: ASUS All Series/Q87T, BIOS 0216 10/16/2013
>> ffffffff8295a5b0 ffff8802158039a8 ffffffff81af20fa 0000000000000000
>> ffffffff822164e0 ffff880215803a08 ffffffff81aee400 0000000000000000
>> ffffffff00000000 ffff880200000001 ffffffff8105ac0f ffffffff82d2abe0
>> Call Trace:
>> <IRQ> [<ffffffff81af20fa>] dump_stack+0x4e/0x68
>> [<ffffffff81aee400>] print_usage_bug+0x1ec/0x1fd
>> [<ffffffff8105ac0f>] ? save_stack_trace+0x2f/0x50
>> [<ffffffff8111b600>] ? print_irq_inversion_bug+0x200/0x200
>> [<ffffffff8111c061>] mark_lock+0x191/0x2b0
>> [<ffffffff8111c96a>] __lock_acquire+0x7ea/0x1c90
>> [<ffffffff8111ca94>] ? __lock_acquire+0x914/0x1c90
>> [<ffffffff8111b600>] ? print_irq_inversion_bug+0x200/0x200
>> [<ffffffff8111ca94>] ? __lock_acquire+0x914/0x1c90
>> [<ffffffff8111e45a>] lock_acquire+0xca/0x120
>> [<ffffffff819580db>] ? skb_queue_tail+0x2b/0x60
>> [<ffffffff81afc590>] _raw_spin_lock_irqsave+0x50/0x90
>> [<ffffffff819580db>] ? skb_queue_tail+0x2b/0x60
>> [<ffffffff819580db>] skb_queue_tail+0x2b/0x60
>> [<ffffffff819a774f>] __netlink_sendskb+0x21f/0x250
>> [<ffffffff819a7d63>] netlink_broadcast_filtered+0x273/0x3b0
>> [<ffffffff819a7ebd>] netlink_broadcast+0x1d/0x20
>> [<ffffffff8152fb8a>] ? nla_reserve+0x2a/0x40
>> [<ffffffff81589728>] acpi_bus_generate_netlink_event+0x160/0x178
>> [<ffffffff815a8db9>] acpi_button_notify+0xe1/0xec
>> [<ffffffff81580648>] acpi_device_notify+0x19/0x1b
>> [<ffffffff81580662>] acpi_device_notify_fixed+0x18/0x1c
>> [<ffffffff8158f039>] acpi_ev_fixed_event_detect+0xe6/0x10d
>> [<ffffffff8159157a>] acpi_ev_sci_xrupt_handler+0x19/0x3f
>> [<ffffffff8157c1a9>] acpi_irq+0x16/0x31
>> [<ffffffff81131e2a>] handle_irq_event_percpu+0x6a/0x1d0
>> [<ffffffff81131fd8>] handle_irq_event+0x48/0x70
>> [<ffffffff8113534f>] ? handle_fasteoi_irq+0x2f/0x160
>> [<ffffffff811353e7>] handle_fasteoi_irq+0xc7/0x160
>> [<ffffffff8104cd94>] handle_irq+0x134/0x150
>> [<ffffffff810f4876>] ? atomic_notifier_call_chain+0x16/0x20
>> [<ffffffff81054dec>] ? __exit_idle+0x2c/0x30
>> [<ffffffff81affe7e>] do_IRQ+0x5e/0x100
>> [<ffffffff81afdfaf>] common_interrupt+0x6f/0x6f
>> <EOI> [<ffffffff8189b1df>] ? cpuidle_enter_state+0xcf/0x190
>> [<ffffffff8189b1d4>] ? cpuidle_enter_state+0xc4/0x190
>> [<ffffffff8189b387>] cpuidle_enter+0x17/0x20
>> [<ffffffff81111ae1>] cpu_startup_entry+0x3a1/0x3c0
>> [<ffffffff81ae92a4>] rest_init+0xc4/0xd0
>> [<ffffffff81ae91e5>] ? rest_init+0x5/0xd0
>> [<ffffffff825718a1>] ? ftrace_init+0xa8/0x13b
>> [<ffffffff8255103a>] start_kernel+0x461/0x46e
>> [<ffffffff82550939>] ? set_init_arg+0x57/0x57
>> [<ffffffff825505af>] x86_64_start_reservations+0x2a/0x2c
>> [<ffffffff825506ae>] x86_64_start_kernel+0xfd/0x101
>
> Sadly I couldn't reproduce it. This looks all to be very general
> functions and my best guess is, netlink_poll() needs to be irq-save.
> Thing is, the corresponding code is quite old and I can't really bisec
> it, because the none-reproducibility.
>
Thinking more about it.. this seems to be unlikely. More like the
acpi-irq chain should not do netlink-events still in irq-context - just
guessing here, sry :).
I tracked around a little and came up with more recent commits in that
call-chain:
commit 0bf6368ee8f25826d0645c0f7a4f17c8845356a4
- adds acpi_bus_generate_netlink_event to the chain
Again, all other places around the chain seems quit old or unrelated.
>
> There is only the small ipv6-fib patch applied, I send in earlier today
> (https://lkml.org/lkml/2014/8/21/506). This should have nothing to do
> with this here.
>
- Benjamin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 648 bytes --]
^ permalink raw reply
* Re: [PATCH] carl9170: Replace rcu_dereference() with rcu_access_pointer()
From: Andreea Bernat @ 2014-08-21 20:10 UTC (permalink / raw)
To: Christian Lamparter
Cc: linville, linux-wireless, netdev, linux-kernel, paulmck
In-Reply-To: <3042528.E5hT1D5bWz@debian64>
On Wed, Aug 20, 2014 at 01:20:02PM -0700, Christian Lamparter wrote:
> On Wednesday, August 20, 2014 08:32:11 PM Andreea Bernat wrote:
> > On Mon, Aug 18, 2014 at 09:29:36PM +0200, Christian Lamparter wrote:
> > > On Sunday, August 17, 2014 01:48:07 PM Andreea-Cristina Bernat wrote:
> > > > The rcu_dereference() call is used directly in a condition.
> > > > Since its return value is never dereferenced it is recommended to use
> > > > "rcu_access_pointer()" instead of "rcu_dereference()".
> > > > Therefore, this patch makes the replacement.
> > > > [...]
> > > > Signed-off-by: Andreea-Cristina Bernat <bernat.ada@gmail.com>
> > > > ---
> > > > drivers/net/wireless/ath/carl9170/main.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/net/wireless/ath/carl9170/main.c b/drivers/net/wireless/ath/carl9170/main.c
> > > > index f8ded84..12018ff 100644
> > > > --- a/drivers/net/wireless/ath/carl9170/main.c
> > > > +++ b/drivers/net/wireless/ath/carl9170/main.c
> > > > @@ -1431,7 +1431,7 @@ static int carl9170_op_ampdu_action(struct ieee80211_hw *hw,
> > > > return -EOPNOTSUPP;
> > > >
> > > > rcu_read_lock();
> > > > - if (rcu_dereference(sta_info->agg[tid])) {
> > > > + if (rcu_access_pointer(sta_info->agg[tid])) {
> > > > rcu_read_unlock();
> > > > return -EBUSY;
> > > > }
> > >
> > > There's more. The check does not do a whole lot. I think *it* [the check] and the
> > > rcu_read_[un]lock [and the return -EBUSY] can be removed completely from the
> > > IEEE80211_AMPDU_TX_START code-path in carl9170_op_ampdu_action.
> > >
> > > It would be awesome, if you could you make a patch which removes this
> > > unneeded cosmic-ray-protection check :-) .
> >
> > Could you tell me why you think that those lines have to be removed?
> The carl9170_op_ampdu_action callback is used exclusively by the mac80211
> framework to notify the driver about setup and tear down of TX and RX
> aggregation sessions. Hence, mac80211 takes great care of performing
> sanity checks and properly serializing calls to the driver's ampdu_action
> callback.
>
> Specifically mac80211 already prevents the START of an TX aggregation session,
> if the aggregation session is already active [0]. Therefore the driver doesn't
> need to perform a similar check as well. This is why:
> - the expression (rcu_dereference(sta_info->agg[tid])) never evaluates to true
> -> the -EBUSY exit path is "dead code"
>
> And without the rcu_dereference(...) the rcu_read protection is not needed
> either. So it can be removed for this case as well.
>
> > I would like to fully understand this before I remove them.
> Let me know if the explanation above answers sufficient :).
> If not, I need some *pointers* to what needs further
> explanation.
Your explanation is clear. I will send a version two of the patch with
those lines removed.
Thank you,
Andreea
>
> Regards
> Christian
>
> [0] <http://lxr.free-electrons.com/source/net/mac80211/agg-tx.c#L583>
>
^ permalink raw reply
* Re: [RFC] net: ipv4: drop unicast encapsulated in L2 multicast
From: Julian Anastasov @ 2014-08-21 19:51 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless, netdev, Johannes Berg
In-Reply-To: <1408641747-22199-1-git-send-email-johannes@sipsolutions.net>
Hello,
On Thu, 21 Aug 2014, Johannes Berg wrote:
> From: Johannes Berg <johannes.berg@intel.com>
>
> RFC 1122 says that unicast packets encapsulated in broadcast
> link-layer packets should be dropped. Implement that, but also
> extend it to link-layer multicast packets.
>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> net/ipv4/route.c | 17 +++++++++++++++++
> 1 file changed, 17 insertions(+)
>
> diff --git a/net/ipv4/route.c b/net/ipv4/route.c
> index eaa4b000c7b4..c374fcc73ee0 100644
> --- a/net/ipv4/route.c
> +++ b/net/ipv4/route.c
> @@ -1710,6 +1710,23 @@ static int ip_route_input_slow(struct sk_buff *skb, __be32 daddr, __be32 saddr,
> goto no_route;
> }
>
> + /* RFC 1122 3.3.6:
> + *
> + * When a host sends a datagram to a link-layer broadcast address,
> + * the IP destination address MUST be a legal IP broadcast or IP
> + * multicast address.
> + *
> + * A host SHOULD silently discard a datagram that is received via
> + * a link-layer broadcast (see Section 2.4) but does not specify
> + * an IP multicast or broadcast destination address.
> + *
> + * We also do this for link-layer multicast.
> + */
> + if ((skb->pkt_type == PACKET_BROADCAST ||
> + skb->pkt_type == PACKET_MULTICAST) &&
> + res.type != RTN_BROADCAST)
> + goto e_inval;
This place is ok for IP context but ip_route_input
is also called from ARP context and other places.
You are using pkt_type in route.c for first time.
At least inet_rtm_getroute() does not set it. You
have to audit all call sites, may be skb->protocol check
can be needed too, I guess ARP is broken otherwise.
And I'm not sure if skb->protocol is actual in
ip4ip6_err() after decapsulation. Adding more skb
fields to check is risky due to such places.
OTOH, the receive routines for protocols like
UDP, TCP, SCTP already have pkt_type checks. As result,
this is an extra check for them.
You should also consider that this change breaks
CLUSTERIP which uses multicast link-layer address and
local (shared) IP.
> if (res.type == RTN_BROADCAST)
> goto brd_input;
Is this place better, after checking for RTN_BROADCAST?
/* ARP link-layer broadcasts are acceptable here */
if ((skb->pkt_type == PACKET_BROADCAST ||
skb->pkt_type == PACKET_MULTICAST) &&
skb->protocol == htons(ETH_P_IP))
goto e_inval;
Regards
--
Julian Anastasov <ja@ssi.bg>
^ 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