* Re: [PATCH net-next v6 06/15] ethtool: netlink bitset handling
From: Johannes Berg @ 2019-07-03 13:44 UTC (permalink / raw)
To: Jiri Pirko, Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, linux-kernel
In-Reply-To: <20190703114933.GW2250@nanopsycho>
On Wed, 2019-07-03 at 13:49 +0200, Jiri Pirko wrote:
>
> > +Value and mask must have length at least ETHTOOL_A_BITSET_SIZE bits rounded up
> > +to a multiple of 32 bits. They consist of 32-bit words in host byte order,
>
> Looks like the blocks are similar to NLA_BITFIELD32. Why don't you user
> nested array of NLA_BITFIELD32 instead?
That would seem kind of awkward to use, IMHO.
Perhaps better to make some kind of generic "arbitrary size bitfield"
attribute type?
Not really sure we want the complexity with _LIST and _SIZE, since you
should always be able to express it as _VALUE and _MASK, right?
Trying to think how we should express this best - bitfield32 is just a
mask/value struct, for arbitrary size I guess we *could* just make it
kind of a binary with arbitrary length that must be a multiple of 2
bytes (or 2 u32-bit-words?) and then the first half is the value and the
second half is the mask? Some more validation would be nicer, but having
a generic attribute that actually is nested is awkward too.
johannes
^ permalink raw reply
* Re: [PATCH net-next v6 08/15] ethtool: move string arrays into common file
From: Jiri Pirko @ 2019-07-03 13:44 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <0647ac484dac2c655d0e4260d81e86405688ff5b.1562067622.git.mkubecek@suse.cz>
Tue, Jul 02, 2019 at 01:50:19PM CEST, mkubecek@suse.cz wrote:
>Introduce file net/ethtool/common.c for code shared by ioctl and netlink
>ethtool interface. Move name tables of features, RSS hash functions,
>tunables and PHY tunables into this file.
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>---
> net/ethtool/Makefile | 2 +-
> net/ethtool/common.c | 84 ++++++++++++++++++++++++++++++++++++++++++++
> net/ethtool/common.h | 17 +++++++++
> net/ethtool/ioctl.c | 83 ++-----------------------------------------
> 4 files changed, 104 insertions(+), 82 deletions(-)
> create mode 100644 net/ethtool/common.c
> create mode 100644 net/ethtool/common.h
>
>diff --git a/net/ethtool/Makefile b/net/ethtool/Makefile
>index 482fdb9380fa..11782306593b 100644
>--- a/net/ethtool/Makefile
>+++ b/net/ethtool/Makefile
>@@ -1,6 +1,6 @@
> # SPDX-License-Identifier: GPL-2.0
>
>-obj-y += ioctl.o
>+obj-y += ioctl.o common.o
>
> obj-$(CONFIG_ETHTOOL_NETLINK) += ethtool_nl.o
>
>diff --git a/net/ethtool/common.c b/net/ethtool/common.c
>new file mode 100644
>index 000000000000..b0ce420e994e
>--- /dev/null
>+++ b/net/ethtool/common.c
>@@ -0,0 +1,84 @@
>+// SPDX-License-Identifier: GPL-2.0 WITH Linux-syscall-note
>+
>+#include "common.h"
>+
>+const char netdev_features_strings[NETDEV_FEATURE_COUNT][ETH_GSTRING_LEN] = {
const char *netdev_features_strings[NETDEV_FEATURE_COUNT] = {
?
Same with the other arrays.
[...]
^ permalink raw reply
* Re: [PATCH net-next v6 07/15] ethtool: support for netlink notifications
From: Johannes Berg @ 2019-07-03 13:39 UTC (permalink / raw)
To: Michal Kubecek, David Miller, netdev
Cc: Jakub Kicinski, Jiri Pirko, Andrew Lunn, Florian Fainelli,
John Linville, Stephen Hemminger, linux-kernel
In-Reply-To: <4dcac81783de8686edefa262a1db75f9e961b123.1562067622.git.mkubecek@suse.cz>
On Tue, 2019-07-02 at 13:50 +0200, Michal Kubecek wrote:
>
> +static bool ethnl_ok __read_mostly;
Not sure it makes a big difference, but it could probably be
__ro_after_init instead?
johannes
^ permalink raw reply
* Re: [PATCH net-next v6 07/15] ethtool: support for netlink notifications
From: Jiri Pirko @ 2019-07-03 13:33 UTC (permalink / raw)
To: Michal Kubecek
Cc: David Miller, netdev, Jakub Kicinski, Andrew Lunn,
Florian Fainelli, John Linville, Stephen Hemminger, Johannes Berg,
linux-kernel
In-Reply-To: <4dcac81783de8686edefa262a1db75f9e961b123.1562067622.git.mkubecek@suse.cz>
Tue, Jul 02, 2019 at 01:50:14PM CEST, mkubecek@suse.cz wrote:
>Add infrastructure for ethtool netlink notifications. There is only one
>multicast group "monitor" which is used to notify userspace about changes
>and actions performed. Notification messages (types using suffix _NTF)
>share the format with replies to GET requests.
>
>Notifications are supposed to be broadcasted on every configuration change,
>whether it is done using the netlink interface or ioctl one. Netlink SET
>requests only trigger a notification if some data is actually changed.
>
>To trigger an ethtool notification, both ethtool netlink and external code
>use ethtool_notify() helper. This helper requires RTNL to be held and may
>sleep. Handlers sending messages for specific notification message types
>are registered in ethnl_notify_handlers array. As notifications can be
>triggered from other code, ethnl_ok flag is used to prevent an attempt to
>send notification before genetlink family is registered.
>
>Signed-off-by: Michal Kubecek <mkubecek@suse.cz>
>---
> include/linux/ethtool_netlink.h | 5 ++++
> include/linux/netdevice.h | 12 ++++++++++
> include/uapi/linux/ethtool_netlink.h | 2 ++
> net/ethtool/netlink.c | 35 ++++++++++++++++++++++++++++
> 4 files changed, 54 insertions(+)
>
>diff --git a/include/linux/ethtool_netlink.h b/include/linux/ethtool_netlink.h
>index 0412adb4f42f..2a15e64a16f3 100644
>--- a/include/linux/ethtool_netlink.h
>+++ b/include/linux/ethtool_netlink.h
>@@ -5,5 +5,10 @@
>
> #include <uapi/linux/ethtool_netlink.h>
> #include <linux/ethtool.h>
>+#include <linux/netdevice.h>
>+
>+enum ethtool_multicast_groups {
>+ ETHNL_MCGRP_MONITOR,
>+};
>
> #endif /* _LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
>index 88292953aa6f..c57d9917fd50 100644
>--- a/include/linux/netdevice.h
>+++ b/include/linux/netdevice.h
>@@ -4350,6 +4350,18 @@ struct netdev_notifier_bonding_info {
> void netdev_bonding_info_change(struct net_device *dev,
> struct netdev_bonding_info *bonding_info);
>
>+#if IS_ENABLED(CONFIG_ETHTOOL_NETLINK)
>+void ethtool_notify(struct net_device *dev, struct netlink_ext_ack *extack,
>+ unsigned int cmd, u32 req_mask, const void *data);
>+#else
>+static inline void ethtool_notify(struct net_device *dev,
>+ struct netlink_ext_ack *extack,
>+ unsigned int cmd, u32 req_mask,
>+ const void *data)
>+{
>+}
>+#endif
>+
> static inline
> struct sk_buff *skb_gso_segment(struct sk_buff *skb, netdev_features_t features)
> {
>diff --git a/include/uapi/linux/ethtool_netlink.h b/include/uapi/linux/ethtool_netlink.h
>index 805f314f4454..8938a1f09057 100644
>--- a/include/uapi/linux/ethtool_netlink.h
>+++ b/include/uapi/linux/ethtool_netlink.h
>@@ -91,4 +91,6 @@ enum {
> #define ETHTOOL_GENL_NAME "ethtool"
> #define ETHTOOL_GENL_VERSION 1
>
>+#define ETHTOOL_MCGRP_MONITOR_NAME "monitor"
>+
> #endif /* _UAPI_LINUX_ETHTOOL_NETLINK_H_ */
>diff --git a/net/ethtool/netlink.c b/net/ethtool/netlink.c
>index e13f29bbd625..a7a0bfe1818c 100644
>--- a/net/ethtool/netlink.c
>+++ b/net/ethtool/netlink.c
>@@ -6,6 +6,8 @@
>
> static struct genl_family ethtool_genl_family;
>
>+static bool ethnl_ok __read_mostly;
>+
> static const struct nla_policy dflt_header_policy[ETHTOOL_A_HEADER_MAX + 1] = {
> [ETHTOOL_A_HEADER_UNSPEC] = { .type = NLA_REJECT },
> [ETHTOOL_A_HEADER_DEV_INDEX] = { .type = NLA_U32 },
>@@ -176,11 +178,41 @@ struct sk_buff *ethnl_reply_init(size_t payload, struct net_device *dev, u8 cmd,
> return NULL;
> }
>
>+/* notifications */
>+
>+typedef void (*ethnl_notify_handler_t)(struct net_device *dev,
>+ struct netlink_ext_ack *extack,
>+ unsigned int cmd, u32 req_mask,
>+ const void *data);
>+
>+static const ethnl_notify_handler_t ethnl_notify_handlers[] = {
>+};
>+
>+void ethtool_notify(struct net_device *dev, struct netlink_ext_ack *extack,
>+ unsigned int cmd, u32 req_mask, const void *data)
What's "req_mask" ?
>+{
>+ if (unlikely(!ethnl_ok))
>+ return;
>+ ASSERT_RTNL();
>+
>+ if (likely(cmd < ARRAY_SIZE(ethnl_notify_handlers) &&
>+ ethnl_notify_handlers[cmd]))
How it could be null?
>+ ethnl_notify_handlers[cmd](dev, extack, cmd, req_mask, data);
>+ else
>+ WARN_ONCE(1, "notification %u not implemented (dev=%s, req_mask=0x%x)\n",
>+ cmd, netdev_name(dev), req_mask);
>+}
>+EXPORT_SYMBOL(ethtool_notify);
>+
> /* genetlink setup */
>
> static const struct genl_ops ethtool_genl_ops[] = {
> };
>
>+static const struct genl_multicast_group ethtool_nl_mcgrps[] = {
>+ [ETHNL_MCGRP_MONITOR] = { .name = ETHTOOL_MCGRP_MONITOR_NAME },
>+};
>+
> static struct genl_family ethtool_genl_family = {
> .name = ETHTOOL_GENL_NAME,
> .version = ETHTOOL_GENL_VERSION,
>@@ -188,6 +220,8 @@ static struct genl_family ethtool_genl_family = {
> .parallel_ops = true,
> .ops = ethtool_genl_ops,
> .n_ops = ARRAY_SIZE(ethtool_genl_ops),
>+ .mcgrps = ethtool_nl_mcgrps,
>+ .n_mcgrps = ARRAY_SIZE(ethtool_nl_mcgrps),
> };
>
> /* module setup */
>@@ -199,6 +233,7 @@ static int __init ethnl_init(void)
> ret = genl_register_family(ðtool_genl_family);
> if (WARN(ret < 0, "ethtool: genetlink family registration failed"))
> return ret;
>+ ethnl_ok = true;
>
> return 0;
> }
>--
>2.22.0
>
^ permalink raw reply
* [PATCH v2 bpf-next 1/4] selftests/bpf: compile progs with -D__TARGET_ARCH_$(ARCH)
From: Ilya Leoshkevich @ 2019-07-03 13:27 UTC (permalink / raw)
To: bpf, netdev, ys114321, daniel; +Cc: Ilya Leoshkevich
In-Reply-To: <20190703132711.57169-1-iii@linux.ibm.com>
This opens up the possibility of accessing registers in an
arch-independent way.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tools/testing/selftests/bpf/Makefile | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index de1754a8f5fe..3bba232e4d31 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -1,4 +1,5 @@
# SPDX-License-Identifier: GPL-2.0
+include ../../../scripts/Makefile.arch
LIBDIR := ../../../lib
BPFDIR := $(LIBDIR)/bpf
@@ -137,7 +138,8 @@ CLANG_SYS_INCLUDES := $(shell $(CLANG) -v -E - </dev/null 2>&1 \
CLANG_FLAGS = -I. -I./include/uapi -I../../../include/uapi \
$(CLANG_SYS_INCLUDES) \
- -Wno-compare-distinct-pointer-types
+ -Wno-compare-distinct-pointer-types \
+ -D__TARGET_ARCH_$(ARCH)
$(OUTPUT)/test_l4lb_noinline.o: CLANG_FLAGS += -fno-inline
$(OUTPUT)/test_xdp_noinline.o: CLANG_FLAGS += -fno-inline
--
2.21.0
^ permalink raw reply related
* [PATCH v2 bpf-next 4/4] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Ilya Leoshkevich @ 2019-07-03 13:27 UTC (permalink / raw)
To: bpf, netdev, ys114321, daniel; +Cc: Ilya Leoshkevich
In-Reply-To: <20190703132711.57169-1-iii@linux.ibm.com>
Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tools/testing/selftests/bpf/progs/loop1.c | 2 +-
tools/testing/selftests/bpf/progs/loop2.c | 2 +-
tools/testing/selftests/bpf/progs/loop3.c | 2 +-
3 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/tools/testing/selftests/bpf/progs/loop1.c b/tools/testing/selftests/bpf/progs/loop1.c
index dea395af9ea9..7cdb7f878310 100644
--- a/tools/testing/selftests/bpf/progs/loop1.c
+++ b/tools/testing/selftests/bpf/progs/loop1.c
@@ -18,7 +18,7 @@ int nested_loops(volatile struct pt_regs* ctx)
for (j = 0; j < 300; j++)
for (i = 0; i < j; i++) {
if (j & 1)
- m = ctx->rax;
+ m = PT_REGS_RC(ctx);
else
m = j;
sum += i * m;
diff --git a/tools/testing/selftests/bpf/progs/loop2.c b/tools/testing/selftests/bpf/progs/loop2.c
index 0637bd8e8bcf..9b2f808a2863 100644
--- a/tools/testing/selftests/bpf/progs/loop2.c
+++ b/tools/testing/selftests/bpf/progs/loop2.c
@@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
int i = 0;
while (true) {
- if (ctx->rax & 1)
+ if (PT_REGS_RC(ctx) & 1)
i += 3;
else
i += 7;
diff --git a/tools/testing/selftests/bpf/progs/loop3.c b/tools/testing/selftests/bpf/progs/loop3.c
index 30a0f6cba080..d727657d51e2 100644
--- a/tools/testing/selftests/bpf/progs/loop3.c
+++ b/tools/testing/selftests/bpf/progs/loop3.c
@@ -16,7 +16,7 @@ int while_true(volatile struct pt_regs* ctx)
__u64 i = 0, sum = 0;
do {
i++;
- sum += ctx->rax;
+ sum += PT_REGS_RC(ctx);
} while (i < 0x100000000ULL);
return sum;
}
--
2.21.0
^ permalink raw reply related
* [PATCH v2 bpf-next 3/4] selftests/bpf: make PT_REGS_* work in userspace
From: Ilya Leoshkevich @ 2019-07-03 13:27 UTC (permalink / raw)
To: bpf, netdev, ys114321, daniel; +Cc: Ilya Leoshkevich
In-Reply-To: <20190703132711.57169-1-iii@linux.ibm.com>
Right now, at least on s390 and x86_64, these macros are usable only
with kernel headers. This patch makes it possible to use them with
userspace headers and, as a consequence, in BPF selftests.
On s390, provide the forward declaration of struct pt_regs and cast it
to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
of the full struct pt_regs, s390 exposes only its first member
user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
(in selftests) and kernel (in samples) headers. It was added in commit
466698e654e8 ("s390/bpf: correct broken uapi for
BPF_PROG_TYPE_PERF_EVENT program type").
On x86, provide userspace versions of PT_REGS_* macros. Unlike s390, x86
provides struct pt_regs to both userspace and kernel, however, with
different member names.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tools/testing/selftests/bpf/bpf_helpers.h | 36 ++++++++++++++++-------
1 file changed, 26 insertions(+), 10 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 622dc4af0c65..faf86d83301a 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -355,6 +355,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
#if defined(bpf_target_x86)
+#ifdef __KERNEL__
#define PT_REGS_PARM1(x) ((x)->di)
#define PT_REGS_PARM2(x) ((x)->si)
#define PT_REGS_PARM3(x) ((x)->dx)
@@ -365,19 +366,34 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
#define PT_REGS_RC(x) ((x)->ax)
#define PT_REGS_SP(x) ((x)->sp)
#define PT_REGS_IP(x) ((x)->ip)
+#else
+#define PT_REGS_PARM1(x) ((x)->rdi)
+#define PT_REGS_PARM2(x) ((x)->rsi)
+#define PT_REGS_PARM3(x) ((x)->rdx)
+#define PT_REGS_PARM4(x) ((x)->rcx)
+#define PT_REGS_PARM5(x) ((x)->r8)
+#define PT_REGS_RET(x) ((x)->rsp)
+#define PT_REGS_FP(x) ((x)->rbp)
+#define PT_REGS_RC(x) ((x)->rax)
+#define PT_REGS_SP(x) ((x)->rsp)
+#define PT_REGS_IP(x) ((x)->rip)
+#endif
#elif defined(bpf_target_s390)
-#define PT_REGS_PARM1(x) ((x)->gprs[2])
-#define PT_REGS_PARM2(x) ((x)->gprs[3])
-#define PT_REGS_PARM3(x) ((x)->gprs[4])
-#define PT_REGS_PARM4(x) ((x)->gprs[5])
-#define PT_REGS_PARM5(x) ((x)->gprs[6])
-#define PT_REGS_RET(x) ((x)->gprs[14])
-#define PT_REGS_FP(x) ((x)->gprs[11]) /* Works only with CONFIG_FRAME_POINTER */
-#define PT_REGS_RC(x) ((x)->gprs[2])
-#define PT_REGS_SP(x) ((x)->gprs[15])
-#define PT_REGS_IP(x) ((x)->psw.addr)
+/* s390 provides user_pt_regs instead of struct pt_regs to userspace */
+struct pt_regs;
+#define PT_REGS_PARM1(x) (((const volatile user_pt_regs *)(x))->gprs[2])
+#define PT_REGS_PARM2(x) (((const volatile user_pt_regs *)(x))->gprs[3])
+#define PT_REGS_PARM3(x) (((const volatile user_pt_regs *)(x))->gprs[4])
+#define PT_REGS_PARM4(x) (((const volatile user_pt_regs *)(x))->gprs[5])
+#define PT_REGS_PARM5(x) (((const volatile user_pt_regs *)(x))->gprs[6])
+#define PT_REGS_RET(x) (((const volatile user_pt_regs *)(x))->gprs[14])
+/* Works only with CONFIG_FRAME_POINTER */
+#define PT_REGS_FP(x) (((const volatile user_pt_regs *)(x))->gprs[11])
+#define PT_REGS_RC(x) (((const volatile user_pt_regs *)(x))->gprs[2])
+#define PT_REGS_SP(x) (((const volatile user_pt_regs *)(x))->gprs[15])
+#define PT_REGS_IP(x) (((const volatile user_pt_regs *)(x))->psw.addr)
#elif defined(bpf_target_arm)
--
2.21.0
^ permalink raw reply related
* [PATCH v2 bpf-next 2/4] selftests/bpf: fix s390 -> s390 typo
From: Ilya Leoshkevich @ 2019-07-03 13:27 UTC (permalink / raw)
To: bpf, netdev, ys114321, daniel; +Cc: Ilya Leoshkevich
In-Reply-To: <20190703132711.57169-1-iii@linux.ibm.com>
Also check for __s390__ instead of __s390x__, just in case bpf_helpers.h
is ever used by 32-bit userspace.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
---
tools/testing/selftests/bpf/bpf_helpers.h | 10 +++++-----
1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/bpf/bpf_helpers.h b/tools/testing/selftests/bpf/bpf_helpers.h
index 1a5b1accf091..622dc4af0c65 100644
--- a/tools/testing/selftests/bpf/bpf_helpers.h
+++ b/tools/testing/selftests/bpf/bpf_helpers.h
@@ -312,8 +312,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
#if defined(__TARGET_ARCH_x86)
#define bpf_target_x86
#define bpf_target_defined
-#elif defined(__TARGET_ARCH_s930x)
- #define bpf_target_s930x
+#elif defined(__TARGET_ARCH_s390)
+ #define bpf_target_s390
#define bpf_target_defined
#elif defined(__TARGET_ARCH_arm)
#define bpf_target_arm
@@ -338,8 +338,8 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
#ifndef bpf_target_defined
#if defined(__x86_64__)
#define bpf_target_x86
-#elif defined(__s390x__)
- #define bpf_target_s930x
+#elif defined(__s390__)
+ #define bpf_target_s390
#elif defined(__arm__)
#define bpf_target_arm
#elif defined(__aarch64__)
@@ -366,7 +366,7 @@ static int (*bpf_skb_adjust_room)(void *ctx, __s32 len_diff, __u32 mode,
#define PT_REGS_SP(x) ((x)->sp)
#define PT_REGS_IP(x) ((x)->ip)
-#elif defined(bpf_target_s390x)
+#elif defined(bpf_target_s390)
#define PT_REGS_PARM1(x) ((x)->gprs[2])
#define PT_REGS_PARM2(x) ((x)->gprs[3])
--
2.21.0
^ permalink raw reply related
* [PATCH v2 bpf-next 0/4] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Ilya Leoshkevich @ 2019-07-03 13:27 UTC (permalink / raw)
To: bpf, netdev, ys114321, daniel; +Cc: Ilya Leoshkevich
Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
This patch series consists of preparatory commits, which make it
possible to use PT_REGS_RC in BPF selftests, followed by the actual fix.
Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
^ permalink raw reply
* Re: [PATCH bpf v2] xdp: fix race on generic receive path
From: Magnus Karlsson @ 2019-07-03 13:20 UTC (permalink / raw)
To: Ilya Maximets
Cc: Network Development, linux-kernel, bpf, xdp-newbies,
David S. Miller, Björn Töpel, Magnus Karlsson,
Jonathan Lemon, Jakub Kicinski, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <20190703120916.19973-1-i.maximets@samsung.com>
On Wed, Jul 3, 2019 at 2:09 PM Ilya Maximets <i.maximets@samsung.com> wrote:
>
> Unlike driver mode, generic xdp receive could be triggered
> by different threads on different CPU cores at the same time
> leading to the fill and rx queue breakage. For example, this
> could happen while sending packets from two processes to the
> first interface of veth pair while the second part of it is
> open with AF_XDP socket.
>
> Need to take a lock for each generic receive to avoid race.
I measured the performance degradation of rxdrop on my local machine
and it went from 2.19 to 2.08, so roughly a 5% drop. I think we can
live with this in XDP_SKB mode. If we at some later point in time need
to boost performance in this mode, let us look at it then from a
broader perspective and find the most low hanging fruit.
Thanks Ilya for this fix.
Acked-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
> Signed-off-by: Ilya Maximets <i.maximets@samsung.com>
> ---
>
> Version 2:
> * spin_lock_irqsave --> spin_lock_bh.
>
> include/net/xdp_sock.h | 2 ++
> net/xdp/xsk.c | 31 ++++++++++++++++++++++---------
> 2 files changed, 24 insertions(+), 9 deletions(-)
>
> diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
> index d074b6d60f8a..ac3c047d058c 100644
> --- a/include/net/xdp_sock.h
> +++ b/include/net/xdp_sock.h
> @@ -67,6 +67,8 @@ struct xdp_sock {
> * in the SKB destructor callback.
> */
> spinlock_t tx_completion_lock;
> + /* Protects generic receive. */
> + spinlock_t rx_lock;
> u64 rx_dropped;
> };
>
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index a14e8864e4fa..5e0637db92ea 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -123,13 +123,17 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
> u64 addr;
> int err;
>
> - if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index)
> - return -EINVAL;
> + spin_lock_bh(&xs->rx_lock);
> +
> + if (xs->dev != xdp->rxq->dev || xs->queue_id != xdp->rxq->queue_index) {
> + err = -EINVAL;
> + goto out_unlock;
> + }
>
> if (!xskq_peek_addr(xs->umem->fq, &addr) ||
> len > xs->umem->chunk_size_nohr - XDP_PACKET_HEADROOM) {
> - xs->rx_dropped++;
> - return -ENOSPC;
> + err = -ENOSPC;
> + goto out_drop;
> }
>
> addr += xs->umem->headroom;
> @@ -138,13 +142,21 @@ int xsk_generic_rcv(struct xdp_sock *xs, struct xdp_buff *xdp)
> memcpy(buffer, xdp->data_meta, len + metalen);
> addr += metalen;
> err = xskq_produce_batch_desc(xs->rx, addr, len);
> - if (!err) {
> - xskq_discard_addr(xs->umem->fq);
> - xsk_flush(xs);
> - return 0;
> - }
> + if (err)
> + goto out_drop;
> +
> + xskq_discard_addr(xs->umem->fq);
> + xskq_produce_flush_desc(xs->rx);
>
> + spin_unlock_bh(&xs->rx_lock);
> +
> + xs->sk.sk_data_ready(&xs->sk);
> + return 0;
> +
> +out_drop:
> xs->rx_dropped++;
> +out_unlock:
> + spin_unlock_bh(&xs->rx_lock);
> return err;
> }
>
> @@ -765,6 +777,7 @@ static int xsk_create(struct net *net, struct socket *sock, int protocol,
>
> xs = xdp_sk(sk);
> mutex_init(&xs->mutex);
> + spin_lock_init(&xs->rx_lock);
> spin_lock_init(&xs->tx_completion_lock);
>
> mutex_lock(&net->xdp.lock);
> --
> 2.17.1
>
^ permalink raw reply
* [PATCH 16/30] net/wireless: Use kmemdup rather than duplicating its implementation
From: Fuqian Huang @ 2019-07-03 13:16 UTC (permalink / raw)
Cc: Kalle Valo, David S . Miller, Solomon Peachy, linux-wireless,
netdev, linux-kernel, Fuqian Huang
kmemdup is introduced to duplicate a region of memory in a neat way.
Rather than kmalloc/kzalloc + memset, which the programmer needs to
write the size twice (sometimes lead to mistakes), kmemdup improves
readability, leads to smaller code and also reduce the chances of mistakes.
Suggestion to use kmemdup rather than using kmalloc/kzalloc + memset.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/net/wireless/ath/ath6kl/wmi.c | 6 ++----
drivers/net/wireless/st/cw1200/queue.c | 3 +--
drivers/net/wireless/ti/wlcore/main.c | 3 +--
3 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/wmi.c b/drivers/net/wireless/ath/ath6kl/wmi.c
index 68854c45d0a4..7452a0f587fe 100644
--- a/drivers/net/wireless/ath/ath6kl/wmi.c
+++ b/drivers/net/wireless/ath/ath6kl/wmi.c
@@ -3643,7 +3643,7 @@ static int ath6kl_wmi_send_action_cmd(struct wmi *wmi, u8 if_idx, u32 id,
if (wait)
return -EINVAL; /* Offload for wait not supported */
- buf = kmalloc(data_len, GFP_KERNEL);
+ buf = kmemdup(data, data_len, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -3654,7 +3654,6 @@ static int ath6kl_wmi_send_action_cmd(struct wmi *wmi, u8 if_idx, u32 id,
}
kfree(wmi->last_mgmt_tx_frame);
- memcpy(buf, data, data_len);
wmi->last_mgmt_tx_frame = buf;
wmi->last_mgmt_tx_frame_len = data_len;
@@ -3682,7 +3681,7 @@ static int __ath6kl_wmi_send_mgmt_cmd(struct wmi *wmi, u8 if_idx, u32 id,
if (wait)
return -EINVAL; /* Offload for wait not supported */
- buf = kmalloc(data_len, GFP_KERNEL);
+ buf = kmemdup(data, data_len, GFP_KERNEL);
if (!buf)
return -ENOMEM;
@@ -3693,7 +3692,6 @@ static int __ath6kl_wmi_send_mgmt_cmd(struct wmi *wmi, u8 if_idx, u32 id,
}
kfree(wmi->last_mgmt_tx_frame);
- memcpy(buf, data, data_len);
wmi->last_mgmt_tx_frame = buf;
wmi->last_mgmt_tx_frame_len = data_len;
diff --git a/drivers/net/wireless/st/cw1200/queue.c b/drivers/net/wireless/st/cw1200/queue.c
index 14133eedb3b6..12952b1c29df 100644
--- a/drivers/net/wireless/st/cw1200/queue.c
+++ b/drivers/net/wireless/st/cw1200/queue.c
@@ -79,10 +79,9 @@ static void cw1200_queue_register_post_gc(struct list_head *gc_list,
struct cw1200_queue_item *item)
{
struct cw1200_queue_item *gc_item;
- gc_item = kmalloc(sizeof(struct cw1200_queue_item),
+ gc_item = kmemdup(item, sizeof(struct cw1200_queue_item),
GFP_ATOMIC);
BUG_ON(!gc_item);
- memcpy(gc_item, item, sizeof(struct cw1200_queue_item));
list_add_tail(&gc_item->head, gc_list);
}
diff --git a/drivers/net/wireless/ti/wlcore/main.c b/drivers/net/wireless/ti/wlcore/main.c
index c9a485ecee7b..297207856494 100644
--- a/drivers/net/wireless/ti/wlcore/main.c
+++ b/drivers/net/wireless/ti/wlcore/main.c
@@ -1434,7 +1434,7 @@ int wl1271_rx_filter_alloc_field(struct wl12xx_rx_filter *filter,
field = &filter->fields[filter->num_fields];
- field->pattern = kzalloc(len, GFP_KERNEL);
+ field->pattern = kmemdup(pattern, len, GFP_KERNEL);
if (!field->pattern) {
wl1271_warning("Failed to allocate RX filter pattern");
return -ENOMEM;
@@ -1445,7 +1445,6 @@ int wl1271_rx_filter_alloc_field(struct wl12xx_rx_filter *filter,
field->offset = cpu_to_le16(offset);
field->flags = flags;
field->len = len;
- memcpy(field->pattern, pattern, len);
return 0;
}
--
2.11.0
^ permalink raw reply related
* [PATCH 15/30] net/wimax: Use kmemdup rather than duplicating its implementation
From: Fuqian Huang @ 2019-07-03 13:16 UTC (permalink / raw)
Cc: Inaky Perez-Gonzalez, linux-wimax, David S . Miller, netdev,
linux-kernel, Fuqian Huang
kmemdup is introduced to duplicate a region of memory in a neat way.
Rather than kmalloc/kzalloc + memset, which the programmer needs to
write the size twice (sometimes lead to mistakes), kmemdup improves
readability, leads to smaller code and also reduce the chances of mistakes.
Suggestion to use kmemdup rather than using kmalloc/kzalloc + memset.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/net/wimax/i2400m/usb.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/wimax/i2400m/usb.c b/drivers/net/wimax/i2400m/usb.c
index 2075e7b1fff6..cdce6c47c444 100644
--- a/drivers/net/wimax/i2400m/usb.c
+++ b/drivers/net/wimax/i2400m/usb.c
@@ -155,12 +155,11 @@ int __i2400mu_send_barker(struct i2400mu *i2400mu,
do_autopm = 0;
}
ret = -ENOMEM;
- buffer = kmalloc(barker_size, GFP_KERNEL);
+ buffer = kmemdup(barker, barker_size, GFP_KERNEL);
if (buffer == NULL)
goto error_kzalloc;
epd = usb_get_epd(i2400mu->usb_iface, endpoint);
pipe = usb_sndbulkpipe(i2400mu->usb_dev, epd->bEndpointAddress);
- memcpy(buffer, barker, barker_size);
retry:
ret = usb_bulk_msg(i2400mu->usb_dev, pipe, buffer, barker_size,
&actual_len, 200);
--
2.11.0
^ permalink raw reply related
* [PATCH 14/30] net/ethernet: Use kmemdup rather than duplicating its implementation
From: Fuqian Huang @ 2019-07-03 13:15 UTC (permalink / raw)
Cc: Tariq Toukan, David S . Miller, netdev, linux-rdma, linux-kernel,
Fuqian Huang
kmemdup is introduced to duplicate a region of memory in a neat way.
Rather than kmalloc/kzalloc + memset, which the programmer needs to
write the size twice (sometimes lead to mistakes), kmemdup improves
readability, leads to smaller code and also reduce the chances of mistakes.
Suggestion to use kmemdup rather than using kmalloc/kzalloc + memset.
Signed-off-by: Fuqian Huang <huangfq.daxian@gmail.com>
---
drivers/net/ethernet/mellanox/mlx4/resource_tracker.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
index 4356f3a58002..e971a6bdf0d5 100644
--- a/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
+++ b/drivers/net/ethernet/mellanox/mlx4/resource_tracker.c
@@ -4437,14 +4437,13 @@ int mlx4_QP_FLOW_STEERING_ATTACH_wrapper(struct mlx4_dev *dev, int slave,
goto err_detach;
mbox_size = qp_attach_mbox_size(inbox->buf);
- rrule->mirr_mbox = kmalloc(mbox_size, GFP_KERNEL);
+ rrule->mirr_mbox = kmemdup(inbox->buf, mbox_size, GFP_KERNEL);
if (!rrule->mirr_mbox) {
err = -ENOMEM;
goto err_put_rule;
}
rrule->mirr_mbox_size = mbox_size;
rrule->mirr_rule_id = 0;
- memcpy(rrule->mirr_mbox, inbox->buf, mbox_size);
/* set different port */
ctrl = (struct mlx4_net_trans_rule_hw_ctrl *)rrule->mirr_mbox;
--
2.11.0
^ permalink raw reply related
* Re: KASAN: use-after-free Read in hci_cmd_timeout
From: syzbot @ 2019-07-03 13:12 UTC (permalink / raw)
To: davem, johan.hedberg, linux-bluetooth, linux-kernel, marcel,
netdev, syzkaller-bugs
In-Reply-To: <00000000000035c756058848954a@google.com>
syzbot has found a reproducer for the following crash on:
HEAD commit: eca94432 Bluetooth: Fix faulty expression for minimum encr..
git tree: upstream
console output: https://syzkaller.appspot.com/x/log.txt?x=1006cc8ba00000
kernel config: https://syzkaller.appspot.com/x/.config?x=f6451f0da3d42d53
dashboard link: https://syzkaller.appspot.com/bug?extid=19a9f729f05272857487
compiler: gcc (GCC) 9.0.0 20181231 (experimental)
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=125b7999a00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=176deefba00000
IMPORTANT: if you fix the bug, please add the following tag to the commit:
Reported-by: syzbot+19a9f729f05272857487@syzkaller.appspotmail.com
Bluetooth: hci0: command 0xfc11 tx timeout
==================================================================
BUG: KASAN: use-after-free in hci_cmd_timeout+0x1fe/0x220
net/bluetooth/hci_core.c:2614
Read of size 8 at addr ffff88809e8a3c48 by task kworker/0:5/9461
CPU: 0 PID: 9461 Comm: kworker/0:5 Not tainted 5.2.0-rc7+ #40
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS
Google 01/01/2011
Workqueue: events hci_cmd_timeout
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0x172/0x1f0 lib/dump_stack.c:113
print_address_description.cold+0x7c/0x20d mm/kasan/report.c:188
__kasan_report.cold+0x1b/0x40 mm/kasan/report.c:317
kasan_report+0x12/0x20 mm/kasan/common.c:614
__asan_report_load8_noabort+0x14/0x20 mm/kasan/generic_report.c:132
hci_cmd_timeout+0x1fe/0x220 net/bluetooth/hci_core.c:2614
process_one_work+0x989/0x1790 kernel/workqueue.c:2269
worker_thread+0x98/0xe40 kernel/workqueue.c:2415
kthread+0x354/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Allocated by task 9446:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
__kasan_kmalloc mm/kasan/common.c:489 [inline]
__kasan_kmalloc.constprop.0+0xcf/0xe0 mm/kasan/common.c:462
kasan_slab_alloc+0xf/0x20 mm/kasan/common.c:497
slab_post_alloc_hook mm/slab.h:437 [inline]
slab_alloc mm/slab.c:3326 [inline]
kmem_cache_alloc+0x11a/0x6f0 mm/slab.c:3488
skb_clone+0x154/0x3d0 net/core/skbuff.c:1321
hci_cmd_work+0xe0/0x2a0 net/bluetooth/hci_core.c:4495
process_one_work+0x989/0x1790 kernel/workqueue.c:2269
worker_thread+0x98/0xe40 kernel/workqueue.c:2415
kthread+0x354/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
Freed by task 1501:
save_stack+0x23/0x90 mm/kasan/common.c:71
set_track mm/kasan/common.c:79 [inline]
__kasan_slab_free+0x102/0x150 mm/kasan/common.c:451
kasan_slab_free+0xe/0x10 mm/kasan/common.c:459
__cache_free mm/slab.c:3432 [inline]
kmem_cache_free+0x86/0x260 mm/slab.c:3698
kfree_skbmem net/core/skbuff.c:620 [inline]
kfree_skbmem+0xc5/0x150 net/core/skbuff.c:614
__kfree_skb net/core/skbuff.c:677 [inline]
kfree_skb net/core/skbuff.c:694 [inline]
kfree_skb+0xf0/0x390 net/core/skbuff.c:688
hci_dev_do_open+0xb20/0x1760 net/bluetooth/hci_core.c:1550
hci_power_on+0x10d/0x580 net/bluetooth/hci_core.c:2171
process_one_work+0x989/0x1790 kernel/workqueue.c:2269
worker_thread+0x98/0xe40 kernel/workqueue.c:2415
kthread+0x354/0x420 kernel/kthread.c:255
ret_from_fork+0x24/0x30 arch/x86/entry/entry_64.S:352
The buggy address belongs to the object at ffff88809e8a3b80
which belongs to the cache skbuff_head_cache of size 224
The buggy address is located 200 bytes inside of
224-byte region [ffff88809e8a3b80, ffff88809e8a3c60)
The buggy address belongs to the page:
page:ffffea00027a28c0 refcount:1 mapcount:0 mapping:ffff88821baabb40
index:0x0
flags: 0x1fffc0000000200(slab)
raw: 01fffc0000000200 ffffea00027b2d08 ffffea00021b83c8 ffff88821baabb40
raw: 0000000000000000 ffff88809e8a3040 000000010000000c 0000000000000000
page dumped because: kasan: bad access detected
Memory state around the buggy address:
ffff88809e8a3b00: fb fb fb fb fc fc fc fc fc fc fc fc fc fc fc fc
ffff88809e8a3b80: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
> ffff88809e8a3c00: fb fb fb fb fb fb fb fb fb fb fb fb fc fc fc fc
^
ffff88809e8a3c80: fc fc fc fc fc fc fc fc fb fb fb fb fb fb fb fb
ffff88809e8a3d00: fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb fb
==================================================================
^ permalink raw reply
* i.mx6ul with DSA in multi chip addressing mode - no MDIO access
From: Benjamin Beckmeyer @ 2019-07-03 13:10 UTC (permalink / raw)
To: netdev
Hey folks,
I'm having a problem with a custom i.mx6ul board. When DSA is loaded I can't
get access to the switch via MDIO, but the DSA is working properly. I set up
a bridge for testing and the switch is in forwarding mode and i can ping the
board. But the MDIO access isn't working at address 2 for the switch. When I
delete the DSA from the devicetree and start the board up, I can access the
switch via MDIO.
With DSA up and running:
mii -i 2 0 0x9800
mii -i 2 1
phyid:2, reg:0x01 -> 0x4000
mii -i 2 0 0x9803
mii -i 2 1
phyid:2, reg:0x01 -> 0x4000
mii -i 2 1 0x1883
mii -i 2 1
phyid:2, reg:0x01 -> 0x4000
No DSA:
mii -i 2 0 0x9800
mii -i 2 1
phyid:2, reg:0x01 -> 0xde04
mii -i 2 0 0x9803
mii -i 2 1
phyid:2, reg:0x01 -> 0x3901
mii -i 2 1 0x1883
mii -i 2 1
phyid:2, reg:0x01 -> 0x1883
Here is the device tree for our board:
&mdio0 {
switch0: switch0@2 {
compatible = "marvell,mv88e6190";
reg = <2>;
pinctrl-0 = <&pinctrl_gpios>;
reset-gpios = <&gpio4 16 GPIO_ACTIVE_LOW>;
dsa,member = <0 0>;
ports {
#address-cells = <1>;
#size-cells = <0>;
port@0 {
reg = <0>;
label = "cpu";
ethernet = <&fec1>;
phy-mode = "rmii";
fixed-link {
speed = <100>;
full-duplex;
};
};
port@1 {
reg = <1>;
label = "lan1";
};
port@2 {
reg = <2>;
label = "lan2";
};
port@3 {
reg = <3>;
label = "lan3";
};
port@4 {
reg = <4>;
label = "lan4";
};
port@5 {
reg = <5>;
label = "lan5";
};
port@6 {
reg = <6>;
label = "lan6";
};
port@7 {
reg = <7>;
label = "lan7";
};
port@8 {
reg = <8>;
label = "lan8";
};
port@9 {
reg = <9>;
label = "serdes1";
fixed-link {
speed = <1000>;
full-duplex;
};
};
port@10 {
reg = <10>;
label = "serdes2";
fixed-link {
speed = <1000>;
full-duplex;
};
};
};
};
};
On a different custom board we have another switching chip in single chip
addressing mode the MDIO access works like a charm with activated DSA.
Currently I'm on linux-4.14.118. Other kernels (4.19.55, 5.1.14) I've
tested stuck at or reboot while DSA is loading. Same devicetree there.
Let me know if you need some more input.
Thanks in advance for your help.
Best regards,
Benjamin Beckmeyer
^ permalink raw reply
* Re: [RFC v2] vhost: introduce mdev based hardware vhost backend
From: Tiwei Bie @ 2019-07-03 13:08 UTC (permalink / raw)
To: Jason Wang
Cc: mst, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang
In-Reply-To: <64833f91-02cd-7143-f12e-56ab93b2418d@redhat.com>
On Wed, Jul 03, 2019 at 08:16:23PM +0800, Jason Wang wrote:
> On 2019/7/3 下午7:52, Tiwei Bie wrote:
> > On Wed, Jul 03, 2019 at 06:09:51PM +0800, Jason Wang wrote:
> > > On 2019/7/3 下午5:13, Tiwei Bie wrote:
> > > > Details about this can be found here:
> > > >
> > > > https://lwn.net/Articles/750770/
> > > >
> > > > What's new in this version
> > > > ==========================
> > > >
> > > > A new VFIO device type is introduced - vfio-vhost. This addressed
> > > > some comments from here: https://patchwork.ozlabs.org/cover/984763/
> > > >
> > > > Below is the updated device interface:
> > > >
> > > > Currently, there are two regions of this device: 1) CONFIG_REGION
> > > > (VFIO_VHOST_CONFIG_REGION_INDEX), which can be used to setup the
> > > > device; 2) NOTIFY_REGION (VFIO_VHOST_NOTIFY_REGION_INDEX), which
> > > > can be used to notify the device.
> > > >
> > > > 1. CONFIG_REGION
> > > >
> > > > The region described by CONFIG_REGION is the main control interface.
> > > > Messages will be written to or read from this region.
> > > >
> > > > The message type is determined by the `request` field in message
> > > > header. The message size is encoded in the message header too.
> > > > The message format looks like this:
> > > >
> > > > struct vhost_vfio_op {
> > > > __u64 request;
> > > > __u32 flags;
> > > > /* Flag values: */
> > > > #define VHOST_VFIO_NEED_REPLY 0x1 /* Whether need reply */
> > > > __u32 size;
> > > > union {
> > > > __u64 u64;
> > > > struct vhost_vring_state state;
> > > > struct vhost_vring_addr addr;
> > > > } payload;
> > > > };
> > > >
> > > > The existing vhost-kernel ioctl cmds are reused as the message
> > > > requests in above structure.
> > >
> > > Still a comments like V1. What's the advantage of inventing a new protocol?
> > I'm trying to make it work in VFIO's way..
> >
> > > I believe either of the following should be better:
> > >
> > > - using vhost ioctl, we can start from SET_VRING_KICK/SET_VRING_CALL and
> > > extend it with e.g notify region. The advantages is that all exist userspace
> > > program could be reused without modification (or minimal modification). And
> > > vhost API hides lots of details that is not necessary to be understood by
> > > application (e.g in the case of container).
> > Do you mean reusing vhost's ioctl on VFIO device fd directly,
> > or introducing another mdev driver (i.e. vhost_mdev instead of
> > using the existing vfio_mdev) for mdev device?
>
>
> Can we simply add them into ioctl of mdev_parent_ops?
Right, either way, these ioctls have to be and just need to be
added in the ioctl of the mdev_parent_ops. But another thing we
also need to consider is that which file descriptor the userspace
will do the ioctl() on. So I'm wondering do you mean let the
userspace do the ioctl() on the VFIO device fd of the mdev
device?
>
>
> >
[...]
> > > > 3. VFIO interrupt ioctl API
> > > >
> > > > VFIO interrupt ioctl API is used to setup device interrupts.
> > > > IRQ-bypass can also be supported.
> > > >
> > > > Currently, the data path interrupt can be configured via the
> > > > VFIO_VHOST_VQ_IRQ_INDEX with virtqueue's callfd.
> > >
> > > How about DMA API? Do you expect to use VFIO IOMMU API or using vhost
> > > SET_MEM_TABLE? VFIO IOMMU API is more generic for sure but with
> > > SET_MEM_TABLE DMA can be done at the level of parent device which means it
> > > can work for e.g the card with on-chip IOMMU.
> > Agree. In this RFC, it assumes userspace will use VFIO IOMMU API
> > to do the DMA programming. But like what you said, there could be
> > a problem when using cards with on-chip IOMMU.
>
>
> Yes, another issue is SET_MEM_TABLE can not be used to update just a part of
> the table. This seems less flexible than VFIO API but it could be extended.
Agree.
>
>
> >
> > > And what's the plan for vIOMMU?
> > As this RFC assumes userspace will use VFIO IOMMU API, userspace
> > just needs to follow the same way like what vfio-pci device does
> > in QEMU to support vIOMMU.
>
>
> Right, this is more a question for the qemu part. It means it needs to go
> for ordinary VFIO path to get all notifiers/listeners support from vIOMMU.
Yeah.
>
>
> >
> > >
> > > > Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> > > > ---
> > > > drivers/vhost/Makefile | 2 +
> > > > drivers/vhost/vdpa.c | 770 +++++++++++++++++++++++++++++++++++++
> > > > include/linux/vdpa_mdev.h | 72 ++++
> > > > include/uapi/linux/vfio.h | 19 +
> > > > include/uapi/linux/vhost.h | 25 ++
> > > > 5 files changed, 888 insertions(+)
> > > > create mode 100644 drivers/vhost/vdpa.c
> > > > create mode 100644 include/linux/vdpa_mdev.h
> > >
> > > We probably need some sample parent device implementation. It could be a
> > > software datapath like e.g we can start from virtio-net device in guest or a
> > > vhost/tap on host.
> > Yeah, something like this would be interesting!
>
>
> Plan to do something like that :) ?
I don't have a plan yet.. But it's something that can be done I think.
Thanks,
Tiwei
>
> Thanks
>
>
> >
> > Thanks,
> > Tiwei
> >
> > > Thanks
> > >
> > >
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU
From: Jes Sorensen @ 2019-07-03 13:01 UTC (permalink / raw)
To: Chris Chiu
Cc: Kalle Valo, David Miller, linux-wireless, netdev, Linux Kernel,
Linux Upstreaming Team
In-Reply-To: <CAB4CAwcEdcg91Bgb+JoCdk_zQKsWT-K+cb07-5mrrx+__X2RMA@mail.gmail.com>
On 7/2/19 11:25 PM, Chris Chiu wrote:
> On Tue, Jul 2, 2019 at 8:44 PM Jes Sorensen <jes.sorensen@gmail.com> wrote:
>>
>> On 6/27/19 5:52 AM, Chris Chiu wrote:
>>> The WiFi tx power of RTL8723BU is extremely low after booting. So
>>> the WiFi scan gives very limited AP list and it always fails to
>>> connect to the selected AP. This module only supports 1x1 antenna
>>> and the antenna is switched to bluetooth due to some incorrect
>>> register settings.
>>>
>>> This commit hand over the antenna control to PTA, the wifi signal
>>> will be back to normal and the bluetooth scan can also work at the
>>> same time. However, the btcoexist still needs to be handled under
>>> different circumstances. If there's a BT connection established,
>>> the wifi still fails to connect until disconneting the BT.
>>>
>>> Signed-off-by: Chris Chiu <chiu@endlessm.com>
>>> ---
>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 9 ++++++---
>>> drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c | 3 ++-
>>> 2 files changed, 8 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> index 3adb1d3d47ac..6c3c70d93ac1 100644
>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
>>> @@ -1525,7 +1525,7 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv)
>>> /*
>>> * WLAN action by PTA
>>> */
>>> - rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x04);
>>> + rtl8xxxu_write8(priv, REG_WLAN_ACT_CONTROL_8723B, 0x0c);
>>>
>>> /*
>>> * BT select S0/S1 controlled by WiFi
>>> @@ -1568,9 +1568,12 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv *priv)
>>> rtl8xxxu_gen2_h2c_cmd(priv, &h2c, sizeof(h2c.ant_sel_rsv));
>>>
>>> /*
>>> - * 0x280, 0x00, 0x200, 0x80 - not clear
>>> + * Different settings per different antenna position.
>>> + * Antenna switch to BT: 0x280, 0x00 (inverse)
>>> + * Antenna switch to WiFi: 0x0, 0x280 (inverse)
>>> + * Antenna controlled by PTA: 0x200, 0x80 (inverse)
>>> */
>>> - rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x00);
>>> + rtl8xxxu_write32(priv, REG_S0S1_PATH_SWITCH, 0x80);
>>>
>>> /*
>>> * Software control, antenna at WiFi side
>>> diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> index 8136e268b4e6..87b2179a769e 100644
>>> --- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> +++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_core.c
>>> @@ -3891,12 +3891,13 @@ static int rtl8xxxu_init_device(struct ieee80211_hw *hw)
>>>
>>> /* Check if MAC is already powered on */
>>> val8 = rtl8xxxu_read8(priv, REG_CR);
>>> + val16 = rtl8xxxu_read16(priv, REG_SYS_CLKR);
>>>
>>> /*
>>> * Fix 92DU-VC S3 hang with the reason is that secondary mac is not
>>> * initialized. First MAC returns 0xea, second MAC returns 0x00
>>> */
>>> - if (val8 == 0xea)
>>> + if (val8 == 0xea || !(val16 & BIT(11)))
>>> macpower = false;
>>> else
>>> macpower = true;
>>
>> This part I would like to ask you take a good look at the other chips to
>> make sure you don't break support for 8192cu, 8723au, 8188eu with this.
>>
>> Cheers,
>> Jes
>
> I checked the vendor code of 8192cu and 8188eu, they don't have this part
> of code to check the REG_CR before power on sequence. I can only find
> similar code in rtl8723be.
> if (tmp_u1b != 0 && tmp_u1b !=0xea)
> rtlhal->mac_func_enable = true;
>
> By definition, the BIT(11) of REG_SYS_CLKR in rtl8xxxu_regs.h is
> SYS_CLK_MAC_CLK_ENABLE. It seems to make sense to check this value
> for macpower no matter what chip it is. I think I can make it more
> self-expressive
> as down below.
>
> if (val8 == 0xea || !(val16 & SYS_CLK_MAC_CLK_ENABLE))
Yes, please always use the descriptive defines rather than hard coding
the bit numbers.
> And per the comment, this code is for 92DU-VC S3 hang problem and I think an
> OR check for SYS_CLK_MAC_CLK_ENABLE is still safe for this.
Sounds reasonable - keep in mind that some of these bugs may have been
fixed for one chip, and then just copied forward.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH] rtl8xxxu: Fix wifi low signal strength issue of RTL8723BU
From: Jes Sorensen @ 2019-07-03 12:59 UTC (permalink / raw)
To: Daniel Drake
Cc: Chris Chiu, Kalle Valo, David Miller, linux-wireless, netdev,
Linux Kernel, Linux Upstreaming Team, Larry Finger
In-Reply-To: <CAD8Lp47mWH1-VsZaHr6_qmSU2EEOr9tQJ3CUhfi_JkQGgKpegA@mail.gmail.com>
On 7/3/19 3:42 AM, Daniel Drake wrote:
> On Tue, Jul 2, 2019 at 8:42 PM Jes Sorensen <jes.sorensen@gmail.com> wrote:
>> We definitely don't want to bring over the vendor code, since it's a
>> pile of spaghetti, but we probably need to get something sorted. This
>> went down the drain when the bluetooth driver was added without taking
>> it into account - long after this driver was merged.
>
> Yeah, I didn't mean bring over quite so literally.. Chris is studying
> it and figuring out the neatest way to reimplement the required bits.
>
> As for the relationship with bluetooth.. actually the bug that Chris
> is working on here is that the rtl8xxxu wifi signal is totally
> unusable *until* the bluetooth driver is loaded.
So this is not my experience at all from when I wrote the code. The
8723bu dongle I used for it came up just fine.
> Once the bluetooth driver is loaded, at the point of bluetooth
> firmware upload, the rtl8xxxu signal magiaclly strength becomes good.
> I think this is consistent with other rtl8xxxu problem reports that we
> saw lying around, although they had not been diagnosed in so much
> detail.
See this is the very opposite of what I have experienced. The bluetooth
driver ruins the signal when it's loaded with my dongle.
> The rtl8723bu vendor driver does not suffer this problem, it works
> fine with or without the bluetooth driver in place.
My point is this seems to be very dongle dependent :( We have to be
careful not breaking it for some users while fixing it for others.
Cheers,
Jes
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: fix compiling loop{1,2,3}.c on s390
From: Daniel Borkmann @ 2019-07-03 12:56 UTC (permalink / raw)
To: Ilya Leoshkevich, bpf, netdev
In-Reply-To: <20190702153908.41562-1-iii@linux.ibm.com>
On 07/02/2019 05:39 PM, Ilya Leoshkevich wrote:
> Use PT_REGS_RC(ctx) instead of ctx->rax, which is not present on s390.
>
> Pass -D__TARGET_ARCH_$(ARCH) to selftests in order to choose a proper
> PT_REGS_RC variant.
>
> Fix s930 -> s390 typo.
>
> On s390, provide the forward declaration of struct pt_regs and cast it
> to user_pt_regs in PT_REGS_* macros. This is necessary, because instead
> of the full struct pt_regs, s390 exposes only its first field
> user_pt_regs to userspace, and bpf_helpers.h is used with both userspace
> (in selftests) and kernel (in samples) headers.
>
> On x86, provide userspace versions of PT_REGS_* macros. Unlike s390, x86
> provides struct pt_regs to both userspace and kernel, however, with
> different field names.
>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
This doesn't apply cleanly to bpf-next, please rebase. I also think this
should be ideally split into multiple patches, seems like 4 different
issues which you are addressing in this single patch.
Thanks,
Daniel
^ permalink raw reply
* RE: [EXT] Re: [PATCH net-next 4/4] qed: Add devlink support for configuration attributes.
From: Sudarsana Reddy Kalluru @ 2019-07-03 12:56 UTC (permalink / raw)
To: Jiri Pirko, Jakub Kicinski
Cc: davem@davemloft.net, netdev@vger.kernel.org, Michal Kalderon,
Ariel Elior
In-Reply-To: <20190620133748.GD2504@nanopsycho>
> -----Original Message-----
> From: Jiri Pirko <jiri@resnulli.us>
> Sent: Thursday, June 20, 2019 7:08 PM
> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>; davem@davemloft.net;
> netdev@vger.kernel.org; Michal Kalderon <mkalderon@marvell.com>; Ariel
> Elior <aelior@marvell.com>
> Subject: Re: [EXT] Re: [PATCH net-next 4/4] qed: Add devlink support for
> configuration attributes.
>
> Thu, Jun 20, 2019 at 02:09:29PM CEST, skalluru@marvell.com wrote:
> >> -----Original Message-----
> >> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> >> Sent: Tuesday, June 18, 2019 4:24 AM
> >> To: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> >> Cc: davem@davemloft.net; netdev@vger.kernel.org; Michal Kalderon
> >> <mkalderon@marvell.com>; Ariel Elior <aelior@marvell.com>; Jiri Pirko
> >> <jiri@resnulli.us>
> >> Subject: [EXT] Re: [PATCH net-next 4/4] qed: Add devlink support for
> >> configuration attributes.
> >>
> >> External Email
> >>
> >> ---------------------------------------------------------------------
> >> - On Mon, 17 Jun 2019 04:45:28 -0700, Sudarsana Reddy Kalluru wrote:
> >> > This patch adds implementation for devlink callbacks for reading/
> >> > configuring the device attributes.
> >> >
> >> > Signed-off-by: Sudarsana Reddy Kalluru <skalluru@marvell.com>
> >> > Signed-off-by: Ariel Elior <aelior@marvell.com>
> >>
> >> You need to provide documentation for your parameters, plus some of
> >> them look like they should potentially be port params, not device params.
> >
> >Thanks a lot for your review. Will add the required documentation. In case
> of Marvell adapter, any of the device/adapter/port parameters can be
> read/configurable via any PF (ethdev) on the port. Hence adding the
> commands at device level. Hope this is fine.
>
> No it is not. Port param should be port param.
>
> Also please be careful not to add any generic param as driver specific.
>
> Thanks!
Apologies for bringing this topic again. From the driver(s) code paths/'devlink man pages', I understood that devlink-port object is an entity on top of the PCI bus device.
Some drivers say NFP represents vnics (on pci-dev) as a devlink-ports and, some represents (virtual?) ports on the PF/device as devlink-ports.
In the case of Marvell NIC driver, we don't have [port] partitioning of the PCI device. And the config attributes are specific to PCI-device (not the vports/vnics of PF).
Hence I didn't see a need for creating devlink-port objects in the system for Marvell NICs. And planning to add the config attributes to 'devlink-dev' object.
Please let me know if my understanding and the proposal is ok?
Code paths which use devlink-port:
mlx4_load_one(struct pci_dev *pdev): mlx4_init_port_info(1 .. dev->caps.num_ports) -> devlink_port_register()
nfp_net_pf_init_vnics (1 .. pf->vnics) -> nfp_net_pf_init_vnic() -> nfp_devlink_port_register()
nsim_dev_probe (1 .. nsim_bus_dev->port_count) -> __nsim_dev_port_add() -> devlink_port_register ()
man page for 'devlink-port':
devlink port set - change devlink port attributes
DEV/PORT_INDEX - specifies the devlink port to operate on.
devlink port show pci/0000:01:00.0/1
Shows the state of specified devlink port.
^ permalink raw reply
* [PATCH bpf-next v3] libbpf: add xsk_ring_prod__nb_free() function
From: Eelco Chaudron @ 2019-07-03 12:52 UTC (permalink / raw)
To: netdev
Cc: ast, daniel, kafai, songliubraving, yhs, andrii.nakryiko,
magnus.karlsson
When an AF_XDP application received X packets, it does not mean X
frames can be stuffed into the producer ring. To make it easier for
AF_XDP applications this API allows them to check how many frames can
be added into the ring.
Signed-off-by: Eelco Chaudron <echaudro@redhat.com>
---
v2 -> v3
- Removed cache by pass option
v1 -> v2
- Renamed xsk_ring_prod__free() to xsk_ring_prod__nb_free()
- Add caching so it will only touch global state when needed
tools/lib/bpf/xsk.h | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/xsk.h b/tools/lib/bpf/xsk.h
index 82ea71a0f3ec..3411556e04d9 100644
--- a/tools/lib/bpf/xsk.h
+++ b/tools/lib/bpf/xsk.h
@@ -76,7 +76,7 @@ xsk_ring_cons__rx_desc(const struct xsk_ring_cons *rx, __u32 idx)
return &descs[idx & rx->mask];
}
-static inline __u32 xsk_prod_nb_free(struct xsk_ring_prod *r, __u32 nb)
+static inline __u32 xsk_prod__nb_free(struct xsk_ring_prod *r, __u32 nb)
{
__u32 free_entries = r->cached_cons - r->cached_prod;
@@ -110,7 +110,7 @@ static inline __u32 xsk_cons_nb_avail(struct xsk_ring_cons *r, __u32 nb)
static inline size_t xsk_ring_prod__reserve(struct xsk_ring_prod *prod,
size_t nb, __u32 *idx)
{
- if (xsk_prod_nb_free(prod, nb) < nb)
+ if (xsk_prod__nb_free(prod, nb) < nb)
return 0;
*idx = prod->cached_prod;
--
2.20.1
^ permalink raw reply related
* Re: [PATCH v2 1/4] net: dsa: Change DT bindings for Vitesse VSC73xx switches
From: Linus Walleij @ 2019-07-03 12:41 UTC (permalink / raw)
To: Pawel Dembicki
Cc: Andrew Lunn, Vivien Didelot, Florian Fainelli, David S. Miller,
Rob Herring, Mark Rutland, netdev,
open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS,
linux-kernel@vger.kernel.org
In-Reply-To: <20190703085757.1027-1-paweldembicki@gmail.com>
On Wed, Jul 3, 2019 at 10:58 AM Pawel Dembicki <paweldembicki@gmail.com> wrote:
> This commit introduce how to use vsc73xx platform driver.
>
> Signed-off-by: Pawel Dembicki <paweldembicki@gmail.com>
> ---
> Changes in v2:
> - Drop -spi and -platform suffix
> - Change commit message
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Yours,
Linus Walleij
^ permalink raw reply
* Re: [PATCH v5 bpf-next 4/9] libbpf: add kprobe/uprobe attach API
From: Daniel Borkmann @ 2019-07-03 12:39 UTC (permalink / raw)
To: Andrii Nakryiko, andrii.nakryiko, bpf, netdev, ast, kernel-team,
yhs
In-Reply-To: <20190701235903.660141-5-andriin@fb.com>
On 07/02/2019 01:58 AM, Andrii Nakryiko wrote:
> Add ability to attach to kernel and user probes and retprobes.
> Implementation depends on perf event support for kprobes/uprobes.
>
> Signed-off-by: Andrii Nakryiko <andriin@fb.com>
> Reviewed-by: Stanislav Fomichev <sdf@google.com>
> ---
> tools/lib/bpf/libbpf.c | 169 +++++++++++++++++++++++++++++++++++++++
> tools/lib/bpf/libbpf.h | 7 ++
> tools/lib/bpf/libbpf.map | 2 +
> 3 files changed, 178 insertions(+)
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index bcaa294f819d..7b6142408b15 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -4021,6 +4021,175 @@ struct bpf_link *bpf_program__attach_perf_event(struct bpf_program *prog,
> return (struct bpf_link *)link;
> }
>
> +/*
> + * this function is expected to parse integer in the range of [0, 2^31-1] from
> + * given file using scanf format string fmt. If actual parsed value is
> + * negative, the result might be indistinguishable from error
> + */
> +static int parse_uint_from_file(const char *file, const char *fmt)
> +{
> + char buf[STRERR_BUFSIZE];
> + int err, ret;
> + FILE *f;
> +
> + f = fopen(file, "r");
> + if (!f) {
> + err = -errno;
> + pr_debug("failed to open '%s': %s\n", file,
> + libbpf_strerror_r(err, buf, sizeof(buf)));
> + return err;
> + }
> + err = fscanf(f, fmt, &ret);
> + if (err != 1) {
> + err = err == EOF ? -EIO : -errno;
> + pr_debug("failed to parse '%s': %s\n", file,
> + libbpf_strerror_r(err, buf, sizeof(buf)));
> + fclose(f);
> + return err;
> + }
> + fclose(f);
> + return ret;
> +}
> +
> +static int determine_kprobe_perf_type(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/kprobe/type";
> +
> + return parse_uint_from_file(file, "%d\n");
> +}
> +
> +static int determine_uprobe_perf_type(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/type";
> +
> + return parse_uint_from_file(file, "%d\n");
> +}
> +
> +static int determine_kprobe_retprobe_bit(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/kprobe/format/retprobe";
> +
> + return parse_uint_from_file(file, "config:%d\n");
> +}
> +
> +static int determine_uprobe_retprobe_bit(void)
> +{
> + const char *file = "/sys/bus/event_source/devices/uprobe/format/retprobe";
> +
> + return parse_uint_from_file(file, "config:%d\n");
> +}
> +
> +static int perf_event_open_probe(bool uprobe, bool retprobe, const char *name,
> + uint64_t offset, int pid)
> +{
> + struct perf_event_attr attr = {};
> + char errmsg[STRERR_BUFSIZE];
> + int type, pfd, err;
> +
> + type = uprobe ? determine_uprobe_perf_type()
> + : determine_kprobe_perf_type();
> + if (type < 0) {
> + pr_warning("failed to determine %s perf type: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(type, errmsg, sizeof(errmsg)));
> + return type;
> + }
> + if (retprobe) {
> + int bit = uprobe ? determine_uprobe_retprobe_bit()
> + : determine_kprobe_retprobe_bit();
> +
> + if (bit < 0) {
> + pr_warning("failed to determine %s retprobe bit: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(bit, errmsg,
> + sizeof(errmsg)));
> + return bit;
> + }
> + attr.config |= 1 << bit;
> + }
> + attr.size = sizeof(attr);
> + attr.type = type;
> + attr.config1 = (uint64_t)(void *)name; /* kprobe_func or uprobe_path */
> + attr.config2 = offset; /* kprobe_addr or probe_offset */
> +
> + /* pid filter is meaningful only for uprobes */
> + pfd = syscall(__NR_perf_event_open, &attr,
> + pid < 0 ? -1 : pid /* pid */,
> + pid == -1 ? 0 : -1 /* cpu */,
> + -1 /* group_fd */, PERF_FLAG_FD_CLOEXEC);
> + if (pfd < 0) {
> + err = -errno;
> + pr_warning("%s perf_event_open() failed: %s\n",
> + uprobe ? "uprobe" : "kprobe",
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return err;
> + }
> + return pfd;
> +}
> +
> +struct bpf_link *bpf_program__attach_kprobe(struct bpf_program *prog,
> + bool retprobe,
> + const char *func_name)
> +{
> + char errmsg[STRERR_BUFSIZE];
> + struct bpf_link *link;
> + int pfd, err;
> +
> + pfd = perf_event_open_probe(false /* uprobe */, retprobe, func_name,
> + 0 /* offset */, -1 /* pid */);
> + if (pfd < 0) {
> + pr_warning("program '%s': failed to create %s '%s' perf event: %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "kretprobe" : "kprobe", func_name,
> + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> + return ERR_PTR(pfd);
> + }
> + link = bpf_program__attach_perf_event(prog, pfd);
> + if (IS_ERR(link)) {
> + close(pfd);
> + err = PTR_ERR(link);
> + pr_warning("program '%s': failed to attach to %s '%s': %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "kretprobe" : "kprobe", func_name,
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return link;
> + }
> + return link;
> +}
> +
> +struct bpf_link *bpf_program__attach_uprobe(struct bpf_program *prog,
> + bool retprobe, pid_t pid,
> + const char *binary_path,
> + size_t func_offset)
> +{
> + char errmsg[STRERR_BUFSIZE];
> + struct bpf_link *link;
> + int pfd, err;
> +
> + pfd = perf_event_open_probe(true /* uprobe */, retprobe,
> + binary_path, func_offset, pid);
> + if (pfd < 0) {
> + pr_warning("program '%s': failed to create %s '%s:0x%zx' perf event: %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "uretprobe" : "uprobe",
> + binary_path, func_offset,
> + libbpf_strerror_r(pfd, errmsg, sizeof(errmsg)));
> + return ERR_PTR(pfd);
> + }
> + link = bpf_program__attach_perf_event(prog, pfd);
> + if (IS_ERR(link)) {
> + close(pfd);
> + err = PTR_ERR(link);
> + pr_warning("program '%s': failed to attach to %s '%s:0x%zx': %s\n",
> + bpf_program__title(prog, false),
> + retprobe ? "uretprobe" : "uprobe",
> + binary_path, func_offset,
> + libbpf_strerror_r(err, errmsg, sizeof(errmsg)));
> + return link;
> + }
> + return link;
> +}
Hm, this only addresses half the feedback I had in prior version [0]. Patch 2/9
with bpf_link with destructor looks good to me, but my feedback from back then was
that all the kprobe/uprobe/tracepoint/raw_tracepoint should be split API-wise, so
you'll end up with something like the below, that is, 1) a set of functions that
only /create/ the bpf_link handle /once/, and 2) a helper that allows /attaching/
progs to one or multiple bpf_links. The set of APIs would look like:
struct bpf_link *bpf_link__create_kprobe(bool retprobe, const char *func_name);
struct bpf_link *bpf_link__create_uprobe(bool retprobe, pid_t pid,
const char *binary_path,
size_t func_offset);
int bpf_program__attach_to_link(struct bpf_link *link, struct bpf_program *prog);
int bpf_link__destroy(struct bpf_link *link);
This seems much more natural to me. Right now you sort of do both in one single API.
Detangling the bpf_program__attach_{uprobe,kprobe}() would also avoid that you have
to redo all the perf_event_open_probe() work over and over in order to get the pfd
context where you can later attach something to. Given bpf_program__attach_to_link()
API, you also wouldn't need to expose the bpf_program__attach_perf_event() from
patch 3/9. Thoughts?
[0] https://lore.kernel.org/bpf/a7780057-1d70-9ace-960b-ff65867dc277@iogearbox.net/
> enum bpf_perf_event_ret
> bpf_perf_event_read_simple(void *mmap_mem, size_t mmap_size, size_t page_size,
> void **copy_mem, size_t *copy_size,
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 1bf66c4a9330..bd767cc11967 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -171,6 +171,13 @@ LIBBPF_API int bpf_link__destroy(struct bpf_link *link);
>
> LIBBPF_API struct bpf_link *
> bpf_program__attach_perf_event(struct bpf_program *prog, int pfd);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_kprobe(struct bpf_program *prog, bool retprobe,
> + const char *func_name);
> +LIBBPF_API struct bpf_link *
> +bpf_program__attach_uprobe(struct bpf_program *prog, bool retprobe,
> + pid_t pid, const char *binary_path,
> + size_t func_offset);
>
> struct bpf_insn;
>
> diff --git a/tools/lib/bpf/libbpf.map b/tools/lib/bpf/libbpf.map
> index 756f5aa802e9..57a40fb60718 100644
> --- a/tools/lib/bpf/libbpf.map
> +++ b/tools/lib/bpf/libbpf.map
> @@ -169,7 +169,9 @@ LIBBPF_0.0.4 {
> global:
> bpf_link__destroy;
> bpf_object__load_xattr;
> + bpf_program__attach_kprobe;
> bpf_program__attach_perf_event;
> + bpf_program__attach_uprobe;
> btf_dump__dump_type;
> btf_dump__free;
> btf_dump__new;
>
^ permalink raw reply
* Re: [PATCH net] sctp: count data bundling sack chunk for outctrlchunks
From: Marcelo Ricardo Leitner @ 2019-07-03 12:26 UTC (permalink / raw)
To: Xin Long; +Cc: network dev, linux-sctp, davem, Neil Horman
In-Reply-To: <62e917e312bc582e96fa19b502561e37ca7f91a6.1562149220.git.lucien.xin@gmail.com>
On Wed, Jul 03, 2019 at 06:20:20PM +0800, Xin Long wrote:
> Now all ctrl chunks are counted for asoc stats.octrlchunks and net
> SCTP_MIB_OUTCTRLCHUNKS either after queuing up or bundling, other
> than the chunk maked and bundled in sctp_packet_bundle_sack, which
> caused 'outctrlchunks' not consistent with 'inctrlchunks' in peer.
>
> This issue exists since very beginning, here to fix it by increasing
> both net SCTP_MIB_OUTCTRLCHUNKS and asoc stats.octrlchunks when sack
> chunk is maked and bundled in sctp_packet_bundle_sack.
>
> Reported-by: Ja Ram Jeon <jajeon@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Acked-by: Marcelo Ricardo Leitner <marcelo.leitner@gmail.com>
> ---
> net/sctp/output.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/net/sctp/output.c b/net/sctp/output.c
> index e0c2747..dbda7e7 100644
> --- a/net/sctp/output.c
> +++ b/net/sctp/output.c
> @@ -282,6 +282,9 @@ static enum sctp_xmit sctp_packet_bundle_sack(struct sctp_packet *pkt,
> sctp_chunk_free(sack);
> goto out;
> }
> + SCTP_INC_STATS(sock_net(asoc->base.sk),
> + SCTP_MIB_OUTCTRLCHUNKS);
> + asoc->stats.octrlchunks++;
> asoc->peer.sack_needed = 0;
> if (del_timer(timer))
> sctp_association_put(asoc);
> --
> 2.1.0
>
^ permalink raw reply
* Re: [PATCH 1/2 nf-next v3] netfilter: nft_meta: Add NFT_META_BRI_IIFVPROTO support
From: Pablo Neira Ayuso @ 2019-07-03 12:23 UTC (permalink / raw)
To: Nikolay Aleksandrov; +Cc: wenxu, fw, netfilter-devel, netdev
In-Reply-To: <366e228f-7253-e388-4799-f0f9c18d1111@cumulusnetworks.com>
On Wed, Jul 03, 2019 at 03:08:01PM +0300, Nikolay Aleksandrov wrote:
> On 28/06/2019 03:49, wenxu@ucloud.cn wrote:
> > From: wenxu <wenxu@ucloud.cn>
> >
> > This patch provide a meta to get the bridge vlan proto
> >
> > nft add rule bridge firewall zones counter meta br_vlan_proto 0x8100
> >
> > Signed-off-by: wenxu <wenxu@ucloud.cn>
> > ---
> > include/uapi/linux/netfilter/nf_tables.h | 2 ++
> > net/netfilter/nft_meta.c | 9 +++++++++
> > 2 files changed, 11 insertions(+)
> >
>
> Hi,
> When using the internal bridge API outside of the bridge I'd advise you to CC bridge
> maintainers as well.
Will keep this mind, thanks.
> This patch is clearly wrong since you cannot access the vlan
> fields directly because bridge vlan support might be disabled from the kernel config
> as Pablo has noticed as well. In general I'd try to avoid using the internal API directly,
> but that is a different matter.
BROPT_VLAN_ENABLED is exposed through netlink and sysfs, and this only
consults the value. I guess you refer to the fact that...
> Please consult with include/linux/if_bridge.h for exported
> functions that are supposed to be visible outside of the bridge, if you need anything else
> make sure to add support for it there. The usage of br_opt_get directly for example must
> be changed to br_vlan_enabled().
Indeed... this patch should be using br_vlan_enabled() instead.
Thanks.
^ 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