* Re: [PATCH bpf-next 1/5] samples: bpf: force IPv4 in ping
From: Andrii Nakryiko @ 2019-02-28 0:18 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-2-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> 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>
Acked-by: Andrii Nakryiko <andriin@fb.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
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Yonghong Song @ 2019-02-28 0:19 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <CAEf4BzbCd9ZWyunwEu78PCK14UA+kR0b7Xp8Lt46Xb2HqUdbQA@mail.gmail.com>
On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> 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?
-1 (-EPERM) probably not the best choice. The verifier already has
knowledge that a particular tracked map is an inner map or not. So
keeping the original error code (mostly -EINVAL) is preferred I think.
>
> 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?
Not exactly sure about the reason. Maybe with this patch, it can get
proper support. Not 100% sure.
>
>>
>> /* Misc members not needed in bpf_map_meta_equal() check. */
>> inner_map_meta->ops = inner_map->ops;
>> --
>> 2.17.1
>>
^ permalink raw reply
* Re: [PATCH bpf-next 2/5] samples: bpf: remove load_sock_ops in favour of bpftool
From: Andrii Nakryiko @ 2019-02-28 0:20 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-3-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> 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>
thanks!
Acked-by: Andrii Nakryiko <andriin@fb.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
* Re: [PATCH bpf-next 4/5] samples: bpf: use libbpf where easy
From: Jakub Kicinski @ 2019-02-28 0:21 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <CAEf4BzZsYnvQZTqB6g9RocZpuCMxZ+eCnLqBccerFNkQ8ReEFw@mail.gmail.com>
On Wed, 27 Feb 2019 16:05:45 -0800, Andrii Nakryiko wrote:
> > if (object) {
> > - assert(!load_bpf_file((char *)object));
> > - return prog_fd[0];
> > + assert(!bpf_prog_load(object, 0, &obj, &prog_fd));
>
> Here and in few more places below: is it possible to specify correct
> bpf_prog_type instead of 0?
Some of them yes, some of them no. There are objects with multiple
program types IIRC. This program type argument is only used for
checking if kernel versions are needed, and they are provided in
samples so it doesn't matter.
I'll try to add where possible.
^ permalink raw reply
* Re: [PATCH net-next 0/6] net: add individual virtual device filtering
From: Florian Fainelli @ 2019-02-28 0:23 UTC (permalink / raw)
To: Ivan Khoronzhuk, davem, grygorii.strashko
Cc: linux-omap, netdev, linux-kernel, jiri, ilias.apalodimas
In-Reply-To: <20190226184556.16082-1-ivan.khoronzhuk@linaro.org>
Hi Ivan,
On 2/26/19 10:45 AM, Ivan Khoronzhuk wrote:
> One of the reasons of this proposition is safety and performance -
> host should not receive traffic which is not designated for it.
>
> Some network devices can hold separate address tables for vlans and
> real device, but for some reason there is no possibility to apply it
> with generic net addressing scheme easily. At this moment the fastest
> solution is to add mcast/ucast entries for every created vlan
> including real device. But it not only consumes forwarding table but
> also adds holes in the filtering and thus wastes cpus cycles.
>
> This patchseries tries to correct core to assign mcast and ucast
> addresses only for vlans that really require it and as result an end
> driver can exclusively and simply set its rx filters. As an example
> it's implemented on cpsw TI driver, but generic changes provided by
> this series can be reused by other ethernet drivers having similar
> rx filter address possibilities.
>
> An address+vid is considered as separate address. The reserved device
> address length is 32 Bytes, for ethernet devices it's additional
> opportunity to pass auxiliary address info, like virtual ID
> identifying a device the address belongs to. This series makes it
> possible at least for ETH_P_8021Q.
>
> Thus end real device can setup separate tables for virtual devices
> just retrieving VID from the address. A device address space can
> maintain addresses and references on them separately for each virtual
> device if it needs so, or only addresses for real device (and all its
> vlans) it holds usually.
>
> A vlan device can be in any place of device chain upper real device,
> say smth like rdevice/bonding/vlan or even rdevice/macvlan/vlan.
>
> This series is verified on TI am572x EVM that can hold separate tables
> for vlans. Potentially it can be easily extended to netcp driver for
> keystone 2 boards (including k2g) and also new am6 chipsets. As a
> simple test case, different combinations of vlan+macvlan, macvlan+vlan
> were used and tested as with unicast as multicast addresses.
>
> Based on net-next/master
Thanks a lot for posting this patch series, I will take a look later
tonight.
>
> It's continuation of RFC:
>
> [RFC PATCH net-next 0/5] net: allow hw addresses for virtual device
> https://lkml.org/lkml/2018/12/3/817
>
> Ivan Khoronzhuk (6):
> net: core: dev_addr_lists: add VID to device address
> net: 8021q: vlan_dev: add vid tag to addresses of uc and mc lists
> net: 8021q: vlan_dev: add vid tag for vlan device own address
> ethernet: eth: add default vid len for all ehternet kind devices
> net: ethernet: ti: cpsw: update mc filtering to use IVDF
> net: ethernet: ti: cpsw: add macvlan and ucast/vlan filtering support
>
> drivers/net/ethernet/ti/Kconfig | 1 +
> drivers/net/ethernet/ti/cpsw.c | 139 ++++++++++++--------------------
> include/linux/if_vlan.h | 2 +
> include/linux/netdevice.h | 4 +
> net/8021q/Kconfig | 12 +++
> net/8021q/vlan.c | 3 +
> net/8021q/vlan.h | 2 +
> net/8021q/vlan_core.c | 25 ++++++
> net/8021q/vlan_dev.c | 103 ++++++++++++++++++-----
> net/core/dev_addr_lists.c | 124 ++++++++++++++++++++++------
> net/ethernet/eth.c | 10 ++-
> 11 files changed, 292 insertions(+), 133 deletions(-)
>
--
Florian
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Andrii Nakryiko @ 2019-02-28 0:28 UTC (permalink / raw)
To: Yonghong Song
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <e517e58e-a031-189f-658b-0138e3e22cc4@fb.com>
On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> > 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>
Acked-by: Andrii Nakryiko <andriin@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?
>
> -1 (-EPERM) probably not the best choice. The verifier already has
> knowledge that a particular tracked map is an inner map or not. So
> keeping the original error code (mostly -EINVAL) is preferred I think.
Ah, I actually missed the fact that verifier actually checks those
values (so it's not just >= 0 or < 0), so yeah, let's just pass
through. Btw, the value when there is no spinlock is actually -ENOENT.
>
> >
> > 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?
>
> Not exactly sure about the reason. Maybe with this patch, it can get
> proper support. Not 100% sure.
No, it won't, because bpf_map_meta_alloc explicitly tests for it:
if (map_value_has_spin_lock(inner_map)) {
fdput(f);
return ERR_PTR(-ENOTSUPP);
}
Maybe Alexei can clarify?
>
> >
> >>
> >> /* Misc members not needed in bpf_map_meta_equal() check. */
> >> inner_map_meta->ops = inner_map->ops;
> >> --
> >> 2.17.1
> >>
^ permalink raw reply
* [PATCH net-next] switchdev: Remove unused transaction item queue
From: Florian Fainelli @ 2019-02-28 0:29 UTC (permalink / raw)
To: netdev
Cc: Florian Fainelli, David S. Miller, Ido Schimmel, open list,
open list:STAGING SUBSYSTEM, moderated list:ETHERNET BRIDGE, jiri,
andrew, vivien.didelot
There are no more in tree users of the
switchdev_trans_item_{dequeue,enqueue} or switchdev_trans_item structure
in the kernel since commit 00fc0c51e35b ("rocker: Change world_ops API
and implementation to be switchdev independant").
Remove this unused code and update the documentation accordingly since.
Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
Documentation/networking/switchdev.txt | 19 -----
include/net/switchdev.h | 12 ---
net/switchdev/switchdev.c | 100 +------------------------
3 files changed, 2 insertions(+), 129 deletions(-)
diff --git a/Documentation/networking/switchdev.txt b/Documentation/networking/switchdev.txt
index 633dd1fd81b7..86174ce8cd13 100644
--- a/Documentation/networking/switchdev.txt
+++ b/Documentation/networking/switchdev.txt
@@ -371,22 +371,3 @@ The driver can monitor for updates to arp_tbl using the netevent notifier
NETEVENT_NEIGH_UPDATE. The device can be programmed with resolved nexthops
for the routes as arp_tbl updates. The driver implements ndo_neigh_destroy
to know when arp_tbl neighbor entries are purged from the port.
-
-Transaction item queue
-^^^^^^^^^^^^^^^^^^^^^^
-
-For switchdev ops attr_set and obj_add, there is a 2 phase transaction model
-used. First phase is to "prepare" anything needed, including various checks,
-memory allocation, etc. The goal is to handle the stuff that is not unlikely
-to fail here. The second phase is to "commit" the actual changes.
-
-Switchdev provides an infrastructure for sharing items (for example memory
-allocations) between the two phases.
-
-The object created by a driver in "prepare" phase and it is queued up by:
-switchdev_trans_item_enqueue()
-During the "commit" phase, the driver gets the object by:
-switchdev_trans_item_dequeue()
-
-If a transaction is aborted during "prepare" phase, switchdev code will handle
-cleanup of the queued-up objects.
diff --git a/include/net/switchdev.h b/include/net/switchdev.h
index e4f751e19ecf..0ebd67ae7012 100644
--- a/include/net/switchdev.h
+++ b/include/net/switchdev.h
@@ -20,14 +20,7 @@
#define SWITCHDEV_F_SKIP_EOPNOTSUPP BIT(1)
#define SWITCHDEV_F_DEFER BIT(2)
-struct switchdev_trans_item {
- struct list_head list;
- void *data;
- void (*destructor)(const void *data);
-};
-
struct switchdev_trans {
- struct list_head item_list;
bool ph_prepare;
};
@@ -105,11 +98,6 @@ struct switchdev_obj_port_mdb {
#define SWITCHDEV_OBJ_PORT_MDB(OBJ) \
container_of((OBJ), struct switchdev_obj_port_mdb, obj)
-void switchdev_trans_item_enqueue(struct switchdev_trans *trans,
- void *data, void (*destructor)(void const *),
- struct switchdev_trans_item *tritem);
-void *switchdev_trans_item_dequeue(struct switchdev_trans *trans);
-
typedef int switchdev_obj_dump_cb_t(struct switchdev_obj *obj);
enum switchdev_notifier_type {
diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
index d81cfcee9ad9..90ba4a1f0a6d 100644
--- a/net/switchdev/switchdev.c
+++ b/net/switchdev/switchdev.c
@@ -23,78 +23,6 @@
#include <linux/rtnetlink.h>
#include <net/switchdev.h>
-/**
- * switchdev_trans_item_enqueue - Enqueue data item to transaction queue
- *
- * @trans: transaction
- * @data: pointer to data being queued
- * @destructor: data destructor
- * @tritem: transaction item being queued
- *
- * Enqeueue data item to transaction queue. tritem is typically placed in
- * cointainter pointed at by data pointer. Destructor is called on
- * transaction abort and after successful commit phase in case
- * the caller did not dequeue the item before.
- */
-void switchdev_trans_item_enqueue(struct switchdev_trans *trans,
- void *data, void (*destructor)(void const *),
- struct switchdev_trans_item *tritem)
-{
- tritem->data = data;
- tritem->destructor = destructor;
- list_add_tail(&tritem->list, &trans->item_list);
-}
-EXPORT_SYMBOL_GPL(switchdev_trans_item_enqueue);
-
-static struct switchdev_trans_item *
-__switchdev_trans_item_dequeue(struct switchdev_trans *trans)
-{
- struct switchdev_trans_item *tritem;
-
- if (list_empty(&trans->item_list))
- return NULL;
- tritem = list_first_entry(&trans->item_list,
- struct switchdev_trans_item, list);
- list_del(&tritem->list);
- return tritem;
-}
-
-/**
- * switchdev_trans_item_dequeue - Dequeue data item from transaction queue
- *
- * @trans: transaction
- */
-void *switchdev_trans_item_dequeue(struct switchdev_trans *trans)
-{
- struct switchdev_trans_item *tritem;
-
- tritem = __switchdev_trans_item_dequeue(trans);
- BUG_ON(!tritem);
- return tritem->data;
-}
-EXPORT_SYMBOL_GPL(switchdev_trans_item_dequeue);
-
-static void switchdev_trans_init(struct switchdev_trans *trans)
-{
- INIT_LIST_HEAD(&trans->item_list);
-}
-
-static void switchdev_trans_items_destroy(struct switchdev_trans *trans)
-{
- struct switchdev_trans_item *tritem;
-
- while ((tritem = __switchdev_trans_item_dequeue(trans)))
- tritem->destructor(tritem->data);
-}
-
-static void switchdev_trans_items_warn_destroy(struct net_device *dev,
- struct switchdev_trans *trans)
-{
- WARN(!list_empty(&trans->item_list), "%s: transaction item queue is not empty.\n",
- dev->name);
- switchdev_trans_items_destroy(trans);
-}
-
static LIST_HEAD(deferred);
static DEFINE_SPINLOCK(deferred_lock);
@@ -208,8 +136,6 @@ static int switchdev_port_attr_set_now(struct net_device *dev,
struct switchdev_trans trans;
int err;
- switchdev_trans_init(&trans);
-
/* Phase I: prepare for attr set. Driver/device should fail
* here if there are going to be issues in the commit phase,
* such as lack of resources or support. The driver/device
@@ -220,17 +146,8 @@ static int switchdev_port_attr_set_now(struct net_device *dev,
trans.ph_prepare = true;
err = switchdev_port_attr_notify(SWITCHDEV_PORT_ATTR_SET, dev, attr,
&trans);
- if (err) {
- /* Prepare phase failed: abort the transaction. Any
- * resources reserved in the prepare phase are
- * released.
- */
-
- if (err != -EOPNOTSUPP)
- switchdev_trans_items_destroy(&trans);
-
+ if (err)
return err;
- }
/* Phase II: commit attr set. This cannot fail as a fault
* of driver/device. If it does, it's a bug in the driver/device
@@ -242,7 +159,6 @@ static int switchdev_port_attr_set_now(struct net_device *dev,
&trans);
WARN(err, "%s: Commit of attribute (id=%d) failed.\n",
dev->name, attr->id);
- switchdev_trans_items_warn_destroy(dev, &trans);
return err;
}
@@ -341,8 +257,6 @@ static int switchdev_port_obj_add_now(struct net_device *dev,
ASSERT_RTNL();
- switchdev_trans_init(&trans);
-
/* Phase I: prepare for obj add. Driver/device should fail
* here if there are going to be issues in the commit phase,
* such as lack of resources or support. The driver/device
@@ -353,17 +267,8 @@ static int switchdev_port_obj_add_now(struct net_device *dev,
trans.ph_prepare = true;
err = switchdev_port_obj_notify(SWITCHDEV_PORT_OBJ_ADD,
dev, obj, &trans, extack);
- if (err) {
- /* Prepare phase failed: abort the transaction. Any
- * resources reserved in the prepare phase are
- * released.
- */
-
- if (err != -EOPNOTSUPP)
- switchdev_trans_items_destroy(&trans);
-
+ if (err)
return err;
- }
/* Phase II: commit obj add. This cannot fail as a fault
* of driver/device. If it does, it's a bug in the driver/device
@@ -374,7 +279,6 @@ static int switchdev_port_obj_add_now(struct net_device *dev,
err = switchdev_port_obj_notify(SWITCHDEV_PORT_OBJ_ADD,
dev, obj, &trans, extack);
WARN(err, "%s: Commit of object (id=%d) failed.\n", dev->name, obj->id);
- switchdev_trans_items_warn_destroy(dev, &trans);
return err;
}
--
2.17.1
^ permalink raw reply related
* 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-28 0:38 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: <20190227184601-mutt-send-email-mst@kernel.org>
On 2/27/2019 3:50 PM, Michael S. Tsirkin wrote:
> On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
>>
>> 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?
> That's the idea.
>
>> 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)?
> Yes the idea would be to fail renaming slaves.
No I was asking about the userspace expectation: whether it should track
and detect the lifecycle events of failover slaves and decide what to
do. How does it get back to the user specified name if VF is not
enslaved (say someone unloads the virtio-net module)?
As this scheme adds much complexity to the kernel naming convention
(currently it's just ethX names) that no userspace can understand. Will
the change break userspace further?
-Siwei
>
>> 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
> With this approach kernel will deny attempts by userspace to rename
> slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> will rename both slaves.
>
> It seems pretty solid to me, the only issue is that in theory userspace
> can use a name like XXXnsby for something else. But this seems unlikely.
>
>
>>>> 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
* 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: Michael S. Tsirkin @ 2019-02-28 0:38 UTC (permalink / raw)
To: Stephen Hemminger
Cc: si-wei liu, Samudrala, Sridhar, Siwei Liu, Jiri Pirko,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <20190227160342.788dc2b4@shemminger-XPS-13-9360>
On Wed, Feb 27, 2019 at 04:03:42PM -0800, Stephen Hemminger wrote:
> > With this approach kernel will deny attempts by userspace to rename
> > slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> > will rename both slaves.
> >
> > It seems pretty solid to me, the only issue is that in theory userspace
> > can use a name like XXXnsby for something else. But this seems unlikely.
>
> Similar schemes (with kernel providing naming) were also previously rejected
> upstream.
Links?
I'm inclined to try and see what happens.
> It has been a consistent theme that the kernel should not be in
> the renaming business.
In this case it's not in renaming business per se. The only reason
we even have the original name is due to the ways internal APIs
work. You can look at it as simply having slaves names being
part of master.
> It will certainly break userspace.
That's a strong claim. What is it based on? It so happens that
userspace renaming slaves is already broken on virtio. So we can fix it
any way we like :)
And yes it won't help netvsc because netvsc wants compatibility with old
scripts but then netvsc uses a 2 device model anyway.
--
MST
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Yonghong Song @ 2019-02-28 0:40 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <CAEf4BzZE8W7_VnoPTBftMUjuhVOTFmfWZS8kihPFjqZ+tP5N6w@mail.gmail.com>
On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@fb.com> wrote:
>>
>>
>>
>> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
>>> 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>
>
> Acked-by: Andrii Nakryiko <andriin@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?
>>
>> -1 (-EPERM) probably not the best choice. The verifier already has
>> knowledge that a particular tracked map is an inner map or not. So
>> keeping the original error code (mostly -EINVAL) is preferred I think.
>
> Ah, I actually missed the fact that verifier actually checks those
> values (so it's not just >= 0 or < 0), so yeah, let's just pass
> through. Btw, the value when there is no spinlock is actually -ENOENT.
If there is no BTF, it will be -EINVAL. If there is BTF and no spinlock
member, mostly -ENOENT.
>
>>
>>>
>>> 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?
>>
>> Not exactly sure about the reason. Maybe with this patch, it can get
>> proper support. Not 100% sure.
>
> No, it won't, because bpf_map_meta_alloc explicitly tests for it:
>
> if (map_value_has_spin_lock(inner_map)) {
> fdput(f);
> return ERR_PTR(-ENOTSUPP);
> }
I mean that this can be removed after my patch and it may work :-)
>
> Maybe Alexei can clarify?
>
>
>>
>>>
>>>>
>>>> /* 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: Michael S. Tsirkin @ 2019-02-28 0:41 UTC (permalink / raw)
To: si-wei liu
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: <a617ce13-4114-469d-ef33-a1c91150eeca@oracle.com>
On Wed, Feb 27, 2019 at 04:38:00PM -0800, si-wei liu wrote:
>
>
> On 2/27/2019 3:50 PM, Michael S. Tsirkin wrote:
> > On Wed, Feb 27, 2019 at 03:34:56PM -0800, si-wei liu wrote:
> > >
> > > 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?
> > That's the idea.
> >
> > > 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)?
> > Yes the idea would be to fail renaming slaves.
> No I was asking about the userspace expectation: whether it should track and
> detect the lifecycle events of failover slaves and decide what to do. How
> does it get back to the user specified name if VF is not enslaved (say
> someone unloads the virtio-net module)?
When virtio net is removed VF will shortly be removed too.
> As this scheme adds much complexity to the kernel naming convention
> (currently it's just ethX names) that no userspace can understand.
Anything that pokes at slaves needs to be specially designed anyway.
Naming seems like a minor issue.
> Will the
> change break userspace further?
>
> -Siwei
Didn't you show userspace is already broken. You can't "further
break it", rename already fails.
> >
> > > 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
> > With this approach kernel will deny attempts by userspace to rename
> > slaves. Slaves will always be named XXXnsby and XXnpry. Master renames
> > will rename both slaves.
> >
> > It seems pretty solid to me, the only issue is that in theory userspace
> > can use a name like XXXnsby for something else. But this seems unlikely.
> >
> >
> > > > > 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
* Re: [PATCH 3/8] tlan: use pci_zalloc instead of pci_alloc
From: Robert Eshleman @ 2019-02-28 0:41 UTC (permalink / raw)
To: Joe Perches
Cc: Tariq Toukan, David S. Miller, Samuel Chessman, netdev,
linux-rdma, linux-kernel
In-Reply-To: <ce11feb21863662cf2ad96f0f7b69f0edcabfcf7.camel@perches.com>
On Tue, Feb 26, 2019 at 10:22:06PM -0800, Joe Perches wrote:
> On Tue, 2019-02-26 at 22:09 -0800, Robert Eshleman wrote:
> > This patch replaces a pci_alloc_consistent and memset(,0) call
> > with a single call to pci_zalloc_consistent.
> []
> > diff --git a/drivers/net/ethernet/ti/tlan.c b/drivers/net/ethernet/ti/tlan.c
> []
> > @@ -845,17 +845,16 @@ static int tlan_init(struct net_device *dev)
> >
> > dma_size = (TLAN_NUM_RX_LISTS + TLAN_NUM_TX_LISTS)
> > * (sizeof(struct tlan_list));
> > - priv->dma_storage = pci_alloc_consistent(priv->pci_dev,
> > - dma_size,
> > - &priv->dma_storage_dma);
> > + priv->dma_storage = pci_zalloc_consistent(priv->pci_dev,
> > + dma_size,
> > + &priv->dma_storage_dma);
> > priv->dma_size = dma_size;
> >
> > - if (priv->dma_storage == NULL) {
> > + if (!priv->dma_storage) {
> > pr_err("Could not allocate lists and buffers for %s\n",
> > dev->name);
>
> unrelated trivia:
>
> This pr_err (and likely others in this file)
> could be replace by netdev_err
>
Definitely good to know (I may make that change too). Thanks Joe.
-Bobby
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Andrii Nakryiko @ 2019-02-28 0:43 UTC (permalink / raw)
To: Yonghong Song
Cc: netdev@vger.kernel.org, Alexei Starovoitov, Daniel Borkmann,
Kernel Team
In-Reply-To: <e02db95d-107d-79b4-653c-c3a4436b7376@fb.com>
On Wed, Feb 27, 2019 at 4:40 PM Yonghong Song <yhs@fb.com> wrote:
>
>
>
> On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> > On Wed, Feb 27, 2019 at 4:19 PM Yonghong Song <yhs@fb.com> wrote:
> >>
> >>
> >>
> >> On 2/27/19 3:34 PM, Andrii Nakryiko wrote:
> >>> 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>
> >
> > Acked-by: Andrii Nakryiko <andriin@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?
> >>
> >> -1 (-EPERM) probably not the best choice. The verifier already has
> >> knowledge that a particular tracked map is an inner map or not. So
> >> keeping the original error code (mostly -EINVAL) is preferred I think.
> >
> > Ah, I actually missed the fact that verifier actually checks those
> > values (so it's not just >= 0 or < 0), so yeah, let's just pass
> > through. Btw, the value when there is no spinlock is actually -ENOENT.
>
> If there is no BTF, it will be -EINVAL. If there is BTF and no spinlock
> member, mostly -ENOENT.
You are right, I stand corrected. In both cases the effect should be
the same (no way to use spinlock).
>
> >
> >>
> >>>
> >>> 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?
> >>
> >> Not exactly sure about the reason. Maybe with this patch, it can get
> >> proper support. Not 100% sure.
> >
> > No, it won't, because bpf_map_meta_alloc explicitly tests for it:
> >
> > if (map_value_has_spin_lock(inner_map)) {
> > fdput(f);
> > return ERR_PTR(-ENOTSUPP);
> > }
>
> I mean that this can be removed after my patch and it may work :-)
Ah, got it, yeah, maybe.
>
> >
> > Maybe Alexei can clarify?
> >
> >
> >>
> >>>
> >>>>
> >>>> /* Misc members not needed in bpf_map_meta_equal() check. */
> >>>> inner_map_meta->ops = inner_map->ops;
> >>>> --
> >>>> 2.17.1
> >>>>
^ permalink raw reply
* Re: [PATCH 7/8] net: seeq: replace kmalloc / memset(,0) with kzalloc
From: Robert Eshleman @ 2019-02-28 0:48 UTC (permalink / raw)
To: Robert Eshleman
Cc: Tariq Toukan, David S. Miller, Russell King, netdev, linux-rdma,
linux-kernel, linux-arm-kernel
In-Reply-To: <5ff7bcf7b8db8daa129703e5f7b632825f25c766.1551246708.git.bobbyeshleman@gmail.com>
On Tue, Feb 26, 2019 at 10:09:54PM -0800, Robert Eshleman wrote:
> This patch reduces a call to memset(,0) by replacing
> a kmalloc call with a kzalloc call.
>
> Signed-off-by: Robert Eshleman <bobbyeshleman@gmail.com>
> ---
> drivers/net/ethernet/seeq/ether3.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/seeq/ether3.c b/drivers/net/ethernet/seeq/ether3.c
> index d1bb73bf9914..7456cf08a48f 100644
> --- a/drivers/net/ethernet/seeq/ether3.c
> +++ b/drivers/net/ethernet/seeq/ether3.c
> @@ -223,7 +223,7 @@ ether3_addr(char *addr, struct expansion_card *ec)
> static int
> ether3_ramtest(struct net_device *dev, unsigned char byte)
> {
> - unsigned char *buffer = kmalloc(RX_END, GFP_KERNEL);
> + unsigned char *buffer = kzalloc(RX_END, GFP_KERNEL);
> int i,ret = 0;
> int max_errors = 4;
> int bad = -1;
> @@ -231,7 +231,6 @@ ether3_ramtest(struct net_device *dev, unsigned char byte)
> if (!buffer)
> return 1;
>
> - memset(buffer, byte, RX_END);
Byte is not zero, so the intent of this patch does not apply here.
Dropping this from the patch set in v2.
> ether3_setbuffer(dev, buffer_write, 0);
> ether3_writebuffer(dev, buffer, TX_END);
> ether3_setbuffer(dev, buffer_write, RX_START);
> --
> 2.20.1
>
^ permalink raw reply
* Re: [PATCH net-next 01/12] net: sched: flower: don't check for rtnl on head dereference
From: Cong Wang @ 2019-02-28 0:49 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbf5zt6mwby.fsf@mellanox.com>
On Tue, Feb 26, 2019 at 6:57 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Mon 25 Feb 2019 at 22:39, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Feb 25, 2019 at 8:11 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >>
> >> On Fri 22 Feb 2019 at 19:32, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> >
> >> > So if it is no longer RCU any more, why do you still use
> >> > rcu_dereference_protected()? That is, why not just deref it as a raw
> >> > pointer?
> >
> >
> > Any answer for this question?
>
> I decided that since there is neither possibility of concurrent pointer
> assignment nor deallocation of object that it points to, most performant
> solution would be using rcu_dereference_protected() which is the only
> RCU dereference helper that doesn't use READ_ONCE. I now understand that
> this is confusing (and most likely doesn't provide any noticeable
> performance improvement anyway!) and will change this patch to use
> rcu_dereference_raw() as you suggest.
Yeah, please make sure sparse is happy with that. :)
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Alexei Starovoitov @ 2019-02-28 0:56 UTC (permalink / raw)
To: Andrii Nakryiko, Yonghong Song
Cc: netdev@vger.kernel.org, Daniel Borkmann, Kernel Team
In-Reply-To: <CAEf4BzZE8W7_VnoPTBftMUjuhVOTFmfWZS8kihPFjqZ+tP5N6w@mail.gmail.com>
On 2/27/19 4:28 PM, Andrii Nakryiko wrote:
> No, it won't, because bpf_map_meta_alloc explicitly tests for it:
>
> if (map_value_has_spin_lock(inner_map)) {
> fdput(f);
> return ERR_PTR(-ENOTSUPP);
> }
>
> Maybe Alexei can clarify?
inner map can be replaced while progs are running.
I don't think added complexity is worth it.
Hence it's disallowed for time being.
^ 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: Jakub Kicinski @ 2019-02-28 0:52 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: si-wei liu, Samudrala, Sridhar, Siwei Liu, Jiri Pirko,
Stephen Hemminger, David Miller, Netdev, virtualization,
virtio-dev, Brandeburg, Jesse, Alexander Duyck, Jason Wang,
liran.alon
In-Reply-To: <20190227193923-mutt-send-email-mst@kernel.org>
On Wed, 27 Feb 2019 19:41:32 -0500, Michael S. Tsirkin wrote:
> > As this scheme adds much complexity to the kernel naming convention
> > (currently it's just ethX names) that no userspace can understand.
>
> Anything that pokes at slaves needs to be specially designed anyway.
> Naming seems like a minor issue.
Can the users who care about the naming put net_failover into
"user space will do the bond enslavement" mode, and do the bond
creation/management themselves from user space (in systemd/
Network Manager) based on the failover flag?
^ permalink raw reply
* [PATCH RFC v3 0/5] SCTP: Event skb list overhaul.
From: David Miller @ 2019-02-28 0:59 UTC (permalink / raw)
To: netdev; +Cc: marcelo.leitner, lucien.xin, nhorman
This patch series eliminates the explicit reference to the skb list
implementation via skb->prev dereferences.
The approach used is to pass a non-empty skb list around instead of an
event skb object which may or may not be on a list.
I'd like to thank Marcelo Leitner, Xin Long, and Neil Horman for
reviewing previous versions of this series.
Testing would be very much appreciated, in addition to the review of
course.
Signed-off-by: David S. Miller <davem@davemloft.net>
^ permalink raw reply
* [PATCH RFC v3 1/5] sctp: Remove superfluous test in sctp_ulpq_reasm_drain().
From: David Miller @ 2019-02-28 1:00 UTC (permalink / raw)
To: netdev; +Cc: marcelo.leitner, lucien.xin, nhorman
Inside the loop, we always start with event non-NULL.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/sctp/ulpqueue.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 5dde92101743..0fecc1fb4ab7 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -745,7 +745,7 @@ static void sctp_ulpq_reasm_drain(struct sctp_ulpq *ulpq)
while ((event = sctp_ulpq_retrieve_reassembled(ulpq)) != NULL) {
/* Do ordering if needed. */
- if ((event) && (event->msg_flags & MSG_EOR)) {
+ if (event->msg_flags & MSG_EOR) {
skb_queue_head_init(&temp);
__skb_queue_tail(&temp, sctp_event2skb(event));
--
2.20.1
^ permalink raw reply related
* [PATCH RFC v3 2/5] sctp: Always pass skbs on a list to sctp_ulpq_tail_event().
From: David Miller @ 2019-02-28 1:00 UTC (permalink / raw)
To: netdev; +Cc: marcelo.leitner, lucien.xin, nhorman
This way we can simplify the logic and remove assumptions
about the implementation of skb lists.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/sctp/ulpqueue.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index 0fecc1fb4ab7..b22f558adc49 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -738,19 +738,19 @@ void sctp_ulpq_reasm_flushtsn(struct sctp_ulpq *ulpq, __u32 fwd_tsn)
static void sctp_ulpq_reasm_drain(struct sctp_ulpq *ulpq)
{
struct sctp_ulpevent *event = NULL;
- struct sk_buff_head temp;
if (skb_queue_empty(&ulpq->reasm))
return;
while ((event = sctp_ulpq_retrieve_reassembled(ulpq)) != NULL) {
- /* Do ordering if needed. */
- if (event->msg_flags & MSG_EOR) {
- skb_queue_head_init(&temp);
- __skb_queue_tail(&temp, sctp_event2skb(event));
+ struct sk_buff_head temp;
+
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
+ /* Do ordering if needed. */
+ if (event->msg_flags & MSG_EOR)
event = sctp_ulpq_order(ulpq, event);
- }
/* Send event to the ULP. 'event' is the
* sctp_ulpevent for very first SKB on the temp' list.
@@ -1082,6 +1082,10 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
event = sctp_ulpq_retrieve_first(ulpq);
/* Send event to the ULP. */
if (event) {
+ struct sk_buff_head temp;
+
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
sctp_ulpq_tail_event(ulpq, event);
sctp_ulpq_set_pd(ulpq);
return;
--
2.20.1
^ permalink raw reply related
* [PATCH RFC v3 3/5] sctp: Use helper for sctp_ulpq_tail_event() when hooked up to ->enqueue_event
From: David Miller @ 2019-02-28 1:00 UTC (permalink / raw)
To: netdev; +Cc: marcelo.leitner, lucien.xin, nhorman
This way we can make sure events sent this way to
sctp_ulpq_tail_event() are on a list as well. Now all such code paths
are fully covered.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/sctp/stream_interleave.c | 11 ++++++++++-
1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index a6bf21579466..b6b251b8b3cf 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1298,6 +1298,15 @@ static void sctp_handle_iftsn(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk)
ntohl(skip->mid), skip->flags);
}
+static int do_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *event)
+{
+ struct sk_buff_head temp;
+
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
+ return sctp_ulpq_tail_event(ulpq, event);
+}
+
static struct sctp_stream_interleave sctp_stream_interleave_0 = {
.data_chunk_len = sizeof(struct sctp_data_chunk),
.ftsn_chunk_len = sizeof(struct sctp_fwdtsn_chunk),
@@ -1306,7 +1315,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_0 = {
.assign_number = sctp_chunk_assign_ssn,
.validate_data = sctp_validate_data,
.ulpevent_data = sctp_ulpq_tail_data,
- .enqueue_event = sctp_ulpq_tail_event,
+ .enqueue_event = do_ulpq_tail_event,
.renege_events = sctp_ulpq_renege,
.start_pd = sctp_ulpq_partial_delivery,
.abort_pd = sctp_ulpq_abort_pd,
--
2.20.1
^ permalink raw reply related
* [PATCH RFC v3 4/5] sctp: Make sctp_enqueue_event tak an skb list.
From: David Miller @ 2019-02-28 1:00 UTC (permalink / raw)
To: netdev; +Cc: marcelo.leitner, lucien.xin, nhorman
Pass this, instead of an event. Then everything trickles down and we
always have events a non-empty list.
Then we needs a list creating stub to place into .enqueue_event for sctp_stream_interleave_1.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
net/sctp/stream_interleave.c | 44 +++++++++++++++++++++++++++---------
1 file changed, 33 insertions(+), 11 deletions(-)
diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index b6b251b8b3cf..0bc3d9329d9a 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -484,14 +484,15 @@ static struct sctp_ulpevent *sctp_intl_order(struct sctp_ulpq *ulpq,
}
static int sctp_enqueue_event(struct sctp_ulpq *ulpq,
- struct sctp_ulpevent *event)
+ struct sk_buff_head *skb_list)
{
- struct sk_buff *skb = sctp_event2skb(event);
struct sock *sk = ulpq->asoc->base.sk;
struct sctp_sock *sp = sctp_sk(sk);
- struct sk_buff_head *skb_list;
+ struct sctp_ulpevent *event;
+ struct sk_buff *skb;
- skb_list = (struct sk_buff_head *)skb->prev;
+ skb = __skb_peek(skb_list);
+ event = sctp_skb2event(skb);
if (sk->sk_shutdown & RCV_SHUTDOWN &&
(sk->sk_shutdown & SEND_SHUTDOWN ||
@@ -866,11 +867,15 @@ static int sctp_ulpevent_idata(struct sctp_ulpq *ulpq,
}
} else {
event = sctp_intl_reasm_uo(ulpq, event);
+ if (event) {
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
+ }
}
if (event) {
event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
- sctp_enqueue_event(ulpq, event);
+ sctp_enqueue_event(ulpq, &temp);
}
return event_eor;
@@ -944,20 +949,27 @@ static struct sctp_ulpevent *sctp_intl_retrieve_first(struct sctp_ulpq *ulpq)
static void sctp_intl_start_pd(struct sctp_ulpq *ulpq, gfp_t gfp)
{
struct sctp_ulpevent *event;
+ struct sk_buff_head temp;
if (!skb_queue_empty(&ulpq->reasm)) {
do {
event = sctp_intl_retrieve_first(ulpq);
- if (event)
- sctp_enqueue_event(ulpq, event);
+ if (event) {
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
+ sctp_enqueue_event(ulpq, &temp);
+ }
} while (event);
}
if (!skb_queue_empty(&ulpq->reasm_uo)) {
do {
event = sctp_intl_retrieve_first_uo(ulpq);
- if (event)
- sctp_enqueue_event(ulpq, event);
+ if (event) {
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
+ sctp_enqueue_event(ulpq, &temp);
+ }
} while (event);
}
}
@@ -1059,7 +1071,7 @@ static void sctp_intl_reap_ordered(struct sctp_ulpq *ulpq, __u16 sid)
if (event) {
sctp_intl_retrieve_ordered(ulpq, event);
- sctp_enqueue_event(ulpq, event);
+ sctp_enqueue_event(ulpq, &temp);
}
}
@@ -1326,6 +1338,16 @@ static struct sctp_stream_interleave sctp_stream_interleave_0 = {
.handle_ftsn = sctp_handle_fwdtsn,
};
+static int do_sctp_enqueue_event(struct sctp_ulpq *ulpq,
+ struct sctp_ulpevent *event)
+{
+ struct sk_buff_head temp;
+
+ skb_queue_head_init(&temp);
+ __skb_queue_tail(&temp, sctp_event2skb(event));
+ return sctp_enqueue_event(ulpq, &temp);
+}
+
static struct sctp_stream_interleave sctp_stream_interleave_1 = {
.data_chunk_len = sizeof(struct sctp_idata_chunk),
.ftsn_chunk_len = sizeof(struct sctp_ifwdtsn_chunk),
@@ -1334,7 +1356,7 @@ static struct sctp_stream_interleave sctp_stream_interleave_1 = {
.assign_number = sctp_chunk_assign_mid,
.validate_data = sctp_validate_idata,
.ulpevent_data = sctp_ulpevent_idata,
- .enqueue_event = sctp_enqueue_event,
+ .enqueue_event = do_sctp_enqueue_event,
.renege_events = sctp_renege_events,
.start_pd = sctp_intl_start_pd,
.abort_pd = sctp_intl_abort_pd,
--
2.20.1
^ permalink raw reply related
* [PATCH RFC v3 5/5] sctp: Pass sk_buff_head explicitly to sctp_ulpq_tail_event().
From: David Miller @ 2019-02-28 1:00 UTC (permalink / raw)
To: netdev; +Cc: marcelo.leitner, lucien.xin, nhorman
Now the SKB list implementation assumption can be removed.
And now that we know that the list head is always non-NULL
we can remove the code blocks dealing with that as well.
Signed-off-by: David S. Miller <davem@davemloft.net>
---
include/net/sctp/ulpqueue.h | 2 +-
net/sctp/stream_interleave.c | 2 +-
net/sctp/ulpqueue.c | 29 +++++++++++------------------
3 files changed, 13 insertions(+), 20 deletions(-)
diff --git a/include/net/sctp/ulpqueue.h b/include/net/sctp/ulpqueue.h
index bb0ecba3db2b..f4ac7117ff29 100644
--- a/include/net/sctp/ulpqueue.h
+++ b/include/net/sctp/ulpqueue.h
@@ -59,7 +59,7 @@ void sctp_ulpq_free(struct sctp_ulpq *);
int sctp_ulpq_tail_data(struct sctp_ulpq *, struct sctp_chunk *, gfp_t);
/* Add a new event for propagation to the ULP. */
-int sctp_ulpq_tail_event(struct sctp_ulpq *, struct sctp_ulpevent *ev);
+int sctp_ulpq_tail_event(struct sctp_ulpq *, struct sk_buff_head *skb_list);
/* Renege previously received chunks. */
void sctp_ulpq_renege(struct sctp_ulpq *, struct sctp_chunk *, gfp_t);
diff --git a/net/sctp/stream_interleave.c b/net/sctp/stream_interleave.c
index 0bc3d9329d9a..bf87b29e7323 100644
--- a/net/sctp/stream_interleave.c
+++ b/net/sctp/stream_interleave.c
@@ -1316,7 +1316,7 @@ static int do_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *even
skb_queue_head_init(&temp);
__skb_queue_tail(&temp, sctp_event2skb(event));
- return sctp_ulpq_tail_event(ulpq, event);
+ return sctp_ulpq_tail_event(ulpq, &temp);
}
static struct sctp_stream_interleave sctp_stream_interleave_0 = {
diff --git a/net/sctp/ulpqueue.c b/net/sctp/ulpqueue.c
index b22f558adc49..acdf77c7f71c 100644
--- a/net/sctp/ulpqueue.c
+++ b/net/sctp/ulpqueue.c
@@ -129,7 +129,7 @@ int sctp_ulpq_tail_data(struct sctp_ulpq *ulpq, struct sctp_chunk *chunk,
*/
if (event) {
event_eor = (event->msg_flags & MSG_EOR) ? 1 : 0;
- sctp_ulpq_tail_event(ulpq, event);
+ sctp_ulpq_tail_event(ulpq, &temp);
}
return event_eor;
@@ -193,18 +193,17 @@ static int sctp_ulpq_clear_pd(struct sctp_ulpq *ulpq)
return sctp_clear_pd(ulpq->asoc->base.sk, ulpq->asoc);
}
-/* If the SKB of 'event' is on a list, it is the first such member
- * of that list.
- */
-int sctp_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *event)
+int sctp_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sk_buff_head *skb_list)
{
struct sock *sk = ulpq->asoc->base.sk;
struct sctp_sock *sp = sctp_sk(sk);
- struct sk_buff_head *queue, *skb_list;
- struct sk_buff *skb = sctp_event2skb(event);
+ struct sctp_ulpevent *event;
+ struct sk_buff_head *queue;
+ struct sk_buff *skb;
int clear_pd = 0;
- skb_list = (struct sk_buff_head *) skb->prev;
+ skb = __skb_peek(skb_list);
+ event = sctp_skb2event(skb);
/* If the socket is just going to throw this away, do not
* even try to deliver it.
@@ -257,13 +256,7 @@ int sctp_ulpq_tail_event(struct sctp_ulpq *ulpq, struct sctp_ulpevent *event)
}
}
- /* If we are harvesting multiple skbs they will be
- * collected on a list.
- */
- if (skb_list)
- skb_queue_splice_tail_init(skb_list, queue);
- else
- __skb_queue_tail(queue, skb);
+ skb_queue_splice_tail_init(skb_list, queue);
/* Did we just complete partial delivery and need to get
* rolling again? Move pending data to the receive
@@ -756,7 +749,7 @@ static void sctp_ulpq_reasm_drain(struct sctp_ulpq *ulpq)
* sctp_ulpevent for very first SKB on the temp' list.
*/
if (event)
- sctp_ulpq_tail_event(ulpq, event);
+ sctp_ulpq_tail_event(ulpq, &temp);
}
}
@@ -956,7 +949,7 @@ static void sctp_ulpq_reap_ordered(struct sctp_ulpq *ulpq, __u16 sid)
if (event) {
/* see if we have more ordered that we can deliver */
sctp_ulpq_retrieve_ordered(ulpq, event);
- sctp_ulpq_tail_event(ulpq, event);
+ sctp_ulpq_tail_event(ulpq, &temp);
}
}
@@ -1086,7 +1079,7 @@ void sctp_ulpq_partial_delivery(struct sctp_ulpq *ulpq,
skb_queue_head_init(&temp);
__skb_queue_tail(&temp, sctp_event2skb(event));
- sctp_ulpq_tail_event(ulpq, event);
+ sctp_ulpq_tail_event(ulpq, &temp);
sctp_ulpq_set_pd(ulpq);
return;
}
--
2.20.1
^ permalink raw reply related
* [PATCH bpf-next 1/7] selftests: bpf: break up test_progs
From: Stanislav Fomichev @ 2019-02-28 1:01 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
Recently we had linux-next bpf/bpf-next conflict when we added new
functionality to the test_progs.c at the same location. Let's split
test_progs.c the same way we recently split test_verifier.c.
I follow the same patten we did in commit 2dfb40121ee8 ("selftests: bpf:
prepare for break up of verifier tests") for verifier: create
scaffolding to support dedicated files and slowly move the tests into
separate files.
Compared to the verifier split, each separate file here is
a compilation unit.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
tools/testing/selftests/bpf/Makefile | 27 +++++-
.../selftests/bpf/prog_tests/.gitignore | 1 +
tools/testing/selftests/bpf/test_progs.c | 92 +++----------------
tools/testing/selftests/bpf/test_progs.h | 81 ++++++++++++++++
4 files changed, 122 insertions(+), 79 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/.gitignore
create mode 100644 tools/testing/selftests/bpf/test_progs.h
diff --git a/tools/testing/selftests/bpf/Makefile b/tools/testing/selftests/bpf/Makefile
index ccffaa0a0787..518cd587cd63 100644
--- a/tools/testing/selftests/bpf/Makefile
+++ b/tools/testing/selftests/bpf/Makefile
@@ -165,7 +165,11 @@ $(ALU32_BUILD_DIR)/urandom_read: $(OUTPUT)/urandom_read
$(ALU32_BUILD_DIR)/test_progs_32: test_progs.c $(ALU32_BUILD_DIR) \
$(ALU32_BUILD_DIR)/urandom_read
$(CC) $(CFLAGS) -o $(ALU32_BUILD_DIR)/test_progs_32 $< \
- trace_helpers.c $(OUTPUT)/libbpf.a $(LDLIBS)
+ trace_helpers.c prog_tests/*.c $(OUTPUT)/libbpf.a $(LDLIBS)
+
+$(ALU32_BUILD_DIR)/test_progs_32: $(PROG_TESTS_H)
+$(ALU32_BUILD_DIR)/test_progs_32: CFLAGS += -I$(OUTPUT)
+$(ALU32_BUILD_DIR)/test_progs_32: prog_tests/*.c
$(ALU32_BUILD_DIR)/%.o: progs/%.c $(ALU32_BUILD_DIR) \
$(ALU32_BUILD_DIR)/test_progs_32
@@ -196,6 +200,25 @@ ifeq ($(DWARF2BTF),y)
$(BTF_PAHOLE) -J $@
endif
+PROG_TESTS_H := $(OUTPUT)/prog_tests/tests.h
+$(OUTPUT)/test_progs: $(PROG_TESTS_H)
+$(OUTPUT)/test_progs: CFLAGS += -I$(OUTPUT)
+$(OUTPUT)/test_progs: prog_tests/*.c
+
+PROG_TESTS_FILES := $(wildcard prog_tests/*.c)
+$(PROG_TESTS_H): $(PROG_TESTS_FILES)
+ $(shell ( cd prog_tests/
+ echo '/* Generated header, do not edit */'; \
+ echo '#ifdef DECLARE'; \
+ ls *.c 2> /dev/null | \
+ sed -e 's@\([^\.]*\)\.c@extern void test_\1(void);@'; \
+ echo '#endif'; \
+ echo '#ifdef CALL'; \
+ ls *.c 2> /dev/null | \
+ sed -e 's@\([^\.]*\)\.c@test_\1();@'; \
+ echo '#endif' \
+ ) > $(PROG_TESTS_H))
+
VERIFIER_TESTS_H := $(OUTPUT)/verifier/tests.h
$(OUTPUT)/test_verifier: $(VERIFIER_TESTS_H)
$(OUTPUT)/test_verifier: CFLAGS += -I$(OUTPUT)
@@ -211,4 +234,4 @@ $(OUTPUT)/verifier/tests.h: $(VERIFIER_TEST_FILES)
) > $(VERIFIER_TESTS_H))
EXTRA_CLEAN := $(TEST_CUSTOM_PROGS) $(ALU32_BUILD_DIR) \
- $(VERIFIER_TESTS_H)
+ $(VERIFIER_TESTS_H) $(PROG_TESTS_H)
diff --git a/tools/testing/selftests/bpf/prog_tests/.gitignore b/tools/testing/selftests/bpf/prog_tests/.gitignore
new file mode 100644
index 000000000000..45984a364647
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/.gitignore
@@ -0,0 +1 @@
+tests.h
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index c59d2e015d16..e49a1e20dc63 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -4,57 +4,12 @@
* 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 <unistd.h>
-#include <errno.h>
-#include <string.h>
-#include <assert.h>
-#include <stdlib.h>
-#include <stdarg.h>
-#include <time.h>
-#include <signal.h>
-
-#include <linux/types.h>
-typedef __u16 __sum16;
-#include <arpa/inet.h>
-#include <linux/if_ether.h>
-#include <linux/if_packet.h>
-#include <linux/ip.h>
-#include <linux/ipv6.h>
-#include <linux/tcp.h>
-#include <linux/filter.h>
-#include <linux/perf_event.h>
-#include <linux/unistd.h>
-
-#include <sys/ioctl.h>
-#include <sys/wait.h>
-#include <sys/types.h>
-#include <sys/time.h>
-#include <fcntl.h>
-#include <pthread.h>
-#include <linux/bpf.h>
-#include <linux/err.h>
-#include <bpf/bpf.h>
-#include <bpf/libbpf.h>
-
-#include "test_iptunnel_common.h"
-#include "bpf_util.h"
-#include "bpf_endian.h"
-#include "bpf_rlimit.h"
-#include "trace_helpers.h"
-#include "flow_dissector_load.h"
-
-static int error_cnt, pass_cnt;
-static bool jit_enabled;
-
-#define MAGIC_BYTES 123
-
-/* ipv4 test vector */
-static struct {
- struct ethhdr eth;
- struct iphdr iph;
- struct tcphdr tcp;
-} __packed pkt_v4 = {
+#include "test_progs.h"
+
+int error_cnt, pass_cnt;
+bool jit_enabled;
+
+struct ipv4_packet pkt_v4 = {
.eth.h_proto = __bpf_constant_htons(ETH_P_IP),
.iph.ihl = 5,
.iph.protocol = IPPROTO_TCP,
@@ -63,12 +18,7 @@ static struct {
.tcp.doff = 5,
};
-/* ipv6 test vector */
-static struct {
- struct ethhdr eth;
- struct ipv6hdr iph;
- struct tcphdr tcp;
-} __packed pkt_v6 = {
+struct ipv6_packet pkt_v6 = {
.eth.h_proto = __bpf_constant_htons(ETH_P_IPV6),
.iph.nexthdr = IPPROTO_TCP,
.iph.payload_len = __bpf_constant_htons(MAGIC_BYTES),
@@ -76,26 +26,7 @@ static struct {
.tcp.doff = 5,
};
-#define _CHECK(condition, tag, duration, format...) ({ \
- int __ret = !!(condition); \
- if (__ret) { \
- error_cnt++; \
- printf("%s:FAIL:%s ", __func__, tag); \
- printf(format); \
- } else { \
- pass_cnt++; \
- printf("%s:PASS:%s %d nsec\n", __func__, tag, duration);\
- } \
- __ret; \
-})
-
-#define CHECK(condition, tag, format...) \
- _CHECK(condition, tag, duration, format)
-#define CHECK_ATTR(condition, tag, format...) \
- _CHECK(condition, tag, tattr.duration, format)
-
-static int bpf_find_map(const char *test, struct bpf_object *obj,
- const char *name)
+int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
{
struct bpf_map *map;
@@ -2150,12 +2081,19 @@ static void test_signal_pending(enum bpf_prog_type prog_type)
signal(SIGALRM, SIG_DFL);
}
+#define DECLARE
+#include <prog_tests/tests.h>
+#undef DECLARE
+
int main(void)
{
srand(time(NULL));
jit_enabled = is_jit_enabled();
+#define CALL
+#include <prog_tests/tests.h>
+#undef CALL
test_pkt_access();
test_prog_run_xattr();
test_xdp();
diff --git a/tools/testing/selftests/bpf/test_progs.h b/tools/testing/selftests/bpf/test_progs.h
new file mode 100644
index 000000000000..8b79ab3e3042
--- /dev/null
+++ b/tools/testing/selftests/bpf/test_progs.h
@@ -0,0 +1,81 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#include <stdio.h>
+#include <unistd.h>
+#include <errno.h>
+#include <string.h>
+#include <assert.h>
+#include <stdlib.h>
+#include <stdarg.h>
+#include <time.h>
+#include <signal.h>
+
+#include <linux/types.h>
+typedef __u16 __sum16;
+#include <arpa/inet.h>
+#include <linux/if_ether.h>
+#include <linux/if_packet.h>
+#include <linux/ip.h>
+#include <linux/ipv6.h>
+#include <linux/tcp.h>
+#include <linux/filter.h>
+#include <linux/perf_event.h>
+#include <linux/unistd.h>
+
+#include <sys/ioctl.h>
+#include <sys/wait.h>
+#include <sys/types.h>
+#include <sys/time.h>
+#include <fcntl.h>
+#include <pthread.h>
+#include <linux/bpf.h>
+#include <linux/err.h>
+#include <bpf/bpf.h>
+#include <bpf/libbpf.h>
+
+#include "test_iptunnel_common.h"
+#include "bpf_util.h"
+#include "bpf_endian.h"
+#include "bpf_rlimit.h"
+#include "trace_helpers.h"
+#include "flow_dissector_load.h"
+
+extern int error_cnt, pass_cnt;
+extern bool jit_enabled;
+
+#define MAGIC_BYTES 123
+
+/* ipv4 test vector */
+struct ipv4_packet {
+ struct ethhdr eth;
+ struct iphdr iph;
+ struct tcphdr tcp;
+} __packed;
+extern struct ipv4_packet pkt_v4;
+
+/* ipv6 test vector */
+struct ipv6_packet {
+ struct ethhdr eth;
+ struct ipv6hdr iph;
+ struct tcphdr tcp;
+} __packed;
+extern struct ipv6_packet pkt_v6;
+
+#define _CHECK(condition, tag, duration, format...) ({ \
+ int __ret = !!(condition); \
+ if (__ret) { \
+ error_cnt++; \
+ printf("%s:FAIL:%s ", __func__, tag); \
+ printf(format); \
+ } else { \
+ pass_cnt++; \
+ printf("%s:PASS:%s %d nsec\n", __func__, tag, duration);\
+ } \
+ __ret; \
+})
+
+#define CHECK(condition, tag, format...) \
+ _CHECK(condition, tag, duration, format)
+#define CHECK_ATTR(condition, tag, format...) \
+ _CHECK(condition, tag, tattr.duration, format)
+
+int bpf_find_map(const char *test, struct bpf_object *obj, const char *name);
--
2.21.0.rc2.261.ga7da99ff1b-goog
^ permalink raw reply related
* [PATCH bpf-next 2/7] selftests: bpf: break up test_progs - pkt access
From: Stanislav Fomichev @ 2019-02-28 1:01 UTC (permalink / raw)
To: netdev, bpf; +Cc: davem, ast, daniel, Stanislav Fomichev
In-Reply-To: <20190228010156.82981-1-sdf@google.com>
Move pkt access prog tests into separate files.
Signed-off-by: Stanislav Fomichev <sdf@google.com>
---
.../selftests/bpf/prog_tests/pkt_access.c | 29 +++++++++++
.../selftests/bpf/prog_tests/pkt_md_access.c | 24 +++++++++
tools/testing/selftests/bpf/test_progs.c | 51 -------------------
3 files changed, 53 insertions(+), 51 deletions(-)
create mode 100644 tools/testing/selftests/bpf/prog_tests/pkt_access.c
create mode 100644 tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
new file mode 100644
index 000000000000..4ecfd721a044
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/pkt_access.c
@@ -0,0 +1,29 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_pkt_access(void)
+{
+ const char *file = "./test_pkt_access.o";
+ struct bpf_object *obj;
+ __u32 duration, retval;
+ int err, prog_fd;
+
+ err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+ if (err) {
+ error_cnt++;
+ return;
+ }
+
+ err = bpf_prog_test_run(prog_fd, 100000, &pkt_v4, sizeof(pkt_v4),
+ NULL, NULL, &retval, &duration);
+ CHECK(err || retval, "ipv4",
+ "err %d errno %d retval %d duration %d\n",
+ err, errno, retval, duration);
+
+ err = bpf_prog_test_run(prog_fd, 100000, &pkt_v6, sizeof(pkt_v6),
+ NULL, NULL, &retval, &duration);
+ CHECK(err || retval, "ipv6",
+ "err %d errno %d retval %d duration %d\n",
+ err, errno, retval, duration);
+ bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
new file mode 100644
index 000000000000..ac0d43435806
--- /dev/null
+++ b/tools/testing/selftests/bpf/prog_tests/pkt_md_access.c
@@ -0,0 +1,24 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <test_progs.h>
+
+void test_pkt_md_access(void)
+{
+ const char *file = "./test_pkt_md_access.o";
+ struct bpf_object *obj;
+ __u32 duration, retval;
+ int err, prog_fd;
+
+ err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
+ if (err) {
+ error_cnt++;
+ return;
+ }
+
+ err = bpf_prog_test_run(prog_fd, 10, &pkt_v4, sizeof(pkt_v4),
+ NULL, NULL, &retval, &duration);
+ CHECK(err || retval, "",
+ "err %d errno %d retval %d duration %d\n",
+ err, errno, retval, duration);
+
+ bpf_object__close(obj);
+}
diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
index e49a1e20dc63..91bb5c8a97c9 100644
--- a/tools/testing/selftests/bpf/test_progs.c
+++ b/tools/testing/selftests/bpf/test_progs.c
@@ -39,33 +39,6 @@ int bpf_find_map(const char *test, struct bpf_object *obj, const char *name)
return bpf_map__fd(map);
}
-static void test_pkt_access(void)
-{
- const char *file = "./test_pkt_access.o";
- struct bpf_object *obj;
- __u32 duration, retval;
- int err, prog_fd;
-
- err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
- if (err) {
- error_cnt++;
- return;
- }
-
- err = bpf_prog_test_run(prog_fd, 100000, &pkt_v4, sizeof(pkt_v4),
- NULL, NULL, &retval, &duration);
- CHECK(err || retval, "ipv4",
- "err %d errno %d retval %d duration %d\n",
- err, errno, retval, duration);
-
- err = bpf_prog_test_run(prog_fd, 100000, &pkt_v6, sizeof(pkt_v6),
- NULL, NULL, &retval, &duration);
- CHECK(err || retval, "ipv6",
- "err %d errno %d retval %d duration %d\n",
- err, errno, retval, duration);
- bpf_object__close(obj);
-}
-
static void test_prog_run_xattr(void)
{
const char *file = "./test_pkt_access.o";
@@ -647,28 +620,6 @@ static void test_bpf_obj_id(void)
bpf_object__close(objs[i]);
}
-static void test_pkt_md_access(void)
-{
- const char *file = "./test_pkt_md_access.o";
- struct bpf_object *obj;
- __u32 duration, retval;
- int err, prog_fd;
-
- err = bpf_prog_load(file, BPF_PROG_TYPE_SCHED_CLS, &obj, &prog_fd);
- if (err) {
- error_cnt++;
- return;
- }
-
- err = bpf_prog_test_run(prog_fd, 10, &pkt_v4, sizeof(pkt_v4),
- NULL, NULL, &retval, &duration);
- CHECK(err || retval, "",
- "err %d errno %d retval %d duration %d\n",
- err, errno, retval, duration);
-
- bpf_object__close(obj);
-}
-
static void test_obj_name(void)
{
struct {
@@ -2094,7 +2045,6 @@ int main(void)
#define CALL
#include <prog_tests/tests.h>
#undef CALL
- test_pkt_access();
test_prog_run_xattr();
test_xdp();
test_xdp_adjust_tail();
@@ -2102,7 +2052,6 @@ int main(void)
test_xdp_noinline();
test_tcp_estats();
test_bpf_obj_id();
- test_pkt_md_access();
test_obj_name();
test_tp_attach_query();
test_stacktrace_map();
--
2.21.0.rc2.261.ga7da99ff1b-goog
^ permalink raw reply related
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