* Re: [PATCH 0/2] doc: net: ieee802154: move from plain text to rst
From: Stefan Schmidt @ 2019-02-27 21:20 UTC (permalink / raw)
To: Jonathan Corbet; +Cc: davem, netdev, linux-doc, linux-wpan
In-Reply-To: <20190227131856.2a9cfdde@lwn.net>
Hello Jon.
On 27.02.19 21:18, Jonathan Corbet wrote:
> On Wed, 27 Feb 2019 20:59:12 +0100
> Stefan Schmidt <stefan@datenfreihafen.org> wrote:
>
>> The patches are based on net-next, but they only touch the networking book so I
>> would not expect and trouble. From what I have seen they would go through
>> Jonathan's tree after being acked by Dave? If you want this patches against a
>> different tree let me know.
>
> Usually Dave takes networking documentation patches directly, so that is
> what I would expect here.
OK, so I got that wrong. Works for me. Dave, you want to take them
directly, if nothing else comes up during review, or should I apply them
to my tree and send them with the next pull request?
> I took a quick look anyway; seems generally good. The main comment I
> would make is that much of what's there would be better placed as
> kerneldoc comments in the code itself that can then be pulled into the
> formatted docs. But that can be a job for another day...
Interesting point. That might be indeed a good idea on some parts of
this sparse doc. Need to check how this is handled in other docs. It
also needs extending scope and context, but again something for a follow
up patchset.
regards
Stefan Schmidt
^ permalink raw reply
* [PATCH bpf-next 0/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Yonghong Song @ 2019-02-27 21:22 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
The inner_map_meta->spin_lock_off is not set correctly during
map creation for BPF_MAP_TYPE_ARRAY_OF_MAPS and BPF_MAP_TYPE_HASH_OF_MAPS.
This may lead verifier error due to misinformation.
This patch set fixed the issue with Patch #1 for the kernel change
and Patch #2 for enhanced selftest test_maps.
Yonghong Song (2):
bpf: set inner_map_meta->spin_lock_off correctly
tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
kernel/bpf/map_in_map.c | 1 +
tools/testing/selftests/bpf/progs/test_map_in_map.c | 4 ++++
2 files changed, 5 insertions(+)
--
2.17.1
^ permalink raw reply
* [PATCH bpf-next 1/2] bpf: set inner_map_meta->spin_lock_off correctly
From: Yonghong Song @ 2019-02-27 21:22 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
In-Reply-To: <20190227212256.3856416-1-yhs@fb.com>
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;
/* Misc members not needed in bpf_map_meta_equal() check. */
inner_map_meta->ops = inner_map->ops;
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 2/2] tools/bpf: selftests: add map lookup to test_map_in_map bpf prog
From: Yonghong Song @ 2019-02-27 21:22 UTC (permalink / raw)
To: netdev; +Cc: Alexei Starovoitov, Daniel Borkmann, kernel-team, Yonghong Song
In-Reply-To: <20190227212256.3856416-1-yhs@fb.com>
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
^ permalink raw reply related
* phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Heiner Kallweit @ 2019-02-27 21:25 UTC (permalink / raw)
To: Russell King - ARM Linux, Andrew Lunn, Florian Fainelli
Cc: netdev@vger.kernel.org
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?
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.
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?
We're reading the port status, but IMO we want to set it based on the
auto-negotiated settings we get from the PHY.
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?
Heiner
^ permalink raw reply
* Re: [PATCH bpf-next] selftests/bpf: use __bpf_constant_htons in test_prog.c for flow dissector
From: Song Liu @ 2019-02-27 21:27 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S . Miller, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <20190227191511.80859-1-sdf@google.com>
On Wed, Feb 27, 2019 at 11:16 AM Stanislav Fomichev <sdf@google.com> wrote:
>
> Older GCC (<4.8) isn't smart enough to optimize !__builtin_constant_p()
> branch in bpf_htons.
>
> I recently fixed it for pkt_v4 and pkt_v6 in commit a0517a0f7ef23
> ("selftests/bpf: use __bpf_constant_htons in test_prog.c"), but later
> added another bunch of bpf_htons in commit bf0f0fd939451 ("selftests/bpf:
> add simple BPF_PROG_TEST_RUN examples for flow dissector").
>
> Fixes: bf0f0fd939451 ("selftests/bpf: add simple BPF_PROG_TEST_RUN examples for flow dissector")
>
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
Acked-by: Song Liu <songliubraving@fb.com>
> ---
> tools/testing/selftests/bpf/test_progs.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tools/testing/selftests/bpf/test_progs.c b/tools/testing/selftests/bpf/test_progs.c
> index c59d2e015d16..87cde42559f7 100644
> --- a/tools/testing/selftests/bpf/test_progs.c
> +++ b/tools/testing/selftests/bpf/test_progs.c
> @@ -1954,7 +1954,7 @@ static struct bpf_flow_keys pkt_v4_flow_keys = {
> .thoff = sizeof(struct iphdr),
> .addr_proto = ETH_P_IP,
> .ip_proto = IPPROTO_TCP,
> - .n_proto = bpf_htons(ETH_P_IP),
> + .n_proto = __bpf_constant_htons(ETH_P_IP),
> };
>
> static struct bpf_flow_keys pkt_v6_flow_keys = {
> @@ -1962,7 +1962,7 @@ static struct bpf_flow_keys pkt_v6_flow_keys = {
> .thoff = sizeof(struct ipv6hdr),
> .addr_proto = ETH_P_IPV6,
> .ip_proto = IPPROTO_TCP,
> - .n_proto = bpf_htons(ETH_P_IPV6),
> + .n_proto = __bpf_constant_htons(ETH_P_IPV6),
> };
>
> static void test_flow_dissector(void)
> --
> 2.21.0.rc2.261.ga7da99ff1b-goog
>
^ permalink raw reply
* Re: Request for suggestion on net device re-naming/re-ordering based on DT alias
From: Stephen Hemminger @ 2019-02-27 21:42 UTC (permalink / raw)
To: Florian Fainelli
Cc: Harini Katakam, netdev, linux-kernel, Nicolas Ferre, David Miller,
Michal Simek, Harini Katakam, Harini Katakam
In-Reply-To: <9a07d721-7ea8-03a0-d10b-5fdf2bfd9217@gmail.com>
On Wed, 27 Feb 2019 10:45:44 -0800
Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 2/27/19 10:40 AM, Stephen Hemminger wrote:
> > On Wed, 27 Feb 2019 17:24:03 +0530
> > Harini Katakam <harinik@xilinx.com> wrote:
> >
> >> Hi,
> >>
> >> We've had some users requesting control over net device name order
> >> when multiple ethernet devices are present on a system. I've tried a
> >> few solutions to this and looked it up on forums. But I apologize if
> >> I have missed something.
> >>
> >> I know that the current system allocates eth<n> as per probe order
> >> but that is obviously not stably controlled by user (tried DT
> >> re-ordering and defer probe). One solution is to use DT alias names
> >> to write to (net_device)->name as follows:
> >> Devicetree:
> >> aliases {
> >> ethernet0 = &mac1;
> >> ethernet1 = &mac0;
> >> };
> >> Driver probe:
> >> + /* Read ethernet DT alias id and assign to right device name*/
> >> + id = of_alias_get_id(np, "ethernet");
> >> + if (id < 0) {
> >> + dev_warn(&pdev->dev, "failed to get alias id (%u)\n", id);
> >> + return id;
> >> + }
> >> + snprintf(dev->name, sizeof(dev->name), "eth%d", id);
> >> +
> >>
> >> These three drivers seem to have something similar for mdio/phy bus IDs:
> >> drivers/net/ethernet/broadcom/genet/bcmmii.c:409: id =
> >> of_alias_get_id(dn, "eth");
> >> drivers/net/ethernet/samsung/sxgbe/sxgbe_platform.c:43: plat->bus_id =
> >> of_alias_get_id(np, "ethernet");
> >> drivers/net/ethernet/stmicro/stmmac/stmmac_platform.c:404:
> >> plat->bus_id = of_alias_get_id(np, "ethernet");
> >>
> >> Drawback: This approach will break if alias is not provided for one
> >> of the interfaces on board. Not to mention, there could be systems
> >> with multiple ethernet makes (for ex. Cadence macb and Xilinx axienet)
> >> If one of the drivers does not have an alias read mechanism, it is
> >> possible to have clashing ID assignments. Is there any way this
> >> solution can be changed to be stable/acceptable?
> >>
> >> One other alternative I've tried is netdev kernel bootargs but this
> >> device name was not being picked by the kernel:
> >> https://www.kernel.org/doc/html/v4.19/admin-guide/kernel-parameters.html
> >> netdev= <mac1’s interrupt id>, <mac1’s base address>, eth0
> >> netdev=<mac0’s interrupt id>, <mac0’s base address>, eth1
> >>
> >> Could you please suggest any alternatives?
> >> Thanks!
> >>
> >> Regards,
> >> Harini
> >
> > Device naming is a hard problem, and there is no perfect solution.
> >
> > Device tree should be providing hints to userspace policy for naming, not
> > trying to do it in the kernel.
>
> And Device Tree does already, if you look at the uevent attributes that
> the kernel sends, there should be ample information to uniquely
> determine which physical network device the network device maps to, e
> for instance:
>
> cat /sys/class/net/eth*/device/uevent
> DRIVER=brcm-systemport
> OF_NAME=ethernet
> OF_FULLNAME=/rdb/ethernet@9300000
> OF_TYPE=network
> OF_COMPATIBLE_0=brcm,systemportlite-v1.00
> OF_COMPATIBLE_1=brcm,systemport
> OF_COMPATIBLE_N=2
> OF_ALIAS_0=eth0
> MODALIAS=of:NethernetTnetworkCbrcm,systemportlite-v1.00Cbrcm,systemport
Then you need to work with udev developers to handle device tree better.
By design ethN is not used for persistent naming, only other names like ensX.
This allows for safer renaming and provides way to handle devices that may
not match any rules.
Most of udev naming is based of properties in sysfs like pci-slot, port etc.
If you had that available on these devices, it would just work now.
^ permalink raw reply
* Re: [PATCH 1/2] doc: net: ieee802154: introduce IEEE 802.15.4 subsystem doc in rst style
From: Randy Dunlap @ 2019-02-27 21:51 UTC (permalink / raw)
To: Stefan Schmidt, davem, corbet; +Cc: netdev, linux-doc, linux-wpan
In-Reply-To: <20190227195914.4594-2-stefan@datenfreihafen.org>
On 2/27/19 11:59 AM, Stefan Schmidt wrote:
> Moving the ieee802154 docs from a plain text file into the new rst
> style. This commit only does the minimal needed change to bring the
> documentation over. Follow up patches will improve and extend on this.
>
> Signed-off-by: Stefan Schmidt <stefan@datenfreihafen.org>
> ---
> Documentation/networking/ieee802154.rst | 180 ++++++++++++++++++++++++
> Documentation/networking/index.rst | 1 +
> 2 files changed, 181 insertions(+)
> create mode 100644 Documentation/networking/ieee802154.rst
Tested-by: Randy Dunlap <rdunlap@infradead.org>
Thanks.
--
~Randy
^ 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-27 21:57 UTC (permalink / raw)
To: si-wei liu
Cc: Michael S. Tsirkin, 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: <d1060c75-eaba-ab6f-ff31-38cb3a47c711@oracle.com>
On Tue, 26 Feb 2019 16:17:21 -0800
si-wei liu <si-wei.liu@oracle.com> 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. I don't like the idea to delay exposing failover
> master until VF is hot plugged in (probably subject to various failures)
> later.
What netvsc does now is wait 2 seconds (to allow udev to do rename)
before bringing the VF link up. This works, has had no problems even
with slow distributions and is widely used.
A patch to allow ending the timeout after rename was proposed but
rejected.
https://lore.kernel.org/netdev/20171220223323.21125-1-sthemmin@microsoft.com/
Allow network devices to change name when up is too risky. There are things
like netfilter rules and other state in and out of the kernel that may break.
Userspace does not like it when the rules change.
^ permalink raw reply
* Re: [virtio-dev] Re: net_failover slave udev renaming (was Re: [RFC PATCH net-next v6 4/4] netvsc: refactor notifier/event handling code to use the bypass framework)
From: si-wei liu @ 2019-02-27 22:30 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Michael S. Tsirkin, 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: <20190227135732.04cbced3@shemminger-XPS-13-9360>
On 2/27/2019 1:57 PM, Stephen Hemminger wrote:
> On Tue, 26 Feb 2019 16:17:21 -0800
> si-wei liu <si-wei.liu@oracle.com> 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. I don't like the idea to delay exposing failover
>> master until VF is hot plugged in (probably subject to various failures)
>> later.
>
> What netvsc does now is wait 2 seconds (to allow udev to do rename)
> before bringing the VF link up. This works, has had no problems even
> with slow distributions and is widely used.
>
> A patch to allow ending the timeout after rename was proposed but
> rejected.
>
> https://lore.kernel.org/netdev/20171220223323.21125-1-sthemmin@microsoft.com/
>
> Allow network devices to change name when up is too risky.
I understand the concern in general, the thread above referenced this patch:
https://patchwork.ozlabs.org/patch/799646/
That was in the context of netvsc without a proper framework (net_failover).
What I was saying is that we should consider opening up the rename
restriction for IFF_FAILOVER_SLAVE. It looks to me that all the
userspace usage are trying to ignore the slave instead of operating it
directly. The netfilter rules and what mentioned below can/should be
applied to on top of the master if I'm not mistaken. The current
userspace doesn't speak the net_failover way, and it is already broken
since its introduction. If anything, those userspace can be fixed up to
listen for rename events to track name changes. Whatever those cases are
it should not affect current use cases.
Thanks,
-Siwei
> There are things
> like netfilter rules and other state in and out of the kernel that may break.
> Userspace does not like it when the rules change.
^ 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 22:38 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: <d1060c75-eaba-ab6f-ff31-38cb3a47c711@oracle.com>
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.
> 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 net-next 2/8] devlink: add PF and VF port flavours
From: Jakub Kicinski @ 2019-02-27 22:42 UTC (permalink / raw)
To: Jiri Pirko; +Cc: davem, oss-drivers, netdev
In-Reply-To: <20190227201727.GA2042@nanopsycho>
On Wed, 27 Feb 2019 21:17:27 +0100, Jiri Pirko wrote:
> Wed, Feb 27, 2019 at 06:23:26PM CET, jakub.kicinski@netronome.com wrote:
> >On Wed, 27 Feb 2019 13:41:35 +0100, Jiri Pirko wrote:
> >> Wed, Feb 27, 2019 at 01:23:27PM CET, jiri@resnulli.us wrote:
> >> >Tue, Feb 26, 2019 at 07:24:30PM CET, jakub.kicinski@netronome.com wrote:
> >> >>Current port flavours cover simple switches and DSA. Add PF
> >> >>and VF flavours to cover "switchdev" SR-IOV NICs.
> >> >>
> >> >>Example devlink user space output:
> >> >>
> >> >>$ devlink port
> >> >>pci/0000:82:00.0/0: type eth netdev p4p1 flavour physical
> >> >>pci/0000:82:00.0/10000: type eth netdev eth0 flavour pcie_pf pf 0
> >> >>pci/0000:82:00.0/10001: type eth netdev eth1 flavour pcie_vf pf 0 vf 0
> >> >>pci/0000:82:00.0/10002: type eth netdev eth2 flavour pcie_vf pf 0 vf 1
> >> >
> >> >Wait a second, howcome pf and vfs have the same PCI address?
> >>
> >> Oh, I think you have these as eswitch port representors. Confusing...
> >
> >FWIW I don't like the word representor, its a port. We don't call
> >physical ports "representors" even though from ASIC's point of view
> >they are exactly the same.
>
> My point is, they are not PFs and VFs. We have to find a way to clearly
> see what's what.
Okay, so let me explain the way I see it, and you can explain your way
or tell me where you disagree. Those devlink ports and netdevs are pf
ports and vf ports, which most refer to as "representor". If one sends
packets to the netdev indicated in DEVLINK_ATTR_PORT_NETDEV_*
attributes they will _egress_ the switch from that port. For physical
port that means going onto the Ethernet or IB wire. For PCIe it means
getting DMAed over the PCIe link to host memory.
There is a netdev construct on the host which is in charge of that
host memory. Maybe we shall call that host netdev?
(I said I don't like "representor" for the reason that people don't
refer to the physical port as "representor" even though it has exactly
the semantics we are following. This distinction between behaviour of
physical and PCI ports is what leads to confusion, I think.)
Let me bring out the moose :)
HOST A || HOST B
||
PF A | V | V | V | V || PF B | V | V | V
|*F |*F |*F |*F ... || |*F |*F |*F ...
*port A0 |*port A1 | 0 | 1 | 2 | 3 ||*port B0 |*port B1 | 0 | 1 | 2
||
PCI Express link || PCI Express link
\ \ \ | | | | | / / /
\ \ \ | | | | | / / /
/\ \______\______\'___|___|__________|_______'____/___/___/__ /\
|| |+PF0s0|+PF0s1 |+VF0|+VF1| ...| |+PF1s0|+PF1s1|+VF0|+VF1| ||
i || |------ ------ ----- ---- ----|--- ------ ------ ---- ----| || i
d n H || | <<========== | || d n H
e s O || | ==========>> | || e s O
v t S || | SR-IOV e-switch | || v t S
l a T || | <<========== | || l a T
i n || | ==========>> | || i n
n c A || | ________ _________ ________ | || n c B
k e || | |+Phys 0 |+Phys 1 |+Phys 2 | | || k e
|| \---------------------------------------------------------/ ||
\/ | | | \/
| | |
|| ||
MAC 0 || MAC 1 || MAC 2
|| ||
Things marked with + are devlink ports and have port (-repr-) netdevs
(including physical ports).
Things marked with * are host netdevs, don't have devlink ports.
^ permalink raw reply
* Re: [PATCH bpf-next 2/4] libbpf: Support 32-bit static data loads
From: Y Song @ 2019-02-27 22:42 UTC (permalink / raw)
To: Joe Stringer; +Cc: bpf, netdev, Daniel Borkmann, Alexei Starovoitov
In-Reply-To: <CAH3MdRWrwMgjd7UT+7w=xCmEBD+mxv-YBqW+Q8zL8t5WvGTn8A@mail.gmail.com>
FYI.The latest llvm trunk will not emit errors for static variables. The patch
also gives detailed information how to relate a particular static
variable to a particular
relocation.
https://reviews.llvm.org/rL354954
Thanks,
Yonghong
On Fri, Feb 15, 2019 at 12:18 PM Y Song <ys114321@gmail.com> wrote:
>
> On Thu, Feb 14, 2019 at 11:16 PM Joe Stringer <joe@wand.net.nz> wrote:
> >
> > On Thu, 14 Feb 2019 at 21:39, Y Song <ys114321@gmail.com> wrote:
> > >
> > > On Mon, Feb 11, 2019 at 4:48 PM Joe Stringer <joe@wand.net.nz> wrote:
> > > >
> > > > Support loads of static 32-bit data when BPF writers make use of
> > > > convenience macros for accessing static global data variables. A later
> > > > patch in this series will demonstrate its usage in a selftest.
> > > >
> > > > As of LLVM-7, this technique only works with 32-bit data, as LLVM will
> > > > complain if this technique is attempted with data of other sizes:
> > > >
> > > > LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> > > > or check your static variable usage
> > >
> > > A little bit clarification from compiler side.
> > > The above compiler error is to prevent people use static variables since current
> > > kernel/libbpf does not handle this. The compiler only warns if .bss or
> > > .data section
> > > has more than one definitions. The first definition always has section offset 0
> > > and the compiler did not warn.
> >
> > Ah, interesting. I observed that warning when I attempted to define
> > global variables of multiple sizes, and I thought also with sizes
> > other than 32-bit. This clarifies things a bit, thanks.
> >
> > For the .bss my observation was that if you had a definition like:
> >
> > static int a = 0;
> >
> > Then this will be placed into .bss, hence why I looked into the
> > approach from this patch for patch 3 as well.
> >
> > > The restriction is a little strange. To only work with 32-bit data is
> > > not a right
> > > statement. The following are some examples.
> > >
> > > The following static variable definitions will succeed:
> > > static int a; /* one in .bss */
> > > static long b = 2; /* one in .data */
> > >
> > > The following definitions will fail as both in .bss.
> > > static int a;
> > > static int b;
> > >
> > > The following definitions will fail as both in .data:
> > > static char a = 2;
> > > static int b = 3;
> >
> > Are there type restrictions or something? I've been defining multiple
>
> There is no type restrictions.
> -bash-4.4$ cat g.c
> struct t {
> int a;
> char b;
> long c;
> };
> static volatile struct t v;
> int test() { return v.a + v.b; }
> -bash-4.4$ clang -O2 -g -c -target bpf g.c
> -bash-4.4$
>
> > static uint32_t and using them per the approach in this patch series
> > without hitting this compiler assertion.
>
> -bash-4.4$ cat g1.c
> static volatile int a;
> static volatile int b;
> int test() { return a + b; }
> -bash-4.4$ clang -O2 -g -c -target bpf g1.c
> fatal error: error in backend: Unsupported relocation: try to compile
> with -O2 or above, or check your static variable
> usage
> -bash-4.4$
>
> >
> > > Using global variables can prevent compiler errors.
> > > maps are defined as globals and the compiler does not
> > > check whether a particular global variable is defining a map or not.
> > >
> > > If you just use static variable like below
> > > static int a = 2;
> > > without potential assignment to a, the compiler will replace variable
> > > a with 2 at compile time.
> > > An alternative is to define like below
> > > static volatile int a = 2;
> > > You can get a "load" for variable "a" in the bpf load even if there is
> > > no assignment to a.
> >
> > I'll take a closer look at this too.
> >
> > > Maybe now is the time to remove the compiler assertions as
> > > libbpf/kernel starts to
> > > handle static variables?
> >
> > I don't understand why those assertions exists in this form. It
> > already allows code which will not load with libbpf (ie generate any
> > .data/.bss), does it help prevent unexpected situations for
> > developers?
>
> The error is introduced by the following llvm commit:
> commit 39184e407cd937f2f20d3f61eec205925bae7b13
> Author: Yonghong Song <yhs@fb.com>
> Date: Wed Aug 22 21:21:03 2018 +0000
>
> bpf: fix an assertion in BPFAsmBackend applyFixup()
>
> Fix bug https://bugs.llvm.org/show_bug.cgi?id=38643
>
> In BPFAsmBackend applyFixup(), there is an assertion for FixedValue to be 0.
> This may not be true, esp. for optimiation level 0.
> For example, in the above bug, for the following two
> static variables:
> @bpf_map_lookup_elem = internal global i8* (i8*, i8*)*
> inttoptr (i64 1 to i8* (i8*, i8*)*), align 8
> @bpf_map_update_elem = internal global i32 (i8*, i8*, i8*, i64)*
> inttoptr (i64 2 to i32 (i8*, i8*, i8*, i64)*), align 8
>
> The static variable @bpf_map_update_elem will have a symbol
> offset of 8 and a FK_SecRel_8 with FixupValue 8 will cause
> the assertion if llvm is built with -DLLVM_ENABLE_ASSERTIONS=ON.
>
> The above relocations will not exist if the program is compiled
> with optimization level -O1 and above as the compiler optimizes
> those static variables away. In the below error message, -O2
> is suggested as this is the common practice.
>
> Note that FixedValue = 0 in applyFixup() does exist and is valid,
> e.g., for the global variable my_map in the above bug. The bpf
> loader will process them properly for map_id's before loading
> the program into the kernel.
>
> The static variables, which are not optimized away by compiler,
> may have FK_SecRel_8 relocation with non-zero FixedValue.
>
> The patch removed the offending assertion and will issue
> a hard error as below if the FixedValue in applyFixup()
> is not 0.
> $ llc -march=bpf -filetype=obj fixup.ll
> LLVM ERROR: Unsupported relocation: try to compile with -O2 or above,
> or check your static variable usage
>
> Its main purpose is to fix a behavior difference with and without
> -DLLVM_ENABLE_ASSERTIONS=ON. The patch generated an error
> regardless whether the compiler time assertion is turned on or not.
>
> It does not catch all the cases e.g., only one static variable is defined,
> which needs fine tuning as there are legitimate cases (e.g., in some dwarf
> sessions) where the Fixup is valid with FixedValue = 0.
>
> If you try to use more than onee static variables, the compiler will
> print an error and let you know your potential issues.
>
> The question is since we are on the path to allow static variables
> in the bpf programs for later patching, we probably should remove
> this compiler fatal error?
^ permalink raw reply
* [PATCH bpf-next 2/5] libbpf: fix formatting for btf_ext__get_raw_data
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
Fix invalid formatting of pointer arg.
Fixes: ae4ab4b4117d ("btf: expose API to work with raw btf_ext data")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index 94bbc249b0f1..b60bb7cf5fff 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -76,7 +76,7 @@ LIBBPF_API int btf__get_map_kv_tids(const struct btf *btf, const char *map_name,
LIBBPF_API struct btf_ext *btf_ext__new(__u8 *data, __u32 size);
LIBBPF_API void btf_ext__free(struct btf_ext *btf_ext);
-LIBBPF_API const void *btf_ext__get_raw_data(const struct btf_ext* btf_ext,
+LIBBPF_API const void *btf_ext__get_raw_data(const struct btf_ext *btf_ext,
__u32 *size);
LIBBPF_API int btf_ext__reloc_func_info(const struct btf *btf,
const struct btf_ext *btf_ext,
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 3/5] btf: allow to customize dedup hash table size
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
Default size of dedup table (16k) is good enough for most binaries, even
typical vmlinux images. But there are cases of binaries with huge amount
of BTF types (e.g., allyesconfig variants of kernel), which benefit from
having bigger dedup table size to lower amount of unnecessary hash
collisions. Tools like pahole, thus, can tune this parameter to reach
optimal performance.
This change also serves double purpose of allowing tests to force hash
collisions to test some corner cases, used in follow up patch.
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf.c | 43 ++++++++++++++++++++++++++-----------------
tools/lib/bpf/btf.h | 1 +
2 files changed, 27 insertions(+), 17 deletions(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 68b50e9bbde1..6bbb710216e6 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -1070,8 +1070,7 @@ int btf__dedup(struct btf *btf, struct btf_ext *btf_ext,
return err;
}
-#define BTF_DEDUP_TABLE_SIZE_LOG 14
-#define BTF_DEDUP_TABLE_MOD ((1 << BTF_DEDUP_TABLE_SIZE_LOG) - 1)
+#define BTF_DEDUP_TABLE_DEFAULT_SIZE (1 << 14)
#define BTF_UNPROCESSED_ID ((__u32)-1)
#define BTF_IN_PROGRESS_ID ((__u32)-2)
@@ -1128,18 +1127,21 @@ static inline __u32 hash_combine(__u32 h, __u32 value)
#undef GOLDEN_RATIO_PRIME
}
-#define for_each_hash_node(table, hash, node) \
- for (node = table[hash & BTF_DEDUP_TABLE_MOD]; node; node = node->next)
+#define for_each_dedup_cand(d, hash, node) \
+ for (node = d->dedup_table[hash & (d->opts.dedup_table_size - 1)]; \
+ node; \
+ node = node->next)
static int btf_dedup_table_add(struct btf_dedup *d, __u32 hash, __u32 type_id)
{
struct btf_dedup_node *node = malloc(sizeof(struct btf_dedup_node));
+ int bucket = hash & (d->opts.dedup_table_size - 1);
if (!node)
return -ENOMEM;
node->type_id = type_id;
- node->next = d->dedup_table[hash & BTF_DEDUP_TABLE_MOD];
- d->dedup_table[hash & BTF_DEDUP_TABLE_MOD] = node;
+ node->next = d->dedup_table[bucket];
+ d->dedup_table[bucket] = node;
return 0;
}
@@ -1177,7 +1179,7 @@ static void btf_dedup_table_free(struct btf_dedup *d)
if (!d->dedup_table)
return;
- for (i = 0; i < (1 << BTF_DEDUP_TABLE_SIZE_LOG); i++) {
+ for (i = 0; i < d->opts.dedup_table_size; i++) {
while (d->dedup_table[i]) {
tmp = d->dedup_table[i];
d->dedup_table[i] = tmp->next;
@@ -1221,10 +1223,19 @@ static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
if (!d)
return ERR_PTR(-ENOMEM);
+ d->opts.dont_resolve_fwds = opts && opts->dont_resolve_fwds;
+ /* ensure table size is power of two and limit to 2G */
+ d->opts.dedup_table_size = opts && opts->dedup_table_size
+ ? opts->dedup_table_size
+ : BTF_DEDUP_TABLE_DEFAULT_SIZE;
+ for (i = 0; i < 31 && (1 << i) < d->opts.dedup_table_size; i++)
+ ;
+ d->opts.dedup_table_size = 1 << i;
+
d->btf = btf;
d->btf_ext = btf_ext;
- d->dedup_table = calloc(1 << BTF_DEDUP_TABLE_SIZE_LOG,
+ d->dedup_table = calloc(d->opts.dedup_table_size,
sizeof(struct btf_dedup_node *));
if (!d->dedup_table) {
err = -ENOMEM;
@@ -1249,8 +1260,6 @@ static struct btf_dedup *btf_dedup_new(struct btf *btf, struct btf_ext *btf_ext,
for (i = 0; i <= btf->nr_types; i++)
d->hypot_map[i] = BTF_UNPROCESSED_ID;
- d->opts.dont_resolve_fwds = opts && opts->dont_resolve_fwds;
-
done:
if (err) {
btf_dedup_free(d);
@@ -1824,7 +1833,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_INT:
h = btf_hash_int(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_int(t, cand)) {
new_id = cand_node->type_id;
@@ -1835,7 +1844,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_ENUM:
h = btf_hash_enum(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_enum(t, cand)) {
new_id = cand_node->type_id;
@@ -1846,7 +1855,7 @@ static int btf_dedup_prim_type(struct btf_dedup *d, __u32 type_id)
case BTF_KIND_FWD:
h = btf_hash_common(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_common(t, cand)) {
new_id = cand_node->type_id;
@@ -2263,7 +2272,7 @@ static int btf_dedup_struct_type(struct btf_dedup *d, __u32 type_id)
return 0;
h = btf_hash_struct(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
int eq;
btf_dedup_clear_hypot_map(d);
@@ -2349,7 +2358,7 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
t->type = ref_type_id;
h = btf_hash_common(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_common(t, cand)) {
new_id = cand_node->type_id;
@@ -2372,7 +2381,7 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
info->index_type = ref_type_id;
h = btf_hash_array(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_array(t, cand)) {
new_id = cand_node->type_id;
@@ -2403,7 +2412,7 @@ static int btf_dedup_ref_type(struct btf_dedup *d, __u32 type_id)
}
h = btf_hash_fnproto(t);
- for_each_hash_node(d->dedup_table, h, cand_node) {
+ for_each_dedup_cand(d, h, cand_node) {
cand = d->btf->types[cand_node->type_id];
if (btf_equal_fnproto(t, cand)) {
new_id = cand_node->type_id;
diff --git a/tools/lib/bpf/btf.h b/tools/lib/bpf/btf.h
index b60bb7cf5fff..28a1e1e59861 100644
--- a/tools/lib/bpf/btf.h
+++ b/tools/lib/bpf/btf.h
@@ -90,6 +90,7 @@ LIBBPF_API __u32 btf_ext__func_info_rec_size(const struct btf_ext *btf_ext);
LIBBPF_API __u32 btf_ext__line_info_rec_size(const struct btf_ext *btf_ext);
struct btf_dedup_opts {
+ unsigned int dedup_table_size;
bool dont_resolve_fwds;
};
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 0/5] btf_dedup algorithm and test fixes
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
This patchset fixes a bug in btf_dedup() algorithm, which under specific hash
collision causes infinite loop. It also exposes ability to tune BTF
deduplication table size, with double purpose of allowing applications to
adjust size according to the size of BTF data, as well as allowing a simple way
to force hash collisions by setting table size to 1.
- Patch #1 fixes bug in btf_dedup testing code that's checking strings
- Patch #2 fixes pointer arg formatting in btf.h
- Patch #3 adds option to specify custom dedup table size
- Patch #4 fixes aforementioned bug in btf_dedup
- Patch #5 adds test that validates the fix
Andrii Nakryiko (5):
selftests/bpf: fix btf_dedup testing code
libbpf: fix formatting for btf_ext__get_raw_data
btf: allow to customize dedup hash table size
btf: fix bug with resolving STRUCT/UNION into corresponding FWD
selftests/bpf: add btf_dedup test of FWD/STRUCT resolution
tools/lib/bpf/btf.c | 49 ++++++++++++++++----------
tools/lib/bpf/btf.h | 3 +-
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/test_btf.c | 49 ++++++++++++++++++++++++--
4 files changed, 81 insertions(+), 21 deletions(-)
--
2.17.1
^ permalink raw reply
* [PATCH bpf-next 1/5] selftests/bpf: fix btf_dedup testing code
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
btf_dedup testing code doesn't account for length of struct btf_header
when calculating the start of a string section. This patch fixes this
problem.
Fixes: 49b57e0d01db ("tools/bpf: remove btf__get_strings() superseded by raw data API")
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/.gitignore | 1 +
tools/testing/selftests/bpf/test_btf.c | 4 ++--
2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/tools/testing/selftests/bpf/.gitignore b/tools/testing/selftests/bpf/.gitignore
index e47168d1257d..3b74d23fffab 100644
--- a/tools/testing/selftests/bpf/.gitignore
+++ b/tools/testing/selftests/bpf/.gitignore
@@ -14,6 +14,7 @@ feature
test_libbpf_open
test_sock
test_sock_addr
+test_sock_fields
urandom_read
test_btf
test_sockmap
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 02d314383a9c..1426c0a905c8 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -5936,9 +5936,9 @@ static int do_test_dedup(unsigned int test_num)
}
test_hdr = test_btf_data;
- test_strs = test_btf_data + test_hdr->str_off;
+ test_strs = test_btf_data + sizeof(*test_hdr) + test_hdr->str_off;
expect_hdr = expect_btf_data;
- expect_strs = expect_btf_data + expect_hdr->str_off;
+ expect_strs = expect_btf_data + sizeof(*test_hdr) + expect_hdr->str_off;
if (CHECK(test_hdr->str_len != expect_hdr->str_len,
"test_hdr->str_len:%u != expect_hdr->str_len:%u",
test_hdr->str_len, expect_hdr->str_len)) {
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 4/5] btf: fix bug with resolving STRUCT/UNION into corresponding FWD
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
When checking available canonical candidates for struct/union algorithm
utilizes btf_dedup_is_equiv to determine if candidate is suitable. This
check is not enough when candidate is corresponding FWD for that
struct/union, because according to equivalence logic they are
equivalent. When it so happens that FWD and STRUCT/UNION end in hashing
to the same bucket, it's possible to create remapping loop from FWD to
STRUCT and STRUCT to same FWD, which will cause btf_dedup() to loop
forever.
This patch fixes the issue by additionally checking that type and
canonical candidate are strictly equal (utilizing btf_equal_struct).
Fixes: d5caef5b5655 ("btf: add BTF types deduplication algorithm")
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/lib/bpf/btf.c | 6 +++++-
1 file changed, 5 insertions(+), 1 deletion(-)
diff --git a/tools/lib/bpf/btf.c b/tools/lib/bpf/btf.c
index 6bbb710216e6..53db26d158c9 100644
--- a/tools/lib/bpf/btf.c
+++ b/tools/lib/bpf/btf.c
@@ -2255,7 +2255,7 @@ static void btf_dedup_merge_hypot_map(struct btf_dedup *d)
static int btf_dedup_struct_type(struct btf_dedup *d, __u32 type_id)
{
struct btf_dedup_node *cand_node;
- struct btf_type *t;
+ struct btf_type *cand_type, *t;
/* if we don't find equivalent type, then we are canonical */
__u32 new_id = type_id;
__u16 kind;
@@ -2275,6 +2275,10 @@ static int btf_dedup_struct_type(struct btf_dedup *d, __u32 type_id)
for_each_dedup_cand(d, h, cand_node) {
int eq;
+ cand_type = d->btf->types[cand_node->type_id];
+ if (!btf_equal_struct(t, cand_type))
+ continue;
+
btf_dedup_clear_hypot_map(d);
eq = btf_dedup_is_equiv(d, type_id, cand_node->type_id);
if (eq < 0)
--
2.17.1
^ permalink raw reply related
* [PATCH bpf-next 5/5] selftests/bpf: add btf_dedup test of FWD/STRUCT resolution
From: Andrii Nakryiko @ 2019-02-27 22:46 UTC (permalink / raw)
To: andrii.nakryiko, kernel-team, ast, acme, netdev, bpf, daniel
Cc: Andrii Nakryiko
In-Reply-To: <20190227224642.1069138-1-andriin@fb.com>
This patch adds a btf_dedup test exercising logic of STRUCT<->FWD
resolution and validating that STRUCT is not resolved to a FWD. It also
forces hash collisions, forcing both FWD and STRUCT to be candidates for
each other. Previously this condition caused infinite loop due to FWD
pointing to STRUCT and STRUCT pointing to its FWD.
Reported-by: Arnaldo Carvalho de Melo <acme@redhat.com>
Signed-off-by: Andrii Nakryiko <andriin@fb.com>
---
tools/testing/selftests/bpf/test_btf.c | 45 ++++++++++++++++++++++++++
1 file changed, 45 insertions(+)
diff --git a/tools/testing/selftests/bpf/test_btf.c b/tools/testing/selftests/bpf/test_btf.c
index 1426c0a905c8..38797aa627a7 100644
--- a/tools/testing/selftests/bpf/test_btf.c
+++ b/tools/testing/selftests/bpf/test_btf.c
@@ -5731,6 +5731,51 @@ const struct btf_dedup_test dedup_tests[] = {
.dont_resolve_fwds = false,
},
},
+{
+ .descr = "dedup: struct <-> fwd resolution w/ hash collision",
+ /*
+ * // CU 1:
+ * struct x;
+ * struct s {
+ * struct x *x;
+ * };
+ * // CU 2:
+ * struct x {};
+ * struct s {
+ * struct x *x;
+ * };
+ */
+ .input = {
+ .raw_types = {
+ /* CU 1 */
+ BTF_FWD_ENC(NAME_TBD, 0 /* struct fwd */), /* [1] fwd x */
+ BTF_PTR_ENC(1), /* [2] ptr -> [1] */
+ BTF_STRUCT_ENC(NAME_TBD, 1, 8), /* [3] struct s */
+ BTF_MEMBER_ENC(NAME_TBD, 2, 0),
+ /* CU 2 */
+ BTF_STRUCT_ENC(NAME_TBD, 0, 0), /* [4] struct x */
+ BTF_PTR_ENC(4), /* [5] ptr -> [4] */
+ BTF_STRUCT_ENC(NAME_TBD, 1, 8), /* [6] struct s */
+ BTF_MEMBER_ENC(NAME_TBD, 5, 0),
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0x\0s\0x\0x\0s\0x\0"),
+ },
+ .expect = {
+ .raw_types = {
+ BTF_PTR_ENC(3), /* [1] ptr -> [3] */
+ BTF_STRUCT_ENC(NAME_TBD, 1, 8), /* [2] struct s */
+ BTF_MEMBER_ENC(NAME_TBD, 1, 0),
+ BTF_STRUCT_ENC(NAME_NTH(2), 0, 0), /* [3] struct x */
+ BTF_END_RAW,
+ },
+ BTF_STR_SEC("\0s\0x"),
+ },
+ .opts = {
+ .dont_resolve_fwds = false,
+ .dedup_table_size = 1, /* force hash collisions */
+ },
+},
{
.descr = "dedup: all possible kinds (no duplicates)",
.input = {
--
2.17.1
^ permalink raw reply related
* Re: phylink / mv8e6xxx and SGMII aneg not working, link doesn't come up
From: Russell King - ARM Linux admin @ 2019-02-27 22:52 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <f507564b-7dc3-9449-ebb6-865eaeaa8a29@gmail.com>
On Wed, Feb 27, 2019 at 10:25:54PM +0100, Heiner Kallweit wrote:
> For the ones who don't know my story yet:
> I have a Marvell 88E6390 switch with a Clause 45 PHY connected via
> SGMII to port 9. For other reasons I limit the PHY to 100Mbps currently.
> DT says: managed = "in-band-status"
> Driver is mv8e6xxx + phylink.
> Problem is that the link doesn't come up. Debugging resulted in:
>
> PHY establishes link to link partner (100Mbps, FD) and fires the
> "link up" interrupt. The "link up" is also properly transmitted via
> SGMII in-band signalling and fires the SERDES interrupt in mv8e6xxx.
> Problem is that the in-band transmitted values for speed and duplex
> don't show up anywhere. And phylink_resolve() doesn't even print
> the link-up message.
>
> First question: Both, PHY and SERDES interrupt, try to do the same
> thing: they eventually call phylink_run_resolve(). Is this correct?
Correct - when the PHY interrupts, it triggers phylib to read the
status and issue a callback via the link status hook into phylink,
which stores the information from the PHY. That triggers a resolve.
Even though the PHY reports that it has established link, the link
may not yet be up.
The Serdes is the MAC side of the link, and that should be calling
phylink_mac_change(). That will re-run the resolve.
Each time the resolve is triggered in "in-band" mode, we expect to
read the link status from the MAC side of the link, along with the
speed and duplex. Since SGMII does not carry the flow control
parameters, if a PHY is present, we merge the PHYs flow control
information and pass that back to the MAC to configure the MAC's
flow control to match.
> I have some problems with understanding the code for MLO_AN_INBAND
> in phylink_resolve(). Maybe also something is missing there for
> proper in-band aneg support.
Nope, it works with mvneta and mvpp2.
> First we get the mac state (which is link down, 10Mbps, HD in my case).
> Then the link state is calculated as "mac link up" && "phy link up".
> This results in "link down", because mac link is down and phy link is
> up. This logic isn't clear to me. How is the "link up" info supposed
> to ever reach the mac?
Via the in-band status, and the MAC detecting that the link is now up.
> We're reading the port status, but IMO we want to set it based on the
> auto-negotiated settings we get from the PHY.
No we don't - in SGMII in-band mode, the PHY is optional, and even if
it is present, if the MAC reports that the link is down even if the PHY
reports that the link is up, then the link is _not_ up.
If you want to use SGMII without in-band, then phylink supports that
(which is PHY mode). If the MAC is unable to report these parameters,
then in-band mode is not appropriate.
> Later pause settings and possibly changed interface mode are
> propagated to the mac. But no word about speed and duplex.
>
> Via "some callback" "some code" should read the in-band-transmitted
> speed and duplex values from the SGMII port and use them to configure
> the mac. Is this simply missing or do I miss something?
From what you've described, it sounds like what you actually have is:
MAC <---> Serdes PHY <---> PHY
The Serdes PHY receives the SGMII in-band negotiation from the external
PHY, but there is no propagation of the status from the serdes PHY to
the MAC. What's more is the Serdes PHY is hidden in mv88e6xxx setups.
This is a classic stacked PHY setup, one which we're now starting to
encounter, and we need to support it - we have three cases I'm aware of
now where we have:
MAC <---> PHY <---> SFP
and "SFP" can be another PHY, so we end up with:
MAC <---> PHY <---> PHY
and phylib does not support having two PHYs on the same net_device.
What's more is that the need for an "attached_dev" net_device is pretty
heavily embedded into phylib, so attaching one PHY to another PHY
doesn't work very well.
I've been tinkering with some patches to abstract out the "attached_dev"
stuff, but in doing so I've walked into a whole load of issues in
phy_attach_direct() and phy_detach() - it looks like phy_attach_direct()
is lacking any form of locking while binding one of the generic drivers
(it needs to hold the device lock while doing so.) The comments around
device_release_driver() are vague about whether the parent device needs
to be locked. Questions have been asked about that but so far I haven't
had a response.
We can replace a load of phydev->attached_dev != NULL tests (which are
checking whether the device is attached to a netdev) fairly easily,
but there's a load of places which do (phydev->attached_dev &&
phydev->adjust_link) - and I fear that phylink (which doesn't set
phydev->adjust_link) results in this code being broken.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net-next] net: sched: don't release block->lock when dumping chains
From: Cong Wang @ 2019-02-27 23:03 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbf36oamsz3.fsf@mellanox.com>
On Tue, Feb 26, 2019 at 8:10 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Tue 26 Feb 2019 at 00:15, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Mon, Feb 25, 2019 at 7:45 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >> Function tc_dump_chain() obtains and releases block->lock on each iteration
> >> of its inner loop that dumps all chains on block. Outputting chain template
> >> info is fast operation so locking/unlocking mutex multiple times is an
> >> overhead when lock is highly contested. Modify tc_dump_chain() to only
> >> obtain block->lock once and dump all chains without releasing it.
> >>
> >> Signed-off-by: Vlad Buslov <vladbu@mellanox.com>
> >> Suggested-by: Cong Wang <xiyou.wangcong@gmail.com>
> >
> > Thanks for the followup!
> >
> > Isn't it similar for __tcf_get_next_proto() in tcf_chain_dump()?
> > And for tc_dump_tfilter()?
>
> Not really. These two dump all tp filters and not just a template, which
> is O(n) on number of filters and can be slow because it calls hw offload
> API for each of them. Our typical use-case involves periodic filter dump
> (to update stats) while multiple concurrent user-space threads are
> updating filters, so it is important for them to be able to execute in
> parallel.
Hmm, but if these are read-only, you probably don't even need a
mutex, you can just use RCU read lock to protect list iteration
and you still can grab the refcnt in the same way.
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Rafael J. Wysocki @ 2019-02-27 23:03 UTC (permalink / raw)
To: Brian Norris
Cc: Ard Biesheuvel, Marc Zyngier, Ganapathi Bhat, Jeffy Chen,
Heiko Stuebner, Devicetree List, Xinming Hu,
<netdev@vger.kernel.org>, linux-pm,
<linux-wireless@vger.kernel.org>, Linux Kernel Mailing List,
Amitkumar Karwar, linux-rockchip, Nishant Sarmukadam, Rob Herring,
Rafael J. Wysocki, linux-arm-kernel, Enric Balletbo i Serra,
Lorenzo Pieralisi, David S. Miller, Kalle Valo, Tony Lindgren,
Mark Rutland
In-Reply-To: <20190227205754.GF174696@google.com>
On Wed, Feb 27, 2019 at 9:58 PM Brian Norris <briannorris@chromium.org> wrote:
>
> Hi Ard,
>
> On Wed, Feb 27, 2019 at 11:16:12AM +0100, Ard Biesheuvel wrote:
> > On Wed, 27 Feb 2019 at 11:02, Marc Zyngier <marc.zyngier@arm.com> wrote:
> > > On 26/02/2019 23:28, Brian Norris wrote:
> > > > You're not the first person to notice this. All the motivations are not
> > > > necessarily painted clearly in their cover letter, but here are some
> > > > previous attempts at solving this problem:
> > > >
> > > > [RFC PATCH v11 0/5] PCI: rockchip: Move PCIe WAKE# handling into pci core
> > > > https://lkml.kernel.org/lkml/20171225114742.18920-1-jeffy.chen@rock-chips.com/
> > > > http://lkml.kernel.org/lkml/20171226023646.17722-1-jeffy.chen@rock-chips.com/
> > > >
> > > > As you can see by the 12th iteration, it wasn't left unsolved for lack
> > > > of trying...
> > >
> > > I wasn't aware of this. That's definitely a better approach than my
> > > hack, and I would really like this to be revived.
> > >
> >
> > I don't think this approach is entirely sound either.
>
> (I'm sure there may be problems with the above series. We probably
> should give it another shot though someday, as I think it's closer to
> the mark.)
>
> > From the side of the PCI device, WAKE# is just a GPIO line, and how it
> > is wired into the system is an entirely separate matter. So I don't
> > think it is justified to overload the notion of legacy interrupts with
> > some other pin that may behave in a way that is vaguely similar to how
> > a true wake-up capable interrupt works.
>
> I think you've conflated INTx with WAKE# just a bit (and to be fair,
> that's exactly what the bad binding we're trying to replace did,
> accidentally). We're not trying to claim this WAKE# signal replaces the
> typical PCI interrupts, but it *is* an interrupt in some sense --
> "depending on your definition of interrupt", per our IRC conversation ;)
>
> > So I'd argue that we should add an optional 'wake-gpio' DT property
> > instead to the generic PCI device binding, and leave the interrupt
> > binding and discovery alone.
>
> So I think Mark Rutland already shot that one down; it's conceptually an
> interrupt from the device's perspective.
Which device are you talking about? The one that signals wakeup? If
so, then I beg to differ.
On ACPI platforms WAKE# is represented as an ACPI GPE that is signaled
through SCI and handled at a different level (on HW-reduced ACPI it
actually can be a GPIO interrupt, but it still is handled with the
help of AML). The driver of the device signaling wakeup need not even
be aware that WAKE# has been asserted.
> We just need to figure out a good way of representing it that doesn't stomp on the existing INTx
> definitions.
WAKE# is a signal that is converted into an interrupt, but that
interrupt may arrive at some place your driver has nothing to do with.
It generally doesn't make sense to represent it as an interrupt for
the target device.
^ permalink raw reply
* Re: [PATCH net-next] net: sched: set dedicated tcf_walker flag when tp is empty
From: Cong Wang @ 2019-02-27 23:09 UTC (permalink / raw)
To: Vlad Buslov
Cc: Linux Kernel Network Developers, Jamal Hadi Salim, Jiri Pirko,
David Miller
In-Reply-To: <vbfy361i9uy.fsf@mellanox.com>
On Wed, Feb 27, 2019 at 6:28 AM Vlad Buslov <vladbu@mellanox.com> wrote:
>
>
> On Tue 26 Feb 2019 at 22:38, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> > On Tue, Feb 26, 2019 at 7:08 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >>
> >>
> >> On Mon 25 Feb 2019 at 22:52, Cong Wang <xiyou.wangcong@gmail.com> wrote:
> >> > On Mon, Feb 25, 2019 at 7:38 AM Vlad Buslov <vladbu@mellanox.com> wrote:
> >> >>
> >> >> Using tcf_walker->stop flag to determine when tcf_walker->fn() was called
> >> >> at least once is unreliable. Some classifiers set 'stop' flag on error
> >> >> before calling walker callback, other classifiers used to call it with NULL
> >> >> filter pointer when empty. In order to prevent further regressions, extend
> >> >> tcf_walker structure with dedicated 'nonempty' flag. Set this flag in
> >> >> tcf_walker->fn() implementation that is used to check if classifier has
> >> >> filters configured.
> >> >
> >> >
> >> > So, after this patch commits like 31a998487641 ("net: sched: fw: don't
> >> > set arg->stop in fw_walk() when empty") can be reverted??
> >>
> >> Yes, it is safe now to revert following commits:
> >>
> >> 3027ff41f67c ("net: sched: route: don't set arg->stop in route4_walk() when empty")
> >> 31a998487641 ("net: sched: fw: don't set arg->stop in fw_walk() when empty")
> >
> > Yeah, and probably commit d66022cd1623
> > ("net: sched: matchall: verify that filter is not NULL in mall_walk()").
> >
> > Please send a patch to revert them all.
> >
> > Thanks.
>
> I think commit d66022cd1623 ("net: sched: matchall: verify that filter
> is not NULL in mall_walk()") and commit 8b58d12f4ae1 ("net: sched:
> cgroup: verify that filter is not NULL during walk") shouldn't be
> reverted. They are still necessary to prevent tcf_chain_dump() from
> dumping NULL filter pointer. It can happen when dump is initiated in
> parallel with inserting first filter to unlocked classifier.
> tcf_fill_node() verifies that filter pointer is not NULL, so it will not
> crash, but will output tcf_proto info for second time. This might
> "confuse" user-space.
I don't get this.
First of all, what's confused here?
Secondly, if there is something confusing, isn't it all because of
your parallel algorithm? That is, the retry logic. I don't see how
commit d66022cd1623 could be useful in this context, it helps
to prevent a NULL crash which isn't a concern as long as it is
checked in tcf_fill_node() as you described.
Thanks.
^ permalink raw reply
* tc ebpf da failed to read the udp data when data length >= 215
From: IMBRIUS AGER @ 2019-02-27 23:12 UTC (permalink / raw)
To: netdev; +Cc: daniel, xiyou.wangcong, dcaratti, davem, jhs, jiri, vladbu
Hello:
I am having an odd issue.
I tried to read the udp data by tc ebpf da, however it worked only
when the data length <= 214.
As far as I know, data_end points to the end of the linear data,
that means the skb should be enough for more than 214 in general.
Any ideas about how to avoid the issue? Thanks!
# uname -a
Linux ip-10-0-0-2 4.15.0-1021-aws #21-Ubuntu SMP Tue Aug 28 10:23:07
UTC 2018 x86_64 x86_64 x86_64 GNU/Linux
# tc -V
tc utility, iproute2-ss180129
Here is the test code:
#############################################################
ebpf_debug("level 4: %p - %p\n", udph, data_end);
__u32 *data32 = (__u32 *)(udph + 1);;
ebpf_debug("level 7: %p - %p\n", data32, data_end);
if (data32 + 1 > data_end)
return;
ebpf_debug("%u\n", data32[0]);
############################################################
Here is the test:
1) 10.0.0.3-me # nmap -sU -p 10000 10.0.0.2 --data-length 214
level 4: 00000000c6983314 - 00000000b11428cf
level 7: 000000002a31fcec - 00000000b11428cf
4036810185
2) 10.0.0.3-me # nmap -sU -p 10000 10.0.0.2 --data-length 215
level 4: 00000000ce4077ab - 00000000ce32b26c
level 7: 00000000ce32b26c - 00000000ce32b26c
^ permalink raw reply
* Re: 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
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