* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Andrii Nakryiko @ 2019-02-27 23:34 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190227212256.3856473-1-yhs@fb.com>
On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
>
> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
> introduced bpf_spin_lock and the field spin_lock_off
> in kernel internal structure bpf_map has the following
> meaning:
> >=0 valid offset, <0 error
>
> For every map created, the kernel will ensure
> spin_lock_off has correct value.
>
> Currently, bpf_map->spin_lock_off is not copied
> from the inner map to the map_in_map inner_map_meta
> during a map_in_map type map creation, so
> inner_map_meta->spin_lock_off = 0.
> This will give verifier wrong information that
> inner_map has bpf_spin_lock and the bpf_spin_lock
> is defined at offset 0. An access to offset 0
> of a value pointer will trigger the following error:
> bpf_spin_lock cannot be accessed directly by load/store
>
> This patch fixed the issue by copy inner map's spin_lock_off
> value to inner_map_meta->spin_lock_off.
>
> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> kernel/bpf/map_in_map.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
> index 583346a0ab29..3dff41403583 100644
> --- a/kernel/bpf/map_in_map.c
> +++ b/kernel/bpf/map_in_map.c
> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
> inner_map_meta->value_size = inner_map->value_size;
> inner_map_meta->map_flags = inner_map->map_flags;
> inner_map_meta->max_entries = inner_map->max_entries;
> + inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
Looks like spinlock inside inner map is not supported: there is
specific check few lines above returning -ENOSUPP for such case. In
that case, maybe assign -1 here to make this explicit?
Though I guess that also brings up the question: is there any harm in
supporting spin lock for inner map and why it was disabled in the
first place?
>
> /* Misc members not needed in bpf_map_meta_equal() check. */
> inner_map_meta->ops = inner_map->ops;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: si-wei liu @ 2019-02-27 23:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Samudrala, Sridhar, Siwei Liu, Jiri Pirko, Stephen Hemminger,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <20190227173710-mutt-send-email-mst@kernel.org>
On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
>>
>> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
>>> On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
>>>> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
>>>>>> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
>>>>>>> On 2/21/2019 7:33 PM, si-wei liu wrote:
>>>>>>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
>>>>>>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
>>>>>>>>>> Sorry for replying to this ancient thread. There was some remaining
>>>>>>>>>> issue that I don't think the initial net_failover patch got addressed
>>>>>>>>>> cleanly, see:
>>>>>>>>>>
>>>>>>>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
>>>>>>>>>>
>>>>>>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
>>>>>>>>>> not specifically writtten for such kernel automatic enslavement.
>>>>>>>>>> Specifically, if it is a bond or team, the slave would typically get
>>>>>>>>>> renamed *before* virtual device gets created, that's what udev can
>>>>>>>>>> control (without getting netdev opened early by the other part of
>>>>>>>>>> kernel) and other userspace components for e.g. initramfs,
>>>>>>>>>> init-scripts can coordinate well in between. The in-kernel
>>>>>>>>>> auto-enslavement of net_failover breaks this userspace convention,
>>>>>>>>>> which don't provides a solution if user care about consistent naming
>>>>>>>>>> on the slave netdevs specifically.
>>>>>>>>>>
>>>>>>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
>>>>>>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
>>>>>>>>>> problem ever since. Please share your mind how to proceed and solve
>>>>>>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
>>>>>>>>> Above says:
>>>>>>>>>
>>>>>>>>> there's no motivation in the systemd/udevd community at
>>>>>>>>> this point to refactor the rename logic and make it work well with
>>>>>>>>> 3-netdev.
>>>>>>>>>
>>>>>>>>> What would the fix be? Skip slave devices?
>>>>>>>>>
>>>>>>>> There's nothing user can get if just skipping slave devices - the
>>>>>>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
>>>>>>>> next reboot, while the rest may conform to the naming scheme (ens3
>>>>>>>> and such). There's no way one can fix this in userspace alone - when
>>>>>>>> the failover is created the enslaved netdev was opened by the kernel
>>>>>>>> earlier than the userspace is made aware of, and there's no
>>>>>>>> negotiation protocol for kernel to know when userspace has done
>>>>>>>> initial renaming of the interface. I would expect netdev list should
>>>>>>>> at least provide the direction in general for how this can be
>>>>>>>> solved...
>>>>> I was just wondering what did you mean when you said
>>>>> "refactor the rename logic and make it work well with 3-netdev" -
>>>>> was there a proposal udev rejected?
>>>> No. I never believed this particular issue can be fixed in userspace alone.
>>>> Previously someone had said it could be, but I never see any work or
>>>> relevant discussion ever happened in various userspace communities (for e.g.
>>>> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
>>>> of the issue derives from the kernel, it makes more sense to start from
>>>> netdev, work out and decide on a solution: see what can be done in the
>>>> kernel in order to fix it, then after that engage userspace community for
>>>> the feasibility...
>>>>
>>>>> Anyway, can we write a time diagram for what happens in which order that
>>>>> leads to failure? That would help look for triggers that we can tie
>>>>> into, or add new ones.
>>>>>
>>>> See attached diagram.
>>>>
>>>>>
>>>>>
>>>>>>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
>>>>>>> to only work with the master failover device.
>>>>>> Where does this expectation come from?
>>>>>>
>>>>>> Admin users may have ethtool or tc configurations that need to deal with
>>>>>> predictable interface name. Third-party app which was built upon specifying
>>>>>> certain interface name can't be modified to chase dynamic names.
>>>>>>
>>>>>> Specifically, we have pre-canned image that uses ethtool to fine tune VF
>>>>>> offload settings post boot for specific workload. Those images won't work
>>>>>> well if the name is constantly changing just after couple rounds of live
>>>>>> migration.
>>>>> It should be possible to specify the ethtool configuration on the
>>>>> master and have it automatically propagated to the slave.
>>>>>
>>>>> BTW this is something we should look at IMHO.
>>>> I was elaborating a few examples that the expectation and assumption that
>>>> user/admin scripts only deal with master failover device is incorrect. It
>>>> had never been taken good care of, although I did try to emphasize it from
>>>> the very beginning.
>>>>
>>>> Basically what you said about propagating the ethtool configuration down to
>>>> the slave is the key pursuance of 1-netdev model. However, what I am seeking
>>>> now is any alternative that can also fix the specific udev rename problem,
>>>> before concluding that 1-netdev is the only solution. Generally a 1-netdev
>>>> scheme would take time to implement, while I'm trying to find a way out to
>>>> fix this particular naming problem under 3-netdev.
>>>>
>>>>>>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
>>>>>>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
>>>>>>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
>>>>>>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
>>>>>> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
>>>>>> model as much transparent to a real NIC as possible, while a hidden netns is
>>>>>> just the vehicle). However, I recall there was resistance around this
>>>>>> discussion that even the concept of hiding itself is a taboo for Linux
>>>>>> netdev. I would like to summon potential alternatives before concluding
>>>>>> 1-netdev is the only solution too soon.
>>>>>>
>>>>>> Thanks,
>>>>>> -Siwei
>>>>> Your scripts would not work at all then, right?
>>>> At this point we don't claim images with such usage as SR-IOV live
>>>> migrate-able. We would flag it as live migrate-able until this ethtool
>>>> config issue is fully addressed and a transparent live migration solution
>>>> emerges in upstream eventually.
>>>>
>>>>
>>>> Thanks,
>>>> -Siwei
>>>>>>>> -Siwei
>>>>>>>>
>>>>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>>>
>>>> net_failover(kernel) | network.service (user) | systemd-udevd (user)
>>>> --------------------------------------------------+------------------------------+--------------------------------------------
>>>> (standby virtio-net and net_failover | |
>>>> devices created and initialized, | |
>>>> i.e. virtnet_probe()-> | |
>>>> net_failover_create() | |
>>>> was done.) | |
>>>> | |
>>>> | runs `ifup ens3' -> |
>>>> | ip link set dev ens3 up |
>>>> net_failover_open() | |
>>>> dev_open(virtnet_dev) | |
>>>> virtnet_open(virtnet_dev) | |
>>>> netif_carrier_on(failover_dev) | |
>>>> ... | |
>>>> | |
>>>> (VF hot plugged in) | |
>>>> ixgbevf_probe() | |
>>>> register_netdev(ixgbevf_netdev) | |
>>>> netdev_register_kobject(ixgbevf_netdev) | |
>>>> kobject_add(ixgbevf_dev) | |
>>>> device_add(ixgbevf_dev) | |
>>>> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
>>>> netlink_broadcast() | |
>>>> ... | |
>>>> call_netdevice_notifiers(NETDEV_REGISTER) | |
>>>> failover_event(..., NETDEV_REGISTER, ...) | |
>>>> failover_slave_register(ixgbevf_netdev) | |
>>>> net_failover_slave_register(ixgbevf_netdev) | |
>>>> dev_open(ixgbevf_netdev) | |
>>>> | |
>>>> | |
>>>> | | received ADD uevent from netlink fd
>>>> | | ...
>>>> | | udev-builtin-net_id.c:dev_pci_slot()
>>>> | | (decided to renamed 'eth0' )
>>>> | | ip link set dev eth0 name ens4
>>>> (dev_change_name() returns -EBUSY as | |
>>>> ixgbevf_netdev->flags has IFF_UP) | |
>>>> | |
>>>>
>>> Given renaming slaves does not work anyway:
>> I was actually thinking what if we relieve the rename restriction just for
>> the failover slave? What the impact would be? I think users don't care about
>> slave being renamed when it's in use, especially the initial rename.
>> Thoughts?
>>
>>> would it work if we just
>>> hard-coded slave names instead?
>>>
>>> E.g.
>>> 1. fail slave renames
>>> 2. rename of failover to XX automatically renames standby to XXnsby
>>> and primary to XXnpry
>> That wouldn't help. The time when the failover master gets renamed, the VF
>> may not be present.
> In this scheme if VF is not there it will be renamed immediately after registration.
Who will be responsible to rename the slave, the kernel? Note the
master's name may or may not come from the userspace. If it comes from
the userspace, should the userspace daemon change their expectation not
to name/rename _any_ slaves (today there's no distinction)? How do users
know which name to trust, depending on which wins the race more often?
Say if kernel wants a ens3npry name while userspace wants it named as ens4.
-Siwei
>
>> I don't like the idea to delay exposing failover master
>> until VF is hot plugged in (probably subject to various failures) later.
>>
>> Thanks,
>> -Siwei
>
> I agree, this was not what I meant.
>
>>>
^ permalink raw reply
* [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
For historical reasons the helper to loop over maps in an object
is called bpf_map__for_each while it really should be called
bpf_object__for_each_map. Rename and add a correctly named
define for backward compatibility.
Switch all in-tree users to the correct name (Quentin).
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
tools/bpf/bpftool/prog.c | 4 ++--
tools/lib/bpf/libbpf.c | 8 ++++----
tools/lib/bpf/libbpf.h | 3 ++-
tools/perf/util/bpf-loader.c | 4 ++--
tools/testing/selftests/bpf/test_libbpf_open.c | 2 +-
5 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 0c35dd543d49..8ef80d65a474 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1053,7 +1053,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
j = 0;
while (j < old_map_fds && map_replace[j].name) {
i = 0;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
if (!strcmp(bpf_map__name(map), map_replace[j].name)) {
map_replace[j].idx = i;
break;
@@ -1074,7 +1074,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
/* Set ifindex and name reuse */
j = 0;
idx = 0;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
if (!bpf_map__is_offload_neutral(map))
bpf_map__set_ifindex(map, ifindex);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b38dcbe7460a..f5eb60379c8d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2100,7 +2100,7 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
if (err)
return err;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
char buf[PATH_MAX];
int len;
@@ -2147,7 +2147,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
if (!obj)
return -ENOENT;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
char buf[PATH_MAX];
int len;
@@ -2835,7 +2835,7 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
{
struct bpf_map *pos;
- bpf_map__for_each(pos, obj) {
+ bpf_object__for_each_map(pos, obj) {
if (pos->name && !strcmp(pos->name, name))
return pos;
}
@@ -2928,7 +2928,7 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
first_prog = prog;
}
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
if (!bpf_map__is_offload_neutral(map))
map->map_ifindex = attr->ifindex;
}
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6c0168f8bba5..b4652aa1a58a 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
LIBBPF_API struct bpf_map *
bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
-#define bpf_map__for_each(pos, obj) \
+#define bpf_object__for_each_map(pos, obj) \
for ((pos) = bpf_map__next(NULL, (obj)); \
(pos) != NULL; \
(pos) = bpf_map__next((pos), (obj)))
+#define bpf_map__for_each bpf_object__for_each_map
LIBBPF_API struct bpf_map *
bpf_map__prev(struct bpf_map *map, struct bpf_object *obj);
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 037d8ff6a634..31b7e5a1453b 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -1489,7 +1489,7 @@ apply_obj_config_object(struct bpf_object *obj)
struct bpf_map *map;
int err;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
err = apply_obj_config_map(map);
if (err)
return err;
@@ -1513,7 +1513,7 @@ int bpf__apply_obj_config(void)
#define bpf__for_each_map(pos, obj, objtmp) \
bpf_object__for_each_safe(obj, objtmp) \
- bpf_map__for_each(pos, obj)
+ bpf_object__for_each_map(pos, obj)
#define bpf__for_each_map_named(pos, obj, objtmp, name) \
bpf__for_each_map(pos, obj, objtmp) \
diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
index 1909ecf4d999..65cbd30704b5 100644
--- a/tools/testing/selftests/bpf/test_libbpf_open.c
+++ b/tools/testing/selftests/bpf/test_libbpf_open.c
@@ -67,7 +67,7 @@ int test_walk_maps(struct bpf_object *obj, bool verbose)
struct bpf_map *map;
int cnt = 0;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
cnt++;
if (verbose)
printf("Map (count:%d) name: %s\n", cnt,
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 5/5] tools: libbpf: make sure readelf shows full names in build checks
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
readelf truncates its output by default to attempt to make it more
readable. This can lead to function names getting aliased if they
differ late in the string. Use --wide parameter to avoid
truncation.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
tools/lib/bpf/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 761691bd72ad..a05c43468bd0 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -132,9 +132,9 @@ BPF_IN := $(OUTPUT)libbpf-in.o
LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
VERSION_SCRIPT := libbpf.map
-GLOBAL_SYM_COUNT = $(shell readelf -s $(BPF_IN) | \
+GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {s++} END{print s}')
-VERSIONED_SYM_COUNT = $(shell readelf -s $(OUTPUT)libbpf.so | \
+VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so | \
grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
CMD_TARGETS = $(LIB_FILE)
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 4/5] samples: bpf: use libbpf where easy
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
Some samples don't really need the magic of bpf_load,
switch them to libbpf.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
samples/bpf/Makefile | 6 +++---
samples/bpf/fds_example.c | 9 ++++++---
samples/bpf/sockex1_user.c | 22 ++++++++++++----------
samples/bpf/sockex2_user.c | 20 +++++++++++---------
4 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 4dd98100678e..0c62ac39c697 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -59,9 +59,9 @@ LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
-fds_example-objs := bpf_load.o fds_example.o
-sockex1-objs := bpf_load.o sockex1_user.o
-sockex2-objs := bpf_load.o sockex2_user.o
+fds_example-objs := fds_example.o
+sockex1-objs := sockex1_user.o
+sockex2-objs := sockex2_user.o
sockex3-objs := bpf_load.o sockex3_user.o
tracex1-objs := bpf_load.o tracex1_user.o
tracex2-objs := bpf_load.o tracex2_user.o
diff --git a/samples/bpf/fds_example.c b/samples/bpf/fds_example.c
index 9854854f05d1..36f1f18aae3c 100644
--- a/samples/bpf/fds_example.c
+++ b/samples/bpf/fds_example.c
@@ -14,8 +14,8 @@
#include <bpf/bpf.h>
+#include "bpf/libbpf.h"
#include "bpf_insn.h"
-#include "bpf_load.h"
#include "sock_example.h"
#define BPF_F_PIN (1 << 0)
@@ -57,10 +57,13 @@ static int bpf_prog_create(const char *object)
BPF_EXIT_INSN(),
};
size_t insns_cnt = sizeof(insns) / sizeof(struct bpf_insn);
+ char bpf_log_buf[BPF_LOG_BUF_SIZE];
+ struct bpf_object *obj;
+ int prog_fd;
if (object) {
- assert(!load_bpf_file((char *)object));
- return prog_fd[0];
+ assert(!bpf_prog_load(object, 0, &obj, &prog_fd));
+ return prog_fd;
} else {
return bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER,
insns, insns_cnt, "GPL", 0,
diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
index be8ba5686924..5e7c4be3e645 100644
--- a/samples/bpf/sockex1_user.c
+++ b/samples/bpf/sockex1_user.c
@@ -3,28 +3,30 @@
#include <assert.h>
#include <linux/bpf.h>
#include <bpf/bpf.h>
-#include "bpf_load.h"
+#include "bpf/libbpf.h"
#include "sock_example.h"
#include <unistd.h>
#include <arpa/inet.h>
int main(int ac, char **argv)
{
+ struct bpf_object *obj;
+ int map_fd, prog_fd;
char filename[256];
- FILE *f;
int i, sock;
+ FILE *f;
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
- if (load_bpf_file(filename)) {
- printf("%s", bpf_log_buf);
+ if (bpf_prog_load(filename, 0, &obj, &prog_fd))
return 1;
- }
+
+ map_fd = bpf_object__find_map_fd_by_name(obj, "my_map");
sock = open_raw_sock("lo");
- assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
- sizeof(prog_fd[0])) == 0);
+ assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
+ sizeof(prog_fd)) == 0);
f = popen("ping -4 -c5 localhost", "r");
(void) f;
@@ -34,13 +36,13 @@ int main(int ac, char **argv)
int key;
key = IPPROTO_TCP;
- assert(bpf_map_lookup_elem(map_fd[0], &key, &tcp_cnt) == 0);
+ assert(bpf_map_lookup_elem(map_fd, &key, &tcp_cnt) == 0);
key = IPPROTO_UDP;
- assert(bpf_map_lookup_elem(map_fd[0], &key, &udp_cnt) == 0);
+ assert(bpf_map_lookup_elem(map_fd, &key, &udp_cnt) == 0);
key = IPPROTO_ICMP;
- assert(bpf_map_lookup_elem(map_fd[0], &key, &icmp_cnt) == 0);
+ assert(bpf_map_lookup_elem(map_fd, &key, &icmp_cnt) == 0);
printf("TCP %lld UDP %lld ICMP %lld bytes\n",
tcp_cnt, udp_cnt, icmp_cnt);
diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
index 125ee6efc913..e3611dbfce97 100644
--- a/samples/bpf/sockex2_user.c
+++ b/samples/bpf/sockex2_user.c
@@ -3,7 +3,7 @@
#include <assert.h>
#include <linux/bpf.h>
#include <bpf/bpf.h>
-#include "bpf_load.h"
+#include "bpf/libbpf.h"
#include "sock_example.h"
#include <unistd.h>
#include <arpa/inet.h>
@@ -17,22 +17,24 @@ struct pair {
int main(int ac, char **argv)
{
struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+ struct bpf_object *obj;
+ int map_fd, prog_fd;
char filename[256];
- FILE *f;
int i, sock;
+ FILE *f;
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
setrlimit(RLIMIT_MEMLOCK, &r);
- if (load_bpf_file(filename)) {
- printf("%s", bpf_log_buf);
+ if (bpf_prog_load(filename, 0, &obj, &prog_fd))
return 1;
- }
+
+ map_fd = bpf_object__find_map_fd_by_name(obj, "hash_map");
sock = open_raw_sock("lo");
- assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
- sizeof(prog_fd[0])) == 0);
+ assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
+ sizeof(prog_fd)) == 0);
f = popen("ping -4 -c5 localhost", "r");
(void) f;
@@ -41,8 +43,8 @@ int main(int ac, char **argv)
int key = 0, next_key;
struct pair value;
- while (bpf_map_get_next_key(map_fd[0], &key, &next_key) == 0) {
- bpf_map_lookup_elem(map_fd[0], &next_key, &value);
+ while (bpf_map_get_next_key(map_fd, &key, &next_key) == 0) {
+ bpf_map_lookup_elem(map_fd, &next_key, &value);
printf("ip %s bytes %lld packets %lld\n",
inet_ntoa((struct in_addr){htonl(next_key)}),
value.bytes, value.packets);
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 1/5] samples: bpf: force IPv4 in ping
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
ping localhost may default of IPv6 on modern systems, but
samples are trying to only parse IPv4. Force IPv4.
samples/bpf/tracex1_user.c doesn't interpret the packet so
we don't care which IP version will be used there.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
samples/bpf/sock_example.c | 2 +-
samples/bpf/sockex1_user.c | 2 +-
samples/bpf/sockex2_user.c | 2 +-
samples/bpf/sockex3_user.c | 2 +-
samples/bpf/tracex2_user.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/samples/bpf/sock_example.c b/samples/bpf/sock_example.c
index 60ec467c78ab..00aae1d33fca 100644
--- a/samples/bpf/sock_example.c
+++ b/samples/bpf/sock_example.c
@@ -99,7 +99,7 @@ int main(void)
{
FILE *f;
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void)f;
return test_sock();
diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
index 93ec01c56104..be8ba5686924 100644
--- a/samples/bpf/sockex1_user.c
+++ b/samples/bpf/sockex1_user.c
@@ -26,7 +26,7 @@ int main(int ac, char **argv)
assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
sizeof(prog_fd[0])) == 0);
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void) f;
for (i = 0; i < 5; i++) {
diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
index 1d5c6e9a6d27..125ee6efc913 100644
--- a/samples/bpf/sockex2_user.c
+++ b/samples/bpf/sockex2_user.c
@@ -34,7 +34,7 @@ int main(int ac, char **argv)
assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
sizeof(prog_fd[0])) == 0);
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void) f;
for (i = 0; i < 5; i++) {
diff --git a/samples/bpf/sockex3_user.c b/samples/bpf/sockex3_user.c
index 9d02e0404719..bbb1cd0666a9 100644
--- a/samples/bpf/sockex3_user.c
+++ b/samples/bpf/sockex3_user.c
@@ -58,7 +58,7 @@ int main(int argc, char **argv)
sizeof(__u32)) == 0);
if (argc > 1)
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
else
f = popen("netperf -l 4 localhost", "r");
(void) f;
diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
index 1a81e6a5c2ea..c9544a4ce61a 100644
--- a/samples/bpf/tracex2_user.c
+++ b/samples/bpf/tracex2_user.c
@@ -131,7 +131,7 @@ int main(int ac, char **argv)
signal(SIGTERM, int_exit);
/* start 'ping' in the background to have some kfree_skb events */
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void) f;
/* start 'dd' in the background to have plenty of 'write' syscalls */
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 2/5] samples: bpf: remove load_sock_ops in favour of bpftool
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
bpftool can do all the things load_sock_ops used to do, and more.
Point users to bpftool instead of maintaining this sample utility.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
samples/bpf/.gitignore | 1 -
samples/bpf/Makefile | 2 -
samples/bpf/load_sock_ops.c | 97 ------------------------------
samples/bpf/tcp_basertt_kern.c | 2 +-
samples/bpf/tcp_bpf.readme | 14 +++--
samples/bpf/tcp_bufs_kern.c | 2 +-
samples/bpf/tcp_clamp_kern.c | 2 +-
samples/bpf/tcp_cong_kern.c | 2 +-
samples/bpf/tcp_iw_kern.c | 2 +-
samples/bpf/tcp_rwnd_kern.c | 2 +-
samples/bpf/tcp_synrto_kern.c | 2 +-
samples/bpf/tcp_tos_reflect_kern.c | 2 +-
12 files changed, 16 insertions(+), 114 deletions(-)
delete mode 100644 samples/bpf/load_sock_ops.c
diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
index 8ae4940025f8..dbb817dbacfc 100644
--- a/samples/bpf/.gitignore
+++ b/samples/bpf/.gitignore
@@ -1,7 +1,6 @@
cpustat
fds_example
lathist
-load_sock_ops
lwt_len_hist
map_perf_test
offwaketime
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index a333e258f319..4dd98100678e 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -40,7 +40,6 @@ hostprogs-y += lwt_len_hist
hostprogs-y += xdp_tx_iptunnel
hostprogs-y += test_map_in_map
hostprogs-y += per_socket_stats_example
-hostprogs-y += load_sock_ops
hostprogs-y += xdp_redirect
hostprogs-y += xdp_redirect_map
hostprogs-y += xdp_redirect_cpu
@@ -71,7 +70,6 @@ tracex4-objs := bpf_load.o tracex4_user.o
tracex5-objs := bpf_load.o tracex5_user.o
tracex6-objs := bpf_load.o tracex6_user.o
tracex7-objs := bpf_load.o tracex7_user.o
-load_sock_ops-objs := bpf_load.o load_sock_ops.o
test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o
trace_output-objs := bpf_load.o trace_output_user.o $(TRACE_HELPERS)
lathist-objs := bpf_load.o lathist_user.o
diff --git a/samples/bpf/load_sock_ops.c b/samples/bpf/load_sock_ops.c
deleted file mode 100644
index 8ecb41ea0c03..000000000000
--- a/samples/bpf/load_sock_ops.c
+++ /dev/null
@@ -1,97 +0,0 @@
-/* Copyright (c) 2017 Facebook
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 of the GNU General Public
- * License as published by the Free Software Foundation.
- */
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <linux/bpf.h>
-#include <bpf/bpf.h>
-#include "bpf_load.h"
-#include <unistd.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <linux/unistd.h>
-
-static void usage(char *pname)
-{
- printf("USAGE:\n %s [-l] <cg-path> <prog filename>\n", pname);
- printf("\tLoad and attach a sock_ops program to the specified "
- "cgroup\n");
- printf("\tIf \"-l\" is used, the program will continue to run\n");
- printf("\tprinting the BPF log buffer\n");
- printf("\tIf the specified filename does not end in \".o\", it\n");
- printf("\tappends \"_kern.o\" to the name\n");
- printf("\n");
- printf(" %s -r <cg-path>\n", pname);
- printf("\tDetaches the currently attached sock_ops program\n");
- printf("\tfrom the specified cgroup\n");
- printf("\n");
- exit(1);
-}
-
-int main(int argc, char **argv)
-{
- int logFlag = 0;
- int error = 0;
- char *cg_path;
- char fn[500];
- char *prog;
- int cg_fd;
-
- if (argc < 3)
- usage(argv[0]);
-
- if (!strcmp(argv[1], "-r")) {
- cg_path = argv[2];
- cg_fd = open(cg_path, O_DIRECTORY, O_RDONLY);
- error = bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
- if (error) {
- printf("ERROR: bpf_prog_detach: %d (%s)\n",
- error, strerror(errno));
- return 2;
- }
- return 0;
- } else if (!strcmp(argv[1], "-h")) {
- usage(argv[0]);
- } else if (!strcmp(argv[1], "-l")) {
- logFlag = 1;
- if (argc < 4)
- usage(argv[0]);
- }
-
- prog = argv[argc - 1];
- cg_path = argv[argc - 2];
- if (strlen(prog) > 480) {
- fprintf(stderr, "ERROR: program name too long (> 480 chars)\n");
- return 3;
- }
- cg_fd = open(cg_path, O_DIRECTORY, O_RDONLY);
-
- if (!strcmp(prog + strlen(prog)-2, ".o"))
- strcpy(fn, prog);
- else
- sprintf(fn, "%s_kern.o", prog);
- if (logFlag)
- printf("loading bpf file:%s\n", fn);
- if (load_bpf_file(fn)) {
- printf("ERROR: load_bpf_file failed for: %s\n", fn);
- printf("%s", bpf_log_buf);
- return 4;
- }
- if (logFlag)
- printf("TCP BPF Loaded %s\n", fn);
-
- error = bpf_prog_attach(prog_fd[0], cg_fd, BPF_CGROUP_SOCK_OPS, 0);
- if (error) {
- printf("ERROR: bpf_prog_attach: %d (%s)\n",
- error, strerror(errno));
- return 5;
- } else if (logFlag) {
- read_trace_pipe();
- }
-
- return error;
-}
diff --git a/samples/bpf/tcp_basertt_kern.c b/samples/bpf/tcp_basertt_kern.c
index 4bf4fc597db9..6ef1625e8b2c 100644
--- a/samples/bpf/tcp_basertt_kern.c
+++ b/samples/bpf/tcp_basertt_kern.c
@@ -7,7 +7,7 @@
* BPF program to set base_rtt to 80us when host is running TCP-NV and
* both hosts are in the same datacenter (as determined by IPv6 prefix).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
index 831fb601e3c9..fee746621aec 100644
--- a/samples/bpf/tcp_bpf.readme
+++ b/samples/bpf/tcp_bpf.readme
@@ -8,14 +8,16 @@ a cgroupv2 and attach a bash shell to the group.
bash
echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
-Anything that runs under this shell belongs to the foo cgroupv2 To load
+Anything that runs under this shell belongs to the foo cgroupv2. To load
(attach) one of the tcp_*_kern.o programs:
- ./load_sock_ops -l /tmp/cgroupv2/foo tcp_basertt_kern.o
+ bpftool prog load tcp_basertt_kern.o /sys/fs/bpf/tcp_prog
+ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
+ bpftool prog tracelog
-If the "-l" flag is used, the load_sock_ops program will continue to run
-printing the BPF log buffer. The tcp_*_kern.o programs use special print
-functions to print logging information (if enabled by the ifdef).
+"bpftool prog tracelog" will continue to run printing the BPF log buffer.
+The tcp_*_kern.o programs use special print functions to print logging
+information (if enabled by the ifdef).
If using netperf/netserver to create traffic, you need to run them under the
cgroupv2 to which the BPF programs are attached (i.e. under bash shell
@@ -23,4 +25,4 @@ attached to the cgroupv2).
To remove (unattach) a socket_ops BPF program from a cgroupv2:
- ./load_sock_ops -r /tmp/cgroupv2/foo
+ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
diff --git a/samples/bpf/tcp_bufs_kern.c b/samples/bpf/tcp_bufs_kern.c
index 0566b7fa38a1..e03e204739fa 100644
--- a/samples/bpf/tcp_bufs_kern.c
+++ b/samples/bpf/tcp_bufs_kern.c
@@ -9,7 +9,7 @@
* doing appropriate checks that indicate the hosts are far enough
* away (i.e. large RTT).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_clamp_kern.c b/samples/bpf/tcp_clamp_kern.c
index f4225c9d2c0c..a0dc2d254aca 100644
--- a/samples/bpf/tcp_clamp_kern.c
+++ b/samples/bpf/tcp_clamp_kern.c
@@ -9,7 +9,7 @@
* the same datacenter. For his example, we assume they are within the same
* datacenter when the first 5.5 bytes of their IPv6 addresses are the same.
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_cong_kern.c b/samples/bpf/tcp_cong_kern.c
index ad0f1ba8206a..4fd3ca979a06 100644
--- a/samples/bpf/tcp_cong_kern.c
+++ b/samples/bpf/tcp_cong_kern.c
@@ -7,7 +7,7 @@
* BPF program to set congestion control to dctcp when both hosts are
* in the same datacenter (as deteremined by IPv6 prefix).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_iw_kern.c b/samples/bpf/tcp_iw_kern.c
index 4ca5ecc9f580..9b139ec69560 100644
--- a/samples/bpf/tcp_iw_kern.c
+++ b/samples/bpf/tcp_iw_kern.c
@@ -9,7 +9,7 @@
* would usually be done after doing appropriate checks that indicate
* the hosts are far enough away (i.e. large RTT).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_rwnd_kern.c b/samples/bpf/tcp_rwnd_kern.c
index 09ff65b40b31..cc71ee96e044 100644
--- a/samples/bpf/tcp_rwnd_kern.c
+++ b/samples/bpf/tcp_rwnd_kern.c
@@ -8,7 +8,7 @@
* and the first 5.5 bytes of the IPv6 addresses are not the same (in this
* example that means both hosts are not the same datacenter).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_synrto_kern.c b/samples/bpf/tcp_synrto_kern.c
index 232bb242823e..ca87ed34f896 100644
--- a/samples/bpf/tcp_synrto_kern.c
+++ b/samples/bpf/tcp_synrto_kern.c
@@ -8,7 +8,7 @@
* and the first 5.5 bytes of the IPv6 addresses are the same (in this example
* that means both hosts are in the same datacenter).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_tos_reflect_kern.c b/samples/bpf/tcp_tos_reflect_kern.c
index d51dab19eca6..de788be6f862 100644
--- a/samples/bpf/tcp_tos_reflect_kern.c
+++ b/samples/bpf/tcp_tos_reflect_kern.c
@@ -4,7 +4,7 @@
*
* BPF program to automatically reflect TOS option from received syn packet
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 0/5] samples: bpf: start effort to get rid of bpf_load
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
Hi!
This set is next part of a quest to get rid of the bpf_load
ELF loader. It fixes some minor issues with the samples and
starts the conversion.
First patch fixes ping invocations, ping localhost defaults
to IPv6 on modern setups. Next load_sock_ops sample is removed
and users are directed towards using bpftool directly.
Patch 4 removes the use of bpf_load from samples which don't
need the auto-attachment functionality at all.
Patch 5 improves symbol counting in libbpf, it's not currently
an issue but it will be when anyone adds a symbol with a long
name. Let's make sure that person doesn't have to spend time
scratching their head and wondering why .a and .so symbol
counts don't match.
Jakub Kicinski (5):
samples: bpf: force IPv4 in ping
samples: bpf: remove load_sock_ops in favour of bpftool
tools: libbpf: add a correctly named define for map iteration
samples: bpf: use libbpf where easy
tools: libbpf: make sure readelf shows full names in build checks
samples/bpf/.gitignore | 1 -
samples/bpf/Makefile | 8 +-
samples/bpf/fds_example.c | 9 +-
samples/bpf/load_sock_ops.c | 97 -------------------
samples/bpf/sock_example.c | 2 +-
samples/bpf/sockex1_user.c | 24 ++---
samples/bpf/sockex2_user.c | 22 +++--
samples/bpf/sockex3_user.c | 2 +-
samples/bpf/tcp_basertt_kern.c | 2 +-
samples/bpf/tcp_bpf.readme | 14 +--
samples/bpf/tcp_bufs_kern.c | 2 +-
samples/bpf/tcp_clamp_kern.c | 2 +-
samples/bpf/tcp_cong_kern.c | 2 +-
samples/bpf/tcp_iw_kern.c | 2 +-
samples/bpf/tcp_rwnd_kern.c | 2 +-
samples/bpf/tcp_synrto_kern.c | 2 +-
samples/bpf/tcp_tos_reflect_kern.c | 2 +-
samples/bpf/tracex2_user.c | 2 +-
tools/bpf/bpftool/prog.c | 4 +-
tools/lib/bpf/Makefile | 4 +-
tools/lib/bpf/libbpf.c | 8 +-
tools/lib/bpf/libbpf.h | 3 +-
tools/perf/util/bpf-loader.c | 4 +-
.../testing/selftests/bpf/test_libbpf_open.c | 2 +-
24 files changed, 66 insertions(+), 156 deletions(-)
delete mode 100644 samples/bpf/load_sock_ops.c
--
2.19.2
^ permalink raw reply
* Re: [PATCH -next] appletalk: use remove_proc_subtree to simplify procfs code
From: kbuild test robot @ 2019-02-27 23:28 UTC (permalink / raw)
To: Yue Haibing
Cc: kbuild-all, davem, joe, gregkh, linux-kernel, netdev, YueHaibing
In-Reply-To: <20190227150023.28420-1-yuehaibing@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]
Hi YueHaibing,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20190227]
url: https://github.com/0day-ci/linux/commits/Yue-Haibing/appletalk-use-remove_proc_subtree-to-simplify-procfs-code/20190228-014856
config: i386-randconfig-s3-02271654 (attached as .config)
compiler: gcc-6 (Debian 6.5.0-2) 6.5.0 20181026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
net//appletalk/atalk_proc.c: In function 'atalk_proc_init':
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> net//appletalk/atalk_proc.c:218:2: note: in expansion of macro 'if'
if (!proc_create_seq("atalk/interface", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:220:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net//appletalk/atalk_proc.c:222:2: note: in expansion of macro 'if'
if (!proc_create_seq("atalk/route", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:224:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net//appletalk/atalk_proc.c:226:2: note: in expansion of macro 'if'
if (!proc_create_seq("atalk/socket", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:228:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net//appletalk/atalk_proc.c:230:2: note: in expansion of macro 'if'
if (!proc_create_seq_private("atalk/arp", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:233:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
vim +/if +218 net//appletalk/atalk_proc.c
212
213 int __init atalk_proc_init(void)
214 {
215 if (!proc_mkdir("atalk", init_net.proc_net))
216 return -ENOMEM;
217
> 218 if (!proc_create_seq("atalk/interface", 0444, init_net.proc_net,
219 &atalk_seq_interface_ops));
220 goto out;
221
222 if (!proc_create_seq("atalk/route", 0444, init_net.proc_net,
223 &atalk_seq_route_ops));
224 goto out;
225
226 if (!proc_create_seq("atalk/socket", 0444, init_net.proc_net,
227 &atalk_seq_socket_ops));
228 goto out;
229
230 if (!proc_create_seq_private("atalk/arp", 0444, init_net.proc_net,
231 &aarp_seq_ops,
232 sizeof(struct aarp_iter_state), NULL));
233 goto out;
234
235 out:
236 remove_proc_subtree("atalk", init_net.proc_net);
237 return -ENOMEM;
238 }
239
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29122 bytes --]
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Russell King - ARM Linux admin @ 2019-02-27 23:27 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Heiner Kallweit, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227231940.GN3809@lunn.ch>
On Thu, Feb 28, 2019 at 12:19:40AM +0100, Andrew Lunn wrote:
> > >From what you've described, it sounds like what you actually have is:
> >
> > MAC <---> Serdes PHY <---> PHY
> >
> > The Serdes PHY receives the SGMII in-band negotiation from the external
> > PHY, but there is no propagation of the status from the serdes PHY to
> > the MAC.
>
> Yes, that is a good description. So far, we have not yet got the MAC
> to read the speed and duplex from the SERDES to configure itself. In
> theory it should be able to, it is all in the same device.
>
> It might be that once the SERDES interrupts saying it has link we need
> to program the MAC with the result of the in-band signalling. in-band
> then seems a bit pointless. Or we are missing some configuration
> somewhere to tell the MAC to use the in-band signalling result from
> the SERDES.
in-band doesn't become pointless if we can't read from the PHY at the
other end of the in-band link (for example, a Microtik copper SFP
module with an AR8033 on, which has no connectivity to the host other
than the serdes lane.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Heiner Kallweit @ 2019-02-27 23:25 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227225221.vubwqa6vg5odq26w@shell.armlinux.org.uk>
On 27.02.2019 23:52, Russell King - ARM Linux admin wrote:
> On Wed, Feb 27, 2019 at 10:25:54PM +0100, Heiner Kallweit wrote:
>> For the ones who don't know my story yet:
>> I have a Marvell 88E6390 switch with a Clause 45 PHY connected via
>> SGMII to port 9. For other reasons I limit the PHY to 100Mbps currently.
>> DT says: managed = "in-band-status"
>> Driver is mv8e6xxx + phylink.
>> Problem is that the link doesn't come up. Debugging resulted in:
>>
>> PHY establishes link to link partner (100Mbps, FD) and fires the
>> "link up" interrupt. The "link up" is also properly transmitted via
>> SGMII in-band signalling and fires the SERDES interrupt in mv8e6xxx.
>> Problem is that the in-band transmitted values for speed and duplex
>> don't show up anywhere. And phylink_resolve() doesn't even print
>> the link-up message.
>>
>> First question: Both, PHY and SERDES interrupt, try to do the same
>> thing: they eventually call phylink_run_resolve(). Is this correct?
>
> Correct - when the PHY interrupts, it triggers phylib to read the
> status and issue a callback via the link status hook into phylink,
> which stores the information from the PHY. That triggers a resolve.
> Even though the PHY reports that it has established link, the link
> may not yet be up.
>
> The Serdes is the MAC side of the link, and that should be calling
> phylink_mac_change(). That will re-run the resolve.
>
OK, then most of my headache was caused by the fundamental
misunderstanding of who's PHY and who's MAC.
> Each time the resolve is triggered in "in-band" mode, we expect to
> read the link status from the MAC side of the link, along with the
> speed and duplex. Since SGMII does not carry the flow control
> parameters, if a PHY is present, we merge the PHYs flow control
> information and pass that back to the MAC to configure the MAC's
> flow control to match.
>
>> I have some problems with understanding the code for MLO_AN_INBAND
>> in phylink_resolve(). Maybe also something is missing there for
>> proper in-band aneg support.
>
> Nope, it works with mvneta and mvpp2.
>
>> First we get the mac state (which is link down, 10Mbps, HD in my case).
>> Then the link state is calculated as "mac link up" && "phy link up".
>> This results in "link down", because mac link is down and phy link is
>> up. This logic isn't clear to me. How is the "link up" info supposed
>> to ever reach the mac?
>
> Via the in-band status, and the MAC detecting that the link is now up.
>
>> We're reading the port status, but IMO we want to set it based on the
>> auto-negotiated settings we get from the PHY.
>
> No we don't - in SGMII in-band mode, the PHY is optional, and even if
> it is present, if the MAC reports that the link is down even if the PHY
> reports that the link is up, then the link is _not_ up.
>
> If you want to use SGMII without in-band, then phylink supports that
> (which is PHY mode). If the MAC is unable to report these parameters,
> then in-band mode is not appropriate.
>
Now that it's clear that MAC is the SERDES PHY: I have these parameters
available in SGMII registers.
>> Later pause settings and possibly changed interface mode are
>> propagated to the mac. But no word about speed and duplex.
>>
>> Via "some callback" "some code" should read the in-band-transmitted
>> speed and duplex values from the SGMII port and use them to configure
>> the mac. Is this simply missing or do I miss something?
>
>>From what you've described, it sounds like what you actually have is:
>
> MAC <---> Serdes PHY <---> PHY
>
> The Serdes PHY receives the SGMII in-band negotiation from the external
> PHY, but there is no propagation of the status from the serdes PHY to
> the MAC. What's more is the Serdes PHY is hidden in mv88e6xxx setups.
>
That's exactly my setup. So what's missing is the status reporting
from SERDES PHY to actual MAC.
I could switch the port to forced mode and copy the parameters in the
SERDES "link-up" interrupt from the SGMII registers to the port forced
config. Hmm, would that be sufficient?
At least I could manage to get the link being reported as up. Whether
traffic flows I'll have to see. And I would have to see that I don't
break other usage of the SERDES port.
> This is a classic stacked PHY setup, one which we're now starting to
> encounter, and we need to support it - we have three cases I'm aware of
> now where we have:
>
> MAC <---> PHY <---> SFP
>
> and "SFP" can be another PHY, so we end up with:
>
> MAC <---> PHY <---> PHY
>
> and phylib does not support having two PHYs on the same net_device.
>
> What's more is that the need for an "attached_dev" net_device is pretty
> heavily embedded into phylib, so attaching one PHY to another PHY
> doesn't work very well.
>
> I've been tinkering with some patches to abstract out the "attached_dev"
> stuff, but in doing so I've walked into a whole load of issues in
> phy_attach_direct() and phy_detach() - it looks like phy_attach_direct()
> is lacking any form of locking while binding one of the generic drivers
> (it needs to hold the device lock while doing so.) The comments around
> device_release_driver() are vague about whether the parent device needs
> to be locked. Questions have been asked about that but so far I haven't
> had a response.
>
> We can replace a load of phydev->attached_dev != NULL tests (which are
> checking whether the device is attached to a netdev) fairly easily,
> but there's a load of places which do (phydev->attached_dev &&
> phydev->adjust_link) - and I fear that phylink (which doesn't set
> phydev->adjust_link) results in this code being broken.
>
Really appreciate the comprehensive explanation!
Heiner
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Andrew Lunn @ 2019-02-27 23:19 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Heiner Kallweit, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227225221.vubwqa6vg5odq26w@shell.armlinux.org.uk>
> >From what you've described, it sounds like what you actually have is:
>
> MAC <---> Serdes PHY <---> PHY
>
> The Serdes PHY receives the SGMII in-band negotiation from the external
> PHY, but there is no propagation of the status from the serdes PHY to
> the MAC.
Yes, that is a good description. So far, we have not yet got the MAC
to read the speed and duplex from the SERDES to configure itself. In
theory it should be able to, it is all in the same device.
It might be that once the SERDES interrupts saying it has link we need
to program the MAC with the result of the in-band signalling. in-band
then seems a bit pointless. Or we are missing some configuration
somewhere to tell the MAC to use the in-band signalling result from
the SERDES.
Andrew
^ permalink raw reply
* tc ebpf da failed to read the udp data when data length >= 215
From: IMBRIUS AGER @ 2019-02-27 23:12 UTC (permalink / raw)
To: netdev; +Cc: daniel, xiyou.wangcong, dcaratti, davem, jhs, jiri, vladbu
Hello:
I am having an odd issue.
I tried to read the udp data by tc ebpf da, however it worked only
when the data length <= 214.
As far as I know, data_end points to the end of the linear data,
that means the skb should be enough for more than 214 in general.
Any ideas about how to avoid the issue? Thanks!
# uname -a
Linux ip-10-0-0-2 4.15.0-1021-aws #21-Ubuntu SMP Tue Aug 28 10:23:07
UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
# tc -V
tc utility, iproute2-ss180129
Here is the test code:
#############################################################
ebpf_debug("level 4: %p - %p\n", udph, data_end);
__u32 *data32 = (__u32 *)(udph + 1);;
ebpf_debug("level 7: %p - %p\n", data32, data_end);
if (data32 + 1 > data_end)
return;
ebpf_debug("%u\n", data32[0]);
############################################################
Here is the test:
1) 10.0.0.3-me # nmap -sU -p 10000 10.0.0.2 --data-length 214
level 4: 00000000c6983314 - 00000000b11428cf
level 7: 000000002a31fcec - 00000000b11428cf
4036810185
2) 10.0.0.3-me # nmap -sU -p 10000 10.0.0.2 --data-length 215
level 4: 00000000ce4077ab - 00000000ce32b26c
level 7: 00000000ce32b26c - 00000000ce32b26c
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Cong Wang @ 2019-02-27 23:09 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbfy361i9uy.fsf@mellanox.com>
On Wed, Feb 27, 2019 at 6:28 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Tue 26 Feb 2019 at 22:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >>
> >> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >> >>
> >> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> >> >> at least once is unreliable. Some classifiers set 'stop' flag on error
> >> >> before calling walker callback, other classifiers used to call it with NULL
> >> >> filter pointer when empty. In order to prevent further regressions, extend
> >> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> >> >> tcf_walker->fn() implementation that is used to check if classifier has
> >> >> filters configured.
> >> >
> >> >
> >> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> >> > set arg->stop in fw_walk() when empty") can be reverted??
> >>
> >> Yes, it is safe now to revert following commits:
> >>
> >> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
> >> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
> >
> > Yeah, and probably commit d66022cd1623
> > ("net: sched: matchall: verify that filter is not NULL in mall_walk()").
> >
> > Please send a patch to revert them all.
> >
> > Thanks.
>
> I think commit d66022cd1623 ("net: sched: matchall: verify that filter
> is not NULL in mall_walk()") and commit 8b58d12f4ae1 ("net: sched:
> cgroup: verify that filter is not NULL during walk") shouldn't be
> reverted. They are still necessary to prevent tcf_chain_dump() from
> dumping NULL filter pointer. It can happen when dump is initiated in
> parallel with inserting first filter to unlocked classifier.
> tcf_fill_node() verifies that filter pointer is not NULL, so it will not
> crash, but will output tcf_proto info for second time. This might
> "confuse" user-space.
I don't get this.
First of all, what's confused here?
Secondly, if there is something confusing, isn't it all because of
your parallel algorithm? That is, the retry logic. I don't see how
commit d66022cd1623 could be useful in this context, it helps
to prevent a NULL crash which isn't a concern as long as it is
checked in tcf_fill_node() as you described.
Thanks.
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Rafael J. Wysocki @ 2019-02-27 23:03 UTC (permalink / raw)
To: Brian Norris
Cc: Ard Biesheuvel, Marc Zyngier, Ganapathi Bhat, Jeffy Chen,
Heiko Stuebner, Devicetree List, Xinming Hu,
<netdev@vger.kernel.org>, linux-pm,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
Amitkumar Karwar, linux-rockchip, Nishant Sarmukadam, Rob Herring,
Rafael J. Wysocki, linux-arm-kernel, Enric Balletbo i Serra,
Lorenzo Pieralisi, David S. Miller, Kalle Valo, Tony Lindgren,
Mark Rutland
In-Reply-To: <20190227205754.GF174696@google.com>
On Wed, Feb 27, 2019 at 9:58 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Ard,
>
> On Wed, Feb 27, 2019 at 11:16:12AM +0100, Ard Biesheuvel wrote:
> > On Wed, 27 Feb 2019 at 11:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > On 26/02/2019 23:28, Brian Norris wrote:
> > > > You're not the first person to notice this. All the motivations are not
> > > > necessarily painted clearly in their cover letter, but here are some
> > > > previous attempts at solving this problem:
> > > >
> > > > [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> > > > https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
> > > > http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
> > > >
> > > > As you can see by the 12th iteration, it wasn't left unsolved for lack
> > > > of trying...
> > >
> > > I wasn't aware of this. That's definitely a better approach than my
> > > hack, and I would really like this to be revived.
> > >
> >
> > I don't think this approach is entirely sound either.
>
> (I'm sure there may be problems with the above series. We probably
> should give it another shot though someday, as I think it's closer to
> the mark.)
>
> > From the side of the PCI device, WAKE# is just a GPIO line, and how it
> > is wired into the system is an entirely separate matter. So I don't
> > think it is justified to overload the notion of legacy interrupts with
> > some other pin that may behave in a way that is vaguely similar to how
> > a true wake-up capable interrupt works.
>
> I think you've conflated INTx with WAKE# just a bit (and to be fair,
> that's exactly what the bad binding we're trying to replace did,
> accidentally). We're not trying to claim this WAKE# signal replaces the
> typical PCI interrupts, but it *is* an interrupt in some sense --
> "depending on your definition of interrupt", per our IRC conversation ;)
>
> > So I'd argue that we should add an optional 'wake-gpio' DT property
> > instead to the generic PCI device binding, and leave the interrupt
> > binding and discovery alone.
>
> So I think Mark Rutland already shot that one down; it's conceptually an
> interrupt from the device's perspective.
Which device are you talking about? The one that signals wakeup? If
so, then I beg to differ.
On ACPI platforms WAKE# is represented as an ACPI GPE that is signaled
through SCI and handled at a different level (on HW-reduced ACPI it
actually can be a GPIO interrupt, but it still is handled with the
help of AML). The driver of the device signaling wakeup need not even
be aware that WAKE# has been asserted.
> We just need to figure out a good way of representing it that doesn't stomp on the existing INTx
> definitions.
WAKE# is a signal that is converted into an interrupt, but that
interrupt may arrive at some place your driver has nothing to do with.
It generally doesn't make sense to represent it as an interrupt for
the target device.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: Cong Wang @ 2019-02-27 23:03 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbf36oamsz3.fsf@mellanox.com>
On Tue, Feb 26, 2019 at 8:10 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Tue 26 Feb 2019 at 00:15, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Feb 25, 2019 at 7:45 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> Function tc_dump_chain() obtains and releases block->lock on each iteration
> >> of its inner loop that dumps all chains on block. Outputting chain template
> >> info is fast operation so locking/unlocking mutex multiple times is an
> >> overhead when lock is highly contested. Modify tc_dump_chain() to only
> >> obtain block->lock once and dump all chains without releasing it.
> >>
> >> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> >> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> >
> > Thanks for the followup!
> >
> > Isn't it similar for __tcf_get_next_proto() in tcf_chain_dump()?
> > And for tc_dump_tfilter()?
>
> Not really. These two dump all tp filters and not just a template, which
> is O(n) on number of filters and can be slow because it calls hw offload
> API for each of them. Our typical use-case involves periodic filter dump
> (to update stats) while multiple concurrent user-space threads are
> updating filters, so it is important for them to be able to execute in
> parallel.
Hmm, but if these are read-only, you probably don't even need a
mutex, you can just use RCU read lock to protect list iteration
and you still can grab the refcnt in the same way.
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Russell King - ARM Linux admin @ 2019-02-27 22:52 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <f507564b-7dc3-9449-ebb6-865eaeaa8a29@gmail.com>
On Wed, Feb 27, 2019 at 10:25:54PM +0100, Heiner Kallweit wrote:
> For the ones who don't know my story yet:
> I have a Marvell 88E6390 switch with a Clause 45 PHY connected via
> SGMII to port 9. For other reasons I limit the PHY to 100Mbps currently.
> DT says: managed = "in-band-status"
> Driver is mv8e6xxx + phylink.
> Problem is that the link doesn't come up. Debugging resulted in:
>
> PHY establishes link to link partner (100Mbps, FD) and fires the
> "link up" interrupt. The "link up" is also properly transmitted via
> SGMII in-band signalling and fires the SERDES interrupt in mv8e6xxx.
> Problem is that the in-band transmitted values for speed and duplex
> don't show up anywhere. And phylink_resolve() doesn't even print
> the link-up message.
>
> First question: Both, PHY and SERDES interrupt, try to do the same
> thing: they eventually call phylink_run_resolve(). Is this correct?
Correct - when the PHY interrupts, it triggers phylib to read the
status and issue a callback via the link status hook into phylink,
which stores the information from the PHY. That triggers a resolve.
Even though the PHY reports that it has established link, the link
may not yet be up.
The Serdes is the MAC side of the link, and that should be calling
phylink_mac_change(). That will re-run the resolve.
Each time the resolve is triggered in "in-band" mode, we expect to
read the link status from the MAC side of the link, along with the
speed and duplex. Since SGMII does not carry the flow control
parameters, if a PHY is present, we merge the PHYs flow control
information and pass that back to the MAC to configure the MAC's
flow control to match.
> I have some problems with understanding the code for MLO_AN_INBAND
> in phylink_resolve(). Maybe also something is missing there for
> proper in-band aneg support.
Nope, it works with mvneta and mvpp2.
> First we get the mac state (which is link down, 10Mbps, HD in my case).
> Then the link state is calculated as "mac link up" && "phy link up".
> This results in "link down", because mac link is down and phy link is
> up. This logic isn't clear to me. How is the "link up" info supposed
> to ever reach the mac?
Via the in-band status, and the MAC detecting that the link is now up.
> We're reading the port status, but IMO we want to set it based on the
> auto-negotiated settings we get from the PHY.
No we don't - in SGMII in-band mode, the PHY is optional, and even if
it is present, if the MAC reports that the link is down even if the PHY
reports that the link is up, then the link is _not_ up.
If you want to use SGMII without in-band, then phylink supports that
(which is PHY mode). If the MAC is unable to report these parameters,
then in-band mode is not appropriate.
> Later pause settings and possibly changed interface mode are
> propagated to the mac. But no word about speed and duplex.
>
> Via "some callback" "some code" should read the in-band-transmitted
> speed and duplex values from the SGMII port and use them to configure
> the mac. Is this simply missing or do I miss something?
From what you've described, it sounds like what you actually have is:
MAC <---> Serdes PHY <---> PHY
The Serdes PHY receives the SGMII in-band negotiation from the external
PHY, but there is no propagation of the status from the serdes PHY to
the MAC. What's more is the Serdes PHY is hidden in mv88e6xxx setups.
This is a classic stacked PHY setup, one which we're now starting to
encounter, and we need to support it - we have three cases I'm aware of
now where we have:
MAC <---> PHY <---> SFP
and "SFP" can be another PHY, so we end up with:
MAC <---> PHY <---> PHY
and phylib does not support having two PHYs on the same net_device.
What's more is that the need for an "attached_dev" net_device is pretty
heavily embedded into phylib, so attaching one PHY to another PHY
doesn't work very well.
I've been tinkering with some patches to abstract out the "attached_dev"
stuff, but in doing so I've walked into a whole load of issues in
phy_attach_direct() and phy_detach() - it looks like phy_attach_direct()
is lacking any form of locking while binding one of the generic drivers
(it needs to hold the device lock while doing so.) The comments around
device_release_driver() are vague about whether the parent device needs
to be locked. Questions have been asked about that but so far I haven't
had a response.
We can replace a load of phydev->attached_dev != NULL tests (which are
checking whether the device is attached to a netdev) fairly easily,
but there's a load of places which do (phydev->attached_dev &&
phydev->adjust_link) - and I fear that phylink (which doesn't set
phydev->adjust_link) results in this code being broken.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* [PATCH bpf-next 5/5] selftests/bpf: add btf_dedup test of FWD/STRUCT resolution
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
This patch adds a btf_dedup test exercising logic of STRUCT<->FWD
resolution and validating that STRUCT is not resolved to a FWD. It also
forces hash collisions, forcing both FWD and STRUCT to be candidates for
each other. Previously this condition caused infinite loop due to FWD
pointing to STRUCT and STRUCT pointing to its FWD.
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_btf.c | 45 ++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 1426c0a905c8..38797aa627a7 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -5731,6 +5731,51 @@ const struct btf_dedup_test dedup_tests[] = {
.dont_resolve_fwds = false,
},
},
+{
+ .descr = "dedup: struct <-> fwd resolution w/ hash collision",
+ /*
+ * // CU 1:
+ * struct x;
+ * struct s {
+ * struct x *x;
+ * };
+ * // CU 2:
+ * struct x {};
+ * struct s {
+ * struct x *x;
+ * };
+ */
+ .input = {
+ .raw_types = {
+ /* CU 1 */
+ BTF_FWD_ENC(NAME_TBD, 0 /* struct fwd */), /* [1] fwd x */
+ BTF_PTR_ENC(1), /* [2] ptr -> [1] */
+ BTF_STRUCT_ENC(NAME_TBD, 1, 8), /* [3] struct s */
+ BTF_MEMBER_ENC(NAME_TBD, 2, 0),
+ /* CU 2 */
+ BTF_STRUCT_ENC(NAME_TBD, 0, 0), /* [4] struct x */
+ BTF_PTR_ENC(4), /* [5] ptr -> [4] */
+ BTF_STRUCT_ENC(NAME_TBD, 1, 8), /* [6] struct s */
+ BTF_MEMBER_ENC(NAME_TBD, 5, 0),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0x\0s\0x\0x\0s\0x\0"),
+ },
+ .expect = {
+ .raw_types = {
+ BTF_PTR_ENC(3), /* [1] ptr -> [3] */
+ BTF_STRUCT_ENC(NAME_TBD, 1, 8), /* [2] struct s */
+ BTF_MEMBER_ENC(NAME_TBD, 1, 0),
+ BTF_STRUCT_ENC(NAME_NTH(2), 0, 0), /* [3] struct x */
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0s\0x"),
+ },
+ .opts = {
+ .dont_resolve_fwds = false,
+ .dedup_table_size = 1, /* force hash collisions */
+ },
+},
{
.descr = "dedup: all possible kinds (no duplicates)",
.input = {
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 4/5] btf: fix bug with resolving STRUCT/UNION into corresponding FWD
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
When checking available canonical candidates for struct/union algorithm
utilizes btf_dedup_is_equiv to determine if candidate is suitable. This
check is not enough when candidate is corresponding FWD for that
struct/union, because according to equivalence logic they are
equivalent. When it so happens that FWD and STRUCT/UNION end in hashing
to the same bucket, it's possible to create remapping loop from FWD to
STRUCT and STRUCT to same FWD, which will cause btf_dedup() to loop
forever.
This patch fixes the issue by additionally checking that type and
canonical candidate are strictly equal (utilizing btf_equal_struct).
Fixes: d5caef5b5655 ("btf: add BTF types deduplication algorithm")
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 6bbb710216e6..53db26d158c9 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -2255,7 +2255,7 @@ static void btf_dedup_merge_hypot_map(struct btf_dedup *d)
static int btf_dedup_struct_type(struct btf_dedup *d, __u32 type_id)
{
struct btf_dedup_node *cand_node;
- struct btf_type *t;
+ struct btf_type *cand_type, *t;
/* if we don't find equivalent type, then we are canonical */
__u32 new_id = type_id;
__u16 kind;
@@ -2275,6 +2275,10 @@ static int btf_dedup_struct_type(struct btf_dedup *d, __u32 type_id)
for_each_dedup_cand(d, h, cand_node) {
int eq;
+ cand_type = d->btf->types[cand_node->type_id];
+ if (!btf_equal_struct(t, cand_type))
+ continue;
+
btf_dedup_clear_hypot_map(d);
eq = btf_dedup_is_equiv(d, type_id, cand_node->type_id);
if (eq < 0)
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 1/5] selftests/bpf: fix btf_dedup testing code
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
btf_dedup testing code doesn't account for length of struct btf_header
when calculating the start of a string section. This patch fixes this
problem.
Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/test_btf.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e47168d1257d..3b74d23fffab 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -14,6 +14,7 @@ feature
test_libbpf_open
test_sock
test_sock_addr
+test_sock_fields
urandom_read
test_btf
test_sockmap
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 02d314383a9c..1426c0a905c8 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num)
}
test_hdr = test_btf_data;
- test_strs = test_btf_data + test_hdr->str_off;
+ test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off;
expect_hdr = expect_btf_data;
- expect_strs = expect_btf_data + expect_hdr->str_off;
+ expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off;
if (CHECK(test_hdr->str_len != expect_hdr->str_len,
"test_hdr->str_len:%u != expect_hdr->str_len:%u",
test_hdr->str_len, expect_hdr->str_len)) {
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 0/5] btf_dedup algorithm and test fixes
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
This patchset fixes a bug in btf_dedup() algorithm, which under specific hash
collision causes infinite loop. It also exposes ability to tune BTF
deduplication table size, with double purpose of allowing applications to
adjust size according to the size of BTF data, as well as allowing a simple way
to force hash collisions by setting table size to 1.
- Patch #1 fixes bug in btf_dedup testing code that's checking strings
- Patch #2 fixes pointer arg formatting in btf.h
- Patch #3 adds option to specify custom dedup table size
- Patch #4 fixes aforementioned bug in btf_dedup
- Patch #5 adds test that validates the fix
Andrii Nakryiko (5):
selftests/bpf: fix btf_dedup testing code
libbpf: fix formatting for btf_ext__get_raw_data
btf: allow to customize dedup hash table size
btf: fix bug with resolving STRUCT/UNION into corresponding FWD
selftests/bpf: add btf_dedup test of FWD/STRUCT resolution
tools/lib/bpf/btf.c | 49 ++++++++++++++++----------
tools/lib/bpf/btf.h | 3 +-
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/test_btf.c | 49 ++++++++++++++++++++++++--
4 files changed, 81 insertions(+), 21 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH bpf-next 3/5] btf: allow to customize dedup hash table size
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
Default size of dedup table (16k) is good enough for most binaries, even
typical vmlinux images. But there are cases of binaries with huge amount
of BTF types (e.g., allyesconfig variants of kernel), which benefit from
having bigger dedup table size to lower amount of unnecessary hash
collisions. Tools like pahole, thus, can tune this parameter to reach
optimal performance.
This change also serves double purpose of allowing tests to force hash
collisions to test some corner cases, used in follow up patch.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf.c | 43 ++++++++++++++++++++++++++-----------------
tools/lib/bpf/btf.h | 1 +
2 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 68b50e9bbde1..6bbb710216e6 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1070,8 +1070,7 @@ int btf__dedup(struct btf *btf, struct btf_ext *btf_ext,
return err;
}
-#define BTF_DEDUP_TABLE_SIZE_LOG 14
-#define BTF_DEDUP_TABLE_MOD ((1 << BTF_DEDUP_TABLE_SIZE_LOG) - 1)
+#define BTF_DEDUP_TABLE_DEFAULT_SIZE (1 << 14)
#define BTF_UNPROCESSED_ID ((__u32)-1)
#define BTF_IN_PROGRESS_ID ((__u32)-2)
@@ -1128,18 +1127,21 @@ static inline __u32 hash_combine(__u32 h, __u32 value)
#undef GOLDEN_RATIO_PRIME
}
-#define for_each_hash_node(table, hash, node) \
- for (node = table[hash & BTF_DEDUP_TABLE_MOD]; node; node = node->next)
+#define for_each_dedup_cand(d, hash, node) \
+ for (node = d->dedup_table[hash & (d->opts.dedup_table_size - 1)]; \
+ node; \
+ node = node->next)
static int btf_dedup_table_add(struct btf_dedup *d, __u32 hash, __u32 type_id)
{
struct btf_dedup_node *node = malloc(sizeof(struct btf_dedup_node));
+ int bucket = hash & (d->opts.dedup_table_size - 1);
if (!node)
return -ENOMEM;
node->type_id = type_id;
- node->next = d->dedup_table[hash & BTF_DEDUP_TABLE_MOD];
- d->dedup_table[hash & BTF_DEDUP_TABLE_MOD] = node;
+ node->next = d->dedup_table[bucket];
+ d->dedup_table[bucket] = node;
return 0;
}
@@ -1177,7 +1179,7 @@ static void btf_dedup_table_free(struct btf_dedup *d)
if (!d->dedup_table)
return;
- for (i = 0; i < (1 << BTF_DEDUP_TABLE_SIZE_LOG); i++) {
+ for (i = 0; i < d->opts.dedup_table_size; i++) {
while (d->dedup_table[i]) {
tmp = d->dedup_table[i];
d->dedup_table[i] = tmp->next;
@@ -1221,10 +1223,19 @@ static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
if (!d)
return ERR_PTR(-ENOMEM);
+ d->opts.dont_resolve_fwds = opts && opts->dont_resolve_fwds;
+ /* ensure table size is power of two and limit to 2G */
+ d->opts.dedup_table_size = opts && opts->dedup_table_size
+ ? opts->dedup_table_size
+ : BTF_DEDUP_TABLE_DEFAULT_SIZE;
+ for (i = 0; i < 31 && (1 << i) < d->opts.dedup_table_size; i++)
+ ;
+ d->opts.dedup_table_size = 1 << i;
+
d->btf = btf;
d->btf_ext = btf_ext;
- d->dedup_table = calloc(1 << BTF_DEDUP_TABLE_SIZE_LOG,
+ d->dedup_table = calloc(d->opts.dedup_table_size,
sizeof(struct btf_dedup_node *));
if (!d->dedup_table) {
err = -ENOMEM;
@@ -1249,8 +1260,6 @@ static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
for (i = 0; i <= btf->nr_types; i++)
d->hypot_map[i] = BTF_UNPROCESSED_ID;
- d->opts.dont_resolve_fwds = opts && opts->dont_resolve_fwds;
-
done:
if (err) {
btf_dedup_free(d);
@@ -1824,7 +1833,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_INT:
h = btf_hash_int(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_int(t, cand)) {
new_id = cand_node->type_id;
@@ -1835,7 +1844,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_ENUM:
h = btf_hash_enum(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_enum(t, cand)) {
new_id = cand_node->type_id;
@@ -1846,7 +1855,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_FWD:
h = btf_hash_common(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_common(t, cand)) {
new_id = cand_node->type_id;
@@ -2263,7 +2272,7 @@ static int btf_dedup_struct_type(struct btf_dedup *d, __u32 type_id)
return 0;
h = btf_hash_struct(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
int eq;
btf_dedup_clear_hypot_map(d);
@@ -2349,7 +2358,7 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
t->type = ref_type_id;
h = btf_hash_common(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_common(t, cand)) {
new_id = cand_node->type_id;
@@ -2372,7 +2381,7 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
info->index_type = ref_type_id;
h = btf_hash_array(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_array(t, cand)) {
new_id = cand_node->type_id;
@@ -2403,7 +2412,7 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
}
h = btf_hash_fnproto(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_fnproto(t, cand)) {
new_id = cand_node->type_id;
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index b60bb7cf5fff..28a1e1e59861 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -90,6 +90,7 @@ LIBBPF_API __u32 btf_ext__func_info_rec_size(const struct btf_ext *btf_ext);
LIBBPF_API __u32 btf_ext__line_info_rec_size(const struct btf_ext *btf_ext);
struct btf_dedup_opts {
+ unsigned int dedup_table_size;
bool dont_resolve_fwds;
};
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 2/5] libbpf: fix formatting for btf_ext__get_raw_data
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
Fix invalid formatting of pointer arg.
Fixes: ae4ab4b4117d ("btf: expose API to work with raw btf_ext data")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 94bbc249b0f1..b60bb7cf5fff 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -76,7 +76,7 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size);
LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
-LIBBPF_API const void *btf_ext__get_raw_data(const struct btf_ext* btf_ext,
+LIBBPF_API const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext,
__u32 *size);
LIBBPF_API int btf_ext__reloc_func_info(const struct btf *btf,
const struct btf_ext *btf_ext,
--
2.17.1
^ permalink raw reply related
* Re: [PATCH bpf-next 2/4] libbpf: Support 32-bit static data loads
From: Y Song @ 2019-02-27 22:42 UTC (permalink / raw)
To: Joe Stringer; +Cc: bpf, netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <CAH3MdRWrwMgjd7UT+7w=xCmEBD+mxv-YBqW+Q8zL8t5WvGTn8A@mail.gmail.com>
FYI.The latest llvm trunk will not emit errors for static variables. The patch
also gives detailed information how to relate a particular static
variable to a particular
relocation.
https://reviews.llvm.org/rL354954
Thanks,
Yonghong
On Fri, Feb 15, 2019 at 12:18 PM Y Song <ys114321@gmail.com> wrote:
>
> On Thu, Feb 14, 2019 at 11:16 PM Joe Stringer <joe@wand.net.nz> wrote:
> >
> > On Thu, 14 Feb 2019 at 21:39, Y Song <ys114321@gmail.com> wrote:
> > >
> > > On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@wand.net.nz> wrote:
> > > >
> > > > Support loads of static 32-bit data when BPF writers make use of
> > > > convenience macros for accessing static global data variables. A later
> > > > patch in this series will demonstrate its usage in a selftest.
> > > >
> > > > As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> > > > complain if this technique is attempted with data of other sizes:
> > > >
> > > > LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> > > > or check your static variable usage
> > >
> > > A little bit clarification from compiler side.
> > > The above compiler error is to prevent people use static variables since current
> > > kernel/libbpf does not handle this. The compiler only warns if .bss or
> > > .data section
> > > has more than one definitions. The first definition always has section offset 0
> > > and the compiler did not warn.
> >
> > Ah, interesting. I observed that warning when I attempted to define
> > global variables of multiple sizes, and I thought also with sizes
> > other than 32-bit. This clarifies things a bit, thanks.
> >
> > For the .bss my observation was that if you had a definition like:
> >
> > static int a = 0;
> >
> > Then this will be placed into .bss, hence why I looked into the
> > approach from this patch for patch 3 as well.
> >
> > > The restriction is a little strange. To only work with 32-bit data is
> > > not a right
> > > statement. The following are some examples.
> > >
> > > The following static variable definitions will succeed:
> > > static int a; /* one in .bss */
> > > static long b = 2; /* one in .data */
> > >
> > > The following definitions will fail as both in .bss.
> > > static int a;
> > > static int b;
> > >
> > > The following definitions will fail as both in .data:
> > > static char a = 2;
> > > static int b = 3;
> >
> > Are there type restrictions or something? I've been defining multiple
>
> There is no type restrictions.
> -bash-4.4$ cat g.c
> struct t {
> int a;
> char b;
> long c;
> };
> static volatile struct t v;
> int test() { return v.a + v.b; }
> -bash-4.4$ clang -O2 -g -c -target bpf g.c
> -bash-4.4$
>
> > static uint32_t and using them per the approach in this patch series
> > without hitting this compiler assertion.
>
> -bash-4.4$ cat g1.c
> static volatile int a;
> static volatile int b;
> int test() { return a + b; }
> -bash-4.4$ clang -O2 -g -c -target bpf g1.c
> fatal error: error in backend: Unsupported relocation: try to compile
> with -O2 or above, or check your static variable
> usage
> -bash-4.4$
>
> >
> > > Using global variables can prevent compiler errors.
> > > maps are defined as globals and the compiler does not
> > > check whether a particular global variable is defining a map or not.
> > >
> > > If you just use static variable like below
> > > static int a = 2;
> > > without potential assignment to a, the compiler will replace variable
> > > a with 2 at compile time.
> > > An alternative is to define like below
> > > static volatile int a = 2;
> > > You can get a "load" for variable "a" in the bpf load even if there is
> > > no assignment to a.
> >
> > I'll take a closer look at this too.
> >
> > > Maybe now is the time to remove the compiler assertions as
> > > libbpf/kernel starts to
> > > handle static variables?
> >
> > I don't understand why those assertions exists in this form. It
> > already allows code which will not load with libbpf (ie generate any
> > .data/.bss), does it help prevent unexpected situations for
> > developers?
>
> The error is introduced by the following llvm commit:
> commit 39184e407cd937f2f20d3f61eec205925bae7b13
> Author: Yonghong Song <yhs@fb.com>
> Date: Wed Aug 22 21:21:03 2018 +0000
>
> bpf: fix an assertion in BPFAsmBackend applyFixup()
>
> Fix bug https://bugs.llvm.org/show_bug.cgi?id=38643
>
> In BPFAsmBackend applyFixup(), there is an assertion for FixedValue to be 0.
> This may not be true, esp. for optimiation level 0.
> For example, in the above bug, for the following two
> static variables:
> @bpf_map_lookup_elem = internal global i8* (i8*, i8*)*
> inttoptr (i64 1 to i8* (i8*, i8*)*), align 8
> @bpf_map_update_elem = internal global i32 (i8*, i8*, i8*, i64)*
> inttoptr (i64 2 to i32 (i8*, i8*, i8*, i64)*), align 8
>
> The static variable @bpf_map_update_elem will have a symbol
> offset of 8 and a FK_SecRel_8 with FixupValue 8 will cause
> the assertion if llvm is built with -DLLVM_ENABLE_ASSERTIONS=ON.
>
> The above relocations will not exist if the program is compiled
> with optimization level -O1 and above as the compiler optimizes
> those static variables away. In the below error message, -O2
> is suggested as this is the common practice.
>
> Note that FixedValue = 0 in applyFixup() does exist and is valid,
> e.g., for the global variable my_map in the above bug. The bpf
> loader will process them properly for map_id's before loading
> the program into the kernel.
>
> The static variables, which are not optimized away by compiler,
> may have FK_SecRel_8 relocation with non-zero FixedValue.
>
> The patch removed the offending assertion and will issue
> a hard error as below if the FixedValue in applyFixup()
> is not 0.
> $ llc -march=bpf -filetype=obj fixup.ll
> LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> or check your static variable usage
>
> Its main purpose is to fix a behavior difference with and without
> -DLLVM_ENABLE_ASSERTIONS=ON. The patch generated an error
> regardless whether the compiler time assertion is turned on or not.
>
> It does not catch all the cases e.g., only one static variable is defined,
> which needs fine tuning as there are legitimate cases (e.g., in some dwarf
> sessions) where the Fixup is valid with FixedValue = 0.
>
> If you try to use more than onee static variables, the compiler will
> print an error and let you know your potential issues.
>
> The question is since we are on the path to allow static variables
> in the bpf programs for later patching, we probably should remove
> this compiler fatal error?
^ permalink raw reply
* Re: [PATCH net-next 2/8] devlink: add PF and VF port flavours
From: Jakub Kicinski @ 2019-02-27 22:42 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, oss-drivers, netdev
In-Reply-To: <20190227201727.GA2042@nanopsycho>
On Wed, 27 Feb 2019 21:17:27 +0100, Jiri Pirko wrote:
> Wed, Feb 27, 2019 at 06:23:26PM CET, jakub.kicinski@netronome.com wrote:
> >On Wed, 27 Feb 2019 13:41:35 +0100, Jiri Pirko wrote:
> >> Wed, Feb 27, 2019 at 01:23:27PM CET, jiri@resnulli.us wrote:
> >> >Tue, Feb 26, 2019 at 07:24:30PM CET, jakub.kicinski@netronome.com wrote:
> >> >>Current port flavours cover simple switches and DSA. Add PF
> >> >>and VF flavours to cover "switchdev" SR-IOV NICs.
> >> >>
> >> >>Example devlink user space output:
> >> >>
> >> >>$ devlink port
> >> >>pci/0000:82:00.0/0: type eth netdev p4p1 flavour physical
> >> >>pci/0000:82:00.0/10000: type eth netdev eth0 flavour pcie_pf pf 0
> >> >>pci/0000:82:00.0/10001: type eth netdev eth1 flavour pcie_vf pf 0 vf 0
> >> >>pci/0000:82:00.0/10002: type eth netdev eth2 flavour pcie_vf pf 0 vf 1
> >> >
> >> >Wait a second, howcome pf and vfs have the same PCI address?
> >>
> >> Oh, I think you have these as eswitch port representors. Confusing...
> >
> >FWIW I don't like the word representor, its a port. We don't call
> >physical ports "representors" even though from ASIC's point of view
> >they are exactly the same.
>
> My point is, they are not PFs and VFs. We have to find a way to clearly
> see what's what.
Okay, so let me explain the way I see it, and you can explain your way
or tell me where you disagree. Those devlink ports and netdevs are pf
ports and vf ports, which most refer to as "representor". If one sends
packets to the netdev indicated in DEVLINK_ATTR_PORT_NETDEV_*
attributes they will _egress_ the switch from that port. For physical
port that means going onto the Ethernet or IB wire. For PCIe it means
getting DMAed over the PCIe link to host memory.
There is a netdev construct on the host which is in charge of that
host memory. Maybe we shall call that host netdev?
(I said I don't like "representor" for the reason that people don't
refer to the physical port as "representor" even though it has exactly
the semantics we are following. This distinction between behaviour of
physical and PCI ports is what leads to confusion, I think.)
Let me bring out the moose :)
HOST A || HOST B
||
PF A | V | V | V | V || PF B | V | V | V
|*F |*F |*F |*F ... || |*F |*F |*F ...
*port A0 |*port A1 | 0 | 1 | 2 | 3 ||*port B0 |*port B1 | 0 | 1 | 2
||
PCI Express link || PCI Express link
\ \ \ | | | | | / / /
\ \ \ | | | | | / / /
/\ \______\______\'___|___|__________|_______'____/___/___/__ /\
|| |+PF0s0|+PF0s1 |+VF0|+VF1| ...| |+PF1s0|+PF1s1|+VF0|+VF1| ||
i || |------ ------ ----- ---- ----|--- ------ ------ ---- ----| || i
d n H || | <<========== | || d n H
e s O || | ==========>> | || e s O
v t S || | SR-IOV e-switch | || v t S
l a T || | <<========== | || l a T
i n || | ==========>> | || i n
n c A || | ________ _________ ________ | || n c B
k e || | |+Phys 0 |+Phys 1 |+Phys 2 | | || k e
|| \---------------------------------------------------------/ ||
\/ | | | \/
| | |
|| ||
MAC 0 || MAC 1 || MAC 2
|| ||
Things marked with + are devlink ports and have port (-repr-) netdevs
(including physical ports).
Things marked with * are host netdevs, don't have devlink ports.
^ 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