* 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
* [PATCH bpf-next 8/8] samples/bpf: fix tcp_bpf.readme detach command
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Copy-paste, should be detach, not attach.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
samples/bpf/tcp_bpf.readme | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
index fee746621aec..78e247f62108 100644
--- a/samples/bpf/tcp_bpf.readme
+++ b/samples/bpf/tcp_bpf.readme
@@ -25,4 +25,4 @@ attached to the cgroupv2).
To remove (unattach) a socket_ops BPF program from a cgroupv2:
- bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
+ bpftool cgroup detach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 7/8] samples/bpf: add sample program that periodically dumps TCP stats
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Uses new RTT callback to dump stats every second.
$ mkdir -p /tmp/cgroupv2
$ mount -t cgroup2 none /tmp/cgroupv2
$ mkdir -p /tmp/cgroupv2/foo
$ echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
$ bpftool prog load ./tcp_dumpstats_kern.o /sys/fs/bpf/tcp_prog
$ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
$ bpftool prog tracelog
$ # run neper/netperf/etc
Used neper to compare performance with and without this program attached
and didn't see any noticeable performance impact.
Sample output:
<idle>-0 [015] ..s. 2074.128800: 0: dsack_dups=0 delivered=242526
<idle>-0 [015] ..s. 2074.128808: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [015] ..s. 2075.130133: 0: dsack_dups=0 delivered=323599
<idle>-0 [015] ..s. 2075.130138: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [005] .Ns. 2076.131440: 0: dsack_dups=0 delivered=404648
<idle>-0 [005] .Ns. 2076.131447: 0: delivered_ce=0 icsk_retransmits=0
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
samples/bpf/Makefile | 1 +
samples/bpf/tcp_dumpstats_kern.c | 65 ++++++++++++++++++++++++++++++++
2 files changed, 66 insertions(+)
create mode 100644 samples/bpf/tcp_dumpstats_kern.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 0917f8cf4fab..eaebbeead42f 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -154,6 +154,7 @@ always += tcp_iw_kern.o
always += tcp_clamp_kern.o
always += tcp_basertt_kern.o
always += tcp_tos_reflect_kern.o
+always += tcp_dumpstats_kern.o
always += xdp_redirect_kern.o
always += xdp_redirect_map_kern.o
always += xdp_redirect_cpu_kern.o
diff --git a/samples/bpf/tcp_dumpstats_kern.c b/samples/bpf/tcp_dumpstats_kern.c
new file mode 100644
index 000000000000..5d22bf61db65
--- /dev/null
+++ b/samples/bpf/tcp_dumpstats_kern.c
@@ -0,0 +1,65 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+#define INTERVAL 1000000000ULL
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+struct {
+ __u32 type;
+ __u32 map_flags;
+ int *key;
+ __u64 *value;
+} bpf_next_dump SEC(".maps") = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct bpf_tcp_sock *tcp_sk;
+ struct bpf_sock *sk;
+ __u64 *next_dump;
+ __u64 now;
+
+ switch (ctx->op) {
+ case BPF_SOCK_OPS_TCP_CONNECT_CB:
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ case BPF_SOCK_OPS_RTT_CB:
+ break;
+ default:
+ return 1;
+ }
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ next_dump = bpf_sk_storage_get(&bpf_next_dump, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!next_dump)
+ return 1;
+
+ now = bpf_ktime_get_ns();
+ if (now < *next_dump)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ *next_dump = now + INTERVAL;
+
+ bpf_printk("dsack_dups=%u delivered=%u\n",
+ tcp_sk->dsack_dups, tcp_sk->delivered);
+ bpf_printk("delivered_ce=%u icsk_retransmits=%u\n",
+ tcp_sk->delivered_ce, tcp_sk->icsk_retransmits);
+
+ return 1;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 6/8] selftests/bpf: test BPF_SOCK_OPS_RTT_CB
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Make sure the callback is invoked for syn-ack and data packet.
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/Makefile | 3 +-
tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
tools/testing/selftests/bpf/test_tcp_rtt.c | 253 ++++++++++++++++++++
3 files changed, 316 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index de1754a8f5fe..2620406a53ec 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -27,7 +27,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
test_cgroup_storage test_select_reuseport test_section_names \
test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
test_btf_dump test_cgroup_attach xdping test_sockopt test_sockopt_sk \
- test_sockopt_multi
+ test_sockopt_multi test_tcp_rtt
BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
TEST_GEN_FILES = $(BPF_OBJ_FILES)
@@ -107,6 +107,7 @@ $(OUTPUT)/test_cgroup_attach: cgroup_helpers.c
$(OUTPUT)/test_sockopt: cgroup_helpers.c
$(OUTPUT)/test_sockopt_sk: cgroup_helpers.c
$(OUTPUT)/test_sockopt_multi: cgroup_helpers.c
+$(OUTPUT)/test_tcp_rtt: cgroup_helpers.c
.PHONY: force
diff --git a/tools/testing/selftests/bpf/progs/tcp_rtt.c b/tools/testing/selftests/bpf/progs/tcp_rtt.c
new file mode 100644
index 000000000000..233bdcb1659e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tcp_rtt.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+__u32 _version SEC("version") = 1;
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+struct bpf_map_def SEC("maps") socket_storage_map = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .key_size = sizeof(int),
+ .value_size = sizeof(struct tcp_rtt_storage),
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct tcp_rtt_storage *storage;
+ struct bpf_tcp_sock *tcp_sk;
+ int op = (int) ctx->op;
+ struct bpf_sock *sk;
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!storage)
+ return 1;
+
+ if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ }
+
+ if (op != BPF_SOCK_OPS_RTT_CB)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ storage->invoked++;
+
+ storage->dsack_dups = tcp_sk->dsack_dups;
+ storage->delivered = tcp_sk->delivered;
+ storage->delivered_ce = tcp_sk->delivered_ce;
+ storage->icsk_retransmits = tcp_sk->icsk_retransmits;
+
+ return 1;
+}
diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
new file mode 100644
index 000000000000..413fd8514adc
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
@@ -0,0 +1,253 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <error.h>
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <pthread.h>
+
+#include <linux/filter.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "bpf_util.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH "/tcp_rtt"
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+static void send_byte(int fd)
+{
+ char b = 0x55;
+
+ if (write(fd, &b, sizeof(b)) != 1)
+ error(1, errno, "Failed to send single byte");
+}
+
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
+ __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
+ __u32 icsk_retransmits)
+{
+ int err = 0;
+ struct tcp_rtt_storage val;
+
+ if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
+ error(1, errno, "Failed to read socket storage");
+
+ if (val.invoked != invoked) {
+ log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
+ msg, val.invoked, invoked);
+ err++;
+ }
+
+ if (val.dsack_dups != dsack_dups) {
+ log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
+ msg, val.dsack_dups, dsack_dups);
+ err++;
+ }
+
+ if (val.delivered != delivered) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
+ msg, val.delivered, delivered);
+ err++;
+ }
+
+ if (val.delivered_ce != delivered_ce) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
+ msg, val.delivered_ce, delivered_ce);
+ err++;
+ }
+
+ if (val.icsk_retransmits != icsk_retransmits) {
+ log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
+ msg, val.icsk_retransmits, icsk_retransmits);
+ err++;
+ }
+
+ return err;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create client socket");
+ return -1;
+ }
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
+ log_err("Fail to connect to server");
+ goto out;
+ }
+
+ return fd;
+
+out:
+ close(fd);
+ return -1;
+}
+
+static int run_test(int cgroup_fd, int server_fd)
+{
+ struct bpf_prog_load_attr attr = {
+ .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+ .file = "./tcp_rtt.o",
+ .expected_attach_type = BPF_CGROUP_SOCK_OPS,
+ };
+ struct bpf_program *prog;
+ struct bpf_object *obj;
+ struct bpf_map *map;
+ int client_fd;
+ int ignored;
+ int map_fd;
+ int err;
+
+ err = bpf_prog_load_xattr(&attr, &obj, &ignored);
+ if (err) {
+ log_err("Failed to load BPF object");
+ return -1;
+ }
+
+ map = bpf_map__next(NULL, obj);
+ map_fd = bpf_map__fd(map);
+
+ prog = bpf_program__next(NULL, obj);
+ err = bpf_prog_attach(bpf_program__fd(prog), cgroup_fd,
+ BPF_CGROUP_SOCK_OPS, 0);
+ if (err) {
+ log_err("Failed to attach BPF program");
+ goto close_bpf_object;
+ }
+
+ client_fd = connect_to_server(server_fd);
+ if (client_fd < 0) {
+ err = -1;
+ goto close_bpf_object;
+ }
+
+ err += verify_sk(map_fd, client_fd, "syn-ack",
+ /*invoked=*/1,
+ /*dsack_dups=*/0,
+ /*delivered=*/1,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ send_byte(client_fd);
+
+ err += verify_sk(map_fd, client_fd, "first payload byte",
+ /*invoked=*/2,
+ /*dsack_dups=*/0,
+ /*delivered=*/2,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ close(client_fd);
+
+close_bpf_object:
+ bpf_object__close(obj);
+ return err;
+}
+
+static int start_server(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+ };
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create server socket");
+ return -1;
+ }
+
+ if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
+ log_err("Failed to bind socket");
+ close(fd);
+ return -1;
+ }
+
+ return fd;
+}
+
+static void *server_thread(void *arg)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd = *(int *)arg;
+ int client_fd;
+
+ if (listen(fd, 1) < 0)
+ error(1, errno, "Failed to listed on socket");
+
+ client_fd = accept(fd, (struct sockaddr *)&addr, &len);
+ if (client_fd < 0)
+ error(1, errno, "Failed to accept client");
+
+ if (accept(fd, (struct sockaddr *)&addr, &len) >= 0)
+ error(1, errno, "Unexpected success in second accept");
+
+ close(client_fd);
+
+ return NULL;
+}
+
+int main(int args, char **argv)
+{
+ int server_fd, cgroup_fd;
+ int err = EXIT_SUCCESS;
+ pthread_t tid;
+
+ if (setup_cgroup_environment())
+ goto cleanup_obj;
+
+ cgroup_fd = create_and_get_cgroup(CG_PATH);
+ if (cgroup_fd < 0)
+ goto cleanup_cgroup_env;
+
+ if (join_cgroup(CG_PATH))
+ goto cleanup_cgroup;
+
+ server_fd = start_server();
+ if (server_fd < 0) {
+ err = EXIT_FAILURE;
+ goto cleanup_cgroup;
+ }
+
+ pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
+
+ if (run_test(cgroup_fd, server_fd))
+ err = EXIT_FAILURE;
+
+ close(server_fd);
+
+ printf("test_sockopt_sk: %s\n",
+ err == EXIT_SUCCESS ? "PASSED" : "FAILED");
+
+cleanup_cgroup:
+ close(cgroup_fd);
+cleanup_cgroup_env:
+ cleanup_cgroup_environment();
+cleanup_obj:
+ return err;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 5/8] bpf/tools: sync bpf.h
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Sync new bpf_tcp_sock fields and new BPF_PROG_TYPE_SOCK_OPS RTT callback.
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a396b516a2b2..cecf42c871d4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1767,6 +1767,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3069,6 +3070,12 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
@@ -3311,7 +3318,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3366,6 +3374,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 4/8] bpf: add icsk_retransmits to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Add some inet_connection_sock fields to bpf_tcp_sock that might be useful
for debugging congestion control issues.
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 1 +
net/core/filter.c | 20 +++++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bfb0b1a76684..ead27aebf491 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3078,6 +3078,7 @@ struct bpf_tcp_sock {
*/
__u32 delivered; /* Total data packets delivered incl. rexmits */
__u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index 3da4b6c38b46..089aaea0ccc6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,8 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock,
+ icsk_retransmits))
return false;
if (off % size != 0)
@@ -5575,6 +5576,20 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
+#define BPF_INET_SOCK_GET_COMMON(FIELD) \
+ do { \
+ BUILD_BUG_ON(FIELD_SIZEOF(struct inet_connection_sock, \
+ FIELD) > \
+ FIELD_SIZEOF(struct bpf_tcp_sock, FIELD)); \
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \
+ struct inet_connection_sock, \
+ FIELD), \
+ si->dst_reg, si->src_reg, \
+ offsetof( \
+ struct inet_connection_sock, \
+ FIELD)); \
+ } while (0)
+
if (insn > insn_buf)
return insn - insn_buf;
@@ -5661,6 +5676,9 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, delivered_ce):
BPF_TCP_SOCK_GET_COMMON(delivered_ce);
break;
+ case offsetof(struct bpf_tcp_sock, icsk_retransmits):
+ BPF_INET_SOCK_GET_COMMON(icsk_retransmits);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 3/8] bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Add more fields to bpf_tcp_sock that might be useful for debugging
congestion control issues.
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 5 +++++
net/core/filter.c | 11 ++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9cdd0aaeba06..bfb0b1a76684 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3073,6 +3073,11 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index ad908526545d..3da4b6c38b46 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,7 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, bytes_acked))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
return false;
if (off % size != 0)
@@ -5652,6 +5652,15 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, bytes_acked):
BPF_TCP_SOCK_GET_COMMON(bytes_acked);
break;
+ case offsetof(struct bpf_tcp_sock, dsack_dups):
+ BPF_TCP_SOCK_GET_COMMON(dsack_dups);
+ break;
+ case offsetof(struct bpf_tcp_sock, delivered):
+ BPF_TCP_SOCK_GET_COMMON(delivered);
+ break;
+ case offsetof(struct bpf_tcp_sock, delivered_ce):
+ BPF_TCP_SOCK_GET_COMMON(delivered_ce);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 2/8] bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
We've added bpf_tcp_sock member to bpf_sock_ops and don't expect
any new tcp_sock fields in bpf_sock_ops. Let's remove
CONVERT_COMMON_TCP_SOCK_FIELDS so bpf_tcp_sock can be independently
extended.
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
net/core/filter.c | 180 ++++++++++++++++++++++++++++++++--------------
1 file changed, 126 insertions(+), 54 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 4836264f82ee..ad908526545d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5194,54 +5194,6 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
};
#endif /* CONFIG_IPV6_SEG6_BPF */
-#define CONVERT_COMMON_TCP_SOCK_FIELDS(md_type, CONVERT) \
-do { \
- switch (si->off) { \
- case offsetof(md_type, snd_cwnd): \
- CONVERT(snd_cwnd); break; \
- case offsetof(md_type, srtt_us): \
- CONVERT(srtt_us); break; \
- case offsetof(md_type, snd_ssthresh): \
- CONVERT(snd_ssthresh); break; \
- case offsetof(md_type, rcv_nxt): \
- CONVERT(rcv_nxt); break; \
- case offsetof(md_type, snd_nxt): \
- CONVERT(snd_nxt); break; \
- case offsetof(md_type, snd_una): \
- CONVERT(snd_una); break; \
- case offsetof(md_type, mss_cache): \
- CONVERT(mss_cache); break; \
- case offsetof(md_type, ecn_flags): \
- CONVERT(ecn_flags); break; \
- case offsetof(md_type, rate_delivered): \
- CONVERT(rate_delivered); break; \
- case offsetof(md_type, rate_interval_us): \
- CONVERT(rate_interval_us); break; \
- case offsetof(md_type, packets_out): \
- CONVERT(packets_out); break; \
- case offsetof(md_type, retrans_out): \
- CONVERT(retrans_out); break; \
- case offsetof(md_type, total_retrans): \
- CONVERT(total_retrans); break; \
- case offsetof(md_type, segs_in): \
- CONVERT(segs_in); break; \
- case offsetof(md_type, data_segs_in): \
- CONVERT(data_segs_in); break; \
- case offsetof(md_type, segs_out): \
- CONVERT(segs_out); break; \
- case offsetof(md_type, data_segs_out): \
- CONVERT(data_segs_out); break; \
- case offsetof(md_type, lost_out): \
- CONVERT(lost_out); break; \
- case offsetof(md_type, sacked_out): \
- CONVERT(sacked_out); break; \
- case offsetof(md_type, bytes_received): \
- CONVERT(bytes_received); break; \
- case offsetof(md_type, bytes_acked): \
- CONVERT(bytes_acked); break; \
- } \
-} while (0)
-
#ifdef CONFIG_INET
static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
int dif, int sdif, u8 family, u8 proto)
@@ -5623,9 +5575,6 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
- CONVERT_COMMON_TCP_SOCK_FIELDS(struct bpf_tcp_sock,
- BPF_TCP_SOCK_GET_COMMON);
-
if (insn > insn_buf)
return insn - insn_buf;
@@ -5640,6 +5589,69 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, rtt_min) +
offsetof(struct minmax_sample, v));
break;
+ case offsetof(struct bpf_tcp_sock, snd_cwnd):
+ BPF_TCP_SOCK_GET_COMMON(snd_cwnd);
+ break;
+ case offsetof(struct bpf_tcp_sock, srtt_us):
+ BPF_TCP_SOCK_GET_COMMON(srtt_us);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_ssthresh):
+ BPF_TCP_SOCK_GET_COMMON(snd_ssthresh);
+ break;
+ case offsetof(struct bpf_tcp_sock, rcv_nxt):
+ BPF_TCP_SOCK_GET_COMMON(rcv_nxt);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_nxt):
+ BPF_TCP_SOCK_GET_COMMON(snd_nxt);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_una):
+ BPF_TCP_SOCK_GET_COMMON(snd_una);
+ break;
+ case offsetof(struct bpf_tcp_sock, mss_cache):
+ BPF_TCP_SOCK_GET_COMMON(mss_cache);
+ break;
+ case offsetof(struct bpf_tcp_sock, ecn_flags):
+ BPF_TCP_SOCK_GET_COMMON(ecn_flags);
+ break;
+ case offsetof(struct bpf_tcp_sock, rate_delivered):
+ BPF_TCP_SOCK_GET_COMMON(rate_delivered);
+ break;
+ case offsetof(struct bpf_tcp_sock, rate_interval_us):
+ BPF_TCP_SOCK_GET_COMMON(rate_interval_us);
+ break;
+ case offsetof(struct bpf_tcp_sock, packets_out):
+ BPF_TCP_SOCK_GET_COMMON(packets_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, retrans_out):
+ BPF_TCP_SOCK_GET_COMMON(retrans_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, total_retrans):
+ BPF_TCP_SOCK_GET_COMMON(total_retrans);
+ break;
+ case offsetof(struct bpf_tcp_sock, segs_in):
+ BPF_TCP_SOCK_GET_COMMON(segs_in);
+ break;
+ case offsetof(struct bpf_tcp_sock, data_segs_in):
+ BPF_TCP_SOCK_GET_COMMON(data_segs_in);
+ break;
+ case offsetof(struct bpf_tcp_sock, segs_out):
+ BPF_TCP_SOCK_GET_COMMON(segs_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, data_segs_out):
+ BPF_TCP_SOCK_GET_COMMON(data_segs_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, lost_out):
+ BPF_TCP_SOCK_GET_COMMON(lost_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, sacked_out):
+ BPF_TCP_SOCK_GET_COMMON(sacked_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, bytes_received):
+ BPF_TCP_SOCK_GET_COMMON(bytes_received);
+ break;
+ case offsetof(struct bpf_tcp_sock, bytes_acked):
+ BPF_TCP_SOCK_GET_COMMON(bytes_acked);
+ break;
}
return insn - insn_buf;
@@ -7913,9 +7925,6 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
SOCK_OPS_GET_FIELD(BPF_FIELD, OBJ_FIELD, OBJ); \
} while (0)
- CONVERT_COMMON_TCP_SOCK_FIELDS(struct bpf_sock_ops,
- SOCK_OPS_GET_TCP_SOCK_FIELD);
-
if (insn > insn_buf)
return insn - insn_buf;
@@ -8085,6 +8094,69 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
SOCK_OPS_GET_OR_SET_FIELD(sk_txhash, sk_txhash,
struct sock, type);
break;
+ case offsetof(struct bpf_sock_ops, snd_cwnd):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_cwnd);
+ break;
+ case offsetof(struct bpf_sock_ops, srtt_us):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(srtt_us);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_ssthresh):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_ssthresh);
+ break;
+ case offsetof(struct bpf_sock_ops, rcv_nxt):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rcv_nxt);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_nxt):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_nxt);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_una):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_una);
+ break;
+ case offsetof(struct bpf_sock_ops, mss_cache):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(mss_cache);
+ break;
+ case offsetof(struct bpf_sock_ops, ecn_flags):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(ecn_flags);
+ break;
+ case offsetof(struct bpf_sock_ops, rate_delivered):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rate_delivered);
+ break;
+ case offsetof(struct bpf_sock_ops, rate_interval_us):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rate_interval_us);
+ break;
+ case offsetof(struct bpf_sock_ops, packets_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(packets_out);
+ break;
+ case offsetof(struct bpf_sock_ops, retrans_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(retrans_out);
+ break;
+ case offsetof(struct bpf_sock_ops, total_retrans):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(total_retrans);
+ break;
+ case offsetof(struct bpf_sock_ops, segs_in):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(segs_in);
+ break;
+ case offsetof(struct bpf_sock_ops, data_segs_in):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(data_segs_in);
+ break;
+ case offsetof(struct bpf_sock_ops, segs_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(segs_out);
+ break;
+ case offsetof(struct bpf_sock_ops, data_segs_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(data_segs_out);
+ break;
+ case offsetof(struct bpf_sock_ops, lost_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(lost_out);
+ break;
+ case offsetof(struct bpf_sock_ops, sacked_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(sacked_out);
+ break;
+ case offsetof(struct bpf_sock_ops, bytes_received):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(bytes_received);
+ break;
+ case offsetof(struct bpf_sock_ops, bytes_acked):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(bytes_acked);
+ break;
case offsetof(struct bpf_sock_ops, sk):
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
struct bpf_sock_ops_kern,
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 1/8] bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190701204821.44230-1-sdf@google.com>
Performance impact should be minimal because it's under a new
BPF_SOCK_OPS_RTT_CB_FLAG flag that has to be explicitly enabled.
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>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/net/tcp.h | 8 ++++++++
include/uapi/linux/bpf.h | 6 +++++-
net/ipv4/tcp_input.c | 4 ++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 9d36cc88d043..e16d8a3fd3b4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2221,6 +2221,14 @@ static inline bool tcp_bpf_ca_needs_ecn(struct sock *sk)
return (tcp_call_bpf(sk, BPF_SOCK_OPS_NEEDS_ECN, 0, NULL) == 1);
}
+static inline void tcp_bpf_rtt(struct sock *sk)
+{
+ struct tcp_sock *tp = tcp_sk(sk);
+
+ if (BPF_SOCK_OPS_TEST_FLAG(tp, BPF_SOCK_OPS_RTT_CB_FLAG))
+ tcp_call_bpf(sk, BPF_SOCK_OPS_RTT_CB, 0, NULL);
+}
+
#if IS_ENABLED(CONFIG_SMC)
extern struct static_key_false tcp_have_smc;
#endif
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index cffea1826a1f..9cdd0aaeba06 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1770,6 +1770,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3314,7 +3315,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3369,6 +3371,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index b71efeb0ae5b..c21e8a22fb3b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -778,6 +778,8 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
tp->rttvar_us -= (tp->rttvar_us - tp->mdev_max_us) >> 2;
tp->rtt_seq = tp->snd_nxt;
tp->mdev_max_us = tcp_rto_min_us(sk);
+
+ tcp_bpf_rtt(sk);
}
} else {
/* no previous measure. */
@@ -786,6 +788,8 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
tp->rttvar_us = max(tp->mdev_us, tcp_rto_min_us(sk));
tp->mdev_max_us = tp->rttvar_us;
tp->rtt_seq = tp->snd_nxt;
+
+ tcp_bpf_rtt(sk);
}
tp->srtt_us = max(1U, srtt);
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next 0/8] bpf: TCP RTT sock_ops bpf callback
From: Stanislav Fomichev @ 2019-07-01 20:48 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
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>
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 20:41 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <505e9af7-a0cd-bf75-4a72-5d883ee06bf1@oracle.com>
On 7/1/19 9:39 AM, Gerd Rausch wrote:
> In order to:
> 1) avoid a silly bouncing between "clean_list" and "drop_list"
> triggered by function "rds_ib_reg_frmr" as it is releases frmr
> regions whose state is not "FRMR_IS_FREE" right away.
>
> 2) prevent an invalid access error in a race from a pending
> "IB_WR_LOCAL_INV" operation with a teardown ("dma_unmap_sg", "put_page")
> and de-registration ("ib_dereg_mr") of the corresponding
> memory region.
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
> net/rds/ib_frmr.c | 89 ++++++++++++++++++++++++++++++-----------------
> net/rds/ib_mr.h | 2 ++
> 2 files changed, 59 insertions(+), 32 deletions(-)
>
> diff --git a/net/rds/ib_frmr.c b/net/rds/ib_frmr.c
> index 9f8aa310c27a..3c953034dca3 100644
> --- a/net/rds/ib_frmr.c
> +++ b/net/rds/ib_frmr.c
> @@ -76,6 +76,7 @@ static struct rds_ib_mr *rds_ib_alloc_frmr(struct rds_ib_device *rds_ibdev,
>
> frmr->fr_state = FRMR_IS_FREE;
> init_waitqueue_head(&frmr->fr_inv_done);
> + init_waitqueue_head(&frmr->fr_reg_done);
> return ibmr;
>
> out_no_cigar:
> @@ -124,6 +125,7 @@ static int rds_ib_post_reg_frmr(struct rds_ib_mr *ibmr)
> */
> ib_update_fast_reg_key(frmr->mr, ibmr->remap_count++);
> frmr->fr_state = FRMR_IS_INUSE;
> + frmr->fr_reg = true;
>
> memset(®_wr, 0, sizeof(reg_wr));
> reg_wr.wr.wr_id = (unsigned long)(void *)ibmr;
> @@ -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 ?
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.
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net-next] r8169: fix ntohs/htons sparse warnings
From: Heiner Kallweit @ 2019-07-01 20:36 UTC (permalink / raw)
To: Al Viro; +Cc: Realtek linux nic maintainers, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190701195621.GC17978@ZenIV.linux.org.uk>
On 01.07.2019 21:56, Al Viro wrote:
> On Mon, Jul 01, 2019 at 09:35:28PM +0200, Heiner Kallweit wrote:
>> Sparse complains about casting to/from restricted __be16. Fix this.
>
> Fix what, exactly? Force-cast is not a fix - it's "STFU, I know
> better, it's really correct" to sparse. Which may or may not
> match the reality, but it definitely requires more in way of
> commit message than "sparse says it's wrong; shut it up".
>
>> static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>> @@ -1537,7 +1537,7 @@ static void rtl8169_rx_vlan_tag(struct RxDesc *desc, struct sk_buff *skb)
>>
>> if (opts2 & RxVlanTag)
>> __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
>> - ntohs(opts2 & 0xffff));
>> + ntohs((__force __be16)(opts2 & 0xffff)));
>> }
>
> Should that be ntohs at all? What behaviour is correct on big-endian host?
>
> AFAICS, in that code opts2 comes from little-endian 32bit. It's converted to
> host-endian, lower 16 bits (i.e. the first two octets in memory) are then
> fed to ntohs. Suppose we had in-core value stored as A0, A1, A2, A3.
> On little-endian that code will yield A0 * 256 + A1, treated as host-endian.
> On big-endian the same will yield A1 * 256 + A0. Is that actually correct?
>
I think you're right and the original patch should be reverted.
> 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.
^ permalink raw reply
* Re: [PATCH net-next 2/7] net/rds: Get rid of "wait_clean_list_grace" and add locking
From: santosh.shilimkar @ 2019-07-01 20:33 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <5c49f180-0dbf-88b9-965d-6cb88061f31b@oracle.com>
On 7/1/19 9:39 AM, Gerd Rausch wrote:
> Waiting for activity on the "clean_list" to quiesce is no substitute
> for proper locking.
>
> We can have multiple threads competing for "llist_del_first"
> via "rds_ib_reuse_mr", and a single thread competing
> for "llist_del_all" and "llist_del_first" via "rds_ib_flush_mr_pool".
>
> Since "llist_del_first" depends on "list->first->next" not to change
> in the midst of the operation, simply waiting for all current calls
> to "rds_ib_reuse_mr" to quiesce across all CPUs is woefully inadequate:
>
> By the time "wait_clean_list_grace" is done iterating over all CPUs to see
> that there is no concurrent caller to "rds_ib_reuse_mr", a new caller may
> have just shown up on the first CPU.
>
> Furthermore, <linux/llist.h> explicitly calls out the need for locking:
> * Cases where locking is needed:
> * If we have multiple consumers with llist_del_first used in one consumer,
> * and llist_del_first or llist_del_all used in other consumers,
> * then a lock is needed.
>
> Also, while at it, drop the unused "pool" parameter
> from "list_to_llist_nodes".
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good.
^ permalink raw reply
* Re: [PATCH 1/4] net: dsa: Change DT bindings for Vitesse VSC73xx switches
From: Linus Walleij @ 2019-07-01 20:23 UTC (permalink / raw)
To: Florian Fainelli
Cc: Pawel Dembicki, Andrew Lunn, Vivien Didelot, David S. Miller,
Rob Herring, Mark Rutland, netdev,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org
In-Reply-To: <45ff597a-5090-3874-b43d-5b5f45d2d2f6@gmail.com>
On Mon, Jul 1, 2019 at 6:44 PM Florian Fainelli <f.fainelli@gmail.com> wrote:
> Take b53 for instance which supports MDIO and SPI by default, and
> optionally memory mapped and SRAB (indirect memory map) accesses, they
> all have the same compatible strings. Whether the switches will appear
> as spi_device, platform_device, or something else is entirely based on
> how the Device Tree is laid out.
That's clever.
Pawel can you restructure the series around this observation?
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH net-next 4/7] net/rds: Fix NULL/ERR_PTR inconsistency
From: santosh.shilimkar @ 2019-07-01 20:14 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <9f46098a-bcc7-bc0e-20db-2cbf05fefdee@oracle.com>
On 7/1/19 9:39 AM, Gerd Rausch wrote:
> Make function "rds_ib_try_reuse_ibmr" return NULL in case
> memory region could not be allocated, since callers
> simply check if the return value is not NULL.
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good to me. Will add this to other fixes.
^ permalink raw reply
* Re: [PATCH net-next 7/7] net/rds: Initialize ic->i_fastreg_wrs upon allocation
From: santosh.shilimkar @ 2019-07-01 20:15 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <a97a92d4-c443-12e1-6d82-1646f9584828@oracle.com>
On 7/1/19 9:40 AM, Gerd Rausch wrote:
> Otherwise, if an IB connection is torn down before "rds_ib_setup_qp"
> is called, the value of "ic->i_fastreg_wrs" is still at zero
> (as it wasn't initialized by "rds_ib_setup_qp").
> Consequently "rds_ib_conn_path_shutdown" will spin forever,
> waiting for it to go back to "RDS_IB_DEFAULT_FR_WR",
> which of course will never happen as there are no
> outstanding work requests.
>
> Signed-off-by: Gerd Rausch <gerd.rausch@oracle.com>
> ---
Looks good to me. Will add this to other fixes.
^ 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