* [PATCHv8 net-next 3/4] sunvnet: allow admin to set sunvnet MTU
From: David L Stevens @ 2014-09-29 19:54 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Sowmini Varadhan, Raghuram Kothahota
This patch allows an admin to set the MTU on a sunvnet device to arbitrary
values between the minimum (68) and maximum (65535) IPv4 packet sizes.
Signed-off-by: David L Stevens <david.stevens@oracle.com>
---
arch/sparc/kernel/ldc.c | 2 +-
drivers/net/ethernet/sun/sunvnet.c | 7 +++++--
drivers/net/ethernet/sun/sunvnet.h | 6 ++++--
3 files changed, 10 insertions(+), 5 deletions(-)
diff --git a/arch/sparc/kernel/ldc.c b/arch/sparc/kernel/ldc.c
index 66dacd5..0af28b9 100644
--- a/arch/sparc/kernel/ldc.c
+++ b/arch/sparc/kernel/ldc.c
@@ -2159,7 +2159,7 @@ int ldc_map_single(struct ldc_channel *lp,
state.pte_idx = (base - iommu->page_table);
state.nc = 0;
fill_cookies(&state, (pa & PAGE_MASK), (pa & ~PAGE_MASK), len);
- BUG_ON(state.nc != 1);
+ BUG_ON(state.nc > ncookies);
return state.nc;
}
diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index 7faa0ca..e1f954c 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -887,6 +887,9 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
if (unlikely(!skb))
goto out_dropped;
+ if (skb->len > port->rmtu)
+ goto out_dropped;
+
spin_lock_irqsave(&port->vio.lock, flags);
dr = &port->vio.drings[VIO_DRIVER_TX_RING];
@@ -917,7 +920,7 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
port->tx_bufs[txi].skb = skb;
err = ldc_map_single(port->vio.lp, start, nlen,
- port->tx_bufs[txi].cookies, 2,
+ port->tx_bufs[txi].cookies, VNET_MAXCOOKIES,
(LDC_MAP_SHADOW | LDC_MAP_DIRECT | LDC_MAP_RW));
if (err < 0) {
netdev_info(dev, "tx buffer map error %d\n", err);
@@ -1146,7 +1149,7 @@ static void vnet_set_rx_mode(struct net_device *dev)
static int vnet_change_mtu(struct net_device *dev, int new_mtu)
{
- if (new_mtu != ETH_DATA_LEN)
+ if (new_mtu < 68 || new_mtu > 65535)
return -EINVAL;
dev->mtu = new_mtu;
diff --git a/drivers/net/ethernet/sun/sunvnet.h b/drivers/net/ethernet/sun/sunvnet.h
index f18409b..ead9001 100644
--- a/drivers/net/ethernet/sun/sunvnet.h
+++ b/drivers/net/ethernet/sun/sunvnet.h
@@ -11,7 +11,7 @@
*/
#define VNET_TX_TIMEOUT (5 * HZ)
-#define VNET_MAXPACKET 1518ULL /* ETH_FRAMELEN + VLAN_HDR */
+#define VNET_MAXPACKET (65535ULL + ETH_HLEN + VLAN_HLEN)
#define VNET_TX_RING_SIZE 512
#define VNET_TX_WAKEUP_THRESH(dr) ((dr)->pending / 4)
@@ -21,10 +21,12 @@
*/
#define VNET_PACKET_SKIP 6
+#define VNET_MAXCOOKIES (VNET_MAXPACKET/PAGE_SIZE + 1)
+
struct vnet_tx_entry {
struct sk_buff *skb;
unsigned int ncookies;
- struct ldc_trans_cookie cookies[2];
+ struct ldc_trans_cookie cookies[VNET_MAXCOOKIES];
};
struct vnet;
--
1.7.1
^ permalink raw reply related
* [PATCHv8 net-next 4/4] sunvnet: generate ICMP PTMUD messages for smaller port MTUs
From: David L Stevens @ 2014-09-29 19:54 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Sowmini Varadhan, Raghuram Kothahota
This patch sends ICMP and ICMPv6 messages for Path MTU Discovery when a remote
port MTU is smaller than the device MTU. This allows mixing newer VIO protocol
devices that support MTU negotiation with older devices that do not on the
same vswitch. It also allows Linux-Linux LDOMs to use 64K-1 data packets even
though Solaris vswitch is limited to <16K MTU.
Signed-off-by: David L Stevens <david.stevens@oracle.com>
---
drivers/net/ethernet/sun/sunvnet.c | 37 +++++++++++++++++++++++++++++++++++-
1 files changed, 36 insertions(+), 1 deletions(-)
diff --git a/drivers/net/ethernet/sun/sunvnet.c b/drivers/net/ethernet/sun/sunvnet.c
index e1f954c..b4e5303 100644
--- a/drivers/net/ethernet/sun/sunvnet.c
+++ b/drivers/net/ethernet/sun/sunvnet.c
@@ -17,6 +17,13 @@
#include <linux/mutex.h>
#include <linux/if_vlan.h>
+#if IS_ENABLED(CONFIG_IPV6)
+#include <linux/icmpv6.h>
+#endif
+
+#include <net/icmp.h>
+#include <net/route.h>
+
#include <asm/vio.h>
#include <asm/ldc.h>
@@ -887,8 +894,36 @@ static int vnet_start_xmit(struct sk_buff *skb, struct net_device *dev)
if (unlikely(!skb))
goto out_dropped;
- if (skb->len > port->rmtu)
+ if (skb->len > port->rmtu) {
+ unsigned long localmtu = port->rmtu - ETH_HLEN;
+
+ if (vio_version_after_eq(&port->vio, 1, 3))
+ localmtu -= VLAN_HLEN;
+
+ if (skb->protocol == htons(ETH_P_IP)) {
+ struct flowi4 fl4;
+ struct rtable *rt = NULL;
+
+ memset(&fl4, 0, sizeof(fl4));
+ fl4.flowi4_oif = dev->ifindex;
+ fl4.flowi4_tos = RT_TOS(ip_hdr(skb)->tos);
+ fl4.daddr = ip_hdr(skb)->daddr;
+ fl4.saddr = ip_hdr(skb)->saddr;
+
+ rt = ip_route_output_key(dev_net(dev), &fl4);
+ if (!IS_ERR(rt)) {
+ skb_dst_set(skb, &rt->dst);
+ icmp_send(skb, ICMP_DEST_UNREACH,
+ ICMP_FRAG_NEEDED,
+ htonl(localmtu));
+ }
+ }
+#if IS_ENABLED(CONFIG_IPV6)
+ else if (skb->protocol == htons(ETH_P_IPV6))
+ icmpv6_send(skb, ICMPV6_PKT_TOOBIG, 0, localmtu);
+#endif
goto out_dropped;
+ }
spin_lock_irqsave(&port->vio.lock, flags);
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] net: Add ndo_gso_check
From: Or Gerlitz @ 2014-09-29 19:59 UTC (permalink / raw)
To: Tom Herbert, Jeff Kirsher, Alexander Duyck, David Miller
Cc: Linux Netdev List, Thomas Graf, Pravin Shelar, John Fastabend
In-Reply-To: <1411962607-27878-1-git-send-email-therbert@google.com>
On Mon, Sep 29, 2014 at 6:50 AM, Tom Herbert <therbert@google.com> wrote:
> Add ndo_gso_check which a device can define to indicate whether is
> is capable of doing GSO on a packet. This funciton would be called from
> the stack to determine whether software GSO is needed to be done.
please, no...
We should strive to have a model/architecture under which the driver
can clearly advertize up something (bits, collection of bits,
whatever) which the stack can run through some dec-sion making code
and decide if to GSO yes/no, do it ala SW or ala HW. As life, this
model need not be perfect and can be biased towards being
simple/robust/conservative and developed incrementally.
We certainly don't want each driver that support some sort of HW
offloads for tunnels (today we have 4-5, tomorrow maybe 40...) to
implement their super sophisticated/efficient "dig in a packet and
tell myself if I can GSO/CSUM it or not" code, while repeating (expect
many copy/paste bugs...) the other drivers' code.
Saying that, while looking quickly on the fm10k driver during it's
super fast upstream review cycle, I saw something which looked like
yellow light, I drove one, but now I see it was very much a red one:
the chain of calls:
fm10k_xmit_frame_ring --> fm10k_tso -->
fm10k_tx_encap_offload --> {fm10k_port_is_vxlan, fm10k_port_is_nvgre}
is pretty much live example to what you are describing here, right?
L2 Drivers are not supposed to do such heavy duty lookups in packets
with tons of code that can be generic.
I would suggest as band aid to the current situation with non-VXLAN
UDP offloads enabled upstream and few drivers that don't really
support GSO/CSUM for udp tunnels which are not vxlan -- to have a
VXLAN bit which tells to the stack "I can do HW GSO to VXLAN" and have
the stack to enable HW GSO over these devices only to VXLAN. This
would bring the upstream code into consistent/well-defined state, and
we can take it from there.
Or.
SB another comment
> A driver should populate this function if it advertises GSO types for
> which there are combinations that it wouldn't be able to handle. For
> instance a device that performs UDP tunneling might only implement
> support for transparent Ethernet bridging type of inner packets
> or might have limitations on lengths of inner headers.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
> ---
> include/linux/netdevice.h | 12 +++++++++++-
> net/core/dev.c | 2 +-
> 2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> index 9f5d293..f8c2027 100644
> --- a/include/linux/netdevice.h
> +++ b/include/linux/netdevice.h
> @@ -997,6 +997,12 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
> * Callback to use for xmit over the accelerated station. This
> * is used in place of ndo_start_xmit on accelerated net
> * devices.
> + * bool (*ndo_gso_check) (struct sk_buff *skb,
> + * struct net_device *dev);
> + * Called by core transmit path to determine if device is capable of
> + * performing GSO on a packet. The device returns true if it is
> + * able to GSO the packet, false otherwise. If the return value is
> + * false the stack will do software GSO.
> */
> struct net_device_ops {
> int (*ndo_init)(struct net_device *dev);
> @@ -1146,6 +1152,8 @@ struct net_device_ops {
> struct net_device *dev,
> void *priv);
> int (*ndo_get_lock_subclass)(struct net_device *dev);
> + bool (*ndo_gso_check) (struct sk_buff *skb,
> + struct net_device *dev);
> };
>
> /**
> @@ -3536,10 +3544,12 @@ static inline bool skb_gso_ok(struct sk_buff *skb, netdev_features_t features)
> (!skb_has_frag_list(skb) || (features & NETIF_F_FRAGLIST));
> }
>
> -static inline bool netif_needs_gso(struct sk_buff *skb,
> +static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
> netdev_features_t features)
> {
> return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
> + (dev->netdev_ops->ndo_gso_check &&
> + !dev->netdev_ops->ndo_gso_check(skb, dev)) ||
> unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
> (skb->ip_summed != CHECKSUM_UNNECESSARY)));
> }
> diff --git a/net/core/dev.c b/net/core/dev.c
> index e2ced01..8c2b9bb 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2680,7 +2680,7 @@ struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
> if (skb->encapsulation)
> features &= dev->hw_enc_features;
>
> - if (netif_needs_gso(skb, features)) {
> + if (netif_needs_gso(dev, skb, features)) {
> struct sk_buff *segs;
>
> segs = skb_gso_segment(skb, features);
unrelated fix? best to put in a different patch.
^ permalink raw reply
* Re: [PATCH v5 2/3] net: Add Keystone NetCP ethernet driver
From: Santosh Shilimkar @ 2014-09-29 20:02 UTC (permalink / raw)
To: David Miller
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A,
grant.likely-QSEj5FYQhm4dnm+yROfE0A,
devicetree-u79uwXL29TY76Z2rM5mHXA, sandeep_n-l0cyMroinI0,
joe-6d6DIl74uiNBDgjK7y7TUQ
In-Reply-To: <20140929.155200.2231132602322345812.davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org>
On Monday 29 September 2014 03:52 PM, David Miller wrote:
> From: Santosh Shilimkar <santosh.shilimkar-l0cyMroinI0@public.gmane.org>
> Date: Thu, 25 Sep 2014 13:48:36 -0400
>
>> +static inline int gbe_phy_link_status(struct gbe_slave *slave)
>> +{
>> + if (!slave->phy)
>> + return 1;
>> +
>> + if (slave->phy->link)
>> + return 1;
>> +
>> + return 0;
>> +}
>
> Please use 'bool' as the return type and return 'true' or 'false'.
>
ok
> Do not use 'inline' in foo.c files, let the compiler decide. Please
> audit this entire submission for this problem.
>
ok.
>> +static int gbe_port_reset(struct gbe_slave *slave)
>> +{
>> + u32 i, v;
>> +
>> + /* Set the soft reset bit */
>> + writel_relaxed(SOFT_RESET, GBE_REG_ADDR(slave, emac_regs, soft_reset));
>
> This driver seems to use relaxed readl and writel for almost everything.
>
> That absolutely cannot be right. For example, here, you depend upon the
> ordering of this writel_relaxed() to reset the chip relative to the
> real_relaxed() you subsequently do to check ths bits.
>
> I seriously think that *_relaxed() should only be done in very special
> circumstances where 1) the performance matters and 2) the validity of
> the usage has been put under a microscope and fully documented with huge
> comments above the *_relaxed() calls.
>
> If you cannot reduce and properly document the really necessary *_relaxed()
> uses, just convert them all to non-_relaxed() for now.
>
We can stick to non-*relaxed() versions. No problems here.
> I'm also warning you ahead of time that since nobody else seems to feel
> like reviewing this enormous submission, you are going to have to get used
> to me pushing back on these changes over and over for small things like
> coding style and structural/API issues until some reviews it on a higher
> level.
>
> I really don't want to apply this series until someone thinks seriously
> about the driver's design and the long term ramifications of having a
> driver like this in the tree with so many random TX etc. hooks.
>
The driver has been on the list. Jamal and you have given your comments,
suggestion and we have incorporated that. What else we can do ?
We are badly missing mainline network driver support for the Keystone
and hence I request you to help here.
regards,
Santosh
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH v5 02/10] net: pxa168_eth: add device tree support
From: David Miller @ 2014-09-29 20:04 UTC (permalink / raw)
To: antoine.tenart
Cc: sebastian.hesselbarth, alexandre.belloni, thomas.petazzoni, zmxu,
jszhang, netdev, linux-arm-kernel, devicetree, linux-kernel
In-Reply-To: <1411742036-23520-3-git-send-email-antoine.tenart@free-electrons.com>
From: Antoine Tenart <antoine.tenart@free-electrons.com>
Date: Fri, 26 Sep 2014 16:33:48 +0200
> @@ -1603,6 +1620,12 @@ static int pxa168_eth_suspend(struct platform_device *pdev, pm_message_t state)
> #define pxa168_eth_suspend NULL
> #endif
>
> +static const struct of_device_id pxa168_eth_of_match[] = {
> + { .compatible = "marvell,pxa168-eth" },
> + { },
> +};
> +MODULE_DEVICE_TABLE(of, pxa168_eth_of_match)
> +
> static struct platform_driver pxa168_eth_driver = {
You didn't even compile test this change with module support enabled.
drivers/net/ethernet/marvell/pxa168_eth.c:1670:1: error: expected ‘,’ or ‘;’ before ‘static’
MODULE_DEVICE_TABLE() must be completed with a trailing ';'
^ permalink raw reply
* Re: [PATCH] tun: make sure interface usage can not overflow
From: Hannes Frederic Sowa @ 2014-09-29 20:04 UTC (permalink / raw)
To: Kees Cook
Cc: David Laight, linux-kernel@vger.kernel.org, David S. Miller,
Jason Wang, Zhi Yong Wu, Michael S. Tsirkin, Tom Herbert,
Masatake YAMATO, Xi Wang, stephen hemminger,
netdev@vger.kernel.org
In-Reply-To: <CAGXu5jKL13qdCBet3yqpc3vLYh_LifBACzL+NP8n-A+E8RMZXw@mail.gmail.com>
On Mo, 2014-09-29 at 12:41 -0700, Kees Cook wrote:
> On Mon, Sep 29, 2014 at 4:04 AM, David Laight <David.Laight@aculab.com> wrote:
> > From: Kees Cook
> >> This makes the size argument a const, since it is always populated by
> >> the caller.
> >
> > There is almost no point making parameters 'const.
> > ('const foo *' makes sense).
> >
> >> Additionally double-checks to make sure the copy_from_user
> >> can never overflow, keeping CONFIG_DEBUG_STRICT_USER_COPY_CHECKS happy:
> >>
> >> In function 'copy_from_user',
> >> inlined from '__tun_chr_ioctl' at drivers/net/tun.c:1871:7:
> >> ... copy_from_user() buffer size is not provably correct
> >
> > If 'ifreq_len' could be too big then you want to error the ioctl, not panic.
> > If it can't be too big you don't need the check.
>
> The ifreq_len comes from the callers, and is the output of "sizeof"
> which is const. Changing the function parameter to "const" means any
> changes made in the future where the incoming value isn't const, the
> compiler will throw a warning.
Hmmm, I think you want something like BUILD_BUG_ON(!
__builtin_constant_p(var)). const in function argument only ensures that
the value cannot be modified in the function.
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH] net: Add ndo_gso_check
From: David Miller @ 2014-09-29 20:12 UTC (permalink / raw)
To: gerlitz.or
Cc: therbert, jeffrey.t.kirsher, alexander.h.duyck, netdev, tgraf,
pshelar, john.r.fastabend
In-Reply-To: <CAJ3xEMi-TRXevenx87UM7JnKqRQQATW4MBjbz8JdnQE9gk3KZw@mail.gmail.com>
From: Or Gerlitz <gerlitz.or@gmail.com>
Date: Mon, 29 Sep 2014 22:59:39 +0300
> Saying that, while looking quickly on the fm10k driver during it's
> super fast upstream review cycle,
I call bullshit. fm10k had been posted for review starting around 10
days before I pulled it in from Jeff's tree.
If I let most patches rot that long in patchwork there'd be a crowd
with pitchforks approaching me.
^ permalink raw reply
* Re: [PATCH v5 2/3] net: Add Keystone NetCP ethernet driver
From: David Miller @ 2014-09-29 20:12 UTC (permalink / raw)
To: santosh.shilimkar
Cc: netdev, linux-arm-kernel, linux-kernel, robh+dt, grant.likely,
devicetree, sandeep_n, joe
In-Reply-To: <5429BAD0.1050101@ti.com>
From: Santosh Shilimkar <santosh.shilimkar@ti.com>
Date: Mon, 29 Sep 2014 16:02:24 -0400
> We are badly missing mainline network driver support for the Keystone
> and hence I request you to help here.
It is absolutely not reasonable for you depend specifically upon me
for top-level review of a given change, sorry.
^ permalink raw reply
* Re: [net-next PATCH 1/1 V4] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jesper Dangaard Brouer @ 2014-09-29 20:14 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: Tom Herbert, Jamal Hadi Salim, Eric Dumazet, Linux Netdev List,
David S. Miller, Alexander Duyck,
Toke Høiland-Jørgensen, Florian Westphal, Dave Taht,
John Fastabend, Daniel Borkmann, Hannes Frederic Sowa
In-Reply-To: <20140925172329.7460f787@redhat.com>
On Thu, 25 Sep 2014 17:23:29 +0200 Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> I will redo the tests, once I get home to my testlab, as the remote lab
> I'm using now is annoyingly slow rebooting machines, as we not longer
> have a runtime option for enable/disable (I'm currently in Switzerland).
I'm going to change the bulking "budget" from 8 packet to 7 packets,
based on my netperf-wrapper test.
I've designed some tests for the tool "netperf-wrapper" that tries to
measure the HoL blocking effect. I've turned down the link speed to
100Mbit/s on driver igb. And uses a single TXQ setup
(cmdline: ethtool -L eth1 combined 1).
I can now show some kind of HoL blocking effect. The strange part is
the HoL effect only occurs above 8 packets, 7 packet and below show no
bad effect of this bulking patch.
Results 100Mbit/s test qdisc_prio_hol, latency in high prio band:
* GSO: stable average on 2.23ms
* TSO: varies between min 4.13ms to 4.41ms (range 0.28ms)
* No bulking: stable average on 1.71ms (3x outliners on 1.95ms)
* Bulking(8): varies between 1.71ms to 1.95ms (range 0.24ms)
* Bulking(7): stable average on 1.71ms (1x outliner on 1.95ms)
* Bulking(6): stable average on 1.71ms (3x outliners on 1.91ms)
* Bulking(5): stable average on 1.71ms (1x outliner on 1.91ms)
* Bulking(5): stable average on 1.71ms (1x outliner on 1.91ms)
* Bulking(4): stable average on 1.71ms (0x outliner)
Bulking(8) the calculation:
* Added delay were 0.24ms (1.95 - 1.71ms) corrosponding to 3000 bytes
* 1500 bytes *8 / 100Mbit = 0.12 ms
* (100*10^6)*(1.95/10^3)/8 = 24375 bytes
* (100*10^6)*(1.71/10^3)/8 = 21375 bytes
* Added HoL: 3000 bytes
* Still compared to TSO and GSO, the added HoL blocking is small.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH iproute2 v2] ip monitor: Changed 'Unknown message' format to be more informative
From: Vadim Kochan @ 2014-09-29 20:07 UTC (permalink / raw)
To: netdev; +Cc: Vadim Kochan
In case if unknown message was handled then it will be displayed as:
Unknown message: type=0x00000044(68) flags=0x00000000(0) len=0x0000004c(76)
Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
ip/ipmonitor.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/ip/ipmonitor.c b/ip/ipmonitor.c
index 088ec54..b7b2d98 100644
--- a/ip/ipmonitor.c
+++ b/ip/ipmonitor.c
@@ -143,8 +143,10 @@ static int accept_msg(const struct sockaddr_nl *who,
return 0;
if (n->nlmsg_type != NLMSG_ERROR && n->nlmsg_type != NLMSG_NOOP &&
n->nlmsg_type != NLMSG_DONE) {
- fprintf(fp, "Unknown message: %08x %08x %08x\n",
- n->nlmsg_len, n->nlmsg_type, n->nlmsg_flags);
+ fprintf(fp, "Unknown message: type=0x%08x(%d) flags=0x%08x(%d)"
+ "len=0x%08x(%d)\n", n->nlmsg_type, n->nlmsg_type,
+ n->nlmsg_flags, n->nlmsg_flags, n->nlmsg_len,
+ n->nlmsg_len);
}
return 0;
}
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] net: Add ndo_gso_check
From: Jesse Gross @ 2014-09-29 20:13 UTC (permalink / raw)
To: Tom Herbert; +Cc: David Miller, netdev, Or Gerlitz
In-Reply-To: <1411962607-27878-1-git-send-email-therbert@google.com>
On Sun, Sep 28, 2014 at 8:50 PM, Tom Herbert <therbert@google.com> wrote:
> Add ndo_gso_check which a device can define to indicate whether is
> is capable of doing GSO on a packet. This funciton would be called from
> the stack to determine whether software GSO is needed to be done. A
> driver should populate this function if it advertises GSO types for
> which there are combinations that it wouldn't be able to handle. For
> instance a device that performs UDP tunneling might only implement
> support for transparent Ethernet bridging type of inner packets
> or might have limitations on lengths of inner headers.
>
> Signed-off-by: Tom Herbert <therbert@google.com>
Offloads that might have limitations extend beyond GSO - checksum is
another possibility that could need something like a length check.
In addition, while I would also like to be optimistic about the
capabilities of existing NICs it's unlikely that any of them that are
advertising SKB_GSO_UDP_TUNNEL can actually do it with full
generality. So unless we can get the driver writers to chime in about
their capabilities (maybe we can, there's only a handful of them right
now), we probably need to provide a more conservative implementation
for those drivers. I guess I would probably do length equal to VXLAN
and perhaps containing Ethernet as a balance between the most
conservative and the most optimistic.
^ permalink raw reply
* [PATCH net-next v2] tcp: abort orphan sockets stalling on zero window probes
From: Yuchung Cheng @ 2014-09-29 20:20 UTC (permalink / raw)
To: davem; +Cc: edumazet, andrey.dmitrov, ncardwell, netdev, Yuchung Cheng
Currently we have two different policies for orphan sockets
that repeatedly stall on zero window ACKs. If a socket gets
a zero window ACK when it is transmitting data, the RTO is
used to probe the window. The socket is aborted after roughly
tcp_orphan_retries() retries (as in tcp_write_timeout()).
But if the socket was idle when it received the zero window ACK,
and later wants to send more data, we use the probe timer to
probe the window. If the receiver always returns zero window ACKs,
icsk_probes keeps getting reset in tcp_ack() and the orphan socket
can stall forever until the system reaches the orphan limit (as
commented in tcp_probe_timer()). This opens up a simple attack
to create lots of hanging orphan sockets to burn the memory
and the CPU, as demonstrated in the recent netdev post "TCP
connection will hang in FIN_WAIT1 after closing if zero window is
advertised." http://www.spinics.net/lists/netdev/msg296539.html
This patch follows the design in RTO-based probe: we abort an orphan
socket stalling on zero window when the probe timer reaches both
the maximum backoff and the maximum RTO. For example, an 100ms RTT
connection will timeout after roughly 153 seconds (0.3 + 0.6 +
.... + 76.8) if the receiver keeps the window shut. If the orphan
socket passes this check, but the system already has too many orphans
(as in tcp_out_of_resources()), we still abort it but we'll also
send an RST packet as the connection may still be active.
In addition, we change TCP_USER_TIMEOUT to cover (life or dead)
sockets stalled on zero-window probes. This changes the semantics
of TCP_USER_TIMEOUT slightly because it previously only applies
when the socket has pending transmission.
Signed-off-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Eric Dumazet <edumazet@google.com>
Signed-off-by: Neal Cardwell <ncardwell@google.com>
Reported-by: Andrey Dmitrov <andrey.dmitrov@oktetlabs.ru>
---
net/ipv4/tcp.c | 2 +-
net/ipv4/tcp_timer.c | 41 +++++++++++++++++++++--------------------
2 files changed, 22 insertions(+), 21 deletions(-)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index cf5e508..753b728 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -2693,7 +2693,7 @@ static int do_tcp_setsockopt(struct sock *sk, int level,
break;
#endif
case TCP_USER_TIMEOUT:
- /* Cap the max timeout in ms TCP will retry/retrans
+ /* Cap the max time in ms TCP will retry or probe the window
* before giving up and aborting (ETIMEDOUT) a connection.
*/
if (val < 0)
diff --git a/net/ipv4/tcp_timer.c b/net/ipv4/tcp_timer.c
index b24360f..9b21ae8 100644
--- a/net/ipv4/tcp_timer.c
+++ b/net/ipv4/tcp_timer.c
@@ -52,7 +52,7 @@ static void tcp_write_err(struct sock *sk)
* limit.
* 2. If we have strong memory pressure.
*/
-static int tcp_out_of_resources(struct sock *sk, int do_reset)
+static int tcp_out_of_resources(struct sock *sk, bool do_reset)
{
struct tcp_sock *tp = tcp_sk(sk);
int shift = 0;
@@ -72,7 +72,7 @@ static int tcp_out_of_resources(struct sock *sk, int do_reset)
if ((s32)(tcp_time_stamp - tp->lsndtime) <= TCP_TIMEWAIT_LEN ||
/* 2. Window is closed. */
(!tp->snd_wnd && !tp->packets_out))
- do_reset = 1;
+ do_reset = true;
if (do_reset)
tcp_send_active_reset(sk, GFP_ATOMIC);
tcp_done(sk);
@@ -270,40 +270,41 @@ static void tcp_probe_timer(struct sock *sk)
struct inet_connection_sock *icsk = inet_csk(sk);
struct tcp_sock *tp = tcp_sk(sk);
int max_probes;
+ u32 start_ts;
if (tp->packets_out || !tcp_send_head(sk)) {
icsk->icsk_probes_out = 0;
return;
}
- /* *WARNING* RFC 1122 forbids this
- *
- * It doesn't AFAIK, because we kill the retransmit timer -AK
- *
- * FIXME: We ought not to do it, Solaris 2.5 actually has fixing
- * this behaviour in Solaris down as a bug fix. [AC]
- *
- * Let me to explain. icsk_probes_out is zeroed by incoming ACKs
- * even if they advertise zero window. Hence, connection is killed only
- * if we received no ACKs for normal connection timeout. It is not killed
- * only because window stays zero for some time, window may be zero
- * until armageddon and even later. We are in full accordance
- * with RFCs, only probe timer combines both retransmission timeout
- * and probe timeout in one bottle. --ANK
+ /* RFC 1122 4.2.2.17 requires the sender to stay open indefinitely as
+ * long as the receiver continues to respond probes. We support this by
+ * default and reset icsk_probes_out with incoming ACKs. But if the
+ * socket is orphaned or the user specifies TCP_USER_TIMEOUT, we
+ * kill the socket when the retry count and the time exceeds the
+ * corresponding system limit. We also implement similar policy when
+ * we use RTO to probe window in tcp_retransmit_timer().
*/
- max_probes = sysctl_tcp_retries2;
+ start_ts = tcp_skb_timestamp(tcp_send_head(sk));
+ if (!start_ts)
+ skb_mstamp_get(&tcp_send_head(sk)->skb_mstamp);
+ else if (icsk->icsk_user_timeout &&
+ (s32)(tcp_time_stamp - start_ts) > icsk->icsk_user_timeout)
+ goto abort;
+ max_probes = sysctl_tcp_retries2;
if (sock_flag(sk, SOCK_DEAD)) {
const int alive = inet_csk_rto_backoff(icsk, TCP_RTO_MAX) < TCP_RTO_MAX;
max_probes = tcp_orphan_retries(sk, alive);
-
- if (tcp_out_of_resources(sk, alive || icsk->icsk_probes_out <= max_probes))
+ if (!alive && icsk->icsk_backoff >= max_probes)
+ goto abort;
+ if (tcp_out_of_resources(sk, true))
return;
}
if (icsk->icsk_probes_out > max_probes) {
- tcp_write_err(sk);
+abort: tcp_write_err(sk);
} else {
/* Only send another probe if we didn't close things up. */
tcp_send_probe0(sk);
--
2.1.0.rc2.206.gedb03e5
^ permalink raw reply related
* Re: [net-next PATCH 1/1 V4] qdisc: bulk dequeue support for qdiscs with TCQ_F_ONETXQUEUE
From: Jesper Dangaard Brouer @ 2014-09-29 20:23 UTC (permalink / raw)
To: Tom Herbert
Cc: Jamal Hadi Salim, Eric Dumazet, Linux Netdev List,
David S. Miller, Alexander Duyck,
Toke Høiland-Jørgensen, Florian Westphal, Dave Taht,
John Fastabend, Daniel Borkmann, Hannes Frederic Sowa, brouer
In-Reply-To: <CA+mtBx990LcBrZVhfNk224cofpDWGeOD=F0Nbe0DWoEF=LvOUA@mail.gmail.com>
Hi Tom,
On Thu, 25 Sep 2014 08:58:54 -0700 Tom Herbert <therbert@google.com> wrote:
> On Thu, Sep 25, 2014 at 8:23 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> > On Thu, 25 Sep 2014 08:05:38 -0700 Tom Herbert <therbert@google.com> wrote:
> >
> >> On Thu, Sep 25, 2014 at 7:57 AM, Jesper Dangaard Brouer <brouer@redhat.com> wrote:
> >> > On Thu, 25 Sep 2014 07:40:33 -0700 Tom Herbert <therbert@google.com> wrote:
> >> >
[...]
> >>
> >> That's great. In commit log, would be good to have results with
> >> TCP_STREAM also and please report aggregate CPU utilization changes
> >> (like from mpstat).
> >
> > The TCP_STREAM is not a good test for this, because unless disabling
> > both TSO and GSO the packets will not hit the code path (that this
> > patch changes). When we later add support for TSO and GSO bulking,
> > then it will make sense to include TCP_STREAM testing, not before.
> >
> Disabling TSO and GSO is fine. I'm interested to see interactions with TCP.
TCP already benefit from bulking, via TSO and especially for GSO
segmented packets, and this patch does not bulk TSO and GSO packets.
Testing effect for TCP involves disabling TSO and GSO, but I had to
enable GRO on the receiver, which reduces ACK packets, else the system
could not exceed the 10Gbit/s link capacilty with none-bulking.
Test cmd:
* netperf -H 192.168.8.2 -t TCP_STREAM -l 1000 -D 1 -n -N
The measured perf diff benefit for TCP_STREAM were 4.66% less CPU used
on calls to _raw_spin_lock() (mostly from sch_direct_xmit()).
Tool mpstat, while stressing the system with netperf 24x TCP_STREAM, shows:
* Disabled bulking: 8.30% soft 88.75% idle
* Enabled bulking: 7.80% soft 89.36% idle
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Sr. Network Kernel Developer at Red Hat
Author of http://www.iptv-analyzer.org
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* [PATCH v2 net-next] net: cleanup and document skb fclone layout
From: Eric Dumazet @ 2014-09-29 20:29 UTC (permalink / raw)
To: David Miller; +Cc: netdev
In-Reply-To: <1411860341.15768.55.camel@edumazet-glaptop2.roam.corp.google.com>
From: Eric Dumazet <edumazet@google.com>
Lets use a proper structure to clearly document and implement
skb fast clones.
Then, we might experiment more easily alternative layouts.
This patch adds a new skb_fclone_busy() helper, used by tcp and xfrm,
to stop leaking of implementation details.
Signed-off-by: Eric Dumazet <edumazet@google.com>
---
v2: rebased on latest net-next
include/linux/skbuff.h | 25 +++++++++++++++++++++++
net/core/skbuff.c | 41 +++++++++++++++++++--------------------
net/ipv4/tcp_output.c | 5 ----
net/xfrm/xfrm_policy.c | 4 ---
4 files changed, 47 insertions(+), 28 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 262efdbc346b..d8f7d74d5a4d 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -781,6 +781,31 @@ struct sk_buff *alloc_skb_with_frags(unsigned long header_len,
int *errcode,
gfp_t gfp_mask);
+/* Layout of fast clones : [skb1][skb2][fclone_ref] */
+struct sk_buff_fclones {
+ struct sk_buff skb1;
+
+ struct sk_buff skb2;
+
+ atomic_t fclone_ref;
+};
+
+/**
+ * skb_fclone_busy - check if fclone is busy
+ * @skb: buffer
+ *
+ * Returns true is skb is a fast clone, and its clone is not freed.
+ */
+static inline bool skb_fclone_busy(const struct sk_buff *skb)
+{
+ const struct sk_buff_fclones *fclones;
+
+ fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+ return skb->fclone == SKB_FCLONE_ORIG &&
+ fclones->skb2.fclone == SKB_FCLONE_CLONE;
+}
+
static inline struct sk_buff *alloc_skb_fclone(unsigned int size,
gfp_t priority)
{
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 4be570a4ab21..a8cebb40699c 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -257,15 +257,16 @@ struct sk_buff *__alloc_skb(unsigned int size, gfp_t gfp_mask,
kmemcheck_annotate_variable(shinfo->destructor_arg);
if (flags & SKB_ALLOC_FCLONE) {
- struct sk_buff *child = skb + 1;
- atomic_t *fclone_ref = (atomic_t *) (child + 1);
+ struct sk_buff_fclones *fclones;
- kmemcheck_annotate_bitfield(child, flags1);
+ fclones = container_of(skb, struct sk_buff_fclones, skb1);
+
+ kmemcheck_annotate_bitfield(&fclones->skb2, flags1);
skb->fclone = SKB_FCLONE_ORIG;
- atomic_set(fclone_ref, 1);
+ atomic_set(&fclones->fclone_ref, 1);
- child->fclone = SKB_FCLONE_UNAVAILABLE;
- child->pfmemalloc = pfmemalloc;
+ fclones->skb2.fclone = SKB_FCLONE_UNAVAILABLE;
+ fclones->skb2.pfmemalloc = pfmemalloc;
}
out:
return skb;
@@ -524,8 +525,7 @@ static void skb_release_data(struct sk_buff *skb)
*/
static void kfree_skbmem(struct sk_buff *skb)
{
- struct sk_buff *other;
- atomic_t *fclone_ref;
+ struct sk_buff_fclones *fclones;
switch (skb->fclone) {
case SKB_FCLONE_UNAVAILABLE:
@@ -533,22 +533,21 @@ static void kfree_skbmem(struct sk_buff *skb)
break;
case SKB_FCLONE_ORIG:
- fclone_ref = (atomic_t *) (skb + 2);
- if (atomic_dec_and_test(fclone_ref))
- kmem_cache_free(skbuff_fclone_cache, skb);
+ fclones = container_of(skb, struct sk_buff_fclones, skb1);
+ if (atomic_dec_and_test(&fclones->fclone_ref))
+ kmem_cache_free(skbuff_fclone_cache, fclones);
break;
case SKB_FCLONE_CLONE:
- fclone_ref = (atomic_t *) (skb + 1);
- other = skb - 1;
+ fclones = container_of(skb, struct sk_buff_fclones, skb2);
/* The clone portion is available for
* fast-cloning again.
*/
skb->fclone = SKB_FCLONE_UNAVAILABLE;
- if (atomic_dec_and_test(fclone_ref))
- kmem_cache_free(skbuff_fclone_cache, other);
+ if (atomic_dec_and_test(&fclones->fclone_ref))
+ kmem_cache_free(skbuff_fclone_cache, fclones);
break;
}
}
@@ -859,17 +858,18 @@ EXPORT_SYMBOL_GPL(skb_copy_ubufs);
struct sk_buff *skb_clone(struct sk_buff *skb, gfp_t gfp_mask)
{
- struct sk_buff *n;
+ struct sk_buff_fclones *fclones = container_of(skb,
+ struct sk_buff_fclones,
+ skb1);
+ struct sk_buff *n = &fclones->skb2;
if (skb_orphan_frags(skb, gfp_mask))
return NULL;
- n = skb + 1;
if (skb->fclone == SKB_FCLONE_ORIG &&
n->fclone == SKB_FCLONE_UNAVAILABLE) {
- atomic_t *fclone_ref = (atomic_t *) (n + 1);
n->fclone = SKB_FCLONE_CLONE;
- atomic_inc(fclone_ref);
+ atomic_inc(&fclones->fclone_ref);
} else {
if (skb_pfmemalloc(skb))
gfp_mask |= __GFP_MEMALLOC;
@@ -3240,8 +3240,7 @@ void __init skb_init(void)
SLAB_HWCACHE_ALIGN|SLAB_PANIC,
NULL);
skbuff_fclone_cache = kmem_cache_create("skbuff_fclone_cache",
- (2*sizeof(struct sk_buff)) +
- sizeof(atomic_t),
+ sizeof(struct sk_buff_fclones),
0,
SLAB_HWCACHE_ALIGN|SLAB_PANIC,
NULL);
diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
index ee567e9e98c3..8d4eac793700 100644
--- a/net/ipv4/tcp_output.c
+++ b/net/ipv4/tcp_output.c
@@ -2110,10 +2110,7 @@ bool tcp_schedule_loss_probe(struct sock *sk)
static bool skb_still_in_host_queue(const struct sock *sk,
const struct sk_buff *skb)
{
- const struct sk_buff *fclone = skb + 1;
-
- if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
- fclone->fclone == SKB_FCLONE_CLONE)) {
+ if (unlikely(skb_fclone_busy(skb))) {
NET_INC_STATS_BH(sock_net(sk),
LINUX_MIB_TCPSPURIOUS_RTX_HOSTQUEUES);
return true;
diff --git a/net/xfrm/xfrm_policy.c b/net/xfrm/xfrm_policy.c
index f623dca6ce30..4c4e457e7888 100644
--- a/net/xfrm/xfrm_policy.c
+++ b/net/xfrm/xfrm_policy.c
@@ -1961,10 +1961,8 @@ static int xdst_queue_output(struct sock *sk, struct sk_buff *skb)
struct xfrm_dst *xdst = (struct xfrm_dst *) dst;
struct xfrm_policy *pol = xdst->pols[0];
struct xfrm_policy_queue *pq = &pol->polq;
- const struct sk_buff *fclone = skb + 1;
- if (unlikely(skb->fclone == SKB_FCLONE_ORIG &&
- fclone->fclone == SKB_FCLONE_CLONE)) {
+ if (unlikely(skb_fclone_busy(skb))) {
kfree_skb(skb);
return 0;
}
^ permalink raw reply related
* Re: [PATCHv8 net-next 2/4] sunvnet: make transmit path zero-copy in the kernel
From: David Miller @ 2014-09-29 20:29 UTC (permalink / raw)
To: david.stevens; +Cc: netdev, sowmini.varadhan, raghuram.kothakota
In-Reply-To: <5429B8E2.40204@oracle.com>
From: David L Stevens <david.stevens@oracle.com>
Date: Mon, 29 Sep 2014 15:54:10 -0400
> This patch removes pre-allocated transmit buffers and instead directly maps
> pending packets on demand. This saves O(n^2) maximum-sized transmit buffers,
> for n hosts on a vswitch, as well as a copy to those buffers.
>
> Single-stream TCP throughput linux-solaris dropped ~5% for 1500-byte MTU,
> but linux-linux at 1500-bytes increased ~20%.
>
> Signed-off-by: David L Stevens <david.stevens@oracle.com>
It doesn't work to liberate SKBs in the TX ring purely from the
->ndo_start_xmit() method.
All SKBs given to a device must be liberated in a finite, short,
amount of time.
This means that there must be an event which indicates TX completion
(either precisely, or at some small finite amount of time afterwards)
which will trigger kfree_skb().
Otherwise you can get a set of TX skbs in the TX queue, then if the
network goes quiet they are all stuck there indefinitely.
These SKBS hold onto resources such as sockets, netfilter state, etc.
Even if you apply a sledgehammer and skb_orphan() these packets, that
doesn't release the netfilter and other pieces of state.
^ permalink raw reply
* Re: [PATCH v2] net: stmmac: fix stmmac_pci_probe failed when CONFIG_HAVE_CLK is selected
From: David Miller @ 2014-09-29 20:37 UTC (permalink / raw)
To: hock.leong.kweh
Cc: peppe.cavallaro, rayagond, vbridgers2013, wens, netdev,
linux-kernel, boon.leong.ong, tobias.johannes.klausmann
In-Reply-To: <1411738975-20056-1-git-send-email-hock.leong.kweh@intel.com>
From: Kweh Hock Leong <hock.leong.kweh@intel.com>
Date: Fri, 26 Sep 2014 21:42:55 +0800
> From: "Kweh, Hock Leong" <hock.leong.kweh@intel.com>
>
> When the CONFIG_HAVE_CLK is selected for the system, the stmmac_pci_probe
> will fail with dmesg:
> [ 2.167225] stmmaceth 0000:00:14.6: enabling device (0000 -> 0002)
> [ 2.178267] stmmaceth 0000:00:14.6: enabling bus mastering
> [ 2.178436] stmmaceth 0000:00:14.6: irq 24 for MSI/MSI-X
> [ 2.178703] stmmaceth 0000:00:14.6: stmmac_dvr_probe: warning: cannot
> get CSR clock
> [ 2.186503] stmmac_pci_probe: main driver probe failed
> [ 2.194003] stmmaceth 0000:00:14.6: disabling bus mastering
> [ 2.196473] stmmaceth: probe of 0000:00:14.6 failed with error -2
>
> This patch fix the issue by breaking the dependency to devm_clk_get()
> as the CSR clock can be obtained at priv->plat->clk_csr from pci driver.
>
> Reported-by: Tobias Klausmann <tobias.johannes.klausmann@mni.thm.de>
> Signed-off-by: Kweh, Hock Leong <hock.leong.kweh@intel.com>
Applied, thank you.
^ permalink raw reply
* Re: IPv6 FIB related crash with MACVLANs in 3.9.11+ kernel.
From: Cong Wang @ 2014-09-29 20:39 UTC (permalink / raw)
To: Ben Greear; +Cc: Hannes Frederic Sowa, Hongmei Li, netdev
In-Reply-To: <5429B783.20207@candelatech.com>
On Mon, Sep 29, 2014 at 12:48 PM, Ben Greear <greearb@candelatech.com> wrote:
>
> We are going to be running up to 20 concurrent scripts that
> will be dumping routes & ips, and configuring ip addresses
> and routes during bringup.
>
> I don't have a simple stand-alone way to reproduce this,
> but at least when we reported the problem is was very easy
> for us to reproduce with our tool.
>
> Maybe 20 scripts running in parallel that randomly configured and dumped routes
> and ip addresses on random interfaces would do the trick?
>
I think the most important question is why is this related with macvlan?
IPv6 is L3 while macvlan pure L2, if this is a IPv6 routing bug, it should
not be limited to macvlan.
What does your IPv6 routing table look like? And how do you configure those
macvlan interfaces?
^ permalink raw reply
* Re: [PATCH net-next 0/5] udp: Generalize GSO for UDP tunnels
From: David Miller @ 2014-09-29 20:43 UTC (permalink / raw)
To: therbert; +Cc: netdev
In-Reply-To: <1411748554-7346-1-git-send-email-therbert@google.com>
From: Tom Herbert <therbert@google.com>
Date: Fri, 26 Sep 2014 09:22:29 -0700
> This patch set generalizes the UDP tunnel segmentation functions so
> that they can work with various protocol encapsulations. The primary
> change is to set the inner_protocol field in the skbuff when creating
> the encapsulated packet, and then in skb_udp_tunnel_segment this data
> is used to determine the function for segmenting the encapsulated
> packet. The inner_protocol field is overloaded to take either an
> Ethertype or IP protocol.
Tom, this series needs to be respun due to Eric Dumazet's sk_buff flags
rework in net-next.
Thanks.
^ permalink raw reply
* Re: [PATCHv8 net-next 2/4] sunvnet: make transmit path zero-copy in the kernel
From: David L Stevens @ 2014-09-29 20:44 UTC (permalink / raw)
To: David Miller; +Cc: netdev, sowmini.varadhan, raghuram.kothakota
In-Reply-To: <20140929.162950.1960056644564225055.davem@davemloft.net>
On 09/29/2014 04:29 PM, David Miller wrote:
>
> It doesn't work to liberate SKBs in the TX ring purely from the
> ->ndo_start_xmit() method.
>
> All SKBs given to a device must be liberated in a finite, short,
> amount of time.
I did consider putting a garbage-collector via timer on them, since
we got such a boost from not ACKing every packet. I guess the question
is "how short?"
For example, I could leave the "normal" path like this and just start/mod
a timer to do it after 1 sec if we haven't done it through start_xmit. Do
you think that's sufficiently short?
+-DLS
^ permalink raw reply
* [PATCH iproute2] tests: Add runtime generated files to .gitignore
From: Vadim Kochan @ 2014-09-29 20:35 UTC (permalink / raw)
To: netdev; +Cc: Vadim Kochan
When make tests then 2 folders are generated:
testsuite/results
testsuite/iproute2/iproute2-this
Signed-off-by: Vadim Kochan <vadim4j@gmail.com>
---
.gitignore | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/.gitignore b/.gitignore
index 3ba2632..5d1e189 100644
--- a/.gitignore
+++ b/.gitignore
@@ -31,3 +31,7 @@ series
.gdbinit
.gdb_history
*.gdb
+
+# tests
+testsuite/results
+testsuite/iproute2/iproute2-this
--
2.1.0
^ permalink raw reply related
* Re: [PATCH] net: llc: check for device before dereferencing
From: David Miller @ 2014-09-29 20:45 UTC (permalink / raw)
To: sasha.levin; +Cc: acme, netdev, linux-kernel
In-Reply-To: <1411782103-3059-1-git-send-email-sasha.levin@oracle.com>
From: Sasha Levin <sasha.levin@oracle.com>
Date: Fri, 26 Sep 2014 21:41:43 -0400
> llc_ui_sendmsg would not make sure that a device indeed exists before
> dereferencing it. This caused a user triggerable NULL ptr deref:
...
> Signed-off-by: Sasha Levin <sasha.levin@oracle.com>
The bug is real, but I don't think the fix is in the correct spot.
The real problem seems to be that the guard for calling
llc_ui_autobind() is inaccurate.
Fix that and there will always be a proper llc->dev attached at
this location.
^ permalink raw reply
* Re: [PATCH] net: Add ndo_gso_check
From: Tom Herbert @ 2014-09-29 20:47 UTC (permalink / raw)
To: Jesse Gross; +Cc: David Miller, netdev, Or Gerlitz
In-Reply-To: <CAEP_g=-bGgu8QqtvcEDj=txbmvoQgKtcJxrmzX3DR7xB05c0RQ@mail.gmail.com>
On Mon, Sep 29, 2014 at 1:13 PM, Jesse Gross <jesse@nicira.com> wrote:
> On Sun, Sep 28, 2014 at 8:50 PM, Tom Herbert <therbert@google.com> wrote:
>> Add ndo_gso_check which a device can define to indicate whether is
>> is capable of doing GSO on a packet. This funciton would be called from
>> the stack to determine whether software GSO is needed to be done. A
>> driver should populate this function if it advertises GSO types for
>> which there are combinations that it wouldn't be able to handle. For
>> instance a device that performs UDP tunneling might only implement
>> support for transparent Ethernet bridging type of inner packets
>> or might have limitations on lengths of inner headers.
>>
>> Signed-off-by: Tom Herbert <therbert@google.com>
>
> Offloads that might have limitations extend beyond GSO - checksum is
> another possibility that could need something like a length check.
>
There are several examples of drivers that advertise checksum offload
but then in TX path decide that they can't do it for various reasons--
in this case they need to do software checksum calculation themselves.
Applying this model to GSO, that is drivers do software GSO when they
need to punt on packet, seems pretty hard to me, but maybe someone can
look at it.
> In addition, while I would also like to be optimistic about the
> capabilities of existing NICs it's unlikely that any of them that are
> advertising SKB_GSO_UDP_TUNNEL can actually do it with full
> generality. So unless we can get the driver writers to chime in about
> their capabilities (maybe we can, there's only a handful of them right
> now), we probably need to provide a more conservative implementation
> for those drivers. I guess I would probably do length equal to VXLAN
> and perhaps containing Ethernet as a balance between the most
> conservative and the most optimistic.
Checking header length in ndo_gso_check would be trivial.
^ permalink raw reply
* Re: [PATCH net] ipv6: remove rt6i_genid
From: David Miller @ 2014-09-29 20:51 UTC (permalink / raw)
To: hannes; +Cc: netdev, eric.dumazet, hideaki, vyasevich, nicolas.dichtel, kafai
In-Reply-To: <7e1b4e2dbe855fa461a4e87e0482ac16b6982c2f.1411857958.git.hannes@stressinduktion.org>
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
Date: Sun, 28 Sep 2014 00:46:06 +0200
> Eric Dumazet noticed that all no-nonexthop or no-gateway routes which
> are already marked DST_HOST (e.g. input routes routes) will always be
> invalidated during sk_dst_check. Thus per-socket dst caching absolutely
> had no effect and early demuxing had no effect.
>
> Thus this patch removes rt6i_genid: fn_sernum already gets modified during
> add operations, so we only must ensure we mutate fn_sernum during ipv6
> address remove operations. This is a fairly cost extensive operations,
> but address removal should not happen that often. Also our mtu update
> functions do the same and we heard no complains so far. xfrm policy
> changes also cause a call into fib6_flush_trees. Also plug a hole in
> rt6_info (no cacheline changes).
>
> I verified via tracing that this change has effect.
>
> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
Ok, I'll apply this because it does correctly and simply fix the
problem now, thanks.
I'll also queue it up for -stable.
However, longer-term:
> static inline void rt_genid_bump_ipv6(struct net *net)
> {
> + if (__fib6_flush_trees)
> + __fib6_flush_trees(net);
> }
I'd really like to see this go away.
^ permalink raw reply
* Re: [PATCH] net: Add ndo_gso_check
From: Tom Herbert @ 2014-09-29 20:53 UTC (permalink / raw)
To: Or Gerlitz
Cc: Jeff Kirsher, Alexander Duyck, David Miller, Linux Netdev List,
Thomas Graf, Pravin Shelar, John Fastabend
In-Reply-To: <CAJ3xEMi-TRXevenx87UM7JnKqRQQATW4MBjbz8JdnQE9gk3KZw@mail.gmail.com>
On Mon, Sep 29, 2014 at 12:59 PM, Or Gerlitz <gerlitz.or@gmail.com> wrote:
> On Mon, Sep 29, 2014 at 6:50 AM, Tom Herbert <therbert@google.com> wrote:
>> Add ndo_gso_check which a device can define to indicate whether is
>> is capable of doing GSO on a packet. This funciton would be called from
>> the stack to determine whether software GSO is needed to be done.
>
> please, no...
>
> We should strive to have a model/architecture under which the driver
> can clearly advertize up something (bits, collection of bits,
> whatever) which the stack can run through some dec-sion making code
> and decide if to GSO yes/no, do it ala SW or ala HW. As life, this
> model need not be perfect and can be biased towards being
> simple/robust/conservative and developed incrementally.
>
Please make a specific proposal then.
> We certainly don't want each driver that support some sort of HW
> offloads for tunnels (today we have 4-5, tomorrow maybe 40...) to
> implement their super sophisticated/efficient "dig in a packet and
> tell myself if I can GSO/CSUM it or not" code, while repeating (expect
> many copy/paste bugs...) the other drivers' code.
>
I would encourage vendors to implement protocol agnostic, generic
mechanisms so that they don't need to dig into the packet.
> Saying that, while looking quickly on the fm10k driver during it's
> super fast upstream review cycle, I saw something which looked like
> yellow light, I drove one, but now I see it was very much a red one:
> the chain of calls:
>
> fm10k_xmit_frame_ring --> fm10k_tso -->
> fm10k_tx_encap_offload --> {fm10k_port_is_vxlan, fm10k_port_is_nvgre}
>
> is pretty much live example to what you are describing here, right?
>
> L2 Drivers are not supposed to do such heavy duty lookups in packets
> with tons of code that can be generic.
>
> I would suggest as band aid to the current situation with non-VXLAN
> UDP offloads enabled upstream and few drivers that don't really
> support GSO/CSUM for udp tunnels which are not vxlan -- to have a
> VXLAN bit which tells to the stack "I can do HW GSO to VXLAN" and have
> the stack to enable HW GSO over these devices only to VXLAN. This
> would bring the upstream code into consistent/well-defined state, and
> we can take it from there.
>
> Or.
>
> SB another comment
>
>> A driver should populate this function if it advertises GSO types for
>> which there are combinations that it wouldn't be able to handle. For
>> instance a device that performs UDP tunneling might only implement
>> support for transparent Ethernet bridging type of inner packets
>> or might have limitations on lengths of inner headers.
>>
>> Signed-off-by: Tom Herbert <therbert@google.com>
>> ---
>> include/linux/netdevice.h | 12 +++++++++++-
>> net/core/dev.c | 2 +-
>> 2 files changed, 12 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>> index 9f5d293..f8c2027 100644
>> --- a/include/linux/netdevice.h
>> +++ b/include/linux/netdevice.h
>> @@ -997,6 +997,12 @@ typedef u16 (*select_queue_fallback_t)(struct net_device *dev,
>> * Callback to use for xmit over the accelerated station. This
>> * is used in place of ndo_start_xmit on accelerated net
>> * devices.
>> + * bool (*ndo_gso_check) (struct sk_buff *skb,
>> + * struct net_device *dev);
>> + * Called by core transmit path to determine if device is capable of
>> + * performing GSO on a packet. The device returns true if it is
>> + * able to GSO the packet, false otherwise. If the return value is
>> + * false the stack will do software GSO.
>> */
>> struct net_device_ops {
>> int (*ndo_init)(struct net_device *dev);
>> @@ -1146,6 +1152,8 @@ struct net_device_ops {
>> struct net_device *dev,
>> void *priv);
>> int (*ndo_get_lock_subclass)(struct net_device *dev);
>> + bool (*ndo_gso_check) (struct sk_buff *skb,
>> + struct net_device *dev);
>> };
>>
>> /**
>> @@ -3536,10 +3544,12 @@ static inline bool skb_gso_ok(struct sk_buff *skb, netdev_features_t features)
>> (!skb_has_frag_list(skb) || (features & NETIF_F_FRAGLIST));
>> }
>>
>> -static inline bool netif_needs_gso(struct sk_buff *skb,
>> +static inline bool netif_needs_gso(struct net_device *dev, struct sk_buff *skb,
>> netdev_features_t features)
>> {
>> return skb_is_gso(skb) && (!skb_gso_ok(skb, features) ||
>> + (dev->netdev_ops->ndo_gso_check &&
>> + !dev->netdev_ops->ndo_gso_check(skb, dev)) ||
>> unlikely((skb->ip_summed != CHECKSUM_PARTIAL) &&
>> (skb->ip_summed != CHECKSUM_UNNECESSARY)));
>> }
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index e2ced01..8c2b9bb 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -2680,7 +2680,7 @@ struct sk_buff *validate_xmit_skb(struct sk_buff *skb, struct net_device *dev)
>> if (skb->encapsulation)
>> features &= dev->hw_enc_features;
>>
>> - if (netif_needs_gso(skb, features)) {
>> + if (netif_needs_gso(dev, skb, features)) {
>> struct sk_buff *segs;
>>
>> segs = skb_gso_segment(skb, features);
>
> unrelated fix? best to put in a different patch.
^ permalink raw reply
* Re: [PATCHv8 net-next 2/4] sunvnet: make transmit path zero-copy in the kernel
From: David Miller @ 2014-09-29 20:54 UTC (permalink / raw)
To: david.stevens; +Cc: netdev, sowmini.varadhan, raghuram.kothakota
In-Reply-To: <5429C498.1000705@oracle.com>
From: David L Stevens <david.stevens@oracle.com>
Date: Mon, 29 Sep 2014 16:44:08 -0400
> On 09/29/2014 04:29 PM, David Miller wrote:
>
>>
>> It doesn't work to liberate SKBs in the TX ring purely from the
>> ->ndo_start_xmit() method.
>>
>> All SKBs given to a device must be liberated in a finite, short,
>> amount of time.
>
> I did consider putting a garbage-collector via timer on them, since
> we got such a boost from not ACKing every packet. I guess the question
> is "how short?"
>
> For example, I could leave the "normal" path like this and just start/mod
> a timer to do it after 1 sec if we haven't done it through start_xmit. Do
> you think that's sufficiently short?
To be honest I'm not %100 sure.
SKB liberation frees up space in the TCP socket send buffer, so... but
arguably if the connection actually cares and is in steady state then
we'll be liberating via the ->ndo_start_xmit() flush you do here.
^ 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