* [PATCH v4 bpf-next] bpf: Add support for fq's EDT to HBM
From: brakmo @ 2019-07-02 22:09 UTC (permalink / raw)
To: netdev
Cc: Alexei Starovoitov, Yonghong Song, Daniel Borkmann, Eric Dumazet,
Kernel Team
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
v2 -> v3: Incorporated Yonghong's suggestions
v3 -> v4: Removed credit update that is not needed
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 | 168 +++++++++++++++++++++++++++++++++++++
samples/bpf/hbm_kern.h | 40 +++++++--
5 files changed, 231 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..a65b677acdb0
--- /dev/null
+++ b/samples/bpf/hbm_edt_kern.c
@@ -0,0 +1,168 @@
+// 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)
+{
+ long long delta = 0, delta_send;
+ 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);
+
+ // Check if we should ignore loopback traffic
+ if (qsp != NULL && !qsp->loopback && (skb->ifindex == 1))
+ 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);
+ __sync_add_and_fetch(&(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;
+
+ // 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->lasttime), -delta_send);
+ 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,
--
2.17.1
^ permalink raw reply related
* 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-02 21:18 UTC (permalink / raw)
To: Gerd Rausch, netdev; +Cc: David Miller
In-Reply-To: <697adfba-ac8b-db4d-5819-f4c22ec6c76a@oracle.com>
On 7/2/19 2:05 PM, Gerd Rausch wrote:
> On 02/07/2019 09.49, santosh.shilimkar@oracle.com wrote:
>> On 7/1/19 10:11 PM, Gerd Rausch wrote:
>>> For the registration work-requests there is a benefit to wait a short
>>> amount of time only (the trade-off described in patch #1 of this series).
>>>
>> Actually we should just switch this code to what Avinash has
>> finally made in downstream code. That keeps the RDS_GET_MR
>> semantics and makes sure MR is really valid before handing over
>> the key to userland. There is no need for any timeout
>> for registration case.
>>
>
> What do you call "RDS_GET_MR" semantics?
>
Its a blocking socket call. Meaning after this call return to the
user, the key must be valid. With async registration that can't be
guaranteed.
> The purpose of waiting for a IB_WR_REG_MR request to complete
> (inside rds_ib_post_reg_frmr) is in fact to make sure
> the memory region is valid.
>
> Regardless of this being true after a specific time-out,
> or an infinite timeout.
>
> For the non-infinite time-out case, there is a check if the request
> was handled by the firmware.
>
> And if a time-out occurred and the firmware didn't handle the request,
> function "rds_ib_post_reg_frmr" will return -EBUSY.
>
>>> Actually, no:
>>> Socket option RDS_GET_MR wasn't even in the code-path of the
>>> tests I performed:
>>>
>>> It were there RDS_CMSG_RDMA_MAP / RDS_CMSG_RDMA_DEST control
>>> messages that ended up calling '__rds_rdma_map".
>>>
>> What option did you use ? Default option with rds-stress is
>> RDS_GET_MR and hence the question.
>>
>
> Not true!:
Its other way round. Thanks for info so default its using inline
registration instead of explicit call.
> How is socket option RDS_GET_MR special with regards to this proposed fix?
>
>>> I don't understand, please elaborate:
>>> a) Are you saying this issue should not be fixed?
>>> b) Or are you suggesting to replace this fix with a different fix?
>>> If it's the later, please point out what you have in mind.
>>> c) ???
>>>
>> All am saying is the code got changed for good reason and that changed
>> code makes some of these race conditions possibly not applicable.
>
> I don't understand this. Please elaborate.
>
>> So instead of these timeout fixes, am suggesting to use that
>> code as fix. At least test it with those changes and see whats
>> the behavior.
>>
>
> Are you suggesting to
> a) Not fix this bug right now and wait until some later point in time
When did I say that ? I said have you explored alternate approach to
fix the issue and if not could you try it out.
> b) Use a different fix. If you've got a different fix, please share.
>
I don't but its a review of the fix and possible alternate needs to
be discussed. It is not like take my fix or provide an alternate fix.
> And besides these options, is there anything wrong with this fix
> (other than the discussion of what the timeout value ought to be,
> which we can address)?
>
That timeout is a problem because it doesn't guarantee the failure
of operation since its an asyn operation for registration. Instead
of timing out if you poll the CQ for that operation completion, it
makes it full proof. That is the change Avinash has done iirc and
am requesting to look at that fix.
Other 5 fixes from the series looks fine.
Regards,
Santosh
Regards,
Santosh
^ permalink raw reply
* Re: [PATCH net 2/2] macsec: fix checksumming after decryption
From: Andreas Steinmetz @ 2019-07-02 19:47 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: Network Development, Sabrina Dubroca
In-Reply-To: <CAF=yD-LBRZjns1x9_UrhBYZGX8JNeM+r-cYJV=eyYKDTSG8rBQ@mail.gmail.com>
On Tue, 2019-07-02 at 10:35 -0400, Willem de Bruijn wrote:
> On Tue, Jul 2, 2019 at 12:25 AM Andreas Steinmetz <ast@domdv.de> wrote:
> > On Sun, 2019-06-30 at 21:47 -0400, Willem de Bruijn wrote:
> > > On Sun, Jun 30, 2019 at 4:48 PM Andreas Steinmetz <ast@domdv.de>
> > > wrote:
> > > > Fix checksumming after decryption.
> > > >
> > > > Signed-off-by: Andreas Steinmetz <ast@domdv.de>
> > > >
> > > > --- a/drivers/net/macsec.c 2019-06-30 22:14:10.250285314 +0200
> > > > +++ b/drivers/net/macsec.c 2019-06-30 22:15:11.931230417 +0200
> > > > @@ -869,6 +869,7 @@
> > > >
> > > > static void macsec_finalize_skb(struct sk_buff *skb, u8 icv_len,
> > > > u8 hdr_len)
> > > > {
> > > > + skb->ip_summed = CHECKSUM_NONE;
> > > > memmove(skb->data + hdr_len, skb->data, 2 * ETH_ALEN);
> > > > skb_pull(skb, hdr_len);
> > > > pskb_trim_unique(skb, skb->len - icv_len);
> > >
> > > Does this belong in macset_reset_skb?
> >
> > Putting this in macsec_reset_skb would then miss out the "nosci:" part
> > of the RX path in macsec_handle_frame().
>
> It is called on each nskb before calling netif_rx.
>
> It indeed is not called when returning RX_HANDLER_PASS, but that is correct?
This is correct. Packets passed on with RX_HANDLER_PASS are either not for this
driver or the special case of being destined for a KaY and in this case being
the MACsec ethernet protocol and thus not IP, so no checksumming.
^ permalink raw reply
* [PATCH net-next] skbuff: increase verbosity when dumping skb data
From: Willem de Bruijn @ 2019-07-02 19:39 UTC (permalink / raw)
To: netdev
Cc: davem, xiyou.wangcong, herbert, eric.dumazet, saeedm,
Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
skb_warn_bad_offload and netdev_rx_csum_fault trigger on hard to debug
issues. Dump more state and the header.
Optionally dump the entire packet and linear segment. This is required
to debug checksum bugs that may include bytes past skb_tail_pointer().
Both call sites call this function inside a net_ratelimit() block.
Limit full packet log further to a hard limit of can_dump_full (5).
Based on an earlier patch by Cong Wang, see link below.
Link: https://patchwork.ozlabs.org/patch/1000841/
Signed-off-by: Willem de Bruijn <willemb@google.com>
---
include/linux/skbuff.h | 1 +
net/core/dev.c | 16 ++-----
net/core/skbuff.c | 103 +++++++++++++++++++++++++++++++++++++++++
3 files changed, 108 insertions(+), 12 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index b5d427b149c92..48b08549a8b78 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1024,6 +1024,7 @@ static inline bool skb_unref(struct sk_buff *skb)
void skb_release_head_state(struct sk_buff *skb);
void kfree_skb(struct sk_buff *skb);
void kfree_skb_list(struct sk_buff *segs);
+void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt);
void skb_tx_error(struct sk_buff *skb);
void consume_skb(struct sk_buff *skb);
void __consume_stateless_skb(struct sk_buff *skb);
diff --git a/net/core/dev.c b/net/core/dev.c
index 58529318b3a94..fc676b2610e3c 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2900,12 +2900,10 @@ static void skb_warn_bad_offload(const struct sk_buff *skb)
else
name = netdev_name(dev);
}
- WARN(1, "%s: caps=(%pNF, %pNF) len=%d data_len=%d gso_size=%d "
- "gso_type=%d ip_summed=%d\n",
+ skb_dump(KERN_WARNING, skb, false);
+ WARN(1, "%s: caps=(%pNF, %pNF)\n",
name, dev ? &dev->features : &null_features,
- skb->sk ? &skb->sk->sk_route_caps : &null_features,
- skb->len, skb->data_len, skb_shinfo(skb)->gso_size,
- skb_shinfo(skb)->gso_type, skb->ip_summed);
+ skb->sk ? &skb->sk->sk_route_caps : &null_features);
}
/*
@@ -3124,13 +3122,7 @@ void netdev_rx_csum_fault(struct net_device *dev, struct sk_buff *skb)
{
if (net_ratelimit()) {
pr_err("%s: hw csum failure\n", dev ? dev->name : "<unknown>");
- if (dev)
- pr_err("dev features: %pNF\n", &dev->features);
- pr_err("skb len=%u data_len=%u pkt_type=%u gso_size=%u gso_type=%u nr_frags=%u ip_summed=%u csum=%x csum_complete_sw=%d csum_valid=%d csum_level=%u\n",
- skb->len, skb->data_len, skb->pkt_type,
- skb_shinfo(skb)->gso_size, skb_shinfo(skb)->gso_type,
- skb_shinfo(skb)->nr_frags, skb->ip_summed, skb->csum,
- skb->csum_complete_sw, skb->csum_valid, skb->csum_level);
+ skb_dump(KERN_ERR, skb, true);
dump_stack();
}
}
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index 5323441a12ccf..5d501066d00ca 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -707,6 +707,109 @@ void kfree_skb_list(struct sk_buff *segs)
}
EXPORT_SYMBOL(kfree_skb_list);
+/* Dump skb information and contents.
+ *
+ * Must only be called from net_ratelimit()-ed paths.
+ *
+ * Dumps up to can_dump_full whole packets if full_pkt, headers otherwise.
+ */
+void skb_dump(const char *level, const struct sk_buff *skb, bool full_pkt)
+{
+ static atomic_t can_dump_full = ATOMIC_INIT(5);
+ struct skb_shared_info *sh = skb_shinfo(skb);
+ struct net_device *dev = skb->dev;
+ struct sock *sk = skb->sk;
+ struct sk_buff *list_skb;
+ bool has_mac, has_trans;
+ int headroom, tailroom;
+ int i, len, seg_len;
+
+ if (full_pkt)
+ full_pkt = atomic_dec_if_positive(&can_dump_full) >= 0;
+
+ if (full_pkt)
+ len = skb->len;
+ else
+ len = min_t(int, skb->len, MAX_HEADER + 128);
+
+ headroom = skb_headroom(skb);
+ tailroom = skb_tailroom(skb);
+
+ has_mac = skb_mac_header_was_set(skb);
+ has_trans = skb_transport_header_was_set(skb);
+
+ printk("%sskb len=%u headroom=%u headlen=%u tailroom=%u\n"
+ "mac=(%d,%d) net=(%d,%d) trans=%d\n"
+ "shinfo(txflags=%u nr_frags=%u gso(size=%hu type=%u segs=%hu))\n"
+ "csum(0x%x ip_summed=%u complete_sw=%u valid=%u level=%u)\n"
+ "hash(0x%x sw=%u l4=%u) proto=0x%04x pkttype=%u iif=%d\n",
+ level, skb->len, headroom, skb_headlen(skb), tailroom,
+ has_mac ? skb->mac_header : -1,
+ has_mac ? skb_mac_header_len(skb) : -1,
+ skb->network_header,
+ has_trans ? skb_network_header_len(skb) : -1,
+ has_trans ? skb->transport_header : -1,
+ sh->tx_flags, sh->nr_frags,
+ sh->gso_size, sh->gso_type, sh->gso_segs,
+ skb->csum, skb->ip_summed, skb->csum_complete_sw,
+ skb->csum_valid, skb->csum_level,
+ skb->hash, skb->sw_hash, skb->l4_hash,
+ ntohs(skb->protocol), skb->pkt_type, skb->skb_iif);
+
+ if (dev)
+ printk("%sdev name=%s feat=0x%pNF\n",
+ level, dev->name, &dev->features);
+ if (sk)
+ printk("%ssk family=%hu type=%hu proto=%hu\n",
+ level, sk->sk_family, sk->sk_type, sk->sk_protocol);
+
+ if (full_pkt && headroom)
+ print_hex_dump(level, "skb headroom: ", DUMP_PREFIX_OFFSET,
+ 16, 1, skb->head, headroom, false);
+
+ seg_len = min_t(int, skb_headlen(skb), len);
+ if (seg_len)
+ print_hex_dump(level, "skb linear: ", DUMP_PREFIX_OFFSET,
+ 16, 1, skb->data, seg_len, false);
+ len -= seg_len;
+
+ if (full_pkt && tailroom)
+ print_hex_dump(level, "skb tailroom: ", DUMP_PREFIX_OFFSET,
+ 16, 1, skb_tail_pointer(skb), tailroom, false);
+
+ for (i = 0; len && i < skb_shinfo(skb)->nr_frags; i++) {
+ skb_frag_t *frag = &skb_shinfo(skb)->frags[i];
+ u32 p_off, p_len, copied;
+ struct page *p;
+ u8 *vaddr;
+
+ skb_frag_foreach_page(frag, frag->page_offset,
+ skb_frag_size(frag), p, p_off, p_len,
+ copied) {
+ seg_len = min_t(int, p_len, len);
+ vaddr = kmap_atomic(p);
+ print_hex_dump(level, "skb frag: ",
+ DUMP_PREFIX_OFFSET,
+ 16, 1, vaddr + p_off, seg_len, false);
+ kunmap_atomic(vaddr);
+ len -= seg_len;
+ if (!len)
+ break;
+ }
+ }
+
+ if (len && skb_has_frag_list(skb)) {
+ printk("skb fraglist:\n");
+ skb_walk_frags(skb, list_skb) {
+ if (len <= 0)
+ break;
+ skb_dump(level, list_skb, len);
+ len -= list_skb->len;
+ }
+ }
+}
+EXPORT_SYMBOL(skb_dump);
+
/**
* skb_tx_error - report an sk_buff xmit error
* @skb: buffer that triggered an error
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* Re: veth pair ping fail if one of them enslaved into a VRF
From: Zoltán Elek @ 2019-07-02 19:36 UTC (permalink / raw)
To: Ido Schimmel; +Cc: netdev, dsa
In-Reply-To: <20190702192122.GA16784@splinter>
Thank you for the detailed answer, I tried your solution and it works
as expected.
Ido Schimmel <idosch@idosch.org> ezt írta (időpont: 2019. júl. 2., K, 21:21):
>
> On Tue, Jul 02, 2019 at 08:42:15PM +0200, Zoltán Elek wrote:
> > Hi!
> >
> > I have a simple scenario, with a veth pair, IP addresses assigned from
> > the same subnet. They can ping eachother. But when I put one of them
> > into a VRF (in the example below, I put veth in-vrf into the test-vrf
> > VRF) the ping fails. My first question: that is the expected behavior?
> > And my second question: is there any way to overcome this?
> >
> > Here are my test commands:
> > ip link add out-of-vrf type veth peer name in-vrf
> > ip link set dev out-of-vrf up
> > ip link set dev in-vrf up
> > ip link add test-vrf type vrf table 10
> > ip link set dev test-vrf up
> > ip -4 addr add 100.127.253.2/24 dev in-vrf
> > ip -4 addr add 100.127.253.1/24 dev out-of-vrf
> >
> > Then ping works as expected:
> > ping -c1 -I 100.127.253.1 100.127.253.2
> >
> > After I put the in-vrf into test-vrf, ping fails:
> > ip link set in-vrf vrf test-vrf up
>
> You need to re-order the FIB rules so that lookup for 100.127.253.1
> happens in table 10 and not in the local table:
>
> # ip -4 rule add pref 32765 table local
> # ip -4 rule del pref 0
> # ip -4 rule show
> 1000: from all lookup [l3mdev-table]
> 32765: from all lookup local
> 32766: from all lookup main
> 32767: from all lookup default
>
> Bad:
>
> ping 16735 [001] 13726.398115: fib:fib_table_lookup: table 255 oif 0 iif
> 9 proto 0 100.127.253.2/0 -> 100.127.253.1/0 tos 0 scope 0 flags 4 ==>
> dev out-of-vrf gw 0.0.0.0 src 100.127.253.1 err 0
>
> Good:
>
> ping 16665 [001] 13500.937145: fib:fib_table_lookup: table 10 oif 0 iif
> 9 proto 0 100.127.253.2/0 -> 100.127.253.1/0 tos 0 scope 0 flags 4 ==>
> dev in-vrf gw 0.0.0.0 src 100.127.253.2 err 0
>
> >
> > Thanks,
> > Zoltan Elek,
> > VI1
^ permalink raw reply
* Re: [PATCH next] sysctl: add proc_dointvec_jiffies_minmax to limit the min/max write value
From: Luis Chamberlain @ 2019-07-02 19:32 UTC (permalink / raw)
To: Zhiqiang Liu
Cc: Kees Cook, akpm, linux-kernel, linux-fsdevel, ebiederm, pbonzini,
viro, adobriyan, mingfangsen, wangxiaogang3, Zhoukang (A), netdev
In-Reply-To: <dd40ae3b-8e0a-2d55-d402-6f261a6c0e09@huawei.com>
On Tue, Jun 04, 2019 at 11:27:51PM +0800, Zhiqiang Liu wrote:
> > On Wed, May 15, 2019 at 10:53:55PM +0800, Zhiqiang Liu wrote:
> >>>> In proc_dointvec_jiffies func, the write value is only checked
> >>>> whether it is larger than INT_MAX. If the write value is less
> >>>> than zero, it can also be successfully writen in the data.
> >
> > This appears to be "be design", but I see many "unsigned int" users
> > that might be tricked into giant values... (for example, see
> > net/netfilter/nf_conntrack_standalone.c)
> >
> > Should proc_dointvec_jiffies() just be fixed to disallow negative values
> > entirely? Looking at the implementation, it seems to be very intentional
> > about accepting negative values.
> >
> > However, when I looked through a handful of proc_dointvec_jiffies()
> > users, it looks like they're all expecting a positive value. Many in the
> > networking subsystem are, in fact, writing to unsigned long variables,
> > as I mentioned.
> >
> I totally agree with you. And I also cannot find an scenario that expects
> negative values. Consideing the "negative" scenario may be exist, I add the
> proc_dointvec_jiffies_minmax like proc_dointvec_minmax.
If no negative values exist, and there is no real point to it, then just
rename the existing one and update the docs.
Luis
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] libbpf: capture value in BTF type info for BTF-defined map defs
From: Andrii Nakryiko @ 2019-07-02 19:29 UTC (permalink / raw)
To: Y Song
Cc: Andrii Nakryiko, Alexei Starovoitov, Daniel Borkmann, bpf, netdev,
Song Liu
In-Reply-To: <CAH3MdRXm9EnP-jqLQb-1o7KhtE2Dd3QWhPDF3SHou_At19u-CQ@mail.gmail.com>
On Tue, Jul 2, 2019 at 12:19 PM Y Song <ys114321@gmail.com> wrote:
>
> On Fri, Jun 28, 2019 at 8:26 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > Change BTF-defined map definitions to capture compile-time integer
> > values as part of BTF type definition, to avoid split of key/value type
> > information and actual type/size/flags initialization for maps.
> >
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> > ---
> > tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++----------------------
> > 1 file changed, 28 insertions(+), 30 deletions(-)
> >
> > diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> > index 6e6ebef11ba3..9e099ecb2c2b 100644
> > --- a/tools/lib/bpf/libbpf.c
> > +++ b/tools/lib/bpf/libbpf.c
> > @@ -1028,40 +1028,40 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> > }
> > }
> >
> > -static bool get_map_field_int(const char *map_name,
> > - const struct btf *btf,
> > +/*
> > + * Fetch integer attribute of BTF map definition. Such attributes are
> > + * represented using a pointer to an array, in which dimensionality of array
> > + * encodes specified integer value. E.g., int (*type)[BPF_MAP_TYPE_ARRAY];
> > + * encodes `type => BPF_MAP_TYPE_ARRAY` key/value pair completely using BTF
> > + * type definition, while using only sizeof(void *) space in ELF data section.
> > + */
> > +static bool get_map_field_int(const char *map_name, const struct btf *btf,
> > const struct btf_type *def,
> > - const struct btf_member *m,
> > - const void *data, __u32 *res) {
> > + const struct btf_member *m, __u32 *res) {
> > const struct btf_type *t = skip_mods_and_typedefs(btf, m->type);
> > const char *name = btf__name_by_offset(btf, m->name_off);
> > - __u32 int_info = *(const __u32 *)(const void *)(t + 1);
> > + const struct btf_array *arr_info;
> > + const struct btf_type *arr_t;
> >
> > - if (BTF_INFO_KIND(t->info) != BTF_KIND_INT) {
> > - pr_warning("map '%s': attr '%s': expected INT, got %u.\n",
> > + if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) {
> > + pr_warning("map '%s': attr '%s': expected PTR, got %u.\n",
> > map_name, name, BTF_INFO_KIND(t->info));
> > return false;
> > }
> > - if (t->size != 4 || BTF_INT_BITS(int_info) != 32 ||
> > - BTF_INT_OFFSET(int_info)) {
> > - pr_warning("map '%s': attr '%s': expected 32-bit non-bitfield integer, "
> > - "got %u-byte (%d-bit) one with bit offset %d.\n",
> > - map_name, name, t->size, BTF_INT_BITS(int_info),
> > - BTF_INT_OFFSET(int_info));
> > - return false;
> > - }
> > - if (BTF_INFO_KFLAG(def->info) && BTF_MEMBER_BITFIELD_SIZE(m->offset)) {
> > - pr_warning("map '%s': attr '%s': bitfield is not supported.\n",
> > - map_name, name);
> > +
> > + arr_t = btf__type_by_id(btf, t->type);
> > + if (!arr_t) {
> > + pr_warning("map '%s': attr '%s': type [%u] not found.\n",
> > + map_name, name, t->type);
> > return false;
> > }
> > - if (m->offset % 32) {
> > - pr_warning("map '%s': attr '%s': unaligned fields are not supported.\n",
> > - map_name, name);
> > + if (BTF_INFO_KIND(arr_t->info) != BTF_KIND_ARRAY) {
> > + pr_warning("map '%s': attr '%s': expected ARRAY, got %u.\n",
> > + map_name, name, BTF_INFO_KIND(arr_t->info));
> > return false;
> > }
> > -
> > - *res = *(const __u32 *)(data + m->offset / 8);
> > + arr_info = (const void *)(arr_t + 1);
> > + *res = arr_info->nelems;
>
> Here, we use number of array elements (__u32 type) as the value.
>
> But we have
> #define __int(name, val) int (*name)[val]
> which suggests that "val" have type "int".
>
> Do you think using __uint(name, val) may be more consistent?
> If this is something to be enforced, it may be worthwhile to check
> array element type should be __uint, just in case that in the
> future we have different array element type (e.g., int)
> for different purpose.
Currently we don't care about array element type at all. It seems
highly unlikely that we'll use array element type as some sort of
flag, as it would be **way** to much unobvious magic. I'm a bit
worried that there could be some inconsistencies in how compiler emits
or omits SIGNED flag on BTF_KIND_INT, so I'd rather minimize amount of
possible confusion here and not enforce that.
As for __uint vs __int, you are right, the value we can encode is
always unsigned, so it might be a good idea to do __uint instead, I'll
post an updated version.
>
> > return true;
> > }
> >
> > @@ -1074,7 +1074,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > const struct btf_var_secinfo *vi;
> > const struct btf_var *var_extra;
> > const struct btf_member *m;
> > - const void *def_data;
> > const char *map_name;
> > struct bpf_map *map;
> > int vlen, i;
> > @@ -1131,7 +1130,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > pr_debug("map '%s': at sec_idx %d, offset %zu.\n",
> > map_name, map->sec_idx, map->sec_offset);
> >
> > - def_data = data->d_buf + vi->offset;
> > vlen = BTF_INFO_VLEN(def->info);
> > m = (const void *)(def + 1);
> > for (i = 0; i < vlen; i++, m++) {
> > @@ -1144,19 +1142,19 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > }
> > if (strcmp(name, "type") == 0) {
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &map->def.type))
> > + &map->def.type))
> > return -EINVAL;
> > pr_debug("map '%s': found type = %u.\n",
> > map_name, map->def.type);
> > } else if (strcmp(name, "max_entries") == 0) {
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &map->def.max_entries))
> > + &map->def.max_entries))
> > return -EINVAL;
> > pr_debug("map '%s': found max_entries = %u.\n",
> > map_name, map->def.max_entries);
> > } else if (strcmp(name, "map_flags") == 0) {
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &map->def.map_flags))
> > + &map->def.map_flags))
> > return -EINVAL;
> > pr_debug("map '%s': found map_flags = %u.\n",
> > map_name, map->def.map_flags);
> > @@ -1164,7 +1162,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > __u32 sz;
> >
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &sz))
> > + &sz))
> > return -EINVAL;
> > pr_debug("map '%s': found key_size = %u.\n",
> > map_name, sz);
> > @@ -1207,7 +1205,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> > __u32 sz;
> >
> > if (!get_map_field_int(map_name, obj->btf, def, m,
> > - def_data, &sz))
> > + &sz))
> > return -EINVAL;
> > pr_debug("map '%s': found value_size = %u.\n",
> > map_name, sz);
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH v2 bpf-next] bpf: Add support for fq's EDT to HBM
From: Lawrence Brakmo @ 2019-07-02 19:26 UTC (permalink / raw)
To: Yonghong Song, netdev
Cc: Alexei Starovoitov, Andrii Nakryiko, Daniel Borkmann,
Eric Dumazet, Kernel Team
In-Reply-To: <d4b43599-3901-920d-377e-c6f2589ac622@fb.com>
Yonhhong, thank you for the feedbac. Repies inlined. I will submit a new revision later today.
On 7/1/19, 2:04 PM, "Yonghong Song" <yhs@fb.com> wrote:
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.
Removed
> + 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.
Added comment
> + 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?
Removed
> + }
> +
> + // 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?
Good catch. Should not be an issue as long as both updates are concurrent. I changed the update within the spin_lock_region to also be atomic.
> + 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 net-next 1/1] qed: Add support for Timestamping the unicast PTP packets.
From: David Miller @ 2019-07-02 19:23 UTC (permalink / raw)
To: skalluru; +Cc: netdev, mkalderon, aelior
In-Reply-To: <20190702150412.31132-1-skalluru@marvell.com>
From: Sudarsana Reddy Kalluru <skalluru@marvell.com>
Date: Tue, 2 Jul 2019 08:04:12 -0700
> The register definition in the header file captures more details on
> the individual bits.
Where is this register definition in the header file and why aren't
CPP defines from there being used instead of a mask constant mask?
Thanks.
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] bpf: unprivileged BPF access via /dev/bpf
From: Andrii Nakryiko @ 2019-07-02 19:22 UTC (permalink / raw)
To: Song Liu
Cc: Networking, bpf, Alexei Starovoitov, Daniel Borkmann, Kernel Team,
Lorenz Bauer, Jann Horn, gregkh
In-Reply-To: <20190627201923.2589391-2-songliubraving@fb.com>
On Thu, Jun 27, 2019 at 1:20 PM Song Liu <songliubraving@fb.com> wrote:
>
> This patch introduce unprivileged BPF access. The access control is
> achieved via device /dev/bpf. Users with write access to /dev/bpf are able
> to call sys_bpf().
>
> Two ioctl command are added to /dev/bpf:
>
> The two commands enable/disable permission to call sys_bpf() for current
> task. This permission is noted by bpf_permitted in task_struct. This
> permission is inherited during clone(CLONE_THREAD).
>
> Helper function bpf_capable() is added to check whether the task has got
> permission via /dev/bpf.
>
> Signed-off-by: Song Liu <songliubraving@fb.com>
> ---
> Documentation/ioctl/ioctl-number.txt | 1 +
> include/linux/bpf.h | 11 +++++
> include/linux/sched.h | 3 ++
> include/uapi/linux/bpf.h | 6 +++
> kernel/bpf/arraymap.c | 2 +-
> kernel/bpf/cgroup.c | 2 +-
> kernel/bpf/core.c | 4 +-
> kernel/bpf/cpumap.c | 2 +-
> kernel/bpf/devmap.c | 2 +-
> kernel/bpf/hashtab.c | 4 +-
> kernel/bpf/lpm_trie.c | 2 +-
> kernel/bpf/offload.c | 2 +-
> kernel/bpf/queue_stack_maps.c | 2 +-
> kernel/bpf/reuseport_array.c | 2 +-
> kernel/bpf/stackmap.c | 2 +-
> kernel/bpf/syscall.c | 71 +++++++++++++++++++++-------
> kernel/bpf/verifier.c | 2 +-
> kernel/bpf/xskmap.c | 2 +-
> kernel/fork.c | 5 ++
> net/core/filter.c | 6 +--
> 20 files changed, 99 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/ioctl/ioctl-number.txt b/Documentation/ioctl/ioctl-number.txt
> index c9558146ac58..19998b99d603 100644
> --- a/Documentation/ioctl/ioctl-number.txt
> +++ b/Documentation/ioctl/ioctl-number.txt
> @@ -327,6 +327,7 @@ Code Seq#(hex) Include File Comments
> 0xB4 00-0F linux/gpio.h <mailto:linux-gpio@vger.kernel.org>
> 0xB5 00-0F uapi/linux/rpmsg.h <mailto:linux-remoteproc@vger.kernel.org>
> 0xB6 all linux/fpga-dfl.h
> +0xBP 01-02 uapi/linux/bpf.h <mailto:bpf@vger.kernel.org>
should this be 0xBF?
> 0xC0 00-0F linux/usb/iowarrior.h
> 0xCA 00-0F uapi/misc/cxl.h
> 0xCA 10-2F uapi/misc/ocxl.h
<snip>
^ permalink raw reply
* Re: veth pair ping fail if one of them enslaved into a VRF
From: Ido Schimmel @ 2019-07-02 19:21 UTC (permalink / raw)
To: Zoltán Elek; +Cc: netdev, dsa
In-Reply-To: <CANsP1a4HCthstZP16k-ABajni1m75+VKT+mgLPF=4yGJ-H_ONQ@mail.gmail.com>
On Tue, Jul 02, 2019 at 08:42:15PM +0200, Zoltán Elek wrote:
> Hi!
>
> I have a simple scenario, with a veth pair, IP addresses assigned from
> the same subnet. They can ping eachother. But when I put one of them
> into a VRF (in the example below, I put veth in-vrf into the test-vrf
> VRF) the ping fails. My first question: that is the expected behavior?
> And my second question: is there any way to overcome this?
>
> Here are my test commands:
> ip link add out-of-vrf type veth peer name in-vrf
> ip link set dev out-of-vrf up
> ip link set dev in-vrf up
> ip link add test-vrf type vrf table 10
> ip link set dev test-vrf up
> ip -4 addr add 100.127.253.2/24 dev in-vrf
> ip -4 addr add 100.127.253.1/24 dev out-of-vrf
>
> Then ping works as expected:
> ping -c1 -I 100.127.253.1 100.127.253.2
>
> After I put the in-vrf into test-vrf, ping fails:
> ip link set in-vrf vrf test-vrf up
You need to re-order the FIB rules so that lookup for 100.127.253.1
happens in table 10 and not in the local table:
# ip -4 rule add pref 32765 table local
# ip -4 rule del pref 0
# ip -4 rule show
1000: from all lookup [l3mdev-table]
32765: from all lookup local
32766: from all lookup main
32767: from all lookup default
Bad:
ping 16735 [001] 13726.398115: fib:fib_table_lookup: table 255 oif 0 iif
9 proto 0 100.127.253.2/0 -> 100.127.253.1/0 tos 0 scope 0 flags 4 ==>
dev out-of-vrf gw 0.0.0.0 src 100.127.253.1 err 0
Good:
ping 16665 [001] 13500.937145: fib:fib_table_lookup: table 10 oif 0 iif
9 proto 0 100.127.253.2/0 -> 100.127.253.1/0 tos 0 scope 0 flags 4 ==>
dev in-vrf gw 0.0.0.0 src 100.127.253.2 err 0
>
> Thanks,
> Zoltan Elek,
> VI1
^ permalink raw reply
* Re: [PATCH v2 bpf-next 1/4] libbpf: capture value in BTF type info for BTF-defined map defs
From: Y Song @ 2019-07-02 19:18 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: andrii.nakryiko, Alexei Starovoitov, Daniel Borkmann, bpf, netdev,
Song Liu
In-Reply-To: <20190628152539.3014719-2-andriin@fb.com>
On Fri, Jun 28, 2019 at 8:26 AM Andrii Nakryiko <andriin@fb.com> wrote:
>
> Change BTF-defined map definitions to capture compile-time integer
> values as part of BTF type definition, to avoid split of key/value type
> information and actual type/size/flags initialization for maps.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> ---
> tools/lib/bpf/libbpf.c | 58 ++++++++++++++++++++----------------------
> 1 file changed, 28 insertions(+), 30 deletions(-)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index 6e6ebef11ba3..9e099ecb2c2b 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -1028,40 +1028,40 @@ static const struct btf_type *skip_mods_and_typedefs(const struct btf *btf,
> }
> }
>
> -static bool get_map_field_int(const char *map_name,
> - const struct btf *btf,
> +/*
> + * Fetch integer attribute of BTF map definition. Such attributes are
> + * represented using a pointer to an array, in which dimensionality of array
> + * encodes specified integer value. E.g., int (*type)[BPF_MAP_TYPE_ARRAY];
> + * encodes `type => BPF_MAP_TYPE_ARRAY` key/value pair completely using BTF
> + * type definition, while using only sizeof(void *) space in ELF data section.
> + */
> +static bool get_map_field_int(const char *map_name, const struct btf *btf,
> const struct btf_type *def,
> - const struct btf_member *m,
> - const void *data, __u32 *res) {
> + const struct btf_member *m, __u32 *res) {
> const struct btf_type *t = skip_mods_and_typedefs(btf, m->type);
> const char *name = btf__name_by_offset(btf, m->name_off);
> - __u32 int_info = *(const __u32 *)(const void *)(t + 1);
> + const struct btf_array *arr_info;
> + const struct btf_type *arr_t;
>
> - if (BTF_INFO_KIND(t->info) != BTF_KIND_INT) {
> - pr_warning("map '%s': attr '%s': expected INT, got %u.\n",
> + if (BTF_INFO_KIND(t->info) != BTF_KIND_PTR) {
> + pr_warning("map '%s': attr '%s': expected PTR, got %u.\n",
> map_name, name, BTF_INFO_KIND(t->info));
> return false;
> }
> - if (t->size != 4 || BTF_INT_BITS(int_info) != 32 ||
> - BTF_INT_OFFSET(int_info)) {
> - pr_warning("map '%s': attr '%s': expected 32-bit non-bitfield integer, "
> - "got %u-byte (%d-bit) one with bit offset %d.\n",
> - map_name, name, t->size, BTF_INT_BITS(int_info),
> - BTF_INT_OFFSET(int_info));
> - return false;
> - }
> - if (BTF_INFO_KFLAG(def->info) && BTF_MEMBER_BITFIELD_SIZE(m->offset)) {
> - pr_warning("map '%s': attr '%s': bitfield is not supported.\n",
> - map_name, name);
> +
> + arr_t = btf__type_by_id(btf, t->type);
> + if (!arr_t) {
> + pr_warning("map '%s': attr '%s': type [%u] not found.\n",
> + map_name, name, t->type);
> return false;
> }
> - if (m->offset % 32) {
> - pr_warning("map '%s': attr '%s': unaligned fields are not supported.\n",
> - map_name, name);
> + if (BTF_INFO_KIND(arr_t->info) != BTF_KIND_ARRAY) {
> + pr_warning("map '%s': attr '%s': expected ARRAY, got %u.\n",
> + map_name, name, BTF_INFO_KIND(arr_t->info));
> return false;
> }
> -
> - *res = *(const __u32 *)(data + m->offset / 8);
> + arr_info = (const void *)(arr_t + 1);
> + *res = arr_info->nelems;
Here, we use number of array elements (__u32 type) as the value.
But we have
#define __int(name, val) int (*name)[val]
which suggests that "val" have type "int".
Do you think using __uint(name, val) may be more consistent?
If this is something to be enforced, it may be worthwhile to check
array element type should be __uint, just in case that in the
future we have different array element type (e.g., int)
for different purpose.
> return true;
> }
>
> @@ -1074,7 +1074,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> const struct btf_var_secinfo *vi;
> const struct btf_var *var_extra;
> const struct btf_member *m;
> - const void *def_data;
> const char *map_name;
> struct bpf_map *map;
> int vlen, i;
> @@ -1131,7 +1130,6 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> pr_debug("map '%s': at sec_idx %d, offset %zu.\n",
> map_name, map->sec_idx, map->sec_offset);
>
> - def_data = data->d_buf + vi->offset;
> vlen = BTF_INFO_VLEN(def->info);
> m = (const void *)(def + 1);
> for (i = 0; i < vlen; i++, m++) {
> @@ -1144,19 +1142,19 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> }
> if (strcmp(name, "type") == 0) {
> if (!get_map_field_int(map_name, obj->btf, def, m,
> - def_data, &map->def.type))
> + &map->def.type))
> return -EINVAL;
> pr_debug("map '%s': found type = %u.\n",
> map_name, map->def.type);
> } else if (strcmp(name, "max_entries") == 0) {
> if (!get_map_field_int(map_name, obj->btf, def, m,
> - def_data, &map->def.max_entries))
> + &map->def.max_entries))
> return -EINVAL;
> pr_debug("map '%s': found max_entries = %u.\n",
> map_name, map->def.max_entries);
> } else if (strcmp(name, "map_flags") == 0) {
> if (!get_map_field_int(map_name, obj->btf, def, m,
> - def_data, &map->def.map_flags))
> + &map->def.map_flags))
> return -EINVAL;
> pr_debug("map '%s': found map_flags = %u.\n",
> map_name, map->def.map_flags);
> @@ -1164,7 +1162,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> __u32 sz;
>
> if (!get_map_field_int(map_name, obj->btf, def, m,
> - def_data, &sz))
> + &sz))
> return -EINVAL;
> pr_debug("map '%s': found key_size = %u.\n",
> map_name, sz);
> @@ -1207,7 +1205,7 @@ static int bpf_object__init_user_btf_map(struct bpf_object *obj,
> __u32 sz;
>
> if (!get_map_field_int(map_name, obj->btf, def, m,
> - def_data, &sz))
> + &sz))
> return -EINVAL;
> pr_debug("map '%s': found value_size = %u.\n",
> map_name, sz);
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH net] rxrpc: Fix send on a connected, but unbound socket
From: David Miller @ 2019-07-02 19:16 UTC (permalink / raw)
To: dhowells
Cc: netdev, syzbot+7966f2a0b2c7da8939b4, marc.dionne, linux-afs,
linux-kernel
In-Reply-To: <156207955265.1655.13658692984261290810.stgit@warthog.procyon.org.uk>
From: David Howells <dhowells@redhat.com>
Date: Tue, 02 Jul 2019 15:59:12 +0100
> If sendmsg() or sendmmsg() is called on a connected socket that hasn't had
> bind() called on it, then an oops will occur when the kernel tries to
> connect the call because no local endpoint has been allocated.
>
> Fix this by implicitly binding the socket if it is in the
> RXRPC_CLIENT_UNBOUND state, just like it does for the RXRPC_UNBOUND state.
>
> Further, the state should be transitioned to RXRPC_CLIENT_BOUND after this
> to prevent further attempts to bind it.
>
> This can be tested with:
...
> Leading to the following oops:
...
> Fixes: 2341e0775747 ("rxrpc: Simplify connect() implementation and simplify sendmsg() op")
> Reported-by: syzbot+7966f2a0b2c7da8939b4@syzkaller.appspotmail.com
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Marc Dionne <marc.dionne@auristor.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next 07/12] net: use tcf_block_setup() infrastructure
From: Marcelo Ricardo Leitner @ 2019-07-02 19:16 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, netfilter-devel, davem, thomas.lendacky, f.fainelli,
ariel.elior, michael.chan, santosh, madalin.bucur, yisen.zhuang,
salil.mehta, jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch,
jakub.kicinski, peppe.cavallaro, grygorii.strashko, andrew,
vivien.didelot, alexandre.torgue, joabreu, linux-net-drivers,
ganeshgr, ogerlitz, Manish.Chopra, mkubecek, venkatkumar.duvvuru,
cphealy
In-Reply-To: <20190620194917.2298-8-pablo@netfilter.org>
On Thu, Jun 20, 2019 at 09:49:12PM +0200, Pablo Neira Ayuso wrote:
...
> @@ -1173,8 +1191,10 @@ static int tcf_block_offload_cmd(struct tcf_block *block,
> struct tc_block_offload bo = {};
> int err;
>
> + bo.net = dev_net(dev);
> bo.command = command;
> bo.binder_type = ei->binder_type;
> + bo.net = dev_net(dev),
^
And it's assigning the same thing twice in this chunk.
> bo.block = block;
> bo.extack = extack;
> INIT_LIST_HEAD(&bo.cb_list);
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next 02/12] net: sched: add tcf_block_cb_alloc()
From: Marcelo Ricardo Leitner @ 2019-07-02 19:14 UTC (permalink / raw)
To: Pablo Neira Ayuso
Cc: netdev, netfilter-devel, davem, thomas.lendacky, f.fainelli,
ariel.elior, michael.chan, santosh, madalin.bucur, yisen.zhuang,
salil.mehta, jeffrey.t.kirsher, tariqt, saeedm, jiri, idosch,
jakub.kicinski, peppe.cavallaro, grygorii.strashko, andrew,
vivien.didelot, alexandre.torgue, joabreu, linux-net-drivers,
ganeshgr, ogerlitz, Manish.Chopra, mkubecek, venkatkumar.duvvuru,
cphealy
In-Reply-To: <20190620194917.2298-3-pablo@netfilter.org>
On Thu, Jun 20, 2019 at 09:49:07PM +0200, Pablo Neira Ayuso wrote:
> Add a new helper function to allocate tcf_block_cb objects.
>
> Signed-off-by: Pablo Neira Ayuso <pablo@netfilter.org>
> ---
> include/net/pkt_cls.h | 8 ++++++++
> net/sched/cls_api.c | 23 +++++++++++++++++++----
> 2 files changed, 27 insertions(+), 4 deletions(-)
>
> diff --git a/include/net/pkt_cls.h b/include/net/pkt_cls.h
> index 720f2b32fc2f..276a17a3547b 100644
> --- a/include/net/pkt_cls.h
> +++ b/include/net/pkt_cls.h
> @@ -72,6 +72,8 @@ static inline struct Qdisc *tcf_block_q(struct tcf_block *block)
> return block->q;
> }
>
> +struct tcf_block_cb *tcf_block_cb_alloc(tc_setup_cb_t *cb,
> + void *cb_ident, void *cb_priv);
> void *tcf_block_cb_priv(struct tcf_block_cb *block_cb);
> struct tcf_block_cb *tcf_block_cb_lookup(struct tcf_block *block,
> tc_setup_cb_t *cb, void *cb_ident);
> @@ -150,6 +152,12 @@ void tc_setup_cb_block_unregister(struct tcf_block *block, tc_setup_cb_t *cb,
> {
> }
>
> +static inline struct tcf_block_cb *
> +tcf_block_cb_alloc(tc_setup_cb_t *cb, void *cb_ident, void *cb_priv)
> +{
> + return NULL;
[A] This...
> +}
> +
> static inline
> void *tcf_block_cb_priv(struct tcf_block_cb *block_cb)
> {
> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
> index b2417fda26ec..c01d825edab5 100644
> --- a/net/sched/cls_api.c
> +++ b/net/sched/cls_api.c
> @@ -746,6 +746,23 @@ unsigned int tcf_block_cb_decref(struct tcf_block_cb *block_cb)
> }
> EXPORT_SYMBOL(tcf_block_cb_decref);
>
> +struct tcf_block_cb *tcf_block_cb_alloc(tc_setup_cb_t *cb,
> + void *cb_ident, void *cb_priv)
> +{
> + struct tcf_block_cb *block_cb;
> +
> + block_cb = kzalloc(sizeof(*block_cb), GFP_KERNEL);
> + if (!block_cb)
> + return NULL;
> +
> + block_cb->cb = cb;
> + block_cb->cb_ident = cb_ident;
> + block_cb->cb_priv = cb_priv;
> +
> + return block_cb;
> +}
> +EXPORT_SYMBOL(tcf_block_cb_alloc);
> +
> struct tcf_block_cb *__tcf_block_cb_register(struct tcf_block *block,
> tc_setup_cb_t *cb, void *cb_ident,
> void *cb_priv,
> @@ -761,12 +778,10 @@ struct tcf_block_cb *__tcf_block_cb_register(struct tcf_block *block,
> if (err)
> return ERR_PTR(err);
>
> - block_cb = kzalloc(sizeof(*block_cb), GFP_KERNEL);
> + block_cb = tcf_block_cb_alloc(cb, cb_ident, cb_priv);
> if (!block_cb)
> return ERR_PTR(-ENOMEM);
... will be translated into -ENOMEM here.
Would be nice to in [A] return ERR_PTR(-EOPNOTSUPP) instead and adjust
the actual implementation as well.
With patch 11, this will be visible to the user then.
> - block_cb->cb = cb;
> - block_cb->cb_ident = cb_ident;
> - block_cb->cb_priv = cb_priv;
> +
> list_add(&block_cb->list, &block->cb_list);
> return block_cb;
> }
> --
> 2.11.0
>
^ permalink raw reply
* Re: [PATCH net-next] rxrpc: Fix uninitialized error code in rxrpc_send_data_packet()
From: David Miller @ 2019-07-02 19:09 UTC (permalink / raw)
To: dhowells; +Cc: netdev, geert, linux-afs, linux-kernel
In-Reply-To: <156207932870.853.14700731055154895417.stgit@warthog.procyon.org.uk>
From: David Howells <dhowells@redhat.com>
Date: Tue, 02 Jul 2019 15:55:28 +0100
> With gcc 4.1:
>
> net/rxrpc/output.c: In function ‘rxrpc_send_data_packet’:
> net/rxrpc/output.c:338: warning: ‘ret’ may be used uninitialized in this function
>
> Indeed, if the first jump to the send_fragmentable label is made, and
> the address family is not handled in the switch() statement, ret will be
> used uninitialized.
>
> Fix this by BUG()'ing as is done in other places in rxrpc where internal
> support for future address families will need adding. It should not be
> possible to reach this normally as the address families are checked
> up-front.
>
> Fixes: 5a924b8951f835b5 ("rxrpc: Don't store the rxrpc header in the Tx queue sk_buffs")
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
Applied, thanks David.
^ permalink raw reply
* Re: [PATCHv2] selftests/net: skip psock_tpacket test if KALLSYMS was not enabled
From: shuah @ 2019-07-02 19:09 UTC (permalink / raw)
To: Po-Hsu Lin, davem, linux-kselftest; +Cc: netdev, linux-kernel, shuah
In-Reply-To: <20190701044031.19451-1-po-hsu.lin@canonical.com>
On 6/30/19 10:40 PM, Po-Hsu Lin wrote:
> The psock_tpacket test will need to access /proc/kallsyms, this would
> require the kernel config CONFIG_KALLSYMS to be enabled first.
>
> Apart from adding CONFIG_KALLSYMS to the net/config file here, check the
> file existence to determine if we can run this test will be helpful to
> avoid a false-positive test result when testing it directly with the
> following commad against a kernel that have CONFIG_KALLSYMS disabled:
> make -C tools/testing/selftests TARGETS=net run_tests
>
> Signed-off-by: Po-Hsu Lin <po-hsu.lin@canonical.com>
> ---
> tools/testing/selftests/net/config | 1 +
> tools/testing/selftests/net/run_afpackettests | 14 +++++++++-----
> 2 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/net/config b/tools/testing/selftests/net/config
> index 4740404..3dea2cb 100644
> --- a/tools/testing/selftests/net/config
> +++ b/tools/testing/selftests/net/config
> @@ -25,3 +25,4 @@ CONFIG_NF_TABLES_IPV6=y
> CONFIG_NF_TABLES_IPV4=y
> CONFIG_NFT_CHAIN_NAT_IPV6=m
> CONFIG_NFT_CHAIN_NAT_IPV4=m
> +CONFIG_KALLSYMS=y
> diff --git a/tools/testing/selftests/net/run_afpackettests b/tools/testing/selftests/net/run_afpackettests
> index ea5938e..8b42e8b 100755
> --- a/tools/testing/selftests/net/run_afpackettests
> +++ b/tools/testing/selftests/net/run_afpackettests
> @@ -21,12 +21,16 @@ fi
> echo "--------------------"
> echo "running psock_tpacket test"
> echo "--------------------"
> -./in_netns.sh ./psock_tpacket
> -if [ $? -ne 0 ]; then
> - echo "[FAIL]"
> - ret=1
> +if [ -f /proc/kallsyms ]; then
> + ./in_netns.sh ./psock_tpacket
> + if [ $? -ne 0 ]; then
> + echo "[FAIL]"
> + ret=1
> + else
> + echo "[PASS]"
> + fi
> else
> - echo "[PASS]"
> + echo "[SKIP] CONFIG_KALLSYMS not enabled"
> fi
>
> echo "--------------------"
>
Looks good to me. Thanks for the patch.
Acked-by: Shuah Khan <skhan@linuxfoundation.org>
thanks,
-- Shuah
^ permalink raw reply
* [PATCH net-next] mlxsw: spectrum_ptp: Fix validation in mlxsw_sp1_ptp_packet_finish()
From: Petr Machata @ 2019-07-02 19:06 UTC (permalink / raw)
To: netdev@vger.kernel.org; +Cc: Petr Machata, Colin Ian King, Ido Schimmel
Before mlxsw_sp1_ptp_packet_finish() sends the packet back, it validates
whether the corresponding port is still valid. However the condition is
incorrect: when mlxsw_sp_port == NULL, the code dereferences the port to
compare it to skb->dev.
The condition needs to check whether the port is present and skb->dev still
refers to that port (or else is NULL). If that does not hold, bail out.
Add a pair of parentheses to fix the condition.
Fixes: d92e4e6e33c8 ("mlxsw: spectrum: PTP: Support timestamping on Spectrum-1")
Reported-by: Colin Ian King <colin.king@canonical.com>
Signed-off-by: Petr Machata <petrm@mellanox.com>
Signed-off-by: Colin Ian King <colin.king@canonical.com>
Reviewed-by: Ido Schimmel <idosch@mellanox.com>
---
drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
index 7d42f86237cd..437023d67a3b 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/spectrum_ptp.c
@@ -425,7 +425,7 @@ static void mlxsw_sp1_ptp_packet_finish(struct mlxsw_sp *mlxsw_sp,
* split). Also make sure the SKB device reference is still valid.
*/
mlxsw_sp_port = mlxsw_sp->ports[local_port];
- if (!mlxsw_sp_port && (!skb->dev || skb->dev == mlxsw_sp_port->dev)) {
+ if (!(mlxsw_sp_port && (!skb->dev || skb->dev == mlxsw_sp_port->dev))) {
dev_kfree_skb_any(skb);
return;
}
--
2.20.1
^ permalink raw reply related
* Re: [PATCH] nfc: st-nci: remove redundant assignment to variable r
From: David Miller @ 2019-07-02 19:02 UTC (permalink / raw)
To: colin.king; +Cc: tglx, netdev, kernel-janitors, linux-kernel
In-Reply-To: <20190702131642.9865-1-colin.king@canonical.com>
From: Colin King <colin.king@canonical.com>
Date: Tue, 2 Jul 2019 14:16:42 +0100
> From: Colin Ian King <colin.king@canonical.com>
>
> The variable r is being initialized with a value that is never
> read and it is being updated later with a new value. The
> initialization is redundant and can be removed.
>
> Addresses-Coverity: ("Unused value")
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
Applied to net-next.
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Ivan Khoronzhuk @ 2019-07-02 18:58 UTC (permalink / raw)
To: Jesper Dangaard Brouer
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap
In-Reply-To: <20190702202907.15fb30ce@carbon>
On Tue, Jul 02, 2019 at 08:29:07PM +0200, Jesper Dangaard Brouer wrote:
>On Tue, 2 Jul 2019 18:21:13 +0300
>Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>
>> On Tue, Jul 02, 2019 at 05:10:29PM +0200, Jesper Dangaard Brouer wrote:
>> >On Tue, 2 Jul 2019 17:56:13 +0300
>> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >
>> >> On Tue, Jul 02, 2019 at 04:52:30PM +0200, Jesper Dangaard Brouer wrote:
>> >> >On Tue, 2 Jul 2019 17:44:27 +0300
>> >> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
>> >> >
>> >> >> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
>> >> >> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
>> >> >> >
>> >> >> >Jesper recently removed page_pool_destroy() (from driver invocation) and
>> >> >> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
>> >> >> >handle in-flight packets/pages. This created an asymmetry in drivers
>> >> >> >create/destroy pairs.
>> >> >> >
>> >> >> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
>> >> >> >This serves two purposes, (1) simplify drivers error handling as driver now
>> >> >> >drivers always calls page_pool_destroy() and don't need to track if
>> >> >> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
>> >> >> >where a single RX-queue (with a single page_pool) provides packets for two
>> >> >> >net_device'es, and thus needs to register the same page_pool twice with two
>> >> >> >xdp_rxq_info structures.
>> >> >>
>> >> >> As I tend to use xdp level patch there is no more reason to mention (2) case
>> >> >> here. XDP patch serves it better and can prevent not only obj deletion but also
>> >> >> pool flush, so, this one patch I could better leave only for (1) case.
>> >> >
>> >> >I don't understand what you are saying.
>> >> >
>> >> >Do you approve this patch, or do you reject this patch?
>> >> >
>> >> It's not reject, it's proposition to use both, XDP and page pool patches,
>> >> each having its goal.
>> >
>> >Just to be clear, if you want this patch to get accepted you have to
>> >reply with your Signed-off-by (as I wrote).
>> >
>> >Maybe we should discuss it in another thread, about why you want two
>> >solutions to the same problem.
>>
>> If it solves same problem I propose to reject this one and use this:
>> https://lkml.org/lkml/2019/7/2/651
>
>No, I propose using this one, and rejecting the other one.
There is at least several arguments against this one (related (2) purpose)
It allows:
- avoid changes to page_pool/mlx5/netsec
- save not only allocator obj but allocator "page/buffer flush"
- buffer flush can be present not only in page_pool but for other allocators
that can behave differently and not so simple solution.
- to not limit cpsw/(potentially others) to use "page_pool" allocator only
....
This patch better leave also, as it simplifies error path for page_pool and
have more error prone usage comparing with existent one.
Please, don't limit cpsw and potentially other drivers to use only
page_pool it can be zca or etc... I don't won't to modify each allocator.
I propose to add both as by fact they solve different problems with common
solution.
--
Regards,
Ivan Khoronzhuk
^ permalink raw reply
* Re: [PATCH net 0/4] net: bridge: fix possible stale skb pointers
From: David Miller @ 2019-07-02 18:54 UTC (permalink / raw)
To: nikolay; +Cc: netdev, roopa, martin, bridge, yoshfuji
In-Reply-To: <20190702120021.13096-1-nikolay@cumulusnetworks.com>
From: Nikolay Aleksandrov <nikolay@cumulusnetworks.com>
Date: Tue, 2 Jul 2019 15:00:17 +0300
> In the bridge driver we have a couple of places which call pskb_may_pull
> but we've cached skb pointers before that and use them after which can
> lead to out-of-bounds/stale pointer use. I've had these in my "to fix"
> list for some time and now we got a report (patch 01) so here they are.
> Patches 02-04 are fixes based on code inspection. Also patch 01 was
> tested by Martin Weinelt, Martin if you don't mind please add your
> tested-by tag to it by replying with Tested-by: name <email>.
> I've also briefly tested the set by trying to exercise those code paths.
Series applied, thanks.
^ permalink raw reply
* RE: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and port attribute
From: Parav Pandit @ 2019-07-02 18:50 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Jiri Pirko, netdev@vger.kernel.org, Saeed Mahameed
In-Reply-To: <20190702104711.77618f6a@cakuba.netronome.com>
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Tuesday, July 2, 2019 11:17 PM
> To: Parav Pandit <parav@mellanox.com>
> Cc: Jiri Pirko <jiri@mellanox.com>; netdev@vger.kernel.org; Saeed
> Mahameed <saeedm@mellanox.com>
> Subject: Re: [PATCH net-next 1/3] devlink: Introduce PCI PF port flavour and
> port attribute
>
> On Tue, 2 Jul 2019 04:26:47 +0000, Parav Pandit wrote:
> > > On Mon, 1 Jul 2019 07:27:32 -0500, Parav Pandit wrote:
> > > > In an eswitch, PCI PF may have port which is normally represented
> > > > using a representor netdevice.
> > > > To have better visibility of eswitch port, its association with
> > > > PF, a representor netdevice and port number, introduce a PCI PF
> > > > port flavour and port attriute.
> > > >
> > > > When devlink port flavour is PCI PF, fill up PCI PF attributes of
> > > > the port.
> > > >
> > > > Extend port name creation using PCI PF number on best effort basis.
> > > > So that vendor drivers can skip defining their own scheme.
> > > >
> > > > $ devlink port show
> > > > pci/0000:05:00.0/0: type eth netdev eth0 flavour pcipf pfnum 0
> > > >
> > > > Acked-by: Jiri Pirko <jiri@mellanox.com>
> > > > Signed-off-by: Parav Pandit <parav@mellanox.com> diff --git
> > > > a/include/net/devlink.h b/include/net/devlink.h index
> > > > 6625ea068d5e..8db9c0e83fb5 100644
> > > > --- a/include/net/devlink.h
> > > > +++ b/include/net/devlink.h
> > > > @@ -38,6 +38,10 @@ struct devlink {
> > > > char priv[0] __aligned(NETDEV_ALIGN); };
> > > >
> > > > +struct devlink_port_pci_pf_attrs {
> > >
> > > Why the named structure? Anonymous one should be just fine?
> > >
> > No specific reason for this patch. But named structure allows to
> > extend it more easily with code readability.
>
> I'd argue the readability - I hove to scroll up/look up the structure just to see
> it has a single member. But no big deal :)
>
Ok. :-)
> > Such as subsequently we want to add the peer_mac etc port attributes.
> > Named structure to store those attributes are helpful.
>
> It remains to be seen if peer attributes are flavour specific 🤔
> I'd imagine most port types would have some form of a peer (other than a
> network port, perhaps). But perhaps different peer attributes.
>
Few attributes may be common and few will be port specific.
So as it evolves, data structure will evolve.
Common attribute I can think of is - mac address.
> > > > diff --git a/net/core/devlink.c b/net/core/devlink.c index
> > > > 89c533778135..001f9e2c96f0 100644
> > > > --- a/net/core/devlink.c
> > > > +++ b/net/core/devlink.c
> > > > @@ -517,6 +517,11 @@ static int devlink_nl_port_attrs_put(struct
> sk_buff *msg,
> > > > return -EMSGSIZE;
> > > > if (nla_put_u32(msg, DEVLINK_ATTR_PORT_NUMBER, attrs-
> >port_number))
> > > > return -EMSGSIZE;
> > >
> > > Why would we report network port information for PF and VF port
> > > flavours?
> >
> > I didn't see any immediate need to report, at the same time didn't
> > find any reason to treat such port flavours differently than existing
> > one. It just gives a clear view of the device's eswitch. Might find it
> > useful during debugging while inspecting device internal tables..
>
> PFs and VFs ports are not tied to network ports in switchdev mode.
> You have only one network port under a devlink instance AFAIR, anyway.
>
I am not sure what do you mean by network port.
Do you intent to see a physical port that connects to physical network?
As I described in the comment of the PF and VF flavour, it is an eswitch port.
I have shown the diagram also of the eswitch in the cover letter.
Port_number doesn't have to a physical port. Flavour describe what port type is and number says what is the eswitch port number.
Hope it clarifies.
> > > > + if (devlink_port->attrs.flavour == DEVLINK_PORT_FLAVOUR_PCI_PF) {
> > > > + if (nla_put_u16(msg, DEVLINK_ATTR_PORT_PCI_PF_NUMBER,
> > > > + attrs->pci_pf.pf))
> > > > + return -EMSGSIZE;
> > > > + }
> > > > if (!attrs->split)
> > > > return 0;
> > > > if (nla_put_u32(msg, DEVLINK_ATTR_PORT_SPLIT_GROUP,
> > > > attrs->port_number))
^ permalink raw reply
* [PATCH v2 net-next] r8169: add random MAC address fallback
From: Heiner Kallweit @ 2019-07-02 18:46 UTC (permalink / raw)
To: Realtek linux nic maintainers, David Miller; +Cc: netdev@vger.kernel.org
It was reported that the GPD MicroPC is broken in a way that no valid
MAC address can be read from the network chip. The vendor driver deals
with this by assigning a random MAC address as fallback. So let's do
the same.
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
v2:
- fix broken commit message
---
drivers/net/ethernet/realtek/r8169_main.c | 40 +++++++++++++++--------
1 file changed, 27 insertions(+), 13 deletions(-)
diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index 450c74dc1..d6c137b7f 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -6651,13 +6651,36 @@ static int rtl_get_ether_clk(struct rtl8169_private *tp)
return rc;
}
+static void rtl_init_mac_address(struct rtl8169_private *tp)
+{
+ struct net_device *dev = tp->dev;
+ u8 *mac_addr = dev->dev_addr;
+ int rc, i;
+
+ rc = eth_platform_get_mac_address(tp_to_dev(tp), mac_addr);
+ if (!rc)
+ goto done;
+
+ rtl_read_mac_address(tp, mac_addr);
+ if (is_valid_ether_addr(mac_addr))
+ goto done;
+
+ for (i = 0; i < ETH_ALEN; i++)
+ mac_addr[i] = RTL_R8(tp, MAC0 + i);
+ if (is_valid_ether_addr(mac_addr))
+ goto done;
+
+ eth_hw_addr_random(dev);
+ dev_warn(tp_to_dev(tp), "can't read MAC address, setting random one\n");
+done:
+ rtl_rar_set(tp, mac_addr);
+}
+
static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
{
- /* align to u16 for is_valid_ether_addr() */
- u8 mac_addr[ETH_ALEN] __aligned(2) = {};
struct rtl8169_private *tp;
struct net_device *dev;
- int chipset, region, i;
+ int chipset, region;
int jumbo_max, rc;
dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
@@ -6749,16 +6772,7 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
u64_stats_init(&tp->rx_stats.syncp);
u64_stats_init(&tp->tx_stats.syncp);
- /* get MAC address */
- rc = eth_platform_get_mac_address(&pdev->dev, mac_addr);
- if (rc)
- rtl_read_mac_address(tp, mac_addr);
-
- if (is_valid_ether_addr(mac_addr))
- rtl_rar_set(tp, mac_addr);
-
- for (i = 0; i < ETH_ALEN; i++)
- dev->dev_addr[i] = RTL_R8(tp, MAC0 + i);
+ rtl_init_mac_address(tp);
dev->ethtool_ops = &rtl8169_ethtool_ops;
--
2.22.0
^ permalink raw reply related
* veth pair ping fail if one of them enslaved into a VRF
From: Zoltán Elek @ 2019-07-02 18:42 UTC (permalink / raw)
To: netdev, dsa
Hi!
I have a simple scenario, with a veth pair, IP addresses assigned from
the same subnet. They can ping eachother. But when I put one of them
into a VRF (in the example below, I put veth in-vrf into the test-vrf
VRF) the ping fails. My first question: that is the expected behavior?
And my second question: is there any way to overcome this?
Here are my test commands:
ip link add out-of-vrf type veth peer name in-vrf
ip link set dev out-of-vrf up
ip link set dev in-vrf up
ip link add test-vrf type vrf table 10
ip link set dev test-vrf up
ip -4 addr add 100.127.253.2/24 dev in-vrf
ip -4 addr add 100.127.253.1/24 dev out-of-vrf
Then ping works as expected:
ping -c1 -I 100.127.253.1 100.127.253.2
After I put the in-vrf into test-vrf, ping fails:
ip link set in-vrf vrf test-vrf up
Thanks,
Zoltan Elek,
VI1
^ permalink raw reply
* Re: [PATCH] net: core: page_pool: add user refcnt and reintroduce page_pool_destroy
From: Jesper Dangaard Brouer @ 2019-07-02 18:29 UTC (permalink / raw)
To: Ivan Khoronzhuk
Cc: netdev, Ilias Apalodimas, grygorii.strashko, jakub.kicinski,
daniel, john.fastabend, ast, linux-kernel, linux-omap, brouer
In-Reply-To: <20190702152112.GG4510@khorivan>
On Tue, 2 Jul 2019 18:21:13 +0300
Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> On Tue, Jul 02, 2019 at 05:10:29PM +0200, Jesper Dangaard Brouer wrote:
> >On Tue, 2 Jul 2019 17:56:13 +0300
> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >
> >> On Tue, Jul 02, 2019 at 04:52:30PM +0200, Jesper Dangaard Brouer wrote:
> >> >On Tue, 2 Jul 2019 17:44:27 +0300
> >> >Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org> wrote:
> >> >
> >> >> On Tue, Jul 02, 2019 at 04:31:39PM +0200, Jesper Dangaard Brouer wrote:
> >> >> >From: Ivan Khoronzhuk <ivan.khoronzhuk@linaro.org>
> >> >> >
> >> >> >Jesper recently removed page_pool_destroy() (from driver invocation) and
> >> >> >moved shutdown and free of page_pool into xdp_rxq_info_unreg(), in-order to
> >> >> >handle in-flight packets/pages. This created an asymmetry in drivers
> >> >> >create/destroy pairs.
> >> >> >
> >> >> >This patch add page_pool user refcnt and reintroduce page_pool_destroy.
> >> >> >This serves two purposes, (1) simplify drivers error handling as driver now
> >> >> >drivers always calls page_pool_destroy() and don't need to track if
> >> >> >xdp_rxq_info_reg_mem_model() was unsuccessful. (2) allow special cases
> >> >> >where a single RX-queue (with a single page_pool) provides packets for two
> >> >> >net_device'es, and thus needs to register the same page_pool twice with two
> >> >> >xdp_rxq_info structures.
> >> >>
> >> >> As I tend to use xdp level patch there is no more reason to mention (2) case
> >> >> here. XDP patch serves it better and can prevent not only obj deletion but also
> >> >> pool flush, so, this one patch I could better leave only for (1) case.
> >> >
> >> >I don't understand what you are saying.
> >> >
> >> >Do you approve this patch, or do you reject this patch?
> >> >
> >> It's not reject, it's proposition to use both, XDP and page pool patches,
> >> each having its goal.
> >
> >Just to be clear, if you want this patch to get accepted you have to
> >reply with your Signed-off-by (as I wrote).
> >
> >Maybe we should discuss it in another thread, about why you want two
> >solutions to the same problem.
>
> If it solves same problem I propose to reject this one and use this:
> https://lkml.org/lkml/2019/7/2/651
No, I propose using this one, and rejecting the other one.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ 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