* Re: [PATCH v2 bpf-next] libbpf: fix GCC8 warning for strncpy
From: Y Song @ 2019-07-02 16:10 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: andrii.nakryiko, bpf, netdev, Alexei Starovoitov, Daniel Borkmann,
Magnus Karlsson
In-Reply-To: <20190702151620.3382559-1-andriin@fb.com>
On Tue, Jul 2, 2019 at 8:17 AM Andrii Nakryiko <andriin@fb.com> wrote:
>
> GCC8 started emitting warning about using strncpy with number of bytes
> exactly equal destination size, which is generally unsafe, as can lead
> to non-zero terminated string being copied. Use IFNAMSIZ - 1 as number
> of bytes to ensure name is always zero-terminated.
>
> Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
Acked-by: Yonghong Song <yhs@fb.com>
> ---
> tools/lib/bpf/xsk.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> index bf15a80a37c2..b33740221b7e 100644
> --- a/tools/lib/bpf/xsk.c
> +++ b/tools/lib/bpf/xsk.c
> @@ -327,7 +327,8 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
>
> channels.cmd = ETHTOOL_GCHANNELS;
> ifr.ifr_data = (void *)&channels;
> - strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
> + strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1);
> + ifr.ifr_name[IFNAMSIZ - 1] = '\0';
> err = ioctl(fd, SIOCETHTOOL, &ifr);
> if (err && errno != EOPNOTSUPP) {
> ret = -errno;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Andrew Lunn @ 2019-07-02 16:11 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru; +Cc: davem, netdev, mkalderon, aelior, jiri
In-Reply-To: <20190702152056.31728-1-skalluru@marvell.com>
On Tue, Jul 02, 2019 at 08:20:56AM -0700, Sudarsana Reddy Kalluru wrote:
> The patch adds devlink interfaces for drivers to publish/unpublish the
> devlink port parameters.
Hi Sudarsana
A good commit message says more about 'why' than 'what'. I can see the
'what' by reading the code. But the 'why' is often not so clear.
Why would i want to unpublish port parameters?
Thanks
Andrew
^ permalink raw reply
* [PATCH bpf-next v2 0/8] bpf: TCP RTT sock_ops bpf callback
From: Stanislav Fomichev @ 2019-07-02 16:13 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
Congestion control team would like to have a periodic callback to
track some TCP statistics. Let's add a sock_ops callback that can be
selectively enabled on a socket by socket basis and is executed for
every RTT. BPF program frequency can be further controlled by calling
bpf_ktime_get_ns and bailing out early.
I run neper tcp_stream and tcp_rr tests with the sample program
from the last patch and didn't observe any noticeable performance
difference.
v2:
* add a comment about second accept() in selftest (Yonghong Song)
* refer to tcp_bpf.readme in sample program (Yonghong Song)
Suggested-by: Eric Dumazet <edumazet@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Stanislav Fomichev (8):
bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
bpf: add icsk_retransmits to bpf_tcp_sock
bpf/tools: sync bpf.h
selftests/bpf: test BPF_SOCK_OPS_RTT_CB
samples/bpf: add sample program that periodically dumps TCP stats
samples/bpf: fix tcp_bpf.readme detach command
include/net/tcp.h | 8 +
include/uapi/linux/bpf.h | 12 +-
net/core/filter.c | 207 +++++++++++-----
net/ipv4/tcp_input.c | 4 +
samples/bpf/Makefile | 1 +
samples/bpf/tcp_bpf.readme | 2 +-
samples/bpf/tcp_dumpstats_kern.c | 68 ++++++
tools/include/uapi/linux/bpf.h | 12 +-
tools/testing/selftests/bpf/Makefile | 3 +-
tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
tools/testing/selftests/bpf/test_tcp_rtt.c | 254 ++++++++++++++++++++
11 files changed, 574 insertions(+), 58 deletions(-)
create mode 100644 samples/bpf/tcp_dumpstats_kern.c
create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply
* [PATCH bpf-next v2 1/8] bpf: add BPF_CGROUP_SOCK_OPS callback that is executed on every RTT
From: Stanislav Fomichev @ 2019-07-02 16:13 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Performance impact should be minimal because it's under a new
BPF_SOCK_OPS_RTT_CB_FLAG flag that has to be explicitly enabled.
Suggested-by: Eric Dumazet <edumazet@google.com>
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/net/tcp.h | 8 ++++++++
include/uapi/linux/bpf.h | 6 +++++-
net/ipv4/tcp_input.c | 4 ++++
3 files changed, 17 insertions(+), 1 deletion(-)
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 9d36cc88d043..e16d8a3fd3b4 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2221,6 +2221,14 @@ static inline bool tcp_bpf_ca_needs_ecn(struct sock *sk)
return (tcp_call_bpf(sk, BPF_SOCK_OPS_NEEDS_ECN, 0, NULL) == 1);
}
+static inline void tcp_bpf_rtt(struct sock *sk)
+{
+ struct tcp_sock *tp = tcp_sk(sk);
+
+ if (BPF_SOCK_OPS_TEST_FLAG(tp, BPF_SOCK_OPS_RTT_CB_FLAG))
+ tcp_call_bpf(sk, BPF_SOCK_OPS_RTT_CB, 0, NULL);
+}
+
#if IS_ENABLED(CONFIG_SMC)
extern struct static_key_false tcp_have_smc;
#endif
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index cffea1826a1f..9cdd0aaeba06 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -1770,6 +1770,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3314,7 +3315,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3369,6 +3371,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
index b71efeb0ae5b..c21e8a22fb3b 100644
--- a/net/ipv4/tcp_input.c
+++ b/net/ipv4/tcp_input.c
@@ -778,6 +778,8 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
tp->rttvar_us -= (tp->rttvar_us - tp->mdev_max_us) >> 2;
tp->rtt_seq = tp->snd_nxt;
tp->mdev_max_us = tcp_rto_min_us(sk);
+
+ tcp_bpf_rtt(sk);
}
} else {
/* no previous measure. */
@@ -786,6 +788,8 @@ static void tcp_rtt_estimator(struct sock *sk, long mrtt_us)
tp->rttvar_us = max(tp->mdev_us, tcp_rto_min_us(sk));
tp->mdev_max_us = tp->rttvar_us;
tp->rtt_seq = tp->snd_nxt;
+
+ tcp_bpf_rtt(sk);
}
tp->srtt_us = max(1U, srtt);
}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 2/8] bpf: split shared bpf_tcp_sock and bpf_sock_ops implementation
From: Stanislav Fomichev @ 2019-07-02 16:13 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
We've added bpf_tcp_sock member to bpf_sock_ops and don't expect
any new tcp_sock fields in bpf_sock_ops. Let's remove
CONVERT_COMMON_TCP_SOCK_FIELDS so bpf_tcp_sock can be independently
extended.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
net/core/filter.c | 180 ++++++++++++++++++++++++++++++++--------------
1 file changed, 126 insertions(+), 54 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 4836264f82ee..ad908526545d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5194,54 +5194,6 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
};
#endif /* CONFIG_IPV6_SEG6_BPF */
-#define CONVERT_COMMON_TCP_SOCK_FIELDS(md_type, CONVERT) \
-do { \
- switch (si->off) { \
- case offsetof(md_type, snd_cwnd): \
- CONVERT(snd_cwnd); break; \
- case offsetof(md_type, srtt_us): \
- CONVERT(srtt_us); break; \
- case offsetof(md_type, snd_ssthresh): \
- CONVERT(snd_ssthresh); break; \
- case offsetof(md_type, rcv_nxt): \
- CONVERT(rcv_nxt); break; \
- case offsetof(md_type, snd_nxt): \
- CONVERT(snd_nxt); break; \
- case offsetof(md_type, snd_una): \
- CONVERT(snd_una); break; \
- case offsetof(md_type, mss_cache): \
- CONVERT(mss_cache); break; \
- case offsetof(md_type, ecn_flags): \
- CONVERT(ecn_flags); break; \
- case offsetof(md_type, rate_delivered): \
- CONVERT(rate_delivered); break; \
- case offsetof(md_type, rate_interval_us): \
- CONVERT(rate_interval_us); break; \
- case offsetof(md_type, packets_out): \
- CONVERT(packets_out); break; \
- case offsetof(md_type, retrans_out): \
- CONVERT(retrans_out); break; \
- case offsetof(md_type, total_retrans): \
- CONVERT(total_retrans); break; \
- case offsetof(md_type, segs_in): \
- CONVERT(segs_in); break; \
- case offsetof(md_type, data_segs_in): \
- CONVERT(data_segs_in); break; \
- case offsetof(md_type, segs_out): \
- CONVERT(segs_out); break; \
- case offsetof(md_type, data_segs_out): \
- CONVERT(data_segs_out); break; \
- case offsetof(md_type, lost_out): \
- CONVERT(lost_out); break; \
- case offsetof(md_type, sacked_out): \
- CONVERT(sacked_out); break; \
- case offsetof(md_type, bytes_received): \
- CONVERT(bytes_received); break; \
- case offsetof(md_type, bytes_acked): \
- CONVERT(bytes_acked); break; \
- } \
-} while (0)
-
#ifdef CONFIG_INET
static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
int dif, int sdif, u8 family, u8 proto)
@@ -5623,9 +5575,6 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
- CONVERT_COMMON_TCP_SOCK_FIELDS(struct bpf_tcp_sock,
- BPF_TCP_SOCK_GET_COMMON);
-
if (insn > insn_buf)
return insn - insn_buf;
@@ -5640,6 +5589,69 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, rtt_min) +
offsetof(struct minmax_sample, v));
break;
+ case offsetof(struct bpf_tcp_sock, snd_cwnd):
+ BPF_TCP_SOCK_GET_COMMON(snd_cwnd);
+ break;
+ case offsetof(struct bpf_tcp_sock, srtt_us):
+ BPF_TCP_SOCK_GET_COMMON(srtt_us);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_ssthresh):
+ BPF_TCP_SOCK_GET_COMMON(snd_ssthresh);
+ break;
+ case offsetof(struct bpf_tcp_sock, rcv_nxt):
+ BPF_TCP_SOCK_GET_COMMON(rcv_nxt);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_nxt):
+ BPF_TCP_SOCK_GET_COMMON(snd_nxt);
+ break;
+ case offsetof(struct bpf_tcp_sock, snd_una):
+ BPF_TCP_SOCK_GET_COMMON(snd_una);
+ break;
+ case offsetof(struct bpf_tcp_sock, mss_cache):
+ BPF_TCP_SOCK_GET_COMMON(mss_cache);
+ break;
+ case offsetof(struct bpf_tcp_sock, ecn_flags):
+ BPF_TCP_SOCK_GET_COMMON(ecn_flags);
+ break;
+ case offsetof(struct bpf_tcp_sock, rate_delivered):
+ BPF_TCP_SOCK_GET_COMMON(rate_delivered);
+ break;
+ case offsetof(struct bpf_tcp_sock, rate_interval_us):
+ BPF_TCP_SOCK_GET_COMMON(rate_interval_us);
+ break;
+ case offsetof(struct bpf_tcp_sock, packets_out):
+ BPF_TCP_SOCK_GET_COMMON(packets_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, retrans_out):
+ BPF_TCP_SOCK_GET_COMMON(retrans_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, total_retrans):
+ BPF_TCP_SOCK_GET_COMMON(total_retrans);
+ break;
+ case offsetof(struct bpf_tcp_sock, segs_in):
+ BPF_TCP_SOCK_GET_COMMON(segs_in);
+ break;
+ case offsetof(struct bpf_tcp_sock, data_segs_in):
+ BPF_TCP_SOCK_GET_COMMON(data_segs_in);
+ break;
+ case offsetof(struct bpf_tcp_sock, segs_out):
+ BPF_TCP_SOCK_GET_COMMON(segs_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, data_segs_out):
+ BPF_TCP_SOCK_GET_COMMON(data_segs_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, lost_out):
+ BPF_TCP_SOCK_GET_COMMON(lost_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, sacked_out):
+ BPF_TCP_SOCK_GET_COMMON(sacked_out);
+ break;
+ case offsetof(struct bpf_tcp_sock, bytes_received):
+ BPF_TCP_SOCK_GET_COMMON(bytes_received);
+ break;
+ case offsetof(struct bpf_tcp_sock, bytes_acked):
+ BPF_TCP_SOCK_GET_COMMON(bytes_acked);
+ break;
}
return insn - insn_buf;
@@ -7913,9 +7925,6 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
SOCK_OPS_GET_FIELD(BPF_FIELD, OBJ_FIELD, OBJ); \
} while (0)
- CONVERT_COMMON_TCP_SOCK_FIELDS(struct bpf_sock_ops,
- SOCK_OPS_GET_TCP_SOCK_FIELD);
-
if (insn > insn_buf)
return insn - insn_buf;
@@ -8085,6 +8094,69 @@ static u32 sock_ops_convert_ctx_access(enum bpf_access_type type,
SOCK_OPS_GET_OR_SET_FIELD(sk_txhash, sk_txhash,
struct sock, type);
break;
+ case offsetof(struct bpf_sock_ops, snd_cwnd):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_cwnd);
+ break;
+ case offsetof(struct bpf_sock_ops, srtt_us):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(srtt_us);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_ssthresh):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_ssthresh);
+ break;
+ case offsetof(struct bpf_sock_ops, rcv_nxt):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rcv_nxt);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_nxt):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_nxt);
+ break;
+ case offsetof(struct bpf_sock_ops, snd_una):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(snd_una);
+ break;
+ case offsetof(struct bpf_sock_ops, mss_cache):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(mss_cache);
+ break;
+ case offsetof(struct bpf_sock_ops, ecn_flags):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(ecn_flags);
+ break;
+ case offsetof(struct bpf_sock_ops, rate_delivered):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rate_delivered);
+ break;
+ case offsetof(struct bpf_sock_ops, rate_interval_us):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(rate_interval_us);
+ break;
+ case offsetof(struct bpf_sock_ops, packets_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(packets_out);
+ break;
+ case offsetof(struct bpf_sock_ops, retrans_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(retrans_out);
+ break;
+ case offsetof(struct bpf_sock_ops, total_retrans):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(total_retrans);
+ break;
+ case offsetof(struct bpf_sock_ops, segs_in):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(segs_in);
+ break;
+ case offsetof(struct bpf_sock_ops, data_segs_in):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(data_segs_in);
+ break;
+ case offsetof(struct bpf_sock_ops, segs_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(segs_out);
+ break;
+ case offsetof(struct bpf_sock_ops, data_segs_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(data_segs_out);
+ break;
+ case offsetof(struct bpf_sock_ops, lost_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(lost_out);
+ break;
+ case offsetof(struct bpf_sock_ops, sacked_out):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(sacked_out);
+ break;
+ case offsetof(struct bpf_sock_ops, bytes_received):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(bytes_received);
+ break;
+ case offsetof(struct bpf_sock_ops, bytes_acked):
+ SOCK_OPS_GET_TCP_SOCK_FIELD(bytes_acked);
+ break;
case offsetof(struct bpf_sock_ops, sk):
*insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF(
struct bpf_sock_ops_kern,
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 3/8] bpf: add dsack_dups/delivered{,_ce} to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-02 16:13 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Add more fields to bpf_tcp_sock that might be useful for debugging
congestion control issues.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 5 +++++
net/core/filter.c | 11 ++++++++++-
2 files changed, 15 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index 9cdd0aaeba06..bfb0b1a76684 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3073,6 +3073,11 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index ad908526545d..3da4b6c38b46 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,7 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, bytes_acked))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
return false;
if (off % size != 0)
@@ -5652,6 +5652,15 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, bytes_acked):
BPF_TCP_SOCK_GET_COMMON(bytes_acked);
break;
+ case offsetof(struct bpf_tcp_sock, dsack_dups):
+ BPF_TCP_SOCK_GET_COMMON(dsack_dups);
+ break;
+ case offsetof(struct bpf_tcp_sock, delivered):
+ BPF_TCP_SOCK_GET_COMMON(delivered);
+ break;
+ case offsetof(struct bpf_tcp_sock, delivered_ce):
+ BPF_TCP_SOCK_GET_COMMON(delivered_ce);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 4/8] bpf: add icsk_retransmits to bpf_tcp_sock
From: Stanislav Fomichev @ 2019-07-02 16:13 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Add some inet_connection_sock fields to bpf_tcp_sock that might be useful
for debugging congestion control issues.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
include/uapi/linux/bpf.h | 1 +
net/core/filter.c | 20 +++++++++++++++++++-
2 files changed, 20 insertions(+), 1 deletion(-)
diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
index bfb0b1a76684..ead27aebf491 100644
--- a/include/uapi/linux/bpf.h
+++ b/include/uapi/linux/bpf.h
@@ -3078,6 +3078,7 @@ struct bpf_tcp_sock {
*/
__u32 delivered; /* Total data packets delivered incl. rexmits */
__u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
diff --git a/net/core/filter.c b/net/core/filter.c
index 3da4b6c38b46..089aaea0ccc6 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -5544,7 +5544,8 @@ static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
bool bpf_tcp_sock_is_valid_access(int off, int size, enum bpf_access_type type,
struct bpf_insn_access_aux *info)
{
- if (off < 0 || off >= offsetofend(struct bpf_tcp_sock, delivered_ce))
+ if (off < 0 || off >= offsetofend(struct bpf_tcp_sock,
+ icsk_retransmits))
return false;
if (off % size != 0)
@@ -5575,6 +5576,20 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
offsetof(struct tcp_sock, FIELD)); \
} while (0)
+#define BPF_INET_SOCK_GET_COMMON(FIELD) \
+ do { \
+ BUILD_BUG_ON(FIELD_SIZEOF(struct inet_connection_sock, \
+ FIELD) > \
+ FIELD_SIZEOF(struct bpf_tcp_sock, FIELD)); \
+ *insn++ = BPF_LDX_MEM(BPF_FIELD_SIZEOF( \
+ struct inet_connection_sock, \
+ FIELD), \
+ si->dst_reg, si->src_reg, \
+ offsetof( \
+ struct inet_connection_sock, \
+ FIELD)); \
+ } while (0)
+
if (insn > insn_buf)
return insn - insn_buf;
@@ -5661,6 +5676,9 @@ u32 bpf_tcp_sock_convert_ctx_access(enum bpf_access_type type,
case offsetof(struct bpf_tcp_sock, delivered_ce):
BPF_TCP_SOCK_GET_COMMON(delivered_ce);
break;
+ case offsetof(struct bpf_tcp_sock, icsk_retransmits):
+ BPF_INET_SOCK_GET_COMMON(icsk_retransmits);
+ break;
}
return insn - insn_buf;
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 5/8] bpf/tools: sync bpf.h
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Sync new bpf_tcp_sock fields and new BPF_PROG_TYPE_SOCK_OPS RTT callback.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/include/uapi/linux/bpf.h | 12 +++++++++++-
1 file changed, 11 insertions(+), 1 deletion(-)
diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h
index a396b516a2b2..cecf42c871d4 100644
--- a/tools/include/uapi/linux/bpf.h
+++ b/tools/include/uapi/linux/bpf.h
@@ -1767,6 +1767,7 @@ union bpf_attr {
* * **BPF_SOCK_OPS_RTO_CB_FLAG** (retransmission time out)
* * **BPF_SOCK_OPS_RETRANS_CB_FLAG** (retransmission)
* * **BPF_SOCK_OPS_STATE_CB_FLAG** (TCP state change)
+ * * **BPF_SOCK_OPS_RTT_CB_FLAG** (every RTT)
*
* Therefore, this function can be used to clear a callback flag by
* setting the appropriate bit to zero. e.g. to disable the RTO
@@ -3069,6 +3070,12 @@ struct bpf_tcp_sock {
* sum(delta(snd_una)), or how many bytes
* were acked.
*/
+ __u32 dsack_dups; /* RFC4898 tcpEStatsStackDSACKDups
+ * total number of DSACK blocks received
+ */
+ __u32 delivered; /* Total data packets delivered incl. rexmits */
+ __u32 delivered_ce; /* Like the above but only ECE marked packets */
+ __u32 icsk_retransmits; /* Number of unrecovered [RTO] timeouts */
};
struct bpf_sock_tuple {
@@ -3311,7 +3318,8 @@ struct bpf_sock_ops {
#define BPF_SOCK_OPS_RTO_CB_FLAG (1<<0)
#define BPF_SOCK_OPS_RETRANS_CB_FLAG (1<<1)
#define BPF_SOCK_OPS_STATE_CB_FLAG (1<<2)
-#define BPF_SOCK_OPS_ALL_CB_FLAGS 0x7 /* Mask of all currently
+#define BPF_SOCK_OPS_RTT_CB_FLAG (1<<3)
+#define BPF_SOCK_OPS_ALL_CB_FLAGS 0xF /* Mask of all currently
* supported cb flags
*/
@@ -3366,6 +3374,8 @@ enum {
BPF_SOCK_OPS_TCP_LISTEN_CB, /* Called on listen(2), right after
* socket transition to LISTEN state.
*/
+ BPF_SOCK_OPS_RTT_CB, /* Called on every RTT.
+ */
};
/* List of TCP states. There is a build check in net/ipv4/tcp.c to detect
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 6/8] selftests/bpf: test BPF_SOCK_OPS_RTT_CB
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Make sure the callback is invoked for syn-ack and data packet.
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/Makefile | 3 +-
tools/testing/selftests/bpf/progs/tcp_rtt.c | 61 +++++
tools/testing/selftests/bpf/test_tcp_rtt.c | 254 ++++++++++++++++++++
3 files changed, 317 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/bpf/progs/tcp_rtt.c
create mode 100644 tools/testing/selftests/bpf/test_tcp_rtt.c
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index de1754a8f5fe..2620406a53ec 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -27,7 +27,7 @@ TEST_GEN_PROGS = test_verifier test_tag test_maps test_lru_map test_lpm_map test
test_cgroup_storage test_select_reuseport test_section_names \
test_netcnt test_tcpnotify_user test_sock_fields test_sysctl test_hashmap \
test_btf_dump test_cgroup_attach xdping test_sockopt test_sockopt_sk \
- test_sockopt_multi
+ test_sockopt_multi test_tcp_rtt
BPF_OBJ_FILES = $(patsubst %.c,%.o, $(notdir $(wildcard progs/*.c)))
TEST_GEN_FILES = $(BPF_OBJ_FILES)
@@ -107,6 +107,7 @@ $(OUTPUT)/test_cgroup_attach: cgroup_helpers.c
$(OUTPUT)/test_sockopt: cgroup_helpers.c
$(OUTPUT)/test_sockopt_sk: cgroup_helpers.c
$(OUTPUT)/test_sockopt_multi: cgroup_helpers.c
+$(OUTPUT)/test_tcp_rtt: cgroup_helpers.c
.PHONY: force
diff --git a/tools/testing/selftests/bpf/progs/tcp_rtt.c b/tools/testing/selftests/bpf/progs/tcp_rtt.c
new file mode 100644
index 000000000000..233bdcb1659e
--- /dev/null
+++ b/tools/testing/selftests/bpf/progs/tcp_rtt.c
@@ -0,0 +1,61 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/bpf.h>
+#include "bpf_helpers.h"
+
+char _license[] SEC("license") = "GPL";
+__u32 _version SEC("version") = 1;
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+struct bpf_map_def SEC("maps") socket_storage_map = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .key_size = sizeof(int),
+ .value_size = sizeof(struct tcp_rtt_storage),
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+BPF_ANNOTATE_KV_PAIR(socket_storage_map, int, struct tcp_rtt_storage);
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct tcp_rtt_storage *storage;
+ struct bpf_tcp_sock *tcp_sk;
+ int op = (int) ctx->op;
+ struct bpf_sock *sk;
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ storage = bpf_sk_storage_get(&socket_storage_map, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!storage)
+ return 1;
+
+ if (op == BPF_SOCK_OPS_TCP_CONNECT_CB) {
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ }
+
+ if (op != BPF_SOCK_OPS_RTT_CB)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ storage->invoked++;
+
+ storage->dsack_dups = tcp_sk->dsack_dups;
+ storage->delivered = tcp_sk->delivered;
+ storage->delivered_ce = tcp_sk->delivered_ce;
+ storage->icsk_retransmits = tcp_sk->icsk_retransmits;
+
+ return 1;
+}
diff --git a/tools/testing/selftests/bpf/test_tcp_rtt.c b/tools/testing/selftests/bpf/test_tcp_rtt.c
new file mode 100644
index 000000000000..90c3862f74a8
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_tcp_rtt.c
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <error.h>
+#include <errno.h>
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/types.h>
+#include <sys/socket.h>
+#include <netinet/in.h>
+#include <pthread.h>
+
+#include <linux/filter.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "bpf_rlimit.h"
+#include "bpf_util.h"
+#include "cgroup_helpers.h"
+
+#define CG_PATH "/tcp_rtt"
+
+struct tcp_rtt_storage {
+ __u32 invoked;
+ __u32 dsack_dups;
+ __u32 delivered;
+ __u32 delivered_ce;
+ __u32 icsk_retransmits;
+};
+
+static void send_byte(int fd)
+{
+ char b = 0x55;
+
+ if (write(fd, &b, sizeof(b)) != 1)
+ error(1, errno, "Failed to send single byte");
+}
+
+static int verify_sk(int map_fd, int client_fd, const char *msg, __u32 invoked,
+ __u32 dsack_dups, __u32 delivered, __u32 delivered_ce,
+ __u32 icsk_retransmits)
+{
+ int err = 0;
+ struct tcp_rtt_storage val;
+
+ if (bpf_map_lookup_elem(map_fd, &client_fd, &val) < 0)
+ error(1, errno, "Failed to read socket storage");
+
+ if (val.invoked != invoked) {
+ log_err("%s: unexpected bpf_tcp_sock.invoked %d != %d",
+ msg, val.invoked, invoked);
+ err++;
+ }
+
+ if (val.dsack_dups != dsack_dups) {
+ log_err("%s: unexpected bpf_tcp_sock.dsack_dups %d != %d",
+ msg, val.dsack_dups, dsack_dups);
+ err++;
+ }
+
+ if (val.delivered != delivered) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered %d != %d",
+ msg, val.delivered, delivered);
+ err++;
+ }
+
+ if (val.delivered_ce != delivered_ce) {
+ log_err("%s: unexpected bpf_tcp_sock.delivered_ce %d != %d",
+ msg, val.delivered_ce, delivered_ce);
+ err++;
+ }
+
+ if (val.icsk_retransmits != icsk_retransmits) {
+ log_err("%s: unexpected bpf_tcp_sock.icsk_retransmits %d != %d",
+ msg, val.icsk_retransmits, icsk_retransmits);
+ err++;
+ }
+
+ return err;
+}
+
+static int connect_to_server(int server_fd)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create client socket");
+ return -1;
+ }
+
+ if (getsockname(server_fd, (struct sockaddr *)&addr, &len)) {
+ log_err("Failed to get server addr");
+ goto out;
+ }
+
+ if (connect(fd, (const struct sockaddr *)&addr, len) < 0) {
+ log_err("Fail to connect to server");
+ goto out;
+ }
+
+ return fd;
+
+out:
+ close(fd);
+ return -1;
+}
+
+static int run_test(int cgroup_fd, int server_fd)
+{
+ struct bpf_prog_load_attr attr = {
+ .prog_type = BPF_PROG_TYPE_SOCK_OPS,
+ .file = "./tcp_rtt.o",
+ .expected_attach_type = BPF_CGROUP_SOCK_OPS,
+ };
+ struct bpf_object *obj;
+ struct bpf_map *map;
+ int client_fd;
+ int prog_fd;
+ int map_fd;
+ int err;
+
+ err = bpf_prog_load_xattr(&attr, &obj, &prog_fd);
+ if (err) {
+ log_err("Failed to load BPF object");
+ return -1;
+ }
+
+ map = bpf_map__next(NULL, obj);
+ map_fd = bpf_map__fd(map);
+
+ err = bpf_prog_attach(prog_fd, cgroup_fd, BPF_CGROUP_SOCK_OPS, 0);
+ if (err) {
+ log_err("Failed to attach BPF program");
+ goto close_bpf_object;
+ }
+
+ client_fd = connect_to_server(server_fd);
+ if (client_fd < 0) {
+ err = -1;
+ goto close_bpf_object;
+ }
+
+ err += verify_sk(map_fd, client_fd, "syn-ack",
+ /*invoked=*/1,
+ /*dsack_dups=*/0,
+ /*delivered=*/1,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ send_byte(client_fd);
+
+ err += verify_sk(map_fd, client_fd, "first payload byte",
+ /*invoked=*/2,
+ /*dsack_dups=*/0,
+ /*delivered=*/2,
+ /*delivered_ce=*/0,
+ /*icsk_retransmits=*/0);
+
+ close(client_fd);
+
+close_bpf_object:
+ bpf_object__close(obj);
+ return err;
+}
+
+static int start_server(void)
+{
+ struct sockaddr_in addr = {
+ .sin_family = AF_INET,
+ .sin_addr.s_addr = htonl(INADDR_LOOPBACK),
+ };
+ int fd;
+
+ fd = socket(AF_INET, SOCK_STREAM, 0);
+ if (fd < 0) {
+ log_err("Failed to create server socket");
+ return -1;
+ }
+
+ if (bind(fd, (const struct sockaddr *)&addr, sizeof(addr)) < 0) {
+ log_err("Failed to bind socket");
+ close(fd);
+ return -1;
+ }
+
+ return fd;
+}
+
+static void *server_thread(void *arg)
+{
+ struct sockaddr_storage addr;
+ socklen_t len = sizeof(addr);
+ int fd = *(int *)arg;
+ int client_fd;
+
+ if (listen(fd, 1) < 0)
+ error(1, errno, "Failed to listed on socket");
+
+ client_fd = accept(fd, (struct sockaddr *)&addr, &len);
+ if (client_fd < 0)
+ error(1, errno, "Failed to accept client");
+
+ /* Wait for the next connection (that never arrives)
+ * to keep this thread alive to prevent calling
+ * close() on client_fd.
+ */
+ if (accept(fd, (struct sockaddr *)&addr, &len) >= 0)
+ error(1, errno, "Unexpected success in second accept");
+
+ close(client_fd);
+
+ return NULL;
+}
+
+int main(int args, char **argv)
+{
+ int server_fd, cgroup_fd;
+ int err = EXIT_SUCCESS;
+ pthread_t tid;
+
+ if (setup_cgroup_environment())
+ goto cleanup_obj;
+
+ cgroup_fd = create_and_get_cgroup(CG_PATH);
+ if (cgroup_fd < 0)
+ goto cleanup_cgroup_env;
+
+ if (join_cgroup(CG_PATH))
+ goto cleanup_cgroup;
+
+ server_fd = start_server();
+ if (server_fd < 0) {
+ err = EXIT_FAILURE;
+ goto cleanup_cgroup;
+ }
+
+ pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
+
+ if (run_test(cgroup_fd, server_fd))
+ err = EXIT_FAILURE;
+
+ close(server_fd);
+
+ printf("test_sockopt_sk: %s\n",
+ err == EXIT_SUCCESS ? "PASSED" : "FAILED");
+
+cleanup_cgroup:
+ close(cgroup_fd);
+cleanup_cgroup_env:
+ cleanup_cgroup_environment();
+cleanup_obj:
+ return err;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 7/8] samples/bpf: add sample program that periodically dumps TCP stats
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Eric Dumazet,
Priyaranjan Jha, Yuchung Cheng, Soheil Hassas Yeganeh
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Uses new RTT callback to dump stats every second.
$ mkdir -p /tmp/cgroupv2
$ mount -t cgroup2 none /tmp/cgroupv2
$ mkdir -p /tmp/cgroupv2/foo
$ echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
$ bpftool prog load ./tcp_dumpstats_kern.o /sys/fs/bpf/tcp_prog
$ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
$ bpftool prog tracelog
$ # run neper/netperf/etc
Used neper to compare performance with and without this program attached
and didn't see any noticeable performance impact.
Sample output:
<idle>-0 [015] ..s. 2074.128800: 0: dsack_dups=0 delivered=242526
<idle>-0 [015] ..s. 2074.128808: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [015] ..s. 2075.130133: 0: dsack_dups=0 delivered=323599
<idle>-0 [015] ..s. 2075.130138: 0: delivered_ce=0 icsk_retransmits=0
<idle>-0 [005] .Ns. 2076.131440: 0: dsack_dups=0 delivered=404648
<idle>-0 [005] .Ns. 2076.131447: 0: delivered_ce=0 icsk_retransmits=0
Cc: Eric Dumazet <edumazet@google.com>
Cc: Priyaranjan Jha <priyarjha@google.com>
Cc: Yuchung Cheng <ycheng@google.com>
Cc: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
samples/bpf/Makefile | 1 +
samples/bpf/tcp_dumpstats_kern.c | 68 ++++++++++++++++++++++++++++++++
2 files changed, 69 insertions(+)
create mode 100644 samples/bpf/tcp_dumpstats_kern.c
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 0917f8cf4fab..eaebbeead42f 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -154,6 +154,7 @@ always += tcp_iw_kern.o
always += tcp_clamp_kern.o
always += tcp_basertt_kern.o
always += tcp_tos_reflect_kern.o
+always += tcp_dumpstats_kern.o
always += xdp_redirect_kern.o
always += xdp_redirect_map_kern.o
always += xdp_redirect_cpu_kern.o
diff --git a/samples/bpf/tcp_dumpstats_kern.c b/samples/bpf/tcp_dumpstats_kern.c
new file mode 100644
index 000000000000..8557913106a0
--- /dev/null
+++ b/samples/bpf/tcp_dumpstats_kern.c
@@ -0,0 +1,68 @@
+// SPDX-License-Identifier: GPL-2.0
+/* Refer to samples/bpf/tcp_bpf.readme for the instructions on
+ * how to run this sample program.
+ */
+#include <linux/bpf.h>
+
+#include "bpf_helpers.h"
+#include "bpf_endian.h"
+
+#define INTERVAL 1000000000ULL
+
+int _version SEC("version") = 1;
+char _license[] SEC("license") = "GPL";
+
+struct {
+ __u32 type;
+ __u32 map_flags;
+ int *key;
+ __u64 *value;
+} bpf_next_dump SEC(".maps") = {
+ .type = BPF_MAP_TYPE_SK_STORAGE,
+ .map_flags = BPF_F_NO_PREALLOC,
+};
+
+SEC("sockops")
+int _sockops(struct bpf_sock_ops *ctx)
+{
+ struct bpf_tcp_sock *tcp_sk;
+ struct bpf_sock *sk;
+ __u64 *next_dump;
+ __u64 now;
+
+ switch (ctx->op) {
+ case BPF_SOCK_OPS_TCP_CONNECT_CB:
+ bpf_sock_ops_cb_flags_set(ctx, BPF_SOCK_OPS_RTT_CB_FLAG);
+ return 1;
+ case BPF_SOCK_OPS_RTT_CB:
+ break;
+ default:
+ return 1;
+ }
+
+ sk = ctx->sk;
+ if (!sk)
+ return 1;
+
+ next_dump = bpf_sk_storage_get(&bpf_next_dump, sk, 0,
+ BPF_SK_STORAGE_GET_F_CREATE);
+ if (!next_dump)
+ return 1;
+
+ now = bpf_ktime_get_ns();
+ if (now < *next_dump)
+ return 1;
+
+ tcp_sk = bpf_tcp_sock(sk);
+ if (!tcp_sk)
+ return 1;
+
+ *next_dump = now + INTERVAL;
+
+ bpf_printk("dsack_dups=%u delivered=%u\n",
+ tcp_sk->dsack_dups, tcp_sk->delivered);
+ bpf_printk("delivered_ce=%u icsk_retransmits=%u\n",
+ tcp_sk->delivered_ce, tcp_sk->icsk_retransmits);
+
+ return 1;
+}
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* [PATCH bpf-next v2 8/8] samples/bpf: fix tcp_bpf.readme detach command
From: Stanislav Fomichev @ 2019-07-02 16:14 UTC (permalink / raw)
To: netdev, bpf
Cc: davem, ast, daniel, Stanislav Fomichev, Soheil Hassas Yeganeh,
Yuchung Cheng
In-Reply-To: <20190702161403.191066-1-sdf@google.com>
Copy-paste, should be detach, not attach.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Soheil Hassas Yeganeh <soheil@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
---
samples/bpf/tcp_bpf.readme | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
index fee746621aec..78e247f62108 100644
--- a/samples/bpf/tcp_bpf.readme
+++ b/samples/bpf/tcp_bpf.readme
@@ -25,4 +25,4 @@ attached to the cgroupv2).
To remove (unattach) a socket_ops BPF program from a cgroupv2:
- bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
+ bpftool cgroup detach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
--
2.22.0.410.gd8fdbe21b5-goog
^ permalink raw reply related
* Re: [for-next V2 06/10] linux/dim: Move implementation to .c files
From: Geert Uytterhoeven @ 2019-07-02 16:15 UTC (permalink / raw)
To: Saeed Mahameed, Tal Gilboa
Cc: David S. Miller, Doug Ledford, Jason Gunthorpe, Leon Romanovsky,
Or Gerlitz, Sagi Grimberg, netdev@vger.kernel.org,
linux-rdma@vger.kernel.org, linux-kernel
In-Reply-To: <20190625205701.17849-7-saeedm@mellanox.com>
Hi Saeed, Tal,
On Tue, 25 Jun 2019, Saeed Mahameed wrote:
> From: Tal Gilboa <talgi@mellanox.com>
>
> Moved all logic from dim.h and net_dim.h to dim.c and net_dim.c.
> This is both more structurally appealing and would allow to only
> expose externally used functions.
>
> Signed-off-by: Tal Gilboa <talgi@mellanox.com>
> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
This is now commit 4f75da3666c0c572 ("linux/dim: Move implementation to
.c files") in net-next.
> --- a/drivers/net/ethernet/broadcom/Kconfig
> +++ b/drivers/net/ethernet/broadcom/Kconfig
> @@ -8,6 +8,7 @@ config NET_VENDOR_BROADCOM
> default y
> depends on (SSB_POSSIBLE && HAS_DMA) || PCI || BCM63XX || \
> SIBYTE_SB1xxx_SOC
> + select DIMLIB
Merely enabling a NET_VENDOR_* symbol should not enable inclusion of
any additional code, cfr. the help text for the NET_VENDOR_BROADCOM
option.
Hence please move the select to the config symbol(s) for the driver(s)
that need it.
> --- a/lib/Kconfig
> +++ b/lib/Kconfig
> @@ -562,6 +562,14 @@ config SIGNATURE
> Digital signature verification. Currently only RSA is supported.
> Implementation is done using GnuPG MPI library
>
> +config DIMLIB
> + bool "DIM library"
> + default y
Please drop this line, as optional library code should never be included
by default.
Thanks!
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* WARNING: locking bug in do_ipv6_getsockopt
From: syzbot @ 2019-07-02 16:17 UTC (permalink / raw)
To: davem, kuznet, linux-kernel, netdev, syzkaller-bugs, yoshfuji
Hello,
syzbot found the following crash on:
HEAD commit: 6fbc7275 Linux 5.2-rc7
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=13fb8fe5a00000
kernel config: https://syzkaller.appspot.com/x/.config?x=f6451f0da3d42d53
dashboard link: https://syzkaller.appspot.com/bug?extid=babfdd7368c72aac3875
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1460390ba00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+babfdd7368c72aac3875@syzkaller.appspotmail.com
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
arch_local_save_flags arch/x86/include/asm/paravirt.h:762 [inline]
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
arch_local_save_flags arch/x86/include/asm/paravirt.h:760 [inline]
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
look_up_lock_class kernel/locking/lockdep.c:726 [inline]
WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735
register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
Kernel panic - not syncing: panic_on_warn set ...
CPU: 1 PID: 18374 Comm: syz-executor.0 Not tainted 5.2.0-rc7 #65
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
panic+0x2cb/0x744 kernel/panic.c:219
__warn.cold+0x20/0x4d kernel/panic.c:576
report_bug+0x263/0x2b0 lib/bug.c:186
fixup_bug arch/x86/kernel/traps.c:179 [inline]
fixup_bug arch/x86/kernel/traps.c:174 [inline]
do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:986
RIP: 0010:look_up_lock_class kernel/locking/lockdep.c:735 [inline]
RIP: 0010:register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
Code: 00 48 89 da 4d 8b 76 c0 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 80
3c 02 00 0f 85 23 07 00 00 4c 89 33 e9 e3 f4 ff ff 0f 0b <0f> 0b e9 ea f3
ff ff 44 89 e0 4c 8b 95 50 ff ff ff 83 c0 01 4c 8b
RSP: 0018:ffff88809ea3f678 EFLAGS: 00010087
RAX: dffffc0000000000 RBX: ffff888090b948e0 RCX: 0000000000000000
RDX: 1ffff1101217291f RSI: 0000000000000000 RDI: ffff888090b948f8
RBP: ffff88809ea3f740 R08: 1ffff11013d47ed7 R09: ffffffff8a65ad40
R10: ffffffff8a2e9180 R11: 0000000000000000 R12: ffffffff8a3243a0
R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff88023e60
__lock_acquire+0x116/0x5490 kernel/locking/lockdep.c:3674
lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4303
__raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline]
_raw_spin_lock_bh+0x33/0x50 kernel/locking/spinlock.c:175
spin_lock_bh include/linux/spinlock.h:343 [inline]
lock_sock_nested+0x41/0x120 net/core/sock.c:2913
lock_sock include/net/sock.h:1522 [inline]
do_ipv6_getsockopt.isra.0+0x289/0x2590 net/ipv6/ipv6_sockglue.c:1167
ipv6_getsockopt+0x186/0x280 net/ipv6/ipv6_sockglue.c:1391
udpv6_getsockopt+0x68/0xa0 net/ipv6/udp.c:1590
sock_common_getsockopt+0x94/0xd0 net/core/sock.c:3085
__sys_getsockopt+0x15f/0x240 net/socket.c:2109
__do_sys_getsockopt net/socket.c:2120 [inline]
__se_sys_getsockopt net/socket.c:2117 [inline]
__x64_sys_getsockopt+0xbe/0x150 net/socket.c:2117
do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
entry_SYSCALL_64_after_hwframe+0x49/0xbe
RIP: 0033:0x4597c9
Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7
48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff
ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
RSP: 002b:00007fb5fd78ac78 EFLAGS: 00000246 ORIG_RAX: 0000000000000037
RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000004597c9
RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000004
RBP: 000000000075bfc8 R08: 0000000020000080 R09: 0000000000000000
R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb5fd78b6d4
R13: 00000000004c0803 R14: 00000000004d2b88 R15: 00000000ffffffff
Kernel Offset: disabled
Rebooting in 86400 seconds..
---
This bug is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.
syzbot will keep track of this bug report. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this bug, for details see:
https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* RE: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Parav Pandit @ 2019-07-02 16:18 UTC (permalink / raw)
To: Andrew Lunn, Sudarsana Reddy Kalluru
Cc: davem@davemloft.net, netdev@vger.kernel.org,
mkalderon@marvell.com, aelior@marvell.com, jiri@resnulli.us
In-Reply-To: <20190702161133.GP30468@lunn.ch>
> -----Original Message-----
> From: netdev-owner@vger.kernel.org <netdev-owner@vger.kernel.org> On
> Behalf Of Andrew Lunn
> Sent: Tuesday, July 2, 2019 9:42 PM
> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> Cc: davem@davemloft.net; netdev@vger.kernel.org; mkalderon@marvell.com;
> aelior@marvell.com; jiri@resnulli.us
> Subject: Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the
> port parameters.
>
> On Tue, Jul 02, 2019 at 08:20:56AM -0700, Sudarsana Reddy Kalluru wrote:
> > The patch adds devlink interfaces for drivers to publish/unpublish the
> > devlink port parameters.
>
> Hi Sudarsana
>
> A good commit message says more about 'why' than 'what'. I can see the
> 'what' by reading the code. But the 'why' is often not so clear.
>
> Why would i want to unpublish port parameters?
>
> Thanks
> Andrew
A vendor driver calling these APIs is needed at minimum.
^ permalink raw reply
* Re: WARNING: locking bug in do_ipv6_getsockopt
From: Eric Dumazet @ 2019-07-02 16:30 UTC (permalink / raw)
To: syzbot, davem, kuznet, linux-kernel, netdev, syzkaller-bugs,
yoshfuji
In-Reply-To: <000000000000607bf4058cb5135c@google.com>
On 7/2/19 9:17 AM, syzbot wrote:
> Hello,
>
> syzbot found the following crash on:
>
> HEAD commit: 6fbc7275 Linux 5.2-rc7
> git tree: upstream
> console output: https://syzkaller.appspot.com/x/log.txt?x=13fb8fe5a00000
> kernel config: https://syzkaller.appspot.com/x/.config?x=f6451f0da3d42d53
> dashboard link: https://syzkaller.appspot.com/bug?extid=babfdd7368c72aac3875
> compiler: gcc (GCC) 9.0.0 20181231 (experimental)
> syz repro: https://syzkaller.appspot.com/x/repro.syz?x=1460390ba00000
>
> IMPORTANT: if you fix the bug, please add the following tag to the commit:
> Reported-by: syzbot+babfdd7368c72aac3875@syzkaller.appspotmail.com
>
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 arch_local_save_flags arch/x86/include/asm/paravirt.h:762 [inline]
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 arch_local_save_flags arch/x86/include/asm/paravirt.h:760 [inline]
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 look_up_lock_class kernel/locking/lockdep.c:726 [inline]
> WARNING: CPU: 1 PID: 18374 at kernel/locking/lockdep.c:735 register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
WARN_ON_ONCE(class->name != lock->name);
Might be useful to trace the content of class->name and lock->name.
> Kernel panic - not syncing: panic_on_warn set ...
> CPU: 1 PID: 18374 Comm: syz-executor.0 Not tainted 5.2.0-rc7 #65
> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0x172/0x1f0 lib/dump_stack.c:113
> panic+0x2cb/0x744 kernel/panic.c:219
> __warn.cold+0x20/0x4d kernel/panic.c:576
> report_bug+0x263/0x2b0 lib/bug.c:186
> fixup_bug arch/x86/kernel/traps.c:179 [inline]
> fixup_bug arch/x86/kernel/traps.c:174 [inline]
> do_error_trap+0x11b/0x200 arch/x86/kernel/traps.c:272
> do_invalid_op+0x37/0x50 arch/x86/kernel/traps.c:291
> invalid_op+0x14/0x20 arch/x86/entry/entry_64.S:986
> RIP: 0010:look_up_lock_class kernel/locking/lockdep.c:735 [inline]
> RIP: 0010:register_lock_class+0xe10/0x1860 kernel/locking/lockdep.c:1079
> Code: 00 48 89 da 4d 8b 76 c0 48 b8 00 00 00 00 00 fc ff df 48 c1 ea 03 80 3c 02 00 0f 85 23 07 00 00 4c 89 33 e9 e3 f4 ff ff 0f 0b <0f> 0b e9 ea f3 ff ff 44 89 e0 4c 8b 95 50 ff ff ff 83 c0 01 4c 8b
> RSP: 0018:ffff88809ea3f678 EFLAGS: 00010087
> RAX: dffffc0000000000 RBX: ffff888090b948e0 RCX: 0000000000000000
> RDX: 1ffff1101217291f RSI: 0000000000000000 RDI: ffff888090b948f8
> RBP: ffff88809ea3f740 R08: 1ffff11013d47ed7 R09: ffffffff8a65ad40
> R10: ffffffff8a2e9180 R11: 0000000000000000 R12: ffffffff8a3243a0
> R13: 0000000000000000 R14: 0000000000000000 R15: ffffffff88023e60
> __lock_acquire+0x116/0x5490 kernel/locking/lockdep.c:3674
> lock_acquire+0x16f/0x3f0 kernel/locking/lockdep.c:4303
> __raw_spin_lock_bh include/linux/spinlock_api_smp.h:135 [inline]
> _raw_spin_lock_bh+0x33/0x50 kernel/locking/spinlock.c:175
> spin_lock_bh include/linux/spinlock.h:343 [inline]
> lock_sock_nested+0x41/0x120 net/core/sock.c:2913
> lock_sock include/net/sock.h:1522 [inline]
> do_ipv6_getsockopt.isra.0+0x289/0x2590 net/ipv6/ipv6_sockglue.c:1167
> ipv6_getsockopt+0x186/0x280 net/ipv6/ipv6_sockglue.c:1391
> udpv6_getsockopt+0x68/0xa0 net/ipv6/udp.c:1590
> sock_common_getsockopt+0x94/0xd0 net/core/sock.c:3085
> __sys_getsockopt+0x15f/0x240 net/socket.c:2109
> __do_sys_getsockopt net/socket.c:2120 [inline]
> __se_sys_getsockopt net/socket.c:2117 [inline]
> __x64_sys_getsockopt+0xbe/0x150 net/socket.c:2117
> do_syscall_64+0xfd/0x680 arch/x86/entry/common.c:301
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x4597c9
> Code: fd b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00 00 66 90 48 89 f8 48 89 f7 48 89 d6 48 89 ca 4d 89 c2 4d 89 c8 4c 8b 4c 24 08 0f 05 <48> 3d 01 f0 ff ff 0f 83 cb b7 fb ff c3 66 2e 0f 1f 84 00 00 00 00
> RSP: 002b:00007fb5fd78ac78 EFLAGS: 00000246 ORIG_RAX: 0000000000000037
> RAX: ffffffffffffffda RBX: 0000000000000005 RCX: 00000000004597c9
> RDX: 0000000000000036 RSI: 0000000000000029 RDI: 0000000000000004
> RBP: 000000000075bfc8 R08: 0000000020000080 R09: 0000000000000000
> R10: 0000000000000000 R11: 0000000000000246 R12: 00007fb5fd78b6d4
> R13: 00000000004c0803 R14: 00000000004d2b88 R15: 00000000ffffffff
> Kernel Offset: disabled
> Rebooting in 86400 seconds..
>
>
> ---
> This bug is generated by a bot. It may contain errors.
> See https://goo.gl/tpsmEJ for more information about syzbot.
> syzbot engineers can be reached at syzkaller@googlegroups.com.
>
> syzbot will keep track of this bug report. See:
> https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
> syzbot can test patches for this bug, for details see:
> https://goo.gl/tpsmEJ#testing-patches
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Jonathan Lemon @ 2019-07-02 16:31 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: netdev, bjorn.topel, magnus.karlsson, jakub.kicinski,
jeffrey.t.kirsher, kernel-team
In-Reply-To: <f9c7b86c-91a6-9585-d1f1-f6f325794038@mellanox.com>
On 2 Jul 2019, at 0:07, Maxim Mikityanskiy wrote:
> On 2019-06-29 01:15, Jonathan Lemon wrote:
>> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
>> + if (unlikely(!xdpf))
>> + return I40E_XDP_CONSUMED;
>> + xdpf->handle = xdp->handle;
>
> Shouldn't this line belong to convert_to_xdp_frame_keep_zc (and the
> previous patch)? It looks like it's code common for all drivers, and
> also patch 1 adds the handle field, but doesn't use it, which looks weird.
Good point. I'll move it into the function.
--
Jonathan
^ permalink raw reply
* Re: [PATCH 00/11] XDP unaligned chunk placement support
From: Jonathan Lemon @ 2019-07-02 16:33 UTC (permalink / raw)
To: Richardson, Bruce
Cc: Jakub Kicinski, Laatz, Kevin, netdev, ast, daniel, Topel, Bjorn,
Karlsson, Magnus, bpf, intel-wired-lan, Loftus, Ciara
In-Reply-To: <59AF69C657FD0841A61C55336867B5B07ED8B210@IRSMSX103.ger.corp.intel.com>
On 2 Jul 2019, at 2:27, Richardson, Bruce wrote:
>> -----Original Message-----
>> From: Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
>> Sent: Monday, July 1, 2019 10:20 PM
>> To: Laatz, Kevin <kevin.laatz@intel.com>
>> Cc: Jonathan Lemon <jonathan.lemon@gmail.com>;
>> netdev@vger.kernel.org;
>> ast@kernel.org; daniel@iogearbox.net; Topel, Bjorn
>> <bjorn.topel@intel.com>; Karlsson, Magnus
>> <magnus.karlsson@intel.com>;
>> bpf@vger.kernel.org; intel-wired-lan@lists.osuosl.org; Richardson,
>> Bruce
>> <bruce.richardson@intel.com>; Loftus, Ciara <ciara.loftus@intel.com>
>> Subject: Re: [PATCH 00/11] XDP unaligned chunk placement support
>>
>> On Mon, 1 Jul 2019 15:44:29 +0100, Laatz, Kevin wrote:
>>> On 28/06/2019 21:29, Jonathan Lemon wrote:
>>>> On 28 Jun 2019, at 9:19, Laatz, Kevin wrote:
>>>>> On 27/06/2019 22:25, Jakub Kicinski wrote:
>>>>>> I think that's very limiting. What is the challenge in
>>>>>> providing
>>>>>> aligned addresses, exactly?
>>>>> The challenges are two-fold:
>>>>> 1) it prevents using arbitrary buffer sizes, which will be an
>>>>> issue
>>>>> supporting e.g. jumbo frames in future.
>>>>> 2) higher level user-space frameworks which may want to use
>>>>> AF_XDP,
>>>>> such as DPDK, do not currently support having buffers with 'fixed'
>>>>> alignment.
>>>>> The reason that DPDK uses arbitrary placement is that:
>>>>> - it would stop things working on certain NICs which
>>>>> need
>>>>> the actual writable space specified in units of 1k - therefore we
>>>>> need 2k
>>>>> + metadata space.
>>>>> - we place padding between buffers to avoid
>>>>> constantly
>>>>> hitting the same memory channels when accessing memory.
>>>>> - it allows the application to choose the actual
>>>>> buffer
>>>>> size it wants to use.
>>>>> We make use of the above to allow us to speed up processing
>>>>> significantly and also reduce the packet buffer memory size.
>>>>>
>>>>> Not having arbitrary buffer alignment also means an AF_XDP
>>>>> driver for DPDK cannot be a drop-in replacement for existing
>>>>> drivers in those frameworks. Even with a new capability to allow
>>>>> an
>>>>> arbitrary buffer alignment, existing apps will need to be modified
>>>>> to use that new capability.
>>>>
>>>> Since all buffers in the umem are the same chunk size, the original
>>>> buffer address can be recalculated with some multiply/shift math.
>>>> However, this is more expensive than just a mask operation.
>>>
>>> Yes, we can do this.
>>
>> That'd be best, can DPDK reasonably guarantee the slicing is uniform?
>> E.g. it's not desperate buffer pools with different bases?
>
> It's generally uniform, but handling the crossing of (huge)page
> boundaries
> complicates things a bit. Therefore I think the final option below
> is best as it avoids any such problems.
>
>>
>>> Another option we have is to add a socket option for querying the
>>> metadata length from the driver (assuming it doesn't vary per
>>> packet).
>>> We can use that information to get back to the original address
>>> using
>>> subtraction.
>>
>> Unfortunately the metadata depends on the packet and how much info
>> the
>> device was able to extract. So it's variable length.
>>
>>> Alternatively, we can change the Rx descriptor format to include the
>>> metadata length. We could do this in a couple of ways, for example,
>>> rather than returning the address as the start of the packet,
>>> instead
>>> return the buffer address that was passed in, and adding another
>>> 16-bit field to specify the start of packet offset with that buffer.
>>> If using another 16-bits of the descriptor space is not desirable,
>>> an
>>> alternative could be to limit umem sizes to e.g. 2^48 bits (256
>>> terabytes should be enough, right :-) ) and use the remaining 16
>>> bits
>>> of the address as a packet offset. Other variations on these
>>> approach
>>> are obviously possible too.
>>
>> Seems reasonable to me..
>
> I think this is probably the best solution, and also has the advantage
> that
> a buffer retains its base address the full way through the cycle of Rx
> and Tx.
I like this as well - it also has the advantage that drivers can keep
performing adjustments on the handle, which ends up just modifying the
offset.
--
Jonathan
^ permalink raw reply
* Re: [PATCH net-next v6 05/15] ethtool: helper functions for netlink interface
From: Michal Kubecek @ 2019-07-02 16:34 UTC (permalink / raw)
To: netdev
Cc: Jiri Pirko, David Miller, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <20190702130515.GO2250@nanopsycho>
On Tue, Jul 02, 2019 at 03:05:15PM +0200, Jiri Pirko wrote:
> Tue, Jul 02, 2019 at 01:50:04PM CEST, mkubecek@suse.cz wrote:
> >
> >+/* request header */
> >+
> >+/* use compact bitsets in reply */
> >+#define ETHTOOL_RF_COMPACT (1 << 0)
>
> "COMPACT_BITSETS"?
>
> >+/* provide optional reply for SET or ACT requests */
> >+#define ETHTOOL_RF_REPLY (1 << 1)
>
> "OPTIONAL_REPLY"?
OK
> >+ ret = nla_parse_nested(tb, ETHTOOL_A_HEADER_MAX, nest,
> >+ policy ?: dflt_header_policy, extack);
> >+ if (ret < 0)
>
> if (ret)
>
> Same remark goes to the rest of the code (also the rest of the patches),
> in case called function cannot return positive values.
The "if (ret < 0)" idiom for "on error do ..." is so ubiquitous through
the whole kernel that I don't think it's worth it to carefully check
which function can return a positive value and which cannot and risk
that one day I overlook that some function. And yet another question is
what exactly "cannot return" means: is it whenever the function does not
return a positive value or only if it's explicitly documented not to?
Looking at existing networking code, e.g. net/netfilter (except ipvs),
net/sched or net/core/rtnetlink.c are using "if (ret < 0)" rather
uniformly. And (as you objected to the check of genl_register_family()
previous patch) even genetlink itself has
err = genl_register_family(&genl_ctrl);
if (err < 0)
goto problem;
in genl_init().
>
>
> >+ return ret;
> >+ devname_attr = tb[ETHTOOL_A_HEADER_DEV_NAME];
> >+
> >+ if (tb[ETHTOOL_A_HEADER_DEV_INDEX]) {
> >+ u32 ifindex = nla_get_u32(tb[ETHTOOL_A_HEADER_DEV_INDEX]);
> >+
> >+ dev = dev_get_by_index(net, ifindex);
> >+ if (!dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack,
> >+ tb[ETHTOOL_A_HEADER_DEV_INDEX],
> >+ "no device matches ifindex");
> >+ return -ENODEV;
> >+ }
> >+ /* if both ifindex and ifname are passed, they must match */
> >+ if (devname_attr &&
> >+ strncmp(dev->name, nla_data(devname_attr), IFNAMSIZ)) {
> >+ dev_put(dev);
> >+ NL_SET_ERR_MSG_ATTR(extack, nest,
> >+ "ifindex and name do not match");
> >+ return -ENODEV;
> >+ }
> >+ } else if (devname_attr) {
> >+ dev = dev_get_by_name(net, nla_data(devname_attr));
> >+ if (!dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack, devname_attr,
> >+ "no device matches name");
> >+ return -ENODEV;
> >+ }
> >+ } else if (require_dev) {
> >+ NL_SET_ERR_MSG_ATTR(extack, nest,
> >+ "neither ifindex nor name specified");
> >+ return -EINVAL;
> >+ }
> >+
> >+ if (dev && !netif_device_present(dev)) {
> >+ dev_put(dev);
> >+ NL_SET_ERR_MSG(extack, "device not present");
> >+ return -ENODEV;
> >+ }
> >+
> >+ req_info->dev = dev;
> >+ ethnl_update_u32(&req_info->req_mask, tb[ETHTOOL_A_HEADER_INFOMASK]);
> >+ ethnl_update_u32(&req_info->global_flags, tb[ETHTOOL_A_HEADER_GFLAGS]);
> >+ ethnl_update_u32(&req_info->req_flags, tb[ETHTOOL_A_HEADER_RFLAGS]);
>
> Just:
> req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK];
> ...
>
> Not sure what ethnl_update_u32() is good for, but it is not needed here.
That would result in null pointer dereference if the attribute is
missing. So you would need at least
if (tb[ETHTOOL_A_HEADER_INFOMASK])
req_info->req_mask = nla_get_u32(tb[ETHTOOL_A_HEADER_INFOMASK]);
if (tb[ETHTOOL_A_HEADER_GFLAGS])
req_info->global_flags =
nla_get_u32(tb[ETHTOOL_A_HEADER_GFLAGS]);
if (tb[ETHTOOL_A_HEADER_RFLAGS])
req_info->req_flags = nla_get_u32(tb[ETHTOOL_A_HEADER_RFLAGS]);
I don't think it looks better.
> >+
> >+ return 0;
> >+}
> >+
> >+/**
> >+ * ethnl_fill_reply_header() - Put standard header into a reply message
> >+ * @skb: skb with the message
> >+ * @dev: network device to describe in header
> >+ * @attrtype: attribute type to use for the nest
> >+ *
> >+ * Create a nested attribute with attributes describing given network device.
> >+ * Clean up on error.
>
> Cleanup is obvious, no need to mention it in API docs.
OK
> >+ *
> >+ * Return: 0 on success, error value (-EMSGSIZE only) on error
> >+ */
> >+int ethnl_fill_reply_header(struct sk_buff *skb, struct net_device *dev,
> >+ u16 attrtype)
> >+{
> >+ struct nlattr *nest;
> >+
> >+ if (!dev)
> >+ return 0;
> >+ nest = nla_nest_start(skb, attrtype);
> >+ if (!nest)
> >+ return -EMSGSIZE;
> >+
> >+ if (nla_put_u32(skb, ETHTOOL_A_HEADER_DEV_INDEX, (u32)dev->ifindex) ||
> >+ nla_put_string(skb, ETHTOOL_A_HEADER_DEV_NAME, dev->name))
> >+ goto nla_put_failure;
> >+ /* If more attributes are put into reply header, ethnl_header_size()
> >+ * must be updated to account for them.
> >+ */
> >+
> >+ nla_nest_end(skb, nest);
> >+ return 0;
> >+
> >+nla_put_failure:
> >+ nla_nest_cancel(skb, nest);
> >+ return -EMSGSIZE;
> >+}
> >+
> >+/**
> >+ * ethnl_reply_init() - Create skb for a reply and fill device identification
> >+ * @payload: payload length (without netlink and genetlink header)
> >+ * @dev: device the reply is about (may be null)
> >+ * @cmd: ETHTOOL_MSG_* message type for reply
> >+ * @info: genetlink info of the received packet we respond to
> >+ * @ehdrp: place to store payload pointer returned by genlmsg_new()
> >+ *
> >+ * Return: pointer to allocated skb on success, NULL on error
> >+ */
> >+struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
> >+ u16 hdr_attrtype, struct genl_info *info,
> >+ void **ehdrp)
> >+{
> >+ struct sk_buff *skb;
> >+
> >+ skb = genlmsg_new(payload, GFP_KERNEL);
> >+ if (!skb)
> >+ goto err;
> >+ *ehdrp = genlmsg_put_reply(skb, info, ðtool_genl_family, 0, cmd);
> >+ if (!*ehdrp)
> >+ goto err_free;
> >+
> >+ if (dev) {
> >+ int ret;
> >+
> >+ ret = ethnl_fill_reply_header(skb, dev, hdr_attrtype);
> >+ if (ret < 0)
> >+ goto err;
> >+ }
> >+ return skb;
> >+
> >+err_free:
> >+ nlmsg_free(skb);
> >+ if (info)
> >+ GENL_SET_ERR_MSG(info, "failed to setup reply message");
> >+err:
>
> Why also not fillup extack msg here?
Right, err label should be right below the nlmsg_free(skb), thanks. And
now I noticed another mistake: on ethnl_fill_reply_header() failure, we
should go to err_free, not err.
> >+static inline int ethnl_str_size(const char *s)
>
> If you really need this helper, put it into netlink code. There's nothing
> ethtool-specific about this.
OK, I'll look into it. I've been already thinking about some kind of
NLA_SIZEOF() macro as about 1/3 of all uses of nla_total_size() follow
the nla_total_size(sizeof(...)) pattern (and lot more should follow it
but are written like e.g. nla_total_size(4) instead). This is another
common pattern.
> >+/* The ethnl_update_* helpers set value pointed to by @dst to the value of
> >+ * netlink attribute @attr (if attr is not null). They return true if *dst
> >+ * value was changed, false if not.
> >+ */
> >+static inline bool ethnl_update_u32(u32 *dst, struct nlattr *attr)
>
> I'm still not sure I'm convinced about these "update helpers" :)
Just imagine what would e.g.
if (ethnl_update_u32(&data.rx_pending, tb[ETHTOOL_A_RING_RX_PENDING]))
mod = true;
if (ethnl_update_u32(&data.rx_mini_pending,
tb[ETHTOOL_A_RING_RX_MINI_PENDING]))
mod = true;
if (ethnl_update_u32(&data.rx_jumbo_pending,
tb[ETHTOOL_A_RING_RX_JUMBO_PENDING]))
mod = true;
if (ethnl_update_u32(&data.tx_pending, tb[ETHTOOL_A_RING_TX_PENDING]))
mod = true;
if (!mod)
return 0;
look like without them. And coalescing parameters would be much worse
(22 attributes / struct members).
> >+{
> >+ u32 val;
> >+
> >+ if (!attr)
> >+ return false;
> >+ val = nla_get_u32(attr);
> >+ if (*dst == val)
> >+ return false;
> >+
> >+ *dst = val;
> >+ return true;
> >+}
...
> >+static inline bool ethnl_update_binary(u8 *dst, unsigned int len,
>
> void *dst
OK.
> >+/**
> >+ * ethnl_is_privileged() - check if request has sufficient privileges
> >+ * @skb: skb with client request
> >+ *
> >+ * Checks if client request has CAP_NET_ADMIN in its netns. Unlike the flags
> >+ * in genl_ops, this allows finer access control, e.g. allowing or denying
> >+ * the request based on its contents or witholding only part of the data
> >+ * from unprivileged users.
> >+ *
> >+ * Return: true if request is privileged, false if not
> >+ */
> >+static inline bool ethnl_is_privileged(struct sk_buff *skb)
>
> I wonder why you need this helper. Genetlink uses
> ops->flags & GENL_ADMIN_PERM for this.
It's explained in the function description. Sometimes we need finer
control than by request message type. An example is the WoL password:
ETHTOOL_GWOL is privileged because of it but I believe there si no
reason why unprivileged user couldn't see enabled WoL modes, we can
simply omit the password for him. (Also, it allows to combine query for
WoL settings with other unprivileged settings.)
> >+/**
> >+ * ethnl_reply_header_size() - total size of reply header
> >+ *
> >+ * This is an upper estimate so that we do not need to hold RTNL lock longer
> >+ * than necessary (to prevent rename between size estimate and composing the
>
> I guess this description is not relevant anymore. I don't see why to
> hold rtnl mutex for this function...
You don't need it for this function, it's the other way around: unless
you hold RTNL lock for the whole time covering both checking needed
message size and filling the message - and we don't - the device could
be renamed in between. Thus if we returned size based on current device
name, it might not be sufficient at the time the header is filled.
That's why this function returns maximum possible size (which is
actually a constant).
Michal
> >+ * message). Accounts only for device ifindex and name as those are the only
> >+ * attributes ethnl_fill_reply_header() puts into the reply header.
> >+ */
> >+static inline unsigned int ethnl_reply_header_size(void)
> >+{
> >+ return nla_total_size(nla_total_size(sizeof(u32)) +
> >+ nla_total_size(IFNAMSIZ));
> >+}
^ permalink raw reply
* Re: [PATCH net-next v6 01/15] rtnetlink: provide permanent hardware address in RTM_NEWLINK
From: Michal Kubecek @ 2019-07-02 16:35 UTC (permalink / raw)
To: netdev
Cc: Stephen Hemminger, David Miller, Jakub Kicinski, Jiri Pirko,
Andrew Lunn, Florian Fainelli, John Linville, Johannes Berg,
linux-kernel
In-Reply-To: <20190702075500.1b9845e1@hermes.lan>
On Tue, Jul 02, 2019 at 07:55:00AM -0700, Stephen Hemminger wrote:
> On Tue, 2 Jul 2019 13:49:44 +0200 (CEST)
> Michal Kubecek <mkubecek@suse.cz> wrote:
>
> > Permanent hardware address of a network device was traditionally provided
> > via ethtool ioctl interface but as Jiri Pirko pointed out in a review of
> > ethtool netlink interface, rtnetlink is much more suitable for it so let's
> > add it to the RTM_NEWLINK message.
> >
> > Add IFLA_PERM_ADDRESS attribute to RTM_NEWLINK messages unless the
> > permanent address is all zeros (i.e. device driver did not fill it). As
> > permanent address is not modifiable, reject userspace requests containing
> > IFLA_PERM_ADDRESS attribute.
> >
> > Note: we already provide permanent hardware address for bond slaves;
> > unfortunately we cannot drop that attribute for backward compatibility
> > reasons.
> >
> > v5 -> v6: only add the attribute if permanent address is not zero
> >
> > Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>
> Do you want to make an iproute patch to display this?
Yes, I'm going to submit it once this patch gets into net-next.
Michal
> Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [RFC PATCH v2 2/2] Documentation: net: dsa: b53: Describe b53 configuration
From: Benedikt Spranger @ 2019-07-02 16:28 UTC (permalink / raw)
To: Andrew Lunn
Cc: Florian Fainelli, netdev, Sebastian Andrzej Siewior,
Kurt Kanzenbach, Vivien Didelot
In-Reply-To: <20190701173550.GH30468@lunn.ch>
Am Mon, 1 Jul 2019 19:35:50 +0200
schrieb Andrew Lunn <andrew@lunn.ch>:
> > +Configuration without tagging support
> > +-------------------------------------
>
> How does this differ to the text you just added in the previous patch?
The b53 has some implementation specific detail:
The b53 DSA driver tags the CPU port in all VLANs, since otherwise any
PVID untagged VLAN programming would basically change the CPU port's
default PVID and make it untagged, undesirable.
This need some attention while configuring. Therefore the configuration
is slightly different to the generic one:
The following extra commands are needed on b53 single port and gateway
configuration:
bridge vlan del dev lan1 vid 1
bridge vlan del dev lan2 vid 1
On bridge config the following commands are not needed:
bridge vlan add dev lan1 vid 1 pvid untagged
bridge vlan add dev lan2 vid 1 pvid untagged
bridge vlan add dev lan3 vid 1 pvid untagged
> Do we need both?
I like full configuration examples. But it can also be done by
describing the changes. I would prefer both - description and full
script:
...
Configuration without tagging support
-------------------------------------
Older models (5325, 5365) support a different tag format that is not
supported yet. 539x and 531x5 require managed mode and some special
handling, which is also not yet supported. The tagging support is
disabled in these cases and the switch need a different configuration.
The configuration slightly differ from
the :ref:`dsa-vlan-configuration`.
+ The b53 tags the CPU port in all VLANs, since otherwise any PVID
+ untagged VLAN programming would basically change the CPU port's
+ default PVID and make it untagged, undesirable.
+ In difference to the configuration described
+ in :ref:`dsa-vlan-configuration` the default VLAN 1 has to be removed
+ from the slave interface configuration in single port and gateway
+ configuration, while there is no need to add an extra VLAN
+ configuration in the bridge showcase.
single port
~~~~~~~~~~~
...
Regards
Bene Spranger
^ permalink raw reply
* Re: [PATCH v2 bpf-next] libbpf: fix GCC8 warning for strncpy
From: Magnus Karlsson @ 2019-07-02 16:38 UTC (permalink / raw)
To: Y Song
Cc: Andrii Nakryiko, Andrii Nakryiko, bpf, netdev, Alexei Starovoitov,
Daniel Borkmann, Magnus Karlsson
In-Reply-To: <CAH3MdRWeH=Ko_mAvWk2mUaMK50iNHLbZkHKK=dVTzuwihZeRuA@mail.gmail.com>
On Tue, Jul 2, 2019 at 6:11 PM Y Song <ys114321@gmail.com> wrote:
>
> On Tue, Jul 2, 2019 at 8:17 AM Andrii Nakryiko <andriin@fb.com> wrote:
> >
> > GCC8 started emitting warning about using strncpy with number of bytes
> > exactly equal destination size, which is generally unsafe, as can lead
> > to non-zero terminated string being copied. Use IFNAMSIZ - 1 as number
> > of bytes to ensure name is always zero-terminated.
> >
> > Cc: Magnus Karlsson <magnus.karlsson@intel.com>
> > Signed-off-by: Andrii Nakryiko <andriin@fb.com>
>
> Acked-by: Yonghong Song <yhs@fb.com>
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
>
> > ---
> > tools/lib/bpf/xsk.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c
> > index bf15a80a37c2..b33740221b7e 100644
> > --- a/tools/lib/bpf/xsk.c
> > +++ b/tools/lib/bpf/xsk.c
> > @@ -327,7 +327,8 @@ static int xsk_get_max_queues(struct xsk_socket *xsk)
> >
> > channels.cmd = ETHTOOL_GCHANNELS;
> > ifr.ifr_data = (void *)&channels;
> > - strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ);
> > + strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ - 1);
> > + ifr.ifr_name[IFNAMSIZ - 1] = '\0';
> > err = ioctl(fd, SIOCETHTOOL, &ifr);
> > if (err && errno != EOPNOTSUPP) {
> > ret = -errno;
> > --
> > 2.17.1
> >
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Y Song @ 2019-07-02 16:42 UTC (permalink / raw)
To: Ilya Leoshkevich; +Cc: bpf, netdev
In-Reply-To: <20190702153908.41562-1-iii@linux.ibm.com>
On Tue, Jul 2, 2019 at 8:40 AM Ilya Leoshkevich <iii@linux.ibm.com> wrote:
>
> Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
>
> Pass -D__TARGET_ARCH_$(ARCH) to selftests in order to choose a proper
> PT_REGS_RC variant.
>
> Fix s930 -> s390 typo.
>
> On s390, provide the forward declaration of struct pt_regs and cast it
> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
> of the full struct pt_regs, s390 exposes only its first field
> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
> (in selftests) and kernel (in samples) headers.
>
> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390, x86
> provides struct pt_regs to both userspace and kernel, however, with
> different field names.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
> tools/testing/selftests/bpf/Makefile | 4 +-
> tools/testing/selftests/bpf/bpf_helpers.h | 46 +++++++++++++++--------
> tools/testing/selftests/bpf/progs/loop1.c | 2 +-
> tools/testing/selftests/bpf/progs/loop2.c | 2 +-
> tools/testing/selftests/bpf/progs/loop3.c | 2 +-
> 5 files changed, 37 insertions(+), 19 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
> index d60fee59fbd1..599b320bef65 100644
> --- a/tools/testing/selftests/bpf/Makefile
> +++ b/tools/testing/selftests/bpf/Makefile
> @@ -1,5 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
> include ../../../../scripts/Kbuild.include
> +include ../../../scripts/Makefile.arch
>
> LIBDIR := ../../../lib
> BPFDIR := $(LIBDIR)/bpf
> @@ -138,7 +139,8 @@ CLANG_SYS_INCLUDES := $(shell $(CLANG) -v -E - </dev/null 2>&1 \
>
> CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
> $(CLANG_SYS_INCLUDES) \
> - -Wno-compare-distinct-pointer-types
> + -Wno-compare-distinct-pointer-types \
> + -D__TARGET_ARCH_$(ARCH)
>
> $(OUTPUT)/test_l4lb_noinline.o: CLANG_FLAGS += -fno-inline
> $(OUTPUT)/test_xdp_noinline.o: CLANG_FLAGS += -fno-inline
> diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
> index 1a5b1accf091..faf86d83301a 100644
> --- a/tools/testing/selftests/bpf/bpf_helpers.h
> +++ b/tools/testing/selftests/bpf/bpf_helpers.h
> @@ -312,8 +312,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #if defined(__TARGET_ARCH_x86)
> #define bpf_target_x86
> #define bpf_target_defined
> -#elif defined(__TARGET_ARCH_s930x)
> - #define bpf_target_s930x
> +#elif defined(__TARGET_ARCH_s390)
> + #define bpf_target_s390
> #define bpf_target_defined
> #elif defined(__TARGET_ARCH_arm)
> #define bpf_target_arm
> @@ -338,8 +338,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #ifndef bpf_target_defined
> #if defined(__x86_64__)
> #define bpf_target_x86
> -#elif defined(__s390x__)
> - #define bpf_target_s930x
I see in some other places (e.g., bcc) where
macro __s390x__ is also used to indicate a s390 architecture.
Could you explain the difference between __s390__ and
__s390x__?
> +#elif defined(__s390__)
> + #define bpf_target_s390
> #elif defined(__arm__)
> #define bpf_target_arm
> #elif defined(__aarch64__)
> @@ -355,6 +355,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
>
> #if defined(bpf_target_x86)
>
> +#ifdef __KERNEL__
In samples/bpf/, __KERNEL__ is defined at clang options and
in selftests/bpf/, the __KERNEL__ is not defined.
I checked x86 pt_regs definition with and without __KERNEL__.
They are identical except some register name difference.
I am wondering whether we can unify into all without
__KERNEL__. Is __KERNEL__ really needed?
> #define PT_REGS_PARM1(x) ((x)->di)
> #define PT_REGS_PARM2(x) ((x)->si)
> #define PT_REGS_PARM3(x) ((x)->dx)
> @@ -365,19 +366,34 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
> #define PT_REGS_RC(x) ((x)->ax)
> #define PT_REGS_SP(x) ((x)->sp)
> #define PT_REGS_IP(x) ((x)->ip)
> +#else
> +#define PT_REGS_PARM1(x) ((x)->rdi)
> +#define PT_REGS_PARM2(x) ((x)->rsi)
> +#define PT_REGS_PARM3(x) ((x)->rdx)
> +#define PT_REGS_PARM4(x) ((x)->rcx)
> +#define PT_REGS_PARM5(x) ((x)->r8)
> +#define PT_REGS_RET(x) ((x)->rsp)
> +#define PT_REGS_FP(x) ((x)->rbp)
> +#define PT_REGS_RC(x) ((x)->rax)
> +#define PT_REGS_SP(x) ((x)->rsp)
> +#define PT_REGS_IP(x) ((x)->rip)
> +#endif
>
> -#elif defined(bpf_target_s390x)
> +#elif defined(bpf_target_s390)
>
> -#define PT_REGS_PARM1(x) ((x)->gprs[2])
> -#define PT_REGS_PARM2(x) ((x)->gprs[3])
> -#define PT_REGS_PARM3(x) ((x)->gprs[4])
> -#define PT_REGS_PARM4(x) ((x)->gprs[5])
> -#define PT_REGS_PARM5(x) ((x)->gprs[6])
> -#define PT_REGS_RET(x) ((x)->gprs[14])
> -#define PT_REGS_FP(x) ((x)->gprs[11]) /* Works only with CONFIG_FRAME_POINTER */
> -#define PT_REGS_RC(x) ((x)->gprs[2])
> -#define PT_REGS_SP(x) ((x)->gprs[15])
> -#define PT_REGS_IP(x) ((x)->psw.addr)
> +/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
> +struct pt_regs;
> +#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_PARM2(x) (((const volatile user_pt_regs *)(x))->gprs[3])
> +#define PT_REGS_PARM3(x) (((const volatile user_pt_regs *)(x))->gprs[4])
> +#define PT_REGS_PARM4(x) (((const volatile user_pt_regs *)(x))->gprs[5])
> +#define PT_REGS_PARM5(x) (((const volatile user_pt_regs *)(x))->gprs[6])
> +#define PT_REGS_RET(x) (((const volatile user_pt_regs *)(x))->gprs[14])
> +/* Works only with CONFIG_FRAME_POINTER */
> +#define PT_REGS_FP(x) (((const volatile user_pt_regs *)(x))->gprs[11])
> +#define PT_REGS_RC(x) (((const volatile user_pt_regs *)(x))->gprs[2])
> +#define PT_REGS_SP(x) (((const volatile user_pt_regs *)(x))->gprs[15])
> +#define PT_REGS_IP(x) (((const volatile user_pt_regs *)(x))->psw.addr)
Is user_pt_regs a recent change or has been there for quite some time?
I am asking since bcc did not use user_pt_regs yet.
>
> #elif defined(bpf_target_arm)
>
> diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
> index dea395af9ea9..7cdb7f878310 100644
> --- a/tools/testing/selftests/bpf/progs/loop1.c
> +++ b/tools/testing/selftests/bpf/progs/loop1.c
> @@ -18,7 +18,7 @@ int nested_loops(volatile struct pt_regs* ctx)
> for (j = 0; j < 300; j++)
> for (i = 0; i < j; i++) {
> if (j & 1)
> - m = ctx->rax;
> + m = PT_REGS_RC(ctx);
> else
> m = j;
> sum += i * m;
> diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
> index 0637bd8e8bcf..9b2f808a2863 100644
> --- a/tools/testing/selftests/bpf/progs/loop2.c
> +++ b/tools/testing/selftests/bpf/progs/loop2.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> int i = 0;
>
> while (true) {
> - if (ctx->rax & 1)
> + if (PT_REGS_RC(ctx) & 1)
> i += 3;
> else
> i += 7;
> diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
> index 30a0f6cba080..d727657d51e2 100644
> --- a/tools/testing/selftests/bpf/progs/loop3.c
> +++ b/tools/testing/selftests/bpf/progs/loop3.c
> @@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
> __u64 i = 0, sum = 0;
> do {
> i++;
> - sum += ctx->rax;
> + sum += PT_REGS_RC(ctx);
> } while (i < 0x100000000ULL);
> return sum;
> }
> --
> 2.21.0
>
^ permalink raw reply
* Re: [PATCH 2/3 bpf-next] i40e: Support zero-copy XDP_TX on the RX path for AF_XDP sockets.
From: Jonathan Lemon @ 2019-07-02 16:44 UTC (permalink / raw)
To: Magnus Karlsson
Cc: Network Development, Björn Töpel, Karlsson, Magnus,
Jakub Kicinski, jeffrey.t.kirsher, kernel-team
In-Reply-To: <CAJ8uoz0EL9gx87JmhjBmYscx-J2UCYK73OV73T3eohOEp0BEUw@mail.gmail.com>
On 1 Jul 2019, at 4:04, Magnus Karlsson wrote:
> On Sat, Jun 29, 2019 at 12:18 AM Jonathan Lemon
> <jonathan.lemon@gmail.com> wrote:
>>
>> When the XDP program attached to a zero-copy AF_XDP socket returns
>> XDP_TX,
>> queue the umem frame on the XDP TX ring. Space on the recycle stack
>> is
>> pre-allocated when the xsk is created. (taken from tx_ring, since
>> the
>> xdp ring is not initialized yet)
>>
>> Signed-off-by: Jonathan Lemon <jonathan.lemon@gmail.com>
>> ---
>> drivers/net/ethernet/intel/i40e/i40e_txrx.h | 1 +
>> drivers/net/ethernet/intel/i40e/i40e_xsk.c | 54
>> +++++++++++++++++++--
>> 2 files changed, 51 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> index 100e92d2982f..3e7954277737 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.h
>> @@ -274,6 +274,7 @@ static inline unsigned int
>> i40e_txd_use_count(unsigned int size)
>> #define I40E_TX_FLAGS_TSYN BIT(8)
>> #define I40E_TX_FLAGS_FD_SB BIT(9)
>> #define I40E_TX_FLAGS_UDP_TUNNEL BIT(10)
>> +#define I40E_TX_FLAGS_ZC_FRAME BIT(11)
>> #define I40E_TX_FLAGS_VLAN_MASK 0xffff0000
>> #define I40E_TX_FLAGS_VLAN_PRIO_MASK 0xe0000000
>> #define I40E_TX_FLAGS_VLAN_PRIO_SHIFT 29
>> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> index ce8650d06962..020f9859215d 100644
>> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
>> @@ -91,7 +91,8 @@ static int i40e_xsk_umem_enable(struct i40e_vsi
>> *vsi, struct xdp_umem *umem,
>> qid >= netdev->real_num_tx_queues)
>> return -EINVAL;
>>
>> - if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count))
>> + if (!xsk_umem_recycle_alloc(umem, vsi->rx_rings[0]->count +
>> + vsi->tx_rings[0]->count))
>> return -ENOMEM;
>>
>> err = i40e_xsk_umem_dma_map(vsi, umem);
>> @@ -175,6 +176,48 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi,
>> struct xdp_umem *umem,
>> i40e_xsk_umem_disable(vsi, qid);
>> }
>>
>> +static int i40e_xmit_rcvd_zc(struct i40e_ring *rx_ring, struct
>> xdp_buff *xdp)
>
> This function looks very much like i40e_xmit_xdp_ring(). How can we
> refactor them to make them share more code and not lose performance at
> the same time? This comment is also valid for the ixgbe driver patch
> that follows.
The next patch will split these into a small preamble setup and then
call a common send function.
--
Jonathan
>
> Thanks: Magnus
>
>> +{
>> + struct i40e_ring *xdp_ring;
>> + struct i40e_tx_desc *tx_desc;
>> + struct i40e_tx_buffer *tx_bi;
>> + struct xdp_frame *xdpf;
>> + dma_addr_t dma;
>> +
>> + xdp_ring = rx_ring->vsi->xdp_rings[rx_ring->queue_index];
>> +
>> + if (!unlikely(I40E_DESC_UNUSED(xdp_ring))) {
>> + xdp_ring->tx_stats.tx_busy++;
>> + return I40E_XDP_CONSUMED;
>> + }
>> + xdpf = convert_to_xdp_frame_keep_zc(xdp);
>> + if (unlikely(!xdpf))
>> + return I40E_XDP_CONSUMED;
>> + xdpf->handle = xdp->handle;
>> +
>> + dma = xdp_umem_get_dma(rx_ring->xsk_umem, xdp->handle);
>> + tx_bi = &xdp_ring->tx_bi[xdp_ring->next_to_use];
>> + tx_bi->bytecount = xdpf->len;
>> + tx_bi->gso_segs = 1;
>> + tx_bi->xdpf = xdpf;
>> + tx_bi->tx_flags = I40E_TX_FLAGS_ZC_FRAME;
>> +
>> + tx_desc = I40E_TX_DESC(xdp_ring, xdp_ring->next_to_use);
>> + tx_desc->buffer_addr = cpu_to_le64(dma);
>> + tx_desc->cmd_type_offset_bsz =
>> build_ctob(I40E_TX_DESC_CMD_ICRC |
>> +
>> I40E_TX_DESC_CMD_EOP,
>> + 0, xdpf->len, 0);
>> + smp_wmb();
>> +
>> + xdp_ring->next_to_use++;
>> + if (xdp_ring->next_to_use == xdp_ring->count)
>> + xdp_ring->next_to_use = 0;
>> +
>> + tx_bi->next_to_watch = tx_desc;
>> +
>> + return I40E_XDP_TX;
>> +}
>> +
>> /**
>> * i40e_run_xdp_zc - Executes an XDP program on an xdp_buff
>> * @rx_ring: Rx ring
>> @@ -187,7 +230,6 @@ int i40e_xsk_umem_setup(struct i40e_vsi *vsi,
>> struct xdp_umem *umem,
>> static int i40e_run_xdp_zc(struct i40e_ring *rx_ring, struct
>> xdp_buff *xdp)
>> {
>> int err, result = I40E_XDP_PASS;
>> - struct i40e_ring *xdp_ring;
>> struct bpf_prog *xdp_prog;
>> u32 act;
>>
>> @@ -202,8 +244,7 @@ static int i40e_run_xdp_zc(struct i40e_ring
>> *rx_ring, struct xdp_buff *xdp)
>> case XDP_PASS:
>> break;
>> case XDP_TX:
>> - xdp_ring =
>> rx_ring->vsi->xdp_rings[rx_ring->queue_index];
>> - result = i40e_xmit_xdp_tx_ring(xdp, xdp_ring);
>> + result = i40e_xmit_rcvd_zc(rx_ring, xdp);
>> break;
>> case XDP_REDIRECT:
>> err = xdp_do_redirect(rx_ring->netdev, xdp,
>> xdp_prog);
>> @@ -628,6 +669,11 @@ static bool i40e_xmit_zc(struct i40e_ring
>> *xdp_ring, unsigned int budget)
>> static void i40e_clean_xdp_tx_buffer(struct i40e_ring *tx_ring,
>> struct i40e_tx_buffer *tx_bi)
>> {
>> + if (tx_bi->tx_flags & I40E_TX_FLAGS_ZC_FRAME) {
>> + xsk_umem_recycle_addr(tx_ring->xsk_umem,
>> tx_bi->xdpf->handle);
>> + tx_bi->tx_flags = 0;
>> + return;
>> + }
>> xdp_return_frame(tx_bi->xdpf);
>> dma_unmap_single(tx_ring->dev,
>> dma_unmap_addr(tx_bi, dma),
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH 7/7] kbuild: compile-test kernel headers to ensure they are self-contained
From: Joel Fernandes @ 2019-07-02 16:47 UTC (permalink / raw)
To: Masahiro Yamada
Cc: linux-kbuild, Sam Ravnborg, Tony Luck, John Fastabend,
Jakub Kicinski, Daniel Borkmann, xdp-newbies, Anton Vorontsov,
Matthias Brugger, Song Liu, Yonghong Song, Michal Marek,
Jesper Dangaard Brouer, Martin KaFai Lau, linux-mediatek,
linux-arm-kernel, Colin Cross, David S. Miller, Kees Cook,
Alexei Starovoitov, netdev, linux-kernel, bpf
In-Reply-To: <20190701005845.12475-8-yamada.masahiro@socionext.com>
On Mon, Jul 01, 2019 at 09:58:45AM +0900, Masahiro Yamada wrote:
> The headers in include/ are globally used in the kernel source tree
> to provide common APIs. They are included from external modules, too.
>
> It will be useful to make as many headers self-contained as possible
> so that we do not have to rely on a specific include order.
>
> There are more than 4000 headers in include/. In my rough analysis,
> 70% of them are already self-contained. With efforts, most of them
> can be self-contained.
>
> For now, we must exclude more than 1000 headers just because they
> cannot be compiled as standalone units. I added them to header-test-.
> The blacklist was mostly generated by a script, so the reason of the
> breakage should be checked later.
>
> Signed-off-by: Masahiro Yamada <yamada.masahiro@socionext.com>
> Tested-by: Jani Nikula <jani.nikula@intel.com>
> ---
>
> Changes in v4:
> - Fix vmlinux build error
> - Exclude more headers for sparc
>
> Changes in v3:
> - Exclude more headers
> (Tested for allnoconfig + CONFIG_HEADER_TEST=y)
>
> Changes in v2:
> - Add everything to test coverage, and exclude broken ones
> - Rename 'Makefile' to 'Kbuild'
> - Add CONFIG_KERNEL_HEADER_TEST option
>
> Makefile | 1 +
> include/Kbuild | 1253 ++++++++++++++++++++++++++++++++++++++++++++++++
> init/Kconfig | 11 +
> 3 files changed, 1265 insertions(+)
> create mode 100644 include/Kbuild
[snip
> diff --git a/init/Kconfig b/init/Kconfig
> index 74192de8ada6..e2e99544da8d 100644
> --- a/init/Kconfig
> +++ b/init/Kconfig
> @@ -108,6 +108,17 @@ config HEADER_TEST
> If you are a developer or tester and want to ensure the requested
> headers are self-contained, say Y here. Otherwise, choose N.
>
> +config KERNEL_HEADER_TEST
> + bool "Compile test kernel headers"
> + depends on HEADER_TEST
> + help
> + Headers in include/ are used to build external moduls.
Nit:
modules.
Otherwise lgtm, thanks for the cc.
Reviewed-by: Joel Fernandes (Google) <joel@joelfernandes.org>
^ permalink raw reply
* Re: [PATCH net-next 1/1] devlink: Add APIs to publish/unpublish the port parameters.
From: Andrew Lunn @ 2019-07-02 16:48 UTC (permalink / raw)
To: Parav Pandit
Cc: Sudarsana Reddy Kalluru, davem@davemloft.net,
netdev@vger.kernel.org, mkalderon@marvell.com, aelior@marvell.com,
jiri@resnulli.us
In-Reply-To: <AM0PR05MB4866D7B26F48AF0BED9055EED1F80@AM0PR05MB4866.eurprd05.prod.outlook.com>
> A vendor driver calling these APIs is needed at minimum.
Not a vendor driver, but a mainline driver.
But yes, a new API should not be added without at least one user.
Andrew
^ 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