* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Andrew Lunn @ 2019-02-27 23:19 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Heiner Kallweit, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227225221.vubwqa6vg5odq26w@shell.armlinux.org.uk>
> >From what you've described, it sounds like what you actually have is:
>
> MAC <---> Serdes PHY <---> PHY
>
> The Serdes PHY receives the SGMII in-band negotiation from the external
> PHY, but there is no propagation of the status from the serdes PHY to
> the MAC.
Yes, that is a good description. So far, we have not yet got the MAC
to read the speed and duplex from the SERDES to configure itself. In
theory it should be able to, it is all in the same device.
It might be that once the SERDES interrupts saying it has link we need
to program the MAC with the result of the in-band signalling. in-band
then seems a bit pointless. Or we are missing some configuration
somewhere to tell the MAC to use the in-band signalling result from
the SERDES.
Andrew
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Heiner Kallweit @ 2019-02-27 23:25 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227225221.vubwqa6vg5odq26w@shell.armlinux.org.uk>
On 27.02.2019 23:52, Russell King - ARM Linux admin wrote:
> On Wed, Feb 27, 2019 at 10:25:54PM +0100, Heiner Kallweit wrote:
>> For the ones who don't know my story yet:
>> I have a Marvell 88E6390 switch with a Clause 45 PHY connected via
>> SGMII to port 9. For other reasons I limit the PHY to 100Mbps currently.
>> DT says: managed = "in-band-status"
>> Driver is mv8e6xxx + phylink.
>> Problem is that the link doesn't come up. Debugging resulted in:
>>
>> PHY establishes link to link partner (100Mbps, FD) and fires the
>> "link up" interrupt. The "link up" is also properly transmitted via
>> SGMII in-band signalling and fires the SERDES interrupt in mv8e6xxx.
>> Problem is that the in-band transmitted values for speed and duplex
>> don't show up anywhere. And phylink_resolve() doesn't even print
>> the link-up message.
>>
>> First question: Both, PHY and SERDES interrupt, try to do the same
>> thing: they eventually call phylink_run_resolve(). Is this correct?
>
> Correct - when the PHY interrupts, it triggers phylib to read the
> status and issue a callback via the link status hook into phylink,
> which stores the information from the PHY. That triggers a resolve.
> Even though the PHY reports that it has established link, the link
> may not yet be up.
>
> The Serdes is the MAC side of the link, and that should be calling
> phylink_mac_change(). That will re-run the resolve.
>
OK, then most of my headache was caused by the fundamental
misunderstanding of who's PHY and who's MAC.
> Each time the resolve is triggered in "in-band" mode, we expect to
> read the link status from the MAC side of the link, along with the
> speed and duplex. Since SGMII does not carry the flow control
> parameters, if a PHY is present, we merge the PHYs flow control
> information and pass that back to the MAC to configure the MAC's
> flow control to match.
>
>> I have some problems with understanding the code for MLO_AN_INBAND
>> in phylink_resolve(). Maybe also something is missing there for
>> proper in-band aneg support.
>
> Nope, it works with mvneta and mvpp2.
>
>> First we get the mac state (which is link down, 10Mbps, HD in my case).
>> Then the link state is calculated as "mac link up" && "phy link up".
>> This results in "link down", because mac link is down and phy link is
>> up. This logic isn't clear to me. How is the "link up" info supposed
>> to ever reach the mac?
>
> Via the in-band status, and the MAC detecting that the link is now up.
>
>> We're reading the port status, but IMO we want to set it based on the
>> auto-negotiated settings we get from the PHY.
>
> No we don't - in SGMII in-band mode, the PHY is optional, and even if
> it is present, if the MAC reports that the link is down even if the PHY
> reports that the link is up, then the link is _not_ up.
>
> If you want to use SGMII without in-band, then phylink supports that
> (which is PHY mode). If the MAC is unable to report these parameters,
> then in-band mode is not appropriate.
>
Now that it's clear that MAC is the SERDES PHY: I have these parameters
available in SGMII registers.
>> Later pause settings and possibly changed interface mode are
>> propagated to the mac. But no word about speed and duplex.
>>
>> Via "some callback" "some code" should read the in-band-transmitted
>> speed and duplex values from the SGMII port and use them to configure
>> the mac. Is this simply missing or do I miss something?
>
>>From what you've described, it sounds like what you actually have is:
>
> MAC <---> Serdes PHY <---> PHY
>
> The Serdes PHY receives the SGMII in-band negotiation from the external
> PHY, but there is no propagation of the status from the serdes PHY to
> the MAC. What's more is the Serdes PHY is hidden in mv88e6xxx setups.
>
That's exactly my setup. So what's missing is the status reporting
from SERDES PHY to actual MAC.
I could switch the port to forced mode and copy the parameters in the
SERDES "link-up" interrupt from the SGMII registers to the port forced
config. Hmm, would that be sufficient?
At least I could manage to get the link being reported as up. Whether
traffic flows I'll have to see. And I would have to see that I don't
break other usage of the SERDES port.
> This is a classic stacked PHY setup, one which we're now starting to
> encounter, and we need to support it - we have three cases I'm aware of
> now where we have:
>
> MAC <---> PHY <---> SFP
>
> and "SFP" can be another PHY, so we end up with:
>
> MAC <---> PHY <---> PHY
>
> and phylib does not support having two PHYs on the same net_device.
>
> What's more is that the need for an "attached_dev" net_device is pretty
> heavily embedded into phylib, so attaching one PHY to another PHY
> doesn't work very well.
>
> I've been tinkering with some patches to abstract out the "attached_dev"
> stuff, but in doing so I've walked into a whole load of issues in
> phy_attach_direct() and phy_detach() - it looks like phy_attach_direct()
> is lacking any form of locking while binding one of the generic drivers
> (it needs to hold the device lock while doing so.) The comments around
> device_release_driver() are vague about whether the parent device needs
> to be locked. Questions have been asked about that but so far I haven't
> had a response.
>
> We can replace a load of phydev->attached_dev != NULL tests (which are
> checking whether the device is attached to a netdev) fairly easily,
> but there's a load of places which do (phydev->attached_dev &&
> phydev->adjust_link) - and I fear that phylink (which doesn't set
> phydev->adjust_link) results in this code being broken.
>
Really appreciate the comprehensive explanation!
Heiner
^ permalink raw reply
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Russell King - ARM Linux admin @ 2019-02-27 23:27 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Heiner Kallweit, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227231940.GN3809@lunn.ch>
On Thu, Feb 28, 2019 at 12:19:40AM +0100, Andrew Lunn wrote:
> > >From what you've described, it sounds like what you actually have is:
> >
> > MAC <---> Serdes PHY <---> PHY
> >
> > The Serdes PHY receives the SGMII in-band negotiation from the external
> > PHY, but there is no propagation of the status from the serdes PHY to
> > the MAC.
>
> Yes, that is a good description. So far, we have not yet got the MAC
> to read the speed and duplex from the SERDES to configure itself. In
> theory it should be able to, it is all in the same device.
>
> It might be that once the SERDES interrupts saying it has link we need
> to program the MAC with the result of the in-band signalling. in-band
> then seems a bit pointless. Or we are missing some configuration
> somewhere to tell the MAC to use the in-band signalling result from
> the SERDES.
in-band doesn't become pointless if we can't read from the PHY at the
other end of the in-band link (for example, a Microtik copper SFP
module with an AR8033 on, which has no connectivity to the host other
than the serdes lane.)
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH -next] appletalk: use remove_proc_subtree to simplify procfs code
From: kbuild test robot @ 2019-02-27 23:28 UTC (permalink / raw)
To: Yue Haibing
Cc: kbuild-all, davem, joe, gregkh, linux-kernel, netdev, YueHaibing
In-Reply-To: <20190227150023.28420-1-yuehaibing@huawei.com>
[-- Attachment #1: Type: text/plain, Size: 4667 bytes --]
Hi YueHaibing,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on next-20190227]
url: https://github.com/0day-ci/linux/commits/Yue-Haibing/appletalk-use-remove_proc_subtree-to-simplify-procfs-code/20190228-014856
config: i386-randconfig-s3-02271654 (attached as .config)
compiler: gcc-6 (Debian 6.5.0-2) 6.5.0 20181026
reproduce:
# save the attached .config to linux build tree
make ARCH=i386
All warnings (new ones prefixed by >>):
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
net//appletalk/atalk_proc.c: In function 'atalk_proc_init':
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
>> net//appletalk/atalk_proc.c:218:2: note: in expansion of macro 'if'
if (!proc_create_seq("atalk/interface", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:220:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net//appletalk/atalk_proc.c:222:2: note: in expansion of macro 'if'
if (!proc_create_seq("atalk/route", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:224:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net//appletalk/atalk_proc.c:226:2: note: in expansion of macro 'if'
if (!proc_create_seq("atalk/socket", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:228:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
In file included from include/linux/init.h:5:0,
from net//appletalk/atalk_proc.c:11:
include/linux/compiler.h:58:2: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
if (__builtin_constant_p(!!(cond)) ? !!(cond) : \
^
include/linux/compiler.h:56:23: note: in expansion of macro '__trace_if'
#define if(cond, ...) __trace_if( (cond , ## __VA_ARGS__) )
^~~~~~~~~~
net//appletalk/atalk_proc.c:230:2: note: in expansion of macro 'if'
if (!proc_create_seq_private("atalk/arp", 0444, init_net.proc_net,
^~
net//appletalk/atalk_proc.c:233:3: note: ...this statement, but the latter is misleadingly indented as if it is guarded by the 'if'
goto out;
^~~~
vim +/if +218 net//appletalk/atalk_proc.c
212
213 int __init atalk_proc_init(void)
214 {
215 if (!proc_mkdir("atalk", init_net.proc_net))
216 return -ENOMEM;
217
> 218 if (!proc_create_seq("atalk/interface", 0444, init_net.proc_net,
219 &atalk_seq_interface_ops));
220 goto out;
221
222 if (!proc_create_seq("atalk/route", 0444, init_net.proc_net,
223 &atalk_seq_route_ops));
224 goto out;
225
226 if (!proc_create_seq("atalk/socket", 0444, init_net.proc_net,
227 &atalk_seq_socket_ops));
228 goto out;
229
230 if (!proc_create_seq_private("atalk/arp", 0444, init_net.proc_net,
231 &aarp_seq_ops,
232 sizeof(struct aarp_iter_state), NULL));
233 goto out;
234
235 out:
236 remove_proc_subtree("atalk", init_net.proc_net);
237 return -ENOMEM;
238 }
239
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29122 bytes --]
^ permalink raw reply
* [PATCH bpf-next 0/5] samples: bpf: start effort to get rid of bpf_load
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
Hi!
This set is next part of a quest to get rid of the bpf_load
ELF loader. It fixes some minor issues with the samples and
starts the conversion.
First patch fixes ping invocations, ping localhost defaults
to IPv6 on modern setups. Next load_sock_ops sample is removed
and users are directed towards using bpftool directly.
Patch 4 removes the use of bpf_load from samples which don't
need the auto-attachment functionality at all.
Patch 5 improves symbol counting in libbpf, it's not currently
an issue but it will be when anyone adds a symbol with a long
name. Let's make sure that person doesn't have to spend time
scratching their head and wondering why .a and .so symbol
counts don't match.
Jakub Kicinski (5):
samples: bpf: force IPv4 in ping
samples: bpf: remove load_sock_ops in favour of bpftool
tools: libbpf: add a correctly named define for map iteration
samples: bpf: use libbpf where easy
tools: libbpf: make sure readelf shows full names in build checks
samples/bpf/.gitignore | 1 -
samples/bpf/Makefile | 8 +-
samples/bpf/fds_example.c | 9 +-
samples/bpf/load_sock_ops.c | 97 -------------------
samples/bpf/sock_example.c | 2 +-
samples/bpf/sockex1_user.c | 24 ++---
samples/bpf/sockex2_user.c | 22 +++--
samples/bpf/sockex3_user.c | 2 +-
samples/bpf/tcp_basertt_kern.c | 2 +-
samples/bpf/tcp_bpf.readme | 14 +--
samples/bpf/tcp_bufs_kern.c | 2 +-
samples/bpf/tcp_clamp_kern.c | 2 +-
samples/bpf/tcp_cong_kern.c | 2 +-
samples/bpf/tcp_iw_kern.c | 2 +-
samples/bpf/tcp_rwnd_kern.c | 2 +-
samples/bpf/tcp_synrto_kern.c | 2 +-
samples/bpf/tcp_tos_reflect_kern.c | 2 +-
samples/bpf/tracex2_user.c | 2 +-
tools/bpf/bpftool/prog.c | 4 +-
tools/lib/bpf/Makefile | 4 +-
tools/lib/bpf/libbpf.c | 8 +-
tools/lib/bpf/libbpf.h | 3 +-
tools/perf/util/bpf-loader.c | 4 +-
.../testing/selftests/bpf/test_libbpf_open.c | 2 +-
24 files changed, 66 insertions(+), 156 deletions(-)
delete mode 100644 samples/bpf/load_sock_ops.c
--
2.19.2
^ permalink raw reply
* [PATCH bpf-next 2/5] samples: bpf: remove load_sock_ops in favour of bpftool
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
bpftool can do all the things load_sock_ops used to do, and more.
Point users to bpftool instead of maintaining this sample utility.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
samples/bpf/.gitignore | 1 -
samples/bpf/Makefile | 2 -
samples/bpf/load_sock_ops.c | 97 ------------------------------
samples/bpf/tcp_basertt_kern.c | 2 +-
samples/bpf/tcp_bpf.readme | 14 +++--
samples/bpf/tcp_bufs_kern.c | 2 +-
samples/bpf/tcp_clamp_kern.c | 2 +-
samples/bpf/tcp_cong_kern.c | 2 +-
samples/bpf/tcp_iw_kern.c | 2 +-
samples/bpf/tcp_rwnd_kern.c | 2 +-
samples/bpf/tcp_synrto_kern.c | 2 +-
samples/bpf/tcp_tos_reflect_kern.c | 2 +-
12 files changed, 16 insertions(+), 114 deletions(-)
delete mode 100644 samples/bpf/load_sock_ops.c
diff --git a/samples/bpf/.gitignore b/samples/bpf/.gitignore
index 8ae4940025f8..dbb817dbacfc 100644
--- a/samples/bpf/.gitignore
+++ b/samples/bpf/.gitignore
@@ -1,7 +1,6 @@
cpustat
fds_example
lathist
-load_sock_ops
lwt_len_hist
map_perf_test
offwaketime
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index a333e258f319..4dd98100678e 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -40,7 +40,6 @@ hostprogs-y += lwt_len_hist
hostprogs-y += xdp_tx_iptunnel
hostprogs-y += test_map_in_map
hostprogs-y += per_socket_stats_example
-hostprogs-y += load_sock_ops
hostprogs-y += xdp_redirect
hostprogs-y += xdp_redirect_map
hostprogs-y += xdp_redirect_cpu
@@ -71,7 +70,6 @@ tracex4-objs := bpf_load.o tracex4_user.o
tracex5-objs := bpf_load.o tracex5_user.o
tracex6-objs := bpf_load.o tracex6_user.o
tracex7-objs := bpf_load.o tracex7_user.o
-load_sock_ops-objs := bpf_load.o load_sock_ops.o
test_probe_write_user-objs := bpf_load.o test_probe_write_user_user.o
trace_output-objs := bpf_load.o trace_output_user.o $(TRACE_HELPERS)
lathist-objs := bpf_load.o lathist_user.o
diff --git a/samples/bpf/load_sock_ops.c b/samples/bpf/load_sock_ops.c
deleted file mode 100644
index 8ecb41ea0c03..000000000000
--- a/samples/bpf/load_sock_ops.c
+++ /dev/null
@@ -1,97 +0,0 @@
-/* Copyright (c) 2017 Facebook
- *
- * This program is free software; you can redistribute it and/or
- * modify it under the terms of version 2 of the GNU General Public
- * License as published by the Free Software Foundation.
- */
-#include <stdio.h>
-#include <stdlib.h>
-#include <string.h>
-#include <linux/bpf.h>
-#include <bpf/bpf.h>
-#include "bpf_load.h"
-#include <unistd.h>
-#include <errno.h>
-#include <fcntl.h>
-#include <linux/unistd.h>
-
-static void usage(char *pname)
-{
- printf("USAGE:\n %s [-l] <cg-path> <prog filename>\n", pname);
- printf("\tLoad and attach a sock_ops program to the specified "
- "cgroup\n");
- printf("\tIf \"-l\" is used, the program will continue to run\n");
- printf("\tprinting the BPF log buffer\n");
- printf("\tIf the specified filename does not end in \".o\", it\n");
- printf("\tappends \"_kern.o\" to the name\n");
- printf("\n");
- printf(" %s -r <cg-path>\n", pname);
- printf("\tDetaches the currently attached sock_ops program\n");
- printf("\tfrom the specified cgroup\n");
- printf("\n");
- exit(1);
-}
-
-int main(int argc, char **argv)
-{
- int logFlag = 0;
- int error = 0;
- char *cg_path;
- char fn[500];
- char *prog;
- int cg_fd;
-
- if (argc < 3)
- usage(argv[0]);
-
- if (!strcmp(argv[1], "-r")) {
- cg_path = argv[2];
- cg_fd = open(cg_path, O_DIRECTORY, O_RDONLY);
- error = bpf_prog_detach(cg_fd, BPF_CGROUP_SOCK_OPS);
- if (error) {
- printf("ERROR: bpf_prog_detach: %d (%s)\n",
- error, strerror(errno));
- return 2;
- }
- return 0;
- } else if (!strcmp(argv[1], "-h")) {
- usage(argv[0]);
- } else if (!strcmp(argv[1], "-l")) {
- logFlag = 1;
- if (argc < 4)
- usage(argv[0]);
- }
-
- prog = argv[argc - 1];
- cg_path = argv[argc - 2];
- if (strlen(prog) > 480) {
- fprintf(stderr, "ERROR: program name too long (> 480 chars)\n");
- return 3;
- }
- cg_fd = open(cg_path, O_DIRECTORY, O_RDONLY);
-
- if (!strcmp(prog + strlen(prog)-2, ".o"))
- strcpy(fn, prog);
- else
- sprintf(fn, "%s_kern.o", prog);
- if (logFlag)
- printf("loading bpf file:%s\n", fn);
- if (load_bpf_file(fn)) {
- printf("ERROR: load_bpf_file failed for: %s\n", fn);
- printf("%s", bpf_log_buf);
- return 4;
- }
- if (logFlag)
- printf("TCP BPF Loaded %s\n", fn);
-
- error = bpf_prog_attach(prog_fd[0], cg_fd, BPF_CGROUP_SOCK_OPS, 0);
- if (error) {
- printf("ERROR: bpf_prog_attach: %d (%s)\n",
- error, strerror(errno));
- return 5;
- } else if (logFlag) {
- read_trace_pipe();
- }
-
- return error;
-}
diff --git a/samples/bpf/tcp_basertt_kern.c b/samples/bpf/tcp_basertt_kern.c
index 4bf4fc597db9..6ef1625e8b2c 100644
--- a/samples/bpf/tcp_basertt_kern.c
+++ b/samples/bpf/tcp_basertt_kern.c
@@ -7,7 +7,7 @@
* BPF program to set base_rtt to 80us when host is running TCP-NV and
* both hosts are in the same datacenter (as determined by IPv6 prefix).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_bpf.readme b/samples/bpf/tcp_bpf.readme
index 831fb601e3c9..fee746621aec 100644
--- a/samples/bpf/tcp_bpf.readme
+++ b/samples/bpf/tcp_bpf.readme
@@ -8,14 +8,16 @@ a cgroupv2 and attach a bash shell to the group.
bash
echo $$ >> /tmp/cgroupv2/foo/cgroup.procs
-Anything that runs under this shell belongs to the foo cgroupv2 To load
+Anything that runs under this shell belongs to the foo cgroupv2. To load
(attach) one of the tcp_*_kern.o programs:
- ./load_sock_ops -l /tmp/cgroupv2/foo tcp_basertt_kern.o
+ bpftool prog load tcp_basertt_kern.o /sys/fs/bpf/tcp_prog
+ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
+ bpftool prog tracelog
-If the "-l" flag is used, the load_sock_ops program will continue to run
-printing the BPF log buffer. The tcp_*_kern.o programs use special print
-functions to print logging information (if enabled by the ifdef).
+"bpftool prog tracelog" will continue to run printing the BPF log buffer.
+The tcp_*_kern.o programs use special print functions to print logging
+information (if enabled by the ifdef).
If using netperf/netserver to create traffic, you need to run them under the
cgroupv2 to which the BPF programs are attached (i.e. under bash shell
@@ -23,4 +25,4 @@ attached to the cgroupv2).
To remove (unattach) a socket_ops BPF program from a cgroupv2:
- ./load_sock_ops -r /tmp/cgroupv2/foo
+ bpftool cgroup attach /tmp/cgroupv2/foo sock_ops pinned /sys/fs/bpf/tcp_prog
diff --git a/samples/bpf/tcp_bufs_kern.c b/samples/bpf/tcp_bufs_kern.c
index 0566b7fa38a1..e03e204739fa 100644
--- a/samples/bpf/tcp_bufs_kern.c
+++ b/samples/bpf/tcp_bufs_kern.c
@@ -9,7 +9,7 @@
* doing appropriate checks that indicate the hosts are far enough
* away (i.e. large RTT).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_clamp_kern.c b/samples/bpf/tcp_clamp_kern.c
index f4225c9d2c0c..a0dc2d254aca 100644
--- a/samples/bpf/tcp_clamp_kern.c
+++ b/samples/bpf/tcp_clamp_kern.c
@@ -9,7 +9,7 @@
* the same datacenter. For his example, we assume they are within the same
* datacenter when the first 5.5 bytes of their IPv6 addresses are the same.
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_cong_kern.c b/samples/bpf/tcp_cong_kern.c
index ad0f1ba8206a..4fd3ca979a06 100644
--- a/samples/bpf/tcp_cong_kern.c
+++ b/samples/bpf/tcp_cong_kern.c
@@ -7,7 +7,7 @@
* BPF program to set congestion control to dctcp when both hosts are
* in the same datacenter (as deteremined by IPv6 prefix).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_iw_kern.c b/samples/bpf/tcp_iw_kern.c
index 4ca5ecc9f580..9b139ec69560 100644
--- a/samples/bpf/tcp_iw_kern.c
+++ b/samples/bpf/tcp_iw_kern.c
@@ -9,7 +9,7 @@
* would usually be done after doing appropriate checks that indicate
* the hosts are far enough away (i.e. large RTT).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_rwnd_kern.c b/samples/bpf/tcp_rwnd_kern.c
index 09ff65b40b31..cc71ee96e044 100644
--- a/samples/bpf/tcp_rwnd_kern.c
+++ b/samples/bpf/tcp_rwnd_kern.c
@@ -8,7 +8,7 @@
* and the first 5.5 bytes of the IPv6 addresses are not the same (in this
* example that means both hosts are not the same datacenter).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_synrto_kern.c b/samples/bpf/tcp_synrto_kern.c
index 232bb242823e..ca87ed34f896 100644
--- a/samples/bpf/tcp_synrto_kern.c
+++ b/samples/bpf/tcp_synrto_kern.c
@@ -8,7 +8,7 @@
* and the first 5.5 bytes of the IPv6 addresses are the same (in this example
* that means both hosts are in the same datacenter).
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
diff --git a/samples/bpf/tcp_tos_reflect_kern.c b/samples/bpf/tcp_tos_reflect_kern.c
index d51dab19eca6..de788be6f862 100644
--- a/samples/bpf/tcp_tos_reflect_kern.c
+++ b/samples/bpf/tcp_tos_reflect_kern.c
@@ -4,7 +4,7 @@
*
* BPF program to automatically reflect TOS option from received syn packet
*
- * Use load_sock_ops to load this BPF program.
+ * Use "bpftool cgroup attach $cg sock_ops $prog" to load this BPF program.
*/
#include <uapi/linux/bpf.h>
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 1/5] samples: bpf: force IPv4 in ping
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
ping localhost may default of IPv6 on modern systems, but
samples are trying to only parse IPv4. Force IPv4.
samples/bpf/tracex1_user.c doesn't interpret the packet so
we don't care which IP version will be used there.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
samples/bpf/sock_example.c | 2 +-
samples/bpf/sockex1_user.c | 2 +-
samples/bpf/sockex2_user.c | 2 +-
samples/bpf/sockex3_user.c | 2 +-
samples/bpf/tracex2_user.c | 2 +-
5 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/samples/bpf/sock_example.c b/samples/bpf/sock_example.c
index 60ec467c78ab..00aae1d33fca 100644
--- a/samples/bpf/sock_example.c
+++ b/samples/bpf/sock_example.c
@@ -99,7 +99,7 @@ int main(void)
{
FILE *f;
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void)f;
return test_sock();
diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
index 93ec01c56104..be8ba5686924 100644
--- a/samples/bpf/sockex1_user.c
+++ b/samples/bpf/sockex1_user.c
@@ -26,7 +26,7 @@ int main(int ac, char **argv)
assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
sizeof(prog_fd[0])) == 0);
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void) f;
for (i = 0; i < 5; i++) {
diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
index 1d5c6e9a6d27..125ee6efc913 100644
--- a/samples/bpf/sockex2_user.c
+++ b/samples/bpf/sockex2_user.c
@@ -34,7 +34,7 @@ int main(int ac, char **argv)
assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
sizeof(prog_fd[0])) == 0);
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void) f;
for (i = 0; i < 5; i++) {
diff --git a/samples/bpf/sockex3_user.c b/samples/bpf/sockex3_user.c
index 9d02e0404719..bbb1cd0666a9 100644
--- a/samples/bpf/sockex3_user.c
+++ b/samples/bpf/sockex3_user.c
@@ -58,7 +58,7 @@ int main(int argc, char **argv)
sizeof(__u32)) == 0);
if (argc > 1)
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
else
f = popen("netperf -l 4 localhost", "r");
(void) f;
diff --git a/samples/bpf/tracex2_user.c b/samples/bpf/tracex2_user.c
index 1a81e6a5c2ea..c9544a4ce61a 100644
--- a/samples/bpf/tracex2_user.c
+++ b/samples/bpf/tracex2_user.c
@@ -131,7 +131,7 @@ int main(int ac, char **argv)
signal(SIGTERM, int_exit);
/* start 'ping' in the background to have some kfree_skb events */
- f = popen("ping -c5 localhost", "r");
+ f = popen("ping -4 -c5 localhost", "r");
(void) f;
/* start 'dd' in the background to have plenty of 'write' syscalls */
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 4/5] samples: bpf: use libbpf where easy
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
Some samples don't really need the magic of bpf_load,
switch them to libbpf.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
samples/bpf/Makefile | 6 +++---
samples/bpf/fds_example.c | 9 ++++++---
samples/bpf/sockex1_user.c | 22 ++++++++++++----------
samples/bpf/sockex2_user.c | 20 +++++++++++---------
4 files changed, 32 insertions(+), 25 deletions(-)
diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
index 4dd98100678e..0c62ac39c697 100644
--- a/samples/bpf/Makefile
+++ b/samples/bpf/Makefile
@@ -59,9 +59,9 @@ LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
-fds_example-objs := bpf_load.o fds_example.o
-sockex1-objs := bpf_load.o sockex1_user.o
-sockex2-objs := bpf_load.o sockex2_user.o
+fds_example-objs := fds_example.o
+sockex1-objs := sockex1_user.o
+sockex2-objs := sockex2_user.o
sockex3-objs := bpf_load.o sockex3_user.o
tracex1-objs := bpf_load.o tracex1_user.o
tracex2-objs := bpf_load.o tracex2_user.o
diff --git a/samples/bpf/fds_example.c b/samples/bpf/fds_example.c
index 9854854f05d1..36f1f18aae3c 100644
--- a/samples/bpf/fds_example.c
+++ b/samples/bpf/fds_example.c
@@ -14,8 +14,8 @@
#include <bpf/bpf.h>
+#include "bpf/libbpf.h"
#include "bpf_insn.h"
-#include "bpf_load.h"
#include "sock_example.h"
#define BPF_F_PIN (1 << 0)
@@ -57,10 +57,13 @@ static int bpf_prog_create(const char *object)
BPF_EXIT_INSN(),
};
size_t insns_cnt = sizeof(insns) / sizeof(struct bpf_insn);
+ char bpf_log_buf[BPF_LOG_BUF_SIZE];
+ struct bpf_object *obj;
+ int prog_fd;
if (object) {
- assert(!load_bpf_file((char *)object));
- return prog_fd[0];
+ assert(!bpf_prog_load(object, 0, &obj, &prog_fd));
+ return prog_fd;
} else {
return bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER,
insns, insns_cnt, "GPL", 0,
diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
index be8ba5686924..5e7c4be3e645 100644
--- a/samples/bpf/sockex1_user.c
+++ b/samples/bpf/sockex1_user.c
@@ -3,28 +3,30 @@
#include <assert.h>
#include <linux/bpf.h>
#include <bpf/bpf.h>
-#include "bpf_load.h"
+#include "bpf/libbpf.h"
#include "sock_example.h"
#include <unistd.h>
#include <arpa/inet.h>
int main(int ac, char **argv)
{
+ struct bpf_object *obj;
+ int map_fd, prog_fd;
char filename[256];
- FILE *f;
int i, sock;
+ FILE *f;
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
- if (load_bpf_file(filename)) {
- printf("%s", bpf_log_buf);
+ if (bpf_prog_load(filename, 0, &obj, &prog_fd))
return 1;
- }
+
+ map_fd = bpf_object__find_map_fd_by_name(obj, "my_map");
sock = open_raw_sock("lo");
- assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
- sizeof(prog_fd[0])) == 0);
+ assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
+ sizeof(prog_fd)) == 0);
f = popen("ping -4 -c5 localhost", "r");
(void) f;
@@ -34,13 +36,13 @@ int main(int ac, char **argv)
int key;
key = IPPROTO_TCP;
- assert(bpf_map_lookup_elem(map_fd[0], &key, &tcp_cnt) == 0);
+ assert(bpf_map_lookup_elem(map_fd, &key, &tcp_cnt) == 0);
key = IPPROTO_UDP;
- assert(bpf_map_lookup_elem(map_fd[0], &key, &udp_cnt) == 0);
+ assert(bpf_map_lookup_elem(map_fd, &key, &udp_cnt) == 0);
key = IPPROTO_ICMP;
- assert(bpf_map_lookup_elem(map_fd[0], &key, &icmp_cnt) == 0);
+ assert(bpf_map_lookup_elem(map_fd, &key, &icmp_cnt) == 0);
printf("TCP %lld UDP %lld ICMP %lld bytes\n",
tcp_cnt, udp_cnt, icmp_cnt);
diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
index 125ee6efc913..e3611dbfce97 100644
--- a/samples/bpf/sockex2_user.c
+++ b/samples/bpf/sockex2_user.c
@@ -3,7 +3,7 @@
#include <assert.h>
#include <linux/bpf.h>
#include <bpf/bpf.h>
-#include "bpf_load.h"
+#include "bpf/libbpf.h"
#include "sock_example.h"
#include <unistd.h>
#include <arpa/inet.h>
@@ -17,22 +17,24 @@ struct pair {
int main(int ac, char **argv)
{
struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
+ struct bpf_object *obj;
+ int map_fd, prog_fd;
char filename[256];
- FILE *f;
int i, sock;
+ FILE *f;
snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
setrlimit(RLIMIT_MEMLOCK, &r);
- if (load_bpf_file(filename)) {
- printf("%s", bpf_log_buf);
+ if (bpf_prog_load(filename, 0, &obj, &prog_fd))
return 1;
- }
+
+ map_fd = bpf_object__find_map_fd_by_name(obj, "hash_map");
sock = open_raw_sock("lo");
- assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
- sizeof(prog_fd[0])) == 0);
+ assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
+ sizeof(prog_fd)) == 0);
f = popen("ping -4 -c5 localhost", "r");
(void) f;
@@ -41,8 +43,8 @@ int main(int ac, char **argv)
int key = 0, next_key;
struct pair value;
- while (bpf_map_get_next_key(map_fd[0], &key, &next_key) == 0) {
- bpf_map_lookup_elem(map_fd[0], &next_key, &value);
+ while (bpf_map_get_next_key(map_fd, &key, &next_key) == 0) {
+ bpf_map_lookup_elem(map_fd, &next_key, &value);
printf("ip %s bytes %lld packets %lld\n",
inet_ntoa((struct in_addr){htonl(next_key)}),
value.bytes, value.packets);
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 5/5] tools: libbpf: make sure readelf shows full names in build checks
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
readelf truncates its output by default to attempt to make it more
readable. This can lead to function names getting aliased if they
differ late in the string. Use --wide parameter to avoid
truncation.
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
tools/lib/bpf/Makefile | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
index 761691bd72ad..a05c43468bd0 100644
--- a/tools/lib/bpf/Makefile
+++ b/tools/lib/bpf/Makefile
@@ -132,9 +132,9 @@ BPF_IN := $(OUTPUT)libbpf-in.o
LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
VERSION_SCRIPT := libbpf.map
-GLOBAL_SYM_COUNT = $(shell readelf -s $(BPF_IN) | \
+GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \
awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {s++} END{print s}')
-VERSIONED_SYM_COUNT = $(shell readelf -s $(OUTPUT)libbpf.so | \
+VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so | \
grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
CMD_TARGETS = $(LIB_FILE)
--
2.19.2
^ permalink raw reply related
* [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Jakub Kicinski @ 2019-02-27 23:30 UTC (permalink / raw)
To: alexei.starovoitov, daniel; +Cc: netdev, bpf, oss-drivers, Jakub Kicinski
In-Reply-To: <20190227233046.11718-1-jakub.kicinski@netronome.com>
For historical reasons the helper to loop over maps in an object
is called bpf_map__for_each while it really should be called
bpf_object__for_each_map. Rename and add a correctly named
define for backward compatibility.
Switch all in-tree users to the correct name (Quentin).
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
---
tools/bpf/bpftool/prog.c | 4 ++--
tools/lib/bpf/libbpf.c | 8 ++++----
tools/lib/bpf/libbpf.h | 3 ++-
tools/perf/util/bpf-loader.c | 4 ++--
tools/testing/selftests/bpf/test_libbpf_open.c | 2 +-
5 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
index 0c35dd543d49..8ef80d65a474 100644
--- a/tools/bpf/bpftool/prog.c
+++ b/tools/bpf/bpftool/prog.c
@@ -1053,7 +1053,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
j = 0;
while (j < old_map_fds && map_replace[j].name) {
i = 0;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
if (!strcmp(bpf_map__name(map), map_replace[j].name)) {
map_replace[j].idx = i;
break;
@@ -1074,7 +1074,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
/* Set ifindex and name reuse */
j = 0;
idx = 0;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
if (!bpf_map__is_offload_neutral(map))
bpf_map__set_ifindex(map, ifindex);
diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
index b38dcbe7460a..f5eb60379c8d 100644
--- a/tools/lib/bpf/libbpf.c
+++ b/tools/lib/bpf/libbpf.c
@@ -2100,7 +2100,7 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
if (err)
return err;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
char buf[PATH_MAX];
int len;
@@ -2147,7 +2147,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
if (!obj)
return -ENOENT;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
char buf[PATH_MAX];
int len;
@@ -2835,7 +2835,7 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
{
struct bpf_map *pos;
- bpf_map__for_each(pos, obj) {
+ bpf_object__for_each_map(pos, obj) {
if (pos->name && !strcmp(pos->name, name))
return pos;
}
@@ -2928,7 +2928,7 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
first_prog = prog;
}
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
if (!bpf_map__is_offload_neutral(map))
map->map_ifindex = attr->ifindex;
}
diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
index 6c0168f8bba5..b4652aa1a58a 100644
--- a/tools/lib/bpf/libbpf.h
+++ b/tools/lib/bpf/libbpf.h
@@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
LIBBPF_API struct bpf_map *
bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
-#define bpf_map__for_each(pos, obj) \
+#define bpf_object__for_each_map(pos, obj) \
for ((pos) = bpf_map__next(NULL, (obj)); \
(pos) != NULL; \
(pos) = bpf_map__next((pos), (obj)))
+#define bpf_map__for_each bpf_object__for_each_map
LIBBPF_API struct bpf_map *
bpf_map__prev(struct bpf_map *map, struct bpf_object *obj);
diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
index 037d8ff6a634..31b7e5a1453b 100644
--- a/tools/perf/util/bpf-loader.c
+++ b/tools/perf/util/bpf-loader.c
@@ -1489,7 +1489,7 @@ apply_obj_config_object(struct bpf_object *obj)
struct bpf_map *map;
int err;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
err = apply_obj_config_map(map);
if (err)
return err;
@@ -1513,7 +1513,7 @@ int bpf__apply_obj_config(void)
#define bpf__for_each_map(pos, obj, objtmp) \
bpf_object__for_each_safe(obj, objtmp) \
- bpf_map__for_each(pos, obj)
+ bpf_object__for_each_map(pos, obj)
#define bpf__for_each_map_named(pos, obj, objtmp, name) \
bpf__for_each_map(pos, obj, objtmp) \
diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
index 1909ecf4d999..65cbd30704b5 100644
--- a/tools/testing/selftests/bpf/test_libbpf_open.c
+++ b/tools/testing/selftests/bpf/test_libbpf_open.c
@@ -67,7 +67,7 @@ int test_walk_maps(struct bpf_object *obj, bool verbose)
struct bpf_map *map;
int cnt = 0;
- bpf_map__for_each(map, obj) {
+ bpf_object__for_each_map(map, obj) {
cnt++;
if (verbose)
printf("Map (count:%d) name: %s\n", cnt,
--
2.19.2
^ permalink raw reply related
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: si-wei liu @ 2019-02-27 23:34 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Samudrala, Sridhar, Siwei Liu, Jiri Pirko, Stephen Hemminger,
David Miller, Netdev, virtualization, virtio-dev,
Brandeburg, Jesse, Alexander Duyck, Jakub Kicinski, Jason Wang,
liran.alon
In-Reply-To: <20190227173710-mutt-send-email-mst@kernel.org>
On 2/27/2019 2:38 PM, Michael S. Tsirkin wrote:
> On Tue, Feb 26, 2019 at 04:17:21PM -0800, si-wei liu wrote:
>>
>> On 2/25/2019 6:08 PM, Michael S. Tsirkin wrote:
>>> On Mon, Feb 25, 2019 at 04:58:07PM -0800, si-wei liu wrote:
>>>> On 2/22/2019 7:14 AM, Michael S. Tsirkin wrote:
>>>>> On Thu, Feb 21, 2019 at 11:55:11PM -0800, si-wei liu wrote:
>>>>>> On 2/21/2019 11:00 PM, Samudrala, Sridhar wrote:
>>>>>>> On 2/21/2019 7:33 PM, si-wei liu wrote:
>>>>>>>> On 2/21/2019 5:39 PM, Michael S. Tsirkin wrote:
>>>>>>>>> On Thu, Feb 21, 2019 at 05:14:44PM -0800, Siwei Liu wrote:
>>>>>>>>>> Sorry for replying to this ancient thread. There was some remaining
>>>>>>>>>> issue that I don't think the initial net_failover patch got addressed
>>>>>>>>>> cleanly, see:
>>>>>>>>>>
>>>>>>>>>> https://bugs.launchpad.net/ubuntu/+source/linux/+bug/1815268
>>>>>>>>>>
>>>>>>>>>> The renaming of 'eth0' to 'ens4' fails because the udev userspace was
>>>>>>>>>> not specifically writtten for such kernel automatic enslavement.
>>>>>>>>>> Specifically, if it is a bond or team, the slave would typically get
>>>>>>>>>> renamed *before* virtual device gets created, that's what udev can
>>>>>>>>>> control (without getting netdev opened early by the other part of
>>>>>>>>>> kernel) and other userspace components for e.g. initramfs,
>>>>>>>>>> init-scripts can coordinate well in between. The in-kernel
>>>>>>>>>> auto-enslavement of net_failover breaks this userspace convention,
>>>>>>>>>> which don't provides a solution if user care about consistent naming
>>>>>>>>>> on the slave netdevs specifically.
>>>>>>>>>>
>>>>>>>>>> Previously this issue had been specifically called out when IFF_HIDDEN
>>>>>>>>>> and the 1-netdev was proposed, but no one gives out a solution to this
>>>>>>>>>> problem ever since. Please share your mind how to proceed and solve
>>>>>>>>>> this userspace issue if netdev does not welcome a 1-netdev model.
>>>>>>>>> Above says:
>>>>>>>>>
>>>>>>>>> there's no motivation in the systemd/udevd community at
>>>>>>>>> this point to refactor the rename logic and make it work well with
>>>>>>>>> 3-netdev.
>>>>>>>>>
>>>>>>>>> What would the fix be? Skip slave devices?
>>>>>>>>>
>>>>>>>> There's nothing user can get if just skipping slave devices - the
>>>>>>>> name is still unchanged and unpredictable e.g. eth0, or eth1 the
>>>>>>>> next reboot, while the rest may conform to the naming scheme (ens3
>>>>>>>> and such). There's no way one can fix this in userspace alone - when
>>>>>>>> the failover is created the enslaved netdev was opened by the kernel
>>>>>>>> earlier than the userspace is made aware of, and there's no
>>>>>>>> negotiation protocol for kernel to know when userspace has done
>>>>>>>> initial renaming of the interface. I would expect netdev list should
>>>>>>>> at least provide the direction in general for how this can be
>>>>>>>> solved...
>>>>> I was just wondering what did you mean when you said
>>>>> "refactor the rename logic and make it work well with 3-netdev" -
>>>>> was there a proposal udev rejected?
>>>> No. I never believed this particular issue can be fixed in userspace alone.
>>>> Previously someone had said it could be, but I never see any work or
>>>> relevant discussion ever happened in various userspace communities (for e.g.
>>>> dracut, initramfs-tools, systemd, udev, and NetworkManager). IMHO the root
>>>> of the issue derives from the kernel, it makes more sense to start from
>>>> netdev, work out and decide on a solution: see what can be done in the
>>>> kernel in order to fix it, then after that engage userspace community for
>>>> the feasibility...
>>>>
>>>>> Anyway, can we write a time diagram for what happens in which order that
>>>>> leads to failure? That would help look for triggers that we can tie
>>>>> into, or add new ones.
>>>>>
>>>> See attached diagram.
>>>>
>>>>>
>>>>>
>>>>>>> Is there an issue if slave device names are not predictable? The user/admin scripts are expected
>>>>>>> to only work with the master failover device.
>>>>>> Where does this expectation come from?
>>>>>>
>>>>>> Admin users may have ethtool or tc configurations that need to deal with
>>>>>> predictable interface name. Third-party app which was built upon specifying
>>>>>> certain interface name can't be modified to chase dynamic names.
>>>>>>
>>>>>> Specifically, we have pre-canned image that uses ethtool to fine tune VF
>>>>>> offload settings post boot for specific workload. Those images won't work
>>>>>> well if the name is constantly changing just after couple rounds of live
>>>>>> migration.
>>>>> It should be possible to specify the ethtool configuration on the
>>>>> master and have it automatically propagated to the slave.
>>>>>
>>>>> BTW this is something we should look at IMHO.
>>>> I was elaborating a few examples that the expectation and assumption that
>>>> user/admin scripts only deal with master failover device is incorrect. It
>>>> had never been taken good care of, although I did try to emphasize it from
>>>> the very beginning.
>>>>
>>>> Basically what you said about propagating the ethtool configuration down to
>>>> the slave is the key pursuance of 1-netdev model. However, what I am seeking
>>>> now is any alternative that can also fix the specific udev rename problem,
>>>> before concluding that 1-netdev is the only solution. Generally a 1-netdev
>>>> scheme would take time to implement, while I'm trying to find a way out to
>>>> fix this particular naming problem under 3-netdev.
>>>>
>>>>>>> Moreover, you were suggesting hiding the lower slave devices anyway. There was some discussion
>>>>>>> about moving them to a hidden network namespace so that they are not visible from the default namespace.
>>>>>>> I looked into this sometime back, but did not find the right kernel api to create a network namespace within
>>>>>>> kernel. If so, we could use this mechanism to simulate a 1-netdev model.
>>>>>> Yes, that's one possible implementation (IMHO the key is to make 1-netdev
>>>>>> model as much transparent to a real NIC as possible, while a hidden netns is
>>>>>> just the vehicle). However, I recall there was resistance around this
>>>>>> discussion that even the concept of hiding itself is a taboo for Linux
>>>>>> netdev. I would like to summon potential alternatives before concluding
>>>>>> 1-netdev is the only solution too soon.
>>>>>>
>>>>>> Thanks,
>>>>>> -Siwei
>>>>> Your scripts would not work at all then, right?
>>>> At this point we don't claim images with such usage as SR-IOV live
>>>> migrate-able. We would flag it as live migrate-able until this ethtool
>>>> config issue is fully addressed and a transparent live migration solution
>>>> emerges in upstream eventually.
>>>>
>>>>
>>>> Thanks,
>>>> -Siwei
>>>>>>>> -Siwei
>>>>>>>>
>>>>>>>>
>>>>> ---------------------------------------------------------------------
>>>>> To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
>>>>> For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
>>>>>
>>>> net_failover(kernel) | network.service (user) | systemd-udevd (user)
>>>> --------------------------------------------------+------------------------------+--------------------------------------------
>>>> (standby virtio-net and net_failover | |
>>>> devices created and initialized, | |
>>>> i.e. virtnet_probe()-> | |
>>>> net_failover_create() | |
>>>> was done.) | |
>>>> | |
>>>> | runs `ifup ens3' -> |
>>>> | ip link set dev ens3 up |
>>>> net_failover_open() | |
>>>> dev_open(virtnet_dev) | |
>>>> virtnet_open(virtnet_dev) | |
>>>> netif_carrier_on(failover_dev) | |
>>>> ... | |
>>>> | |
>>>> (VF hot plugged in) | |
>>>> ixgbevf_probe() | |
>>>> register_netdev(ixgbevf_netdev) | |
>>>> netdev_register_kobject(ixgbevf_netdev) | |
>>>> kobject_add(ixgbevf_dev) | |
>>>> device_add(ixgbevf_dev) | |
>>>> kobject_uevent(&ixgbevf_dev->kobj, KOBJ_ADD) | |
>>>> netlink_broadcast() | |
>>>> ... | |
>>>> call_netdevice_notifiers(NETDEV_REGISTER) | |
>>>> failover_event(..., NETDEV_REGISTER, ...) | |
>>>> failover_slave_register(ixgbevf_netdev) | |
>>>> net_failover_slave_register(ixgbevf_netdev) | |
>>>> dev_open(ixgbevf_netdev) | |
>>>> | |
>>>> | |
>>>> | | received ADD uevent from netlink fd
>>>> | | ...
>>>> | | udev-builtin-net_id.c:dev_pci_slot()
>>>> | | (decided to renamed 'eth0' )
>>>> | | ip link set dev eth0 name ens4
>>>> (dev_change_name() returns -EBUSY as | |
>>>> ixgbevf_netdev->flags has IFF_UP) | |
>>>> | |
>>>>
>>> Given renaming slaves does not work anyway:
>> I was actually thinking what if we relieve the rename restriction just for
>> the failover slave? What the impact would be? I think users don't care about
>> slave being renamed when it's in use, especially the initial rename.
>> Thoughts?
>>
>>> would it work if we just
>>> hard-coded slave names instead?
>>>
>>> E.g.
>>> 1. fail slave renames
>>> 2. rename of failover to XX automatically renames standby to XXnsby
>>> and primary to XXnpry
>> That wouldn't help. The time when the failover master gets renamed, the VF
>> may not be present.
> In this scheme if VF is not there it will be renamed immediately after registration.
Who will be responsible to rename the slave, the kernel? Note the
master's name may or may not come from the userspace. If it comes from
the userspace, should the userspace daemon change their expectation not
to name/rename _any_ slaves (today there's no distinction)? How do users
know which name to trust, depending on which wins the race more often?
Say if kernel wants a ens3npry name while userspace wants it named as ens4.
-Siwei
>
>> I don't like the idea to delay exposing failover master
>> until VF is hot plugged in (probably subject to various failures) later.
>>
>> Thanks,
>> -Siwei
>
> I agree, this was not what I meant.
>
>>>
^ permalink raw reply
* Re: [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Andrii Nakryiko @ 2019-02-27 23:34 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190227212256.3856473-1-yhs@fb.com>
On Wed, Feb 27, 2019 at 1:23 PM Yonghong Song <yhs@fb.com> wrote:
>
> Commit d83525ca62cf ("bpf: introduce bpf_spin_lock")
> introduced bpf_spin_lock and the field spin_lock_off
> in kernel internal structure bpf_map has the following
> meaning:
> >=0 valid offset, <0 error
>
> For every map created, the kernel will ensure
> spin_lock_off has correct value.
>
> Currently, bpf_map->spin_lock_off is not copied
> from the inner map to the map_in_map inner_map_meta
> during a map_in_map type map creation, so
> inner_map_meta->spin_lock_off = 0.
> This will give verifier wrong information that
> inner_map has bpf_spin_lock and the bpf_spin_lock
> is defined at offset 0. An access to offset 0
> of a value pointer will trigger the following error:
> bpf_spin_lock cannot be accessed directly by load/store
>
> This patch fixed the issue by copy inner map's spin_lock_off
> value to inner_map_meta->spin_lock_off.
>
> Fixes: d83525ca62cf ("bpf: introduce bpf_spin_lock")
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> kernel/bpf/map_in_map.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/kernel/bpf/map_in_map.c b/kernel/bpf/map_in_map.c
> index 583346a0ab29..3dff41403583 100644
> --- a/kernel/bpf/map_in_map.c
> +++ b/kernel/bpf/map_in_map.c
> @@ -58,6 +58,7 @@ struct bpf_map *bpf_map_meta_alloc(int inner_map_ufd)
> inner_map_meta->value_size = inner_map->value_size;
> inner_map_meta->map_flags = inner_map->map_flags;
> inner_map_meta->max_entries = inner_map->max_entries;
> + inner_map_meta->spin_lock_off = inner_map->spin_lock_off;
Looks like spinlock inside inner map is not supported: there is
specific check few lines above returning -ENOSUPP for such case. In
that case, maybe assign -1 here to make this explicit?
Though I guess that also brings up the question: is there any harm in
supporting spin lock for inner map and why it was disabled in the
first place?
>
> /* Misc members not needed in bpf_map_meta_equal() check. */
> inner_map_meta->ops = inner_map->ops;
> --
> 2.17.1
>
^ permalink raw reply
* Re: [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
From: Andrii Nakryiko @ 2019-02-27 23:36 UTC (permalink / raw)
To: Yonghong Song; +Cc: netdev, Alexei Starovoitov, Daniel Borkmann, Kernel Team
In-Reply-To: <20190227212257.3856619-1-yhs@fb.com>
On Wed, Feb 27, 2019 at 1:24 PM Yonghong Song <yhs@fb.com> wrote:
>
> The bpf_map_lookup_elem is added in the bpf program.
> Without previous patch, the test change will trigger the
> following error:
> $ ./test_maps
> ...
> ; value_p = bpf_map_lookup_elem(map, &key);
> 20: (bf) r1 = r7
> 21: (bf) r2 = r8
> 22: (85) call bpf_map_lookup_elem#1
> ; if (!value_p || *value_p != 123)
> 23: (15) if r0 == 0x0 goto pc+16
> R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
> R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
> ; if (!value_p || *value_p != 123)
> 24: (61) r1 = *(u32 *)(r0 +0)
> R0=map_value(id=2,off=0,ks=4,vs=4,imm=0) R6=inv1 R7=map_ptr(id=0,off=0,ks=4,vs=4,imm=0)
> R8=fp-8,call_-1 R10=fp0,call_-1 fp-8=mmmmmmmm
> bpf_spin_lock cannot be accessed directly by load/store
>
> With the kernel fix in the previous commit, the error goes away.
>
> Signed-off-by: Yonghong Song <yhs@fb.com>
> ---
> tools/testing/selftests/bpf/progs/test_map_in_map.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/progs/test_map_in_map.c b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> index ce923e67e08e..2985f262846e 100644
> --- a/tools/testing/selftests/bpf/progs/test_map_in_map.c
> +++ b/tools/testing/selftests/bpf/progs/test_map_in_map.c
> @@ -27,6 +27,7 @@ SEC("xdp_mimtest")
> int xdp_mimtest0(struct xdp_md *ctx)
> {
> int value = 123;
> + int *value_p;
> int key = 0;
> void *map;
>
> @@ -35,6 +36,9 @@ int xdp_mimtest0(struct xdp_md *ctx)
> return XDP_DROP;
>
> bpf_map_update_elem(map, &key, &value, 0);
> + value_p = bpf_map_lookup_elem(map, &key);
> + if (!value_p || *value_p != 123)
> + return XDP_DROP;
>
> map = bpf_map_lookup_elem(&mim_hash, &key);
> if (!map)
> --
> 2.17.1
>
LGTM.
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply
* Re: [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
From: Vivien Didelot @ 2019-02-27 23:40 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller,
netdev@vger.kernel.org
In-Reply-To: <864ac062-51cb-ec9e-388b-b74a62eb6e6a@gmail.com>
Hi Heiner,
On Wed, 27 Feb 2019 20:55:22 +0100, Heiner Kallweit <hkallweit1@gmail.com> wrote:
> When debugging another issue I faced an interrupt storm in this
> driver (88E6390, port 9 in SGMII mode), consisting of alternating
> link-up / link-down interrupts. Analysis showed that the driver
> wanted to set a cmode that was set already. But so far
> mv88e6390x_port_set_cmode() doesn't check this and powers down
> SERDES, what causes the link to break, and eventually results in
> the described interrupt storm.
>
> Fix this by checking whether the cmode actually changes. We want
> that the very first call to mv88e6390x_port_set_cmode() always
> configures the registers, therefore initialize port.cmode with
> a value that is different from any supported cmode value.
>
> Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
> drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
> drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
> drivers/net/dsa/mv88e6xxx/port.h | 1 +
> 3 files changed, 9 insertions(+)
>
> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
> index 32e7af5ca..d4edb61e8 100644
> --- a/drivers/net/dsa/mv88e6xxx/chip.c
> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
> @@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
> static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
> {
> struct mv88e6xxx_chip *chip;
> + int i;
>
> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
> if (!chip)
> @@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
> mutex_init(&chip->reg_lock);
> INIT_LIST_HEAD(&chip->mdios);
>
> + for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
mv88e6xxx_num_ports(chip)
> + chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
> +
> return chip;
> }
>
> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
> index ebd26b6a9..70b7a1463 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.c
> +++ b/drivers/net/dsa/mv88e6xxx/port.c
> @@ -398,6 +398,10 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
> cmode = 0;
> }
>
> + /* cmode doesn't change, nothing to do for us */
> + if (cmode == chip->ports[port].cmode)
> + return 0;
> +
> lane = mv88e6390x_serdes_get_lane(chip, port);
> if (lane < 0)
> return lane;
> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
> index e583641de..4aadf321e 100644
> --- a/drivers/net/dsa/mv88e6xxx/port.h
> +++ b/drivers/net/dsa/mv88e6xxx/port.h
> @@ -52,6 +52,7 @@
> #define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
> #define MV88E6185_PORT_STS_CMODE_PHY 0x0006
> #define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
> +#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
Is this 0xff a mask value from the Port Status register? If so please
the 0x1234 format like above, to make this mask value obvious.
>
> /* Offset 0x01: MAC (or PCS or Physical) Control Register */
> #define MV88E6XXX_PORT_MAC_CTL 0x01
Thanks,
Vivien
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Andrii Nakryiko @ 2019-02-27 23:47 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-4-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> For historical reasons the helper to loop over maps in an object
> is called bpf_map__for_each while it really should be called
> bpf_object__for_each_map. Rename and add a correctly named
> define for backward compatibility.
Seems like there are at least 3 more functions that are not named correctly:
- __bpf_map__iter (__bpf_object__iter_map?)
- bpf_map__next (=> bpf_object__next_map?)
- bpf_map__prev (=> bpf_object__prev_map?)
Let's rename them as well?
>
> Switch all in-tree users to the correct name (Quentin).
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> tools/bpf/bpftool/prog.c | 4 ++--
> tools/lib/bpf/libbpf.c | 8 ++++----
> tools/lib/bpf/libbpf.h | 3 ++-
> tools/perf/util/bpf-loader.c | 4 ++--
> tools/testing/selftests/bpf/test_libbpf_open.c | 2 +-
> 5 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/tools/bpf/bpftool/prog.c b/tools/bpf/bpftool/prog.c
> index 0c35dd543d49..8ef80d65a474 100644
> --- a/tools/bpf/bpftool/prog.c
> +++ b/tools/bpf/bpftool/prog.c
> @@ -1053,7 +1053,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> j = 0;
> while (j < old_map_fds && map_replace[j].name) {
> i = 0;
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> if (!strcmp(bpf_map__name(map), map_replace[j].name)) {
> map_replace[j].idx = i;
> break;
> @@ -1074,7 +1074,7 @@ static int load_with_options(int argc, char **argv, bool first_prog_only)
> /* Set ifindex and name reuse */
> j = 0;
> idx = 0;
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> if (!bpf_map__is_offload_neutral(map))
> bpf_map__set_ifindex(map, ifindex);
>
> diff --git a/tools/lib/bpf/libbpf.c b/tools/lib/bpf/libbpf.c
> index b38dcbe7460a..f5eb60379c8d 100644
> --- a/tools/lib/bpf/libbpf.c
> +++ b/tools/lib/bpf/libbpf.c
> @@ -2100,7 +2100,7 @@ int bpf_object__pin_maps(struct bpf_object *obj, const char *path)
> if (err)
> return err;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> char buf[PATH_MAX];
> int len;
>
> @@ -2147,7 +2147,7 @@ int bpf_object__unpin_maps(struct bpf_object *obj, const char *path)
> if (!obj)
> return -ENOENT;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> char buf[PATH_MAX];
> int len;
>
> @@ -2835,7 +2835,7 @@ bpf_object__find_map_by_name(struct bpf_object *obj, const char *name)
> {
> struct bpf_map *pos;
>
> - bpf_map__for_each(pos, obj) {
> + bpf_object__for_each_map(pos, obj) {
> if (pos->name && !strcmp(pos->name, name))
> return pos;
> }
> @@ -2928,7 +2928,7 @@ int bpf_prog_load_xattr(const struct bpf_prog_load_attr *attr,
> first_prog = prog;
> }
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> if (!bpf_map__is_offload_neutral(map))
> map->map_ifindex = attr->ifindex;
> }
> diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> index 6c0168f8bba5..b4652aa1a58a 100644
> --- a/tools/lib/bpf/libbpf.h
> +++ b/tools/lib/bpf/libbpf.h
> @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
>
> LIBBPF_API struct bpf_map *
> bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> -#define bpf_map__for_each(pos, obj) \
> +#define bpf_object__for_each_map(pos, obj) \
> for ((pos) = bpf_map__next(NULL, (obj)); \
> (pos) != NULL; \
> (pos) = bpf_map__next((pos), (obj)))
> +#define bpf_map__for_each bpf_object__for_each_map
Should we get rid of this as well, instead of accumulating cruft?
>
> LIBBPF_API struct bpf_map *
> bpf_map__prev(struct bpf_map *map, struct bpf_object *obj);
> diff --git a/tools/perf/util/bpf-loader.c b/tools/perf/util/bpf-loader.c
> index 037d8ff6a634..31b7e5a1453b 100644
> --- a/tools/perf/util/bpf-loader.c
> +++ b/tools/perf/util/bpf-loader.c
> @@ -1489,7 +1489,7 @@ apply_obj_config_object(struct bpf_object *obj)
> struct bpf_map *map;
> int err;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> err = apply_obj_config_map(map);
> if (err)
> return err;
> @@ -1513,7 +1513,7 @@ int bpf__apply_obj_config(void)
>
> #define bpf__for_each_map(pos, obj, objtmp) \
> bpf_object__for_each_safe(obj, objtmp) \
> - bpf_map__for_each(pos, obj)
> + bpf_object__for_each_map(pos, obj)
>
> #define bpf__for_each_map_named(pos, obj, objtmp, name) \
> bpf__for_each_map(pos, obj, objtmp) \
> diff --git a/tools/testing/selftests/bpf/test_libbpf_open.c b/tools/testing/selftests/bpf/test_libbpf_open.c
> index 1909ecf4d999..65cbd30704b5 100644
> --- a/tools/testing/selftests/bpf/test_libbpf_open.c
> +++ b/tools/testing/selftests/bpf/test_libbpf_open.c
> @@ -67,7 +67,7 @@ int test_walk_maps(struct bpf_object *obj, bool verbose)
> struct bpf_map *map;
> int cnt = 0;
>
> - bpf_map__for_each(map, obj) {
> + bpf_object__for_each_map(map, obj) {
> cnt++;
> if (verbose)
> printf("Map (count:%d) name: %s\n", cnt,
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH net] net: dsa: mv88e6xxx: prevent interrupt storm caused by mv88e6390x_port_set_cmode
From: Heiner Kallweit @ 2019-02-27 23:48 UTC (permalink / raw)
To: Vivien Didelot
Cc: Andrew Lunn, Florian Fainelli, David Miller,
netdev@vger.kernel.org
In-Reply-To: <20190227184040.GB23759@t480s.localdomain>
On 28.02.2019 00:40, Vivien Didelot wrote:
> Hi Heiner,
>
> On Wed, 27 Feb 2019 20:55:22 +0100, Heiner Kallweit <hkallweit1@gmail.com> wrote:
>> When debugging another issue I faced an interrupt storm in this
>> driver (88E6390, port 9 in SGMII mode), consisting of alternating
>> link-up / link-down interrupts. Analysis showed that the driver
>> wanted to set a cmode that was set already. But so far
>> mv88e6390x_port_set_cmode() doesn't check this and powers down
>> SERDES, what causes the link to break, and eventually results in
>> the described interrupt storm.
>>
>> Fix this by checking whether the cmode actually changes. We want
>> that the very first call to mv88e6390x_port_set_cmode() always
>> configures the registers, therefore initialize port.cmode with
>> a value that is different from any supported cmode value.
>>
>> Fixes: 364e9d7776a3 ("net: dsa: mv88e6xxx: Power on/off SERDES on cmode change")
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>> drivers/net/dsa/mv88e6xxx/chip.c | 4 ++++
>> drivers/net/dsa/mv88e6xxx/port.c | 4 ++++
>> drivers/net/dsa/mv88e6xxx/port.h | 1 +
>> 3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
>> index 32e7af5ca..d4edb61e8 100644
>> --- a/drivers/net/dsa/mv88e6xxx/chip.c
>> +++ b/drivers/net/dsa/mv88e6xxx/chip.c
>> @@ -4568,6 +4568,7 @@ static int mv88e6xxx_detect(struct mv88e6xxx_chip *chip)
>> static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>> {
>> struct mv88e6xxx_chip *chip;
>> + int i;
>>
>> chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL);
>> if (!chip)
>> @@ -4578,6 +4579,9 @@ static struct mv88e6xxx_chip *mv88e6xxx_alloc_chip(struct device *dev)
>> mutex_init(&chip->reg_lock);
>> INIT_LIST_HEAD(&chip->mdios);
>>
>> + for (i = 0; i < ARRAY_SIZE(chip->ports); i++)
>
> mv88e6xxx_num_ports(chip)
>
OK
>> + chip->ports[i].cmode = MV88E6XXX_PORT_STS_CMODE_INVALID;
>> +
>> return chip;
>> }
>>
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.c b/drivers/net/dsa/mv88e6xxx/port.c
>> index ebd26b6a9..70b7a1463 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.c
>> +++ b/drivers/net/dsa/mv88e6xxx/port.c
>> @@ -398,6 +398,10 @@ int mv88e6390x_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
>> cmode = 0;
>> }
>>
>> + /* cmode doesn't change, nothing to do for us */
>> + if (cmode == chip->ports[port].cmode)
>> + return 0;
>> +
>> lane = mv88e6390x_serdes_get_lane(chip, port);
>> if (lane < 0)
>> return lane;
>> diff --git a/drivers/net/dsa/mv88e6xxx/port.h b/drivers/net/dsa/mv88e6xxx/port.h
>> index e583641de..4aadf321e 100644
>> --- a/drivers/net/dsa/mv88e6xxx/port.h
>> +++ b/drivers/net/dsa/mv88e6xxx/port.h
>> @@ -52,6 +52,7 @@
>> #define MV88E6185_PORT_STS_CMODE_1000BASE_X 0x0005
>> #define MV88E6185_PORT_STS_CMODE_PHY 0x0006
>> #define MV88E6185_PORT_STS_CMODE_DISABLED 0x0007
>> +#define MV88E6XXX_PORT_STS_CMODE_INVALID 0xff
>
> Is this 0xff a mask value from the Port Status register? If so please
> the 0x1234 format like above, to make this mask value obvious.
>
No, it's not a mask. I couldn't use 0 for "invalid" because 0 is used
otherwise, so I went with 0xff. All these STS_CMODE values are also
stored in mv88e6xxx_port.cmode that has type u8. Therefore I decided
to write it as an 8 bit value.
>>
>> /* Offset 0x01: MAC (or PCS or Physical) Control Register */
>> #define MV88E6XXX_PORT_MAC_CTL 0x01
>
>
> Thanks,
>
> Vivien
>
Heiner
^ 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-27 23:50 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: <c72ce9eb-254c-cc3e-1969-f7f108506d5e@oracle.com>
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.
> 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: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Heiner Kallweit @ 2019-02-27 23:51 UTC (permalink / raw)
To: Andrew Lunn, Russell King - ARM Linux admin
Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190227231940.GN3809@lunn.ch>
On 28.02.2019 00:19, Andrew Lunn wrote:
>> >From what you've described, it sounds like what you actually have is:
>>
>> MAC <---> Serdes PHY <---> PHY
>>
>> The Serdes PHY receives the SGMII in-band negotiation from the external
>> PHY, but there is no propagation of the status from the serdes PHY to
>> the MAC.
>
> Yes, that is a good description. So far, we have not yet got the MAC
> to read the speed and duplex from the SERDES to configure itself. In
> theory it should be able to, it is all in the same device.
>
> It might be that once the SERDES interrupts saying it has link we need
> to program the MAC with the result of the in-band signalling. in-band
> then seems a bit pointless. Or we are missing some configuration
> somewhere to tell the MAC to use the in-band signalling result from
> the SERDES.
>
I'll play a little with it and see whether I get it working.
As a fallback we have the option to go with out-of-band.
> Andrew
>
>
Heiner
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Jakub Kicinski @ 2019-02-27 23:57 UTC (permalink / raw)
To: Andrii Nakryiko
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <CAEf4BzZR+7R9TjXSMHiYzbHCvuwL+Z_LBD=Pk+rRW+X+pgdg4w@mail.gmail.com>
On Wed, 27 Feb 2019 15:47:56 -0800, Andrii Nakryiko wrote:
> On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
> >
> > For historical reasons the helper to loop over maps in an object
> > is called bpf_map__for_each while it really should be called
> > bpf_object__for_each_map. Rename and add a correctly named
> > define for backward compatibility.
>
> Seems like there are at least 3 more functions that are not named correctly:
> - __bpf_map__iter (__bpf_object__iter_map?)
> - bpf_map__next (=> bpf_object__next_map?)
> - bpf_map__prev (=> bpf_object__prev_map?)
>
> Let's rename them as well?
At least those are consistently named between programs and maps.
I'm happy to do the rename if we don't need backward compat, seems
a little much to add aliases?
> > Switch all in-tree users to the correct name (Quentin).
> >
> > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > index 6c0168f8bba5..b4652aa1a58a 100644
> > --- a/tools/lib/bpf/libbpf.h
> > +++ b/tools/lib/bpf/libbpf.h
> > @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
> >
> > LIBBPF_API struct bpf_map *
> > bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> > -#define bpf_map__for_each(pos, obj) \
> > +#define bpf_object__for_each_map(pos, obj) \
> > for ((pos) = bpf_map__next(NULL, (obj)); \
> > (pos) != NULL; \
> > (pos) = bpf_map__next((pos), (obj)))
> > +#define bpf_map__for_each bpf_object__for_each_map
>
> Should we get rid of this as well, instead of accumulating cruft?
Well, we did some gymnastics in the past to maintain backward compat,
I thought we do need it..?
^ 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: Liran Alon @ 2019-02-28 0:00 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, Jakub Kicinski,
Jason Wang
In-Reply-To: <20190227184601-mutt-send-email-mst@kernel.org>
> On 28 Feb 2019, at 1:50, Michael S. Tsirkin <mst@redhat.com> 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://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.launchpad.net_ubuntu_-2Bsource_linux_-2Bbug_1815268&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Jk6Q8nNzkQ6LJ6g42qARkg6ryIDGQr-yKXPNGZbpTx0&m=aL-QfUoSYx8r0XCOBkcDtF8f-cYxrJI3skYLFTb8XJE&s=yk6Nqv3a6_JMzyrXKY67h00FyNrDJyQ-PYMFffDSTXM&e=
>>>>>>>>>>>>
>>>>>>>>>>>> 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.
>
>> 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’m fond of this idea and I have similar opinion.
I think it simplifies the issue here.
I don’t see a real reason for customer to define udev rule to rename a net-failover slave to have different postfix.
-Liran
>
>
>>>
>>>> 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: Stephen Hemminger @ 2019-02-28 0:03 UTC (permalink / raw)
To: Michael S. Tsirkin
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: <20190227184601-mutt-send-email-mst@kernel.org>
On Wed, 27 Feb 2019 18:50:44 -0500
"Michael S. Tsirkin" <mst@redhat.com> 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.
>
> > 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.
Similar schemes (with kernel providing naming) were also previously rejected
upstream. It has been a consistent theme that the kernel should not be in
the renaming business. It will certainly break userspace.
^ permalink raw reply
* Re: [PATCH bpf-next 4/5] samples: bpf: use libbpf where easy
From: Andrii Nakryiko @ 2019-02-28 0:05 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-5-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> Some samples don't really need the magic of bpf_load,
> switch them to libbpf.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> samples/bpf/Makefile | 6 +++---
> samples/bpf/fds_example.c | 9 ++++++---
> samples/bpf/sockex1_user.c | 22 ++++++++++++----------
> samples/bpf/sockex2_user.c | 20 +++++++++++---------
> 4 files changed, 32 insertions(+), 25 deletions(-)
>
> diff --git a/samples/bpf/Makefile b/samples/bpf/Makefile
> index 4dd98100678e..0c62ac39c697 100644
> --- a/samples/bpf/Makefile
> +++ b/samples/bpf/Makefile
> @@ -59,9 +59,9 @@ LIBBPF = $(TOOLS_PATH)/lib/bpf/libbpf.a
> CGROUP_HELPERS := ../../tools/testing/selftests/bpf/cgroup_helpers.o
> TRACE_HELPERS := ../../tools/testing/selftests/bpf/trace_helpers.o
>
> -fds_example-objs := bpf_load.o fds_example.o
> -sockex1-objs := bpf_load.o sockex1_user.o
> -sockex2-objs := bpf_load.o sockex2_user.o
> +fds_example-objs := fds_example.o
> +sockex1-objs := sockex1_user.o
> +sockex2-objs := sockex2_user.o
> sockex3-objs := bpf_load.o sockex3_user.o
> tracex1-objs := bpf_load.o tracex1_user.o
> tracex2-objs := bpf_load.o tracex2_user.o
> diff --git a/samples/bpf/fds_example.c b/samples/bpf/fds_example.c
> index 9854854f05d1..36f1f18aae3c 100644
> --- a/samples/bpf/fds_example.c
> +++ b/samples/bpf/fds_example.c
> @@ -14,8 +14,8 @@
>
> #include <bpf/bpf.h>
>
> +#include "bpf/libbpf.h"
> #include "bpf_insn.h"
> -#include "bpf_load.h"
> #include "sock_example.h"
>
> #define BPF_F_PIN (1 << 0)
> @@ -57,10 +57,13 @@ static int bpf_prog_create(const char *object)
> BPF_EXIT_INSN(),
> };
> size_t insns_cnt = sizeof(insns) / sizeof(struct bpf_insn);
> + char bpf_log_buf[BPF_LOG_BUF_SIZE];
> + struct bpf_object *obj;
> + int prog_fd;
>
> if (object) {
> - assert(!load_bpf_file((char *)object));
> - return prog_fd[0];
> + assert(!bpf_prog_load(object, 0, &obj, &prog_fd));
Here and in few more places below: is it possible to specify correct
bpf_prog_type instead of 0?
> + return prog_fd;
> } else {
> return bpf_load_program(BPF_PROG_TYPE_SOCKET_FILTER,
> insns, insns_cnt, "GPL", 0,
> diff --git a/samples/bpf/sockex1_user.c b/samples/bpf/sockex1_user.c
> index be8ba5686924..5e7c4be3e645 100644
> --- a/samples/bpf/sockex1_user.c
> +++ b/samples/bpf/sockex1_user.c
> @@ -3,28 +3,30 @@
> #include <assert.h>
> #include <linux/bpf.h>
> #include <bpf/bpf.h>
> -#include "bpf_load.h"
> +#include "bpf/libbpf.h"
> #include "sock_example.h"
> #include <unistd.h>
> #include <arpa/inet.h>
>
> int main(int ac, char **argv)
> {
> + struct bpf_object *obj;
> + int map_fd, prog_fd;
> char filename[256];
> - FILE *f;
> int i, sock;
> + FILE *f;
>
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
>
> - if (load_bpf_file(filename)) {
> - printf("%s", bpf_log_buf);
> + if (bpf_prog_load(filename, 0, &obj, &prog_fd))
> return 1;
> - }
> +
> + map_fd = bpf_object__find_map_fd_by_name(obj, "my_map");
>
> sock = open_raw_sock("lo");
>
> - assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
> - sizeof(prog_fd[0])) == 0);
> + assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
> + sizeof(prog_fd)) == 0);
>
> f = popen("ping -4 -c5 localhost", "r");
> (void) f;
> @@ -34,13 +36,13 @@ int main(int ac, char **argv)
> int key;
>
> key = IPPROTO_TCP;
> - assert(bpf_map_lookup_elem(map_fd[0], &key, &tcp_cnt) == 0);
> + assert(bpf_map_lookup_elem(map_fd, &key, &tcp_cnt) == 0);
>
> key = IPPROTO_UDP;
> - assert(bpf_map_lookup_elem(map_fd[0], &key, &udp_cnt) == 0);
> + assert(bpf_map_lookup_elem(map_fd, &key, &udp_cnt) == 0);
>
> key = IPPROTO_ICMP;
> - assert(bpf_map_lookup_elem(map_fd[0], &key, &icmp_cnt) == 0);
> + assert(bpf_map_lookup_elem(map_fd, &key, &icmp_cnt) == 0);
>
> printf("TCP %lld UDP %lld ICMP %lld bytes\n",
> tcp_cnt, udp_cnt, icmp_cnt);
> diff --git a/samples/bpf/sockex2_user.c b/samples/bpf/sockex2_user.c
> index 125ee6efc913..e3611dbfce97 100644
> --- a/samples/bpf/sockex2_user.c
> +++ b/samples/bpf/sockex2_user.c
> @@ -3,7 +3,7 @@
> #include <assert.h>
> #include <linux/bpf.h>
> #include <bpf/bpf.h>
> -#include "bpf_load.h"
> +#include "bpf/libbpf.h"
> #include "sock_example.h"
> #include <unistd.h>
> #include <arpa/inet.h>
> @@ -17,22 +17,24 @@ struct pair {
> int main(int ac, char **argv)
> {
> struct rlimit r = {RLIM_INFINITY, RLIM_INFINITY};
> + struct bpf_object *obj;
> + int map_fd, prog_fd;
> char filename[256];
> - FILE *f;
> int i, sock;
> + FILE *f;
>
> snprintf(filename, sizeof(filename), "%s_kern.o", argv[0]);
> setrlimit(RLIMIT_MEMLOCK, &r);
>
> - if (load_bpf_file(filename)) {
> - printf("%s", bpf_log_buf);
> + if (bpf_prog_load(filename, 0, &obj, &prog_fd))
> return 1;
> - }
> +
> + map_fd = bpf_object__find_map_fd_by_name(obj, "hash_map");
>
> sock = open_raw_sock("lo");
>
> - assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, prog_fd,
> - sizeof(prog_fd[0])) == 0);
> + assert(setsockopt(sock, SOL_SOCKET, SO_ATTACH_BPF, &prog_fd,
> + sizeof(prog_fd)) == 0);
>
> f = popen("ping -4 -c5 localhost", "r");
> (void) f;
> @@ -41,8 +43,8 @@ int main(int ac, char **argv)
> int key = 0, next_key;
> struct pair value;
>
> - while (bpf_map_get_next_key(map_fd[0], &key, &next_key) == 0) {
> - bpf_map_lookup_elem(map_fd[0], &next_key, &value);
> + while (bpf_map_get_next_key(map_fd, &key, &next_key) == 0) {
> + bpf_map_lookup_elem(map_fd, &next_key, &value);
> printf("ip %s bytes %lld packets %lld\n",
> inet_ntoa((struct in_addr){htonl(next_key)}),
> value.bytes, value.packets);
> --
> 2.19.2
>
^ permalink raw reply
* Re: [PATCH bpf-next 3/5] tools: libbpf: add a correctly named define for map iteration
From: Andrii Nakryiko @ 2019-02-28 0:09 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227155703.121514a2@cakuba.netronome.com>
On Wed, Feb 27, 2019 at 3:57 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> On Wed, 27 Feb 2019 15:47:56 -0800, Andrii Nakryiko wrote:
> > On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
> > <jakub.kicinski@netronome.com> wrote:
> > >
> > > For historical reasons the helper to loop over maps in an object
> > > is called bpf_map__for_each while it really should be called
> > > bpf_object__for_each_map. Rename and add a correctly named
> > > define for backward compatibility.
> >
> > Seems like there are at least 3 more functions that are not named correctly:
> > - __bpf_map__iter (__bpf_object__iter_map?)
> > - bpf_map__next (=> bpf_object__next_map?)
> > - bpf_map__prev (=> bpf_object__prev_map?)
> >
> > Let's rename them as well?
>
> At least those are consistently named between programs and maps.
> I'm happy to do the rename if we don't need backward compat, seems
> a little much to add aliases?
>
> > > Switch all in-tree users to the correct name (Quentin).
> > >
> > > Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
>
> > > diff --git a/tools/lib/bpf/libbpf.h b/tools/lib/bpf/libbpf.h
> > > index 6c0168f8bba5..b4652aa1a58a 100644
> > > --- a/tools/lib/bpf/libbpf.h
> > > +++ b/tools/lib/bpf/libbpf.h
> > > @@ -278,10 +278,11 @@ bpf_object__find_map_by_offset(struct bpf_object *obj, size_t offset);
> > >
> > > LIBBPF_API struct bpf_map *
> > > bpf_map__next(struct bpf_map *map, struct bpf_object *obj);
> > > -#define bpf_map__for_each(pos, obj) \
> > > +#define bpf_object__for_each_map(pos, obj) \
> > > for ((pos) = bpf_map__next(NULL, (obj)); \
> > > (pos) != NULL; \
> > > (pos) = bpf_map__next((pos), (obj)))
> > > +#define bpf_map__for_each bpf_object__for_each_map
> >
> > Should we get rid of this as well, instead of accumulating cruft?
>
> Well, we did some gymnastics in the past to maintain backward compat,
> I thought we do need it..?
I'll let others chime in, but, imo, it feels like while we are at
0.0.2 it might be worthwhile to streamline libbpf early on to reduce
confusion later at the expense of early adopters having to do
straightforward conversion right now.
^ permalink raw reply
* Re: [PATCH bpf-next 5/5] tools: libbpf: make sure readelf shows full names in build checks
From: Andrii Nakryiko @ 2019-02-28 0:11 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Alexei Starovoitov, Daniel Borkmann, netdev, bpf, oss-drivers
In-Reply-To: <20190227233046.11718-6-jakub.kicinski@netronome.com>
On Wed, Feb 27, 2019 at 3:31 PM Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
>
> readelf truncates its output by default to attempt to make it more
> readable. This can lead to function names getting aliased if they
> differ late in the string. Use --wide parameter to avoid
> truncation.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Quentin Monnet <quentin.monnet@netronome.com>
> ---
> tools/lib/bpf/Makefile | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/lib/bpf/Makefile b/tools/lib/bpf/Makefile
> index 761691bd72ad..a05c43468bd0 100644
> --- a/tools/lib/bpf/Makefile
> +++ b/tools/lib/bpf/Makefile
> @@ -132,9 +132,9 @@ BPF_IN := $(OUTPUT)libbpf-in.o
> LIB_FILE := $(addprefix $(OUTPUT),$(LIB_FILE))
> VERSION_SCRIPT := libbpf.map
>
> -GLOBAL_SYM_COUNT = $(shell readelf -s $(BPF_IN) | \
> +GLOBAL_SYM_COUNT = $(shell readelf -s --wide $(BPF_IN) | \
> awk '/GLOBAL/ && /DEFAULT/ && !/UND/ {s++} END{print s}')
> -VERSIONED_SYM_COUNT = $(shell readelf -s $(OUTPUT)libbpf.so | \
> +VERSIONED_SYM_COUNT = $(shell readelf -s --wide $(OUTPUT)libbpf.so | \
> grep -Eo '[^ ]+@LIBBPF_' | cut -d@ -f1 | sort -u | wc -l)
>
> CMD_TARGETS = $(LIB_FILE)
> --
> 2.19.2
>
Looks good.
Acked-by: Andrii Nakryiko <andriin@fb.com>
^ permalink raw reply
* 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
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