* [PATCH bpf-next 2/4] bpf: Split bpf_sk_lookup
From: Andrey Ignatov @ 2018-11-08 16:54 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, joe, kernel-team
In-Reply-To: <cover.1541695683.git.rdna@fb.com>
Split bpf_sk_lookup to separate core functionality, that can be reused
to make socket lookup available to more program types, from
functionality specific to program types that have access to skb.
Core functionality is placed to __bpf_sk_lookup. And bpf_sk_lookup only
gets caller netns and ifindex from skb and passes it to __bpf_sk_lookup.
Program types that don't have access to skb can just pass NULL to
__bpf_sk_lookup that will be handled correctly by both inet{,6}_sdif and
lookup functions.
This is refactoring that simply moves blocks around and does NOT change
existing logic.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 38 +++++++++++++++++++++++---------------
1 file changed, 23 insertions(+), 15 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index 9a1327eb25fa..dc0f86a707b7 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4825,14 +4825,10 @@ static const struct bpf_func_proto bpf_lwt_seg6_adjust_srh_proto = {
#ifdef CONFIG_INET
static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
- struct sk_buff *skb, u8 family, u8 proto)
+ struct sk_buff *skb, u8 family, u8 proto, int dif)
{
bool refcounted = false;
struct sock *sk = NULL;
- int dif = 0;
-
- if (skb->dev)
- dif = skb->dev->ifindex;
if (family == AF_INET) {
__be32 src4 = tuple->ipv4.saddr;
@@ -4875,16 +4871,16 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
return sk;
}
-/* bpf_sk_lookup performs the core lookup for different types of sockets,
+/* __bpf_sk_lookup performs the core lookup for different types of sockets,
* taking a reference on the socket if it doesn't have the flag SOCK_RCU_FREE.
* Returns the socket as an 'unsigned long' to simplify the casting in the
* callers to satisfy BPF_CALL declarations.
*/
static unsigned long
-bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
- u8 proto, u64 netns_id, u64 flags)
+__bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
+ u8 proto, u64 netns_id, struct net *caller_net, int ifindex,
+ u64 flags)
{
- struct net *caller_net;
struct sock *sk = NULL;
u8 family = AF_UNSPEC;
struct net *net;
@@ -4893,19 +4889,15 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
if (unlikely(family == AF_UNSPEC || netns_id > U32_MAX || flags))
goto out;
- if (skb->dev)
- caller_net = dev_net(skb->dev);
- else
- caller_net = sock_net(skb->sk);
if (netns_id) {
net = get_net_ns_by_id(caller_net, netns_id);
if (unlikely(!net))
goto out;
- sk = sk_lookup(net, tuple, skb, family, proto);
+ sk = sk_lookup(net, tuple, skb, family, proto, ifindex);
put_net(net);
} else {
net = caller_net;
- sk = sk_lookup(net, tuple, skb, family, proto);
+ sk = sk_lookup(net, tuple, skb, family, proto, ifindex);
}
if (sk)
@@ -4914,6 +4906,22 @@ bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
return (unsigned long) sk;
}
+static unsigned long
+bpf_sk_lookup(struct sk_buff *skb, struct bpf_sock_tuple *tuple, u32 len,
+ u8 proto, u64 netns_id, u64 flags)
+{
+ struct net *caller_net = sock_net(skb->sk);
+ int ifindex = 0;
+
+ if (skb->dev) {
+ caller_net = dev_net(skb->dev);
+ ifindex = skb->dev->ifindex;
+ }
+
+ return __bpf_sk_lookup(skb, tuple, len, proto, netns_id, caller_net,
+ ifindex, flags);
+}
+
BPF_CALL_5(bpf_sk_lookup_tcp, struct sk_buff *, skb,
struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
{
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 0/4] bpf: Support socket lookup in CGROUP_SOCK_ADDR progs
From: Andrey Ignatov @ 2018-11-08 16:54 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, joe, kernel-team
This patch set makes bpf_sk_lookup_tcp, bpf_sk_lookup_udp and
bpf_sk_release helpers available in programs of type
BPF_PROG_TYPE_CGROUP_SOCK_ADDR.
Patch 1 is a fix for bpf_sk_lookup_udp that was already sent to netdev
separately for bpf (stable) tree. Here it's prerequisite for patch 4.
Patch 2 is refactoring to prepare for patch 3. Similar refactoring was done
as part of "bpf: Extend the sk_lookup() helper to XDP hookpoint." patch
published on netdev earlier but not merged yet. This patch set can reuse
the work done for xdp if it's merged. This patch doesn't make any logic
changes and simply moves code around.
Patch 3 is the main patch in the set, it makes the helpers available for
BPF_PROG_TYPE_CGROUP_SOCK_ADDR and provides more details about use-case.
Patch 4 adds selftest for new functionality.
Andrey Ignatov (4):
bpf: Fix IPv6 dport byte order in bpf_sk_lookup_udp
bpf: Split bpf_sk_lookup
bpf: Support socket lookup in CGROUP_SOCK_ADDR progs
selftest/bpf: Use bpf_sk_lookup_{tcp,udp} in test_sock_addr
net/core/filter.c | 96 +++++++++++++++++----
tools/testing/selftests/bpf/connect4_prog.c | 43 +++++++--
tools/testing/selftests/bpf/connect6_prog.c | 56 +++++++++---
3 files changed, 156 insertions(+), 39 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH bpf-next 4/4] selftest/bpf: Use bpf_sk_lookup_{tcp,udp} in test_sock_addr
From: Andrey Ignatov @ 2018-11-08 16:54 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, joe, kernel-team
In-Reply-To: <cover.1541695683.git.rdna@fb.com>
Use bpf_sk_lookup_tcp, bpf_sk_lookup_udp and bpf_sk_release helpers from
test_sock_addr programs to make sure they're available and can lookup
and release socket properly for IPv4/IPv4, TCP/UDP.
Reading from a few fields of returned struct bpf_sock is also tested.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
tools/testing/selftests/bpf/connect4_prog.c | 43 ++++++++++++----
tools/testing/selftests/bpf/connect6_prog.c | 56 ++++++++++++++++-----
2 files changed, 78 insertions(+), 21 deletions(-)
diff --git a/tools/testing/selftests/bpf/connect4_prog.c b/tools/testing/selftests/bpf/connect4_prog.c
index 5a88a681d2ab..b8395f3c43e9 100644
--- a/tools/testing/selftests/bpf/connect4_prog.c
+++ b/tools/testing/selftests/bpf/connect4_prog.c
@@ -21,23 +21,48 @@ int _version SEC("version") = 1;
SEC("cgroup/connect4")
int connect_v4_prog(struct bpf_sock_addr *ctx)
{
+ struct bpf_sock_tuple tuple = {};
struct sockaddr_in sa;
+ struct bpf_sock *sk;
+
+ /* Verify that new destination is available. */
+ memset(&tuple.ipv4.saddr, 0, sizeof(tuple.ipv4.saddr));
+ memset(&tuple.ipv4.sport, 0, sizeof(tuple.ipv4.sport));
+
+ tuple.ipv4.daddr = bpf_htonl(DST_REWRITE_IP4);
+ tuple.ipv4.dport = bpf_htons(DST_REWRITE_PORT4);
+
+ if (ctx->type != SOCK_STREAM && ctx->type != SOCK_DGRAM)
+ return 0;
+ else if (ctx->type == SOCK_STREAM)
+ sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof(tuple.ipv4), 0, 0);
+ else
+ sk = bpf_sk_lookup_udp(ctx, &tuple, sizeof(tuple.ipv4), 0, 0);
+
+ if (!sk)
+ return 0;
+
+ if (sk->src_ip4 != tuple.ipv4.daddr ||
+ sk->src_port != DST_REWRITE_PORT4) {
+ bpf_sk_release(sk);
+ return 0;
+ }
+
+ bpf_sk_release(sk);
/* Rewrite destination. */
ctx->user_ip4 = bpf_htonl(DST_REWRITE_IP4);
ctx->user_port = bpf_htons(DST_REWRITE_PORT4);
- if (ctx->type == SOCK_DGRAM || ctx->type == SOCK_STREAM) {
- ///* Rewrite source. */
- memset(&sa, 0, sizeof(sa));
+ /* Rewrite source. */
+ memset(&sa, 0, sizeof(sa));
- sa.sin_family = AF_INET;
- sa.sin_port = bpf_htons(0);
- sa.sin_addr.s_addr = bpf_htonl(SRC_REWRITE_IP4);
+ sa.sin_family = AF_INET;
+ sa.sin_port = bpf_htons(0);
+ sa.sin_addr.s_addr = bpf_htonl(SRC_REWRITE_IP4);
- if (bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)) != 0)
- return 0;
- }
+ if (bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)) != 0)
+ return 0;
return 1;
}
diff --git a/tools/testing/selftests/bpf/connect6_prog.c b/tools/testing/selftests/bpf/connect6_prog.c
index 8ea3f7d12dee..25f5dc7b7aa0 100644
--- a/tools/testing/selftests/bpf/connect6_prog.c
+++ b/tools/testing/selftests/bpf/connect6_prog.c
@@ -29,7 +29,41 @@ int _version SEC("version") = 1;
SEC("cgroup/connect6")
int connect_v6_prog(struct bpf_sock_addr *ctx)
{
+ struct bpf_sock_tuple tuple = {};
struct sockaddr_in6 sa;
+ struct bpf_sock *sk;
+
+ /* Verify that new destination is available. */
+ memset(&tuple.ipv6.saddr, 0, sizeof(tuple.ipv6.saddr));
+ memset(&tuple.ipv6.sport, 0, sizeof(tuple.ipv6.sport));
+
+ tuple.ipv6.daddr[0] = bpf_htonl(DST_REWRITE_IP6_0);
+ tuple.ipv6.daddr[1] = bpf_htonl(DST_REWRITE_IP6_1);
+ tuple.ipv6.daddr[2] = bpf_htonl(DST_REWRITE_IP6_2);
+ tuple.ipv6.daddr[3] = bpf_htonl(DST_REWRITE_IP6_3);
+
+ tuple.ipv6.dport = bpf_htons(DST_REWRITE_PORT6);
+
+ if (ctx->type != SOCK_STREAM && ctx->type != SOCK_DGRAM)
+ return 0;
+ else if (ctx->type == SOCK_STREAM)
+ sk = bpf_sk_lookup_tcp(ctx, &tuple, sizeof(tuple.ipv6), 0, 0);
+ else
+ sk = bpf_sk_lookup_udp(ctx, &tuple, sizeof(tuple.ipv6), 0, 0);
+
+ if (!sk)
+ return 0;
+
+ if (sk->src_ip6[0] != tuple.ipv6.daddr[0] ||
+ sk->src_ip6[1] != tuple.ipv6.daddr[1] ||
+ sk->src_ip6[2] != tuple.ipv6.daddr[2] ||
+ sk->src_ip6[3] != tuple.ipv6.daddr[3] ||
+ sk->src_port != DST_REWRITE_PORT6) {
+ bpf_sk_release(sk);
+ return 0;
+ }
+
+ bpf_sk_release(sk);
/* Rewrite destination. */
ctx->user_ip6[0] = bpf_htonl(DST_REWRITE_IP6_0);
@@ -39,21 +73,19 @@ int connect_v6_prog(struct bpf_sock_addr *ctx)
ctx->user_port = bpf_htons(DST_REWRITE_PORT6);
- if (ctx->type == SOCK_DGRAM || ctx->type == SOCK_STREAM) {
- /* Rewrite source. */
- memset(&sa, 0, sizeof(sa));
+ /* Rewrite source. */
+ memset(&sa, 0, sizeof(sa));
- sa.sin6_family = AF_INET6;
- sa.sin6_port = bpf_htons(0);
+ sa.sin6_family = AF_INET6;
+ sa.sin6_port = bpf_htons(0);
- sa.sin6_addr.s6_addr32[0] = bpf_htonl(SRC_REWRITE_IP6_0);
- sa.sin6_addr.s6_addr32[1] = bpf_htonl(SRC_REWRITE_IP6_1);
- sa.sin6_addr.s6_addr32[2] = bpf_htonl(SRC_REWRITE_IP6_2);
- sa.sin6_addr.s6_addr32[3] = bpf_htonl(SRC_REWRITE_IP6_3);
+ sa.sin6_addr.s6_addr32[0] = bpf_htonl(SRC_REWRITE_IP6_0);
+ sa.sin6_addr.s6_addr32[1] = bpf_htonl(SRC_REWRITE_IP6_1);
+ sa.sin6_addr.s6_addr32[2] = bpf_htonl(SRC_REWRITE_IP6_2);
+ sa.sin6_addr.s6_addr32[3] = bpf_htonl(SRC_REWRITE_IP6_3);
- if (bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)) != 0)
- return 0;
- }
+ if (bpf_bind(ctx, (struct sockaddr *)&sa, sizeof(sa)) != 0)
+ return 0;
return 1;
}
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 3/4] bpf: Support socket lookup in CGROUP_SOCK_ADDR progs
From: Andrey Ignatov @ 2018-11-08 16:54 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, joe, kernel-team
In-Reply-To: <cover.1541695683.git.rdna@fb.com>
Make bpf_sk_lookup_tcp, bpf_sk_lookup_udp and bpf_sk_release helpers
available in programs of type BPF_PROG_TYPE_CGROUP_SOCK_ADDR.
Such programs operate on sockets and have access to socket and struct
sockaddr passed by user to system calls such as sys_bind, sys_connect,
sys_sendmsg.
It's useful to be able to lookup other sockets from these programs.
E.g. sys_connect may lookup IP:port endpoint and if there is a server
socket bound to that endpoint ("server" can be defined by saddr & sport
being zero), redirect client connection to it by rewriting IP:port in
sockaddr passed to sys_connect.
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
net/core/filter.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/net/core/filter.c b/net/core/filter.c
index dc0f86a707b7..2e8575a34a1e 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4971,6 +4971,51 @@ static const struct bpf_func_proto bpf_sk_release_proto = {
.ret_type = RET_INTEGER,
.arg1_type = ARG_PTR_TO_SOCKET,
};
+
+static unsigned long
+bpf_sock_addr_sk_lookup(struct sock *sk, struct bpf_sock_tuple *tuple, u32 len,
+ u8 proto, u64 netns_id, u64 flags)
+{
+ return __bpf_sk_lookup(NULL, tuple, len, proto, netns_id, sock_net(sk),
+ 0, flags);
+}
+
+BPF_CALL_5(bpf_sock_addr_sk_lookup_tcp, struct bpf_sock_addr_kern *, ctx,
+ struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
+{
+ return bpf_sock_addr_sk_lookup(ctx->sk, tuple, len, IPPROTO_TCP,
+ netns_id, flags);
+}
+
+static const struct bpf_func_proto bpf_sock_addr_sk_lookup_tcp_proto = {
+ .func = bpf_sock_addr_sk_lookup_tcp,
+ .gpl_only = false,
+ .ret_type = RET_PTR_TO_SOCKET_OR_NULL,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .arg4_type = ARG_ANYTHING,
+ .arg5_type = ARG_ANYTHING,
+};
+
+BPF_CALL_5(bpf_sock_addr_sk_lookup_udp, struct bpf_sock_addr_kern *, ctx,
+ struct bpf_sock_tuple *, tuple, u32, len, u64, netns_id, u64, flags)
+{
+ return bpf_sock_addr_sk_lookup(ctx->sk, tuple, len, IPPROTO_UDP,
+ netns_id, flags);
+}
+
+static const struct bpf_func_proto bpf_sock_addr_sk_lookup_udp_proto = {
+ .func = bpf_sock_addr_sk_lookup_udp,
+ .gpl_only = false,
+ .ret_type = RET_PTR_TO_SOCKET_OR_NULL,
+ .arg1_type = ARG_PTR_TO_CTX,
+ .arg2_type = ARG_PTR_TO_MEM,
+ .arg3_type = ARG_CONST_SIZE,
+ .arg4_type = ARG_ANYTHING,
+ .arg5_type = ARG_ANYTHING,
+};
+
#endif /* CONFIG_INET */
bool bpf_helper_changes_pkt_data(void *func)
@@ -5077,6 +5122,14 @@ sock_addr_func_proto(enum bpf_func_id func_id, const struct bpf_prog *prog)
return &bpf_get_socket_cookie_sock_addr_proto;
case BPF_FUNC_get_local_storage:
return &bpf_get_local_storage_proto;
+#ifdef CONFIG_INET
+ case BPF_FUNC_sk_lookup_tcp:
+ return &bpf_sock_addr_sk_lookup_tcp_proto;
+ case BPF_FUNC_sk_lookup_udp:
+ return &bpf_sock_addr_sk_lookup_udp_proto;
+ case BPF_FUNC_sk_release:
+ return &bpf_sk_release_proto;
+#endif /* CONFIG_INET */
default:
return bpf_base_func_proto(func_id);
}
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 1/4] bpf: Fix IPv6 dport byte order in bpf_sk_lookup_udp
From: Andrey Ignatov @ 2018-11-08 16:54 UTC (permalink / raw)
To: netdev; +Cc: Andrey Ignatov, ast, daniel, joe, kernel-team
In-Reply-To: <cover.1541695683.git.rdna@fb.com>
Lookup functions in sk_lookup have different expectations about byte
order of provided arguments.
Specifically __inet_lookup, __udp4_lib_lookup and __udp6_lib_lookup
expect dport to be in network byte order and do ntohs(dport) internally.
At the same time __inet6_lookup expects dport to be in host byte order
and correspondingly name the argument hnum.
sk_lookup works correctly with __inet_lookup, __udp4_lib_lookup and
__inet6_lookup with regard to dport. But in __udp6_lib_lookup case it
uses host instead of expected network byte order. It makes result
returned by bpf_sk_lookup_udp for IPv6 incorrect.
The patch fixes byte order of dport passed to __udp6_lib_lookup.
Originally sk_lookup properly handled UDPv6, but not TCPv6. 5ef0ae84f02a
fixes TCPv6 but breaks UDPv6.
Fixes: 5ef0ae84f02a ("bpf: Fix IPv6 dport byte-order in bpf_sk_lookup")
Signed-off-by: Andrey Ignatov <rdna@fb.com>
Acked-by: Joe Stringer <joe@wand.net.nz>
Acked-by: Martin KaFai Lau <kafai@fb.com>
---
net/core/filter.c | 5 ++---
1 file changed, 2 insertions(+), 3 deletions(-)
diff --git a/net/core/filter.c b/net/core/filter.c
index e521c5ebc7d1..9a1327eb25fa 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -4852,18 +4852,17 @@ static struct sock *sk_lookup(struct net *net, struct bpf_sock_tuple *tuple,
} else {
struct in6_addr *src6 = (struct in6_addr *)&tuple->ipv6.saddr;
struct in6_addr *dst6 = (struct in6_addr *)&tuple->ipv6.daddr;
- u16 hnum = ntohs(tuple->ipv6.dport);
int sdif = inet6_sdif(skb);
if (proto == IPPROTO_TCP)
sk = __inet6_lookup(net, &tcp_hashinfo, skb, 0,
src6, tuple->ipv6.sport,
- dst6, hnum,
+ dst6, ntohs(tuple->ipv6.dport),
dif, sdif, &refcounted);
else if (likely(ipv6_bpf_stub))
sk = ipv6_bpf_stub->udp6_lib_lookup(net,
src6, tuple->ipv6.sport,
- dst6, hnum,
+ dst6, tuple->ipv6.dport,
dif, sdif,
&udp_table, skb);
#endif
--
2.17.1
^ permalink raw reply related
* Re: [PATCH net-next] SUNRPC: drop pointless static qualifier in xdr_get_next_encode_buffer()
From: bfields @ 2018-11-08 17:26 UTC (permalink / raw)
To: Trond Myklebust
Cc: yuehaibing@huawei.com, anna.schumaker@netapp.com,
davem@davemloft.net, jlayton@kernel.org, netdev@vger.kernel.org,
linux-nfs@vger.kernel.org, kernel-janitors@vger.kernel.org
In-Reply-To: <d23105929adb66c626d382525adef77265585404.camel@hammerspace.com>
On Thu, Nov 08, 2018 at 03:13:25AM +0000, Trond Myklebust wrote:
> On Thu, 2018-11-08 at 02:04 +0000, YueHaibing wrote:
> > There is no need to have the '__be32 *p' variable static since new
> > value
> > always be assigned before use it.
Applying for 4.20 and stable, thanks!
> >
> > Signed-off-by: YueHaibing <yuehaibing@huawei.com>
> > ---
> > net/sunrpc/xdr.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/net/sunrpc/xdr.c b/net/sunrpc/xdr.c
> > index 2bbb8d3..d80b156 100644
> > --- a/net/sunrpc/xdr.c
> > +++ b/net/sunrpc/xdr.c
> > @@ -546,7 +546,7 @@ void xdr_commit_encode(struct xdr_stream *xdr)
> > static __be32 *xdr_get_next_encode_buffer(struct xdr_stream *xdr,
> > size_t nbytes)
> > {
> > - static __be32 *p;
> > + __be32 *p;
> > int space_left;
> > int frag1bytes, frag2bytes;
> >
>
> Ouch, that's a really nasty bug that could definitely cause corruption
> if you have 2 threads simultaneously calling this function! This really
> deserves to be a stable patch.
Agreed. Looks like I introduced that in 3.16, over 5 years ago, so I'm
a little surprised not to have seen a bug report that this would
explain. Maybe it's just that the critical section is only a few lines
of arithemtic at the end of the function. Also it only gets called when
an xdr reply other than a read reaches the end of a page. So you'd need
a lot of concurrent READDIRs of large directories or something. Still,
I'd think it would be possible.....
> Thank you, YueHaibing!
>
> Bruce, do you want to shepherd this one in?
Yes, I've got 3 bugfixes queued up now, I should send them along later
today or tomorrow.
--b.
^ permalink raw reply
* Re: Kernel 4.19 network performance - forwarding/routing normal users traffic
From: Paweł Staszewski @ 2018-11-08 17:30 UTC (permalink / raw)
To: David Ahern, Jesper Dangaard Brouer; +Cc: netdev, Yoel Caspersen
In-Reply-To: <68cc8279-5e3f-85c2-673c-aa3d4a47b353@gmail.com>
W dniu 08.11.2018 o 17:32, David Ahern pisze:
> On 11/8/18 9:27 AM, Paweł Staszewski wrote:
>>>> What hardware is this?
>>>>
>> mellanox connectx 4
>> ethtool -i enp175s0f0
>> driver: mlx5_core
>> version: 5.0-0
>> firmware-version: 12.21.1000 (SM_2001000001033)
>> expansion-rom-version:
>> bus-info: 0000:af:00.0
>> supports-statistics: yes
>> supports-test: yes
>> supports-eeprom-access: no
>> supports-register-dump: no
>> supports-priv-flags: yes
>>
>> ethtool -i enp175s0f1
>> driver: mlx5_core
>> version: 5.0-0
>> firmware-version: 12.21.1000 (SM_2001000001033)
>> expansion-rom-version:
>> bus-info: 0000:af:00.1
>> supports-statistics: yes
>> supports-test: yes
>> supports-eeprom-access: no
>> supports-register-dump: no
>> supports-priv-flags: yes
>>
>>>> Start with:
>>>>
>>>> echo 1 > /sys/kernel/debug/tracing/events/xdp/enable
>>>> cat /sys/kernel/debug/tracing/trace_pipe
>>> cat /sys/kernel/debug/tracing/trace_pipe
>>> <idle>-0 [045] ..s. 68469.467752: xdp_devmap_xmit:
>>> ndo_xdp_xmit map_id=32 map_index=5 action=REDIRECT sent=0 drops=1
>>> from_ifindex=4 to_ifindex=5 err=-6
> FIB lookup is good, the redirect is happening, but the mlx5 driver does
> not like it.
>
> I think the -6 is coming from the mlx5 driver and the packet is getting
> dropped. Perhaps this check in mlx5e_xdp_xmit:
>
> if (unlikely(sq_num >= priv->channels.num))
> return -ENXIO;
>
Wondering about this:
swapper 0 [045] 68494.770287: fib:fib_table_lookup: table 254 oif 0
iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0 ==>
dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
oif 0 ?
Is that correct here ?
>
>>> swapper 0 [045] 68493.746274: fib:fib_table_lookup: table 254 oif
>>> 0 iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0
>>> ==> dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
>>> 7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
>>>
>>> swapper 0 [045] 68494.770287: fib:fib_table_lookup: table 254 oif
>>> 0 iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0
>>> ==> dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
>>> 7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
>>>
>>> swapper 0 [045] 68495.794304: fib:fib_table_lookup: table 254 oif
>>> 0 iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0
>>> ==> dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
>>> 7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
>>>
>>> swapper 0 [045] 68496.818308: fib:fib_table_lookup: table 254 oif
>>> 0 iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0
>>> ==> dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
>>> 7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
>>>
>>> swapper 0 [045] 68497.842313: fib:fib_table_lookup: table 254 oif
>>> 0 iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0
>>> ==> dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
>>> 7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
^ permalink raw reply
* Re: [PATCH 0/5] hy: core: rework phy_set_mode to accept phy mode and submode
From: David Miller @ 2018-11-09 3:20 UTC (permalink / raw)
To: grygorii.strashko
Cc: kishon, netdev, nsekhar, linux-kernel, linux-arm-kernel, tony,
linux-amlogic, linux-mediatek, alexandre.belloni, antoine.tenart,
quentin.schulz, vivek.gautam, maxime.ripard, wens, carlo,
chunfeng.yun, matthias.bgg, mgautam
In-Reply-To: <20181108003617.10334-1-grygorii.strashko@ti.com>
From: Grygorii Strashko <grygorii.strashko@ti.com>
Date: Wed, 7 Nov 2018 18:36:12 -0600
> As was discussed in [1] I'm posting series which introduces rework of
> phy_set_mode to accept phy mode and submode. I've dropped TI specific patches as
> this change is pretty big by itself.
>
> Patch 1 is cumulative change which refactors PHY framework code to
> support dual level PHYs mode configuration - PHY mode and PHY submode. It
> extends .set_mode() callback to support additional parameter "int submode"
> and converts all corresponding PHY drivers to support new .set_mode()
> callback declaration.
> The new extended PHY API
> int phy_set_mode_ext(struct phy *phy, enum phy_mode mode, int submode)
> is introduced to support dual level PHYs mode configuration and existing
> phy_set_mode() API is converted to macros, so PHY framework consumers do
> not need to be changed (~21 matches).
>
> Patches 2-4: Add new PHY's mode to be used by Ethernet PHY interface drivers or
> multipurpose PHYs like serdes and convert ocelot-serdes and mvebu-cp110-comphy
> PHY drivers to use recently introduced PHY_MODE_ETHERNET and phy_set_mode_ext().
>
> Patch 5 - removes unused, ethernet specific phy modes from enum phy_mode.
>
> [1] https://lkml.org/lkml/2018/10/25/366
I guess this will go via Kishon's tree.
^ permalink raw reply
* [PATCH net-next] i40iw: remove use of VLAN_TAG_PRESENT
From: Michał Mirosław @ 2018-11-08 17:44 UTC (permalink / raw)
To: netdev
Cc: Faisal Latif, Shiraz Saleem, Claudiu Manoil, Pravin B Shelar, dev,
linux-rdma
In-Reply-To: <cover.1541698641.git.mirq-linux@rere.qmqm.pl>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/infiniband/hw/i40iw/i40iw_cm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/hw/i40iw/i40iw_cm.c b/drivers/infiniband/hw/i40iw/i40iw_cm.c
index 771eb6bd0785..4b3999d88c9e 100644
--- a/drivers/infiniband/hw/i40iw/i40iw_cm.c
+++ b/drivers/infiniband/hw/i40iw/i40iw_cm.c
@@ -404,7 +404,7 @@ static struct i40iw_puda_buf *i40iw_form_cm_frame(struct i40iw_cm_node *cm_node,
if (pdata)
pd_len = pdata->size;
- if (cm_node->vlan_id < VLAN_TAG_PRESENT)
+ if (cm_node->vlan_id <= VLAN_VID_MASK)
eth_hlen += 4;
if (cm_node->ipv4)
@@ -433,7 +433,7 @@ static struct i40iw_puda_buf *i40iw_form_cm_frame(struct i40iw_cm_node *cm_node,
ether_addr_copy(ethh->h_dest, cm_node->rem_mac);
ether_addr_copy(ethh->h_source, cm_node->loc_mac);
- if (cm_node->vlan_id < VLAN_TAG_PRESENT) {
+ if (cm_node->vlan_id <= VLAN_VID_MASK) {
((struct vlan_ethhdr *)ethh)->h_vlan_proto = htons(ETH_P_8021Q);
vtag = (cm_node->user_pri << VLAN_PRIO_SHIFT) | cm_node->vlan_id;
((struct vlan_ethhdr *)ethh)->h_vlan_TCI = htons(vtag);
@@ -463,7 +463,7 @@ static struct i40iw_puda_buf *i40iw_form_cm_frame(struct i40iw_cm_node *cm_node,
ether_addr_copy(ethh->h_dest, cm_node->rem_mac);
ether_addr_copy(ethh->h_source, cm_node->loc_mac);
- if (cm_node->vlan_id < VLAN_TAG_PRESENT) {
+ if (cm_node->vlan_id <= VLAN_VID_MASK) {
((struct vlan_ethhdr *)ethh)->h_vlan_proto = htons(ETH_P_8021Q);
vtag = (cm_node->user_pri << VLAN_PRIO_SHIFT) | cm_node->vlan_id;
((struct vlan_ethhdr *)ethh)->h_vlan_TCI = htons(vtag);
@@ -3323,7 +3323,7 @@ static void i40iw_init_tcp_ctx(struct i40iw_cm_node *cm_node,
tcp_info->flow_label = 0;
tcp_info->snd_mss = cpu_to_le32(((u32)cm_node->tcp_cntxt.mss));
- if (cm_node->vlan_id < VLAN_TAG_PRESENT) {
+ if (cm_node->vlan_id <= VLAN_VID_MASK) {
tcp_info->insert_vlan_tag = true;
tcp_info->vlan_tag = cpu_to_le16(((u16)cm_node->user_pri << I40IW_VLAN_PRIO_SHIFT) |
cm_node->vlan_id);
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 2/4] cnic: remove use of VLAN_TAG_PRESENT
From: Michał Mirosław @ 2018-11-08 17:44 UTC (permalink / raw)
To: netdev
Cc: Claudiu Manoil, Faisal Latif, Pravin B Shelar, Shiraz Saleem, dev,
linux-rdma
In-Reply-To: <cover.1541698641.git.mirq-linux@rere.qmqm.pl>
This just removes VLAN_TAG_PRESENT use. VLAN TCI=0 special meaning is
deeply embedded in the driver code and so is left as is.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/net/ethernet/broadcom/cnic.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/cnic.c b/drivers/net/ethernet/broadcom/cnic.c
index d83233ae4a15..510dfc1c236b 100644
--- a/drivers/net/ethernet/broadcom/cnic.c
+++ b/drivers/net/ethernet/broadcom/cnic.c
@@ -5731,7 +5731,7 @@ static int cnic_netdev_event(struct notifier_block *this, unsigned long event,
if (realdev) {
dev = cnic_from_netdev(realdev);
if (dev) {
- vid |= VLAN_TAG_PRESENT;
+ vid |= VLAN_CFI_MASK; /* make non-zero */
cnic_rcv_netevent(dev->cnic_priv, event, vid);
cnic_put(dev);
}
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 0/4] Remove VLAN_TAG_PRESENT from drivers
From: Michał Mirosław @ 2018-11-08 17:44 UTC (permalink / raw)
To: netdev
Cc: Claudiu Manoil, Faisal Latif, Pravin B Shelar, Shiraz Saleem, dev,
linux-rdma
This series removes VLAN_TAG_PRESENT use from network drivers in
preparation to removing its special meaning.
Michał Mirosław (4):
i40iw: remove use of VLAN_TAG_PRESENT
cnic: remove use of VLAN_TAG_PRESENT
gianfar: remove use of VLAN_TAG_PRESENT
OVS: remove use of VLAN_TAG_PRESENT
drivers/infiniband/hw/i40iw/i40iw_cm.c | 8 +++----
drivers/net/ethernet/broadcom/cnic.c | 2 +-
.../net/ethernet/freescale/gianfar_ethtool.c | 8 +++----
net/openvswitch/actions.c | 13 +++++++----
net/openvswitch/flow.c | 4 ++--
net/openvswitch/flow.h | 2 +-
net/openvswitch/flow_netlink.c | 22 +++++++++----------
7 files changed, 31 insertions(+), 28 deletions(-)
--
2.19.1
^ permalink raw reply
* [PATCH net-next 3/4] gianfar: remove use of VLAN_TAG_PRESENT
From: Michał Mirosław @ 2018-11-08 17:44 UTC (permalink / raw)
To: netdev
Cc: Claudiu Manoil, Faisal Latif, Pravin B Shelar, Shiraz Saleem, dev,
linux-rdma
In-Reply-To: <cover.1541698641.git.mirq-linux@rere.qmqm.pl>
Reviewed-by: Claudiu Manoil <claudiu.manoil@nxp.com>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
drivers/net/ethernet/freescale/gianfar_ethtool.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/drivers/net/ethernet/freescale/gianfar_ethtool.c b/drivers/net/ethernet/freescale/gianfar_ethtool.c
index 0d76e15cd6dd..241325c35cb4 100644
--- a/drivers/net/ethernet/freescale/gianfar_ethtool.c
+++ b/drivers/net/ethernet/freescale/gianfar_ethtool.c
@@ -1134,11 +1134,9 @@ static int gfar_convert_to_filer(struct ethtool_rx_flow_spec *rule,
prio = vlan_tci_prio(rule);
prio_mask = vlan_tci_priom(rule);
- if (cfi == VLAN_TAG_PRESENT && cfi_mask == VLAN_TAG_PRESENT) {
- vlan |= RQFPR_CFI;
- vlan_mask |= RQFPR_CFI;
- } else if (cfi != VLAN_TAG_PRESENT &&
- cfi_mask == VLAN_TAG_PRESENT) {
+ if (cfi_mask) {
+ if (cfi)
+ vlan |= RQFPR_CFI;
vlan_mask |= RQFPR_CFI;
}
}
--
2.19.1
^ permalink raw reply related
* [PATCH net-next 4/4] OVS: remove use of VLAN_TAG_PRESENT
From: Michał Mirosław @ 2018-11-08 17:44 UTC (permalink / raw)
To: netdev
Cc: Pravin B Shelar, Claudiu Manoil, Faisal Latif, Shiraz Saleem, dev,
linux-rdma
In-Reply-To: <cover.1541698641.git.mirq-linux@rere.qmqm.pl>
This is a minimal change to allow removing of VLAN_TAG_PRESENT.
It leaves OVS unable to use CFI bit, as fixing this would need
a deeper surgery involving userspace interface.
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
net/openvswitch/actions.c | 13 +++++++++----
net/openvswitch/flow.c | 4 ++--
net/openvswitch/flow.h | 2 +-
net/openvswitch/flow_netlink.c | 22 +++++++++++-----------
4 files changed, 23 insertions(+), 18 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 85ae53d8fd09..e47ebbbe71b8 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -301,7 +301,7 @@ static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
key->eth.vlan.tpid = vlan->vlan_tpid;
}
return skb_vlan_push(skb, vlan->vlan_tpid,
- ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
+ ntohs(vlan->vlan_tci) & ~VLAN_CFI_MASK);
}
/* 'src' is already properly masked. */
@@ -822,8 +822,10 @@ static int ovs_vport_output(struct net *net, struct sock *sk, struct sk_buff *sk
__skb_dst_copy(skb, data->dst);
*OVS_CB(skb) = data->cb;
skb->inner_protocol = data->inner_protocol;
- skb->vlan_tci = data->vlan_tci;
- skb->vlan_proto = data->vlan_proto;
+ if (data->vlan_tci & VLAN_CFI_MASK)
+ __vlan_hwaccel_put_tag(skb, data->vlan_proto, data->vlan_tci & ~VLAN_CFI_MASK);
+ else
+ __vlan_hwaccel_clear_tag(skb);
/* Reconstruct the MAC header. */
skb_push(skb, data->l2_len);
@@ -867,7 +869,10 @@ static void prepare_frag(struct vport *vport, struct sk_buff *skb,
data->cb = *OVS_CB(skb);
data->inner_protocol = skb->inner_protocol;
data->network_offset = orig_network_offset;
- data->vlan_tci = skb->vlan_tci;
+ if (skb_vlan_tag_present(skb))
+ data->vlan_tci = skb_vlan_tag_get(skb) | VLAN_CFI_MASK;
+ else
+ data->vlan_tci = 0;
data->vlan_proto = skb->vlan_proto;
data->mac_proto = mac_proto;
data->l2_len = hlen;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 35966da84769..fa393815991e 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -325,7 +325,7 @@ static int parse_vlan_tag(struct sk_buff *skb, struct vlan_head *key_vh,
return -ENOMEM;
vh = (struct vlan_head *)skb->data;
- key_vh->tci = vh->tci | htons(VLAN_TAG_PRESENT);
+ key_vh->tci = vh->tci | htons(VLAN_CFI_MASK);
key_vh->tpid = vh->tpid;
if (unlikely(untag_vlan)) {
@@ -358,7 +358,7 @@ static int parse_vlan(struct sk_buff *skb, struct sw_flow_key *key)
int res;
if (skb_vlan_tag_present(skb)) {
- key->eth.vlan.tci = htons(skb->vlan_tci);
+ key->eth.vlan.tci = htons(skb->vlan_tci) | htons(VLAN_CFI_MASK);
key->eth.vlan.tpid = skb->vlan_proto;
} else {
/* Parse outer vlan tag in the non-accelerated case. */
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index c670dd24b8b7..ba01fc4270bd 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -60,7 +60,7 @@ struct ovs_tunnel_info {
struct vlan_head {
__be16 tpid; /* Vlan type. Generally 802.1q or 802.1ad.*/
- __be16 tci; /* 0 if no VLAN, VLAN_TAG_PRESENT set otherwise. */
+ __be16 tci; /* 0 if no VLAN, VLAN_CFI_MASK set otherwise. */
};
#define OVS_SW_FLOW_KEY_METADATA_SIZE \
diff --git a/net/openvswitch/flow_netlink.c b/net/openvswitch/flow_netlink.c
index 865ecef68196..435a4bdf8f89 100644
--- a/net/openvswitch/flow_netlink.c
+++ b/net/openvswitch/flow_netlink.c
@@ -990,9 +990,9 @@ static int validate_vlan_from_nlattrs(const struct sw_flow_match *match,
if (a[OVS_KEY_ATTR_VLAN])
tci = nla_get_be16(a[OVS_KEY_ATTR_VLAN]);
- if (!(tci & htons(VLAN_TAG_PRESENT))) {
+ if (!(tci & htons(VLAN_CFI_MASK))) {
if (tci) {
- OVS_NLERR(log, "%s TCI does not have VLAN_TAG_PRESENT bit set.",
+ OVS_NLERR(log, "%s TCI does not have VLAN_CFI_MASK bit set.",
(inner) ? "C-VLAN" : "VLAN");
return -EINVAL;
} else if (nla_len(a[OVS_KEY_ATTR_ENCAP])) {
@@ -1013,9 +1013,9 @@ static int validate_vlan_mask_from_nlattrs(const struct sw_flow_match *match,
__be16 tci = 0;
__be16 tpid = 0;
bool encap_valid = !!(match->key->eth.vlan.tci &
- htons(VLAN_TAG_PRESENT));
+ htons(VLAN_CFI_MASK));
bool i_encap_valid = !!(match->key->eth.cvlan.tci &
- htons(VLAN_TAG_PRESENT));
+ htons(VLAN_CFI_MASK));
if (!(key_attrs & (1 << OVS_KEY_ATTR_ENCAP))) {
/* Not a VLAN. */
@@ -1039,8 +1039,8 @@ static int validate_vlan_mask_from_nlattrs(const struct sw_flow_match *match,
(inner) ? "C-VLAN" : "VLAN", ntohs(tpid));
return -EINVAL;
}
- if (!(tci & htons(VLAN_TAG_PRESENT))) {
- OVS_NLERR(log, "%s TCI mask does not have exact match for VLAN_TAG_PRESENT bit.",
+ if (!(tci & htons(VLAN_CFI_MASK))) {
+ OVS_NLERR(log, "%s TCI mask does not have exact match for VLAN_CFI_MASK bit.",
(inner) ? "C-VLAN" : "VLAN");
return -EINVAL;
}
@@ -1095,7 +1095,7 @@ static int parse_vlan_from_nlattrs(struct sw_flow_match *match,
if (err)
return err;
- encap_valid = !!(match->key->eth.vlan.tci & htons(VLAN_TAG_PRESENT));
+ encap_valid = !!(match->key->eth.vlan.tci & htons(VLAN_CFI_MASK));
if (encap_valid) {
err = __parse_vlan_from_nlattrs(match, key_attrs, true, a,
is_mask, log);
@@ -2943,7 +2943,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
vlan = nla_data(a);
if (!eth_type_vlan(vlan->vlan_tpid))
return -EINVAL;
- if (!(vlan->vlan_tci & htons(VLAN_TAG_PRESENT)))
+ if (!(vlan->vlan_tci & htons(VLAN_CFI_MASK)))
return -EINVAL;
vlan_tci = vlan->vlan_tci;
break;
@@ -2959,7 +2959,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
/* Prohibit push MPLS other than to a white list
* for packets that have a known tag order.
*/
- if (vlan_tci & htons(VLAN_TAG_PRESENT) ||
+ if (vlan_tci & htons(VLAN_CFI_MASK) ||
(eth_type != htons(ETH_P_IP) &&
eth_type != htons(ETH_P_IPV6) &&
eth_type != htons(ETH_P_ARP) &&
@@ -2971,7 +2971,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
}
case OVS_ACTION_ATTR_POP_MPLS:
- if (vlan_tci & htons(VLAN_TAG_PRESENT) ||
+ if (vlan_tci & htons(VLAN_CFI_MASK) ||
!eth_p_mpls(eth_type))
return -EINVAL;
@@ -3036,7 +3036,7 @@ static int __ovs_nla_copy_actions(struct net *net, const struct nlattr *attr,
case OVS_ACTION_ATTR_POP_ETH:
if (mac_proto != MAC_PROTO_ETHERNET)
return -EINVAL;
- if (vlan_tci & htons(VLAN_TAG_PRESENT))
+ if (vlan_tci & htons(VLAN_CFI_MASK))
return -EINVAL;
mac_proto = MAC_PROTO_NONE;
break;
--
2.19.1
^ permalink raw reply related
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
From: Edward Cree @ 2018-11-08 17:58 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Martin Lau, Yonghong Song, Alexei Starovoitov,
daniel@iogearbox.net, netdev@vger.kernel.org, Kernel Team
In-Reply-To: <20181107214922.xjqcacj5rc5hmepw@ast-mbp.dhcp.thefacebook.com>
On 07/11/18 21:49, Alexei Starovoitov wrote:
> On Wed, Nov 07, 2018 at 07:29:31PM +0000, Edward Cree wrote:
>> Whereas I don't, and I don't feel like my core criticisms have
>> been addressed _at all_. The only answer I get to "BTF should
>> store type and instance information in separate records" is
>> "it's a debuginfo",
> ...
>> I am just trying to organise
>> BTF to consist of separate _parts_ for types and instances,
>> rather than forcing both into the same Procrustean bed.
> BTF does not have and should not have instances.
> BTF is debug info only.
> This is not negotiable.
I'm not saying the instances go in BTF, I'm saying that debug info
*about* instances goes in BTF (it already does, as you keep saying
BTF is "not just pure types"), and that that ought to be
distinguished within the format from debug info about types.
> So I'm looking forward to your ideas how to describe BTF in .s
> Note such .s must have freedom to describe 'int bar(struct __sk_buff *a1, char a2)'
> as debug info while having '.globl foo; foo:' as symbol name.
I've pushed out a branch with what I have; see
https://github.com/solarflarecom/ebpf_asm/tree/btfdoc
(with some examples in dropper.s and documentation in the README).
In particular note that right now the BTF section is entirely
decoupled from the .text, so indeed there is nothing right now
tying function names to symbol names. I do not yet have anything
generating FuncInfo (or LineInfo) tables, but when I do that will
remain decoupled.
> Your other 'criticism' was about libbpf's bpf_map_find_btf_info()
> and ____btf_map_* hack. Yes. It is a hack and I'm open to change it
> if there are better suggestions. It's a convention between
> libbpf and program writers that produce elf. It's not a kernel abi.
> Nothing to do with BTF and this instance vs debug info discussion.
It's everything to do with it: it's defining a type with a magic name
(____btf_map_foo) when what we really want to do is declare an
instance (the map 'foo'). And it may not be a kernel ABI, but it's
a part of the file format you're defining (whether that's just a
'convention' or something more), and if you want the BTF ecosystem
to be more than just an llvm monoculture then the format needs to be
properly specified so that others can work with it.
> Happy to jump on the call to explain it again.
> 10:30am pacific time works for me tomorrow.
That works for me (that's in ~30 minutes from now if I've converted
correctly.) Please email me offlist with the phone number to call.
-Ed
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Stanislav Fomichev @ 2018-11-08 18:01 UTC (permalink / raw)
To: Quentin Monnet
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
jakub.kicinski, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <8c35340e-3ed7-70cd-3123-7cd0fb8824a7@netronome.com>
On 11/08, Quentin Monnet wrote:
> Hi Stanislav, thanks for the changes! More comments below.
Thank you for another round of review!
> 2018-11-07 21:39 UTC-0800 ~ Stanislav Fomichev <sdf@google.com>
> > This commit adds support for loading/attaching/detaching flow
> > dissector program. The structure of the flow dissector program is
> > assumed to be the same as in the selftests:
> >
> > * flow_dissector section with the main entry point
> > * a bunch of tail call progs
> > * a jmp_table map that is populated with the tail call progs
> >
> > When `bpftool load` is called with a flow_dissector prog (i.e. when the
> > first section is flow_dissector of 'type flow_dissector' argument is
> > passed), we load and pin all the programs/maps. User is responsible to
> > construct the jump table for the tail calls.
> >
> > The last argument of `bpftool attach` is made optional for this use
> > case.
> >
> > Example:
> > bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
> > /sys/fs/bpf/flow type flow_dissector
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 0 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IP
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 1 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IPV6
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 2 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IPV6OP
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 3 0 0 0 \
> > value pinned /sys/fs/bpf/flow/IPV6FR
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 4 0 0 0 \
> > value pinned /sys/fs/bpf/flow/MPLS
> >
> > bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
> > key 5 0 0 0 \
> > value pinned /sys/fs/bpf/flow/VLAN
> >
> > bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
> >
> > Tested by using the above lines to load the prog in
> > the test_flow_dissector.sh selftest.
> >
> > Signed-off-by: Stanislav Fomichev <sdf@google.com>
> > ---
> > .../bpftool/Documentation/bpftool-prog.rst | 36 ++++--
> > tools/bpf/bpftool/bash-completion/bpftool | 6 +-
> > tools/bpf/bpftool/common.c | 30 ++---
> > tools/bpf/bpftool/main.h | 1 +
> > tools/bpf/bpftool/prog.c | 112 +++++++++++++-----
> > 5 files changed, 126 insertions(+), 59 deletions(-)
> >
> > diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > index ac4e904b10fb..0374634c3087 100644
> > --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
> > @@ -15,7 +15,8 @@ SYNOPSIS
> > *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
> > *COMMANDS* :=
> > - { **show** | **list** | **dump xlated** | **dump jited** | **pin** | **load** | **help** }
> > + { **show** | **list** | **dump xlated** | **dump jited** | **pin** | **load**
> > + | **loadall** | **help** }
> > MAP COMMANDS
> > =============
> > @@ -24,9 +25,9 @@ MAP COMMANDS
> > | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
> > | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* | **opcodes**}]
> > | **bpftool** **prog pin** *PROG* *FILE*
> > -| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > +| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > | **bpftool** **prog help**
> > |
> > | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
> > @@ -39,7 +40,9 @@ MAP COMMANDS
> > | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** | **cgroup/post_bind6** |
> > | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4** | **cgroup/sendmsg6**
> > | }
> > -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
> > +| *ATTACH_TYPE* := {
> > +| **msg_verdict** | **skb_verdict** | **skb_parse** | **flow_dissector**
> > +| }
> > DESCRIPTION
> > @@ -79,8 +82,11 @@ DESCRIPTION
> > contain a dot character ('.'), which is reserved for future
> > extensions of *bpffs*.
> > - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
> > Load bpf program from binary *OBJ* and pin as *FILE*.
> > + **bpftool prog load** will pin only the first bpf program
> > + from the *OBJ*, **bpftool prog loadall** will pin all maps
> > + and programs from the *OBJ*.
>
> This could be improved regarding maps: with "bpftool prog load" I think we
> also load and pin all maps, but your description implies this is only the
> case with "loadall"
I don't think we pin any maps with `bpftool prog load`, we certainly load
them, but we don't pin any afaict. Can you point me to the code where we
pin the maps?
> > **type** is optional, if not specified program type will be
> > inferred from section names.
> > By default bpftool will create new maps as declared in the ELF
> > @@ -97,13 +103,17 @@ DESCRIPTION
> > contain a dot character ('.'), which is reserved for future
> > extensions of *bpffs*.
> > - **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
> > - Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > - to the map *MAP*.
> > -
> > - **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
> > - Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
> > - from the map *MAP*.
> > + **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
> > + Attach bpf program *PROG* (with type specified by
> > + *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
> > + parameter, with the exception of *flow_dissector* which is
> > + attached to current networking name space.
> > +
> > + **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
> > + Detach bpf program *PROG* (with type specified by
> > + *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
> > + parameter, with the exception of *flow_dissector* which is
> > + detached from the current networking name space.
>
> While at it could you please fix those two paragraphs to use tabs for
> indentation, as the rest of the doc? Thanks!
Time to teach my vim to use tabs in .rst files. Sorry about that.
> > **bpftool prog help**
> > Print short help message.
> > diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
> > index 3f78e6404589..ad0fc919f7ec 100644
> > --- a/tools/bpf/bpftool/bash-completion/bpftool
> > +++ b/tools/bpf/bpftool/bash-completion/bpftool
> > @@ -243,7 +243,7 @@ _bpftool()
> > # Completion depends on object and command in use
> > case $object in
> > prog)
> > - if [[ $command != "load" ]]; then
> > + if [[ $command != "load" && $command != "loadall" ]]; then
> > case $prev in
> > id)
> > _bpftool_get_prog_ids
> > @@ -299,7 +299,7 @@ _bpftool()
> > fi
> > if [[ ${#words[@]} == 6 ]]; then
> > - COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse" -- "$cur" ) )
> > + COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse flow_dissector" -- "$cur" ) )
> > return 0
> > fi
> > @@ -309,7 +309,7 @@ _bpftool()
> > fi
> > return 0
> > ;;
> > - load)
> > + load|loadall)
> > local obj
> > if [[ ${#words[@]} -lt 6 ]]; then
>
> You also want to update completion for the program types, at line 341 or so.
> Feel free to split that list on several lines, by the way :).
Will do, thanks!
> > diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
> > index 25af85304ebe..f671a921dec5 100644
> > --- a/tools/bpf/bpftool/common.c
> > +++ b/tools/bpf/bpftool/common.c
> > @@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
> > return fd;
> > }
> > -int do_pin_fd(int fd, const char *name)
> > +int mount_bpffs_for_pin(const char *name)
> > {
> > char err_str[ERR_MAX_LEN];
> > char *file;
> > char *dir;
> > int err = 0;
> > - err = bpf_obj_pin(fd, name);
> > - if (!err)
> > - goto out;
> > -
> > file = malloc(strlen(name) + 1);
> > strcpy(file, name);
> > dir = dirname(file);
> > - if (errno != EPERM || is_bpffs(dir)) {
> > - p_err("can't pin the object (%s): %s", name, strerror(errno));
> > + if (is_bpffs(dir)) {
> > + /* nothing to do if already mounted */
> > goto out_free;
> > }
>
> Nitpick: unnecessary brackets.
Ack.
> > - /* Attempt to mount bpffs, then retry pinning. */
> > err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
> > - if (!err) {
> > - err = bpf_obj_pin(fd, name);
> > - if (err)
> > - p_err("can't pin the object (%s): %s", name,
> > - strerror(errno));
> > - } else {
> > + if (err) {
> > err_str[ERR_MAX_LEN - 1] = '\0';
> > p_err("can't mount BPF file system to pin the object (%s): %s",
> > name, err_str);
> > @@ -204,10 +194,20 @@ int do_pin_fd(int fd, const char *name)
> > out_free:
> > free(file);
> > -out:
> > return err;
> > }
> > +int do_pin_fd(int fd, const char *name)
> > +{
> > + int err;
> > +
> > + err = mount_bpffs_for_pin(name);
> > + if (err)
> > + return err;
> > +
> > + return bpf_obj_pin(fd, name);
> > +}
> > +
> > int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
> > {
> > unsigned int id;
> > diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
> > index 28322ace2856..1383824c9baf 100644
> > --- a/tools/bpf/bpftool/main.h
> > +++ b/tools/bpf/bpftool/main.h
> > @@ -129,6 +129,7 @@ const char *get_fd_type_name(enum bpf_obj_type type);
> > char *get_fdinfo(int fd, const char *key);
> > int open_obj_pinned(char *path);
> > int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type);
> > +int mount_bpffs_for_pin(const char *name);
> > int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32));
> > int do_pin_fd(int fd, const char *name);
> > diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> > index 5302ee282409..a4346dd673b1 100644
> > --- a/tools/bpf/bpftool/prog.c
> > +++ b/tools/bpf/bpftool/prog.c
> > @@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
> > [BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
> > [BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
> > [BPF_SK_MSG_VERDICT] = "msg_verdict",
> > + [BPF_FLOW_DISSECTOR] = "flow_dissector",
> > [__MAX_BPF_ATTACH_TYPE] = NULL,
> > };
> > @@ -724,10 +725,11 @@ int map_replace_compar(const void *p1, const void *p2)
> > static int do_attach(int argc, char **argv)
> > {
> > enum bpf_attach_type attach_type;
> > - int err, mapfd, progfd;
> > + int err, progfd;
> > + int mapfd = 0;
> > - if (!REQ_ARGS(5)) {
> > - p_err("too few parameters for map attach");
> > + if (!REQ_ARGS(3)) {
> > + p_err("too few parameters for attach");
> > return -EINVAL;
> > }
> > @@ -740,11 +742,17 @@ static int do_attach(int argc, char **argv)
> > p_err("invalid attach type");
> > return -EINVAL;
> > }
> > - NEXT_ARG();
> > + if (attach_type != BPF_FLOW_DISSECTOR) {
> > + NEXT_ARG();
> > + if (!REQ_ARGS(2)) {
> > + p_err("too few parameters for map attach");
> > + return -EINVAL;
> > + }
> > - mapfd = map_parse_fd(&argc, &argv);
> > - if (mapfd < 0)
> > - return mapfd;
> > + mapfd = map_parse_fd(&argc, &argv);
> > + if (mapfd < 0)
> > + return mapfd;
> > + }
> > err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
> > if (err) {
> > @@ -760,10 +768,11 @@ static int do_attach(int argc, char **argv)
> > static int do_detach(int argc, char **argv)
> > {
> > enum bpf_attach_type attach_type;
> > - int err, mapfd, progfd;
> > + int err, progfd;
> > + int mapfd = 0;
> > - if (!REQ_ARGS(5)) {
> > - p_err("too few parameters for map detach");
> > + if (!REQ_ARGS(3)) {
> > + p_err("too few parameters for detach");
> > return -EINVAL;
> > }
> > @@ -776,11 +785,17 @@ static int do_detach(int argc, char **argv)
> > p_err("invalid attach type");
> > return -EINVAL;
> > }
> > - NEXT_ARG();
> > + if (attach_type != BPF_FLOW_DISSECTOR) {
> > + NEXT_ARG();
> > + if (!REQ_ARGS(2)) {
> > + p_err("too few parameters for map detach");
> > + return -EINVAL;
> > + }
>
> Would that make sense to factor argument checks or parsing for do_attach()
> and do_detach() to some extent? In order to reduce the number of
> attach-type-based exceptions to add in the code if we have other attach
> types that do not take maps in the future.
I can move all argument parsing into a new function and use it from both
do_attach and do_detach.
> > - mapfd = map_parse_fd(&argc, &argv);
> > - if (mapfd < 0)
> > - return mapfd;
> > + mapfd = map_parse_fd(&argc, &argv);
> > + if (mapfd < 0)
> > + return mapfd;
> > + }
> > err = bpf_prog_detach2(progfd, mapfd, attach_type);
> > if (err) {
> > @@ -792,15 +807,16 @@ static int do_detach(int argc, char **argv)
> > jsonw_null(json_wtr);
> > return 0;
> > }
> > -static int do_load(int argc, char **argv)
> > +
> > +static int load_with_options(int argc, char **argv, bool first_prog_only)
> > {
> > enum bpf_attach_type expected_attach_type;
> > struct bpf_object_open_attr attr = {
> > .prog_type = BPF_PROG_TYPE_UNSPEC,
> > };
> > struct map_replace *map_replace = NULL;
> > + struct bpf_program *prog = NULL, *pos;
> > unsigned int old_map_fds = 0;
> > - struct bpf_program *prog;
> > struct bpf_object *obj;
> > struct bpf_map *map;
> > const char *pinfile;
> > @@ -918,14 +934,20 @@ static int do_load(int argc, char **argv)
> > goto err_free_reuse_maps;
> > }
> > - prog = bpf_program__next(NULL, obj);
> > - if (!prog) {
> > - p_err("object file doesn't contain any bpf program");
> > - goto err_close_obj;
> > + if (first_prog_only) {
> > + prog = bpf_program__next(NULL, obj);
> > + if (!prog) {
> > + p_err("object file doesn't contain any bpf program");
> > + goto err_close_obj;
> > + }
> > }
> > - bpf_program__set_ifindex(prog, ifindex);
> > if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
> > + if (!prog) {
> > + p_err("can not guess program type when loading all programs\n");
> > + goto err_close_obj;
> > + }
> > +
> > const char *sec_name = bpf_program__title(prog, false);
> > err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
> > @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
> > goto err_close_obj;
> > }
> > }
> > - bpf_program__set_type(prog, attr.prog_type);
> > - bpf_program__set_expected_attach_type(prog, expected_attach_type);
> > +
> > + bpf_object__for_each_program(pos, obj) {
> > + bpf_program__set_ifindex(pos, ifindex);
> > + bpf_program__set_type(pos, attr.prog_type);
> > + bpf_program__set_expected_attach_type(pos,
> > + expected_attach_type);
> > + }
>
> I still believe you can have programs of different types here, and be able
> to load them. I tried it and managed to have it working fine. If no type is
> provided from command line we can retrieve types for each program from its
> section name. If a type is provided on the command line, we can do the same,
> but I am not sure we should do it, or impose that type for all programs
> instead.
I can move auto-detection into this new bpf_object__for_each_program
loop. So if no type is specified, try to infer the type from each prog
section name, otherwise, use the provided one for all progs. Do we want
something like that?
Btw, do you have some existing real life example of where it's needed so
I can test this new implementation? (maybe something under samples/ ?)
> > qsort(map_replace, old_map_fds, sizeof(*map_replace),
> > map_replace_compar);
> > @@ -1001,9 +1028,25 @@ static int do_load(int argc, char **argv)
> > goto err_close_obj;
> > }
> > - if (do_pin_fd(bpf_program__fd(prog), pinfile))
> > + err = mount_bpffs_for_pin(pinfile);
> > + if (err)
> > goto err_close_obj;
> > + if (prog) {
>
> Nit: Maybe "if (first_prog_only) {" instead? If I understand correctly, at
> this stage it should be equivalent, but in my opinion it would make it
> easier to understand why we have two cases here.
Sure, I can do that if you think that's more readable, I don't have a
preference.
> > + err = bpf_obj_pin(bpf_program__fd(prog), pinfile);
> > + if (err) {
> > + p_err("failed to pin program %s",
> > + bpf_program__title(prog, false));
> > + goto err_close_obj;
> > + }
> > + } else {
> > + err = bpf_object__pin(obj, pinfile);
> > + if (err) {
> > + p_err("failed to pin all programs");
> > + goto err_close_obj;
> > + }
> > + }
> > +
> > if (json_output)
> > jsonw_null(json_wtr);
> > @@ -1023,6 +1066,16 @@ static int do_load(int argc, char **argv)
> > return -1;
> > }
> > +static int do_load(int argc, char **argv)
> > +{
> > + return load_with_options(argc, argv, true);
> > +}
> > +
> > +static int do_loadall(int argc, char **argv)
> > +{
> > + return load_with_options(argc, argv, false);
> > +}
> > +
> > static int do_help(int argc, char **argv)
> > {
> > if (json_output) {
> > @@ -1035,10 +1088,11 @@ static int do_help(int argc, char **argv)
> > " %s %s dump xlated PROG [{ file FILE | opcodes | visual }]\n"
> > " %s %s dump jited PROG [{ file FILE | opcodes }]\n"
> > " %s %s pin PROG FILE\n"
> > - " %s %s load OBJ FILE [type TYPE] [dev NAME] \\\n"
> > + " %s %s { load | loadall } OBJ FILE \\\n"
> > + " [type TYPE] [dev NAME] \\\n"
> > " [map { idx IDX | name NAME } MAP]\n"
> > - " %s %s attach PROG ATTACH_TYPE MAP\n"
> > - " %s %s detach PROG ATTACH_TYPE MAP\n"
> > + " %s %s attach PROG ATTACH_TYPE [MAP]\n"
> > + " %s %s detach PROG ATTACH_TYPE [MAP]\n"
> > " %s %s help\n"
> > "\n"
> > " " HELP_SPEC_MAP "\n"
> > @@ -1050,7 +1104,8 @@ static int do_help(int argc, char **argv)
> > " cgroup/bind4 | cgroup/bind6 | cgroup/post_bind4 |\n"
> > " cgroup/post_bind6 | cgroup/connect4 | cgroup/connect6 |\n"
> > " cgroup/sendmsg4 | cgroup/sendmsg6 }\n"
> > - " ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse }\n"
> > + " ATTACH_TYPE := { msg_verdict | skb_verdict | skb_parse |\n"
> > + " flow_dissector }\n"
> > " " HELP_SPEC_OPTIONS "\n"
> > "",
> > bin_name, argv[-2], bin_name, argv[-2], bin_name, argv[-2],
> > @@ -1067,6 +1122,7 @@ static const struct cmd cmds[] = {
> > { "dump", do_dump },
> > { "pin", do_pin },
> > { "load", do_load },
> > + { "loadall", do_loadall },
> > { "attach", do_attach },
> > { "detach", do_detach },
> > { 0 }
> >
>
^ permalink raw reply
* Re: [PATCH net-next] net: qca_spi: Add available buffer space verification
From: David Miller @ 2018-11-09 3:41 UTC (permalink / raw)
To: stefan.wahren; +Cc: michael.heimpold, netdev, linux-kernel
In-Reply-To: <1541684301-15824-1-git-send-email-stefan.wahren@i2se.com>
From: Stefan Wahren <stefan.wahren@i2se.com>
Date: Thu, 8 Nov 2018 14:38:21 +0100
> Interferences on the SPI line could distort the response of
> available buffer space. So at least we should check that the
> response doesn't exceed the maximum available buffer space.
> In error case increase a new error counter and retry it later.
> This behavior avoids buffer errors in the QCA7000, which
> results in an unnecessary chip reset including packet loss.
>
> Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Applied.
^ permalink raw reply
* Re: Kernel 4.19 network performance - forwarding/routing normal users traffic
From: David Ahern @ 2018-11-08 18:05 UTC (permalink / raw)
To: Paweł Staszewski, Jesper Dangaard Brouer; +Cc: netdev, Yoel Caspersen
In-Reply-To: <6d52b197-c303-eeac-2992-cedfd78115c0@itcare.pl>
On 11/8/18 10:30 AM, Paweł Staszewski wrote:
> Wondering about this:
> swapper 0 [045] 68494.770287: fib:fib_table_lookup: table 254 oif 0
> iif 6 proto 1 192.168.22.237/0 -> 172.16.0.2/0 tos 0 scope 0 flags 0 ==>
> dev vlan1740 gw 0.0.0.0 src 172.16.0.1 err 0
> 7fff818c13b5 fib_table_lookup ([kernel.kallsyms])
>
> oif 0 ?
>
> Is that correct here ?
ingress path so iif is set to the vlan device and oif is 0.
egress lookups (e.g., locally generated traffic) have oif non-0.
^ permalink raw reply
* Re: a propose of snmp counter document
From: Cong Wang @ 2018-11-08 18:05 UTC (permalink / raw)
To: yupeng0921; +Cc: Linux Kernel Network Developers
In-Reply-To: <CAG3TDc3Za0hMk6r=iNq-rkCw9U-w1OL7bn2X=Sc4HP5FCWLcwA@mail.gmail.com>
On Thu, Nov 8, 2018 at 12:10 AM peng yu <yupeng0921@gmail.com> wrote:
>
> I'm planing to write a document which explains the meaning of the
> kernel snmp counters, and combine the explanations with some tests,
> because I found lots of the 'TcpExt' and 'IpExt' counters are not
> explained in any document. Here is a draft:
> https://github.com/yupeng0921/iproute2_learning/blob/master/nstat.md
> It is still on going. I think it might be useful. Besides put it on my
> git repo, could someone have any suggestion about any place I
> could contribute this document to?
Good work! It has been in my todo list for a long time.
I believe we have enough room in Documentation/networking/
for it. You can follow the normal patch submission process to
contribute to upstream:
https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html
Thanks!
^ permalink raw reply
* Re: [RFC perf,bpf 1/5] perf, bpf: Introduce PERF_RECORD_BPF_EVENT
From: Song Liu @ 2018-11-08 18:04 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Netdev, lkml, Kernel Team, ast@kernel.org, daniel@iogearbox.net,
acme@kernel.org
In-Reply-To: <20181108150028.GU9761@hirez.programming.kicks-ass.net>
Hi Peter,
> On Nov 8, 2018, at 7:00 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Nov 07, 2018 at 06:25:04PM +0000, Song Liu wrote:
>>
>>
>>> On Nov 7, 2018, at 12:40 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>>>
>>> On Tue, Nov 06, 2018 at 12:52:42PM -0800, Song Liu wrote:
>>>> For better performance analysis of BPF programs, this patch introduces
>>>> PERF_RECORD_BPF_EVENT, a new perf_event_type that exposes BPF program
>>>> load/unload information to user space.
>>>>
>>>> /*
>>>> * Record different types of bpf events:
>>>> * enum perf_bpf_event_type {
>>>> * PERF_BPF_EVENT_UNKNOWN = 0,
>>>> * PERF_BPF_EVENT_PROG_LOAD = 1,
>>>> * PERF_BPF_EVENT_PROG_UNLOAD = 2,
>>>> * };
>>>> *
>>>> * struct {
>>>> * struct perf_event_header header;
>>>> * u16 type;
>>>> * u16 flags;
>>>> * u32 id; // prog_id or map_id
>>>> * };
>>>> */
>>>> PERF_RECORD_BPF_EVENT = 17,
>>>>
>>>> PERF_RECORD_BPF_EVENT contains minimal information about the BPF program.
>>>> Perf utility (or other user space tools) should listen to this event and
>>>> fetch more details about the event via BPF syscalls
>>>> (BPF_PROG_GET_FD_BY_ID, BPF_OBJ_GET_INFO_BY_FD, etc.).
>>>
>>> Why !? You're failing to explain why it cannot provide the full
>>> information there.
>>
>> Aha, I missed this part. I will add the following to next version. Please
>> let me know if anything is not clear.
>
>>
>> This design decision is picked for the following reasons. First, BPF
>> programs could be loaded-and-jited and/or unloaded before/during/after
>> perf-record run. Once a BPF programs is unloaded, it is impossible to
>> recover details of the program. It is impossible to provide the
>> information through a simple key (like the build ID). Second, BPF prog
>> annotation is under fast developments. Multiple informations will be
>> added to bpf_prog_info in the next few releases. Including all the
>> information of a BPF program in the perf ring buffer requires frequent
>> changes to the perf ABI, and thus makes it very difficult to manage
>> compatibility of perf utility.
>
> So I don't agree with that reasoning. If you want symbol information
> you'll just have to commit to some form of ABI. That bpf_prog_info is an
> ABI too.
At the beginning of the perf-record run, perf need to query bpf_prog_info
of already loaded BPF programs. Therefore, we need to commit to the
bpf_prog_info ABI. If we also include full information of the BPF program
in the perf ring buffer, we will commit to TWO ABIs.
Also, perf-record write the event to perf.data file, so the data need to be
serialized. This is implemented in patch 4/5. To include the data in the
ring buffer, we will need another piece of code in the kernel to do the
same serialization work.
On the other hand, processing BPF load/unload events synchronously should
not introduce too much overhead for meaningful use cases. If many BPF progs
are being loaded/unloaded within short period of time, it is not the steady
state that profiling works care about.
Would these resolve your concerns?
Thanks,
Song
^ permalink raw reply
* Re: [PATCHv2] net: stmmac: Fix RX packet size > 8191
From: David Miller @ 2018-11-09 3:47 UTC (permalink / raw)
To: thor.thayer
Cc: peppe.cavallaro, alexandre.torgue, joabreu, netdev, linux-kernel
In-Reply-To: <1541698935-9752-1-git-send-email-thor.thayer@linux.intel.com>
From: thor.thayer@linux.intel.com
Date: Thu, 8 Nov 2018 11:42:14 -0600
> From: Thor Thayer <thor.thayer@linux.intel.com>
>
> Ping problems with packets > 8191 as shown:
>
> PING 192.168.1.99 (192.168.1.99) 8150(8178) bytes of data.
> 8158 bytes from 192.168.1.99: icmp_seq=1 ttl=64 time=0.669 ms
> wrong data byte 8144 should be 0xd0 but was 0x0
> 16 10 11 12 13 14 15 16 17 18 19 1a 1b 1c 1d 1e 1f
> 20 21 22 23 24 25 26 27 28 29 2a 2b 2c 2d 2e 2f
> %< ---------------snip--------------------------------------
> 8112 b0 b1 b2 b3 b4 b5 b6 b7 b8 b9 ba bb bc bd be bf
> c0 c1 c2 c3 c4 c5 c6 c7 c8 c9 ca cb cc cd ce cf
> 8144 0 0 0 0 d0 d1
> ^^^^^^^
> Notice the 4 bytes of 0 before the expected byte of d0.
>
> Databook notes that the RX buffer must be a multiple of 4/8/16
> bytes [1].
>
> Update the DMA Buffer size define to 8188 instead of 8192. Remove
> the -1 from the RX buffer size allocations and use the new
> DMA Buffer size directly.
>
> [1] Synopsys DesignWare Cores Ethernet MAC Universal v3.70a
> [section 8.4.2 - Table 8-24]
>
> Tested on SoCFPGA Stratix10 with ping sweep from 100 to 8300 byte packets.
>
> Fixes: 286a83721720 ("stmmac: add CHAINED descriptor mode support (V4)")
> Suggested-by: Jose Abreu <jose.abreu@synopsys.com>
> Signed-off-by: Thor Thayer <thor.thayer@linux.intel.com>
Applied.
^ permalink raw reply
* Re: [PATCH bpf-next v2 02/13] bpf: btf: Add BTF_KIND_FUNC and BTF_KIND_FUNC_PROTO
From: Alexei Starovoitov @ 2018-11-08 18:21 UTC (permalink / raw)
To: Edward Cree
Cc: Martin Lau, Yonghong Song, Alexei Starovoitov,
daniel@iogearbox.net, netdev@vger.kernel.org, Kernel Team
In-Reply-To: <14c5120a-b67a-a514-5e9d-2895a4a841df@solarflare.com>
On Thu, Nov 08, 2018 at 05:58:56PM +0000, Edward Cree wrote:
>
> > Happy to jump on the call to explain it again.
> > 10:30am pacific time works for me tomorrow.
> That works for me (that's in ~30 minutes from now if I've converted
> correctly.) Please email me offlist with the phone number to call.
no offlist. public link for anyone to join:
https://bluejeans.com/867080076/
I have hard cutoff at 11am though.
^ permalink raw reply
* Re: [PATCH v3 bpf-next 4/4] bpftool: support loading flow dissector
From: Quentin Monnet @ 2018-11-08 18:21 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Stanislav Fomichev, netdev, linux-kselftest, ast, daniel, shuah,
jakub.kicinski, guro, jiong.wang, bhole_prashant_q7,
john.fastabend, jbenc, treeze.taeung, yhs, osk, sandipan
In-Reply-To: <20181108180153.tbssxcgkkq5xcdxc@mini-arch>
[-- Attachment #1.1: Type: text/plain, Size: 19076 bytes --]
2018-11-08 10:01 UTC-0800 ~ Stanislav Fomichev <sdf@fomichev.me>
> On 11/08, Quentin Monnet wrote:
>> Hi Stanislav, thanks for the changes! More comments below.
> Thank you for another round of review!
>
>> 2018-11-07 21:39 UTC-0800 ~ Stanislav Fomichev <sdf@google.com>
>>> This commit adds support for loading/attaching/detaching flow
>>> dissector program. The structure of the flow dissector program is
>>> assumed to be the same as in the selftests:
>>>
>>> * flow_dissector section with the main entry point
>>> * a bunch of tail call progs
>>> * a jmp_table map that is populated with the tail call progs
>>>
>>> When `bpftool load` is called with a flow_dissector prog (i.e. when the
>>> first section is flow_dissector of 'type flow_dissector' argument is
>>> passed), we load and pin all the programs/maps. User is responsible to
>>> construct the jump table for the tail calls.
>>>
>>> The last argument of `bpftool attach` is made optional for this use
>>> case.
>>>
>>> Example:
>>> bpftool prog load tools/testing/selftests/bpf/bpf_flow.o \
>>> /sys/fs/bpf/flow type flow_dissector
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 0 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IP
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 1 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IPV6
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 2 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IPV6OP
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 3 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/IPV6FR
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 4 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/MPLS
>>>
>>> bpftool map update pinned /sys/fs/bpf/flow/jmp_table \
>>> key 5 0 0 0 \
>>> value pinned /sys/fs/bpf/flow/VLAN
>>>
>>> bpftool prog attach pinned /sys/fs/bpf/flow/flow_dissector flow_dissector
>>>
>>> Tested by using the above lines to load the prog in
>>> the test_flow_dissector.sh selftest.
>>>
>>> Signed-off-by: Stanislav Fomichev <sdf@google.com>
>>> ---
>>> .../bpftool/Documentation/bpftool-prog.rst | 36 ++++--
>>> tools/bpf/bpftool/bash-completion/bpftool | 6 +-
>>> tools/bpf/bpftool/common.c | 30 ++---
>>> tools/bpf/bpftool/main.h | 1 +
>>> tools/bpf/bpftool/prog.c | 112 +++++++++++++-----
>>> 5 files changed, 126 insertions(+), 59 deletions(-)
>>>
>>> diff --git a/tools/bpf/bpftool/Documentation/bpftool-prog.rst b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> index ac4e904b10fb..0374634c3087 100644
>>> --- a/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> +++ b/tools/bpf/bpftool/Documentation/bpftool-prog.rst
>>> @@ -15,7 +15,8 @@ SYNOPSIS
>>> *OPTIONS* := { { **-j** | **--json** } [{ **-p** | **--pretty** }] | { **-f** | **--bpffs** } }
>>> *COMMANDS* :=
>>> - { **show** | **list** | **dump xlated** | **dump jited** | **pin** | **load** | **help** }
>>> + { **show** | **list** | **dump xlated** | **dump jited** | **pin** | **load**
>>> + | **loadall** | **help** }
>>> MAP COMMANDS
>>> =============
>>> @@ -24,9 +25,9 @@ MAP COMMANDS
>>> | **bpftool** **prog dump xlated** *PROG* [{**file** *FILE* | **opcodes** | **visual**}]
>>> | **bpftool** **prog dump jited** *PROG* [{**file** *FILE* | **opcodes**}]
>>> | **bpftool** **prog pin** *PROG* *FILE*
>>> -| **bpftool** **prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> -| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* *MAP*
>>> -| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* *MAP*
>>> +| **bpftool** **prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> +| **bpftool** **prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>>> +| **bpftool** **prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>>> | **bpftool** **prog help**
>>> |
>>> | *MAP* := { **id** *MAP_ID* | **pinned** *FILE* }
>>> @@ -39,7 +40,9 @@ MAP COMMANDS
>>> | **cgroup/bind4** | **cgroup/bind6** | **cgroup/post_bind4** | **cgroup/post_bind6** |
>>> | **cgroup/connect4** | **cgroup/connect6** | **cgroup/sendmsg4** | **cgroup/sendmsg6**
>>> | }
>>> -| *ATTACH_TYPE* := { **msg_verdict** | **skb_verdict** | **skb_parse** }
>>> +| *ATTACH_TYPE* := {
>>> +| **msg_verdict** | **skb_verdict** | **skb_parse** | **flow_dissector**
>>> +| }
>>> DESCRIPTION
>>> @@ -79,8 +82,11 @@ DESCRIPTION
>>> contain a dot character ('.'), which is reserved for future
>>> extensions of *bpffs*.
>>> - **bpftool prog load** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> + **bpftool prog { load | loadall }** *OBJ* *FILE* [**type** *TYPE*] [**map** {**idx** *IDX* | **name** *NAME*} *MAP*] [**dev** *NAME*]
>>> Load bpf program from binary *OBJ* and pin as *FILE*.
>>> + **bpftool prog load** will pin only the first bpf program
>>> + from the *OBJ*, **bpftool prog loadall** will pin all maps
>>> + and programs from the *OBJ*.
>>
>> This could be improved regarding maps: with "bpftool prog load" I think we
>> also load and pin all maps, but your description implies this is only the
>> case with "loadall"
> I don't think we pin any maps with `bpftool prog load`, we certainly load
> them, but we don't pin any afaict. Can you point me to the code where we
> pin the maps?
>
My bad. I read "pin" but thought "load". It does not pin them indeed,
sorry about that.
>>> **type** is optional, if not specified program type will be
>>> inferred from section names.
>>> By default bpftool will create new maps as declared in the ELF
>>> @@ -97,13 +103,17 @@ DESCRIPTION
>>> contain a dot character ('.'), which is reserved for future
>>> extensions of *bpffs*.
>>> - **bpftool prog attach** *PROG* *ATTACH_TYPE* *MAP*
>>> - Attach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
>>> - to the map *MAP*.
>>> -
>>> - **bpftool prog detach** *PROG* *ATTACH_TYPE* *MAP*
>>> - Detach bpf program *PROG* (with type specified by *ATTACH_TYPE*)
>>> - from the map *MAP*.
>>> + **bpftool prog attach** *PROG* *ATTACH_TYPE* [*MAP*]
>>> + Attach bpf program *PROG* (with type specified by
>>> + *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
>>> + parameter, with the exception of *flow_dissector* which is
>>> + attached to current networking name space.
>>> +
>>> + **bpftool prog detach** *PROG* *ATTACH_TYPE* [*MAP*]
>>> + Detach bpf program *PROG* (with type specified by
>>> + *ATTACH_TYPE*). Most *ATTACH_TYPEs* require a *MAP*
>>> + parameter, with the exception of *flow_dissector* which is
>>> + detached from the current networking name space.
>>
>> While at it could you please fix those two paragraphs to use tabs for
>> indentation, as the rest of the doc? Thanks!
> Time to teach my vim to use tabs in .rst files. Sorry about that.
Those paragraphs were using spaces already, so you didn't introduce that
:). But all others use tabs so its a good occasion to fix it.
>>> **bpftool prog help**
>>> Print short help message.
>>> diff --git a/tools/bpf/bpftool/bash-completion/bpftool b/tools/bpf/bpftool/bash-completion/bpftool
>>> index 3f78e6404589..ad0fc919f7ec 100644
>>> --- a/tools/bpf/bpftool/bash-completion/bpftool
>>> +++ b/tools/bpf/bpftool/bash-completion/bpftool
>>> @@ -243,7 +243,7 @@ _bpftool()
>>> # Completion depends on object and command in use
>>> case $object in
>>> prog)
>>> - if [[ $command != "load" ]]; then
>>> + if [[ $command != "load" && $command != "loadall" ]]; then
>>> case $prev in
>>> id)
>>> _bpftool_get_prog_ids
>>> @@ -299,7 +299,7 @@ _bpftool()
>>> fi
>>> if [[ ${#words[@]} == 6 ]]; then
>>> - COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse" -- "$cur" ) )
>>> + COMPREPLY=( $( compgen -W "msg_verdict skb_verdict skb_parse flow_dissector" -- "$cur" ) )
>>> return 0
>>> fi
>>> @@ -309,7 +309,7 @@ _bpftool()
>>> fi
>>> return 0
>>> ;;
>>> - load)
>>> + load|loadall)
>>> local obj
>>> if [[ ${#words[@]} -lt 6 ]]; then
>>
>> You also want to update completion for the program types, at line 341 or so.
>> Feel free to split that list on several lines, by the way :).
> Will do, thanks!
>
>>> diff --git a/tools/bpf/bpftool/common.c b/tools/bpf/bpftool/common.c
>>> index 25af85304ebe..f671a921dec5 100644
>>> --- a/tools/bpf/bpftool/common.c
>>> +++ b/tools/bpf/bpftool/common.c
>>> @@ -169,34 +169,24 @@ int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type)
>>> return fd;
>>> }
>>> -int do_pin_fd(int fd, const char *name)
>>> +int mount_bpffs_for_pin(const char *name)
>>> {
>>> char err_str[ERR_MAX_LEN];
>>> char *file;
>>> char *dir;
>>> int err = 0;
>>> - err = bpf_obj_pin(fd, name);
>>> - if (!err)
>>> - goto out;
>>> -
>>> file = malloc(strlen(name) + 1);
>>> strcpy(file, name);
>>> dir = dirname(file);
>>> - if (errno != EPERM || is_bpffs(dir)) {
>>> - p_err("can't pin the object (%s): %s", name, strerror(errno));
>>> + if (is_bpffs(dir)) {
>>> + /* nothing to do if already mounted */
>>> goto out_free;
>>> }
>>
>> Nitpick: unnecessary brackets.
> Ack.
>
>>> - /* Attempt to mount bpffs, then retry pinning. */
>>> err = mnt_bpffs(dir, err_str, ERR_MAX_LEN);
>>> - if (!err) {
>>> - err = bpf_obj_pin(fd, name);
>>> - if (err)
>>> - p_err("can't pin the object (%s): %s", name,
>>> - strerror(errno));
>>> - } else {
>>> + if (err) {
>>> err_str[ERR_MAX_LEN - 1] = '\0';
>>> p_err("can't mount BPF file system to pin the object (%s): %s",
>>> name, err_str);
>>> @@ -204,10 +194,20 @@ int do_pin_fd(int fd, const char *name)
>>> out_free:
>>> free(file);
>>> -out:
>>> return err;
>>> }
>>> +int do_pin_fd(int fd, const char *name)
>>> +{
>>> + int err;
>>> +
>>> + err = mount_bpffs_for_pin(name);
>>> + if (err)
>>> + return err;
>>> +
>>> + return bpf_obj_pin(fd, name);
>>> +}
>>> +
>>> int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32))
>>> {
>>> unsigned int id;
>>> diff --git a/tools/bpf/bpftool/main.h b/tools/bpf/bpftool/main.h
>>> index 28322ace2856..1383824c9baf 100644
>>> --- a/tools/bpf/bpftool/main.h
>>> +++ b/tools/bpf/bpftool/main.h
>>> @@ -129,6 +129,7 @@ const char *get_fd_type_name(enum bpf_obj_type type);
>>> char *get_fdinfo(int fd, const char *key);
>>> int open_obj_pinned(char *path);
>>> int open_obj_pinned_any(char *path, enum bpf_obj_type exp_type);
>>> +int mount_bpffs_for_pin(const char *name);
>>> int do_pin_any(int argc, char **argv, int (*get_fd_by_id)(__u32));
>>> int do_pin_fd(int fd, const char *name);
>>> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
>>> index 5302ee282409..a4346dd673b1 100644
>>> --- a/tools/bpf/bpftool/prog.c
>>> +++ b/tools/bpf/bpftool/prog.c
>>> @@ -81,6 +81,7 @@ static const char * const attach_type_strings[] = {
>>> [BPF_SK_SKB_STREAM_PARSER] = "stream_parser",
>>> [BPF_SK_SKB_STREAM_VERDICT] = "stream_verdict",
>>> [BPF_SK_MSG_VERDICT] = "msg_verdict",
>>> + [BPF_FLOW_DISSECTOR] = "flow_dissector",
>>> [__MAX_BPF_ATTACH_TYPE] = NULL,
>>> };
>>> @@ -724,10 +725,11 @@ int map_replace_compar(const void *p1, const void *p2)
>>> static int do_attach(int argc, char **argv)
>>> {
>>> enum bpf_attach_type attach_type;
>>> - int err, mapfd, progfd;
>>> + int err, progfd;
>>> + int mapfd = 0;
>>> - if (!REQ_ARGS(5)) {
>>> - p_err("too few parameters for map attach");
>>> + if (!REQ_ARGS(3)) {
>>> + p_err("too few parameters for attach");
>>> return -EINVAL;
>>> }
>>> @@ -740,11 +742,17 @@ static int do_attach(int argc, char **argv)
>>> p_err("invalid attach type");
>>> return -EINVAL;
>>> }
>>> - NEXT_ARG();
>>> + if (attach_type != BPF_FLOW_DISSECTOR) {
>>> + NEXT_ARG();
>>> + if (!REQ_ARGS(2)) {
>>> + p_err("too few parameters for map attach");
>>> + return -EINVAL;
>>> + }
>>> - mapfd = map_parse_fd(&argc, &argv);
>>> - if (mapfd < 0)
>>> - return mapfd;
>>> + mapfd = map_parse_fd(&argc, &argv);
>>> + if (mapfd < 0)
>>> + return mapfd;
>>> + }
>>> err = bpf_prog_attach(progfd, mapfd, attach_type, 0);
>>> if (err) {
>>> @@ -760,10 +768,11 @@ static int do_attach(int argc, char **argv)
>>> static int do_detach(int argc, char **argv)
>>> {
>>> enum bpf_attach_type attach_type;
>>> - int err, mapfd, progfd;
>>> + int err, progfd;
>>> + int mapfd = 0;
>>> - if (!REQ_ARGS(5)) {
>>> - p_err("too few parameters for map detach");
>>> + if (!REQ_ARGS(3)) {
>>> + p_err("too few parameters for detach");
>>> return -EINVAL;
>>> }
>>> @@ -776,11 +785,17 @@ static int do_detach(int argc, char **argv)
>>> p_err("invalid attach type");
>>> return -EINVAL;
>>> }
>>> - NEXT_ARG();
>>> + if (attach_type != BPF_FLOW_DISSECTOR) {
>>> + NEXT_ARG();
>>> + if (!REQ_ARGS(2)) {
>>> + p_err("too few parameters for map detach");
>>> + return -EINVAL;
>>> + }
>>
>> Would that make sense to factor argument checks or parsing for do_attach()
>> and do_detach() to some extent? In order to reduce the number of
>> attach-type-based exceptions to add in the code if we have other attach
>> types that do not take maps in the future.
> I can move all argument parsing into a new function and use it from both
> do_attach and do_detach.
Sounds good to me, thanks!
>>> - mapfd = map_parse_fd(&argc, &argv);
>>> - if (mapfd < 0)
>>> - return mapfd;
>>> + mapfd = map_parse_fd(&argc, &argv);
>>> + if (mapfd < 0)
>>> + return mapfd;
>>> + }
>>> err = bpf_prog_detach2(progfd, mapfd, attach_type);
>>> if (err) {
>>> @@ -792,15 +807,16 @@ static int do_detach(int argc, char **argv)
>>> jsonw_null(json_wtr);
>>> return 0;
>>> }
>>> -static int do_load(int argc, char **argv)
>>> +
>>> +static int load_with_options(int argc, char **argv, bool first_prog_only)
>>> {
>>> enum bpf_attach_type expected_attach_type;
>>> struct bpf_object_open_attr attr = {
>>> .prog_type = BPF_PROG_TYPE_UNSPEC,
>>> };
>>> struct map_replace *map_replace = NULL;
>>> + struct bpf_program *prog = NULL, *pos;
>>> unsigned int old_map_fds = 0;
>>> - struct bpf_program *prog;
>>> struct bpf_object *obj;
>>> struct bpf_map *map;
>>> const char *pinfile;
>>> @@ -918,14 +934,20 @@ static int do_load(int argc, char **argv)
>>> goto err_free_reuse_maps;
>>> }
>>> - prog = bpf_program__next(NULL, obj);
>>> - if (!prog) {
>>> - p_err("object file doesn't contain any bpf program");
>>> - goto err_close_obj;
>>> + if (first_prog_only) {
>>> + prog = bpf_program__next(NULL, obj);
>>> + if (!prog) {
>>> + p_err("object file doesn't contain any bpf program");
>>> + goto err_close_obj;
>>> + }
>>> }
>>> - bpf_program__set_ifindex(prog, ifindex);
>>> if (attr.prog_type == BPF_PROG_TYPE_UNSPEC) {
>>> + if (!prog) {
>>> + p_err("can not guess program type when loading all programs\n");
>>> + goto err_close_obj;
>>> + }
>>> +
>>> const char *sec_name = bpf_program__title(prog, false);
>>> err = libbpf_prog_type_by_name(sec_name, &attr.prog_type,
>>> @@ -936,8 +958,13 @@ static int do_load(int argc, char **argv)
>>> goto err_close_obj;
>>> }
>>> }
>>> - bpf_program__set_type(prog, attr.prog_type);
>>> - bpf_program__set_expected_attach_type(prog, expected_attach_type);
>>> +
>>> + bpf_object__for_each_program(pos, obj) {
>>> + bpf_program__set_ifindex(pos, ifindex);
>>> + bpf_program__set_type(pos, attr.prog_type);
>>> + bpf_program__set_expected_attach_type(pos,
>>> + expected_attach_type);
>>> + }
>>
>> I still believe you can have programs of different types here, and be able
>> to load them. I tried it and managed to have it working fine. If no type is
>> provided from command line we can retrieve types for each program from its
>> section name. If a type is provided on the command line, we can do the same,
>> but I am not sure we should do it, or impose that type for all programs
>> instead.
> I can move auto-detection into this new bpf_object__for_each_program
> loop. So if no type is specified, try to infer the type from each prog
> section name, otherwise, use the provided one for all progs. Do we want
> something like that?
This is what I have in mind. But others may disagree.
> Btw, do you have some existing real life example of where it's needed so
> I can test this new implementation? (maybe something under samples/ ?)
I thought about an ELF file containing both an XDP and a TC classifier
program for example. XDP can mark programs for TC, then TC process them
with all the facilities we have for skbs. It does not _have_ to be in
the same ELF file, but could be.
I haven't searched samples/bpf/ in depth, but a grep on SEC shows a
couple of files with several types (kprobe/kretprobe, classifier/xdp).
samples/bpf/xdp2skb_meta_kern.c looks like a good candidate. Or actually
for testing purposes, I simply used the following:
#define SEC(NAME) __attribute__((section(NAME), used))
int _version SEC("version") = 1;
SEC("classifier")
int func()
{
return 1;
}
SEC("xdp")
int funcbar()
{
return 0;
}
>>> qsort(map_replace, old_map_fds, sizeof(*map_replace),
>>> map_replace_compar);
>>> @@ -1001,9 +1028,25 @@ static int do_load(int argc, char **argv)
>>> goto err_close_obj;
>>> }
>>> - if (do_pin_fd(bpf_program__fd(prog), pinfile))
>>> + err = mount_bpffs_for_pin(pinfile);
>>> + if (err)
>>> goto err_close_obj;
>>> + if (prog) {
>>
>> Nit: Maybe "if (first_prog_only) {" instead? If I understand correctly, at
>> this stage it should be equivalent, but in my opinion it would make it
>> easier to understand why we have two cases here.
> Sure, I can do that if you think that's more readable, I don't have a
> preference.
Thanks!
Quentin
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 801 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-09 3:58 UTC (permalink / raw)
To: Jason Wang
Cc: Tiwei Bie, virtualization, linux-kernel, netdev, virtio-dev, wexu,
jfreimann
In-Reply-To: <21d6dbd9-8f78-6939-0e80-27b470aeb00a@redhat.com>
On Fri, Nov 09, 2018 at 10:25:28AM +0800, Jason Wang wrote:
>
> On 2018/11/8 下午10:14, Michael S. Tsirkin wrote:
> > On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
> > > On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > > > +
> > > > > > + if (vq->vq.num_free < descs_used) {
> > > > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > > + descs_used, vq->vq.num_free);
> > > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > > + * host should service the ring ASAP. */
> > > > > I don't think we have a reason to do this for packed ring.
> > > > > No historical baggage there, right?
> > > > Based on the original commit log, it seems that the notify here
> > > > is just an "optimization". But I don't quite understand what does
> > > > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > > > this in packed ring, I'd like to do it.
> > >
> > > According to the commit log, it seems like a workaround of lguest networking
> > > backend. I agree to drop it, we should not have such burden.
> > >
> > > But we should notice that, with this removed, the compare between packed vs
> > > split is kind of unfair.
> > I don't think this ever triggers to be frank. When would it?
>
>
> I think it can happen e.g in the path of XDP transmission in
> __virtnet_xdp_xmit_one():
>
>
> err = virtqueue_add_outbuf(sq->vq, sq->sg, 1, xdpf, GFP_ATOMIC);
> if (unlikely(err))
> return -ENOSPC; /* Caller handle free/refcnt */
>
I see. We used to do it for regular xmit but stopped
doing it. Is it fine for xdp then?
> >
> > > Consider the removal of lguest support recently,
> > > maybe we can drop this for split ring as well?
> > >
> > > Thanks
> > If it's helpful, then for sure we can drop it for virtio 1.
> > Can you see any perf differences at all? With which device?
>
>
> I don't test but consider the case of XDP_TX in guest plus vhost_net in
> host. Since vhost_net is half duplex, it's pretty easier to trigger this
> condition.
>
> Thanks
Sounds reasonable. Worth testing before we change things though.
>
> >
> > > > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > > > Author: Rusty Russell<rusty@rustcorp.com.au>
> > > > Date: Fri Jul 25 12:06:04 2008 -0500
> > > >
> > > > virtio: don't always force a notification when ring is full
> > > > We force notification when the ring is full, even if the host has
> > > > indicated it doesn't want to know. This seemed like a good idea at
> > > > the time: if we fill the transmit ring, we should tell the host
> > > > immediately.
> > > > Unfortunately this logic also applies to the receiving ring, which is
> > > > refilled constantly. We should introduce real notification thesholds
> > > > to replace this logic. Meanwhile, removing the logic altogether breaks
> > > > the heuristics which KVM uses, so we use a hack: only notify if there are
> > > > outgoing parts of the new buffer.
> > > > Here are the number of exits with lguest's crappy network implementation:
> > > > Before:
> > > > network xmit 7859051 recv 236420
> > > > After:
> > > > network xmit 7858610 recv 118136
> > > > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> > > >
> > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > index 72bf8bc09014..21d9a62767af 100644
> > > > --- a/drivers/virtio/virtio_ring.c
> > > > +++ b/drivers/virtio/virtio_ring.c
> > > > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > > > if (vq->num_free < out + in) {
> > > > pr_debug("Can't add buf len %i - avail = %i\n",
> > > > out + in, vq->num_free);
> > > > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > > > - vq->notify(&vq->vq);
> > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > + * there are outgoing parts to the buffer. Presumably the
> > > > + * host should service the ring ASAP. */
> > > > + if (out)
> > > > + vq->notify(&vq->vq);
> > > > END_USE(vq);
> > > > return -ENOSPC;
> > > > }
> > > >
> > > >
^ permalink raw reply
* Re: [PATCH net-next v2 3/5] virtio_ring: add packed ring support
From: Michael S. Tsirkin @ 2018-11-09 4:00 UTC (permalink / raw)
To: Jason Wang
Cc: Tiwei Bie, virtualization, linux-kernel, netdev, virtio-dev, wexu,
jfreimann
In-Reply-To: <67bd6a88-00f2-ed13-ad13-bdfe92ceeffc@redhat.com>
On Fri, Nov 09, 2018 at 10:30:50AM +0800, Jason Wang wrote:
>
> On 2018/11/8 下午11:56, Michael S. Tsirkin wrote:
> > On Thu, Nov 08, 2018 at 07:51:48PM +0800, Tiwei Bie wrote:
> > > On Thu, Nov 08, 2018 at 04:18:25PM +0800, Jason Wang wrote:
> > > > On 2018/11/8 上午9:38, Tiwei Bie wrote:
> > > > > > > +
> > > > > > > + if (vq->vq.num_free < descs_used) {
> > > > > > > + pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > > > + descs_used, vq->vq.num_free);
> > > > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > > > + * host should service the ring ASAP. */
> > > > > > I don't think we have a reason to do this for packed ring.
> > > > > > No historical baggage there, right?
> > > > > Based on the original commit log, it seems that the notify here
> > > > > is just an "optimization". But I don't quite understand what does
> > > > > the "the heuristics which KVM uses" refer to. If it's safe to drop
> > > > > this in packed ring, I'd like to do it.
> > > >
> > > > According to the commit log, it seems like a workaround of lguest networking
> > > > backend.
> > > Do you know why removing this notify in Tx will break "the
> > > heuristics which KVM uses"? Or what does "the heuristics
> > > which KVM uses" refer to?
> > Yes. QEMU has a mode where it disables notifications and processes TX
> > ring periodically from a timer. It's off by default but used to be on
> > by default a long time ago. If ring becomes full this causes traffic
> > stalls.
>
>
> Do you mean tx-timer? If yes, we can still enable it for packed ring
Yes we can but I doubt anyone does.
> and the
> timer will finally fired and we can go.
on tx ring full we probably don't want to wait for timer.
But I think we can just prevent qemu from using tx timer
with virtio 1.
>
> > As a work-around Rusty put in this hack to kick on ring full
> > even with notifications disabled.
>
>
> From the commit log it looks more like a performance workaround instead of a
> bug fix.
it's a quality of implementation issue, yes.
>
> > It's easy enough to make sure QEMU
> > does not combine devices with packed ring support with the timer hack.
> > And I am guessing it's safe enough to also block that option completely
> > e.g. when virtio 1.0 is enabled.
>
>
> I agree.
>
> Thanks
>
>
> > > > I agree to drop it, we should not have such burden.
> > > >
> > > > But we should notice that, with this removed, the compare between packed vs
> > > > split is kind of unfair. Consider the removal of lguest support recently,
> > > > maybe we can drop this for split ring as well?
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > commit 44653eae1407f79dff6f52fcf594ae84cb165ec4
> > > > > Author: Rusty Russell<rusty@rustcorp.com.au>
> > > > > Date: Fri Jul 25 12:06:04 2008 -0500
> > > > >
> > > > > virtio: don't always force a notification when ring is full
> > > > > We force notification when the ring is full, even if the host has
> > > > > indicated it doesn't want to know. This seemed like a good idea at
> > > > > the time: if we fill the transmit ring, we should tell the host
> > > > > immediately.
> > > > > Unfortunately this logic also applies to the receiving ring, which is
> > > > > refilled constantly. We should introduce real notification thesholds
> > > > > to replace this logic. Meanwhile, removing the logic altogether breaks
> > > > > the heuristics which KVM uses, so we use a hack: only notify if there are
> > > > > outgoing parts of the new buffer.
> > > > > Here are the number of exits with lguest's crappy network implementation:
> > > > > Before:
> > > > > network xmit 7859051 recv 236420
> > > > > After:
> > > > > network xmit 7858610 recv 118136
> > > > > Signed-off-by: Rusty Russell<rusty@rustcorp.com.au>
> > > > >
> > > > > diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
> > > > > index 72bf8bc09014..21d9a62767af 100644
> > > > > --- a/drivers/virtio/virtio_ring.c
> > > > > +++ b/drivers/virtio/virtio_ring.c
> > > > > @@ -87,8 +87,11 @@ static int vring_add_buf(struct virtqueue *_vq,
> > > > > if (vq->num_free < out + in) {
> > > > > pr_debug("Can't add buf len %i - avail = %i\n",
> > > > > out + in, vq->num_free);
> > > > > - /* We notify*even if* VRING_USED_F_NO_NOTIFY is set here. */
> > > > > - vq->notify(&vq->vq);
> > > > > + /* FIXME: for historical reasons, we force a notify here if
> > > > > + * there are outgoing parts to the buffer. Presumably the
> > > > > + * host should service the ring ASAP. */
> > > > > + if (out)
> > > > > + vq->notify(&vq->vq);
> > > > > END_USE(vq);
> > > > > return -ENOSPC;
> > > > > }
> > > > >
> > > > >
^ permalink raw reply
* Re: [PATCH net-next 1/2] dpaa2-eth: defer probe on object allocate
From: Andrew Lunn @ 2018-11-08 18:25 UTC (permalink / raw)
To: Ioana Ciornei
Cc: netdev@vger.kernel.org, davem@davemloft.net,
Ioana Ciocoi Radulescu
In-Reply-To: <1541683054-22273-2-git-send-email-ioana.ciornei@nxp.com>
On Thu, Nov 08, 2018 at 01:17:47PM +0000, Ioana Ciornei wrote:
> The fsl_mc_object_allocate function can fail because not all allocatable
> objects are probed by the fsl_mc_allocator at the call time. Defer the
> dpaa2-eth probe when this happens.
>
> Signed-off-by: Ioana Ciornei <ioana.ciornei@nxp.com>
> ---
> drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c | 30 +++++++++++++++++-------
> 1 file changed, 21 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> index 88f7acc..71f5cd4 100644
> --- a/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> +++ b/drivers/net/ethernet/freescale/dpaa2/dpaa2-eth.c
> @@ -1434,8 +1434,11 @@ static struct fsl_mc_device *setup_dpcon(struct dpaa2_eth_priv *priv)
> err = fsl_mc_object_allocate(to_fsl_mc_device(dev),
> FSL_MC_POOL_DPCON, &dpcon);
> if (err) {
> - dev_info(dev, "Not enough DPCONs, will go on as-is\n");
> - return NULL;
> + if (err == -ENXIO)
> + err = -EPROBE_DEFER;
> + else
> + dev_info(dev, "Not enough DPCONs, will go on as-is\n");
> + return ERR_PTR(err);
> }
>
> err = dpcon_open(priv->mc_io, 0, dpcon->obj_desc.id, &dpcon->mc_handle);
> @@ -1493,8 +1496,10 @@ static void free_dpcon(struct dpaa2_eth_priv *priv,
> return NULL;
>
> channel->dpcon = setup_dpcon(priv);
> - if (!channel->dpcon)
> + if (IS_ERR_OR_NULL(channel->dpcon)) {
> + err = PTR_ERR(channel->dpcon);
> goto err_setup;
> + }
Hi Ioana
You need to be careful with IS_ERR_OR_NULL(). If it is a NULL,
PTR_ERR() is going to return 0. You then jump to the error cleanup
code, but return 0, meaning everything is O.K.
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