* Re: [PATCH 0/2] doc: net: ieee802154: move from plain text to rst
From: Jonathan Corbet @ 2019-02-27 20:18 UTC (permalink / raw)
To: Stefan Schmidt; +Cc: davem, netdev, linux-doc, linux-wpan
In-Reply-To: <20190227195914.4594-1-stefan@datenfreihafen.org>
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.
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...
Thanks,
jon
^ permalink raw reply
* RE: [PATCH v1 iproute2-next 1/4] rdma: add helper rd_sendrecv_msg()
From: Steve Wise @ 2019-02-27 20:23 UTC (permalink / raw)
To: 'Leon Romanovsky'; +Cc: dsahern, stephen, netdev, linux-rdma
In-Reply-To: <11ec7e04-1bff-e3b2-1b89-db134cd537ba@opengridcomputing.com>
> -----Original Message-----
> From: Steve Wise <swise@opengridcomputing.com>
> Sent: Tuesday, February 26, 2019 11:14 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: dsahern@gmail.com; stephen@networkplumber.org;
> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v1 iproute2-next 1/4] rdma: add helper
> rd_sendrecv_msg()
>
>
> On 2/23/2019 3:26 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:03AM -0800, Steve Wise wrote:
> >> This function sends the constructed netlink message and then
> >> receives the response, displaying any error text.
> >>
> >> Change 'rdma dev set' to use it.
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >> rdma/dev.c | 2 +-
> >> rdma/rdma.h | 1 +
> >> rdma/utils.c | 21 +++++++++++++++++++++
> >> 3 files changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/dev.c b/rdma/dev.c
> >> index 60ff4b31e320..d2949c378f08 100644
> >> --- a/rdma/dev.c
> >> +++ b/rdma/dev.c
> >> @@ -273,7 +273,7 @@ static int dev_set_name(struct rd *rd)
> >> mnl_attr_put_u32(rd->nlh, RDMA_NLDEV_ATTR_DEV_INDEX, rd-
> >dev_idx);
> >> mnl_attr_put_strz(rd->nlh, RDMA_NLDEV_ATTR_DEV_NAME,
> rd_argv(rd));
> >>
> >> - return rd_send_msg(rd);
> >> + return rd_sendrecv_msg(rd, seq);
> >> }
> >>
> >> static int dev_one_set(struct rd *rd)
> >> diff --git a/rdma/rdma.h b/rdma/rdma.h
> >> index 547bb5749a39..20be2f12c4f8 100644
> >> --- a/rdma/rdma.h
> >> +++ b/rdma/rdma.h
> >> @@ -115,6 +115,7 @@ bool rd_check_is_key_exist(struct rd *rd, const
> char *key);
> >> */
> >> int rd_send_msg(struct rd *rd);
> >> int rd_recv_msg(struct rd *rd, mnl_cb_t callback, void *data, uint32_t
> seq);
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq);
> >> void rd_prepare_msg(struct rd *rd, uint32_t cmd, uint32_t *seq,
uint16_t
> flags);
> >> int rd_dev_init_cb(const struct nlmsghdr *nlh, void *data);
> >> int rd_attr_cb(const struct nlattr *attr, void *data);
> >> diff --git a/rdma/utils.c b/rdma/utils.c
> >> index 069d44fece10..a6f2826c9605 100644
> >> --- a/rdma/utils.c
> >> +++ b/rdma/utils.c
> >> @@ -664,6 +664,27 @@ int rd_recv_msg(struct rd *rd, mnl_cb_t callback,
> void *data, unsigned int seq)
> >> return ret;
> >> }
> >>
> >> +static int null_cb(const struct nlmsghdr *nlh, void *data)
> >> +{
> >> + return MNL_CB_OK;
> >> +}
> >> +
> >> +int rd_sendrecv_msg(struct rd *rd, unsigned int seq)
> >> +{
> >> + int ret;
> >> +
> >> + ret = rd_send_msg(rd);
> >> + if (ret) {
> >> + perror(NULL);
> > This is more or less already done in rd_send_msg() and that function
> > prints something in case of execution error. So the missing piece
> > is to update rd_recv_msg(), so all places will "magically" print errors
> > and not only dev_set_name().
>
> Yea ok.
>
dev_set_name() doesn't call rd_recv_msg(). So you're suggesting I fix up
rd_recv_msg() to display errors and make dev_set_name() call rd_recv_msg()
with the null_cb function? You sure that's the way to go?
^ permalink raw reply
* Re: [PATCH net-next 2/8] devlink: add PF and VF port flavours
From: Jiri Pirko @ 2019-02-27 20:17 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: davem, oss-drivers, netdev
In-Reply-To: <20190227092326.5abf417b@cakuba.netronome.com>
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.
^ permalink raw reply
* Re: [PATCH net-next v3 0/8] net: Remove switchdev_ops
From: David Miller @ 2019-02-27 20:40 UTC (permalink / raw)
To: f.fainelli
Cc: netdev, idosch, linux-kernel, devel, bridge, jiri, andrew,
vivien.didelot
In-Reply-To: <20190227194432.725-1-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Wed, 27 Feb 2019 11:44:24 -0800
> This patch series completes the removal of the switchdev_ops by
> converting switchdev_port_attr_set() to use either the blocking
> (process) or non-blocking (atomic) notifier since we typically need to
> deal with both depending on where in the bridge code we get called from.
>
> This was tested with the forwarding selftests and DSA hardware.
>
> Ido, hopefully this captures your comments done on v1, if not, can you
> illustrate with some pseudo-code what you had in mind if that's okay?
>
> Changes in v3:
>
> - added Reviewed-by tags from Ido where relevant
> - added missing notifier_to_errno() in net/bridge/br_switchdev.c when
> calling the atomic notifier for PRE_BRIDGE_FLAGS
> - kept mlxsw_sp_switchdev_init() in mlxsw/
>
> Changes in v2:
>
> - do not check for SWITCHDEV_F_DEFER when calling the blocking notifier
> and instead directly call the atomic notifier from the single location
> where this is required
Series applied, thanks Florian.
I'll push this out after my build tests complete.
^ permalink raw reply
* Re: [PATCH] net: nfc: Fix NULL dereference on nfc_llcp_build_tlv fails
From: David Miller @ 2019-02-27 20:49 UTC (permalink / raw)
To: yuehaibing; +Cc: sameo, linux-kernel, netdev, linux-wireless
In-Reply-To: <20190222073758.5352-1-yuehaibing@huawei.com>
From: Yue Haibing <yuehaibing@huawei.com>
Date: Fri, 22 Feb 2019 15:37:58 +0800
> From: YueHaibing <yuehaibing@huawei.com>
>
> KASAN report this:
. ..
> nfc_llcp_build_tlv will return NULL on fails, caller should check it,
> otherwise will trigger a NULL dereference.
>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: eda21f16a5ed ("NFC: Set MIU and RW values from CONNECT and CC LLCP frames")
> Fixes: d646960f7986 ("NFC: Initial LLCP support")
> Signed-off-by: YueHaibing <yuehaibing@huawei.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net-next v4] route: Add multipath_hash in flowi_common to make user-define hash
From: David Miller @ 2019-02-27 20:50 UTC (permalink / raw)
To: wenxu; +Cc: nikolay, netdev
In-Reply-To: <1550979380-6617-1-git-send-email-wenxu@ucloud.cn>
From: wenxu@ucloud.cn
Date: Sun, 24 Feb 2019 11:36:20 +0800
> From: wenxu <wenxu@ucloud.cn>
>
> Current fib_multipath_hash_policy can make hash based on the L3 or
> L4. But it only work on the outer IP. So a specific tunnel always
> has the same hash value. But a specific tunnel may contain so many
> inner connections.
>
> This patch provide a generic multipath_hash in floi_common. It can
> make a user-define hash which can mix with L3 or L4 hash.
>
> Signed-off-by: wenxu <wenxu@ucloud.cn>
Applied to net-next, thanks.
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Brian Norris @ 2019-02-27 20:51 UTC (permalink / raw)
To: Marc Zyngier
Cc: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu, David S. Miller, devicetree, linux-arm-kernel,
linux-kernel, linux-rockchip, linux-wireless, netdev, linux-pm,
Jeffy Chen, Rafael J. Wysocki, Tony Lindgren, Lorenzo Pieralisi
In-Reply-To: <d67512fe-42b4-513f-d27a-fed85c19e9c2@arm.com>
Hi Marc,
On Wed, Feb 27, 2019 at 10:02:16AM +0000, Marc Zyngier wrote:
> On 26/02/2019 23:28, Brian Norris wrote:
> > On Sun, Feb 24, 2019 at 02:04:22PM +0000, Marc Zyngier wrote:
> >> Note how the interrupt is part of the properties directly attached to the
> >> PCI node. And yet, this interrupt has nothing to do with a PCI legacy
> >> interrupt, as it is attached to the wake-up widget that bypasses the PCIe RC
> >> altogether (Yay for the broken design!). This is in total violation of the
> >> IEEE Std 1275-1994 spec[1], which clearly documents that such interrupt
> >> specifiers describe the PCI device interrupts, and must obey the
> >> INT-{A,B,C,D} mapping. Oops!
> >
> > 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.
Well, in some respects it may be better (mostly, handling in the PCI
core rather than each driver). But I'm still unsure about the DT
binding.
And while perhaps I could find time to revive it, it's probably more
expedient to kill the bad binding first.
> > Frankly, if a proper DT replacement to the admittedly bad binding isn't
> > agreed upon quickly, I'd be very happy to just have WAKE# support
> > removed from the DTS for now, and the existing mwifiex binding should
> > just be removed. (Wake-on-WiFi was never properly vetted on these
> > platforms anyway.) It mostly serves to just cause problems like you've
> > noticed.
>
> Agreed. If there is no actual use for this, and that we can build a case
> for a better solution, let's remove the wakeup support from the Gru DT
> (it is invalid anyway), and bring it back if and when we get the right
> level of support.
+1
Today, something simple like NL80211_WOWLAN_TRIG_DISCONNECT and
NL80211_WOWLAN_TRIG_NET_DETECT may work OK, but I'm not confident that
anything more complicated is really a compelling story today (well,
outside of Android, which has a massively more complicated--and not
upstream--setup for this stuff).
> [...]
>
> > One problem Rockchip authors were also trying to resolve here is that
> > PCIe WAKE# handling should not really be something the PCI device driver
> > has to handle directly. Despite your complaints about not using in-band
> > TLP wakeup, a separate WAKE# pin is in fact a documented part of the
> > PCIe standard, and it so happens that the Rockchip RC does not support
> > handling TLPs in S3, if you want to have decent power consumption. (Your
> > "bad hardware" complaints could justifiably fall here, I suppose.)
> >
> > Additionally, I've had pushback from PCI driver authors/maintainers on
> > adding more special handling for this sort of interrupt property (not
> > the binding specifically, but just the concept of handling WAKE# in the
> > driver), as they claim this should be handled by the system firmware,
> > when they set the appropriate wakeup flags, which filter down to
> > __pci_enable_wake() -> platform_pci_set_wakeup(). That's how x86 systems
> > do it (note: I know for a fact that many have a very similar
> > architecture -- WAKE# is not routed to the RC, because, why does it need
> > to? and they *don't* use TLP wakeup either -- they just hide it in
> > firmware better), and it Just Works.
>
> Even on an arm64 platform, there is no reason why a wakeup interrupt
> couldn't be handled by FW rather than the OS. It just need to be wired
> to the right spot (so that it generates a secure interrupt that can be
> handled by FW).
True...but then you also need a configuration (enable/disable) API for
it too. I don't think we have such a per-device API? So it would be a
pretty similar problem to solve anyway.
> > So, we basically concluded that we should standardize on a way to
> > describe WAKE# interrupts such that PCI drivers don't have to deal with
> > it at all, and the PCI core can do it for us. 12 revisions later
> > and...we still never agreed on a good device tree binding for this.
>
> Is the DT binding the only problem? Do we have an agreement for the core
> code?
I'll have to re-read the old threads. I don't really remember where we
got bogged down... I think one outstanding question was whether WAKE#
should be associated with the port vs. the device. That might have been
might fault for confusing that one though.
> > IOW, maybe your wake-up sub-node is the best way to side-step the
> > problems of conflicting with the OF PCI spec. But I'd still really like
> > to avoid parsing it in mwifiex, if at all possible.
>
> Honestly, my solution is just a terrible hack. I wasn't aware that this
> was a more general problem, and I'd love it to be addressed in the core
> PCI code.
Ack, so we agree.
Thanks,
Brian
^ permalink raw reply
* Re: [PATCH v3 1/2] dt-bindings: net: Add bindings for mdio mux consumers
From: David Miller @ 2019-02-27 20:52 UTC (permalink / raw)
To: pankaj.bansal
Cc: leoyang.li, peda, andrew, f.fainelli, hkallweit1, robh+dt,
mark.rutland, devicetree, netdev
In-Reply-To: <20190225114121.7878-1-pankaj.bansal@nxp.com>
From: Pankaj Bansal <pankaj.bansal@nxp.com>
Date: Mon, 25 Feb 2019 06:16:53 +0000
> When we use the bindings defined in Documentation/devicetree/bindings/mux
> to define mdio mux in producer and consumer terms, it results in two
> devices. one is mux producer and other is mux consumer.
>
> Add the bindings needed for Mdio mux consumer devices.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
Applied.
^ permalink raw reply
* Re: [PATCH v3 2/2] drivers: net: phy: mdio-mux: Add support for Generic Mux controls
From: David Miller @ 2019-02-27 20:52 UTC (permalink / raw)
To: pankaj.bansal; +Cc: leoyang.li, peda, andrew, f.fainelli, hkallweit1, netdev
In-Reply-To: <20190225114121.7878-2-pankaj.bansal@nxp.com>
From: Pankaj Bansal <pankaj.bansal@nxp.com>
Date: Mon, 25 Feb 2019 06:16:55 +0000
> Add support for Generic Mux controls, when Mdio mux node is a consumer
> of mux produced by some other device.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
Applied.
^ permalink raw reply
* Re: [PATCH 0/4] mwifiex PCI/wake-up interrupt fixes
From: Brian Norris @ 2019-02-27 20:57 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: 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: <CAKv+Gu_fZK7dUqrrVPCejAZNWyUeMUX2ojQ=vQ3hiMkGF6e6tw@mail.gmail.com>
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. We just need to figure out a
good way of representing it that doesn't stomp on the existing INTx
definitions.
Brian
^ permalink raw reply
* Re: AF_XDP design flaws
From: Jonathan Lemon @ 2019-02-27 21:03 UTC (permalink / raw)
To: Maxim Mikityanskiy
Cc: Björn Töpel, John Fastabend, netdev,
Björn Töpel, Magnus Karlsson, David S. Miller,
Tariq Toukan, Saeed Mahameed, Eran Ben Elisha
In-Reply-To: <AM6PR05MB58797891F72C7B4992EE6811D1740@AM6PR05MB5879.eurprd05.prod.outlook.com>
On 27 Feb 2019, at 11:17, Maxim Mikityanskiy wrote:
>> -----Original Message-----
>> From: Björn Töpel <bjorn.topel@gmail.com>
>> Sent: 27 February, 2019 10:09
>> To: John Fastabend <john.fastabend@gmail.com>
>> Cc: Maxim Mikityanskiy <maximmi@mellanox.com>;
>> netdev@vger.kernel.org; Björn
>> Töpel <bjorn.topel@intel.com>; Magnus Karlsson
>> <magnus.karlsson@intel.com>;
>> David S. Miller <davem@davemloft.net>; Tariq Toukan
>> <tariqt@mellanox.com>;
>> Saeed Mahameed <saeedm@mellanox.com>; Eran Ben Elisha
>> <eranbe@mellanox.com>
>> Subject: Re: AF_XDP design flaws
>>
>> On 2019-02-26 17:41, John Fastabend wrote:
>>> On 2/26/19 6:49 AM, Maxim Mikityanskiy wrote:
>>>> Hi everyone,
>>>>
>>
>> Hi! It's exciting to see more vendors looking into AF_XDP. ARM was
>> involved early on, and now Mellanox. :-) It's really important that
>> the AF_XDP model is a good fit for all drivers/vendors! Thanks for
>> looking into this.
>>
>>>> I would like to discuss some design flaws of AF_XDP socket (XSK)
>> implementation
>>>> in kernel. At the moment I don't see a way to work around them
>>>> without
>> changing
>>>> the API, so I would like to make sure that I'm not missing anything
>>>> and
>> to
>>>> suggest and discuss some possible improvements that can be made.
>>>>
>>>> The issues I describe below are caused by the fact that the driver
>> depends on
>>>> the application doing some things, and if the application is
>>>> slow/buggy/malicious, the driver is forced to busy poll because of
>>>> the
>> lack of a
>>>> notification mechanism from the application side. I will refer to
>>>> the
>> i40e
>>>> driver implementation a lot, as it is the first implementation of
>>>> AF_XDP,
>> but
>>>> the issues are general and affect any driver. I already considered
>>>> trying
>> to fix
>>>> it on driver level, but it doesn't seem possible, so it looks like
>>>> the
>> behavior
>>>> and implementation of AF_XDP in the kernel has to be changed.
>>>>
>>>> RX side busy polling
>>>> ====================
>>>>
>>>> On the RX side, the driver expects the application to put some
>> descriptors in
>>>> the Fill Ring. There is no way for the application to notify the
>>>> driver
>> that
>>>> there are more Fill Ring descriptors to take, so the driver is
>>>> forced to
>> busy
>>>> poll the Fill Ring if it gets empty. E.g., the i40e driver does it
>>>> in
>> NAPI poll:
>>>>
>>>> int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
>>>> {
>>>> ...
>>>> failure = failure ||
>>>>
>> !i40e_alloc_rx_buffers_fast_zc(rx_ring,
>>>>
>> cleaned_count);
>>>> ...
>>>> return failure ? budget : (int)total_rx_packets;
>>>> }
>>>>
>>>> Basically, it means that if there are no descriptors in the Fill
>>>> Ring,
>> NAPI will
>>>> never stop, draining CPU.
>>>>
>>>> Possible cases when it happens
>>>> ------------------------------
>>>>
>>>> 1. The application is slow, it received some frames in the RX Ring,
>>>> and
>> it is
>>>> still handling the data, so it has no free frames to put to the
>>>> Fill
>> Ring.
>>>>
>>>> 2. The application is malicious, it opens an XSK and puts no frames
>>>> to
>> the Fill
>>>> Ring. It can be used as a local DoS attack.
>>>>
>>>> 3. The application is buggy and stops filling the Fill Ring for
>>>> whatever
>> reason
>>>> (deadlock, waiting for another blocking operation, other bugs).
>>>>
>>>> Although loading an XDP program requires root access, the DoS
>>>> attack can
>> be
>>>> targeted to setups that already use XDP, i.e. an XDP program is
>>>> already
>> loaded.
>>>> Even under root, userspace applications should not be able to
>>>> disrupt
>> system
>>>> stability by just calling normal APIs without an intention to
>>>> destroy the
>>>> system, and here it happens in case 1.
>>> I believe this is by design. If packets per second is your top
>>> priority
>>> (at the expense of power, cpu, etc.) this is the behavior you might
>>> want. To resolve your points if you don't trust the application it
>>> should be isolated to a queue pair and cores so the impact is known
>>> and
>>> managed.
>>>
>>
>> Correct, and I agree with John here. AF_XDP is a raw interface, and
>> needs to be managed.
>
> OK, right, if you want high pps sacrificing system stability and
> kernel-userspace isolation, XSK is at your service. It may be a valid
> point. However, there is still the issue of unintended use.
I believe this is a double-edged sword - on one hand, it appears AF_XDP
is
aimed as a competitor to DPDK, and is focused only on raw packet speed,
at
the expense of usability. This isn't necessarily bad, but in my
experience,
if things aren't simple to use, then they tend not to get widely used.
> If you use XSK, you can take measures to prevent the disruption, e.g.
> run only trusted and properly tested applications or isolate them.
> However, there is a case where XSKs are not really used on a given
> machine, but are still available (CONFIG_XDP_SOCKETS=y), opening a
> huge
> hole. If you don't even know that XSK exists, you are still
> vulnerable.
>
>>> That said having a flag to back-off seems like a good idea. But
>>> should
>>> be doable as a feature without breaking current API.
>
> A flag can be added, e.g., to the sxdp_flags field of struct
> sockaddr_xdp, to switch to the behavior without busy polling on empty
> Fill Ring.
>
> However, I don't think the vulnerability can be eliminated while
> keeping
> the API intact. To protect the systems that don't use XSK, we need one
> of these things:
>
> - Disable XSK support by default, until explicitly enabled by root.
>
> - Disable the old behavior by default, until explicitly enabled by
> root.
>
> Both of those things will require additional setup on machines that
> use
> XSK, after upgrading the kernel.
>
>>>> Possible way to solve the issue
>>>> -------------------------------
>>>>
>>>> When the driver can't take new Fill Ring frames, it shouldn't busy
>>>> poll.
>>>> Instead, it signals the failure to the application (e.g., with
>>>> POLLERR),
>> and
>>>> after that it's up to the application to restart polling (e.g., by
>> calling
>>>> sendto()) after refilling the Fill Ring. The issue with this
>>>> approach is
>> that it
>>>> changes the API, so we either have to deal with it or to introduce
>>>> some
>> API
>>>> version field.
>>> See above. I like the idea here though.
>>>
>>
>> The uapi doesn't mandate *how* the driver should behave. The rational
>> for the aggressive i40e fill ring polling (when the ring is empty) is
>> to minimize the latency. The "fill ring is empty" behavior might be
>> different on different drivers.
>
> If the behavior is different with different drivers, it will be
> difficult to create applications that are portable across NICs. The
> point of creating APIs is to provide a single interface to different
> implementations, thus hiding the differences from a higher abstraction
> level. What the driver does internally is up to the driver, but the
> result visible in the userspace should be the same, so we can't just
> make some drivers stop and wait when the Fill Ring is empty, while the
> others busy poll. It should be controlled by some flags if we want to
> preserve both options.
Or if the driver can't obtain new frames from the fill ring, it just
drops the incoming packet, and recycles the old frame in order to keep
the ring full. (This also assumes that the driver was able to initially
populate the ring in the first place, which eliminates one DoS attack of
never putting anything in the fill ring initially).
I do agree that issues cited in this thread can be resolved with the
use of flags to control the different behaviors - there just needs to
be some agreement on what those behaviors are.
--
Jonathan
^ permalink raw reply
* Re: stmmac / meson8b-dwmac
From: Simon Huelck @ 2019-02-27 21:03 UTC (permalink / raw)
To: Jose Abreu, Sebastian Gottschall, Jerome Brunet,
Martin Blumenstingl
Cc: linux-amlogic, Gpeppe.cavallaro, alexandre.torgue,
Emiliano Ingrassia, netdev
In-Reply-To: <c21c30e9-e53e-02a5-c367-25898c4614e9@synopsys.com>
Hi,
i got another question about NAPI and irq assignment.
my ethernet IRQ is assigned to CPU3:
CPU0 CPU1 CPU2 CPU3
1: 0 0 0 0 GICv2 25 Level
vgic
3: 7265279 3145278 4626601 2454147 GICv2 30 Level
arch_timer
4: 0 0 0 0 GICv2 27 Level
kvm guest vtimer
6: 0 0 0 0 GICv2 169 Level
arm-pmu
7: 0 0 0 0 GICv2 170 Level
arm-pmu
8: 0 0 0 0 GICv2 185 Level
arm-pmu
9: 0 0 0 0 GICv2 186 Level
arm-pmu
10: 0 0 0 0 GICv2 53 Edge
c1108500.i2c
11: 3 0 0 0 GICv2 105 Edge
c1108680.adc
13: 1229 0 0 0 GICv2 225 Edge
ttyAML0
14: 334 0 0 0 GICv2 228 Edge
c8100580.ir
--> 18: 498 0 104034 19348144 GICv2 40
Level eth0
19: 4500 331407 0 0 GICv2 249 Edge
d0072000.mmc
20: 13 0 0 0 GICv2 250 Edge
d0074000.mmc
31: 0 0 0 0 GICv2 35 Edge
meson
32: 0 0 0 0 GICv2 89 Edge
dw_hdmi_top_irq, c883a000.hdmi-tx
34: 140014 0 2264814045 0 GICv2 63 Level
c9100000.usb, dwc2_hsotg:usb1
IPI0: 3195735 3384376 2748050 4076544 Rescheduling
interrupts
IPI1: 1683557 683651 15136522 135 Function call
interrupts
IPI2: 0 0 0 0 CPU stop interrupts
IPI3: 0 0 0 0 CPU stop (for
crash dump) interrupts
IPI4: 0 0 0 0 Timer broadcast
interrupts
IPI5: 6 20 517 0 IRQ work interrupts
IPI6: 0 0 0 0 CPU wake-up
interrupts
Err: 0
but when checking the softirqs i see that work is distributed over all
CPUs, is this normal ?
CPU0 CPU1 CPU2 CPU3
HI: 0 0 0 1
TIMER: 6513467 1258789 1058072 2181809
NET_TX: 574886 708816 1820739 140379
NET_RX: 1882454 920141 15537594 34834596
BLOCK: 3369 14936 3008 2389
IRQ_POLL: 0 0 0 0
TASKLET: 512404 626201 20054827 192419
SCHED: 5946453 1067624 777568 1916812
HRTIMER: 0 0 0 0
RCU: 1934215 719530 867003 876727
regards,
Simon
^ permalink raw reply
* RE: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete' commands
From: Steve Wise @ 2019-02-27 21:18 UTC (permalink / raw)
To: 'Leon Romanovsky'; +Cc: dsahern, stephen, netdev, linux-rdma
In-Reply-To: <94c6a44c-6f08-ffe8-9798-a4faf14bd29a@opengridcomputing.com>
> -----Original Message-----
> From: Steve Wise <swise@opengridcomputing.com>
> Sent: Tuesday, February 26, 2019 11:25 AM
> To: Leon Romanovsky <leon@kernel.org>
> Cc: dsahern@gmail.com; stephen@networkplumber.org;
> netdev@vger.kernel.org; linux-rdma@vger.kernel.org
> Subject: Re: [PATCH v1 iproute2-next 3/4] rdma: add 'link add/delete'
> commands
>
>
> On 2/23/2019 3:43 AM, Leon Romanovsky wrote:
> > On Thu, Feb 21, 2019 at 08:19:12AM -0800, Steve Wise wrote:
> >> Add new 'link' subcommand 'add' and 'delete' to allow binding a soft-
> rdma
> >> device to a netdev interface.
> >>
> >> EG:
> >>
> >> rdma link add rxe_eth0 type rxe netdev eth0
> >> rdma link delete rxe_eth0
> >>
> >> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
> >> ---
> >> rdma/link.c | 67
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >> rdma/rdma.h | 1 +
> >> rdma/utils.c | 2 +-
> >> 3 files changed, 69 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/rdma/link.c b/rdma/link.c
> >> index c064be627be2..afaf19663728 100644
> >> --- a/rdma/link.c
> >> +++ b/rdma/link.c
> >> @@ -14,6 +14,9 @@
> >> static int link_help(struct rd *rd)
> >> {
> >> pr_out("Usage: %s link show [DEV/PORT_INDEX]\n", rd->filename);
> >> + pr_out("Usage: %s link add NAME type TYPE netdev NETDEV\n",
> >> + rd->filename);
> >> + pr_out("Usage: %s link delete NAME\n", rd->filename);
> >> return 0;
> >> }
> >>
> >> @@ -341,10 +344,74 @@ static int link_show(struct rd *rd)
> >> return rd_exec_link(rd, link_one_show, true);
> >> }
> >>
> >> +static int link_add(struct rd *rd)
> >> +{
> >> + char *name;
> >> + char *type = NULL;
> >> + char *dev = NULL;
> >> + uint32_t seq;
> >> +
> >> + if (rd_no_arg(rd)) {
> >> + pr_err("No link name was supplied\n");
> > I think that it is better to have instruction message and not error
> > message: "Please provide ...".
>
>
> Ok. Perhaps a new utility rd_exec_require_link() can be created?
>
>
> >> + return -EINVAL;
> >> + }
> >> + name = rd_argv(rd);
> >> + rd_arg_inc(rd);
> >> + while (!rd_no_arg(rd)) {
> >> + if (rd_argv_match(rd, "type")) {
> >> + rd_arg_inc(rd);
> >> + type = rd_argv(rd);
> >> + } else if (rd_argv_match(rd, "netdev")) {
> >> + rd_arg_inc(rd);
> >> + dev = rd_argv(rd);
> >> + } else {
> >> + pr_err("Invalid parameter %s\n", rd_argv(rd));
> >> + return -EINVAL;
> >> + }
> >> + rd_arg_inc(rd);
> > Please use chains of struct rd_cmd and rd_exec_cmd() instead of
> > open-coding parser.
>
>
> Ok. Like your recently merged series did...
>
>
> >> + }
> >> + if (!type) {
> >> + pr_err("No type was supplied\n");
> >> + return -EINVAL;
> > General parser handle it.
>
>
> Ok.
>
>
> >> + }
> >> + if (!dev) {
> >> + pr_err("No net device was supplied\n");
> >> + return -EINVAL;
> >> + }
> > rd_exec_require_dev() ???
>
>
> Looks like I can use that. I'll try it.
Actually, in the above code, the variable 'dev' is the netdev string, and
that is the 3rd parameter to the 'link add' command, so it isn't appropriate
for rd_exec_require_dev().
Steve
^ permalink raw reply
* 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
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