* Re: [PATCH net-next 00/10] udp: implement GRO support
From: David Miller @ 2018-11-08 0:23 UTC (permalink / raw)
To: pabeni; +Cc: netdev, willemb, steffen.klassert, subashab
In-Reply-To: <cover.1541588248.git.pabeni@redhat.com>
From: Paolo Abeni <pabeni@redhat.com>
Date: Wed, 7 Nov 2018 12:38:27 +0100
> This series implements GRO support for UDP sockets, as the RX counterpart
> of commit bec1f6f69736 ("udp: generate gso with UDP_SEGMENT").
> The core functionality is implemented by the second patch, introducing a new
> sockopt to enable UDP_GRO, while patch 3 implements support for passing the
> segment size to the user space via a new cmsg.
> UDP GRO performs a socket lookup for each ingress packets and aggregate datagram
> directed to UDP GRO enabled sockets with constant l4 tuple.
>
> UDP GRO packets can land on non GRO-enabled sockets, e.g. due to iptables NAT
> rules, and that could potentially confuse existing applications.
>
> The solution adopted here is to de-segment the GRO packet before enqueuing
> as needed. Since we must cope with packet reinsertion after de-segmentation,
> the relevant code is factored-out in ipv4 and ipv6 specific helpers and exposed
> to UDP usage.
>
> While the current code can probably be improved, this safeguard ,implemented in
> the patches 4-7, allows future enachements to enable UDP GSO offload on more
> virtual devices eventually even on forwarded packets.
>
> The last 4 for patches implement some performance and functional self-tests,
> re-using the existing udpgso infrastructure. The problematic scenario described
> above is explicitly tested.
>
> This revision of the series try to address the feedback provided by Willem and
> Subash on previous iteration.
Series applied.
^ permalink raw reply
* [PATCH V2 bpf-next 0/2] Perf-based event notification for sock_ops
From: Sowmini Varadhan @ 2018-11-08 0:12 UTC (permalink / raw)
To: sowmini.varadhan, daniel, netdev, davem, brakmo, ast
This patchset uses eBPF perf-event based notification mechanism to solve
the problem described in
https://marc.info/?l=linux-netdev&m=154022219423571&w=2.
Thanks to Daniel Borkmann for feedback/input.
V2: inlined the call to sys_perf_event_open() following the style
of existing code in kselftests/bpf
The problem statement is
We would like to monitor some subset of TCP sockets in user-space,
(the monitoring application would define 4-tuples it wants to monitor)
using TCP_INFO stats to analyze reported problems. The idea is to
use those stats to see where the bottlenecks are likely to be ("is it
application-limited?" or "is there evidence of BufferBloat in the
path?" etc)
Today we can do this by periodically polling for tcp_info, but this
could be made more efficient if the kernel would asynchronously
notify the application via tcp_info when some "interesting"
thresholds (e.g., "RTT variance > X", or "total_retrans > Y" etc)
are reached. And to make this effective, it is better if
we could apply the threshold check *before* constructing the
tcp_info netlink notification, so that we don't waste resources
constructing notifications that will be discarded by the filter.
This patchset solves the problem by adding perf-event based notification
support for sock_ops (Patch1). The eBPF kernel module can thus
be designed to apply any desired filters to the bpf_sock_ops and
trigger a perf-event notification based on the verdict from the filter.
The uspace component can use these perf-event notifications to either
read any state managed by the eBPF kernel module, or issue a TCP_INFO
netlink call if desired.
Patch 2 provides a simple example that shows how to use this infra
(and also provides a test case for it)
Sowmini Varadhan (2):
bpf: add perf-event notificaton support for sock_ops
selftests/bpf: add a test case for sock_ops perf-event notification
net/core/filter.c | 19 ++
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_tcpnotify.h | 19 ++
tools/testing/selftests/bpf/test_tcpnotify_kern.c | 95 +++++++++++
tools/testing/selftests/bpf/test_tcpnotify_user.c | 186 +++++++++++++++++++++
5 files changed, 322 insertions(+), 1 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_tcpnotify.h
create mode 100644 tools/testing/selftests/bpf/test_tcpnotify_kern.c
create mode 100644 tools/testing/selftests/bpf/test_tcpnotify_user.c
^ permalink raw reply
* [PATCH V2 bpf-next 2/2] selftests/bpf: add a test case for sock_ops perf-event notification
From: Sowmini Varadhan @ 2018-11-08 0:12 UTC (permalink / raw)
To: sowmini.varadhan, daniel, netdev, davem, brakmo, ast
In-Reply-To: <cover.1541630641.git.sowmini.varadhan@oracle.com>
This patch provides a tcp_bpf based eBPF sample. The test
- ncat(1) as the TCP client program to connect() to a port
with the intention of triggerring SYN retransmissions: we
first install an iptables DROP rule to make sure ncat SYNs are
resent (instead of aborting instantly after a TCP RST)
- has a bpf kernel module that sends a perf-event notification for
each TCP retransmit, and also tracks the number of such notifications
sent in the global_map
The test passes when the number of event notifications intercepted
in user-space matches the value in the global_map.
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
V2: inline call to sys_perf_event_open() following the style of existing
code in kselftests/bpf
tools/testing/selftests/bpf/Makefile | 4 +-
tools/testing/selftests/bpf/test_tcpnotify.h | 19 ++
tools/testing/selftests/bpf/test_tcpnotify_kern.c | 95 +++++++++++
tools/testing/selftests/bpf/test_tcpnotify_user.c | 186 +++++++++++++++++++++
4 files changed, 303 insertions(+), 1 deletions(-)
create mode 100644 tools/testing/selftests/bpf/test_tcpnotify.h
create mode 100644 tools/testing/selftests/bpf/test_tcpnotify_kern.c
create mode 100644 tools/testing/selftests/bpf/test_tcpnotify_user.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index e39dfb4..6c94048 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -24,12 +24,13 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
test_align test_verifier_log test_dev_cgroup test_tcpbpf_user \
test_sock test_btf test_sockmap test_lirc_mode2_user get_cgroup_id_user \
test_socket_cookie test_cgroup_storage test_select_reuseport test_section_names \
- test_netcnt
+ test_netcnt test_tcpnotify_user
TEST_GEN_FILES = test_pkt_access.o test_xdp.o test_l4lb.o test_tcp_estats.o test_obj_id.o \
test_pkt_md_access.o test_xdp_redirect.o test_xdp_meta.o sockmap_parse_prog.o \
sockmap_verdict_prog.o dev_cgroup.o sample_ret0.o test_tracepoint.o \
test_l4lb_noinline.o test_xdp_noinline.o test_stacktrace_map.o \
+ test_tcpnotify_kern.o \
sample_map_ret0.o test_tcpbpf_kern.o test_stacktrace_build_id.o \
sockmap_tcp_msg_prog.o connect4_prog.o connect6_prog.o test_adjust_tail.o \
test_btf_haskv.o test_btf_nokv.o test_sockmap_kern.o test_tunnel_kern.o \
@@ -74,6 +75,7 @@ $(OUTPUT)/test_sock_addr: cgroup_helpers.c
$(OUTPUT)/test_socket_cookie: cgroup_helpers.c
$(OUTPUT)/test_sockmap: cgroup_helpers.c
$(OUTPUT)/test_tcpbpf_user: cgroup_helpers.c
+$(OUTPUT)/test_tcpnotify_user: cgroup_helpers.c trace_helpers.c
$(OUTPUT)/test_progs: trace_helpers.c
$(OUTPUT)/get_cgroup_id_user: cgroup_helpers.c
$(OUTPUT)/test_cgroup_storage: cgroup_helpers.c
diff --git a/tools/testing/selftests/bpf/test_tcpnotify.h b/tools/testing/selftests/bpf/test_tcpnotify.h
new file mode 100644
index 0000000..8b6cea0
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcpnotify.h
@@ -0,0 +1,19 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#ifndef _TEST_TCPBPF_H
+#define _TEST_TCPBPF_H
+
+struct tcpnotify_globals {
+ __u32 total_retrans;
+ __u32 ncalls;
+};
+
+struct tcp_notifier {
+ __u8 type;
+ __u8 subtype;
+ __u8 source;
+ __u8 hash;
+};
+
+#define TESTPORT 12877
+#endif
diff --git a/tools/testing/selftests/bpf/test_tcpnotify_kern.c b/tools/testing/selftests/bpf/test_tcpnotify_kern.c
new file mode 100644
index 0000000..edbca20
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcpnotify_kern.c
@@ -0,0 +1,95 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <stddef.h>
+#include <string.h>
+#include <linux/bpf.h>
+#include <linux/if_ether.h>
+#include <linux/if_packet.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/types.h>
+#include <linux/socket.h>
+#include <linux/tcp.h>
+#include <netinet/in.h>
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+#include "test_tcpnotify.h"
+
+struct bpf_map_def SEC("maps") global_map = {
+ .type = BPF_MAP_TYPE_ARRAY,
+ .key_size = sizeof(__u32),
+ .value_size = sizeof(struct tcpnotify_globals),
+ .max_entries = 4,
+};
+
+struct bpf_map_def SEC("maps") perf_event_map = {
+ .type = BPF_MAP_TYPE_PERF_EVENT_ARRAY,
+ .key_size = sizeof(int),
+ .value_size = sizeof(__u32),
+ .max_entries = 2,
+};
+
+int _version SEC("version") = 1;
+
+SEC("sockops")
+int bpf_testcb(struct bpf_sock_ops *skops)
+{
+ int rv = -1;
+ int op;
+
+ op = (int) skops->op;
+
+ if (bpf_ntohl(skops->remote_port) != TESTPORT) {
+ skops->reply = -1;
+ return 0;
+ }
+
+ switch (op) {
+ case BPF_SOCK_OPS_TIMEOUT_INIT:
+ case BPF_SOCK_OPS_RWND_INIT:
+ case BPF_SOCK_OPS_NEEDS_ECN:
+ case BPF_SOCK_OPS_BASE_RTT:
+ case BPF_SOCK_OPS_RTO_CB:
+ rv = 1;
+ break;
+
+ case BPF_SOCK_OPS_TCP_CONNECT_CB:
+ case BPF_SOCK_OPS_TCP_LISTEN_CB:
+ case BPF_SOCK_OPS_ACTIVE_ESTABLISHED_CB:
+ case BPF_SOCK_OPS_PASSIVE_ESTABLISHED_CB:
+ bpf_sock_ops_cb_flags_set(skops, (BPF_SOCK_OPS_RETRANS_CB_FLAG|
+ BPF_SOCK_OPS_RTO_CB_FLAG));
+ rv = 1;
+ break;
+ case BPF_SOCK_OPS_RETRANS_CB: {
+ __u32 key = 0;
+ struct tcpnotify_globals g, *gp;
+ struct tcp_notifier msg = {
+ .type = 0xde,
+ .subtype = 0xad,
+ .source = 0xbe,
+ .hash = 0xef,
+ };
+
+ rv = 1;
+
+ /* Update results */
+ gp = bpf_map_lookup_elem(&global_map, &key);
+ if (!gp)
+ break;
+ g = *gp;
+ g.total_retrans = skops->total_retrans;
+ g.ncalls++;
+ bpf_map_update_elem(&global_map, &key, &g,
+ BPF_ANY);
+ bpf_perf_event_output(skops, &perf_event_map,
+ BPF_F_CURRENT_CPU,
+ &msg, sizeof(msg));
+ }
+ break;
+ default:
+ rv = -1;
+ }
+ skops->reply = rv;
+ return 1;
+}
+char _license[] SEC("license") = "GPL";
diff --git a/tools/testing/selftests/bpf/test_tcpnotify_user.c b/tools/testing/selftests/bpf/test_tcpnotify_user.c
new file mode 100644
index 0000000..ff3c452
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcpnotify_user.c
@@ -0,0 +1,186 @@
+// SPDX-License-Identifier: GPL-2.0
+#define _GNU_SOURCE
+#include <pthread.h>
+#include <inttypes.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <unistd.h>
+#include <asm/types.h>
+#include <sys/syscall.h>
+#include <errno.h>
+#include <string.h>
+#include <linux/bpf.h>
+#include <sys/socket.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+#include <sys/ioctl.h>
+#include <linux/rtnetlink.h>
+#include <signal.h>
+#include <linux/perf_event.h>
+
+#include "bpf_rlimit.h"
+#include "bpf_util.h"
+#include "cgroup_helpers.h"
+
+#include "test_tcpnotify.h"
+#include "trace_helpers.h"
+
+#define SOCKET_BUFFER_SIZE (getpagesize() < 8192L ? getpagesize() : 8192L)
+
+pthread_t tid;
+int rx_callbacks;
+
+static int dummyfn(void *data, int size)
+{
+ struct tcp_notifier *t = data;
+
+ if (t->type != 0xde || t->subtype != 0xad ||
+ t->source != 0xbe || t->hash != 0xef)
+ return 1;
+ rx_callbacks++;
+ return 0;
+}
+
+void tcp_notifier_poller(int fd)
+{
+ while (1)
+ perf_event_poller(fd, dummyfn);
+}
+
+static void *poller_thread(void *arg)
+{
+ int fd = *(int *)arg;
+
+ tcp_notifier_poller(fd);
+ return arg;
+}
+
+int verify_result(const struct tcpnotify_globals *result)
+{
+ return (result->ncalls > 0 && result->ncalls == rx_callbacks ? 0 : 1);
+}
+
+static int bpf_find_map(const char *test, struct bpf_object *obj,
+ const char *name)
+{
+ struct bpf_map *map;
+
+ map = bpf_object__find_map_by_name(obj, name);
+ if (!map) {
+ printf("%s:FAIL:map '%s' not found\n", test, name);
+ return -1;
+ }
+ return bpf_map__fd(map);
+}
+
+static int setup_bpf_perf_event(int mapfd)
+{
+ struct perf_event_attr attr = {
+ .sample_type = PERF_SAMPLE_RAW,
+ .type = PERF_TYPE_SOFTWARE,
+ .config = PERF_COUNT_SW_BPF_OUTPUT,
+ };
+ int key = 0;
+ int pmu_fd;
+
+ pmu_fd = syscall(__NR_perf_event_open, &attr, -1, 0, -1, 0);
+ if (pmu_fd < 0)
+ return pmu_fd;
+ bpf_map_update_elem(mapfd, &key, &pmu_fd, BPF_ANY);
+
+ ioctl(pmu_fd, PERF_EVENT_IOC_ENABLE, 0);
+ return pmu_fd;
+}
+
+int main(int argc, char **argv)
+{
+ const char *file = "test_tcpnotify_kern.o";
+ int prog_fd, map_fd, perf_event_fd;
+ struct tcpnotify_globals g = {0};
+ const char *cg_path = "/foo";
+ int error = EXIT_FAILURE;
+ struct bpf_object *obj;
+ int cg_fd = -1;
+ __u32 key = 0;
+ int rv;
+ char test_script[80];
+ int pmu_fd;
+ cpu_set_t cpuset;
+
+ CPU_ZERO(&cpuset);
+ CPU_SET(0, &cpuset);
+ pthread_setaffinity_np(pthread_self(), sizeof(cpu_set_t), &cpuset);
+
+ if (setup_cgroup_environment())
+ goto err;
+
+ cg_fd = create_and_get_cgroup(cg_path);
+ if (!cg_fd)
+ goto err;
+
+ if (join_cgroup(cg_path))
+ goto err;
+
+ if (bpf_prog_load(file, BPF_PROG_TYPE_SOCK_OPS, &obj, &prog_fd)) {
+ printf("FAILED: load_bpf_file failed for: %s\n", file);
+ goto err;
+ }
+
+ rv = bpf_prog_attach(prog_fd, cg_fd, BPF_CGROUP_SOCK_OPS, 0);
+ if (rv) {
+ printf("FAILED: bpf_prog_attach: %d (%s)\n",
+ error, strerror(errno));
+ goto err;
+ }
+
+ perf_event_fd = bpf_find_map(__func__, obj, "perf_event_map");
+ if (perf_event_fd < 0)
+ goto err;
+
+ map_fd = bpf_find_map(__func__, obj, "global_map");
+ if (map_fd < 0)
+ goto err;
+
+ pmu_fd = setup_bpf_perf_event(perf_event_fd);
+ if (pmu_fd < 0 || perf_event_mmap(pmu_fd) < 0)
+ goto err;
+
+ pthread_create(&tid, NULL, poller_thread, (void *)&pmu_fd);
+
+ sprintf(test_script,
+ "/usr/sbin/iptables -A INPUT -p tcp --dport %d -j DROP",
+ TESTPORT);
+ system(test_script);
+
+ sprintf(test_script,
+ "/usr/bin/nc 127.0.0.1 %d < /etc/passwd > /dev/null 2>&1 ",
+ TESTPORT);
+ system(test_script);
+
+ sprintf(test_script,
+ "/usr/sbin/iptables -D INPUT -p tcp --dport %d -j DROP",
+ TESTPORT);
+ system(test_script);
+
+ rv = bpf_map_lookup_elem(map_fd, &key, &g);
+ if (rv != 0) {
+ printf("FAILED: bpf_map_lookup_elem returns %d\n", rv);
+ goto err;
+ }
+
+ sleep(10);
+
+ if (verify_result(&g)) {
+ printf("FAILED: Wrong stats Expected %d calls, got %d\n",
+ g.ncalls, rx_callbacks);
+ goto err;
+ }
+
+ printf("PASSED!\n");
+ error = 0;
+err:
+ bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
+ close(cg_fd);
+ cleanup_cgroup_environment();
+ return error;
+}
--
1.7.1
^ permalink raw reply related
* [PATCH V2 bpf-next 1/2] bpf: add perf-event notificaton support for sock_ops
From: Sowmini Varadhan @ 2018-11-08 0:12 UTC (permalink / raw)
To: sowmini.varadhan, daniel, netdev, davem, brakmo, ast
In-Reply-To: <cover.1541630641.git.sowmini.varadhan@oracle.com>
This patch allows eBPF programs that use sock_ops to send
perf-based event notifications using bpf_perf_event_output()
Signed-off-by: Sowmini Varadhan <sowmini.varadhan@oracle.com>
---
net/core/filter.c | 19 +++++++++++++++++++
1 files changed, 19 insertions(+), 0 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index e521c5e..23464a3 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4048,6 +4048,23 @@ static unsigned long bpf_xdp_copy(void *dst_buff, const void *src_buff,
return ret;
}
+BPF_CALL_5(bpf_sock_opts_event_output, struct bpf_sock_ops *, skops,
+ struct bpf_map *, map, u64, flags, void *, data, u64, size)
+{
+ return bpf_event_output(map, flags, data, size, NULL, 0, NULL);
+}
+
+static const struct bpf_func_proto bpf_sock_ops_event_output_proto = {
+ .func = bpf_sock_opts_event_output,
+ .gpl_only = true,
+ .ret_type = RET_INTEGER,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_CONST_MAP_PTR,
+ .arg3_type = ARG_ANYTHING,
+ .arg4_type = ARG_PTR_TO_MEM,
+ .arg5_type = ARG_CONST_SIZE_OR_ZERO,
+};
+
static const struct bpf_func_proto bpf_setsockopt_proto = {
.func = bpf_setsockopt,
.gpl_only = false,
@@ -5226,6 +5243,8 @@ bool bpf_helper_changes_pkt_data(void *func)
sock_ops_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
{
switch (func_id) {
+ case BPF_FUNC_perf_event_output:
+ return &bpf_sock_ops_event_output_proto;
case BPF_FUNC_setsockopt:
return &bpf_setsockopt_proto;
case BPF_FUNC_getsockopt:
--
1.7.1
^ permalink raw reply related
* Re: [PATCH] [stable, netdev 4.4+] lan78xx: make sure RX_ADDRL & RX_ADDRH regs are always up to date
From: Sasha Levin @ 2018-11-08 0:17 UTC (permalink / raw)
To: Paolo Pisati
Cc: Woojung Huh, Microchip Linux Driver Support, netdev, stable,
linux-usb, linux-kernel
In-Reply-To: <1541609457-28725-1-git-send-email-p.pisati@gmail.com>
On Wed, Nov 07, 2018 at 05:50:57PM +0100, Paolo Pisati wrote:
>[partial backport upstream 760db29bdc97b73ff60b091315ad787b1deb5cf5]
>
>Upon invocation, lan78xx_init_mac_address() checks that the mac address present
>in the RX_ADDRL & RX_ADDRH registers is a valid address, if not, it first tries
>to read a new address from an external eeprom or the otp area, and in case both
>read fail (or the address read back is invalid), it randomly generates a new
>one.
>
>Unfortunately, due to the way the above logic is laid out,
>if both read_eeprom() and read_otp() fail, a new mac address is correctly
>generated but is never written back to RX_ADDRL & RX_ADDRH, leaving the chip in an
>incosistent state and with an invalid mac address (e.g. the nic appears to be
>completely dead, and doesn't receive any packet, etc):
>
>lan78xx_init_mac_address()
>...
>if (lan78xx_read_eeprom(addr ...) || lan78xx_read_otp(addr ...)) {
> if (is_valid_ether_addr(addr) {
> // nop...
> } else {
> random_ether_addr(addr);
> }
>
> // correctly writes back the new address
> lan78xx_write_reg(RX_ADDRL, addr ...);
> lan78xx_write_reg(RX_ADDRH, addr ...);
>} else {
> // XXX if both eeprom and otp read fail, we land here and skip
> // XXX the RX_ADDRL & RX_ADDRH update completely
> random_ether_addr(addr);
>}
>
>This bug went unnoticed because lan78xx_read_otp() was buggy itself and would
>never fail, up until 4bfc338 "lan78xx: Correctly indicate invalid OTP"
>fixed it and as a side effect uncovered this bug.
>
>4.18+ is fine, since the bug was implicitly fixed in 760db29 "lan78xx: Read MAC
>address from DT if present" when the address change logic was reorganized, but
>it's still present in all stable trees below that: linux-4.4.y, linux-4.9.y,
>linux-4.14.y, etc up to linux-4.18.y (not included).
>
>Signed-off-by: Paolo Pisati <p.pisati@gmail.com>
So why not just take 760db29bdc completely? It looks safer than taking a
partial backport, and will make applying future patches easier.
I tried to do it and it doesn't look like there are any dependencies
that would cause an issue.
^ permalink raw reply
* Re: [PATCH net-next v5 0/9] vrf: allow simultaneous service instances in default and other VRFs
From: David Miller @ 2018-11-08 0:13 UTC (permalink / raw)
To: mmanning; +Cc: netdev
In-Reply-To: <20181107153610.7526-1-mmanning@vyatta.att-mail.com>
From: Mike Manning <mmanning@vyatta.att-mail.com>
Date: Wed, 7 Nov 2018 15:36:01 +0000
> Services currently have to be VRF-aware if they are using an unbound
> socket. One cannot have multiple service instances running in the
> default and other VRFs for services that are not VRF-aware and listen
> on an unbound socket. This is because there is no easy way of isolating
> packets received in the default VRF from those arriving in other VRFs.
>
> This series provides this isolation for stream sockets subject to the
> existing kernel parameter net.ipv4.tcp_l3mdev_accept not being set,
> given that this is documented as allowing a single service instance to
> work across all VRF domains. Similarly, net.ipv4.udp_l3mdev_accept is
> checked for datagram sockets, and net.ipv4.raw_l3mdev_accept is
> introduced for raw sockets. The functionality applies to UDP & TCP
> services as well as those using raw sockets, and is for IPv4 and IPv6.
>
> Example of running ssh instances in default and blue VRF:
...
Series applied, thanks Mike.
^ permalink raw reply
* [PATCH] staging: net: ipv4: tcp_westwood: fixed warnings and checks
From: Suraj Singh @ 2018-11-08 9:46 UTC (permalink / raw)
To: davem; +Cc: kuznet, yoshfuji, netdev, linux-kernel, suraj1998
In-Reply-To: <1541425985-31869-1-git-send-email-suraj1998@gmail.com>
Fixed warnings and checks for TCP Westwood
Signed-off-by: Suraj Singh <suraj1998@gmail.com>
---
net/ipv4/tcp_westwood.c | 31 ++++++++++++-------------------
1 file changed, 12 insertions(+), 19 deletions(-)
diff --git a/net/ipv4/tcp_westwood.c b/net/ipv4/tcp_westwood.c
index bec9caf..8879152 100644
--- a/net/ipv4/tcp_westwood.c
+++ b/net/ipv4/tcp_westwood.c
@@ -43,11 +43,10 @@ struct westwood {
};
/* TCP Westwood functions and constants */
-#define TCP_WESTWOOD_RTT_MIN (HZ/20) /* 50ms */
-#define TCP_WESTWOOD_INIT_RTT (20*HZ) /* maybe too conservative?! */
+#define TCP_WESTWOOD_RTT_MIN (HZ / 20) /* 50ms */
+#define TCP_WESTWOOD_INIT_RTT (20 * HZ) /* maybe too conservative?! */
-/*
- * @tcp_westwood_create
+/* @tcp_westwood_create
* This function initializes fields used in TCP Westwood+,
* it is called after the initial SYN, so the sequence numbers
* are correct but new passive connections we have no
@@ -73,8 +72,7 @@ static void tcp_westwood_init(struct sock *sk)
w->first_ack = 1;
}
-/*
- * @westwood_do_filter
+/* @westwood_do_filter
* Low-pass filter. Implemented using constant coefficients.
*/
static inline u32 westwood_do_filter(u32 a, u32 b)
@@ -94,8 +92,7 @@ static void westwood_filter(struct westwood *w, u32 delta)
}
}
-/*
- * @westwood_pkts_acked
+/* @westwood_pkts_acked
* Called after processing group of packets.
* but all westwood needs is the last sample of srtt.
*/
@@ -108,8 +105,7 @@ static void tcp_westwood_pkts_acked(struct sock *sk,
w->rtt = usecs_to_jiffies(sample->rtt_us);
}
-/*
- * @westwood_update_window
+/* @westwood_update_window
* It updates RTT evaluation window if it is the right moment to do
* it. If so it calls filter for evaluating bandwidth.
*/
@@ -127,8 +123,7 @@ static void westwood_update_window(struct sock *sk)
w->first_ack = 0;
}
- /*
- * See if a RTT-window has passed.
+ /* See if a RTT-window has passed.
* Be careful since if RTT is less than
* 50ms we don't filter but we continue 'building the sample'.
* This minimum limit was chosen since an estimation on small
@@ -149,12 +144,12 @@ static inline void update_rtt_min(struct westwood *w)
if (w->reset_rtt_min) {
w->rtt_min = w->rtt;
w->reset_rtt_min = 0;
- } else
+ } else {
w->rtt_min = min(w->rtt, w->rtt_min);
+ }
}
-/*
- * @westwood_fast_bw
+/* @westwood_fast_bw
* It is called when we are in fast path. In particular it is called when
* header prediction is successful. In such case in fact update is
* straight forward and doesn't need any particular care.
@@ -171,8 +166,7 @@ static inline void westwood_fast_bw(struct sock *sk)
update_rtt_min(w);
}
-/*
- * @westwood_acked_count
+/* @westwood_acked_count
* This function evaluates cumul_ack for evaluating bk in case of
* delayed or partial acks.
*/
@@ -207,8 +201,7 @@ static inline u32 westwood_acked_count(struct sock *sk)
return w->cumul_ack;
}
-/*
- * TCP Westwood
+/* TCP Westwood
* Here limit is evaluated as Bw estimation*RTTmin (for obtaining it
* in packets we use mss_cache). Rttmin is guaranteed to be >= 2
* so avoids ever returning 0.
--
2.7.4
^ permalink raw reply related
* Re: net: ipv4: tcp_westwood: fixed warnings and checks
From: Suraj Singh @ 2018-11-08 9:16 UTC (permalink / raw)
To: davem; +Cc: kuznet, yoshfuji, netdev, linux-kernel, suraj1998, Suraj Singh
In-Reply-To: <1541425985-31869-1-git-send-email-suraj1998@gmail.com>
From: Suraj Singh <suraj998@gmail.com>
Regrding why I used "staging: " in the commmit message, I was following Greg Kroah-Hartman's video on YouTube on how to submit your first patch, and in his sample commit, he'd started his commit message with "staging: ", and so I thought it was convention to do so. I'll remove that immediately. I made this same mistake in another patch that I just sent for TCP Vegas, I'll make the change there as well.
I didn't consider the complexities of calling the same function twice. I was looking more towards satisying the scriptpatch.pl's requirements.
- tp->snd_cwnd = tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
+ tp->snd_cwnd = tcp_westwood_bw_rttmin(sk);
+ tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
I've made the same mistake here. I'll make these changes and resubmit. Is there anything else that's wrong?
^ permalink raw reply
* Re: [PATCH bpf-next 3/3] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-07 23:41 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107233448.r7vcnxtdzfnzegas@mini-arch>
On Wed, 7 Nov 2018 15:34:48 -0800, Stanislav Fomichev wrote:
> On 11/07, Jakub Kicinski wrote:
> > On Wed, 7 Nov 2018 15:13:33 -0800, Stanislav Fomichev wrote:
> > > On 11/07, Jakub Kicinski wrote:
> > > > On Wed, 7 Nov 2018 14:43:56 -0800, Stanislav Fomichev wrote:
> > > > > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > > > > key 0 0 0 0 \
> > > > > value pinned /sys/fs/bpf/flow/IP/0
> > > >
> > > > Where is that /0 coming from ? Is that in source code? I don't see
> > > > libbpf adding it, maybe I'm missing something.
> > > libbpf adds that, that's a program instance:
> > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744
> >
> > Ugh, I was looking at bpf_object__pin() which uses names :(
> >
> > We never use this multi-instance thing, and I don't think bpftool ever
> > will, so IMHO it'd be good if we just re-did the pinning loop in
> > bpftool.
> I wonder whether I should just add special case to bpf_program__pin: don't
> create a subdir when instances.nr == 1 (and just create a file pin for
> single instance)? In that case I can continue to use libbpf and don't reinvent
> the wheel. Any objections?
Mm.. I'm afraid libbpf needs to keep backward compatibility. We'd have
to add some way for the user (bpftool code) to request the instance ID
does not appear, but (potential) existing users should keep seeing them.
Perhaps others disagree.
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin
From: Jakub Kicinski @ 2018-11-07 23:38 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107232516.qiy24ccbq3hh4ail@mini-arch>
On Wed, 7 Nov 2018 15:25:16 -0800, Stanislav Fomichev wrote:
> On 11/07, Jakub Kicinski wrote:
> > On Wed, 7 Nov 2018 15:00:21 -0800, Stanislav Fomichev wrote:
> > > > > +err_unpin_programs:
> > > > > + bpf_object__for_each_program(prog, obj) {
> > > > > + char buf[PATH_MAX];
> > > > > + int len;
> > > > > +
> > > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > > > > + prog->section_name);
> > > > > + if (len < 0)
> > > > > + continue;
> > > > > + else if (len >= PATH_MAX)
> > > > > + continue;
> > > > > +
> > > > > + unlink(buf);
> > > >
> > > > I think that's no bueno, if pin failed because the file already exists
> > > > you'll now remove that already existing file.
> > >
> > > How about we check beforehand and bail early if we are going to
> > > overwrite something?
> >
> > Possible, although the most common way to handle situation like this in
> > the kernel is to "continue the iteration in reverse" over the list.
> > I.e. walk the list back. I think the objects are on a double linked
> > list. You may need to add the appropriate foreach macro and helper..
>
> That sounds more complicated than just ensuring that the top directory
> for the pins doesn't exist and then rm -rf it on failure.
Why would we require that the directory does not exist? We can
check if it exists and then either create or just pin all in an existing
one.
I don't think it should be that much effort to write a reverse for
loop - it could actually be less LoC than that rm_rf function :)
> I'm thinking about copy-pasting rm_rf from perf
> (https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/util.c#n119).
> Thoughts?
>
> Btw, current patch won't work because of those /0 added by bpf_program__pin.
^ permalink raw reply
* Re: [PATCH bpf-next] xdp: sample code for redirecting vlan packets to specific cpus
From: Shannon Nelson @ 2018-11-07 23:37 UTC (permalink / raw)
To: daniel
Cc: john.fastabend, Shannon Nelson, ast, netdev, eric.dumazet,
silviu.smarandache
In-Reply-To: <a89d3362-ced4-2851-05ce-98028f558b2f@iogearbox.net>
On Wed, Nov 7, 2018 at 1:44 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 10/29/2018 11:11 PM, John Fastabend wrote:
> > On 10/29/2018 02:19 PM, Shannon Nelson wrote:
> >> This is an example of using XDP to redirect the processing of
> >> particular vlan packets to specific CPUs. This is in response
> >> to comments received on a kernel patch put forth previously
> >> to do something similar using RPS.
> >> https://www.spinics.net/lists/netdev/msg528210.html
> >> [PATCH net-next] net: enable RPS on vlan devices
> >>
> >> This XDP application watches for inbound vlan-tagged packets
> >> and redirects those packets to be processed on a specific CPU
> >> as configured in a BPF map. The BPF map can be modified by
> >> this user program, which can also load and unload the kernel
> >> XDP code.
> >>
> >> One example use is for supporting VMs where we can't control the
> >> OS being used: we'd like to separate the VM CPU processing from
> >> the host's CPUs as a way to help mitigate L1TF related issues.
> >> When running the VM's traffic on a vlan we can stick the host's
> >> Rx processing on one set of CPUs separate from the VM's CPUs.
> >>
> >> This example currently uses a vlan key and cpu value in the
> >> BPF map, so only can do one CPU per vlan. This could easily
> >> be modified to use a bitpattern of CPUs rather than a CPU id
> >> to allow multiple CPUs per vlan.
> >
> > Great, so does this solve your use case then? At least on drivers
> > with XDP support?
> >
> >>
> >> Signed-off-by: Shannon Nelson <shannon.nelson@oracle.com>
> >> ---
> >
> > Some really small and trivial nits below.
> >
> > Acked-by: John Fastabend <john.fastabend@gmail.com>
> >
> > [...]
> >
> >> + if (install) {
> >> +
> >
> > new line probably not needed.
> >
> >> + /* check to see if already installed */
> >> + errno = 0;
> >> + access(pin_prog_name, R_OK);
> >> + if (errno != ENOENT) {
> >> + fprintf(stderr, "ERR: %s is already installed\n", argv[0]);
> >> + return -1;
> >> + }
> >> +
> >> + /* load the XDP program and maps with the convenient library */
> >> + if (load_bpf_file(filename)) {
> >> + fprintf(stderr, "ERR: load_bpf_file(%s): \n%s",
> >> + filename, bpf_log_buf);
> >> + return -1;
> >> + }
> >> + if (!prog_fd[0]) {
> >> + fprintf(stderr, "ERR: load_bpf_file(%s): %d %s\n",
> >> + filename, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> +
> >> + /* pin the XDP program and maps */
> >> + if (bpf_obj_pin(prog_fd[0], pin_prog_name) < 0) {
> >> + fprintf(stderr, "ERR: bpf_obj_pin(%s): %d %s\n",
> >> + pin_prog_name, errno, strerror(errno));
> >> + if (errno == 2)
> >> + fprintf(stderr, " (is the BPF fs mounted on /sys/fs/bpf?)\n");
> >> + return -1;
> >> + }
> >> + if (bpf_obj_pin(map_fd[0], pin_vlanmap_name) < 0) {
> >> + fprintf(stderr, "ERR: bpf_obj_pin(%s): %d %s\n",
> >> + pin_vlanmap_name, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> + if (bpf_obj_pin(map_fd[2], pin_countermap_name) < 0) {
> >> + fprintf(stderr, "ERR: bpf_obj_pin(%s): %d %s\n",
> >> + pin_countermap_name, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> +
> >> + /* prep the vlan map with "not used" values */
> >> + c64 = UNDEF_CPU;
> >> + for (v64 = 0; v64 < 4096; v64++) {
> >
> > maybe #define MAX_VLANS 4096 just to avoid constants.
> >
> >> + if (bpf_map_update_elem(map_fd[0], &v64, &c64, 0)) {
> >> + fprintf(stderr, "ERR: preping vlan map failed on v=%llu: %d %s\n",
> >> + v64, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> + }
> >> +
> >> + /* prep the cpumap with queue sizes */
> >> + c64 = 128+64; /* see note in xdp_redirect_cpu_user.c */
> >> + for (v64 = 0; v64 < MAX_CPUS; v64++) {
> >> + if (bpf_map_update_elem(map_fd[1], &v64, &c64, 0)) {
> >> + if (errno == ENODEV) {
> >> + /* Save the last CPU number attempted
> >> + * into the counters map
> >> + */
> >> + c64 = CPU_COUNT;
> >> + ret = bpf_map_update_elem(map_fd[2], &c64, &v64, 0);
> >> + break;
> >> + }
> >> +
> >> + fprintf(stderr, "ERR: preping cpu map failed on v=%llu: %d %s\n",
> >> + v64, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> + }
> >> +
> >> + /* wire the XDP program to the device */
> >> + if (bpf_set_link_xdp_fd(ifindex, prog_fd[0], 0) < 0) {
> >> + fprintf(stderr, "ERR: bpf_set_link_xdp_fd(): %d %s\n",
> >> + errno, strerror(errno));
> >> + return -1;
> >> + }
> >> +
> >> + return 0;
> >> + }
> >> +
> >> + if (remove) {
> >> +
>
> Ditto
>
> >> + /* unlink the program from the device */
> >> + if (bpf_set_link_xdp_fd(ifindex, -1, 0) < 0)
> >> + fprintf(stderr, "ERR: bpf_set_link_xdp_fd(): %d %s\n",
> >> + errno, strerror(errno));
> >> +
> >> + /* unlink pinned files */
> >> + if (unlink(pin_prog_name))
> >> + fprintf(stderr, "ERR: unlink(%s): %d %s\n",
> >> + pin_prog_name, errno, strerror(errno));
> >> + if (unlink(pin_vlanmap_name))
> >> + fprintf(stderr, "ERR: unlink(%s): %d %s\n",
> >> + pin_vlanmap_name, errno, strerror(errno));
> >> + if (unlink(pin_countermap_name))
> >> + fprintf(stderr, "ERR: unlink(%s): %d %s\n",
> >> + pin_countermap_name, errno, strerror(errno));
> >> +
> >> + return 0;
> >> + }
> >> +
> >> + if (vlan == 0) {
> >> + fprintf(stderr, "ERR: required option --vlan missing\n");
> >> + goto error;
> >> + }
> >> +
> >> + if (cpu == MAX_CPUS && vlan > 0) {
> >> + fprintf(stderr, "ERR: required option --cpu missing\n");
> >> + goto error;
> >> + }
> >> +
> >> + vfd = bpf_obj_get(pin_vlanmap_name);
> >> + if (vfd < 0) {
> >> + fprintf(stderr, "ERR: can't find pinned map %s: %d %s\n",
> >> + pin_vlanmap_name, errno, strerror(errno));
> >> + if (errno == ENOENT)
> >> + fprintf(stderr, " (has %s been installed yet?)\n", argv[0]);
> >> + return -1;
> >> + }
> >> +
> >> + /* decode the requested action */
> >> + if (vlan > 0) {
> >> + /* check cpu against the max value found */
> >> + cfd = bpf_obj_get(pin_countermap_name);
> >> + if (cfd < 0) {
> >> + fprintf(stderr, "ERR: can't find pinned map %s: %d %s\n",
> >> + pin_countermap_name, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> + c64 = CPU_COUNT;
> >> + ret = bpf_map_lookup_elem(cfd, &c64, &v64);
> >> + if (cpu >= v64) {
> >
> > Need to check ret code?
>
> Yeah, I expect this would have another respin, thanks!
Being as I no longer work for Oracle (as of this week), and I'm deep
into the spin-up of learning a new company, this won't be happening
right away. Let's consider this an RFC, and I will try to follow up
in the "near" future.
Thanks
sln
>
> >> + fprintf(stderr, "ERR: cpu %d greater than max %llu\n", cpu, v64);
> >> + return -1;
> >> + }
> >> +
> >> + /* Note that the value and key pointers really do need to be
> >> + * pointers to 64-bit values, else things get a bit muddled.
> >> + */
> >> + v64 = vlan;
> >> + c64 = cpu;
> >> + ret = bpf_map_update_elem(vfd, &v64, &c64, 0);
> >> + if (ret) {
> >> + fprintf(stderr, "Adding vlan %d CPU %d failed: %d %s\n",
> >> + vlan, cpu, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> +
> >> + } else {
> >> + v64 = -vlan;
> >> + c64 = UNDEF_CPU;
> >> +
> >> + /* We can't actually delete from a TYPE_ARRAY map, so we
> >> + * simply set it to an undefined value.
> >> + */
> >> + ret = bpf_map_update_elem(vfd, &v64, &c64, 0);
> >> + if (ret) {
> >> + fprintf(stderr, "Delete of vlan %llu failed: %d %s\n",
> >> + v64, errno, strerror(errno));
> >> + return -1;
> >> + }
> >> + }
> >> +
> >> + return 0;
> >> +
> >> +error:
> >> + usage(argv);
> >> + return -1;
> >> +}
> >>
> >
>
--
==============================================
Mr. Shannon Nelson Parents can't afford to be squeamish.
^ permalink raw reply
* Re: net: ipv4: tcp_westwood: fixed warnings and checks
From: Suraj Singh @ 2018-11-08 9:08 UTC (permalink / raw)
To: davem; +Cc: kuznet, yoshfuji, netdev, linux-kernel, Suraj Singh
In-Reply-To: <1541425985-31869-1-git-send-email-suraj1998@gmail.com--annotate>
From: Suraj Singh <suraj998@gmail.com>
Regrding why I used "staging: " in the commmit message, I was following Greg Kroah-Hartman's video on YouTube on how to submit your first patch, and in his sample commit, he'd started his commit message with "staging: ", and so I thought it was convention to do so. I'll remove that immediately. I made this same mistake in another patch that I just sent for TCP Vegas, I'll make the change there as well.
I didn't consider the complexities of calling the same function twice. I was looking more towards satisying the scriptpatch.pl's requirements.
- tp->snd_cwnd = tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
+ tp->snd_cwnd = tcp_westwood_bw_rttmin(sk);
+ tp->snd_ssthresh = tcp_westwood_bw_rttmin(sk);
I've made the same mistake here. I'll make these changes and resubmit. Is there anything else that's wrong?
^ permalink raw reply
* Re: [PATCH bpf-next 3/3] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-07 23:34 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107152917.729ea83c@cakuba.netronome.com>
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 15:13:33 -0800, Stanislav Fomichev wrote:
> > On 11/07, Jakub Kicinski wrote:
> > > On Wed, 7 Nov 2018 14:43:56 -0800, Stanislav Fomichev wrote:
> > > > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > > > key 0 0 0 0 \
> > > > value pinned /sys/fs/bpf/flow/IP/0
> > >
> > > Where is that /0 coming from ? Is that in source code? I don't see
> > > libbpf adding it, maybe I'm missing something.
> > libbpf adds that, that's a program instance:
> > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744
>
> Ugh, I was looking at bpf_object__pin() which uses names :(
>
> We never use this multi-instance thing, and I don't think bpftool ever
> will, so IMHO it'd be good if we just re-did the pinning loop in
> bpftool.
I wonder whether I should just add special case to bpf_program__pin: don't
create a subdir when instances.nr == 1 (and just create a file pin for
single instance)? In that case I can continue to use libbpf and don't reinvent
the wheel. Any objections?
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-07 23:30 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107231207.v425etmqm4v6rito@mini-arch>
On Wed, 7 Nov 2018 15:12:07 -0800, Stanislav Fomichev wrote:
> > > I agree, constructing the jmp_table is a bit fragile with all the
> > > dependencies on the order of the progs. I'll drop that and will send a
> > > v2 that pins all the programs from the obj file instead and offloads
> > > jmp_table construction to the user. So the supposed use case would be
> > > something like the following:
> > >
> > > bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
> >
> > Okay. One more thing - how do we differentiate between mass pin and the
> > existing pin first behaviour? Should we perhaps add a loadall command
> > or some flag?
> In v2 I did by program type:
> * flow_dissector -> pin all
> * not flow_dissector -> pin first?
>
> But we can have loadall or something like:
> load OBJ [pinfirst|pinall] FILE|DIR [type TYPE]
>
> If we want to add user control, I'd go with loadall command,
> adding more optional flags in between is a mess..
I think user control would be good. Agreed on loadall being better.
^ permalink raw reply
* Re: [PATCH bpf-next 3/3] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-07 23:29 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107231333.n34ffy4dl6rtbs7e@mini-arch>
On Wed, 7 Nov 2018 15:13:33 -0800, Stanislav Fomichev wrote:
> On 11/07, Jakub Kicinski wrote:
> > On Wed, 7 Nov 2018 14:43:56 -0800, Stanislav Fomichev wrote:
> > > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > > key 0 0 0 0 \
> > > value pinned /sys/fs/bpf/flow/IP/0
> >
> > Where is that /0 coming from ? Is that in source code? I don't see
> > libbpf adding it, maybe I'm missing something.
> libbpf adds that, that's a program instance:
> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744
Ugh, I was looking at bpf_object__pin() which uses names :(
We never use this multi-instance thing, and I don't think bpftool ever
will, so IMHO it'd be good if we just re-did the pinning loop in
bpftool.
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin
From: Stanislav Fomichev @ 2018-11-07 23:25 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107150605.59a8ac1c@cakuba.netronome.com>
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 15:00:21 -0800, Stanislav Fomichev wrote:
> > > > +err_unpin_programs:
> > > > + bpf_object__for_each_program(prog, obj) {
> > > > + char buf[PATH_MAX];
> > > > + int len;
> > > > +
> > > > + len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > > > + prog->section_name);
> > > > + if (len < 0)
> > > > + continue;
> > > > + else if (len >= PATH_MAX)
> > > > + continue;
> > > > +
> > > > + unlink(buf);
> > >
> > > I think that's no bueno, if pin failed because the file already exists
> > > you'll now remove that already existing file.
> >
> > How about we check beforehand and bail early if we are going to
> > overwrite something?
>
> Possible, although the most common way to handle situation like this in
> the kernel is to "continue the iteration in reverse" over the list.
> I.e. walk the list back. I think the objects are on a double linked
> list. You may need to add the appropriate foreach macro and helper..
That sounds more complicated than just ensuring that the top directory
for the pins doesn't exist and then rm -rf it on failure.
I'm thinking about copy-pasting rm_rf from perf
(https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/util.c#n119).
Thoughts?
Btw, current patch won't work because of those /0 added by bpf_program__pin.
^ permalink raw reply
* [PATCH iproute2] bridge: fdb: remove redundant dev string in show output
From: Roopa Prabhu @ 2018-11-07 23:14 UTC (permalink / raw)
To: stephen; +Cc: netdev, phil, nikolay, dsahern
From: Roopa Prabhu <roopa@cumulusnetworks.com>
After commit 4abb8c723a64 ("bridge: fdb: Fix for missing
keywords in non-JSON output"), I am seeing a double print for dev
in bridge fdb show. eg:
"44:38:39:00:6a:82 dev dev bridge vlan 1 master bridge permanent"
this patch removes the redundant print.
Fixes: 4abb8c723a64 ("bridge: fdb: Fix for missing keywords in non-JSON output")
CC: Phil Sutter <phil@nwl.cc>
Signed-off-by: Roopa Prabhu <roopa@cumulusnetworks.com>
---
bridge/fdb.c | 5 +----
1 file changed, 1 insertion(+), 4 deletions(-)
diff --git a/bridge/fdb.c b/bridge/fdb.c
index f82938f..a5abc1b 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -181,13 +181,10 @@ int print_fdb(struct nlmsghdr *n, void *arg)
"mac", "%s ", lladdr);
}
- if (!filter_index && r->ndm_ifindex) {
- if (!is_json_context())
- fprintf(fp, "dev ");
+ if (!filter_index && r->ndm_ifindex)
print_color_string(PRINT_ANY, COLOR_IFNAME,
"ifname", "dev %s ",
ll_index_to_name(r->ndm_ifindex));
- }
if (tb[NDA_DST]) {
int family = AF_INET;
--
2.1.4
^ permalink raw reply related
* Re: [PATCH bpf-next 3/3] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-07 23:13 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107150944.5d5480ba@cakuba.netronome.com>
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 14:43:56 -0800, Stanislav Fomichev wrote:
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 0 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IP/0
>
> Where is that /0 coming from ? Is that in source code? I don't see
> libbpf adding it, maybe I'm missing something.
libbpf adds that, that's a program instance:
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-07 23:12 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Quentin Monnet, Stanislav Fomichev, netdev, linux-kselftest, ast,
daniel, shuah, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107145122.170daba2@cakuba.netronome.com>
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 14:15:06 -0800, Stanislav Fomichev wrote:
> > On 11/07, Quentin Monnet wrote:
> > > 2018-11-07 12:32 UTC-0800 ~ Jakub Kicinski <jakub.kicinski@netronome.com>
> > > > On Wed, 7 Nov 2018 20:08:53 +0000, Quentin Monnet wrote:
> > > > > > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > > > > > + if (err) {
> > > > > > + p_err("failed to pin program %s",
> > > > > > + bpf_program__title(prog, false));
> > > > > > + goto err_close_obj;
> > > > > > + }
> > > > >
> > > > > I don't have the same opinion as Jakub for pinning :). I was hoping we
> > > > > could also load additional programs (for tail calls) for
> > > > > non-flow_dissector programs. Could this be an occasion to update the
> > > > > code in that direction?
> > > >
> > > > Do you mean having the bpftool construct an array for tail calling
> > > > automatically when loading an object? Or do a "mass pin" of all
> > > > programs in an object file?
> > > >
> > > > I'm not convinced about this strategy of auto assembling a tail call
> > > > array by assuming that a flow dissector object carries programs for
> > > > protocols in order (apart from the main program which doesn't have to
> > > > be first, for some reason).
> > >
> > > Not constructing the prog array, I don't think this should be the role of
> > > bpftool either. Much more a "mass pin", so that you have a link to each
> > > program loaded from the object file and can later add them to a prog array
> > > map with subsequent calls to bpftool.
>
> Makes sense, specific files named after index or program name/title?
> Program name may be nicer?
>
> > I agree, constructing the jmp_table is a bit fragile with all the
> > dependencies on the order of the progs. I'll drop that and will send a
> > v2 that pins all the programs from the obj file instead and offloads
> > jmp_table construction to the user. So the supposed use case would be
> > something like the following:
> >
> > bpftool prog load bpf_flow.o /sys/fs/bpf/flow type flow_dissector
>
> Okay. One more thing - how do we differentiate between mass pin and the
> existing pin first behaviour? Should we perhaps add a loadall command
> or some flag?
In v2 I did by program type:
* flow_dissector -> pin all
* not flow_dissector -> pin first?
But we can have loadall or something like:
load OBJ [pinfirst|pinall] FILE|DIR [type TYPE]
If we want to add user control, I'd go with loadall command,
adding more optional flags in between is a mess..
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key ... value pinned /sys/fs/bpf/flow/<PROTO>/0
>
> Interesting, why $dir/$title/0 ? Perhaps $dir/$title is sufficient?
That's how bpf_program__pin does the pinning, for each program instance.
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/lib/bpf/libbpf.c#n1744
>
> > bpftool map update ...
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector/0 flow_dissector
>
^ permalink raw reply
* Re: [PATCH bpf-next 3/3] bpftool: support loading flow dissector
From: Jakub Kicinski @ 2018-11-07 23:09 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: netdev, linux-kselftest, ast, daniel, shuah, quentin.monnet, guro,
jiong.wang, bhole_prashant_q7, john.fastabend, jbenc,
treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107224356.73080-4-sdf@google.com>
On Wed, 7 Nov 2018 14:43:56 -0800, Stanislav Fomichev wrote:
> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> key 0 0 0 0 \
> value pinned /sys/fs/bpf/flow/IP/0
Where is that /0 coming from ? Is that in source code? I don't see
libbpf adding it, maybe I'm missing something.
^ permalink raw reply
* Re: [PATCH net-next] sock: Reset dst when changing sk_mark via setsockopt
From: David Barmann @ 2018-11-07 23:07 UTC (permalink / raw)
To: netdev; +Cc: Eric Dumazet
In-Reply-To: <4a6b111a-c0d2-e12c-bc42-3c723a0297bc@gmail.com>
> On 11/07/2018 02:33 PM, David Barmann wrote:
> > When setting the SO_MARK socket option, the dst needs to be reset so
> > that a new route lookup is performed.
> >
> > This fixes the case where an application wants to change routing by
> > setting a new sk_mark. If this is done after some packets have already
> > been sent, the dst is cached and has no effect.
> >
> > + } else {
> > + struct dst_entry *dst = sk_dst_get(sk);
> > sk->sk_mark = val;
> > + sk_dst_reset(sk);
>
>
> sk_dst_get() and dst_release() seems extra overhead ?
>
> net/ipv4/ip_sockglue.c do_ip_setsockopt() has a similar handling for IP_TOS,
> and it only calls sk_dst_reset() (If the new TOS is different than the current one)
>
>
>
> So I would suggest :
>
> if (!ns_capable(...)) {
> ret = -EPERM;
> } else if (val != sk->sk_mark) {
> sk->sk_mark = val;
> sk_dst_reset(sk);
> }
Thanks Eric, I'll redo that and resubmit.
-David
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin
From: Jakub Kicinski @ 2018-11-07 23:06 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107230021.4yr2h6fpzhvarfcj@mini-arch>
On Wed, 7 Nov 2018 15:00:21 -0800, Stanislav Fomichev wrote:
> > > +err_unpin_programs:
> > > + bpf_object__for_each_program(prog, obj) {
> > > + char buf[PATH_MAX];
> > > + int len;
> > > +
> > > + len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > > + prog->section_name);
> > > + if (len < 0)
> > > + continue;
> > > + else if (len >= PATH_MAX)
> > > + continue;
> > > +
> > > + unlink(buf);
> >
> > I think that's no bueno, if pin failed because the file already exists
> > you'll now remove that already existing file.
>
> How about we check beforehand and bail early if we are going to
> overwrite something?
Possible, although the most common way to handle situation like this in
the kernel is to "continue the iteration in reverse" over the list.
I.e. walk the list back. I think the objects are on a double linked
list. You may need to add the appropriate foreach macro and helper..
^ permalink raw reply
* Re: [PATCH bpf-next 2/3] libbpf: cleanup after partial failure in bpf_object__pin
From: Stanislav Fomichev @ 2018-11-07 23:00 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
quentin.monnet, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181107145648.66f47037@cakuba.netronome.com>
On 11/07, Jakub Kicinski wrote:
> On Wed, 7 Nov 2018 14:43:55 -0800, Stanislav Fomichev wrote:
> > bpftool will use bpf_object__pin in the next commit to pin all programs
> > and maps from the file; in case of a partial failure, we need to get
> > back to the clean state (undo previous program/map pins).
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++++++++++++++++--------
> > 1 file changed, 48 insertions(+), 10 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index d6e62e90e8d4..309abe7196f3 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1803,14 +1803,17 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
> >
> > len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > bpf_map__name(map));
> > - if (len < 0)
> > - return -EINVAL;
> > - else if (len >= PATH_MAX)
> > - return -ENAMETOOLONG;
> > + if (len < 0) {
> > + err = -EINVAL;
> > + goto err_unpin_maps;
> > + } else if (len >= PATH_MAX) {
> > + err = -ENAMETOOLONG;
> > + goto err_unpin_maps;
> > + }
> >
> > err = bpf_map__pin(map, buf);
> > if (err)
> > - return err;
> > + goto err_unpin_maps;
> > }
> >
> > bpf_object__for_each_program(prog, obj) {
> > @@ -1819,17 +1822,52 @@ int bpf_object__pin(struct bpf_object *obj, const char *path)
> >
> > len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > prog->section_name);
> > - if (len < 0)
> > - return -EINVAL;
> > - else if (len >= PATH_MAX)
> > - return -ENAMETOOLONG;
> > + if (len < 0) {
> > + err = -EINVAL;
> > + goto err_unpin_programs;
> > + } else if (len >= PATH_MAX) {
> > + err = -ENAMETOOLONG;
> > + goto err_unpin_programs;
> > + }
> >
> > err = bpf_program__pin(prog, buf);
> > if (err)
> > - return err;
> > + goto err_unpin_programs;
> > }
> >
> > return 0;
> > +
> > +err_unpin_programs:
> > + bpf_object__for_each_program(prog, obj) {
> > + char buf[PATH_MAX];
> > + int len;
> > +
> > + len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > + prog->section_name);
> > + if (len < 0)
> > + continue;
> > + else if (len >= PATH_MAX)
> > + continue;
> > +
> > + unlink(buf);
>
> I think that's no bueno, if pin failed because the file already exists
> you'll now remove that already existing file.
How about we check beforehand and bail early if we are going to
overwrite something?
> > + }
> > +
> > +err_unpin_maps:
> > + bpf_map__for_each(map, obj) {
> > + char buf[PATH_MAX];
> > + int len;
> > +
> > + len = snprintf(buf, PATH_MAX, "%s/%s", path,
> > + bpf_map__name(map));
> > + if (len < 0)
> > + continue;
> > + else if (len >= PATH_MAX)
> > + continue;
> > +
> > + unlink(buf);
> > + }
> > +
> > + return err;
> > }
> >
> > void bpf_object__close(struct bpf_object *obj)
>
^ permalink raw reply
* Re: [PATCH net-next] sock: Reset dst when changing sk_mark via setsockopt
From: Eric Dumazet @ 2018-11-07 22:58 UTC (permalink / raw)
To: David Barmann, netdev
In-Reply-To: <20181107223327.GA21018@konacove.com>
On 11/07/2018 02:33 PM, David Barmann wrote:
> When setting the SO_MARK socket option, the dst needs to be reset so
> that a new route lookup is performed.
>
> This fixes the case where an application wants to change routing by
> setting a new sk_mark. If this is done after some packets have already
> been sent, the dst is cached and has no effect.
>
> Signed-off-by: David Barmann <david.barmann@stackpath.com>
> ---
> net/core/sock.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 6fcc4bc07d19..187badac24a3 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -950,10 +950,14 @@ int sock_setsockopt(struct socket *sock, int level, int optname,
> clear_bit(SOCK_PASSSEC, &sock->flags);
> break;
> case SO_MARK:
> - if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN))
> + if (!ns_capable(sock_net(sk)->user_ns, CAP_NET_ADMIN)) {
> ret = -EPERM;
> - else
> + } else {
> + struct dst_entry *dst = sk_dst_get(sk);
> sk->sk_mark = val;
> + sk_dst_reset(sk);
sk_dst_get() and dst_release() seems extra overhead ?
net/ipv4/ip_sockglue.c do_ip_setsockopt() has a similar handling for IP_TOS,
and it only calls sk_dst_reset() (If the new TOS is different than the current one)
So I would suggest :
if (!ns_capable(...)) {
ret = -EPERM;
} else if (val != sk->sk_mark) {
sk->sk_mark = val;
sk_dst_reset(sk);
}
^ permalink raw reply
* [PATCH] staging: net: ipv4: tcp_vegas: fixed checks and warnings
From: Suraj Singh @ 2018-11-08 8:29 UTC (permalink / raw)
To: davem, kuznet, yoshfuji; +Cc: netdev, linux-kernel, edumazet, Suraj Singh
Fixed checks and warnings in TCP Vegas
Signed-off-by: Suraj Singh <suraj1998@gmail.com>
---
net/ipv4/tcp_vegas.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/net/ipv4/tcp_vegas.c b/net/ipv4/tcp_vegas.c
index ee113ff..0f65cbe 100644
--- a/net/ipv4/tcp_vegas.c
+++ b/net/ipv4/tcp_vegas.c
@@ -139,8 +139,7 @@ void tcp_vegas_state(struct sock *sk, u8 ca_state)
}
EXPORT_SYMBOL_GPL(tcp_vegas_state);
-/*
- * If the connection is idle and we are restarting,
+/* If the connection is idle and we are restarting,
* then we don't want to do any Vegas calculations
* until we get fresh RTT samples. So when we
* restart, we reset our Vegas state to a clean
@@ -223,7 +222,7 @@ static void tcp_vegas_cong_avoid(struct sock *sk, u32 ack, u32 acked)
* and the window we would like to have. This quantity
* is the "Diff" from the Arizona Vegas papers.
*/
- diff = tp->snd_cwnd * (rtt-vegas->baseRTT) / vegas->baseRTT;
+ diff = tp->snd_cwnd * (rtt - vegas->baseRTT) / vegas->baseRTT;
if (diff > gamma && tcp_in_slow_start(tp)) {
/* Going too fast. Time to slow down
@@ -237,7 +236,7 @@ static void tcp_vegas_cong_avoid(struct sock *sk, u32 ack, u32 acked)
* truncation robs us of full link
* utilization.
*/
- tp->snd_cwnd = min(tp->snd_cwnd, (u32)target_cwnd+1);
+ tp->snd_cwnd = min(tp->snd_cwnd, (u32)target_cwnd + 1);
tp->snd_ssthresh = tcp_vegas_ssthresh(tp);
} else if (tcp_in_slow_start(tp)) {
@@ -254,8 +253,7 @@ static void tcp_vegas_cong_avoid(struct sock *sk, u32 ack, u32 acked)
* we slow down.
*/
tp->snd_cwnd--;
- tp->snd_ssthresh
- = tcp_vegas_ssthresh(tp);
+ tp->snd_ssthresh = tcp_vegas_ssthresh(tp);
} else if (diff < alpha) {
/* We don't have enough extra packets
* in the network, so speed up.
--
2.7.4
^ permalink raw reply related
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