* Re: [PATCH trivial 1/2] net: Fix Kconfig indentation
From: David Miller @ 2019-09-26 6:56 UTC (permalink / raw)
To: krzk
Cc: m.grzeschik, wg, mkl, andrew, f.fainelli, hkallweit1, kuznet,
yoshfuji, pablo, kadlec, fw, trivial, netdev, linux-kernel,
linux-can, linux-rdma, linux-wireless, b.a.t.m.a.n,
netfilter-devel, coreteam, lvs-devel, rds-devel
In-Reply-To: <20190923155243.6997-1-krzk@kernel.org>
From: Krzysztof Kozlowski <krzk@kernel.org>
Date: Mon, 23 Sep 2019 17:52:42 +0200
> Adjust indentation from spaces to tab (+optional two spaces) as in
> coding style with command like:
> $ sed -e 's/^ /\t/' -i */Kconfig
>
> Signed-off-by: Krzysztof Kozlowski <krzk@kernel.org>
Ok, I'll apply these to 'net'.
^ permalink raw reply
* Re: [PATCH v2 net] net: sched: fix possible crash in tcf_action_destroy()
From: David Miller @ 2019-09-26 6:59 UTC (permalink / raw)
To: edumazet; +Cc: netdev, eric.dumazet, syzkaller, xiyou.wangcong, vladbu, jiri
In-Reply-To: <20190923221857.103908-1-edumazet@google.com>
From: Eric Dumazet <edumazet@google.com>
Date: Mon, 23 Sep 2019 15:18:57 -0700
> If the allocation done in tcf_exts_init() failed,
> we end up with a NULL pointer in exts->actions.
...
> Fixes: 90b73b77d08e ("net: sched: change action API to use array of pointers to actions")
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Reported-by: syzbot <syzkaller@googlegroups.com>
Eric, I applied v1 yesterday that guards at the call site.
Please send a relative patch if you want to do it this way instead.
Sorry.
^ permalink raw reply
* Re: [PATCH] r8152: Use guard clause and fix comment typos
From: David Miller @ 2019-09-26 7:00 UTC (permalink / raw)
To: pmalani; +Cc: hayeswang, grundler, netdev, nic_swsd
In-Reply-To: <20190923222657.253628-1-pmalani@chromium.org>
From: Prashant Malani <pmalani@chromium.org>
Date: Mon, 23 Sep 2019 15:26:57 -0700
> Use a guard clause in tx_bottom() to reduce the indentation of the
> do-while loop. In doing so, convert the do-while to a while to make the
> guard clause checks consistent.
>
> Also, fix a couple of spelling and grammatical mistakes in the
> r8152_csum_workaround() function comment.
>
> Change-Id: I460befde150ad92248fd85b0f189ec2df2ab8431
> Signed-off-by: Prashant Malani <pmalani@chromium.org>
> Reviewed-by: Grant Grundler <grundler@chromium.org>
This is only approiate for net-next, so please resubmit this when the
net-next tree opens back up.
^ permalink raw reply
* Re: [PATCH net v3] net/sched: cbs: Fix not adding cbs instance to list
From: David Miller @ 2019-09-26 7:04 UTC (permalink / raw)
To: vinicius.gomes; +Cc: netdev, jhs, xiyou.wangcong, jiri
In-Reply-To: <20190924050458.14223-1-vinicius.gomes@intel.com>
From: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Date: Mon, 23 Sep 2019 22:04:58 -0700
> When removing a cbs instance when offloading is enabled, the crash
> below can be observed.
>
> The problem happens because that when offloading is enabled, the cbs
> instance is not added to the list.
>
> Also, the current code doesn't handle correctly the case when offload
> is disabled without removing the qdisc: if the link speed changes the
> credit calculations will be wrong. When we create the cbs instance
> with offloading enabled, it's not added to the notification list, when
> later we disable offloading, it's not in the list, so link speed
> changes will not affect it.
>
> The solution for both issues is the same, add the cbs instance being
> created unconditionally to the global list, even if the link state
> notification isn't useful "right now".
>
> Crash log:
...
> Fixes: e0a7683 ("net/sched: cbs: fix port_rate miscalculation")
Please use 12 significant digits for SHA1 IDs in the future in Fixes:
tags.
> Signed-off-by: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Applied and queued up for -stable.
^ permalink raw reply
* Re: [PATCH] net: print proper warning on dst underflow
From: David Miller @ 2019-09-26 7:06 UTC (permalink / raw)
To: Jason; +Cc: netdev, linux-kernel
In-Reply-To: <20190924090937.13001-1-Jason@zx2c4.com>
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Tue, 24 Sep 2019 11:09:37 +0200
> Proper warnings with stack traces make it much easier to figure out
> what's doing the double free and create more meaningful bug reports from
> users.
>
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Applied, thanks.
^ permalink raw reply
* Re: [pull request][net 0/7] Mellanox, mlx5 fixes 2019-09-24
From: David Miller @ 2019-09-26 7:16 UTC (permalink / raw)
To: saeedm; +Cc: netdev
In-Reply-To: <e54d042319eef6a272c1a3e987a0f8513f0de5b7.camel@mellanox.com>
From: Saeed Mahameed <saeedm@mellanox.com>
Date: Tue, 24 Sep 2019 09:51:50 +0000
> On Tue, 2019-09-24 at 09:41 +0000, Saeed Mahameed wrote:
>> Hi Dave,
>>
>> This series introduces some fixes to mlx5 driver.
>> For more information please see tag log below.
>>
>> Please pull and let me know if there is any problem.
>>
>> For -stable v4.10:
>
> correction: 4.20, NOT 4.10.
>
>> ('net/mlx5e: Fix traffic duplication in ethtool steering')
>>
>> For -stable v4.19:
>> ('net/mlx5: Add device ID of upcoming BlueField-2')
>>
>> For -stable v5.3:
>> ('net/mlx5e: Fix matching on tunnel addresses type')
Pulled and queued up for -stable.
^ permalink raw reply
* Re: [PATCH net] macsec: drop skb sk before calling gro_cells_receive
From: David Miller @ 2019-09-26 7:25 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, sd, pabeni
In-Reply-To: <36f492a4977192dc4bc22a8c3bbfaf496ed89328.1569229366.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 23 Sep 2019 17:02:46 +0800
> Fei Liu reported a crash when doing netperf on a topo of macsec
> dev over veth:
...
> The issue was caused by skb's true_size changed without its sk's
> sk_wmem_alloc increased in tcp/skb_gro_receive(). Later when the
> skb is being freed and the skb's truesize is subtracted from its
> sk's sk_wmem_alloc in tcp_wfree(), underflow occurs.
>
> macsec is calling gro_cells_receive() to receive a packet, which
> actually requires skb->sk to be NULL. However when macsec dev is
> over veth, it's possible the skb->sk is still set if the skb was
> not unshared or expanded from the peer veth.
>
> ip_rcv() is calling skb_orphan() to drop the skb's sk for tproxy,
> but it is too late for macsec's calling gro_cells_receive(). So
> fix it by dropping the skb's sk earlier on rx path of macsec.
>
> Fixes: 5491e7c6b1a9 ("macsec: enable GRO and RPS on macsec devices")
> Reported-by: Xiumei Mu <xmu@redhat.com>
> Reported-by: Fei Liu <feliu@redhat.com>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Applied and queued up for -stable, thank you.
^ permalink raw reply
* Re: [PATCH net-next] net: stmmac: Fix page pool size
From: David Miller @ 2019-09-26 7:28 UTC (permalink / raw)
To: thierry.reding
Cc: peppe.cavallaro, alexandre.torgue, joabreu, f.fainelli,
jakub.kicinski, jonathanh, bbiswas, netdev, linux-tegra
In-Reply-To: <20190923095915.11588-1-thierry.reding@gmail.com>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Mon, 23 Sep 2019 11:59:15 +0200
> From: Thierry Reding <treding@nvidia.com>
>
> The size of individual pages in the page pool in given by an order. The
> order is the binary logarithm of the number of pages that make up one of
> the pages in the pool. However, the driver currently passes the number
> of pages rather than the order, so it ends up wasting quite a bit of
> memory.
>
> Fix this by taking the binary logarithm and passing that in the order
> field.
>
> Fixes: 2af6106ae949 ("net: stmmac: Introducing support for Page Pool")
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Not only should this have been marked v2, it should have targetted 'net'
instead of 'net-next'.
In any event, I've applied it, thanks.
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Paul Blakey @ 2019-09-26 7:30 UTC (permalink / raw)
To: Edward Cree, Jakub Kicinski
Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller,
netdev@vger.kernel.org, Jiri Pirko, Cong Wang, Jamal Hadi Salim,
Simon Horman, Or Gerlitz
In-Reply-To: <3c09871f-a367-56ca-0d25-f0699a7b79d0@solarflare.com>
On 9/25/2019 8:01 PM, Edward Cree wrote:
> On 24/09/2019 12:48, Paul Blakey wrote:
>> The 'miss' for all or nothing is easy, but the hard part is combining
>> all the paths a packet can take in software to a single 'all or nothing'
>> rule in hardware.
> But you don't combine them to a single rule in hardware, because you
> have multiple sequential tables. (I just spent the last few weeks
> telling our hardware guys that no, they can't just give us one big
> table and expect the driver to do all that combining, because as you
> say, it's 'the hard part'.)
>
>> What if you 'miss' on the match for the tuple? You already did some
>> processing in hardware, so either you revert those, or you continue in
>> software where you left off (the action ct).
> But the only processing you did was to match stuff and generate metadata
> in the form of lookup keys (e.g. a ct_zone) for the next round of
> matching. There's nothing to "revert" unless you've actually modified
> the packet before sending it to CT, and as I said I don't believe that's
> worth supporting.
>
>> The all or nothing approach will require changing the software model to
>> allow
>>
>> merging the ct zone table matches into the hardware rules
> I don't know how much more clearly I can say this: all-or-nothing does not
> require merging. It just requires any actions that come before a matching
> stage (and that the hw doesn't have the capability to revert) to put a
> rule straight in the 'nothing' bucket.
> So if you write
> chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk action vlan push blah action ct action goto chain X
> the driver can say -EOPNOTSUPP because you pushed a VLAN and might still
> miss in chain X. But if you write
> chain 0 dst_mac aa:bb:cc:dd:ee:ff ct_state -trk action ct action goto chain X
> then the driver will happily offload that because if you miss in the later
> lookups you've not altered the packet — the chain0-rule is *idempotent* so
> it doesn't matter if HW and SW both perform it. (Or even all three of HW,
> tc and OvS.)
Ok, I thought you meant merging the rules because we do want to support
those modifications use-cases.
In nat scenarios the packet will be modified, and then there can be a miss:
-trk .... CT(zone X, Restore NAT),goto chain 1
+trk+est, match on ipv4, CT(zone Y), goto chain 2
+trk+est, output..
In tunneling scenarios, the tunnel device decapsulates the packet before
it even reaches OvS/Tc, which is another modification.
Also, there are stats issues if we already accounted for some actions in
hardware.
Paul.
^ permalink raw reply
* Re: [PATCH] openvswitch: change type of UPCALL_PID attribute to NLA_UNSPEC
From: David Miller @ 2019-09-26 7:33 UTC (permalink / raw)
To: lirongqing; +Cc: netdev, pshelar
In-Reply-To: <1569323512-19195-1-git-send-email-lirongqing@baidu.com>
From: Li RongQing <lirongqing@baidu.com>
Date: Tue, 24 Sep 2019 19:11:52 +0800
> userspace openvswitch patch "(dpif-linux: Implement the API
> functions to allow multiple handler threads read upcall)"
> changes its type from U32 to UNSPEC, but leave the kernel
> unchanged
>
> and after kernel 6e237d099fac "(netlink: Relax attr validation
> for fixed length types)", this bug is exposed by the below
> warning
>
> [ 57.215841] netlink: 'ovs-vswitchd': attribute type 5 has an invalid length.
>
> Signed-off-by: Li RongQing <lirongqing@baidu.com>
Applied, and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH v2] ipv6: do not free rt if FIB_LOOKUP_NOREF is set on suppress rule
From: David Miller @ 2019-09-26 7:35 UTC (permalink / raw)
To: Jason; +Cc: netdev, linux-kernel, weiwan, stable
In-Reply-To: <20190924140128.19394-1-Jason@zx2c4.com>
From: "Jason A. Donenfeld" <Jason@zx2c4.com>
Date: Tue, 24 Sep 2019 16:01:28 +0200
> Commit 7d9e5f422150 removed references from certain dsts, but accounting
> for this never translated down into the fib6 suppression code. This bug
> was triggered by WireGuard users who use wg-quick(8), which uses the
> "suppress-prefix" directive to ip-rule(8) for routing all of their
> internet traffic without routing loops. The test case added here
> causes the reference underflow by causing packets to evaluate a suppress
> rule.
>
> Cc: stable@vger.kernel.org
Please don't CC: stable for networking fixes, I handle these manually as per
the netdev FAQ.
> Fixes: 7d9e5f422150 ("ipv6: convert major tx path to use RT6_LOOKUP_F_DST_NOREF")
> Signed-off-by: Jason A. Donenfeld <Jason@zx2c4.com>
Applied and queued up for -stable, thanks.
^ permalink raw reply
* Re: [PATCH net v2] vsock: Fix a lockdep warning in __vsock_release()
From: Stefano Garzarella @ 2019-09-26 7:47 UTC (permalink / raw)
To: Dexuan Cui
Cc: davem@davemloft.net, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, sashal@kernel.org, stefanha@redhat.com,
gregkh@linuxfoundation.org, arnd@arndb.de, deepa.kernel@gmail.com,
ytht.net@gmail.com, tglx@linutronix.de, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-hyperv@vger.kernel.org,
kvm@vger.kernel.org, virtualization@lists.linux-foundation.org,
Michael Kelley, jhansen@vmware.com
In-Reply-To: <1569460241-57800-1-git-send-email-decui@microsoft.com>
Hi Dexuan,
On Thu, Sep 26, 2019 at 01:11:27AM +0000, Dexuan Cui wrote:
> Lockdep is unhappy if two locks from the same class are held.
>
> Fix the below warning for hyperv and virtio sockets (vmci socket code
> doesn't have the issue) by using lock_sock_nested() when __vsock_release()
> is called recursively:
>
> ============================================
> WARNING: possible recursive locking detected
> 5.3.0+ #1 Not tainted
> --------------------------------------------
> server/1795 is trying to acquire lock:
> ffff8880c5158990 (sk_lock-AF_VSOCK){+.+.}, at: hvs_release+0x10/0x120 [hv_sock]
>
> but task is already holding lock:
> ffff8880c5158150 (sk_lock-AF_VSOCK){+.+.}, at: __vsock_release+0x2e/0xf0 [vsock]
>
> other info that might help us debug this:
> Possible unsafe locking scenario:
>
> CPU0
> ----
> lock(sk_lock-AF_VSOCK);
> lock(sk_lock-AF_VSOCK);
>
> *** DEADLOCK ***
>
> May be due to missing lock nesting notation
>
> 2 locks held by server/1795:
> #0: ffff8880c5d05ff8 (&sb->s_type->i_mutex_key#10){+.+.}, at: __sock_release+0x2d/0xa0
> #1: ffff8880c5158150 (sk_lock-AF_VSOCK){+.+.}, at: __vsock_release+0x2e/0xf0 [vsock]
>
> stack backtrace:
> CPU: 5 PID: 1795 Comm: server Not tainted 5.3.0+ #1
> Call Trace:
> dump_stack+0x67/0x90
> __lock_acquire.cold.67+0xd2/0x20b
> lock_acquire+0xb5/0x1c0
> lock_sock_nested+0x6d/0x90
> hvs_release+0x10/0x120 [hv_sock]
> __vsock_release+0x24/0xf0 [vsock]
> __vsock_release+0xa0/0xf0 [vsock]
> vsock_release+0x12/0x30 [vsock]
> __sock_release+0x37/0xa0
> sock_close+0x14/0x20
> __fput+0xc1/0x250
> task_work_run+0x98/0xc0
> do_exit+0x344/0xc60
> do_group_exit+0x47/0xb0
> get_signal+0x15c/0xc50
> do_signal+0x30/0x720
> exit_to_usermode_loop+0x50/0xa0
> do_syscall_64+0x24e/0x270
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
> RIP: 0033:0x7f4184e85f31
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
>
> NOTE: I only tested the code on Hyper-V. I can not test the code for
> virtio socket, as I don't have a KVM host. :-( Sorry.
>
> @Stefan, @Stefano: please review & test the patch for virtio socket,
> and let me know if the patch breaks anything. Thanks!
Comment below, I'll test it ASAP!
>
> Changes in v2:
> Avoid the duplication of code in v1: https://lkml.org/lkml/2019/8/19/1361
> Also fix virtio socket code.
>
> net/vmw_vsock/af_vsock.c | 19 +++++++++++++++----
> net/vmw_vsock/hyperv_transport.c | 2 +-
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 3 files changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c
> index ab47bf3ab66e..dbae4373cbab 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -638,8 +638,10 @@ struct sock *__vsock_create(struct net *net,
> }
> EXPORT_SYMBOL_GPL(__vsock_create);
>
> -static void __vsock_release(struct sock *sk)
> +static void __vsock_release(struct sock *sk, int level)
> {
> + WARN_ON(level != 1 && level != 2);
> +
> if (sk) {
> struct sk_buff *skb;
> struct sock *pending;
> @@ -648,9 +650,18 @@ static void __vsock_release(struct sock *sk)
> vsk = vsock_sk(sk);
> pending = NULL; /* Compiler warning. */
>
> + /* The release call is supposed to use lock_sock_nested()
> + * rather than lock_sock(), if a sock lock should be acquired.
> + */
> transport->release(vsk);
>
> - lock_sock(sk);
> + /* When "level" is 2, use the nested version to avoid the
> + * warning "possible recursive locking detected".
> + */
> + if (level == 1)
> + lock_sock(sk);
Since lock_sock() calls lock_sock_nested(sk, 0), could we use directly
lock_sock_nested(sk, level) with level = 0 in vsock_release() and
level = SINGLE_DEPTH_NESTING here in the while loop?
> + else
> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> sock_orphan(sk);
> sk->sk_shutdown = SHUTDOWN_MASK;
>
> @@ -659,7 +670,7 @@ static void __vsock_release(struct sock *sk)
>
> /* Clean up any sockets that never were accepted. */
> while ((pending = vsock_dequeue_accept(sk)) != NULL) {
> - __vsock_release(pending);
> + __vsock_release(pending, 2);
> sock_put(pending);
> }
>
> @@ -708,7 +719,7 @@ EXPORT_SYMBOL_GPL(vsock_stream_has_space);
>
> static int vsock_release(struct socket *sock)
> {
> - __vsock_release(sock->sk);
> + __vsock_release(sock->sk, 1);
> sock->sk = NULL;
> sock->state = SS_FREE;
>
> diff --git a/net/vmw_vsock/hyperv_transport.c b/net/vmw_vsock/hyperv_transport.c
> index 261521d286d6..c443db7af8d4 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -559,7 +559,7 @@ static void hvs_release(struct vsock_sock *vsk)
> struct sock *sk = sk_vsock(vsk);
> bool remove_sock;
>
> - lock_sock(sk);
> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> remove_sock = hvs_close_lock_held(vsk);
> release_sock(sk);
> if (remove_sock)
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 5bb70c692b1e..a666ef8fc54e 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -820,7 +820,7 @@ void virtio_transport_release(struct vsock_sock *vsk)
> struct sock *sk = &vsk->sk;
> bool remove_sock = true;
>
> - lock_sock(sk);
> + lock_sock_nested(sk, SINGLE_DEPTH_NESTING);
> if (sk->sk_type == SOCK_STREAM)
> remove_sock = virtio_transport_close(vsk);
>
Thanks,
Stefano
^ permalink raw reply
* [PATCH] net: phy: micrel: add Asym Pause workaround for KSZ9021
From: Hans Andersson @ 2019-09-26 7:54 UTC (permalink / raw)
To: andrew
Cc: f.fainelli, hkallweit1, davem, netdev, linux-kernel,
antoine.tenart, Hans Andersson
From: Hans Andersson <hans.andersson@cellavision.se>
The Micrel KSZ9031 PHY may fail to establish a link when the Asymmetric
Pause capability is set. This issue is described in a Silicon Errata
(DS80000691D or DS80000692D), which advises to always disable the
capability.
Micrel KSZ9021 has no errata, but has the same issue with Asymmetric Pause.
This patch apply the same workaround as the one for KSZ9031.
Signed-off-by: Hans Andersson <hans.andersson@cellavision.se>
---
drivers/net/phy/micrel.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/phy/micrel.c b/drivers/net/phy/micrel.c
index 3c8186f..2fea554 100644
--- a/drivers/net/phy/micrel.c
+++ b/drivers/net/phy/micrel.c
@@ -763,6 +763,8 @@ static int ksz9031_get_features(struct phy_device *phydev)
* Whenever the device's Asymmetric Pause capability is set to 1,
* link-up may fail after a link-up to link-down transition.
*
+ * The Errata Sheet is for ksz9031, but ksz9021 has the same issue
+ *
* Workaround:
* Do not enable the Asymmetric Pause capability bit.
*/
@@ -1076,6 +1078,7 @@ static struct phy_driver ksphy_driver[] = {
/* PHY_GBIT_FEATURES */
.driver_data = &ksz9021_type,
.probe = kszphy_probe,
+ .get_features = ksz9031_get_features,
.config_init = ksz9021_config_init,
.ack_interrupt = kszphy_ack_interrupt,
.config_intr = kszphy_config_intr,
--
2.17.1
^ permalink raw reply related
* Re: [BUG] Unable to handle kernel NULL pointer dereference in phy_support_asym_pause
From: Michal Vokáč @ 2019-09-26 8:11 UTC (permalink / raw)
To: Andrew Lunn
Cc: netdev@vger.kernel.org, Vivien Didelot, Florian Fainelli,
David S. Miller
In-Reply-To: <e7bffa36-f218-d71e-c416-38aff73d35dd@ysoft.com>
On 24. 09. 19 15:10, Michal Vokáč wrote:
> On 24. 09. 19 14:31, Andrew Lunn wrote:
>> On Tue, Sep 24, 2019 at 01:27:24PM +0200, Michal Vokáč wrote:
>>> Hi,
>>>
>>> just tried booting latest next-20190920 on our imx6dl-yapp4-hydra platform
>>> with QCA8334 switch and got this:
>>>
>>> [ 7.424620] [<806840e0>] (phy_support_asym_pause) from [<80686724>] (qca8k_port_enable+0x40/0x48)
>>> [ 7.436911] [<806866e4>] (qca8k_port_enable) from [<80a74134>] (dsa_port_enable+0x3c/0x6c)
>>> [ 7.448629] r7:00000000 r6:e88a02cc r5:e812d090 r4:e812d090
>>> [ 7.457708] [<80a740f8>] (dsa_port_enable) from [<80a730bc>] (dsa_register_switch+0x798/0xacc)
>>> [ 7.469833] r5:e812d0cc r4:e812d090
>>
>> Hi Michal
>>
>> Please could you add a printk to verify it is the CPU port, and that
>> in qca8k_port_enable() phy is a NULL pointer.
>>
>> I think the fix is going to look something like:
>>
>> diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
>> index 16f15c93a102..86c80a873e30 100644
>> --- a/drivers/net/dsa/qca8k.c
>> +++ b/drivers/net/dsa/qca8k.c
>> @@ -939,7 +939,8 @@ qca8k_port_enable(struct dsa_switch *ds, int port,
>> qca8k_port_set_status(priv, port, 1);
>> priv->port_sts[port].enabled = 1;
>
> dev_info(priv->dev, "port: %d, phy: %d", port, (u32)phy);
>
>> - phy_support_asym_pause(phy);
>> + if (phy)
>> + phy_support_asym_pause(phy);
>> return 0;
>> }
>>
>
> I added the printk and the above fix and can confirm that it is the CPU
> port and the phy is NULL pointer:
>
> [ 6.976366] qca8k 2188000.ethernet-1:0a: Using legacy PHYLIB callbacks. Please migrate to PHYLINK!
> [ 6.992021] qca8k 2188000.ethernet-1:0a: port: 0, phy: 0
> [ 7.001323] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): PHY [2188000.ethernet-1:01] driver [Generic PHY]
> [ 7.014221] qca8k 2188000.ethernet-1:0a eth2 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
> [ 7.030598] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): PHY [2188000.ethernet-1:02] driver [Generic PHY]
> [ 7.043500] qca8k 2188000.ethernet-1:0a eth1 (uninitialized): phy: setting supported 00,00000000,000062ef advertising 00,00000000,000062ef
> [ 7.063335] DSA: tree 0 setup
>
> Now the device boots but there is a problem with the CPU port configuration:
>
> root@hydraco:~# ifconfig eth0 up
> [ 255.256047] Generic PHY fixed-0:00: attached PHY driver [Generic PHY] (mii_bus:phy_addr=fixed-0:00, irq=POLL)
> [ 255.272449] fec 2188000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
> [ 255.286539] IPv6: ADDRCONF(NETDEV_CHANGE): eth0: link becomes ready
> root@hydraco:~# ifconfig eth1 up
> [ 268.350078] qca8k 2188000.ethernet-1:0a: port: 3, phy: -393143296
> [ 268.364442] qca8k 2188000.ethernet-1:0a eth1: configuring for phy/ link mode
> [ 268.375400] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//Unknown/Unknown adv=00,00000000,000062ef pause=10 link=0 an=1
> [ 268.393901] qca8k 2188000.ethernet-1:0a eth1: phy link up /1Gbps/Full
> [ 268.404849] qca8k 2188000.ethernet-1:0a eth1: phylink_mac_config: mode=phy//1Gbps/Full adv=00,00000000,00000000 pause=0e link=1 an=0
> [ 268.420740] qca8k 2188000.ethernet-1:0a eth1: Link is Up - 1Gbps/Full - flow control rx/tx
> [ 268.432995] IPv6: ADDRCONF(NETDEV_CHANGE): eth1: link becomes ready
> root@hydraco:~# udhcpc -i eth1
> udhcpc (v1.23.2) started
> Sending discover...
> Sending discover...
> Sending discover...
I found and fixed the problem with CPU and USER ports not working.
The issue was in the fact that the qca8k driver incorrectly allocates
the dsa_switch structure with DSA_MAX_PORTS (which is 12) but the QCA8K
switches support up 7 ports.
So the dsa_tree_setup_switches() looped with num_ports set to 12.
Since commit [1] disables all unused ports, ports 7-11 were disabled.
What happened is that some registers were incorrectly rewritten by
the qca8k_port_disable() calls.
I will submit a patch very soon.
Thank you,
Michal
[1] 0394a63acfe2 ("net: dsa: enable and disable all ports")
^ permalink raw reply
* Re: [PATCH V2 6/8] mdev: introduce virtio device and its device ops
From: Jason Wang @ 2019-09-26 8:12 UTC (permalink / raw)
To: Tian, Kevin, kvm@vger.kernel.org, linux-s390@vger.kernel.org,
linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org,
intel-gfx@lists.freedesktop.org,
intel-gvt-dev@lists.freedesktop.org, kwankhede@nvidia.com,
alex.williamson@redhat.com, mst@redhat.com, Bie, Tiwei
Cc: christophe.de.dinechin@gmail.com, sebott@linux.ibm.com,
airlied@linux.ie, joonas.lahtinen@linux.intel.com,
heiko.carstens@de.ibm.com,
virtualization@lists.linux-foundation.org,
rob.miller@broadcom.com, lulu@redhat.com, eperezma@redhat.com,
pasic@linux.ibm.com, borntraeger@de.ibm.com,
haotian.wang@sifive.com, Wang, Zhi A, farman@linux.ibm.com,
idos@mellanox.com, gor@linux.ibm.com, Liang, Cunming,
zhenyuw@linux.intel.com, Vivi, Rodrigo, Wang, Xiao W,
freude@linux.ibm.com, jani.nikula@linux.intel.com,
parav@mellanox.com, Wang, Zhihong, akrowiak@linux.ibm.com,
netdev@vger.kernel.org, cohuck@redhat.com, oberpar@linux.ibm.com,
maxime.coquelin@redhat.com, daniel@ffwll.ch, Zhu, Lingshan
In-Reply-To: <AADFC41AFE54684AB9EE6CBC0274A5D19D590FE4@SHSMSX104.ccr.corp.intel.com>
On 2019/9/26 上午8:48, Tian, Kevin wrote:
>>>> +};
>>> I'm not sure how stable above ops are.
>> It's the kernel internal API, so there's no strict requirement for this.
>> We will export a version value for userspace for compatibility.
>>
>>
>>> Does it make sense if defining
>>> just two callbacks here, e.g. vq_ctrl and device_ctrl, and then let the
>>> vendor driver to handle specific ops in each category (similar to how
>>> ioctl works)?
>> My understanding is that it introduce another indirection, you still
>> need to differ from different command, and it's less flexible than
>> direct callback.
>>
>> What's the value of doing this?
>>
> I just thought doing so may provide better compatibility to the
> parent driver. Even when new op is introduced, a parent driver
> that was developed against the old set can still be loaded in the
> new kernel. It just returns error when unrecognized ops are
> routed through vq_ctrl and device_ctrl, if the userspace doesn't
> favor the exposed version value. But if above ops set is pretty
> stable, then this comment can be ignored.
This is really good point, we should keep it stable as a real transport.
And when there's major changes, we should advertise through version then
we can provide a new set of functions.
Thanks
>
> Thanks
> Kevin
^ permalink raw reply
* RE: [PATCH v3 0/2] net: stmmac: Enhanced addressing mode for DWMAC 4.10
From: Jose Abreu @ 2019-09-26 8:22 UTC (permalink / raw)
To: Thierry Reding, Florian Fainelli
Cc: Jose Abreu, David Miller, peppe.cavallaro@st.com,
alexandre.torgue@st.com, jonathanh@nvidia.com, bbiswas@nvidia.com,
netdev@vger.kernel.org, linux-tegra@vger.kernel.org
In-Reply-To: <20190925224620.GA8115@mithrandir>
From: Thierry Reding <thierry.reding@gmail.com>
Date: Sep/25/2019, 23:46:20 (UTC+00:00)
> On Wed, Sep 25, 2019 at 10:31:13AM -0700, Florian Fainelli wrote:
> > The way I would approach it (as done in bcmgenet.c) is that if the
> > platform both has CONFIG_PHYS_ADDR_T_64BIT=y and supports > 32-bits
> > addresses, then you write the upper 32-bits otherwise, you do not. Given
> > you indicate that the registers are safe to write regardless, then maybe
> > just the check on CONFIG_PHYS_ADDR_T_64BIT is enough for your case. The
> > rationale in my case is that register writes to on-chip descriptors are
> > fairly expensive (~200ns per operation) and get in the hot-path.
> >
> > The CONFIG_PHYS_ADDR_T_64BIT check addresses both native 64-bit
> > platforms (e.g.: ARM64) and those that do support LPAE (ARM LPAE for
> > instance).
>
> I think we actually want CONFIG_DMA_ADDR_T_64BIT here because we're
> dealing with addresses returned from the DMA API here.
>
> I can add an additional condition for the upper 32-bit register writes,
> something like:
>
> if (IS_ENABLED(CONFIG_DMA_ADDR_T_64BIT) && priv->dma_cfg->eame)
> ...
>
> The compiler should be able to eliminate that as dead code on platforms
> that don't support 64-bit DMA addresses, but the code should still be
> compiler regardless of the setting, thus increasing the compile
> coverage.
I'm fine with this. Some notes:
a) Do not try to enable dma_cfg->eame if CONFIG_DMA_ADDR_T_64BIT is not
enabled;
b) You can even add a likely() around priv->dma_cfg->eame check because
if a given SoC supports 64 bit addressing then its highly probable that
the IP will also support EAME.
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH] vhost: introduce mdev based hardware backend
From: Michael S. Tsirkin @ 2019-09-26 8:35 UTC (permalink / raw)
To: Tiwei Bie
Cc: jasowang, alex.williamson, maxime.coquelin, linux-kernel, kvm,
virtualization, netdev, dan.daly, cunming.liang, zhihong.wang,
lingshan.zhu
In-Reply-To: <20190926045427.4973-1-tiwei.bie@intel.com>
On Thu, Sep 26, 2019 at 12:54:27PM +0800, Tiwei Bie wrote:
> This patch introduces a mdev based hardware vhost backend.
> This backend is built on top of the same abstraction used
> in virtio-mdev and provides a generic vhost interface for
> userspace to accelerate the virtio devices in guest.
>
> This backend is implemented as a mdev device driver on top
> of the same mdev device ops used in virtio-mdev but using
> a different mdev class id, and it will register the device
> as a VFIO device for userspace to use. Userspace can setup
> the IOMMU with the existing VFIO container/group APIs and
> then get the device fd with the device name. After getting
> the device fd of this device, userspace can use vhost ioctls
> to setup the backend.
>
> Signed-off-by: Tiwei Bie <tiwei.bie@intel.com>
> ---
> This patch depends on below series:
> https://lkml.org/lkml/2019/9/24/357
>
> RFC v4 -> v1:
> - Implement vhost-mdev as a mdev device driver directly and
> connect it to VFIO container/group. (Jason);
> - Pass ring addresses as GPAs/IOVAs in vhost-mdev to avoid
> meaningless HVA->GPA translations (Jason);
>
> RFC v3 -> RFC v4:
> - Build vhost-mdev on top of the same abstraction used by
> virtio-mdev (Jason);
> - Introduce vhost fd and pass VFIO fd via SET_BACKEND ioctl (MST);
>
> RFC v2 -> RFC v3:
> - Reuse vhost's ioctls instead of inventing a VFIO regions/irqs
> based vhost protocol on top of vfio-mdev (Jason);
>
> RFC v1 -> RFC v2:
> - Introduce a new VFIO device type to build a vhost protocol
> on top of vfio-mdev;
>
> drivers/vhost/Kconfig | 9 +
> drivers/vhost/Makefile | 3 +
> drivers/vhost/mdev.c | 381 +++++++++++++++++++++++++++++++++++++
> include/uapi/linux/vhost.h | 8 +
> 4 files changed, 401 insertions(+)
> create mode 100644 drivers/vhost/mdev.c
>
> diff --git a/drivers/vhost/Kconfig b/drivers/vhost/Kconfig
> index 3d03ccbd1adc..decf0be8efe9 100644
> --- a/drivers/vhost/Kconfig
> +++ b/drivers/vhost/Kconfig
> @@ -34,6 +34,15 @@ config VHOST_VSOCK
> To compile this driver as a module, choose M here: the module will be called
> vhost_vsock.
>
> +config VHOST_MDEV
> + tristate "Vhost driver for Mediated devices"
> + depends on EVENTFD && VFIO && VFIO_MDEV
> + select VHOST
> + default n
> + ---help---
> + Say M here to enable the vhost_mdev module for use with
> + the mediated device based hardware vhost accelerators
> +
> config VHOST
> tristate
> ---help---
> diff --git a/drivers/vhost/Makefile b/drivers/vhost/Makefile
> index 6c6df24f770c..ad9c0f8c6d8c 100644
> --- a/drivers/vhost/Makefile
> +++ b/drivers/vhost/Makefile
> @@ -10,4 +10,7 @@ vhost_vsock-y := vsock.o
>
> obj-$(CONFIG_VHOST_RING) += vringh.o
>
> +obj-$(CONFIG_VHOST_MDEV) += vhost_mdev.o
> +vhost_mdev-y := mdev.o
> +
> obj-$(CONFIG_VHOST) += vhost.o
> diff --git a/drivers/vhost/mdev.c b/drivers/vhost/mdev.c
> new file mode 100644
> index 000000000000..1c12a25b86a2
> --- /dev/null
> +++ b/drivers/vhost/mdev.c
> @@ -0,0 +1,381 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Copyright (C) 2018-2019 Intel Corporation.
> + */
> +
> +#include <linux/compat.h>
> +#include <linux/kernel.h>
> +#include <linux/miscdevice.h>
> +#include <linux/mdev.h>
> +#include <linux/module.h>
> +#include <linux/vfio.h>
> +#include <linux/vhost.h>
> +#include <linux/virtio_mdev.h>
> +
> +#include "vhost.h"
> +
> +struct vhost_mdev {
> + /* The lock is to protect this structure. */
> + struct mutex mutex;
> + struct vhost_dev dev;
> + struct vhost_virtqueue *vqs;
> + int nvqs;
> + u64 state;
> + u64 features;
> + u64 acked_features;
> + bool opened;
> + struct mdev_device *mdev;
> +};
> +
> +static u8 mdev_get_status(struct mdev_device *mdev)
> +{
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> +
> + return ops->get_status(mdev);
> +}
> +
> +static void mdev_set_status(struct mdev_device *mdev, u8 status)
> +{
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> +
> + return ops->set_status(mdev, status);
> +}
> +
> +static void mdev_add_status(struct mdev_device *mdev, u8 status)
> +{
> + status |= mdev_get_status(mdev);
> + mdev_set_status(mdev, status);
> +}
> +
> +static void mdev_reset(struct mdev_device *mdev)
> +{
> + mdev_set_status(mdev, 0);
> +}
> +
> +static void handle_vq_kick(struct vhost_work *work)
> +{
> + struct vhost_virtqueue *vq = container_of(work, struct vhost_virtqueue,
> + poll.work);
> + struct vhost_mdev *m = container_of(vq->dev, struct vhost_mdev, dev);
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(m->mdev);
> +
> + ops->kick_vq(m->mdev, vq - m->vqs);
> +}
> +
> +static irqreturn_t vhost_mdev_virtqueue_cb(void *private)
> +{
> + struct vhost_virtqueue *vq = private;
> + struct eventfd_ctx *call_ctx = vq->call_ctx;
> +
> + if (call_ctx)
> + eventfd_signal(call_ctx, 1);
> + return IRQ_HANDLED;
> +}
> +
> +static long vhost_mdev_reset(struct vhost_mdev *m)
> +{
> + struct mdev_device *mdev = m->mdev;
> +
> + mdev_reset(mdev);
> + mdev_add_status(mdev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> + mdev_add_status(mdev, VIRTIO_CONFIG_S_DRIVER);
> + return 0;
> +}
> +
> +static long vhost_mdev_start(struct vhost_mdev *m)
> +{
> + struct mdev_device *mdev = m->mdev;
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> + struct virtio_mdev_callback cb;
> + struct vhost_virtqueue *vq;
> + int idx;
> +
> + ops->set_features(mdev, m->acked_features);
> +
> + mdev_add_status(mdev, VIRTIO_CONFIG_S_FEATURES_OK);
> + if (!(mdev_get_status(mdev) & VIRTIO_CONFIG_S_FEATURES_OK))
> + goto reset;
> +
> + for (idx = 0; idx < m->nvqs; idx++) {
> + vq = &m->vqs[idx];
> +
> + if (!vq->desc || !vq->avail || !vq->used)
> + break;
> +
> + if (ops->set_vq_state(mdev, idx, vq->last_avail_idx))
> + goto reset;
> +
> + /*
> + * In vhost-mdev, userspace should pass ring addresses
> + * in guest physical addresses when IOMMU is disabled or
> + * IOVAs when IOMMU is enabled.
> + */
> + if (ops->set_vq_address(mdev, idx, (u64)vq->desc,
> + (u64)vq->avail, (u64)vq->used))
> + goto reset;
> +
> + ops->set_vq_num(mdev, idx, vq->num);
> +
> + cb.callback = vhost_mdev_virtqueue_cb;
> + cb.private = vq;
> + ops->set_vq_cb(mdev, idx, &cb);
> +
> + ops->set_vq_ready(mdev, idx, 1);
> + }
> +
> + mdev_add_status(mdev, VIRTIO_CONFIG_S_DRIVER_OK);
> + if (mdev_get_status(mdev) & VIRTIO_CONFIG_S_NEEDS_RESET)
> + goto reset;
> + return 0;
> +
> +reset:
> + vhost_mdev_reset(m);
> + return -ENODEV;
> +}
> +
> +static long vhost_set_state(struct vhost_mdev *m, u64 __user *statep)
> +{
> + u64 state;
> + long r;
> +
> + if (copy_from_user(&state, statep, sizeof(state)))
> + return -EFAULT;
> +
> + if (state >= VHOST_MDEV_S_MAX)
> + return -EINVAL;
> +
> + if (m->state == state)
> + return 0;
> +
> + m->state = state;
> +
> + switch (m->state) {
> + case VHOST_MDEV_S_RUNNING:
> + r = vhost_mdev_start(m);
> + break;
> + case VHOST_MDEV_S_STOPPED:
> + r = vhost_mdev_reset(m);
> + break;
> + default:
> + r = -EINVAL;
> + break;
> + }
> +
> + return r;
> +}
> +
> +static long vhost_get_features(struct vhost_mdev *m, u64 __user *featurep)
> +{
> + if (copy_to_user(featurep, &m->features, sizeof(m->features)))
> + return -EFAULT;
> + return 0;
> +}
> +
> +static long vhost_set_features(struct vhost_mdev *m, u64 __user *featurep)
> +{
> + u64 features;
> +
> + if (copy_from_user(&features, featurep, sizeof(features)))
> + return -EFAULT;
> +
> + if (features & ~m->features)
> + return -EINVAL;
> +
> + m->acked_features = features;
> +
> + return 0;
> +}
> +
> +static long vhost_get_vring_base(struct vhost_mdev *m, void __user *argp)
> +{
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(m->mdev);
> + struct vhost_virtqueue *vq;
> + u32 idx;
> + long r;
> +
> + r = get_user(idx, (u32 __user *)argp);
> + if (r < 0)
> + return r;
> + if (idx >= m->nvqs)
> + return -ENOBUFS;
> +
> + vq = &m->vqs[idx];
> + vq->last_avail_idx = ops->get_vq_state(m->mdev, idx);
> +
> + return vhost_vring_ioctl(&m->dev, VHOST_GET_VRING_BASE, argp);
> +}
> +
> +static int vhost_mdev_open(void *device_data)
> +{
> + struct vhost_mdev *m = device_data;
> + struct vhost_dev *dev;
> + struct vhost_virtqueue **vqs;
> + int nvqs, i, r;
> +
> + if (!try_module_get(THIS_MODULE))
> + return -ENODEV;
> +
> + mutex_lock(&m->mutex);
> +
> + if (m->opened) {
> + r = -EBUSY;
> + goto err;
> + }
> +
> + nvqs = m->nvqs;
> + vhost_mdev_reset(m);
> +
> + memset(&m->dev, 0, sizeof(m->dev));
> + memset(m->vqs, 0, nvqs * sizeof(struct vhost_virtqueue));
> +
> + vqs = kmalloc_array(nvqs, sizeof(*vqs), GFP_KERNEL);
> + if (!vqs) {
> + r = -ENOMEM;
> + goto err;
> + }
> +
> + dev = &m->dev;
> + for (i = 0; i < nvqs; i++) {
> + vqs[i] = &m->vqs[i];
> + vqs[i]->handle_kick = handle_vq_kick;
> + }
> + vhost_dev_init(dev, vqs, nvqs, 0, 0, 0);
> + m->opened = true;
> + mutex_unlock(&m->mutex);
> +
> + return 0;
> +
> +err:
> + mutex_unlock(&m->mutex);
> + module_put(THIS_MODULE);
> + return r;
> +}
> +
> +static void vhost_mdev_release(void *device_data)
> +{
> + struct vhost_mdev *m = device_data;
> +
> + mutex_lock(&m->mutex);
> + vhost_mdev_reset(m);
> + vhost_dev_stop(&m->dev);
> + vhost_dev_cleanup(&m->dev);
> +
> + kfree(m->dev.vqs);
> + m->opened = false;
> + mutex_unlock(&m->mutex);
> + module_put(THIS_MODULE);
> +}
> +
> +static long vhost_mdev_unlocked_ioctl(void *device_data,
> + unsigned int cmd, unsigned long arg)
> +{
> + struct vhost_mdev *m = device_data;
> + void __user *argp = (void __user *)arg;
> + long r;
> +
> + mutex_lock(&m->mutex);
> +
> + switch (cmd) {
> + case VHOST_MDEV_SET_STATE:
> + r = vhost_set_state(m, argp);
> + break;
> + case VHOST_GET_FEATURES:
> + r = vhost_get_features(m, argp);
> + break;
> + case VHOST_SET_FEATURES:
> + r = vhost_set_features(m, argp);
> + break;
> + case VHOST_GET_VRING_BASE:
> + r = vhost_get_vring_base(m, argp);
> + break;
> + default:
> + r = vhost_dev_ioctl(&m->dev, cmd, argp);
> + if (r == -ENOIOCTLCMD)
> + r = vhost_vring_ioctl(&m->dev, cmd, argp);
> + }
> +
> + mutex_unlock(&m->mutex);
> + return r;
> +}
> +
> +static const struct vfio_device_ops vfio_vhost_mdev_dev_ops = {
> + .name = "vfio-vhost-mdev",
> + .open = vhost_mdev_open,
> + .release = vhost_mdev_release,
> + .ioctl = vhost_mdev_unlocked_ioctl,
> +};
> +
> +static int vhost_mdev_probe(struct device *dev)
> +{
> + struct mdev_device *mdev = mdev_from_dev(dev);
> + const struct virtio_mdev_device_ops *ops = mdev_get_dev_ops(mdev);
> + struct vhost_mdev *m;
> + int nvqs, r;
> +
> + m = kzalloc(sizeof(*m), GFP_KERNEL | __GFP_RETRY_MAYFAIL);
> + if (!m)
> + return -ENOMEM;
> +
> + mutex_init(&m->mutex);
> +
> + nvqs = ops->get_queue_max(mdev);
> + m->nvqs = nvqs;
> +
> + m->vqs = kmalloc_array(nvqs, sizeof(struct vhost_virtqueue),
> + GFP_KERNEL);
> + if (!m->vqs) {
> + r = -ENOMEM;
> + goto err;
> + }
> +
> + r = vfio_add_group_dev(dev, &vfio_vhost_mdev_dev_ops, m);
> + if (r)
> + goto err;
> +
> + m->features = ops->get_features(mdev);
> + m->mdev = mdev;
> + return 0;
> +
> +err:
> + kfree(m->vqs);
> + kfree(m);
> + return r;
> +}
> +
> +static void vhost_mdev_remove(struct device *dev)
> +{
> + struct vhost_mdev *m;
> +
> + m = vfio_del_group_dev(dev);
> + mutex_destroy(&m->mutex);
> + kfree(m->vqs);
> + kfree(m);
> +}
> +
> +static struct mdev_class_id id_table[] = {
> + { MDEV_ID_VHOST },
> + { 0 },
> +};
> +
> +static struct mdev_driver vhost_mdev_driver = {
> + .name = "vhost_mdev",
> + .probe = vhost_mdev_probe,
> + .remove = vhost_mdev_remove,
> + .id_table = id_table,
> +};
> +
> +static int __init vhost_mdev_init(void)
> +{
> + return mdev_register_driver(&vhost_mdev_driver, THIS_MODULE);
> +}
> +module_init(vhost_mdev_init);
> +
> +static void __exit vhost_mdev_exit(void)
> +{
> + mdev_unregister_driver(&vhost_mdev_driver);
> +}
> +module_exit(vhost_mdev_exit);
> +
> +MODULE_VERSION("0.0.1");
> +MODULE_LICENSE("GPL v2");
> +MODULE_DESCRIPTION("Mediated device based accelerator for virtio");
> diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
> index 40d028eed645..5afbc2f08fa3 100644
> --- a/include/uapi/linux/vhost.h
> +++ b/include/uapi/linux/vhost.h
> @@ -116,4 +116,12 @@
> #define VHOST_VSOCK_SET_GUEST_CID _IOW(VHOST_VIRTIO, 0x60, __u64)
> #define VHOST_VSOCK_SET_RUNNING _IOW(VHOST_VIRTIO, 0x61, int)
>
> +/* VHOST_MDEV specific defines */
> +
> +#define VHOST_MDEV_SET_STATE _IOW(VHOST_VIRTIO, 0x70, __u64)
> +
> +#define VHOST_MDEV_S_STOPPED 0
> +#define VHOST_MDEV_S_RUNNING 1
> +#define VHOST_MDEV_S_MAX 2
> +
> #endif
So assuming we have an underlying device that behaves like virtio:
1. Should we use SET_STATUS maybe?
2. Do we want a reset ioctl?
3. Do we want ability to enable rings individually?
4. Does device need to limit max ring size?
5. Does device need to limit max number of queues?
--
MST
^ permalink raw reply
* [PATCH net-next 0/7] optimize openvswitch flow looking up
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
This series patch optimize openvswitch.
Patch 1, 2, 4: Port Pravin B Shelar patches to
linux upstream with little changes.
Patch 5, 6, 7: Optimize the flow looking up and
simplify the flow hash.
The performance test is on Intel Xeon E5-2630 v4.
The test topology is show as below:
+-----------------------------------+
| +---------------------------+ |
| | eth0 ovs-switch eth1 | | Host0
| +---------------------------+ |
+-----------------------------------+
^ |
| |
| |
| |
| v
+-----+----+ +----+-----+
| netperf | Host1 | netserver| Host2
+----------+ +----------+
We use netperf send the 64B frame, and insert 255+ flow-mask:
$ ovs-dpctl add-flow ovs-switch "in_port(1),eth(dst=00:01:00:00:00:00/ff:ff:ff:ff:ff:01),eth_type(0x0800),ipv4(frag=no)" 2
...
$ ovs-dpctl add-flow ovs-switch "in_port(1),eth(dst=00:ff:00:00:00:00/ff:ff:ff:ff:ff:ff),eth_type(0x0800),ipv4(frag=no)" 2
$ netperf -t UDP_STREAM -H 2.2.2.200 -l 40 -- -m 18
* Without series patch, throughput 8.28Mbps
* With series patch, throughput 46.05Mbps
Tonghao Zhang (7):
net: openvswitch: add flow-mask cache for performance
net: openvswitch: convert mask list in mask array
net: openvswitch: shrink the mask array if necessary
net: openvswitch: optimize flow mask cache hash collision
net: openvswitch: optimize flow-mask looking up
net: openvswitch: simplify the flow_hash
net: openvswitch: add likely in flow_lookup
net/openvswitch/datapath.c | 3 +-
net/openvswitch/flow.h | 1 -
net/openvswitch/flow_table.c | 308 ++++++++++++++++++++++++++++++++++++-------
net/openvswitch/flow_table.h | 19 ++-
4 files changed, 282 insertions(+), 49 deletions(-)
--
1.8.3.1
^ permalink raw reply
* [PATCH net-next 1/7] net: openvswitch: add flow-mask cache for performance
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
The idea of this optimization comes from a patch which
is committed in 2014, openvswitch community. The author
is Pravin B Shelar. In order to get high performance, I
implement it again. Later patches will use it.
Pravin B Shelar, says:
| On every packet OVS needs to lookup flow-table with every
| mask until it finds a match. The packet flow-key is first
| masked with mask in the list and then the masked key is
| looked up in flow-table. Therefore number of masks can
| affect packet processing performance.
Link: https://github.com/openvswitch/ovs/commit/5604935e4e1cbc16611d2d97f50b717aa31e8ec5
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/datapath.c | 3 +-
net/openvswitch/flow_table.c | 109 +++++++++++++++++++++++++++++++++++++------
net/openvswitch/flow_table.h | 11 ++++-
3 files changed, 107 insertions(+), 16 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index dde9d76..3d7b1c4 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -227,7 +227,8 @@ void ovs_dp_process_packet(struct sk_buff *skb, struct sw_flow_key *key)
stats = this_cpu_ptr(dp->stats_percpu);
/* Look up flow. */
- flow = ovs_flow_tbl_lookup_stats(&dp->table, key, &n_mask_hit);
+ flow = ovs_flow_tbl_lookup_stats(&dp->table, key, skb_get_hash(skb),
+ &n_mask_hit);
if (unlikely(!flow)) {
struct dp_upcall_info upcall;
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index cf3582c..3d515c0 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -36,6 +36,10 @@
#define TBL_MIN_BUCKETS 1024
#define REHASH_INTERVAL (10 * 60 * HZ)
+#define MC_HASH_SHIFT 8
+#define MC_HASH_ENTRIES (1u << MC_HASH_SHIFT)
+#define MC_HASH_SEGS ((sizeof(uint32_t) * 8) / MC_HASH_SHIFT)
+
static struct kmem_cache *flow_cache;
struct kmem_cache *flow_stats_cache __read_mostly;
@@ -168,10 +172,15 @@ int ovs_flow_tbl_init(struct flow_table *table)
{
struct table_instance *ti, *ufid_ti;
- ti = table_instance_alloc(TBL_MIN_BUCKETS);
+ table->mask_cache = __alloc_percpu(sizeof(struct mask_cache_entry) *
+ MC_HASH_ENTRIES,
+ __alignof__(struct mask_cache_entry));
+ if (!table->mask_cache)
+ return -ENOMEM;
+ ti = table_instance_alloc(TBL_MIN_BUCKETS);
if (!ti)
- return -ENOMEM;
+ goto free_mask_cache;
ufid_ti = table_instance_alloc(TBL_MIN_BUCKETS);
if (!ufid_ti)
@@ -187,6 +196,8 @@ int ovs_flow_tbl_init(struct flow_table *table)
free_ti:
__table_instance_destroy(ti);
+free_mask_cache:
+ free_percpu(table->mask_cache);
return -ENOMEM;
}
@@ -243,6 +254,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
struct table_instance *ti = rcu_dereference_raw(table->ti);
struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
+ free_percpu(table->mask_cache);
table_instance_destroy(ti, ufid_ti, false);
}
@@ -425,7 +437,8 @@ static bool ovs_flow_cmp_unmasked_key(const struct sw_flow *flow,
static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
const struct sw_flow_key *unmasked,
- const struct sw_flow_mask *mask)
+ const struct sw_flow_mask *mask,
+ u32 *n_mask_hit)
{
struct sw_flow *flow;
struct hlist_head *head;
@@ -435,6 +448,8 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
ovs_flow_mask_key(&masked_key, unmasked, false, mask);
hash = flow_hash(&masked_key, &mask->range);
head = find_bucket(ti, hash);
+ (*n_mask_hit)++;
+
hlist_for_each_entry_rcu(flow, head, flow_table.node[ti->node_ver]) {
if (flow->mask == mask && flow->flow_table.hash == hash &&
flow_cmp_masked_key(flow, &masked_key, &mask->range))
@@ -443,30 +458,97 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
return NULL;
}
-struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
- const struct sw_flow_key *key,
- u32 *n_mask_hit)
+static struct sw_flow *flow_lookup(struct flow_table *tbl,
+ struct table_instance *ti,
+ const struct sw_flow_key *key,
+ u32 *n_mask_hit)
{
- struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
struct sw_flow_mask *mask;
struct sw_flow *flow;
- *n_mask_hit = 0;
list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
- (*n_mask_hit)++;
- flow = masked_flow_lookup(ti, key, mask);
+ flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
if (flow) /* Found */
return flow;
}
return NULL;
}
+/*
+ * mask_cache maps flow to probable mask. This cache is not tightly
+ * coupled cache, It means updates to mask list can result in inconsistent
+ * cache entry in mask cache.
+ * This is per cpu cache and is divided in MC_HASH_SEGS segments.
+ * In case of a hash collision the entry is hashed in next segment.
+ * */
+struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
+ const struct sw_flow_key *key,
+ u32 skb_hash,
+ u32 *n_mask_hit)
+{
+ struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
+ struct mask_cache_entry *entries, *ce, *del;
+ struct sw_flow *flow;
+ u32 hash = skb_hash;
+ int seg;
+
+ *n_mask_hit = 0;
+ if (unlikely(!skb_hash))
+ return flow_lookup(tbl, ti, key, n_mask_hit);
+
+ del = NULL;
+ entries = this_cpu_ptr(tbl->mask_cache);
+
+ for (seg = 0; seg < MC_HASH_SEGS; seg++) {
+ int index;
+
+ index = hash & (MC_HASH_ENTRIES - 1);
+ ce = &entries[index];
+
+ if (ce->skb_hash == skb_hash) {
+ struct sw_flow_mask *mask;
+ int i;
+
+ i = 0;
+ list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
+ if (ce->mask_index == i++) {
+ flow = masked_flow_lookup(ti, key, mask,
+ n_mask_hit);
+ if (flow) /* Found */
+ return flow;
+
+ break;
+ }
+ }
+
+ del = ce;
+ break;
+ }
+
+ if (!del || (del->skb_hash && !ce->skb_hash)) {
+ del = ce;
+ }
+
+ hash >>= MC_HASH_SHIFT;
+ }
+
+ flow = flow_lookup(tbl, ti, key, n_mask_hit);
+
+ if (flow) {
+ del->skb_hash = skb_hash;
+ del->mask_index = (*n_mask_hit - 1);
+ }
+
+ return flow;
+}
+
struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *tbl,
const struct sw_flow_key *key)
{
+ struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
u32 __always_unused n_mask_hit;
- return ovs_flow_tbl_lookup_stats(tbl, key, &n_mask_hit);
+ return flow_lookup(tbl, ti, key, &n_mask_hit);
}
struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
@@ -475,10 +557,11 @@ struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
struct sw_flow_mask *mask;
struct sw_flow *flow;
+ u32 __always_unused n_mask_hit;
/* Always called under ovs-mutex. */
list_for_each_entry(mask, &tbl->mask_list, list) {
- flow = masked_flow_lookup(ti, match->key, mask);
+ flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
if (flow && ovs_identifier_is_key(&flow->id) &&
ovs_flow_cmp_unmasked_key(flow, match))
return flow;
@@ -631,7 +714,7 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
return -ENOMEM;
mask->key = new->key;
mask->range = new->range;
- list_add_rcu(&mask->list, &tbl->mask_list);
+ list_add_tail_rcu(&mask->list, &tbl->mask_list);
} else {
BUG_ON(!mask->ref_count);
mask->ref_count++;
diff --git a/net/openvswitch/flow_table.h b/net/openvswitch/flow_table.h
index bc52045..04b6b1c 100644
--- a/net/openvswitch/flow_table.h
+++ b/net/openvswitch/flow_table.h
@@ -22,6 +22,11 @@
#include "flow.h"
+struct mask_cache_entry {
+ u32 skb_hash;
+ u32 mask_index;
+};
+
struct table_instance {
struct hlist_head *buckets;
unsigned int n_buckets;
@@ -34,6 +39,7 @@ struct table_instance {
struct flow_table {
struct table_instance __rcu *ti;
struct table_instance __rcu *ufid_ti;
+ struct mask_cache_entry __percpu *mask_cache;
struct list_head mask_list;
unsigned long last_rehash;
unsigned int count;
@@ -60,8 +66,9 @@ int ovs_flow_tbl_insert(struct flow_table *table, struct sw_flow *flow,
struct sw_flow *ovs_flow_tbl_dump_next(struct table_instance *table,
u32 *bucket, u32 *idx);
struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *,
- const struct sw_flow_key *,
- u32 *n_mask_hit);
+ const struct sw_flow_key *,
+ u32 skb_hash,
+ u32 *n_mask_hit);
struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *,
const struct sw_flow_key *);
struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 2/7] net: openvswitch: convert mask list in mask array
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Port the codes to linux upstream and with little changes.
Pravin B Shelar, says:
| mask caches index of mask in mask_list. On packet recv OVS
| need to traverse mask-list to get cached mask. Therefore array
| is better for retrieving cached mask. This also allows better
| cache replacement algorithm by directly checking mask's existence.
Link: https://github.com/openvswitch/ovs/commit/d49fc3ff53c65e4eca9cabd52ac63396746a7ef5
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/flow.h | 1 -
net/openvswitch/flow_table.c | 218 +++++++++++++++++++++++++++++++++----------
net/openvswitch/flow_table.h | 8 +-
3 files changed, 175 insertions(+), 52 deletions(-)
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index b830d5f..8080518 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -166,7 +166,6 @@ struct sw_flow_key_range {
struct sw_flow_mask {
int ref_count;
struct rcu_head rcu;
- struct list_head list;
struct sw_flow_key_range range;
struct sw_flow_key key;
};
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 3d515c0..99954fa 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -34,6 +34,7 @@
#include <net/ndisc.h>
#define TBL_MIN_BUCKETS 1024
+#define MASK_ARRAY_SIZE_MIN 16
#define REHASH_INTERVAL (10 * 60 * HZ)
#define MC_HASH_SHIFT 8
@@ -168,9 +169,59 @@ static struct table_instance *table_instance_alloc(int new_size)
return ti;
}
+static void mask_array_rcu_cb(struct rcu_head *rcu)
+{
+ struct mask_array *ma = container_of(rcu, struct mask_array, rcu);
+
+ kfree(ma);
+}
+
+static struct mask_array *tbl_mask_array_alloc(int size)
+{
+ struct mask_array *new;
+
+ size = max(MASK_ARRAY_SIZE_MIN, size);
+ new = kzalloc(sizeof(struct mask_array) +
+ sizeof(struct sw_flow_mask *) * size, GFP_KERNEL);
+ if (!new)
+ return NULL;
+
+ new->count = 0;
+ new->max = size;
+
+ return new;
+}
+
+static int tbl_mask_array_realloc(struct flow_table *tbl, int size)
+{
+ struct mask_array *old;
+ struct mask_array *new;
+
+ new = tbl_mask_array_alloc(size);
+ if (!new)
+ return -ENOMEM;
+
+ old = ovsl_dereference(tbl->mask_array);
+ if (old) {
+ int i;
+
+ for (i = 0; i < old->max; i++) {
+ if (ovsl_dereference(old->masks[i]))
+ new->masks[new->count++] = old->masks[i];
+ }
+ }
+ rcu_assign_pointer(tbl->mask_array, new);
+
+ if (old)
+ call_rcu(&old->rcu, mask_array_rcu_cb);
+
+ return 0;
+}
+
int ovs_flow_tbl_init(struct flow_table *table)
{
struct table_instance *ti, *ufid_ti;
+ struct mask_array *ma;
table->mask_cache = __alloc_percpu(sizeof(struct mask_cache_entry) *
MC_HASH_ENTRIES,
@@ -178,9 +229,13 @@ int ovs_flow_tbl_init(struct flow_table *table)
if (!table->mask_cache)
return -ENOMEM;
+ ma = tbl_mask_array_alloc(MASK_ARRAY_SIZE_MIN);
+ if (!ma)
+ goto free_mask_cache;
+
ti = table_instance_alloc(TBL_MIN_BUCKETS);
if (!ti)
- goto free_mask_cache;
+ goto free_mask_array;
ufid_ti = table_instance_alloc(TBL_MIN_BUCKETS);
if (!ufid_ti)
@@ -188,7 +243,7 @@ int ovs_flow_tbl_init(struct flow_table *table)
rcu_assign_pointer(table->ti, ti);
rcu_assign_pointer(table->ufid_ti, ufid_ti);
- INIT_LIST_HEAD(&table->mask_list);
+ rcu_assign_pointer(table->mask_array, ma);
table->last_rehash = jiffies;
table->count = 0;
table->ufid_count = 0;
@@ -196,6 +251,8 @@ int ovs_flow_tbl_init(struct flow_table *table)
free_ti:
__table_instance_destroy(ti);
+free_mask_array:
+ kfree(ma);
free_mask_cache:
free_percpu(table->mask_cache);
return -ENOMEM;
@@ -255,6 +312,7 @@ void ovs_flow_tbl_destroy(struct flow_table *table)
struct table_instance *ufid_ti = rcu_dereference_raw(table->ufid_ti);
free_percpu(table->mask_cache);
+ kfree(rcu_dereference_raw(table->mask_array));
table_instance_destroy(ti, ufid_ti, false);
}
@@ -460,17 +518,27 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
static struct sw_flow *flow_lookup(struct flow_table *tbl,
struct table_instance *ti,
+ struct mask_array *ma,
const struct sw_flow_key *key,
- u32 *n_mask_hit)
+ u32 *n_mask_hit,
+ u32 *index)
{
- struct sw_flow_mask *mask;
struct sw_flow *flow;
+ int i;
- list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
- flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
- if (flow) /* Found */
- return flow;
+ for (i = 0; i < ma->max; i++) {
+ struct sw_flow_mask *mask;
+
+ mask = rcu_dereference_ovsl(ma->masks[i]);
+ if (mask) {
+ flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
+ if (flow) { /* Found */
+ *index = i;
+ return flow;
+ }
+ }
}
+
return NULL;
}
@@ -486,6 +554,7 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
u32 skb_hash,
u32 *n_mask_hit)
{
+ struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
struct mask_cache_entry *entries, *ce, *del;
struct sw_flow *flow;
@@ -493,8 +562,11 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
int seg;
*n_mask_hit = 0;
- if (unlikely(!skb_hash))
- return flow_lookup(tbl, ti, key, n_mask_hit);
+ if (unlikely(!skb_hash)) {
+ u32 __always_unused mask_index;
+
+ return flow_lookup(tbl, ti, ma, key, n_mask_hit, &mask_index);
+ }
del = NULL;
entries = this_cpu_ptr(tbl->mask_cache);
@@ -507,37 +579,33 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
if (ce->skb_hash == skb_hash) {
struct sw_flow_mask *mask;
- int i;
-
- i = 0;
- list_for_each_entry_rcu(mask, &tbl->mask_list, list) {
- if (ce->mask_index == i++) {
- flow = masked_flow_lookup(ti, key, mask,
- n_mask_hit);
- if (flow) /* Found */
- return flow;
-
- break;
- }
+ struct sw_flow *flow;
+
+ mask = rcu_dereference_ovsl(ma->masks[ce->mask_index]);
+ if (mask) {
+ flow = masked_flow_lookup(ti, key, mask,
+ n_mask_hit);
+ if (flow) /* Found */
+ return flow;
}
del = ce;
break;
}
- if (!del || (del->skb_hash && !ce->skb_hash)) {
+ if (!del || (del->skb_hash && !ce->skb_hash) ||
+ (rcu_dereference_ovsl(ma->masks[del->mask_index]) &&
+ !rcu_dereference_ovsl(ma->masks[ce->mask_index]))) {
del = ce;
}
hash >>= MC_HASH_SHIFT;
}
- flow = flow_lookup(tbl, ti, key, n_mask_hit);
+ flow = flow_lookup(tbl, ti, ma, key, n_mask_hit, &del->mask_index);
- if (flow) {
+ if (flow)
del->skb_hash = skb_hash;
- del->mask_index = (*n_mask_hit - 1);
- }
return flow;
}
@@ -546,26 +614,38 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *tbl,
const struct sw_flow_key *key)
{
struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
+ struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
+
u32 __always_unused n_mask_hit;
+ u32 __always_unused index;
- return flow_lookup(tbl, ti, key, &n_mask_hit);
+ return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &index);
}
struct sw_flow *ovs_flow_tbl_lookup_exact(struct flow_table *tbl,
const struct sw_flow_match *match)
{
- struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
- struct sw_flow_mask *mask;
- struct sw_flow *flow;
- u32 __always_unused n_mask_hit;
+ struct mask_array *ma = ovsl_dereference(tbl->mask_array);
+ int i;
/* Always called under ovs-mutex. */
- list_for_each_entry(mask, &tbl->mask_list, list) {
+ for (i = 0; i < ma->max; i++) {
+ struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
+ u32 __always_unused n_mask_hit;
+ struct sw_flow_mask *mask;
+ struct sw_flow *flow;
+
+ mask = ovsl_dereference(ma->masks[i]);
+ if (!mask)
+ continue;
+
flow = masked_flow_lookup(ti, match->key, mask, &n_mask_hit);
if (flow && ovs_identifier_is_key(&flow->id) &&
- ovs_flow_cmp_unmasked_key(flow, match))
+ ovs_flow_cmp_unmasked_key(flow, match)) {
return flow;
+ }
}
+
return NULL;
}
@@ -611,13 +691,8 @@ struct sw_flow *ovs_flow_tbl_lookup_ufid(struct flow_table *tbl,
int ovs_flow_tbl_num_masks(const struct flow_table *table)
{
- struct sw_flow_mask *mask;
- int num = 0;
-
- list_for_each_entry(mask, &table->mask_list, list)
- num++;
-
- return num;
+ struct mask_array *ma = rcu_dereference_ovsl(table->mask_array);
+ return ma->count;
}
static struct table_instance *table_instance_expand(struct table_instance *ti,
@@ -638,8 +713,19 @@ static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
mask->ref_count--;
if (!mask->ref_count) {
- list_del_rcu(&mask->list);
- kfree_rcu(mask, rcu);
+ struct mask_array *ma;
+ int i;
+
+ ma = ovsl_dereference(tbl->mask_array);
+ for (i = 0; i < ma->max; i++) {
+ if (mask == ovsl_dereference(ma->masks[i])) {
+ RCU_INIT_POINTER(ma->masks[i], NULL);
+ ma->count--;
+ kfree_rcu(mask, rcu);
+ return;
+ }
+ }
+ BUG();
}
}
}
@@ -689,13 +775,16 @@ static bool mask_equal(const struct sw_flow_mask *a,
static struct sw_flow_mask *flow_mask_find(const struct flow_table *tbl,
const struct sw_flow_mask *mask)
{
- struct list_head *ml;
+ struct mask_array *ma;
+ int i;
- list_for_each(ml, &tbl->mask_list) {
- struct sw_flow_mask *m;
- m = container_of(ml, struct sw_flow_mask, list);
- if (mask_equal(mask, m))
- return m;
+ ma = ovsl_dereference(tbl->mask_array);
+ for (i = 0; i < ma->max; i++) {
+ struct sw_flow_mask *t;
+ t = ovsl_dereference(ma->masks[i]);
+
+ if (t && mask_equal(mask, t))
+ return t;
}
return NULL;
@@ -706,15 +795,44 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
const struct sw_flow_mask *new)
{
struct sw_flow_mask *mask;
+
mask = flow_mask_find(tbl, new);
if (!mask) {
+ struct mask_array *ma;
+ int i;
+
/* Allocate a new mask if none exsits. */
mask = mask_alloc();
if (!mask)
return -ENOMEM;
mask->key = new->key;
mask->range = new->range;
- list_add_tail_rcu(&mask->list, &tbl->mask_list);
+
+ /* Add mask to mask-list. */
+ ma = ovsl_dereference(tbl->mask_array);
+ if (ma->count >= ma->max) {
+ int err;
+
+ err = tbl_mask_array_realloc(tbl, ma->max +
+ MASK_ARRAY_SIZE_MIN);
+ if (err) {
+ kfree(mask);
+ return err;
+ }
+
+ ma = ovsl_dereference(tbl->mask_array);
+ }
+
+ for (i = 0; i < ma->max; i++) {
+ const struct sw_flow_mask *t;
+
+ t = ovsl_dereference(ma->masks[i]);
+ if (!t) {
+ rcu_assign_pointer(ma->masks[i], mask);
+ ma->count++;
+ break;
+ }
+ }
} else {
BUG_ON(!mask->ref_count);
mask->ref_count++;
diff --git a/net/openvswitch/flow_table.h b/net/openvswitch/flow_table.h
index 04b6b1c..8a5cea6 100644
--- a/net/openvswitch/flow_table.h
+++ b/net/openvswitch/flow_table.h
@@ -27,6 +27,12 @@ struct mask_cache_entry {
u32 mask_index;
};
+struct mask_array {
+ struct rcu_head rcu;
+ int count, max;
+ struct sw_flow_mask __rcu *masks[];
+};
+
struct table_instance {
struct hlist_head *buckets;
unsigned int n_buckets;
@@ -40,7 +46,7 @@ struct flow_table {
struct table_instance __rcu *ti;
struct table_instance __rcu *ufid_ti;
struct mask_cache_entry __percpu *mask_cache;
- struct list_head mask_list;
+ struct mask_array __rcu *mask_array;
unsigned long last_rehash;
unsigned int count;
unsigned int ufid_count;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 3/7] net: openvswitch: shrink the mask array if necessary
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
When creating and inserting flow-mask, if there is no available
flow-mask, we realloc the mask array. When removing flow-mask,
if necessary, we shrink mask array.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/flow_table.c | 33 +++++++++++++++++++++++----------
1 file changed, 23 insertions(+), 10 deletions(-)
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 99954fa..9c72aab 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -701,6 +701,23 @@ static struct table_instance *table_instance_expand(struct table_instance *ti,
return table_instance_rehash(ti, ti->n_buckets * 2, ufid);
}
+static void tbl_mask_array_delete_mask(struct mask_array *ma,
+ struct sw_flow_mask *mask)
+{
+ int i;
+
+ /* Remove the deleted mask pointers from the array */
+ for (i = 0; i < ma->max; i++) {
+ if (mask == ovsl_dereference(ma->masks[i])) {
+ RCU_INIT_POINTER(ma->masks[i], NULL);
+ ma->count--;
+ kfree_rcu(mask, rcu);
+ return;
+ }
+ }
+ BUG();
+}
+
/* Remove 'mask' from the mask list, if it is not needed any more. */
static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
{
@@ -714,18 +731,14 @@ static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
if (!mask->ref_count) {
struct mask_array *ma;
- int i;
ma = ovsl_dereference(tbl->mask_array);
- for (i = 0; i < ma->max; i++) {
- if (mask == ovsl_dereference(ma->masks[i])) {
- RCU_INIT_POINTER(ma->masks[i], NULL);
- ma->count--;
- kfree_rcu(mask, rcu);
- return;
- }
- }
- BUG();
+ tbl_mask_array_delete_mask(ma, mask);
+
+ /* Shrink the mask array if necessary. */
+ if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
+ ma->count <= (ma->max / 3))
+ tbl_mask_array_realloc(tbl, ma->max / 2);
}
}
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 4/7] net: openvswitch: optimize flow mask cache hash collision
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Port the codes to linux upstream and with little changes.
Pravin B Shelar, says:
| In case hash collision on mask cache, OVS does extra flow
| lookup. Following patch avoid it.
Link: https://github.com/openvswitch/ovs/commit/0e6efbe2712da03522532dc5e84806a96f6a0dd1
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/flow_table.c | 95 ++++++++++++++++++++++++--------------------
1 file changed, 53 insertions(+), 42 deletions(-)
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 9c72aab..e59fac5 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -516,6 +516,9 @@ static struct sw_flow *masked_flow_lookup(struct table_instance *ti,
return NULL;
}
+/* Flow lookup does full lookup on flow table. It starts with
+ * mask from index passed in *index.
+ */
static struct sw_flow *flow_lookup(struct flow_table *tbl,
struct table_instance *ti,
struct mask_array *ma,
@@ -524,18 +527,31 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
u32 *index)
{
struct sw_flow *flow;
+ struct sw_flow_mask *mask;
int i;
- for (i = 0; i < ma->max; i++) {
- struct sw_flow_mask *mask;
-
- mask = rcu_dereference_ovsl(ma->masks[i]);
+ if (*index < ma->max) {
+ mask = rcu_dereference_ovsl(ma->masks[*index]);
if (mask) {
flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
- if (flow) { /* Found */
- *index = i;
+ if (flow)
return flow;
- }
+ }
+ }
+
+ for (i = 0; i < ma->max; i++) {
+
+ if (i == *index)
+ continue;
+
+ mask = rcu_dereference_ovsl(ma->masks[i]);
+ if (!mask)
+ continue;
+
+ flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
+ if (flow) { /* Found */
+ *index = i;
+ return flow;
}
}
@@ -554,58 +570,54 @@ struct sw_flow *ovs_flow_tbl_lookup_stats(struct flow_table *tbl,
u32 skb_hash,
u32 *n_mask_hit)
{
- struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
- struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
- struct mask_cache_entry *entries, *ce, *del;
+ struct mask_array *ma = rcu_dereference(tbl->mask_array);
+ struct table_instance *ti = rcu_dereference(tbl->ti);
+ struct mask_cache_entry *entries, *ce;
struct sw_flow *flow;
- u32 hash = skb_hash;
+ u32 hash;
int seg;
*n_mask_hit = 0;
if (unlikely(!skb_hash)) {
- u32 __always_unused mask_index;
+ u32 mask_index = 0;
return flow_lookup(tbl, ti, ma, key, n_mask_hit, &mask_index);
}
- del = NULL;
+ /* Pre and post recirulation flows usually have the same skb_hash
+ * value. To avoid hash collisions, rehash the 'skb_hash' with
+ * 'recirc_id'. */
+ if (key->recirc_id)
+ skb_hash = jhash_1word(skb_hash, key->recirc_id);
+
+ ce = NULL;
+ hash = skb_hash;
entries = this_cpu_ptr(tbl->mask_cache);
+ /* Find the cache entry 'ce' to operate on. */
for (seg = 0; seg < MC_HASH_SEGS; seg++) {
- int index;
-
- index = hash & (MC_HASH_ENTRIES - 1);
- ce = &entries[index];
-
- if (ce->skb_hash == skb_hash) {
- struct sw_flow_mask *mask;
- struct sw_flow *flow;
-
- mask = rcu_dereference_ovsl(ma->masks[ce->mask_index]);
- if (mask) {
- flow = masked_flow_lookup(ti, key, mask,
- n_mask_hit);
- if (flow) /* Found */
- return flow;
- }
-
- del = ce;
- break;
+ int index = hash & (MC_HASH_ENTRIES - 1);
+ struct mask_cache_entry *e;
+
+ e = &entries[index];
+ if (e->skb_hash == skb_hash) {
+ flow = flow_lookup(tbl, ti, ma, key, n_mask_hit,
+ &e->mask_index);
+ if (!flow)
+ e->skb_hash = 0;
+ return flow;
}
- if (!del || (del->skb_hash && !ce->skb_hash) ||
- (rcu_dereference_ovsl(ma->masks[del->mask_index]) &&
- !rcu_dereference_ovsl(ma->masks[ce->mask_index]))) {
- del = ce;
- }
+ if (!ce || e->skb_hash < ce->skb_hash)
+ ce = e; /* A better replacement cache candidate. */
hash >>= MC_HASH_SHIFT;
}
- flow = flow_lookup(tbl, ti, ma, key, n_mask_hit, &del->mask_index);
-
+ /* Cache miss, do full lookup. */
+ flow = flow_lookup(tbl, ti, ma, key, n_mask_hit, &ce->mask_index);
if (flow)
- del->skb_hash = skb_hash;
+ ce->skb_hash = skb_hash;
return flow;
}
@@ -615,9 +627,8 @@ struct sw_flow *ovs_flow_tbl_lookup(struct flow_table *tbl,
{
struct table_instance *ti = rcu_dereference_ovsl(tbl->ti);
struct mask_array *ma = rcu_dereference_ovsl(tbl->mask_array);
-
u32 __always_unused n_mask_hit;
- u32 __always_unused index;
+ u32 index = 0;
return flow_lookup(tbl, ti, ma, key, &n_mask_hit, &index);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 5/7] net: openvswitch: optimize flow-mask looking up
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
The full looking up on flow table traverses all mask array.
If mask-array is too large, the number of invalid flow-mask
increase, performance will be drop.
This patch optimizes mask-array operation:
* Inserting, insert it [ma->count- 1] directly.
* Removing, only change last and current mask point, and free current mask.
* Looking up, full looking up will break if mask is NULL.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/flow_table.c | 93 ++++++++++++++++++++++----------------------
1 file changed, 46 insertions(+), 47 deletions(-)
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index e59fac5..2d74d74 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -546,7 +546,7 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
mask = rcu_dereference_ovsl(ma->masks[i]);
if (!mask)
- continue;
+ break;
flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
if (flow) { /* Found */
@@ -712,21 +712,31 @@ static struct table_instance *table_instance_expand(struct table_instance *ti,
return table_instance_rehash(ti, ti->n_buckets * 2, ufid);
}
-static void tbl_mask_array_delete_mask(struct mask_array *ma,
- struct sw_flow_mask *mask)
+static void tbl_mask_array_del_mask(struct flow_table *tbl,
+ struct sw_flow_mask *mask)
{
+ struct mask_array *ma = ovsl_dereference(tbl->mask_array);
int i;
/* Remove the deleted mask pointers from the array */
- for (i = 0; i < ma->max; i++) {
- if (mask == ovsl_dereference(ma->masks[i])) {
- RCU_INIT_POINTER(ma->masks[i], NULL);
- ma->count--;
- kfree_rcu(mask, rcu);
- return;
- }
+ for (i = 0; i < ma->count; i++) {
+ if (mask == ovsl_dereference(ma->masks[i]))
+ goto found;
}
+
BUG();
+ return;
+
+found:
+ rcu_assign_pointer(ma->masks[i], ma->masks[ma->count -1]);
+ RCU_INIT_POINTER(ma->masks[ma->count -1], NULL);
+ ma->count--;
+ kfree_rcu(mask, rcu);
+
+ /* Shrink the mask array if necessary. */
+ if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
+ ma->count <= (ma->max / 3))
+ tbl_mask_array_realloc(tbl, ma->max / 2);
}
/* Remove 'mask' from the mask list, if it is not needed any more. */
@@ -740,17 +750,8 @@ static void flow_mask_remove(struct flow_table *tbl, struct sw_flow_mask *mask)
BUG_ON(!mask->ref_count);
mask->ref_count--;
- if (!mask->ref_count) {
- struct mask_array *ma;
-
- ma = ovsl_dereference(tbl->mask_array);
- tbl_mask_array_delete_mask(ma, mask);
-
- /* Shrink the mask array if necessary. */
- if (ma->max >= (MASK_ARRAY_SIZE_MIN * 2) &&
- ma->count <= (ma->max / 3))
- tbl_mask_array_realloc(tbl, ma->max / 2);
- }
+ if (!mask->ref_count)
+ tbl_mask_array_del_mask(tbl, mask);
}
}
@@ -814,6 +815,27 @@ static struct sw_flow_mask *flow_mask_find(const struct flow_table *tbl,
return NULL;
}
+static int tbl_mask_array_add_mask(struct flow_table *tbl,
+ struct sw_flow_mask *new)
+{
+ struct mask_array *ma = ovsl_dereference(tbl->mask_array);
+ int err;
+
+ if (ma->count >= ma->max) {
+ err = tbl_mask_array_realloc(tbl, ma->max +
+ MASK_ARRAY_SIZE_MIN);
+ if (err)
+ return err;
+ }
+
+ BUG_ON(ovsl_dereference(ma->masks[ma->count]));
+
+ rcu_assign_pointer(ma->masks[ma->count], new);
+ ma->count++;
+
+ return 0;
+}
+
/* Add 'mask' into the mask list, if it is not already there. */
static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
const struct sw_flow_mask *new)
@@ -822,9 +844,6 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
mask = flow_mask_find(tbl, new);
if (!mask) {
- struct mask_array *ma;
- int i;
-
/* Allocate a new mask if none exsits. */
mask = mask_alloc();
if (!mask)
@@ -833,29 +852,9 @@ static int flow_mask_insert(struct flow_table *tbl, struct sw_flow *flow,
mask->range = new->range;
/* Add mask to mask-list. */
- ma = ovsl_dereference(tbl->mask_array);
- if (ma->count >= ma->max) {
- int err;
-
- err = tbl_mask_array_realloc(tbl, ma->max +
- MASK_ARRAY_SIZE_MIN);
- if (err) {
- kfree(mask);
- return err;
- }
-
- ma = ovsl_dereference(tbl->mask_array);
- }
-
- for (i = 0; i < ma->max; i++) {
- const struct sw_flow_mask *t;
-
- t = ovsl_dereference(ma->masks[i]);
- if (!t) {
- rcu_assign_pointer(ma->masks[i], mask);
- ma->count++;
- break;
- }
+ if (tbl_mask_array_add_mask(tbl, mask)) {
+ kfree(mask);
+ return -ENOMEM;
}
} else {
BUG_ON(!mask->ref_count);
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 6/7] net: openvswitch: simplify the flow_hash
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
Simplify the code and remove the unnecessary BUILD_BUG_ON.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/flow_table.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 2d74d74..223470a 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -440,13 +440,9 @@ int ovs_flow_tbl_flush(struct flow_table *flow_table)
static u32 flow_hash(const struct sw_flow_key *key,
const struct sw_flow_key_range *range)
{
- int key_start = range->start;
- int key_end = range->end;
- const u32 *hash_key = (const u32 *)((const u8 *)key + key_start);
- int hash_u32s = (key_end - key_start) >> 2;
-
+ const u32 *hash_key = (const u32 *)((const u8 *)key + range->start);
/* Make sure number of hash bytes are multiple of u32. */
- BUILD_BUG_ON(sizeof(long) % sizeof(u32));
+ int hash_u32s = range_n_bytes(range) >> 2;
return jhash2(hash_key, hash_u32s, 0);
}
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next 7/7] net: openvswitch: add likely in flow_lookup
From: xiangxia.m.yue @ 2019-09-19 16:54 UTC (permalink / raw)
To: pshelar, gvrose8192; +Cc: netdev, Tonghao Zhang
In-Reply-To: <1568912093-68535-1-git-send-email-xiangxia.m.yue@gmail.com>
From: Tonghao Zhang <xiangxia.m.yue@gmail.com>
The most case *index < ma->max, we add likely for performance.
Signed-off-by: Tonghao Zhang <xiangxia.m.yue@gmail.com>
---
net/openvswitch/flow_table.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/openvswitch/flow_table.c b/net/openvswitch/flow_table.c
index 223470a..bd7e976 100644
--- a/net/openvswitch/flow_table.c
+++ b/net/openvswitch/flow_table.c
@@ -526,7 +526,7 @@ static struct sw_flow *flow_lookup(struct flow_table *tbl,
struct sw_flow_mask *mask;
int i;
- if (*index < ma->max) {
+ if (likely(*index < ma->max)) {
mask = rcu_dereference_ovsl(ma->masks[*index]);
if (mask) {
flow = masked_flow_lookup(ti, key, mask, n_mask_hit);
--
1.8.3.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox