* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Al Viro @ 2019-07-01 21:51 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190701214649.GE17978@ZenIV.linux.org.uk>
On Mon, Jul 01, 2019 at 10:46:49PM +0100, Al Viro wrote:
> already done cpu_to_le32 to the containing 32bit word. So the
le32_to_cpu, sorry
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Al Viro @ 2019-07-01 21:46 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190701211356.GD17978@ZenIV.linux.org.uk>
On Mon, Jul 01, 2019 at 10:13:56PM +0100, Al Viro wrote:
> On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
>
> > > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > > third argument treats it as a host-endian integer. So... Has anyone
> > > tested that code on b-e host? Should that ntohs() actually be swab16(),
> > > yielding (on any host) the same value we currently get for l-e hosts only?
> > >
> > I haven't seen any b-e host with a Realtek network chip yet.
>
> Ever tried to google for realtek 8169 pcie card? The first hit is this:
> https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
> and certainly does look like it should fit into at least some G5 Macs.
> What's more, googling for realtek 8169 PCI card brings quite a bit (top
> hit happens to be on ebay for ~$8). That certainly shall fit into
> any number of big-endian motherboards...
>
> Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
> but these beasts do exist on discrete cards. I'm fairly certain that I've
> got one or two somewhere in the detritus pile and they are fairly cheap
> these days.
>
> So it wouldn't cost too much to put together a mixed network, with
> r8169 both on l-e and b-e hosts and play with VLAN setups there...
FWIW, looking at r8169 docs, they say that Rx descriptor has, at offset
4, a 32bit value (l-e) with
bits 17..31 reserved
bit 16: TAVA (Tag Available)
bits 8..15: VIDL (lower 8 bits of VLAN ID)
bits 5..7: PRIO (priority)
bit 4: CFI (Canonical Format Indicator)
bits 0..3: VIDH (upper 4 bits of VLAN ID)
AFAICS, in kernel-side representation we want VIDL in bits 0..7, VIDH - 8..11,
CFI - 12 and PRIO - 13..15, so that VLAN ID is simply value & 0xfff.
IOW, we do want 256 * octet4 + octet5, i.e.
swab16(le32_to_cpu(desc->opts2) & 0xffff).
Regardless of the host endianness. Some cards might be possible to set up to
byteswap the 32bit values on the way to/from host, but even in that case we
would have wanted
swab16(desc->opts2 & 0xffff),
not
be16_to_cpu(le32_to_cpu(desc->opts2 & 0xffff)
and if it *is* set that way, the current code will break anyway.
Again, all of that is from RTFS alone - I have _not_ tested that, but
it does look like your original patch has missed that while these
16 bits are stored in network order _in_ _card_ _memory_, we'd
already done cpu_to_le32 to the containing 32bit word. So the
value is host-endian, and the lower 16 bits are consistently in
the wrong order, be it b-e or l-e host. IOW, swab16 is the right
thing to do and sparse warning has been correct.
^ permalink raw reply
* Re: [PATCHv3 next 2/3] blackhole_netdev: use blackhole_netdev to invalidate dst entries
From: Michael Chan @ 2019-07-01 21:42 UTC (permalink / raw)
To: Mahesh Bandewar
Cc: Netdev, Eric Dumazet, David Miller, Daniel Axtens,
Mahesh Bandewar
In-Reply-To: <20190701213857.103511-1-maheshb@google.com>
On Mon, Jul 1, 2019 at 2:39 PM Mahesh Bandewar <maheshb@google.com> wrote:
>
> Use blackhole_netdev instead of 'lo' device with lower MTU when marking
> dst "dead".
>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Tested-by: Michael Chan <michael.chan@broadcom.com>
^ permalink raw reply
* Re: [PATCHv2 next 3/3] blackhole_dev: add a selftest
From: Mahesh Bandewar (महेश बंडेवार) @ 2019-07-01 21:40 UTC (permalink / raw)
To: David Miller; +Cc: linux-netdev, Eric Dumazet, michael.chan, dja, mahesh
In-Reply-To: <20190629.122847.986644252948714439.davem@davemloft.net>
On Sat, Jun 29, 2019 at 12:28 PM David Miller <davem@davemloft.net> wrote:
>
> From: Mahesh Bandewar <maheshb@google.com>
> Date: Thu, 27 Jun 2019 12:43:09 -0700
>
> > +config TEST_BLACKHOLE_DEV
> > + tristate "Test BPF filter functionality"
>
> I think the tristate string needs to be changed :-)
side effects of copy-paste :(
sending v3
^ permalink raw reply
* [PATCHv3 next 3/3] blackhole_dev: add a selftest
From: Mahesh Bandewar @ 2019-07-01 21:39 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
Since this is not really a device with all capabilities, this test
ensures that it has *enough* to make it through the data path
without causing unwanted side-effects (read crash!).
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1 -> v2
fixed the conflict resolution in selftests Makefile
v2 -> v3
fixed lib/Kconfig.debug tristate text / string.
lib/Kconfig.debug | 9 ++
lib/Makefile | 1 +
lib/test_blackhole_dev.c | 100 ++++++++++++++++++
tools/testing/selftests/net/Makefile | 2 +-
tools/testing/selftests/net/config | 1 +
.../selftests/net/test_blackhole_dev.sh | 11 ++
6 files changed, 123 insertions(+), 1 deletion(-)
create mode 100644 lib/test_blackhole_dev.c
create mode 100755 tools/testing/selftests/net/test_blackhole_dev.sh
diff --git a/lib/Kconfig.debug b/lib/Kconfig.debug
index cbdfae379896..99272b5dd980 100644
--- a/lib/Kconfig.debug
+++ b/lib/Kconfig.debug
@@ -1909,6 +1909,15 @@ config TEST_BPF
If unsure, say N.
+config TEST_BLACKHOLE_DEV
+ tristate "Test blackhole netdev functionality"
+ depends on m && NET
+ help
+ This builds the "test_blackhole_dev" module that validates the
+ data path through this blackhole netdev.
+
+ If unsure, say N.
+
config FIND_BIT_BENCHMARK
tristate "Test find_bit functions"
help
diff --git a/lib/Makefile b/lib/Makefile
index dcb558c7554d..6ac44fe2a37f 100644
--- a/lib/Makefile
+++ b/lib/Makefile
@@ -91,6 +91,7 @@ obj-$(CONFIG_TEST_DEBUG_VIRTUAL) += test_debug_virtual.o
obj-$(CONFIG_TEST_MEMCAT_P) += test_memcat_p.o
obj-$(CONFIG_TEST_OBJAGG) += test_objagg.o
obj-$(CONFIG_TEST_STACKINIT) += test_stackinit.o
+obj-$(CONFIG_TEST_BLACKHOLE_DEV) += test_blackhole_dev.o
obj-$(CONFIG_TEST_LIVEPATCH) += livepatch/
diff --git a/lib/test_blackhole_dev.c b/lib/test_blackhole_dev.c
new file mode 100644
index 000000000000..4c40580a99a3
--- /dev/null
+++ b/lib/test_blackhole_dev.c
@@ -0,0 +1,100 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * This module tests the blackhole_dev that is created during the
+ * net subsystem initialization. The test this module performs is
+ * by injecting an skb into the stack with skb->dev as the
+ * blackhole_dev and expects kernel to behave in a sane manner
+ * (in other words, *not crash*)!
+ *
+ * Copyright (c) 2018, Mahesh Bandewar <maheshb@google.com>
+ */
+
+#include <linux/init.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+#include <linux/skbuff.h>
+#include <linux/netdevice.h>
+#include <linux/udp.h>
+#include <linux/ipv6.h>
+
+#include <net/dst.h>
+
+#define SKB_SIZE 256
+#define HEAD_SIZE (14+40+8) /* Ether + IPv6 + UDP */
+#define TAIL_SIZE 32 /* random tail-room */
+
+#define UDP_PORT 1234
+
+static int __init test_blackholedev_init(void)
+{
+ struct ipv6hdr *ip6h;
+ struct sk_buff *skb;
+ struct ethhdr *ethh;
+ struct udphdr *uh;
+ int data_len;
+ int ret;
+
+ skb = alloc_skb(SKB_SIZE, GFP_KERNEL);
+ if (!skb)
+ return -ENOMEM;
+
+ /* Reserve head-room for the headers */
+ skb_reserve(skb, HEAD_SIZE);
+
+ /* Add data to the skb */
+ data_len = SKB_SIZE - (HEAD_SIZE + TAIL_SIZE);
+ memset(__skb_put(skb, data_len), 0xf, data_len);
+
+ /* Add protocol data */
+ /* (Transport) UDP */
+ uh = (struct udphdr *)skb_push(skb, sizeof(struct udphdr));
+ skb_set_transport_header(skb, 0);
+ uh->source = uh->dest = htons(UDP_PORT);
+ uh->len = htons(data_len);
+ uh->check = 0;
+ /* (Network) IPv6 */
+ ip6h = (struct ipv6hdr *)skb_push(skb, sizeof(struct ipv6hdr));
+ skb_set_network_header(skb, 0);
+ ip6h->hop_limit = 32;
+ ip6h->payload_len = data_len + sizeof(struct udphdr);
+ ip6h->nexthdr = IPPROTO_UDP;
+ ip6h->saddr = in6addr_loopback;
+ ip6h->daddr = in6addr_loopback;
+ /* Ether */
+ ethh = (struct ethhdr *)skb_push(skb, sizeof(struct ethhdr));
+ skb_set_mac_header(skb, 0);
+
+ skb->protocol = htons(ETH_P_IPV6);
+ skb->pkt_type = PACKET_HOST;
+ skb->dev = blackhole_netdev;
+
+ /* Now attempt to send the packet */
+ ret = dev_queue_xmit(skb);
+
+ switch (ret) {
+ case NET_XMIT_SUCCESS:
+ pr_warn("dev_queue_xmit() returned NET_XMIT_SUCCESS\n");
+ break;
+ case NET_XMIT_DROP:
+ pr_warn("dev_queue_xmit() returned NET_XMIT_DROP\n");
+ break;
+ case NET_XMIT_CN:
+ pr_warn("dev_queue_xmit() returned NET_XMIT_CN\n");
+ break;
+ default:
+ pr_err("dev_queue_xmit() returned UNKNOWN(%d)\n", ret);
+ }
+
+ return 0;
+}
+
+static void __exit test_blackholedev_exit(void)
+{
+ pr_warn("test_blackholedev module terminating.\n");
+}
+
+module_init(test_blackholedev_init);
+module_exit(test_blackholedev_exit);
+
+MODULE_AUTHOR("Mahesh Bandewar <maheshb@google.com>");
+MODULE_LICENSE("GPL");
diff --git a/tools/testing/selftests/net/Makefile b/tools/testing/selftests/net/Makefile
index 9a275d932fd5..1b24e36b4047 100644
--- a/tools/testing/selftests/net/Makefile
+++ b/tools/testing/selftests/net/Makefile
@@ -5,7 +5,7 @@ CFLAGS = -Wall -Wl,--no-as-needed -O2 -g
CFLAGS += -I../../../../usr/include/
TEST_PROGS := run_netsocktests run_afpackettests test_bpf.sh netdevice.sh \
- rtnetlink.sh xfrm_policy.sh
+ rtnetlink.sh xfrm_policy.sh test_blackhole_dev.sh
TEST_PROGS += fib_tests.sh fib-onlink-tests.sh pmtu.sh udpgso.sh ip_defrag.sh
TEST_PROGS += udpgso_bench.sh fib_rule_tests.sh msg_zerocopy.sh psock_snd.sh
TEST_PROGS += udpgro_bench.sh udpgro.sh test_vxlan_under_vrf.sh reuseport_addr_any.sh
diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
index 89f84b5118bf..e4b878d95ba0 100644
--- a/tools/testing/selftests/net/config
+++ b/tools/testing/selftests/net/config
@@ -27,3 +27,4 @@ CONFIG_NFT_CHAIN_NAT_IPV6=m
CONFIG_NFT_CHAIN_NAT_IPV4=m
CONFIG_NET_SCH_FQ=m
CONFIG_NET_SCH_ETF=m
+CONFIG_TEST_BLACKHOLE_DEV=m
diff --git a/tools/testing/selftests/net/test_blackhole_dev.sh b/tools/testing/selftests/net/test_blackhole_dev.sh
new file mode 100755
index 000000000000..3119b80e711f
--- /dev/null
+++ b/tools/testing/selftests/net/test_blackhole_dev.sh
@@ -0,0 +1,11 @@
+#!/bin/sh
+# SPDX-License-Identifier: GPL-2.0
+# Runs blackhole-dev test using blackhole-dev kernel module
+
+if /sbin/modprobe -q test_blackhole_dev ; then
+ /sbin/modprobe -q -r test_blackhole_dev;
+ echo "test_blackhole_dev: ok";
+else
+ echo "test_blackhole_dev: [FAIL]";
+ exit 1;
+fi
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCHv3 next 2/3] blackhole_netdev: use blackhole_netdev to invalidate dst entries
From: Mahesh Bandewar @ 2019-07-01 21:38 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
Use blackhole_netdev instead of 'lo' device with lower MTU when marking
dst "dead".
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1->v2->v3
no change
net/core/dst.c | 2 +-
net/ipv4/route.c | 3 +--
net/ipv6/route.c | 2 +-
3 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/net/core/dst.c b/net/core/dst.c
index e46366228eaf..1325316d9eab 100644
--- a/net/core/dst.c
+++ b/net/core/dst.c
@@ -160,7 +160,7 @@ void dst_dev_put(struct dst_entry *dst)
dst->ops->ifdown(dst, dev, true);
dst->input = dst_discard;
dst->output = dst_discard_out;
- dst->dev = dev_net(dst->dev)->loopback_dev;
+ dst->dev = blackhole_netdev;
dev_hold(dst->dev);
dev_put(dev);
}
diff --git a/net/ipv4/route.c b/net/ipv4/route.c
index bbd55c7f6b2e..dc1f510a7c81 100644
--- a/net/ipv4/route.c
+++ b/net/ipv4/route.c
@@ -1532,7 +1532,6 @@ static void ipv4_dst_destroy(struct dst_entry *dst)
void rt_flush_dev(struct net_device *dev)
{
- struct net *net = dev_net(dev);
struct rtable *rt;
int cpu;
@@ -1543,7 +1542,7 @@ void rt_flush_dev(struct net_device *dev)
list_for_each_entry(rt, &ul->head, rt_uncached) {
if (rt->dst.dev != dev)
continue;
- rt->dst.dev = net->loopback_dev;
+ rt->dst.dev = blackhole_netdev;
dev_hold(rt->dst.dev);
dev_put(dev);
}
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index 7556275b1cef..39361f57351a 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -176,7 +176,7 @@ static void rt6_uncached_list_flush_dev(struct net *net, struct net_device *dev)
}
if (rt_dev == dev) {
- rt->dst.dev = loopback_dev;
+ rt->dst.dev = blackhole_netdev;
dev_hold(rt->dst.dev);
dev_put(rt_dev);
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCHv3 next 1/3] loopback: create blackhole net device similar to loopack.
From: Mahesh Bandewar @ 2019-07-01 21:38 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
Create a blackhole net device that can be used for "dead"
dst entries instead of loopback device. This blackhole device differs
from loopback in few aspects: (a) It's not per-ns. (b) MTU on this
device is ETH_MIN_MTU (c) The xmit function is essentially kfree_skb().
and (d) since it's not registered it won't have ifindex.
Lower MTU effectively make the device not pass the MTU check during
the route check when a dst associated with the skb is dead.
Signed-off-by: Mahesh Bandewar <maheshb@google.com>
---
v1->v2->v3
no change
drivers/net/loopback.c | 76 ++++++++++++++++++++++++++++++++++-----
include/linux/netdevice.h | 2 ++
2 files changed, 69 insertions(+), 9 deletions(-)
diff --git a/drivers/net/loopback.c b/drivers/net/loopback.c
index 87d361666cdd..3b39def5471e 100644
--- a/drivers/net/loopback.c
+++ b/drivers/net/loopback.c
@@ -55,6 +55,13 @@
#include <net/net_namespace.h>
#include <linux/u64_stats_sync.h>
+/* blackhole_netdev - a device used for dsts that are marked expired!
+ * This is global device (instead of per-net-ns) since it's not needed
+ * to be per-ns and gets initialized at boot time.
+ */
+struct net_device *blackhole_netdev;
+EXPORT_SYMBOL(blackhole_netdev);
+
/* The higher levels take care of making this non-reentrant (it's
* called with bh's disabled).
*/
@@ -150,12 +157,14 @@ static const struct net_device_ops loopback_ops = {
.ndo_set_mac_address = eth_mac_addr,
};
-/* The loopback device is special. There is only one instance
- * per network namespace.
- */
-static void loopback_setup(struct net_device *dev)
+static void gen_lo_setup(struct net_device *dev,
+ unsigned int mtu,
+ const struct ethtool_ops *eth_ops,
+ const struct header_ops *hdr_ops,
+ const struct net_device_ops *dev_ops,
+ void (*dev_destructor)(struct net_device *dev))
{
- dev->mtu = 64 * 1024;
+ dev->mtu = mtu;
dev->hard_header_len = ETH_HLEN; /* 14 */
dev->min_header_len = ETH_HLEN; /* 14 */
dev->addr_len = ETH_ALEN; /* 6 */
@@ -174,11 +183,20 @@ static void loopback_setup(struct net_device *dev)
| NETIF_F_NETNS_LOCAL
| NETIF_F_VLAN_CHALLENGED
| NETIF_F_LOOPBACK;
- dev->ethtool_ops = &loopback_ethtool_ops;
- dev->header_ops = ð_header_ops;
- dev->netdev_ops = &loopback_ops;
+ dev->ethtool_ops = eth_ops;
+ dev->header_ops = hdr_ops;
+ dev->netdev_ops = dev_ops;
dev->needs_free_netdev = true;
- dev->priv_destructor = loopback_dev_free;
+ dev->priv_destructor = dev_destructor;
+}
+
+/* The loopback device is special. There is only one instance
+ * per network namespace.
+ */
+static void loopback_setup(struct net_device *dev)
+{
+ gen_lo_setup(dev, (64 * 1024), &loopback_ethtool_ops, ð_header_ops,
+ &loopback_ops, loopback_dev_free);
}
/* Setup and register the loopback device. */
@@ -213,3 +231,43 @@ static __net_init int loopback_net_init(struct net *net)
struct pernet_operations __net_initdata loopback_net_ops = {
.init = loopback_net_init,
};
+
+/* blackhole netdevice */
+static netdev_tx_t blackhole_netdev_xmit(struct sk_buff *skb,
+ struct net_device *dev)
+{
+ kfree_skb(skb);
+ net_warn_ratelimited("%s(): Dropping skb.\n", __func__);
+ return NETDEV_TX_OK;
+}
+
+static const struct net_device_ops blackhole_netdev_ops = {
+ .ndo_start_xmit = blackhole_netdev_xmit,
+};
+
+/* This is a dst-dummy device used specifically for invalidated
+ * DSTs and unlike loopback, this is not per-ns.
+ */
+static void blackhole_netdev_setup(struct net_device *dev)
+{
+ gen_lo_setup(dev, ETH_MIN_MTU, NULL, NULL, &blackhole_netdev_ops, NULL);
+}
+
+/* Setup and register the blackhole_netdev. */
+static int __init blackhole_netdev_init(void)
+{
+ blackhole_netdev = alloc_netdev(0, "blackhole_dev", NET_NAME_UNKNOWN,
+ blackhole_netdev_setup);
+ if (!blackhole_netdev)
+ return -ENOMEM;
+
+ dev_init_scheduler(blackhole_netdev);
+ dev_activate(blackhole_netdev);
+
+ blackhole_netdev->flags |= IFF_UP | IFF_RUNNING;
+ dev_net_set(blackhole_netdev, &init_net);
+
+ return 0;
+}
+
+device_initcall(blackhole_netdev_init);
diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index eeacebd7debb..88292953aa6f 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -4870,4 +4870,6 @@ do { \
#define PTYPE_HASH_SIZE (16)
#define PTYPE_HASH_MASK (PTYPE_HASH_SIZE - 1)
+extern struct net_device *blackhole_netdev;
+
#endif /* _LINUX_NETDEVICE_H */
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCHv3 next 0/3] blackhole device to invalidate dst
From: Mahesh Bandewar @ 2019-07-01 21:38 UTC (permalink / raw)
To: Netdev
Cc: Eric Dumazet, David Miller, Michael Chan, Daniel Axtens,
Mahesh Bandewar, Mahesh Bandewar
When we invalidate dst or mark it "dead", we assign 'lo' to
dst->dev. First of all this assignment is racy and more over,
it has MTU implications.
The standard dev MTU is 1500 while the Loopback MTU is 64k. TCP
code when dereferencing the dst don't check if the dst is valid
or not. TCP when dereferencing a dead-dst while negotiating a
new connection, may use dst device which is 'lo' instead of
using the correct device. Consider the following scenario:
A SYN arrives on an interface and tcp-layer while processing
SYNACK finds a dst and associates it with SYNACK skb. Now before
skb gets passed to L3 for processing, if that dst gets "dead"
(because of the virtual device getting disappeared & then reappeared),
the 'lo' gets assigned to that dst (lo MTU = 64k). Let's assume
the SYN has ADV_MSS set as 9k while the output device through
which this SYNACK is going to go out has standard MTU of 1500.
The MTU check during the route check passes since MIN(9K, 64K)
is 9k and TCP successfully negotiates 9k MSS. The subsequent
data packet; bigger in size gets passed to the device and it
won't be marked as GSO since the assumed MTU of the device is
9k.
This either crashes the NIC and we have seen fixes that went
into drivers to handle this scenario. 8914a595110a ('bnx2x:
disable GSO where gso_size is too big for hardware') and
2b16f048729b ('net: create skb_gso_validate_mac_len()') and
with those fixes TCP eventually recovers but not before
few dropped segments.
Well, I'm not a TCP expert and though we have experienced
these corner cases in our environment, I could not reproduce
this case reliably in my test setup to try this fix myself.
However, Michael Chan <michael.chan@broadcom.com> had a setup
where these fixes helped him mitigate the issue and not cause
the crash.
The idea here is to not alter the data-path with additional
locks or smb()/rmb() barriers to avoid racy assignments but
to create a new device that has really low MTU that has
.ndo_start_xmit essentially a kfree_skb(). Make use of this
device instead of 'lo' when marking the dst dead.
First patch implements the blackhole device and second
patch uses it in IPv4 and IPv6 stack while the third patch
is the self test that ensures the sanity of this device.
v1->v2
fixed the self-test patch to handle the conflict
v2 -> v3
fixed Kconfig text/string.
Mahesh Bandewar (3):
loopback: create blackhole net device similar to loopack.
blackhole_netdev: use blackhole_netdev to invalidate dst entries
blackhole_dev: add a selftest
drivers/net/loopback.c | 76 +++++++++++--
include/linux/netdevice.h | 2 +
lib/Kconfig.debug | 9 ++
lib/Makefile | 1 +
lib/test_blackhole_dev.c | 100 ++++++++++++++++++
net/core/dst.c | 2 +-
net/ipv4/route.c | 3 +-
net/ipv6/route.c | 2 +-
tools/testing/selftests/net/Makefile | 2 +-
tools/testing/selftests/net/config | 1 +
.../selftests/net/test_blackhole_dev.sh | 11 ++
11 files changed, 195 insertions(+), 14 deletions(-)
create mode 100644 lib/test_blackhole_dev.c
create mode 100755 tools/testing/selftests/net/test_blackhole_dev.sh
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Matthias Kaehlcke @ 2019-07-01 21:32 UTC (permalink / raw)
To: Heiner Kallweit
Cc: David S . Miller, Rob Herring, Mark Rutland, Andrew Lunn,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <d2386f7d-b4bc-d983-1b83-cc2aa4aec38b@gmail.com>
On Mon, Jul 01, 2019 at 10:43:12PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> > The RTL8211E has extension pages, which can be accessed after
> > selecting a page through a custom method. Add a function to
> > modify bits in a register of an extension page and a few
> > helpers for dealing with ext pages.
> >
> > rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> > inspired by their counterparts phy_modify_paged() and
> > phy_restore_page().
> >
> > Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> > ---
> > This code might be applicable to other Realtek PHYs, but I don't
> > have access to the datasheets to confirm it, so for now it's just
> > for the RTL8211E.
> >
> This extended page mechanism exists on a number of older Realtek
> PHY's. For most extended pages however Realtek releases no public
> documentation.
> Considering that we use these helpers in one place only, I don't
> really see a need for them.
I see it as self-documenting code, that may be reused, rather than
inline code with comments.
In any case I'm looking into another patch that would write registers
on extented pages rather than doing a modify, if that materializes I
think we would want the helpers.
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Matthias Kaehlcke @ 2019-07-01 21:21 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, David S . Miller, Rob Herring, Mark Rutland,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <35db1bff-f48e-5372-06b7-3140cb7cbb71@gmail.com>
On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 22:02, Andrew Lunn wrote:
> > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> >> The RTL8211E has extension pages, which can be accessed after
> >> selecting a page through a custom method. Add a function to
> >> modify bits in a register of an extension page and a few
> >> helpers for dealing with ext pages.
> >>
> >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> >> inspired by their counterparts phy_modify_paged() and
> >> phy_restore_page().
> >
> > Hi Matthias
> >
> > While an extended page is selected, what happens to the normal
> > registers in the range 0-0x1c? Are they still accessible?
> >
> AFAIK: no
From my observations it looks like registers 0x00 to 0x0f are still
accessible, but not the ones above. IIUC 0x00-0x0f are standard
registers, the others are vendor specific.
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jakub Kicinski @ 2019-07-01 21:20 UTC (permalink / raw)
To: Laatz, Kevin
Cc: Jonathan Lemon, netdev, ast, daniel, bjorn.topel, magnus.karlsson,
bpf, intel-wired-lan, bruce.richardson, ciara.loftus
In-Reply-To: <07e404eb-f712-b15a-4884-315aff3f7c7d@intel.com>
On Mon, 1 Jul 2019 15:44:29 +0100, Laatz, Kevin wrote:
> On 28/06/2019 21:29, Jonathan Lemon wrote:
> > On 28 Jun 2019, at 9:19, Laatz, Kevin wrote:
> >> On 27/06/2019 22:25, Jakub Kicinski wrote:
> >>> I think that's very limiting. What is the challenge in providing
> >>> aligned addresses, exactly?
> >> The challenges are two-fold:
> >> 1) it prevents using arbitrary buffer sizes, which will be an issue
> >> supporting e.g. jumbo frames in future.
> >> 2) higher level user-space frameworks which may want to use AF_XDP,
> >> such as DPDK, do not currently support having buffers with 'fixed'
> >> alignment.
> >> The reason that DPDK uses arbitrary placement is that:
> >> - it would stop things working on certain NICs which need the
> >> actual writable space specified in units of 1k - therefore we need 2k
> >> + metadata space.
> >> - we place padding between buffers to avoid constantly
> >> hitting the same memory channels when accessing memory.
> >> - it allows the application to choose the actual buffer size
> >> it wants to use.
> >> We make use of the above to allow us to speed up processing
> >> significantly and also reduce the packet buffer memory size.
> >>
> >> Not having arbitrary buffer alignment also means an AF_XDP driver
> >> for DPDK cannot be a drop-in replacement for existing drivers in
> >> those frameworks. Even with a new capability to allow an arbitrary
> >> buffer alignment, existing apps will need to be modified to use that
> >> new capability.
> >
> > Since all buffers in the umem are the same chunk size, the original
> > buffer
> > address can be recalculated with some multiply/shift math. However,
> > this is
> > more expensive than just a mask operation.
>
> Yes, we can do this.
That'd be best, can DPDK reasonably guarantee the slicing is uniform?
E.g. it's not desperate buffer pools with different bases?
> Another option we have is to add a socket option for querying the
> metadata length from the driver (assuming it doesn't vary per packet).
> We can use that information to get back to the original address using
> subtraction.
Unfortunately the metadata depends on the packet and how much info
the device was able to extract. So it's variable length.
> Alternatively, we can change the Rx descriptor format to include the
> metadata length. We could do this in a couple of ways, for example,
> rather than returning the address as the start of the packet, instead
> return the buffer address that was passed in, and adding another 16-bit
> field to specify the start of packet offset with that buffer. If using
> another 16-bits of the descriptor space is not desirable, an alternative
> could be to limit umem sizes to e.g. 2^48 bits (256 terabytes should be
> enough, right :-) ) and use the remaining 16 bits of the address as a
> packet offset. Other variations on these approach are obviously possible
> too.
Seems reasonable to me..
^ permalink raw reply
* Re: [PATCH bpf-next 0/8] bpf: TCP RTT sock_ops bpf callback
From: Yuchung Cheng @ 2019-07-01 21:15 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, bpf, David Miller, ast, Daniel Borkmann, Eric Dumazet,
Priyaranjan Jha, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
On Mon, Jul 1, 2019 at 1:48 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Congestion control team would like to have a periodic callback to
> track some TCP statistics. Let's add a sock_ops callback that can be
> selectively enabled on a socket by socket basis and is executed for
> every RTT. BPF program frequency can be further controlled by calling
> bpf_ktime_get_ns and bailing out early.
>
> I run neper tcp_stream and tcp_rr tests with the sample program
> from the last patch and didn't observe any noticeable performance
> difference.
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Priyaranjan Jha <priyarjha@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Thanks!
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
>
> Stanislav Fomichev (8):
> bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
> bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
> bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
> bpf: add icsk_retransmits to bpf_tcp_sock
> bpf/tools: sync bpf.h
> selftests/bpf: test BPF_SOCK_OPS_RTT_CB
> samples/bpf: add sample program that periodically dumps TCP stats
> samples/bpf: fix tcp_bpf.readme detach command
>
> include/net/tcp.h | 8 +
> include/uapi/linux/bpf.h | 12 +-
> net/core/filter.c | 207 +++++++++++-----
> net/ipv4/tcp_input.c | 4 +
> samples/bpf/Makefile | 1 +
> samples/bpf/tcp_bpf.readme | 2 +-
> samples/bpf/tcp_dumpstats_kern.c | 65 +++++
> tools/include/uapi/linux/bpf.h | 12 +-
> tools/testing/selftests/bpf/Makefile | 3 +-
> tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
> tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
> 11 files changed, 570 insertions(+), 58 deletions(-)
> create mode 100644 samples/bpf/tcp_dumpstats_kern.c
> create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
> create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Al Viro @ 2019-07-01 21:13 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <81c45b3c-bbaa-c619-981c-8b8f4b73d5c5@gmail.com>
On Mon, Jul 01, 2019 at 10:36:26PM +0200, Heiner Kallweit wrote:
> > The code dealing with the value passed to __vlan_hwaccel_put_tag() as the
> > third argument treats it as a host-endian integer. So... Has anyone
> > tested that code on b-e host? Should that ntohs() actually be swab16(),
> > yielding (on any host) the same value we currently get for l-e hosts only?
> >
> I haven't seen any b-e host with a Realtek network chip yet.
Ever tried to google for realtek 8169 pcie card? The first hit is this:
https://www.amazon.com/Realtek-Chipset-Ethernet-Interface-Software/dp/B007MWYCG2
and certainly does look like it should fit into at least some G5 Macs.
What's more, googling for realtek 8169 PCI card brings quite a bit (top
hit happens to be on ebay for ~$8). That certainly shall fit into
any number of big-endian motherboards...
Sure, there's a plenty of embedded r8169 on motherboards (mostly x86 ones),
but these beasts do exist on discrete cards. I'm fairly certain that I've
got one or two somewhere in the detritus pile and they are fairly cheap
these days.
So it wouldn't cost too much to put together a mixed network, with
r8169 both on l-e and b-e hosts and play with VLAN setups there...
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Andrew Lunn @ 2019-07-01 21:09 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Matthias Kaehlcke, David S . Miller, Rob Herring, Mark Rutland,
Florian Fainelli, netdev, devicetree, linux-kernel,
Douglas Anderson
In-Reply-To: <35db1bff-f48e-5372-06b7-3140cb7cbb71@gmail.com>
On Mon, Jul 01, 2019 at 10:37:16PM +0200, Heiner Kallweit wrote:
> On 01.07.2019 22:02, Andrew Lunn wrote:
> > On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
> >> The RTL8211E has extension pages, which can be accessed after
> >> selecting a page through a custom method. Add a function to
> >> modify bits in a register of an extension page and a few
> >> helpers for dealing with ext pages.
> >>
> >> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> >> inspired by their counterparts phy_modify_paged() and
> >> phy_restore_page().
> >
> > Hi Matthias
> >
> > While an extended page is selected, what happens to the normal
> > registers in the range 0-0x1c? Are they still accessible?
> >
> AFAIK: no
This it would be better to make use of the core paged access support,
so that locking is done correctly.
Andrew
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: Gerd Rausch @ 2019-07-01 21:06 UTC (permalink / raw)
To: santosh.shilimkar, netdev; +Cc: David Miller
In-Reply-To: <b5669540-3892-9d79-85ba-79e96ddd3a81@oracle.com>
Hi Santosh,
On 01/07/2019 14.00, santosh.shilimkar@oracle.com wrote:
>>
> Look for command timeout in CX3 sources. 60 second is upper bound in
> CX3. Its not standard in specs(at least not that I know) though
> and may vary from vendor to vendor.
>
I am not seeing it. Can you point me to the right place?
% grep -ni timeout drivers/net/ethernet/mellanox/mlx4/*.[ch]
drivers/net/ethernet/mellanox/mlx4/cmd.c:116: GO_BIT_TIMEOUT_MSECS = 10000
[...]
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:101:#define MLX4_EN_WATCHDOG_TIMEOUT (15 * HZ)
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:155:#define MLX4_EN_TX_POLL_TIMEOUT (HZ / 4)
drivers/net/ethernet/mellanox/mlx4/mlx4_en.h:171:#define MLX4_EN_LOOPBACK_TIMEOUT 100
[...]
drivers/net/ethernet/mellanox/mlx4/reset.c:61:#define MLX4_SEM_TIMEOUT_JIFFIES (10 * HZ)
drivers/net/ethernet/mellanox/mlx4/reset.c:62:#define MLX4_RESET_TIMEOUT_JIFFIES (2 * HZ)
% grep -i timeout drivers/infiniband/hw/mlx4/*.[ch]
drivers/infiniband/hw/mlx4/cm.c:42:#define CM_CLEANUP_CACHE_TIMEOUT (30 * HZ)
[...]
drivers/infiniband/hw/mlx4/mcg.c:46:#define MAD_TIMEOUT_MS 2000
[...]
drivers/infiniband/hw/mlx4/qp.c:4358: while (wait_for_completion_timeout(&sdrain->done, HZ / 10) <= 0)
Thanks,
Gerd
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: Add support for fq's EDT to HBM
From: Yonghong Song @ 2019-07-01 21:04 UTC (permalink / raw)
To: Lawrence Brakmo, netdev
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Eric Dumazet, Kernel Team
In-Reply-To: <20190628194133.2831708-1-brakmo@fb.com>
On 6/28/19 12:41 PM, brakmo wrote:
> Adds support for fq's Earliest Departure Time to HBM (Host Bandwidth
> Manager). Includes a new BPF program supporting EDT, and also updates
> corresponding programs.
>
> It will drop packets with an EDT of more than 500us in the future
> unless the packet belongs to a flow with less than 2 packets in flight.
> This is done so each flow has at least 2 packets in flight, so they
> will not starve, and also to help prevent delayed ACK timeouts.
>
> It will also work with ECN enabled traffic, where the packets will be
> CE marked if their EDT is more than 50us in the future.
>
> The table below shows some performance numbers. The flows are back to
> back RPCS. One server sending to another, either 2 or 4 flows.
> One flow is a 10KB RPC, the rest are 1MB RPCs. When there are more
> than one flow of a given RPC size, the numbers represent averages.
>
> The rate limit applies to all flows (they are in the same cgroup).
> Tests ending with "-edt" ran with the new BPF program supporting EDT.
> Tests ending with "-hbt" ran on top HBT qdisc with the specified rate
> (i.e. no HBM). The other tests ran with the HBM BPF program included
> in the HBM patch-set.
>
> EDT has limited value when using DCTCP, but it helps in many cases when
> using Cubic. It usually achieves larger link utilization and lower
> 99% latencies for the 1MB RPCs.
> HBM ends up queueing a lot of packets with its default parameter values,
> reducing the goodput of the 10KB RPCs and increasing their latency. Also,
> the RTTs seen by the flows are quite large.
>
> Aggr 10K 10K 10K 1MB 1MB 1MB
> Limit rate drops RTT rate P90 P99 rate P90 P99
> Test rate Flows Mbps % us Mbps us us Mbps ms ms
> -------- ---- ----- ---- ----- --- ---- ---- ---- ---- ---- ----
> cubic 1G 2 904 0.02 108 257 511 539 647 13.4 24.5
> cubic-edt 1G 2 982 0.01 156 239 656 967 743 14.0 17.2
> dctcp 1G 2 977 0.00 105 324 408 744 653 14.5 15.9
> dctcp-edt 1G 2 981 0.01 142 321 417 811 660 15.7 17.0
> cubic-htb 1G 2 919 0.00 1825 40 2822 4140 879 9.7 9.9
>
> cubic 200M 2 155 0.30 220 81 532 655 74 283 450
> cubic-edt 200M 2 188 0.02 222 87 1035 1095 101 84 85
> dctcp 200M 2 188 0.03 111 77 912 939 111 76 325
> dctcp-edt 200M 2 188 0.03 217 74 1416 1738 114 76 79
> cubic-htb 200M 2 188 0.00 5015 8 14ms 15ms 180 48 50
>
> cubic 1G 4 952 0.03 110 165 516 546 262 38 154
> cubic-edt 1G 4 973 0.01 190 111 1034 1314 287 65 79
> dctcp 1G 4 951 0.00 103 180 617 905 257 37 38
> dctcp-edt 1G 4 967 0.00 163 151 732 1126 272 43 55
> cubic-htb 1G 4 914 0.00 3249 13 7ms 8ms 300 29 34
>
> cubic 5G 4 4236 0.00 134 305 490 624 1310 10 17
> cubic-edt 5G 4 4865 0.00 156 306 425 759 1520 10 16
> dctcp 5G 4 4936 0.00 128 485 221 409 1484 7 9
> dctcp-edt 5G 4 4924 0.00 148 390 392 623 1508 11 26
>
> v1 -> v2: Incorporated Andrii's suggestions
>
> Signed-off-by: Lawrence Brakmo <brakmo@fb.com>
> ---
> samples/bpf/Makefile | 2 +
> samples/bpf/do_hbm_test.sh | 22 ++---
> samples/bpf/hbm.c | 18 +++-
> samples/bpf/hbm_edt_kern.c | 171 +++++++++++++++++++++++++++++++++++++
> samples/bpf/hbm_kern.h | 40 +++++++--
> 5 files changed, 234 insertions(+), 19 deletions(-)
> create mode 100644 samples/bpf/hbm_edt_kern.c
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 0917f8cf4fab..35640414ebb3 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -168,6 +168,7 @@ always += task_fd_query_kern.o
> always += xdp_sample_pkts_kern.o
> always += ibumad_kern.o
> always += hbm_out_kern.o
> +always += hbm_edt_kern.o
>
> KBUILD_HOSTCFLAGS += -I$(objtree)/usr/include
> KBUILD_HOSTCFLAGS += -I$(srctree)/tools/lib/bpf/
> @@ -272,6 +273,7 @@ $(src)/*.c: verify_target_bpf $(LIBBPF)
> $(obj)/tracex5_kern.o: $(obj)/syscall_nrs.h
> $(obj)/hbm_out_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
> $(obj)/hbm.o: $(src)/hbm.h
> +$(obj)/hbm_edt_kern.o: $(src)/hbm.h $(src)/hbm_kern.h
>
> # asm/sysreg.h - inline assembly used by it is incompatible with llvm.
> # But, there is no easy way to fix it, so just exclude it since it is
> diff --git a/samples/bpf/do_hbm_test.sh b/samples/bpf/do_hbm_test.sh
> index e48b047d4646..ffe4c0607341 100755
> --- a/samples/bpf/do_hbm_test.sh
> +++ b/samples/bpf/do_hbm_test.sh
> @@ -14,7 +14,7 @@ Usage() {
> echo "loads. The output is the goodput in Mbps (unless -D was used)."
> echo ""
> echo "USAGE: $name [out] [-b=<prog>|--bpf=<prog>] [-c=<cc>|--cc=<cc>]"
> - echo " [-D] [-d=<delay>|--delay=<delay>] [--debug] [-E]"
> + echo " [-D] [-d=<delay>|--delay=<delay>] [--debug] [-E] [--edt]"
> echo " [-f=<#flows>|--flows=<#flows>] [-h] [-i=<id>|--id=<id >]"
> echo " [-l] [-N] [--no_cn] [-p=<port>|--port=<port>] [-P]"
> echo " [-q=<qdisc>] [-R] [-s=<server>|--server=<server]"
> @@ -30,6 +30,7 @@ Usage() {
> echo " other detailed information. This information is"
> echo " test dependent (i.e. iperf3 or netperf)."
> echo " -E enable ECN (not required for dctcp)"
> + echo " --edt use fq's Earliest Departure Time (requires fq)"
> echo " -f or --flows number of concurrent flows (default=1)"
> echo " -i or --id cgroup id (an integer, default is 1)"
> echo " -N use netperf instead of iperf3"
> @@ -130,13 +131,12 @@ processArgs () {
> details=1
> ;;
> -E)
> - ecn=1
> + ecn=1
> + ;;
> + --edt)
> + flags="$flags --edt"
> + qdisc="fq"
> ;;
> - # Support for upcomming fq Early Departure Time egress rate limiting
> - #--edt)
> - # prog="hbm_out_edt_kern.o"
> - # qdisc="fq"
> - # ;;
> -f=*|--flows=*)
> flows="${i#*=}"
> ;;
> @@ -228,8 +228,8 @@ if [ "$netem" -ne "0" ] ; then
> tc qdisc del dev lo root > /dev/null 2>&1
> tc qdisc add dev lo root netem delay $netem\ms > /dev/null 2>&1
> elif [ "$qdisc" != "" ] ; then
> - tc qdisc del dev lo root > /dev/null 2>&1
> - tc qdisc add dev lo root $qdisc > /dev/null 2>&1
> + tc qdisc del dev eth0 root > /dev/null 2>&1
> + tc qdisc add dev eth0 root $qdisc > /dev/null 2>&1
> fi
>
> n=0
> @@ -399,7 +399,9 @@ fi
> if [ "$netem" -ne "0" ] ; then
> tc qdisc del dev lo root > /dev/null 2>&1
> fi
> -
> +if [ "$qdisc" != "" ] ; then
> + tc qdisc del dev eth0 root > /dev/null 2>&1
> +fi
> sleep 2
>
> hbmPid=`ps ax | grep "hbm " | grep --invert-match "grep" | awk '{ print $1 }'`
> diff --git a/samples/bpf/hbm.c b/samples/bpf/hbm.c
> index b905b32ff185..e0fbab9bec83 100644
> --- a/samples/bpf/hbm.c
> +++ b/samples/bpf/hbm.c
> @@ -62,6 +62,7 @@ bool loopback_flag;
> bool debugFlag;
> bool work_conserving_flag;
> bool no_cn_flag;
> +bool edt_flag;
>
> static void Usage(void);
> static void read_trace_pipe2(void);
> @@ -372,9 +373,14 @@ static int run_bpf_prog(char *prog, int cg_id)
> fprintf(fout, "avg rtt:%d\n",
> (int)(qstats.sum_rtt / (qstats.pkts_total + 1)));
> // Average credit
> - fprintf(fout, "avg credit:%d\n",
> - (int)(qstats.sum_credit /
> - (1500 * ((int)qstats.pkts_total) + 1)));
> + if (edt_flag)
> + fprintf(fout, "avg credit_ms:%.03f\n",
> + (qstats.sum_credit /
> + (qstats.pkts_total + 1.0)) / 1000000.0);
> + else
> + fprintf(fout, "avg credit:%d\n",
> + (int)(qstats.sum_credit /
> + (1500 * ((int)qstats.pkts_total ) + 1)));
>
> // Return values stats
> for (k = 0; k < RET_VAL_COUNT; k++) {
> @@ -408,6 +414,7 @@ static void Usage(void)
> " Where:\n"
> " -o indicates egress direction (default)\n"
> " -d print BPF trace debug buffer\n"
> + " --edt use fq's Earliest Departure Time\n"
> " -l also limit flows using loopback\n"
> " -n <#> to create cgroup \"/hbm#\" and attach prog\n"
> " Default is /hbm1\n"
> @@ -433,6 +440,7 @@ int main(int argc, char **argv)
> char *optstring = "iodln:r:st:wh";
> struct option loptions[] = {
> {"no_cn", 0, NULL, 1},
> + {"edt", 0, NULL, 2},
> {NULL, 0, NULL, 0}
> };
>
> @@ -441,6 +449,10 @@ int main(int argc, char **argv)
> case 1:
> no_cn_flag = true;
> break;
> + case 2:
> + prog = "hbm_edt_kern.o";
> + edt_flag = true;
> + break;
> case'o':
> break;
> case 'd':
> diff --git a/samples/bpf/hbm_edt_kern.c b/samples/bpf/hbm_edt_kern.c
> new file mode 100644
> index 000000000000..004a44a83e1e
> --- /dev/null
> +++ b/samples/bpf/hbm_edt_kern.c
> @@ -0,0 +1,171 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/* Copyright (c) 2019 Facebook
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of version 2 of the GNU General Public
> + * License as published by the Free Software Foundation.
> + *
> + * Sample Host Bandwidth Manager (HBM) BPF program.
> + *
> + * A cgroup skb BPF egress program to limit cgroup output bandwidth.
> + * It uses a modified virtual token bucket queue to limit average
> + * egress bandwidth. The implementation uses credits instead of tokens.
> + * Negative credits imply that queueing would have happened (this is
> + * a virtual queue, so no queueing is done by it. However, queueing may
> + * occur at the actual qdisc (which is not used for rate limiting).
> + *
> + * This implementation uses 3 thresholds, one to start marking packets and
> + * the other two to drop packets:
> + * CREDIT
> + * - <--------------------------|------------------------> +
> + * | | | 0
> + * | Large pkt |
> + * | drop thresh |
> + * Small pkt drop Mark threshold
> + * thresh
> + *
> + * The effect of marking depends on the type of packet:
> + * a) If the packet is ECN enabled and it is a TCP packet, then the packet
> + * is ECN marked.
> + * b) If the packet is a TCP packet, then we probabilistically call tcp_cwr
> + * to reduce the congestion window. The current implementation uses a linear
> + * distribution (0% probability at marking threshold, 100% probability
> + * at drop threshold).
> + * c) If the packet is not a TCP packet, then it is dropped.
> + *
> + * If the credit is below the drop threshold, the packet is dropped. If it
> + * is a TCP packet, then it also calls tcp_cwr since packets dropped by
> + * by a cgroup skb BPF program do not automatically trigger a call to
> + * tcp_cwr in the current kernel code.
> + *
> + * This BPF program actually uses 2 drop thresholds, one threshold
> + * for larger packets (>= 120 bytes) and another for smaller packets. This
> + * protects smaller packets such as SYNs, ACKs, etc.
> + *
> + * The default bandwidth limit is set at 1Gbps but this can be changed by
> + * a user program through a shared BPF map. In addition, by default this BPF
> + * program does not limit connections using loopback. This behavior can be
> + * overwritten by the user program. There is also an option to calculate
> + * some statistics, such as percent of packets marked or dropped, which
> + * a user program, such as hbm, can access.
> + */
> +
> +#include "hbm_kern.h"
> +
> +SEC("cgroup_skb/egress")
> +int _hbm_out_cg(struct __sk_buff *skb)
> +{
> + signed long long delta = 0, delta_send;
"signed" is not needed here.
> + unsigned long long curtime, sendtime;
> + struct hbm_queue_stats *qsp = NULL;
> + unsigned int queue_index = 0;
> + bool congestion_flag = false;
> + bool ecn_ce_flag = false;
> + struct hbm_pkt_info pkti;
> + struct hbm_vqueue *qdp;
> + bool drop_flag = false;
> + bool cwr_flag = false;
> + int len = skb->len;
> + int rv = ALLOW_PKT;
> +
> + qsp = bpf_map_lookup_elem(&queue_stats, &queue_index);
> + if (qsp != NULL && !qsp->loopback && (skb->ifindex == 1))
The skb->ifindex = 1 is skipped. Why? Some comments will
be helpful to understand.
> + return ALLOW_PKT;
> +
> + hbm_get_pkt_info(skb, &pkti);
> +
> + // We may want to account for the length of headers in len
> + // calculation, like ETH header + overhead, specially if it
> + // is a gso packet. But I am not doing it right now.
> +
> + qdp = bpf_get_local_storage(&queue_state, 0);
> + if (!qdp)
> + return ALLOW_PKT;
> + if (qdp->lasttime == 0)
> + hbm_init_edt_vqueue(qdp, 1024);
> +
> + curtime = bpf_ktime_get_ns();
> +
> + // Begin critical section
> + bpf_spin_lock(&qdp->lock);
> + delta = qdp->lasttime - curtime;
> + // bound bursts to 100us
> + if (delta < -BURST_SIZE_NS) {
> + // negative delta is a credit that allows bursts
> + qdp->lasttime = curtime - BURST_SIZE_NS;
> + delta = -BURST_SIZE_NS;
> + }
> + sendtime = qdp->lasttime;
> + delta_send = BYTES_TO_NS(len, qdp->rate);
> + qdp->lasttime += delta_send;
> + bpf_spin_unlock(&qdp->lock);
> + // End critical section
> +
> + // Set EDT of packet
> + skb->tstamp = sendtime;
> +
> + // Check if we should update rate
> + if (qsp != NULL && (qsp->rate * 128) != qdp->rate) {
> + qdp->rate = qsp->rate * 128;
> + bpf_printk("Updating rate: %d (1sec:%llu bits)\n",
> + (int)qdp->rate,
> + CREDIT_PER_NS(1000000000, qdp->rate) * 8);
How often this bpf_printk will fire at runtime?
If it fires relatively frequently, maybe worth to put it
under debug?
> + }
> +
> + // Set flags (drop, congestion, cwr)
> + // last packet will be sent in the future, bound latency
> + if (delta > DROP_THRESH_NS || (delta > LARGE_PKT_DROP_THRESH_NS &&
> + len > LARGE_PKT_THRESH)) {
> + drop_flag = true;
> + if (pkti.is_tcp && pkti.ecn == 0)
> + cwr_flag = true;
> + } else if (delta > MARK_THRESH_NS) {
> + if (pkti.is_tcp)
> + congestion_flag = true;
> + else
> + drop_flag = true;
> + }
> +
> + if (congestion_flag) {
> + if (bpf_skb_ecn_set_ce(skb)) {
> + ecn_ce_flag = true;
> + } else {
> + if (pkti.is_tcp) {
> + unsigned int rand = bpf_get_prandom_u32();
> +
> + if (delta >= MARK_THRESH_NS +
> + (rand % MARK_REGION_SIZE_NS)) {
> + // Do congestion control
> + cwr_flag = true;
> + }
> + } else if (len > LARGE_PKT_THRESH) {
> + // Problem if too many small packets?
> + drop_flag = true;
> + congestion_flag = false;
> + }
> + }
> + }
> +
> + if (pkti.is_tcp && drop_flag && pkti.packets_out <= 1) {
> + drop_flag = false;
> + cwr_flag = true;
> + congestion_flag = false;
> + }
> +
> + if (qsp != NULL && qsp->no_cn)
> + cwr_flag = false;
> +
> + hbm_update_stats(qsp, len, curtime, congestion_flag, drop_flag,
> + cwr_flag, ecn_ce_flag, &pkti, (int) delta);
> +
> + if (drop_flag) {
> + __sync_add_and_fetch(&(qdp->credit), len);
> + __sync_add_and_fetch(&(qdp->lasttime), -delta_send);
We have race here. Updating qdp->lasttime may happen concurrently with
the update in bpf_spin_lock region. Do we have any issue here?
> + rv = DROP_PKT;
> + }
> +
> + if (cwr_flag)
> + rv |= CWR;
> + return rv;
> +}
> +char _license[] SEC("license") = "GPL";
> diff --git a/samples/bpf/hbm_kern.h b/samples/bpf/hbm_kern.h
> index be19cf1d5cd5..aa207a2eebbd 100644
> --- a/samples/bpf/hbm_kern.h
> +++ b/samples/bpf/hbm_kern.h
> @@ -29,6 +29,7 @@
> #define DROP_PKT 0
> #define ALLOW_PKT 1
> #define TCP_ECN_OK 1
> +#define CWR 2
>
> #ifndef HBM_DEBUG // Define HBM_DEBUG to enable debugging
> #undef bpf_printk
> @@ -45,8 +46,18 @@
> #define MAX_CREDIT (100 * MAX_BYTES_PER_PACKET)
> #define INIT_CREDIT (INITIAL_CREDIT_PACKETS * MAX_BYTES_PER_PACKET)
>
> +// Time base accounting for fq's EDT
> +#define BURST_SIZE_NS 100000 // 100us
> +#define MARK_THRESH_NS 50000 // 50us
> +#define DROP_THRESH_NS 500000 // 500us
> +// Reserve 20us of queuing for small packets (less than 120 bytes)
> +#define LARGE_PKT_DROP_THRESH_NS (DROP_THRESH_NS - 20000)
> +#define MARK_REGION_SIZE_NS (LARGE_PKT_DROP_THRESH_NS - MARK_THRESH_NS)
> +
> // rate in bytes per ns << 20
> #define CREDIT_PER_NS(delta, rate) ((((u64)(delta)) * (rate)) >> 20)
> +#define BYTES_PER_NS(delta, rate) ((((u64)(delta)) * (rate)) >> 20)
> +#define BYTES_TO_NS(bytes, rate) div64_u64(((u64)(bytes)) << 20, (u64)(rate))
>
> struct bpf_map_def SEC("maps") queue_state = {
> .type = BPF_MAP_TYPE_CGROUP_STORAGE,
> @@ -67,6 +78,7 @@ BPF_ANNOTATE_KV_PAIR(queue_stats, int, struct hbm_queue_stats);
> struct hbm_pkt_info {
> int cwnd;
> int rtt;
> + int packets_out;
> bool is_ip;
> bool is_tcp;
> short ecn;
> @@ -86,16 +98,20 @@ static int get_tcp_info(struct __sk_buff *skb, struct hbm_pkt_info *pkti)
> if (tp) {
> pkti->cwnd = tp->snd_cwnd;
> pkti->rtt = tp->srtt_us >> 3;
> + pkti->packets_out = tp->packets_out;
> return 0;
> }
> }
> }
> }
> + pkti->cwnd = 0;
> + pkti->rtt = 0;
> + pkti->packets_out = 0;
> return 1;
> }
>
> -static __always_inline void hbm_get_pkt_info(struct __sk_buff *skb,
> - struct hbm_pkt_info *pkti)
> +static void hbm_get_pkt_info(struct __sk_buff *skb,
> + struct hbm_pkt_info *pkti)
> {
> struct iphdr iph;
> struct ipv6hdr *ip6h;
> @@ -123,10 +139,22 @@ static __always_inline void hbm_get_pkt_info(struct __sk_buff *skb,
>
> static __always_inline void hbm_init_vqueue(struct hbm_vqueue *qdp, int rate)
> {
> - bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
> - qdp->lasttime = bpf_ktime_get_ns();
> - qdp->credit = INIT_CREDIT;
> - qdp->rate = rate * 128;
> + bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
> + qdp->lasttime = bpf_ktime_get_ns();
> + qdp->credit = INIT_CREDIT;
> + qdp->rate = rate * 128;
> +}
> +
> +static __always_inline void hbm_init_edt_vqueue(struct hbm_vqueue *qdp,
> + int rate)
> +{
> + unsigned long long curtime;
> +
> + curtime = bpf_ktime_get_ns();
> + bpf_printk("Initializing queue_state, rate:%d\n", rate * 128);
> + qdp->lasttime = curtime - BURST_SIZE_NS; // support initial burst
> + qdp->credit = 0; // not used
> + qdp->rate = rate * 128;
> }
>
> static __always_inline void hbm_update_stats(struct hbm_queue_stats *qsp,
>
^ permalink raw reply
* Re: [PATCH bpf-next 0/8] bpf: TCP RTT sock_ops bpf callback
From: Soheil Hassas Yeganeh @ 2019-07-01 21:03 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, bpf, David Miller, Alexei Starovoitov, Daniel Borkmann,
Eric Dumazet, Priyaranjan Jha, Yuchung Cheng
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
On Mon, Jul 1, 2019 at 4:48 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> Congestion control team would like to have a periodic callback to
> track some TCP statistics. Let's add a sock_ops callback that can be
> selectively enabled on a socket by socket basis and is executed for
> every RTT. BPF program frequency can be further controlled by calling
> bpf_ktime_get_ns and bailing out early.
>
> I run neper tcp_stream and tcp_rr tests with the sample program
> from the last patch and didn't observe any noticeable performance
> difference.
>
> Suggested-by: Eric Dumazet <edumazet@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Priyaranjan Jha <priyarjha@google.com>
> Cc: Yuchung Cheng <ycheng@google.com>
> Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Thank you for the nice patch series!
> Stanislav Fomichev (8):
> bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
> bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
> bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
> bpf: add icsk_retransmits to bpf_tcp_sock
> bpf/tools: sync bpf.h
> selftests/bpf: test BPF_SOCK_OPS_RTT_CB
> samples/bpf: add sample program that periodically dumps TCP stats
> samples/bpf: fix tcp_bpf.readme detach command
>
> include/net/tcp.h | 8 +
> include/uapi/linux/bpf.h | 12 +-
> net/core/filter.c | 207 +++++++++++-----
> net/ipv4/tcp_input.c | 4 +
> samples/bpf/Makefile | 1 +
> samples/bpf/tcp_bpf.readme | 2 +-
> samples/bpf/tcp_dumpstats_kern.c | 65 +++++
> tools/include/uapi/linux/bpf.h | 12 +-
> tools/testing/selftests/bpf/Makefile | 3 +-
> tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
> tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
> 11 files changed, 570 insertions(+), 58 deletions(-)
> create mode 100644 samples/bpf/tcp_dumpstats_kern.c
> create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
> create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
>
> --
> 2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: santosh.shilimkar @ 2019-07-01 21:00 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <01c251f4-c8f8-fcb8-bccc-341d4a3db90a@oracle.com>
On 7/1/19 1:55 PM, Gerd Rausch wrote:
> Hi Santosh,
>
> On 01/07/2019 13.41, santosh.shilimkar@oracle.com wrote:
>>> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>>> if (printk_ratelimit())
>>> pr_warn("RDS/IB: %s returned error(%d)\n",
>>> __func__, ret);
>>> + goto out;
>>> + }
>>> +
>>> + if (!frmr->fr_reg)
>>> + goto out;
>>> +
>>> + /* Wait for the registration to complete in order to prevent an invalid
>>> + * access error resulting from a race between the memory region already
>>> + * being accessed while registration is still pending.
>>> + */
>>> + wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
>>> + msecs_to_jiffies(100));
>>> +
>> This arbitrary timeout in this patch as well as pacth 1/7 which
>> Dave pointed out has any logic ?
>>
>
> It's empirical (see my response to David's question):
> Memory registrations took longer than invalidations, hence 100msec instead of 10msec.
>
>> MR registration command issued to hardware can at times take as
>> much as command timeout(e.g 60 seconds in CX3) and upto that its still
>> legitimate operation and not necessary failure. We shouldn't add
>> arbitrary time outs in ULPs.
>
> Where did you find the 60 seconds for CX3 you are referring to?
> Is there a "generic" upper-bound that is not tied to a specific vendor / HCA?
> Can you provide a pointer?
>
Look for command timeout in CX3 sources. 60 second is upper bound in
CX3. Its not standard in specs(at least not that I know) though
and may vary from vendor to vendor.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: Gerd Rausch @ 2019-07-01 20:58 UTC (permalink / raw)
To: santosh.shilimkar, David Miller; +Cc: netdev
In-Reply-To: <95d566af-30dc-fecc-9a1b-3c8c7d69b880@oracle.com>
Hi Santosh,
On 01/07/2019 13.53, santosh.shilimkar@oracle.com wrote:
> LOCAL_INV/REG etc are all end being HCA commands and the command timeouts are large. 60 seconds is what CX3 HCA has for example.
> Thats the worst case timeout from HCA before marking the command
> to be timeout and hence the operation to be failed.
>
It's a tradeoff between waiting for 60 seconds or just putting that memory-region on a drop_list.
IMO, penalizing an application with an up-to 60 seconds wait time is not necessarily the better of the 2 options.
Thanks,
Gerd
PS: If you've got a pointer to the 60 seconds CX3 HCA timeout, please share.
^ permalink raw reply
* Re: [PATCH net-next 3/7] net/rds: Wait for the FRMR_IS_FREE (or FRMR_IS_STALE) transition after posting IB_WR_LOCAL_INV
From: Gerd Rausch @ 2019-07-01 20:55 UTC (permalink / raw)
To: santosh.shilimkar, netdev; +Cc: David Miller
In-Reply-To: <c79821e0-307c-5736-6eb5-e20983097345@oracle.com>
Hi Santosh,
On 01/07/2019 13.41, santosh.shilimkar@oracle.com wrote:
>> @@ -144,7 +146,29 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
>> if (printk_ratelimit())
>> pr_warn("RDS/IB: %s returned error(%d)\n",
>> __func__, ret);
>> + goto out;
>> + }
>> +
>> + if (!frmr->fr_reg)
>> + goto out;
>> +
>> + /* Wait for the registration to complete in order to prevent an invalid
>> + * access error resulting from a race between the memory region already
>> + * being accessed while registration is still pending.
>> + */
>> + wait_event_timeout(frmr->fr_reg_done, !frmr->fr_reg,
>> + msecs_to_jiffies(100));
>> +
> This arbitrary timeout in this patch as well as pacth 1/7 which
> Dave pointed out has any logic ?
>
It's empirical (see my response to David's question):
Memory registrations took longer than invalidations, hence 100msec instead of 10msec.
> MR registration command issued to hardware can at times take as
> much as command timeout(e.g 60 seconds in CX3) and upto that its still
> legitimate operation and not necessary failure. We shouldn't add
> arbitrary time outs in ULPs.
Where did you find the 60 seconds for CX3 you are referring to?
Is there a "generic" upper-bound that is not tied to a specific vendor / HCA?
Can you provide a pointer?
Thanks,
Gerd
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: santosh.shilimkar @ 2019-07-01 20:53 UTC (permalink / raw)
To: Gerd Rausch, David Miller; +Cc: netdev
In-Reply-To: <a4834749-4aa2-7e79-dbf8-004580364a39@oracle.com>
On 7/1/19 1:50 PM, Gerd Rausch wrote:
> Hi David,
>
> On 01/07/2019 11.27, David Miller wrote:
>> From: Gerd Rausch <gerd.rausch@oracle.com>
>> Date: Mon, 1 Jul 2019 09:39:44 -0700
>>
>>> + /* Memory regions make it onto the "clean_list" via
>>> + * "rds_ib_flush_mr_pool", after the memory region has
>>> + * been posted for invalidation via "rds_ib_post_inv".
>>> + *
>>> + * At that point in time, "fr_state" may still be
>>> + * in state "FRMR_IS_INUSE", since the only place where
>>> + * "fr_state" transitions to "FRMR_IS_FREE" is in
>>> + * is in "rds_ib_mr_cqe_handler", which is
>>> + * triggered by a tasklet.
>>> + *
>>> + * So in case we notice that
>>> + * "fr_state != FRMR_IS_FREE" (see below), * we wait for
>>> + * "fr_inv_done" to trigger with a maximum of 10msec.
>>> + * Then we check again, and only put the memory region
>>> + * onto the drop_list (via "rds_ib_free_frmr")
>>> + * in case the situation remains unchanged.
>>> + *
>>> + * This avoids the problem of memory-regions bouncing
>>> + * between "clean_list" and "drop_list" before they
>>> + * even have a chance to be properly invalidated.
>>> + */
>>> + frmr = &ibmr->u.frmr;
>>> + wait_event_timeout(frmr->fr_inv_done,
>>> + frmr->fr_state == FRMR_IS_FREE,
>>> + msecs_to_jiffies(10));
>>> + if (frmr->fr_state == FRMR_IS_FREE)
>>> + break;
>>
>> If we see FRMR_IS_FREE after the timeout, what cleans this up?
>>
>
> In that case, the memory-region is subjected to the
> "rds_ib_free_frmr(ibmr, true)" call that follows:
> In essence making it onto the "drop_list".
>
> It's the same as if it wouldn't transition to FRMR_IS_FREE at all.
> In both cases, the memory region should get dropped, and the application
> would have been penalized by an extra 10msec wait-time (for having tried to invalidate it).
>
>> Also, why 10msec?
>
> It's empirical.
> I had added some debugging code (not part of this submission) that traced
> the return value of "wait_event_timeout" in order to see the out-lier in terms
> of processing the "IB_WR_LOCAL_INV" request.
>
> On my test-systems the majority of requests were done in less than 1msec.
> I saw an outlier at almost 2msec once.
> So I gave it an extra order-of-magnitude multiplier for extra buffer / paranoia.
>
>> Why that specific value and not some other value?
>
> I looked around to find what Mellanox or any other reference material had
> so say about the expected turn--around time of an "IB_WR_LOCAL_INV" ought to be.
> I wasn't able to find any.
>
LOCAL_INV/REG etc are all end being HCA commands and the command
timeouts are large. 60 seconds is what CX3 HCA has for example.
Thats the worst case timeout from HCA before marking the command
to be timeout and hence the operation to be failed.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net-next 1/7] net/rds: Give fr_state a chance to transition to FRMR_IS_FREE
From: Gerd Rausch @ 2019-07-01 20:50 UTC (permalink / raw)
To: David Miller; +Cc: santosh.shilimkar, netdev
In-Reply-To: <20190701.112751.509316780582361121.davem@davemloft.net>
Hi David,
On 01/07/2019 11.27, David Miller wrote:
> From: Gerd Rausch <gerd.rausch@oracle.com>
> Date: Mon, 1 Jul 2019 09:39:44 -0700
>
>> + /* Memory regions make it onto the "clean_list" via
>> + * "rds_ib_flush_mr_pool", after the memory region has
>> + * been posted for invalidation via "rds_ib_post_inv".
>> + *
>> + * At that point in time, "fr_state" may still be
>> + * in state "FRMR_IS_INUSE", since the only place where
>> + * "fr_state" transitions to "FRMR_IS_FREE" is in
>> + * is in "rds_ib_mr_cqe_handler", which is
>> + * triggered by a tasklet.
>> + *
>> + * So in case we notice that
>> + * "fr_state != FRMR_IS_FREE" (see below), * we wait for
>> + * "fr_inv_done" to trigger with a maximum of 10msec.
>> + * Then we check again, and only put the memory region
>> + * onto the drop_list (via "rds_ib_free_frmr")
>> + * in case the situation remains unchanged.
>> + *
>> + * This avoids the problem of memory-regions bouncing
>> + * between "clean_list" and "drop_list" before they
>> + * even have a chance to be properly invalidated.
>> + */
>> + frmr = &ibmr->u.frmr;
>> + wait_event_timeout(frmr->fr_inv_done,
>> + frmr->fr_state == FRMR_IS_FREE,
>> + msecs_to_jiffies(10));
>> + if (frmr->fr_state == FRMR_IS_FREE)
>> + break;
>
> If we see FRMR_IS_FREE after the timeout, what cleans this up?
>
In that case, the memory-region is subjected to the
"rds_ib_free_frmr(ibmr, true)" call that follows:
In essence making it onto the "drop_list".
It's the same as if it wouldn't transition to FRMR_IS_FREE at all.
In both cases, the memory region should get dropped, and the application
would have been penalized by an extra 10msec wait-time (for having tried to invalidate it).
> Also, why 10msec?
It's empirical.
I had added some debugging code (not part of this submission) that traced
the return value of "wait_event_timeout" in order to see the out-lier in terms
of processing the "IB_WR_LOCAL_INV" request.
On my test-systems the majority of requests were done in less than 1msec.
I saw an outlier at almost 2msec once.
So I gave it an extra order-of-magnitude multiplier for extra buffer / paranoia.
> Why that specific value and not some other value?
I looked around to find what Mellanox or any other reference material had
so say about the expected turn--around time of an "IB_WR_LOCAL_INV" ought to be.
I wasn't able to find any.
Please note that even if there was an upper-bound specified, such as minutes:
It wouldn't necessarily be a good idea to penalize an application by wait-times
up to one minute, if the alternative is to just put this memory region on a
drop-list and pick another one (which is suggested here).
> Why not wait for however long it takes for the tasklet to run and clean it up?
Two reasons I can think of:
1) The penalty of long wait-times would go to the application
2) If there were a firmware-bug, the "wait_event" would not terminate
Thanks,
Gerd
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Heiner Kallweit @ 2019-07-01 20:37 UTC (permalink / raw)
To: Andrew Lunn, Matthias Kaehlcke
Cc: David S . Miller, Rob Herring, Mark Rutland, Florian Fainelli,
netdev, devicetree, linux-kernel, Douglas Anderson
In-Reply-To: <20190701200248.GJ30468@lunn.ch>
On 01.07.2019 22:02, Andrew Lunn wrote:
> On Mon, Jul 01, 2019 at 12:52:24PM -0700, Matthias Kaehlcke wrote:
>> The RTL8211E has extension pages, which can be accessed after
>> selecting a page through a custom method. Add a function to
>> modify bits in a register of an extension page and a few
>> helpers for dealing with ext pages.
>>
>> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
>> inspired by their counterparts phy_modify_paged() and
>> phy_restore_page().
>
> Hi Matthias
>
> While an extended page is selected, what happens to the normal
> registers in the range 0-0x1c? Are they still accessible?
>
AFAIK: no
> Andrew
>
Heiner
^ permalink raw reply
* Re: [PATCH 3/3] net: phy: realtek: Support SSC for the RTL8211E
From: Heiner Kallweit @ 2019-07-01 20:49 UTC (permalink / raw)
To: Matthias Kaehlcke, David S . Miller, Rob Herring, Mark Rutland,
Andrew Lunn, Florian Fainelli
Cc: netdev, devicetree, linux-kernel, Douglas Anderson
In-Reply-To: <20190701195225.120808-3-mka@chromium.org>
On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> By default Spread-Spectrum Clocking (SSC) is disabled on the RTL8211E.
> Enable it if the device tree property 'realtek,enable-ssc' exists.
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> drivers/net/phy/realtek.c | 27 ++++++++++++++++++++++++---
> 1 file changed, 24 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index dfc2e20ef335..b617169ccc8c 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -9,8 +9,10 @@
> * Copyright (c) 2004 Freescale Semiconductor, Inc.
> */
> #include <linux/bitops.h>
> -#include <linux/phy.h>
> +#include <linux/device.h>
> +#include <linux/of.h>
> #include <linux/module.h>
> +#include <linux/phy.h>
>
> #define RTL821x_PHYSR 0x11
> #define RTL821x_PHYSR_DUPLEX BIT(13)
> @@ -28,6 +30,8 @@
>
> #define RTL8211E_EXT_PAGE 7
> #define RTL8211E_EPAGSR 0x1e
> +#define RTL8211E_SCR 0x1a
> +#define RTL8211E_SCR_DISABLE_RXC_SSC BIT(2)
>
> #define RTL8211F_INSR 0x1d
>
> @@ -87,8 +91,8 @@ static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> return ret;
> }
>
> -static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> - int page, u32 regnum, u16 mask, u16 set)
> +static int rtl8211e_modify_ext_paged(struct phy_device *phydev, int page,
> + u32 regnum, u16 mask, u16 set)
> {
> int ret = 0;
> int oldpage;
> @@ -114,6 +118,22 @@ static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> return rtl821e_restore_page(phydev, oldpage, ret);
> }
>
> +static int rtl8211e_probe(struct phy_device *phydev)
> +{
> + struct device *dev = &phydev->mdio.dev;
> + int err;
> +
> + if (of_property_read_bool(dev->of_node, "realtek,enable-ssc")) {
> + err = rtl8211e_modify_ext_paged(phydev, 0xa0, RTL8211E_SCR,
> + RTL8211E_SCR_DISABLE_RXC_SSC,
> + 0);
> + if (err)
> + dev_err(dev, "failed to enable SSC on RXC: %d\n", err);
> + }
> +
> + return 0;
> +}
> +
> static int rtl8201_ack_interrupt(struct phy_device *phydev)
> {
> int err;
> @@ -372,6 +392,7 @@ static struct phy_driver realtek_drvs[] = {
> .config_init = &rtl8211e_config_init,
> .ack_interrupt = &rtl821x_ack_interrupt,
> .config_intr = &rtl8211e_config_intr,
> + .probe = rtl8211e_probe,
I'm not sure whether this setting survives soft reset and power-down.
Maybe it should be better applied in the config_init callback.
> .suspend = genphy_suspend,
> .resume = genphy_resume,
> .read_page = rtl821x_read_page,
>
^ permalink raw reply
* Re: [PATCH 2/3] net: phy: realtek: Enable accessing RTL8211E extension pages
From: Heiner Kallweit @ 2019-07-01 20:43 UTC (permalink / raw)
To: Matthias Kaehlcke, David S . Miller, Rob Herring, Mark Rutland,
Andrew Lunn, Florian Fainelli
Cc: netdev, devicetree, linux-kernel, Douglas Anderson
In-Reply-To: <20190701195225.120808-2-mka@chromium.org>
On 01.07.2019 21:52, Matthias Kaehlcke wrote:
> The RTL8211E has extension pages, which can be accessed after
> selecting a page through a custom method. Add a function to
> modify bits in a register of an extension page and a few
> helpers for dealing with ext pages.
>
> rtl8211e_modify_ext_paged() and rtl821e_restore_page() are
> inspired by their counterparts phy_modify_paged() and
> phy_restore_page().
>
> Signed-off-by: Matthias Kaehlcke <mka@chromium.org>
> ---
> This code might be applicable to other Realtek PHYs, but I don't
> have access to the datasheets to confirm it, so for now it's just
> for the RTL8211E.
>
This extended page mechanism exists on a number of older Realtek
PHY's. For most extended pages however Realtek releases no public
documentation.
Considering that we use these helpers in one place only, I don't
really see a need for them.
> drivers/net/phy/realtek.c | 61 +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 61 insertions(+)
>
> diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
> index a669945eb829..dfc2e20ef335 100644
> --- a/drivers/net/phy/realtek.c
> +++ b/drivers/net/phy/realtek.c
> @@ -26,6 +26,9 @@
> #define RTL821x_EXT_PAGE_SELECT 0x1e
> #define RTL821x_PAGE_SELECT 0x1f
>
> +#define RTL8211E_EXT_PAGE 7
> +#define RTL8211E_EPAGSR 0x1e
> +
> #define RTL8211F_INSR 0x1d
>
> #define RTL8211F_TX_DELAY BIT(8)
> @@ -53,6 +56,64 @@ static int rtl821x_write_page(struct phy_device *phydev, int page)
> return __phy_write(phydev, RTL821x_PAGE_SELECT, page);
> }
>
> +static int rtl821e_select_ext_page(struct phy_device *phydev, int page)
> +{
> + int rc;
> +
> + rc = phy_write(phydev, RTL821x_PAGE_SELECT, RTL8211E_EXT_PAGE);
> + if (rc)
> + return rc;
> +
> + return phy_write(phydev, RTL8211E_EPAGSR, page);
> +}
> +
> +static int rtl821e_restore_page(struct phy_device *phydev, int oldpage, int ret)
> +{
> + int r;
> +
> + if (oldpage >= 0) {
> + r = phy_write(phydev, RTL821x_PAGE_SELECT, oldpage);
> +
> + /* Propagate the operation return code if the page write
> + * was successful.
> + */
> + if (ret >= 0 && r < 0)
> + ret = r;
> + } else {
> + /* Propagate the page selection error code */
> + ret = oldpage;
> + }
> +
> + return ret;
> +}
> +
> +static int __maybe_unused rtl8211e_modify_ext_paged(struct phy_device *phydev,
> + int page, u32 regnum, u16 mask, u16 set)
> +{
> + int ret = 0;
> + int oldpage;
> + int new;
> +
> + oldpage = phy_read(phydev, RTL821x_PAGE_SELECT);
> + if (oldpage < 0)
> + goto out;
> +
> + ret = rtl821e_select_ext_page(phydev, page);
> + if (ret)
> + goto out;
> +
> + ret = phy_read(phydev, regnum);
> + if (ret < 0)
> + goto out;
> +
> + new = (ret & ~mask) | set;
> + if (new != ret)
> + ret = phy_write(phydev, regnum, new);
> +
> +out:
> + return rtl821e_restore_page(phydev, oldpage, ret);
> +}
> +
> static int rtl8201_ack_interrupt(struct phy_device *phydev)
> {
> int err;
>
^ 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