* RE: [PATCH net-next 04/14] vsock: add 'transport' member in the struct vsock_sock
From: Jorgen Hansen @ 2019-10-30 14:57 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-5-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 04/14] vsock: add 'transport' member in the struct
> vsock_sock
>
> As a preparation to support multiple transports, this patch adds the
> 'transport' member at the 'struct vsock_sock'.
> This new field is initialized during the creation in the
> __vsock_create() function.
>
> This patch also renames the global 'transport' pointer to 'transport_single',
> since for now we're only supporting a single transport registered at run-time.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/net/af_vsock.h | 1 +
> net/vmw_vsock/af_vsock.c | 56 +++++++++++++++++++++++++++----------
> ---
> 2 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index
> c660402b10f2..a5e1e134261d 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -27,6 +27,7 @@ extern spinlock_t vsock_table_lock; struct vsock_sock {
> /* sk must be the first member. */
> struct sock sk;
> + const struct vsock_transport *transport;
> struct sockaddr_vm local_addr;
> struct sockaddr_vm remote_addr;
> /* Links for the global tables of bound and connected sockets. */ diff
> --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> 2f2582fb7fdd..c3a14f853eb0 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -126,7 +126,7 @@ static struct proto vsock_proto = {
> */
> #define VSOCK_DEFAULT_CONNECT_TIMEOUT (2 * HZ)
>
> -static const struct vsock_transport *transport;
> +static const struct vsock_transport *transport_single;
> static DEFINE_MUTEX(vsock_register_mutex);
>
> /**** UTILS ****/
> @@ -408,7 +408,9 @@ static bool vsock_is_pending(struct sock *sk)
>
> static int vsock_send_shutdown(struct sock *sk, int mode) {
> - return transport->shutdown(vsock_sk(sk), mode);
> + struct vsock_sock *vsk = vsock_sk(sk);
> +
> + return vsk->transport->shutdown(vsk, mode);
> }
>
> static void vsock_pending_work(struct work_struct *work) @@ -518,7
> +520,7 @@ static int __vsock_bind_stream(struct vsock_sock *vsk, static int
> __vsock_bind_dgram(struct vsock_sock *vsk,
> struct sockaddr_vm *addr)
> {
> - return transport->dgram_bind(vsk, addr);
> + return vsk->transport->dgram_bind(vsk, addr);
> }
>
> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr) @@ -
> 536,7 +538,7 @@ static int __vsock_bind(struct sock *sk, struct sockaddr_vm
> *addr)
> * like AF_INET prevents binding to a non-local IP address (in most
> * cases), we only allow binding to the local CID.
> */
> - cid = transport->get_local_cid();
> + cid = vsk->transport->get_local_cid();
> if (addr->svm_cid != cid && addr->svm_cid != VMADDR_CID_ANY)
> return -EADDRNOTAVAIL;
>
> @@ -586,6 +588,7 @@ struct sock *__vsock_create(struct net *net,
> sk->sk_type = type;
>
> vsk = vsock_sk(sk);
> + vsk->transport = transport_single;
> vsock_addr_init(&vsk->local_addr, VMADDR_CID_ANY,
> VMADDR_PORT_ANY);
> vsock_addr_init(&vsk->remote_addr, VMADDR_CID_ANY,
> VMADDR_PORT_ANY);
>
> @@ -616,7 +619,7 @@ struct sock *__vsock_create(struct net *net,
> vsk->connect_timeout =
> VSOCK_DEFAULT_CONNECT_TIMEOUT;
> }
>
> - if (transport->init(vsk, psk) < 0) {
> + if (vsk->transport->init(vsk, psk) < 0) {
> sk_free(sk);
> return NULL;
> }
> @@ -641,7 +644,7 @@ static void __vsock_release(struct sock *sk, int level)
> /* The release call is supposed to use lock_sock_nested()
> * rather than lock_sock(), if a sock lock should be acquired.
> */
> - transport->release(vsk);
> + vsk->transport->release(vsk);
>
> /* When "level" is SINGLE_DEPTH_NESTING, use the nested
> * version to avoid the warning "possible recursive locking
> @@ -670,7 +673,7 @@ static void vsock_sk_destruct(struct sock *sk) {
> struct vsock_sock *vsk = vsock_sk(sk);
>
> - transport->destruct(vsk);
> + vsk->transport->destruct(vsk);
>
> /* When clearing these addresses, there's no need to set the family
> and
> * possibly register the address family with the kernel.
> @@ -694,13 +697,13 @@ static int vsock_queue_rcv_skb(struct sock *sk,
> struct sk_buff *skb)
>
> s64 vsock_stream_has_data(struct vsock_sock *vsk) {
> - return transport->stream_has_data(vsk);
> + return vsk->transport->stream_has_data(vsk);
> }
> EXPORT_SYMBOL_GPL(vsock_stream_has_data);
>
> s64 vsock_stream_has_space(struct vsock_sock *vsk) {
> - return transport->stream_has_space(vsk);
> + return vsk->transport->stream_has_space(vsk);
> }
> EXPORT_SYMBOL_GPL(vsock_stream_has_space);
>
> @@ -869,6 +872,7 @@ static __poll_t vsock_poll(struct file *file, struct
> socket *sock,
> mask |= EPOLLOUT | EPOLLWRNORM |
> EPOLLWRBAND;
>
> } else if (sock->type == SOCK_STREAM) {
> + const struct vsock_transport *transport = vsk->transport;
> lock_sock(sk);
>
> /* Listening sockets that have connections in their accept
> @@ -944,6 +948,7 @@ static int vsock_dgram_sendmsg(struct socket *sock,
> struct msghdr *msg,
> struct sock *sk;
> struct vsock_sock *vsk;
> struct sockaddr_vm *remote_addr;
> + const struct vsock_transport *transport;
>
> if (msg->msg_flags & MSG_OOB)
> return -EOPNOTSUPP;
> @@ -952,6 +957,7 @@ static int vsock_dgram_sendmsg(struct socket *sock,
> struct msghdr *msg,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> lock_sock(sk);
>
> @@ -1036,8 +1042,8 @@ static int vsock_dgram_connect(struct socket
> *sock,
> if (err)
> goto out;
>
> - if (!transport->dgram_allow(remote_addr->svm_cid,
> - remote_addr->svm_port)) {
> + if (!vsk->transport->dgram_allow(remote_addr->svm_cid,
> + remote_addr->svm_port)) {
> err = -EINVAL;
> goto out;
> }
> @@ -1053,7 +1059,9 @@ static int vsock_dgram_connect(struct socket
> *sock, static int vsock_dgram_recvmsg(struct socket *sock, struct msghdr
> *msg,
> size_t len, int flags)
> {
> - return transport->dgram_dequeue(vsock_sk(sock->sk), msg, len,
> flags);
> + struct vsock_sock *vsk = vsock_sk(sock->sk);
> +
> + return vsk->transport->dgram_dequeue(vsk, msg, len, flags);
> }
>
> static const struct proto_ops vsock_dgram_ops = { @@ -1079,6 +1087,8 @@
> static const struct proto_ops vsock_dgram_ops = {
>
> static int vsock_transport_cancel_pkt(struct vsock_sock *vsk) {
> + const struct vsock_transport *transport = vsk->transport;
> +
> if (!transport->cancel_pkt)
> return -EOPNOTSUPP;
>
> @@ -1115,6 +1125,7 @@ static int vsock_stream_connect(struct socket
> *sock, struct sockaddr *addr,
> int err;
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> struct sockaddr_vm *remote_addr;
> long timeout;
> DEFINE_WAIT(wait);
> @@ -1122,6 +1133,7 @@ static int vsock_stream_connect(struct socket
> *sock, struct sockaddr *addr,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> lock_sock(sk);
>
> @@ -1365,6 +1377,7 @@ static int vsock_stream_setsockopt(struct socket
> *sock,
> int err;
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> u64 val;
>
> if (level != AF_VSOCK)
> @@ -1385,6 +1398,7 @@ static int vsock_stream_setsockopt(struct socket
> *sock,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> lock_sock(sk);
>
> @@ -1442,6 +1456,7 @@ static int vsock_stream_getsockopt(struct socket
> *sock,
> int len;
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> u64 val;
>
> if (level != AF_VSOCK)
> @@ -1465,6 +1480,7 @@ static int vsock_stream_getsockopt(struct socket
> *sock,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> switch (optname) {
> case SO_VM_SOCKETS_BUFFER_SIZE:
> @@ -1509,6 +1525,7 @@ static int vsock_stream_sendmsg(struct socket
> *sock, struct msghdr *msg, {
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> ssize_t total_written;
> long timeout;
> int err;
> @@ -1517,6 +1534,7 @@ static int vsock_stream_sendmsg(struct socket
> *sock, struct msghdr *msg,
>
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
> total_written = 0;
> err = 0;
>
> @@ -1648,6 +1666,7 @@ vsock_stream_recvmsg(struct socket *sock, struct
> msghdr *msg, size_t len, {
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> int err;
> size_t target;
> ssize_t copied;
> @@ -1658,6 +1677,7 @@ vsock_stream_recvmsg(struct socket *sock, struct
> msghdr *msg, size_t len,
>
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
> err = 0;
>
> lock_sock(sk);
> @@ -1872,7 +1892,7 @@ static long vsock_dev_do_ioctl(struct file *filp,
>
> switch (cmd) {
> case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
> - if (put_user(transport->get_local_cid(), p) != 0)
> + if (put_user(transport_single->get_local_cid(), p) != 0)
> retval = -EFAULT;
> break;
>
> @@ -1919,7 +1939,7 @@ int __vsock_core_init(const struct vsock_transport
> *t, struct module *owner)
> if (err)
> return err;
>
> - if (transport) {
> + if (transport_single) {
> err = -EBUSY;
> goto err_busy;
> }
> @@ -1928,7 +1948,7 @@ int __vsock_core_init(const struct vsock_transport
> *t, struct module *owner)
> * unload while there are open sockets.
> */
> vsock_proto.owner = owner;
> - transport = t;
> + transport_single = t;
>
> vsock_device.minor = MISC_DYNAMIC_MINOR;
> err = misc_register(&vsock_device);
> @@ -1958,7 +1978,7 @@ int __vsock_core_init(const struct vsock_transport
> *t, struct module *owner)
> err_deregister_misc:
> misc_deregister(&vsock_device);
> err_reset_transport:
> - transport = NULL;
> + transport_single = NULL;
> err_busy:
> mutex_unlock(&vsock_register_mutex);
> return err;
> @@ -1975,7 +1995,7 @@ void vsock_core_exit(void)
>
> /* We do not want the assignment below re-ordered. */
> mb();
> - transport = NULL;
> + transport_single = NULL;
>
> mutex_unlock(&vsock_register_mutex);
> }
> @@ -1986,7 +2006,7 @@ const struct vsock_transport
> *vsock_core_get_transport(void)
> /* vsock_register_mutex not taken since only the transport uses this
> * function and only while registered.
> */
> - return transport;
> + return transport_single;
> }
> EXPORT_SYMBOL_GPL(vsock_core_get_transport);
>
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 03/14] vsock: remove include/linux/vm_sockets.h file
From: Jorgen Hansen @ 2019-10-30 14:57 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-4-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 03/14] vsock: remove include/linux/vm_sockets.h
> file
>
> This header file now only includes the "uapi/linux/vm_sockets.h".
> We can include directly it when needed.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/linux/vm_sockets.h | 13 -------------
> include/net/af_vsock.h | 2 +-
> include/net/vsock_addr.h | 2 +-
> net/vmw_vsock/vmci_transport_notify.h | 1 -
> 4 files changed, 2 insertions(+), 16 deletions(-) delete mode 100644
> include/linux/vm_sockets.h
>
> diff --git a/include/linux/vm_sockets.h b/include/linux/vm_sockets.h
> deleted file mode 100644 index 7dd899ccb920..000000000000
> --- a/include/linux/vm_sockets.h
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * VMware vSockets Driver
> - *
> - * Copyright (C) 2007-2013 VMware, Inc. All rights reserved.
> - */
> -
> -#ifndef _VM_SOCKETS_H
> -#define _VM_SOCKETS_H
> -
> -#include <uapi/linux/vm_sockets.h>
> -
> -#endif /* _VM_SOCKETS_H */
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index
> 80ea0f93d3f7..c660402b10f2 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -10,7 +10,7 @@
>
> #include <linux/kernel.h>
> #include <linux/workqueue.h>
> -#include <linux/vm_sockets.h>
> +#include <uapi/linux/vm_sockets.h>
>
> #include "vsock_addr.h"
>
> diff --git a/include/net/vsock_addr.h b/include/net/vsock_addr.h index
> 57d2db5c4bdf..cf8cc140d68d 100644
> --- a/include/net/vsock_addr.h
> +++ b/include/net/vsock_addr.h
> @@ -8,7 +8,7 @@
> #ifndef _VSOCK_ADDR_H_
> #define _VSOCK_ADDR_H_
>
> -#include <linux/vm_sockets.h>
> +#include <uapi/linux/vm_sockets.h>
>
> void vsock_addr_init(struct sockaddr_vm *addr, u32 cid, u32 port); int
> vsock_addr_validate(const struct sockaddr_vm *addr); diff --git
> a/net/vmw_vsock/vmci_transport_notify.h
> b/net/vmw_vsock/vmci_transport_notify.h
> index 7843f08d4290..a1aa5a998c0e 100644
> --- a/net/vmw_vsock/vmci_transport_notify.h
> +++ b/net/vmw_vsock/vmci_transport_notify.h
> @@ -11,7 +11,6 @@
> #include <linux/types.h>
> #include <linux/vmw_vmci_defs.h>
> #include <linux/vmw_vmci_api.h>
> -#include <linux/vm_sockets.h>
>
> #include "vmci_transport.h"
>
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 02/14] vsock: remove vm_sockets_get_local_cid()
From: Jorgen Hansen @ 2019-10-30 14:55 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-3-sgarzare@redhat.com>
> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> To: netdev@vger.kernel.org
> Subject: [PATCH net-next 02/14] vsock: remove vm_sockets_get_local_cid()
>
> vm_sockets_get_local_cid() is only used in virtio_transport_common.c.
> We can replace it calling the virtio_transport_get_ops() and using the
> get_local_cid() callback registered by the transport.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/linux/vm_sockets.h | 2 --
> net/vmw_vsock/af_vsock.c | 10 ----------
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 3 files changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/include/linux/vm_sockets.h b/include/linux/vm_sockets.h index
> 33f1a2ecd905..7dd899ccb920 100644
> --- a/include/linux/vm_sockets.h
> +++ b/include/linux/vm_sockets.h
> @@ -10,6 +10,4 @@
>
> #include <uapi/linux/vm_sockets.h>
>
> -int vm_sockets_get_local_cid(void);
> -
> #endif /* _VM_SOCKETS_H */
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> 2ab43b2bba31..2f2582fb7fdd 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -129,16 +129,6 @@ static struct proto vsock_proto = { static const struct
> vsock_transport *transport; static DEFINE_MUTEX(vsock_register_mutex);
>
> -/**** EXPORTS ****/
> -
> -/* Get the ID of the local context. This is transport dependent. */
> -
> -int vm_sockets_get_local_cid(void)
> -{
> - return transport->get_local_cid();
> -}
> -EXPORT_SYMBOL_GPL(vm_sockets_get_local_cid);
> -
> /**** UTILS ****/
>
> /* Each bound VSocket is stored in the bind hash table and each connected
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index d02c9b41a768..b1cd16ed66ea 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -168,7 +168,7 @@ static int virtio_transport_send_pkt_info(struct
> vsock_sock *vsk,
> struct virtio_vsock_pkt *pkt;
> u32 pkt_len = info->pkt_len;
>
> - src_cid = vm_sockets_get_local_cid();
> + src_cid = virtio_transport_get_ops()->transport.get_local_cid();
> src_port = vsk->local_addr.svm_port;
> if (!info->remote_cid) {
> dst_cid = vsk->remote_addr.svm_cid;
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 01/14] vsock/vmci: remove unused VSOCK_DEFAULT_CONNECT_TIMEOUT
From: Jorgen Hansen @ 2019-10-30 14:54 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-2-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 01/14] vsock/vmci: remove unused
> VSOCK_DEFAULT_CONNECT_TIMEOUT
>
> The VSOCK_DEFAULT_CONNECT_TIMEOUT definition was introduced with
> commit d021c344051af ("VSOCK: Introduce VM Sockets"), but it is never used
> in the net/vmw_vsock/vmci_transport.c.
>
> VSOCK_DEFAULT_CONNECT_TIMEOUT is used and defined in
> net/vmw_vsock/af_vsock.c
>
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/vmw_vsock/vmci_transport.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/net/vmw_vsock/vmci_transport.c
> b/net/vmw_vsock/vmci_transport.c index 8c9c4ed90fa7..f8e3131ac480
> 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -78,11 +78,6 @@ static int PROTOCOL_OVERRIDE = -1;
> #define VMCI_TRANSPORT_DEFAULT_QP_SIZE 262144
> #define VMCI_TRANSPORT_DEFAULT_QP_SIZE_MAX 262144
>
> -/* The default peer timeout indicates how long we will wait for a peer
> response
> - * to a control message.
> - */
> -#define VSOCK_DEFAULT_CONNECT_TIMEOUT (2 * HZ)
> -
> /* Helper function to convert from a VMCI error code to a VSock error code.
> */
>
> static s32 vmci_transport_error_to_vsock_error(s32 vmci_error)
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* Re: [GIT PULL] Hyper-V commits for 5.4-rc
From: Sasha Levin @ 2019-10-30 13:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-kernel, linux-hyperv, kys, Stephen Hemminger,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgx_pSBtvmQE9zuNB6aoP52z601SG1pQDtrhm9ZMHNPMw@mail.gmail.com>
On Wed, Oct 30, 2019 at 02:15:08PM +0100, Linus Torvalds wrote:
>On Wed, Oct 30, 2019 at 12:37 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
>
>No, Sasha, I'm not pulling this.
>
>It's completely broken garbage.
It is, appologies!
>You already sent me two of those fixes earlier, and they got pulled in
>commit 56c642e2aa1c ("Merge tag 'hyperv-fixes-signed' of
>git://git.kernel.org/pub/scm/linux/kernel/git/hyper>") two weeks ago.
>
>Fine - of the three fixes you claim, I could do this pull, and get at
>least one of them.
>
>Except YOU HAVE REBASED your branch, so I see the other two fixes that
>I already got as duplicates.
>
>WHY?
Honestly, I forgot that I sent you those two commits two weeks ago, but
still - you must be asking yourself why the heck would he rebase those,
right?
As I was working on the branch a few days ago I messed up and killed my
-fixes branch accidentally. To fix that I figured I'll just pick up
those 3 fixes again from my mailbox since they're grouped so nicely
over there and I haven't sent them to you yet.
Except that I did. And now they ended up with different IDs.
I then did a merge attempt before sending them to you, expecting that
things would blow up if I messed something up, but it didn't because the
commits themselves are identical, so I haven't noticed it then either.
Anyway, sorry about this, I'll resend it with just the relevant fix.
--
Thanks,
Sasha
^ permalink raw reply
* Re: [GIT PULL] Hyper-V commits for 5.4-rc
From: Linus Torvalds @ 2019-10-30 13:15 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel, linux-hyperv, kys, Stephen Hemminger,
Linux Kernel Mailing List
In-Reply-To: <20191030113703.266992083E@mail.kernel.org>
On Wed, Oct 30, 2019 at 12:37 PM Sasha Levin <sashal@kernel.org> wrote:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
No, Sasha, I'm not pulling this.
It's completely broken garbage.
You already sent me two of those fixes earlier, and they got pulled in
commit 56c642e2aa1c ("Merge tag 'hyperv-fixes-signed' of
git://git.kernel.org/pub/scm/linux/kernel/git/hyper>") two weeks ago.
Fine - of the three fixes you claim, I could do this pull, and get at
least one of them.
Except YOU HAVE REBASED your branch, so I see the other two fixes that
I already got as duplicates.
WHY?
Stop this. Read the documentation on rebasing, and stop doing this
kind of insane thing.
Linus
^ permalink raw reply
* [GIT PULL] Hyper-V commits for 5.4-rc
From: Sasha Levin @ 2019-10-30 11:37 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-hyperv, kys, sthemmin, linux-kernel
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
The following changes since commit 54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c:
Linux 5.4-rc1 (2019-09-30 10:35:40 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
for you to fetch changes up to 590c28b9199c99593b879cbb82d7d4be605894ec:
Drivers: hv: vmbus: Fix harmless building warnings without CONFIG_PM_SLEEP (2019-10-28 12:24:53 -0400)
- ----------------------------------------------------------------
- - Fix a leak and improve the handling of the ring buffer in the Hyper-V
HID driver from Dexuan Cui.
- - Fix a (harmless) build warning in vmbus PM code by Dexuan Cui.
- - A fix for a build issue in the Hyper-V IOMMU driver resulting from
enablement on new architectures by Boqun Feng.
- ----------------------------------------------------------------
Boqun Feng (1):
drivers: iommu: hyperv: Make HYPERV_IOMMU only available on x86
Dexuan Cui (2):
HID: hyperv: Use in-place iterator API in the channel callback
Drivers: hv: vmbus: Fix harmless building warnings without CONFIG_PM_SLEEP
drivers/hid/hid-hyperv.c | 56 +++++++++---------------------------------------
drivers/hv/vmbus_drv.c | 6 ++++++
drivers/iommu/Kconfig | 2 +-
3 files changed, 17 insertions(+), 47 deletions(-)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEE4n5dijQDou9mhzu83qZv95d3LNwFAl25dZ0ACgkQ3qZv95d3
LNwEXg/+LjI3+t8Qw5rd2aaCU2NUEVTQJA1bMOx8HT62lKVhrbPT7Y3XZ+zP2K9P
fE6c3bHjTnSymrAx3XMNO2qAXYocA6fo3ggNVleTSA7zrh21fIR2XV0NVDoDNZ/d
KEU/kJbAMyVADKRlwnKA9+O+bfOQhvP4wDb3r1fKaKQ+HznEw8rrbQ0NgohGRO0i
85diHq3KUkXfGvh1PzkzeXGSuxMlmoM/ZtsjE+8Okj4NU0R3Wp0nd6kJzi2EwkDz
ZGmxLpV5Se+qilA7gUfayhSdYS0UyFO0BX+rONfKPdEOLwxVS81/5wROwtivJ2xJ
gMqfZg78X8yiqEkFEMfVngxoeARzJGcFYiqjl8iOGL57tU875Mz2yZgAx1PH0J4y
nZ86X44A13ZViOYlMwV0JG0h94FRtiab3PeVbLCXNU1ULSBJcppLP4lUxdVgeZyz
F9xS6RY35QaGxyT1/cb+g2qr//TAXPQaJh4vnh5vEx/U3lI86+IyCs0xuaOhKP06
frdoXOTdRmLZoqm7ruoCt4gTJUIGbYvGruisAVifdS0401UxXssIKoh1/1CHbBE6
1/bUofBwNrXtyfbru2VyROxRqb/uxZj+UECQwRqJkkg4JUOglBz08h+O+Rjie79i
PrUjPAwhUAKxQ19Cj59M+r6MlT5U9L/Fp2q/BODk1gu1jC3bRAM=
=UL+8
-----END PGP SIGNATURE-----
^ permalink raw reply
* RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-29 22:08 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jakub Kicinski, sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191029145905.414f86c3@hermes.lan>
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, October 29, 2019 5:59 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>; sashal@kernel.org;
> linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> olaf@aepfle.de; vkuznets <vkuznets@redhat.com>; davem@davemloft.net;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
>
> On Tue, 29 Oct 2019 19:17:25 +0000
> Haiyang Zhang <haiyangz@microsoft.com> wrote:
>
> > > -----Original Message-----
> > > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Sent: Monday, October 28, 2019 5:33 PM
> > > To: Haiyang Zhang <haiyangz@microsoft.com>
> > > Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> > > netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> > > <vkuznets@redhat.com>; davem@davemloft.net; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
> > >
> > > On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> > > > This patch adds support of XDP in native mode for hv_netvsc driver,
> and
> > > > transparently sets the XDP program on the associated VF NIC as well.
> > > >
> > > > XDP program cannot run with LRO (RSC) enabled, so you need to
> disable
> > > LRO
> > > > before running XDP:
> > > > ethtool -K eth0 lro off
> > > >
> > > > XDP actions not yet supported:
> > > > XDP_TX, XDP_REDIRECT
> > >
> > > I don't think we want to merge support without at least XDP_TX these
> > > days..
> > Thanks for your detailed comments --
> > I'm working on the XDP_TX...
> >
> > >
> > > And without the ability to prepend headers this may be the least
> > > complete initial XDP implementation we've seen :(
> > The RNDIS packet buffer received by netvsc doesn't have a head room, but
> I'm
> > considering copy the packets to the page buffer, with a head room space
> > reserved for XDP.
>
>
> There is a small amount of headroom available by reusing the RNDIS
> header and packet space. Looks like 40 bytes or so.
Yes, I thought about the RNDIS header. But is this space sufficient?
I saw some drivers, like virtio_net gives bigger headroom: 256 bytes.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Stephen Hemminger @ 2019-10-29 21:59 UTC (permalink / raw)
To: Haiyang Zhang
Cc: Jakub Kicinski, sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <DM6PR21MB1337547067BE5E52DFE05E20CA610@DM6PR21MB1337.namprd21.prod.outlook.com>
On Tue, 29 Oct 2019 19:17:25 +0000
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > -----Original Message-----
> > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Sent: Monday, October 28, 2019 5:33 PM
> > To: Haiyang Zhang <haiyangz@microsoft.com>
> > Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> > netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> > <vkuznets@redhat.com>; davem@davemloft.net; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
> >
> > On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> > > This patch adds support of XDP in native mode for hv_netvsc driver, and
> > > transparently sets the XDP program on the associated VF NIC as well.
> > >
> > > XDP program cannot run with LRO (RSC) enabled, so you need to disable
> > LRO
> > > before running XDP:
> > > ethtool -K eth0 lro off
> > >
> > > XDP actions not yet supported:
> > > XDP_TX, XDP_REDIRECT
> >
> > I don't think we want to merge support without at least XDP_TX these
> > days..
> Thanks for your detailed comments --
> I'm working on the XDP_TX...
>
> >
> > And without the ability to prepend headers this may be the least
> > complete initial XDP implementation we've seen :(
> The RNDIS packet buffer received by netvsc doesn't have a head room, but I'm
> considering copy the packets to the page buffer, with a head room space
> reserved for XDP.
There is a small amount of headroom available by reusing the RNDIS
header and packet space. Looks like 40 bytes or so.
^ permalink raw reply
* RE: [PATCH 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining
From: Dexuan Cui @ 2019-10-29 20:33 UTC (permalink / raw)
To: Michael Kelley, linux-kernel@vger.kernel.org, tglx@linutronix.de,
daniel.lezcano@linaro.org, vkuznets, KY Srinivasan,
Stephen Hemminger, sashal@kernel.org, mingo@redhat.com,
bp@alien8.de, hpa@zytor.com, x86@kernel.org, will@kernel.org,
Ganapatrao.Kulkarni@cavium.com, james.morse@arm.com,
steven.price@arm.com, josephl@nvidia.com,
m.szyprowski@samsumg.com, linux-hyperv@vger.kernel.org
In-Reply-To: <1572228459-10550-1-git-send-email-mikelley@microsoft.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Sunday, October 27, 2019 7:10 PM
> ...
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
Should we add the 2 lines:
Cc: stable@vger.kernel.org
Fixes: fd1fea6834d0 ("clocksource/drivers: Make Hyper-V clocksource ISA agnostic")
fd1fea6834d0() removes the clockevents_unbind_device() call and this patch adds it back.
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -323,8 +323,15 @@ void __init hyperv_init(void)
>
> x86_init.pci.arch_init = hv_pci_init;
>
> + if (hv_stimer_alloc())
> + goto remove_hypercall_page;
> +
The error handling is imperfect here: when I force hv_stimer_alloc() to return
-ENOMEM, I get a panic in hv_apic_eoi_write(). It looks it is because we have cleared
the pointer 'hv_vp_assist_page' to NULL, but hv_apic_eoi_write() is still in-use.
In case hv_stimer_alloc() fails, can we set 'direct_mode_enabled' to false
and go on with the legacy Hyper-V timer or LAPIC timer? If not, maybe
we can use a BUG_ON() to explicitly panic?
> return;
>
> +remove_hypercall_page:
> + hypercall_msr.as_uint64 = 0;
> + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> + hv_hypercall_pg = NULL;
> remove_cpuhp_state:
> cpuhp_remove_state(cpuhp);
> free_vp_assist_page:
> -void hv_stimer_cleanup(unsigned int cpu)
> +static int hv_stimer_cleanup(unsigned int cpu)
> {
> struct clock_event_device *ce;
>
> /* Turn off clockevent device */
> if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> ce = per_cpu_ptr(hv_clock_event, cpu);
> +
> + /*
> + * In the legacy case where Direct Mode is not enabled
> + * (which can only be on x86/64), stimer cleanup happens
> + * relatively early in the CPU offlining process. We
> + * must unbind the stimer-based clockevent device so
> + * that the LAPIC timer can take over until clockevents
> + * are no longer needed in the offlining process. The
> + * unbind should not be done when Direct Mode is enabled
> + * because we may be on an architecture where there are
> + * no other clockevents devices to fallback to.
> + */
> + if (!direct_mode_enabled)
> + clockevents_unbind_device(ce, cpu);
> hv_ce_shutdown(ce);
In the legacy stimer0 mode, IMO this hv_ce_shutdown() is unnecessary,
because "clockevents_unbind_device(ce, cpu)" automatically calls
ce->set_state_shutdown(), if ce is active:
clockevents_unbind
__clockevents_unbind
clockevents_replace
tick_install_replacement
clockevents_exchange_device
clockevents_switch_state(old, CLOCK_EVT_STATE_DETACHED)
__clockevents_switch_state
And, in both modes (legacy mode and direct mode), it looks incorrect to
call hv_ce_shutdown() if the current processid id != 'cpu', because
hv_ce_shutdown() -> hv_init_timer() can only access the current CPU's
MSR. Maybe we should use an IPI to run hv_ce_shutdown() on the target
CPU in direct mode?
> -int hv_stimer_alloc(int sint)
> +int hv_stimer_alloc(void)
> ...
> + ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
> + "clockevents/hyperv/stimer:starting",
> + hv_stimer_init, hv_stimer_cleanup);
> + if (ret < 0)
> + goto free_stimer0_irq;
> + stimer0_cpuhp = ret;
> }
> + return ret;
stimer0_cpuhp is 0 when the call is successful, so IMO the logic in
hv_stimer_free() is incorrect. Please see below.
> void hv_stimer_free(void)
> {
> - if (direct_mode_enabled && (stimer0_irq != 0)) {
> - hv_remove_stimer0_irq(stimer0_irq);
> - stimer0_irq = 0;
> + if (direct_mode_enabled) {
> + if (stimer0_cpuhp) {
> + cpuhp_remove_state(stimer0_cpuhp);
> + stimer0_cpuhp = 0;
> + }
> + if (stimer0_irq) {
> + hv_remove_stimer0_irq(stimer0_irq);
> + stimer0_irq = 0;
> + }
> }
IMO this should be
if (direct_mode_enabled) {
if (stimer0_cpuhp == 0)
cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);
if (stimer0_irq) {
hv_remove_stimer0_irq(stimer0_irq);
stimer0_irq = 0;
}
}
BTW, the default value of 'stimer0_cpuhp' is 0, which means success.
Should we change the default value to a non-zero value, e.g. -1 ?
> free_percpu(hv_clock_event);
> hv_clock_event = NULL;
> @@ -190,14 +274,11 @@ void hv_stimer_free(void)
> void hv_stimer_global_cleanup(void)
> {
> int cpu;
> - struct clock_event_device *ce;
>
> - if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> - for_each_present_cpu(cpu) {
> - ce = per_cpu_ptr(hv_clock_event, cpu);
> - clockevents_unbind_device(ce, cpu);
> - }
> + for_each_present_cpu(cpu) {
> + hv_stimer_cleanup(cpu);
hv_stimer_cleanup() -> hv_ce_shutdown() -> hv_init_timer() can not
access a remote CPU's MSR.
> @@ -2310,7 +2305,6 @@ static void hv_crash_handler(struct pt_regs *regs)
> */
> vmbus_connection.conn_state = DISCONNECTED;
> cpu = smp_processor_id();
> - hv_stimer_cleanup(cpu);
> hv_synic_cleanup(cpu);
> hyperv_cleanup();
Why should we remove the line hv_stimer_cleanup() in the crash handler?
In the crash handler, IMO we'd better disable the timer before we proceed.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-29 20:01 UTC (permalink / raw)
To: Jakub Kicinski
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191029125308.78b52511@cakuba.hsd1.ca.comcast.net>
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Tuesday, October 29, 2019 3:53 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
>
> On Tue, 29 Oct 2019 19:17:25 +0000, Haiyang Zhang wrote:
> > > > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > > + struct netvsc_device *nvdev)
> > > > +{
> > > > + struct bpf_prog *old_prog;
> > > > + int frag_max, i;
> > > > +
> > > > + old_prog = netvsc_xdp_get(nvdev);
> > > > +
> > > > + if (!old_prog && !prog)
> > > > + return 0;
> > >
> > > I think this case is now handled by the core.
> > Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the
> upper layer
> > doesn't call XDP_SETUP_PROG with old/new prog both NULL.
> > But this function is also called by other functions in our driver, like
> netvsc_detach(),
> > netvsc_remove(), etc. Instead of checking for NULL in each place, I still
> keep the check inside
> > netvsc_xdp_set().
>
> I see. Makes sense on a closer look.
>
> BTW would you do me a favour and reformat this line:
>
> static struct netvsc_device_info *netvsc_devinfo_get
> (struct netvsc_device *nvdev)
>
> to look like this:
>
> static
> struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device
> *nvdev)
>
> or
>
> static struct netvsc_device_info *
> netvsc_devinfo_get(struct netvsc_device *nvdev)
>
> Otherwise git diff gets confused about which function given chunk
> belongs to. (Incorrectly thinking your patch is touching
> netvsc_get_channels()). I spent few minutes trying to figure out what's
> going on there :)
I will.
>
> > >
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + if (prog) {
> > > > + prog = bpf_prog_add(prog, nvdev->num_chn);
> > > > + if (IS_ERR(prog))
> > > > + return PTR_ERR(prog);
> > > > + }
> > > > +
> > > > + for (i = 0; i < nvdev->num_chn; i++)
> > > > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> > > > +
> > > > + if (old_prog)
> > > > + for (i = 0; i < nvdev->num_chn; i++)
> > > > + bpf_prog_put(old_prog);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog
> *prog)
> > > > +{
> > > > + struct netdev_bpf xdp;
> > > > + bpf_op_t ndo_bpf;
> > > > +
> > > > + ASSERT_RTNL();
> > > > +
> > > > + if (!vf_netdev)
> > > > + return 0;
> > > > +
> > > > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> > > > + if (!ndo_bpf)
> > > > + return 0;
> > > > +
> > > > + memset(&xdp, 0, sizeof(xdp));
> > > > +
> > > > + xdp.command = XDP_SETUP_PROG;
> > > > + xdp.prog = prog;
> > > > +
> > > > + return ndo_bpf(vf_netdev, &xdp);
> > >
> > > IMHO the automatic propagation is not a good idea. Especially if the
> > > propagation doesn't make the entire installation fail if VF doesn't
> > > have ndo_bpf.
> >
> > On Hyperv and Azure hosts, VF is always acting as a slave below netvsc.
> > And they are both active -- most data packets go to VF, but broadcast,
> > multicast, and TCP SYN packets go to netvsc synthetic data path. The
> synthetic
> > NIC (netvsc) is also a failover NIC when VF is not available.
> > We ask customers to only use the synthetic NIC directly. So propagation
> > of XDP setting to VF NIC is desired.
> > But, I will change the return code to error, so the entire installation fails if
> VF is
> > present but unable to set XDP prog.
>
> Okay, if I read the rest of the code correctly you also fail attach
> if xdp propagation failed? If that's the case and we return an error
> here on missing NDO, then the propagation could be okay.
>
> So the semantics are these:
>
> (a) install on virt - potentially overwrites the existing VF prog;
> (b) install on VF is not noticed by virt;
> (c) uninstall on virt - clears both virt and VF, regardless what
> program was installed on virt;
> (d) uninstall on VF does not propagate;
>
> Since you're adding documentation it would perhaps be worth stating
> there that touching the program on the VF is not supported/may lead
> to breakage, and users should only touch/configure the program on the
> virt.
Sure I will document the recommended way of install xdp prog.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Jakub Kicinski @ 2019-10-29 19:53 UTC (permalink / raw)
To: Haiyang Zhang
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <DM6PR21MB1337547067BE5E52DFE05E20CA610@DM6PR21MB1337.namprd21.prod.outlook.com>
On Tue, 29 Oct 2019 19:17:25 +0000, Haiyang Zhang wrote:
> > > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > + struct netvsc_device *nvdev)
> > > +{
> > > + struct bpf_prog *old_prog;
> > > + int frag_max, i;
> > > +
> > > + old_prog = netvsc_xdp_get(nvdev);
> > > +
> > > + if (!old_prog && !prog)
> > > + return 0;
> >
> > I think this case is now handled by the core.
> Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the upper layer
> doesn't call XDP_SETUP_PROG with old/new prog both NULL.
> But this function is also called by other functions in our driver, like netvsc_detach(),
> netvsc_remove(), etc. Instead of checking for NULL in each place, I still keep the check inside
> netvsc_xdp_set().
I see. Makes sense on a closer look.
BTW would you do me a favour and reformat this line:
static struct netvsc_device_info *netvsc_devinfo_get
(struct netvsc_device *nvdev)
to look like this:
static
struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
or
static struct netvsc_device_info *
netvsc_devinfo_get(struct netvsc_device *nvdev)
Otherwise git diff gets confused about which function given chunk
belongs to. (Incorrectly thinking your patch is touching
netvsc_get_channels()). I spent few minutes trying to figure out what's
going on there :)
> >
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + if (prog) {
> > > + prog = bpf_prog_add(prog, nvdev->num_chn);
> > > + if (IS_ERR(prog))
> > > + return PTR_ERR(prog);
> > > + }
> > > +
> > > + for (i = 0; i < nvdev->num_chn; i++)
> > > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> > > +
> > > + if (old_prog)
> > > + for (i = 0; i < nvdev->num_chn; i++)
> > > + bpf_prog_put(old_prog);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog)
> > > +{
> > > + struct netdev_bpf xdp;
> > > + bpf_op_t ndo_bpf;
> > > +
> > > + ASSERT_RTNL();
> > > +
> > > + if (!vf_netdev)
> > > + return 0;
> > > +
> > > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> > > + if (!ndo_bpf)
> > > + return 0;
> > > +
> > > + memset(&xdp, 0, sizeof(xdp));
> > > +
> > > + xdp.command = XDP_SETUP_PROG;
> > > + xdp.prog = prog;
> > > +
> > > + return ndo_bpf(vf_netdev, &xdp);
> >
> > IMHO the automatic propagation is not a good idea. Especially if the
> > propagation doesn't make the entire installation fail if VF doesn't
> > have ndo_bpf.
>
> On Hyperv and Azure hosts, VF is always acting as a slave below netvsc.
> And they are both active -- most data packets go to VF, but broadcast,
> multicast, and TCP SYN packets go to netvsc synthetic data path. The synthetic
> NIC (netvsc) is also a failover NIC when VF is not available.
> We ask customers to only use the synthetic NIC directly. So propagation
> of XDP setting to VF NIC is desired.
> But, I will change the return code to error, so the entire installation fails if VF is
> present but unable to set XDP prog.
Okay, if I read the rest of the code correctly you also fail attach
if xdp propagation failed? If that's the case and we return an error
here on missing NDO, then the propagation could be okay.
So the semantics are these:
(a) install on virt - potentially overwrites the existing VF prog;
(b) install on VF is not noticed by virt;
(c) uninstall on virt - clears both virt and VF, regardless what
program was installed on virt;
(d) uninstall on VF does not propagate;
Since you're adding documentation it would perhaps be worth stating
there that touching the program on the VF is not supported/may lead
to breakage, and users should only touch/configure the program on the
virt.
^ permalink raw reply
* RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-29 19:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191028143322.45d81da4@cakuba.hsd1.ca.comcast.net>
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Monday, October 28, 2019 5:33 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
>
> On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> > This patch adds support of XDP in native mode for hv_netvsc driver, and
> > transparently sets the XDP program on the associated VF NIC as well.
> >
> > XDP program cannot run with LRO (RSC) enabled, so you need to disable
> LRO
> > before running XDP:
> > ethtool -K eth0 lro off
> >
> > XDP actions not yet supported:
> > XDP_TX, XDP_REDIRECT
>
> I don't think we want to merge support without at least XDP_TX these
> days..
Thanks for your detailed comments --
I'm working on the XDP_TX...
>
> And without the ability to prepend headers this may be the least
> complete initial XDP implementation we've seen :(
The RNDIS packet buffer received by netvsc doesn't have a head room, but I'm
considering copy the packets to the page buffer, with a head room space
reserved for XDP.
>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index d22a36f..688487b 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -122,8 +122,10 @@ static void free_netvsc_device(struct rcu_head
> *head)
> > vfree(nvdev->send_buf);
> > kfree(nvdev->send_section_map);
> >
> > - for (i = 0; i < VRSS_CHANNEL_MAX; i++)
> > + for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
> > + xdp_rxq_info_unreg(&nvdev->chan_table[i].xdp_rxq);
> > vfree(nvdev->chan_table[i].mrc.slots);
> > + }
> >
> > kfree(nvdev);
> > }
> > @@ -1370,6 +1372,10 @@ struct netvsc_device *netvsc_device_add(struct
> hv_device *device,
> > nvchan->net_device = net_device;
> > u64_stats_init(&nvchan->tx_stats.syncp);
> > u64_stats_init(&nvchan->rx_stats.syncp);
> > +
> > + xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
> > + xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
> > + MEM_TYPE_PAGE_SHARED, NULL);
>
> These can fail.
I will add error handling.
>
> > }
> >
> > /* Enable NAPI handler before init callbacks */
> > diff --git a/drivers/net/hyperv/netvsc_bpf.c
> b/drivers/net/hyperv/netvsc_bpf.c
> > new file mode 100644
> > index 0000000..4d235ac
> > --- /dev/null
> > +++ b/drivers/net/hyperv/netvsc_bpf.c
> > @@ -0,0 +1,211 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2019, Microsoft Corporation.
> > + *
> > + * Author:
> > + * Haiyang Zhang <haiyangz@microsoft.com>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/bpf.h>
> > +#include <linux/bpf_trace.h>
> > +#include <linux/kernel.h>
> > +#include <net/xdp.h>
> > +
> > +#include <linux/mutex.h>
> > +#include <linux/rtnetlink.h>
> > +
> > +#include "hyperv_net.h"
> > +
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel
> *nvchan,
> > + void **p_pbuf)
> > +{
> > + struct page *page = NULL;
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + void *pbuf = data;
> > + struct bpf_prog *prog;
> > + struct xdp_buff xdp;
> > + u32 act = XDP_PASS;
> > +
> > + *p_pbuf = NULL;
> > +
> > + rcu_read_lock();
> > + prog = rcu_dereference(nvchan->bpf_prog);
> > +
> > + if (!prog || nvchan->rsc.cnt > 1)
>
> Can rsc.cnt == 1 not be ensured at setup time? This looks quite
> limiting if random frames could be forced to bypass the filter.
Yes, the setup code already check/ensure LRO is disabled. So rsc.cnt > 1
is NOT expected here. Just an error check. I will change the return value
to XDP_ABORTED for this.
>
> > + goto out;
> > +
> > + /* copy to a new page buffer if data are not within a page */
> > + if (virt_to_page(data) != virt_to_page(data + len - 1)) {
> > + page = alloc_page(GFP_ATOMIC);
> > + if (!page)
> > + goto out;
>
> Returning XDP_PASS on allocation failure seems highly questionable.
I will change the return value to XDP_ABORTED for this too.
>
> > + pbuf = page_address(page);
> > + memcpy(pbuf, nvchan->rsc.data[0], len);
> > +
> > + *p_pbuf = pbuf;
> > + }
> > +
> > + xdp.data_hard_start = pbuf;
> > + xdp.data = xdp.data_hard_start;
>
> This patch also doesn't add any headroom for XDP to prepend data :(
I'm considering to add the headroom to the start of the page.
>
> > + xdp_set_data_meta_invalid(&xdp);
> > + xdp.data_end = xdp.data + len;
> > + xdp.rxq = &nvchan->xdp_rxq;
> > + xdp.handle = 0;
> > +
> > + act = bpf_prog_run_xdp(prog, &xdp);
> > +
> > + switch (act) {
> > + case XDP_PASS:
> > + /* Pass to upper layers */
> > + break;
> > +
> > + case XDP_ABORTED:
> > + trace_xdp_exception(ndev, prog, act);
> > + break;
> > +
> > + case XDP_DROP:
> > + break;
> > +
> > + default:
> > + bpf_warn_invalid_xdp_action(act);
> > + }
> > +
> > +out:
> > + rcu_read_unlock();
> > +
> > + if (page && act != XDP_PASS) {
> > + *p_pbuf = NULL;
> > + __free_page(page);
> > + }
> > +
> > + return act;
> > +}
> > +
> > +unsigned int netvsc_xdp_fraglen(unsigned int len)
> > +{
> > + return SKB_DATA_ALIGN(len) +
> > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > +}
> > +
> > +struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev)
> > +{
> > + return rtnl_dereference(nvdev->chan_table[0].bpf_prog);
> > +}
> > +
> > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > + struct netvsc_device *nvdev)
> > +{
> > + struct bpf_prog *old_prog;
> > + int frag_max, i;
> > +
> > + old_prog = netvsc_xdp_get(nvdev);
> > +
> > + if (!old_prog && !prog)
> > + return 0;
>
> I think this case is now handled by the core.
Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the upper layer
doesn't call XDP_SETUP_PROG with old/new prog both NULL.
But this function is also called by other functions in our driver, like netvsc_detach(),
netvsc_remove(), etc. Instead of checking for NULL in each place, I still keep the check inside
netvsc_xdp_set().
>
> > + frag_max = netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
> > + if (prog && frag_max > PAGE_SIZE) {
> > + netdev_err(dev, "XDP: mtu:%u too large, frag:%u\n",
> > + dev->mtu, frag_max);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (prog && (dev->features & NETIF_F_LRO)) {
> > + netdev_err(dev, "XDP: not support LRO\n");
>
> Please report this via extack, that way users will see it in the console
> in which they're installing the program.
I will.
>
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (prog) {
> > + prog = bpf_prog_add(prog, nvdev->num_chn);
> > + if (IS_ERR(prog))
> > + return PTR_ERR(prog);
> > + }
> > +
> > + for (i = 0; i < nvdev->num_chn; i++)
> > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> > +
> > + if (old_prog)
> > + for (i = 0; i < nvdev->num_chn; i++)
> > + bpf_prog_put(old_prog);
> > +
> > + return 0;
> > +}
> > +
> > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog
> *prog)
> > +{
> > + struct netdev_bpf xdp;
> > + bpf_op_t ndo_bpf;
> > +
> > + ASSERT_RTNL();
> > +
> > + if (!vf_netdev)
> > + return 0;
> > +
> > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> > + if (!ndo_bpf)
> > + return 0;
> > +
> > + memset(&xdp, 0, sizeof(xdp));
> > +
> > + xdp.command = XDP_SETUP_PROG;
> > + xdp.prog = prog;
> > +
> > + return ndo_bpf(vf_netdev, &xdp);
>
> IMHO the automatic propagation is not a good idea. Especially if the
> propagation doesn't make the entire installation fail if VF doesn't
> have ndo_bpf.
On Hyperv and Azure hosts, VF is always acting as a slave below netvsc.
And they are both active -- most data packets go to VF, but broadcast,
multicast, and TCP SYN packets go to netvsc synthetic data path. The synthetic
NIC (netvsc) is also a failover NIC when VF is not available.
We ask customers to only use the synthetic NIC directly. So propagation
of XDP setting to VF NIC is desired.
But, I will change the return code to error, so the entire installation fails if VF is
present but unable to set XDP prog.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Stefano Garzarella @ 2019-10-29 16:35 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger,
Arnd Bergmann, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
Dexuan Cui, linux-kernel, virtualization, Haiyang Zhang,
Stefan Hajnoczi, David S. Miller, Jorgen Hansen
In-Reply-To: <20191027081752.GD4472@stefanha-x1.localdomain>
On Sun, Oct 27, 2019 at 09:17:52AM +0100, Stefan Hajnoczi wrote:
> On Wed, Oct 23, 2019 at 11:55:52AM +0200, Stefano Garzarella wrote:
> > +static int __init vmci_transport_init(void)
> > +{
> > + int features = VSOCK_TRANSPORT_F_DGRAM;
>
> Where is this variable used?
It is introduced in the previous patch "vsock: add multi-transports support",
and it is used in the vsock_core_register(), but since now the
vmci_transport_init() registers the vmci_transport only with DGRAM
feature, I can remove this variable and I can use directly the
VSOCK_TRANSPORT_F_DGRAM.
I'll fix in the v3.
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH net-next 00/14] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-10-29 16:27 UTC (permalink / raw)
To: Stefan Hajnoczi, Adit Ranadive, Vishnu Dasa, Andy king,
Aditya Sarwade, George Zhang, Jorgen Hansen
Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger,
Arnd Bergmann, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
Dexuan Cui, linux-kernel, virtualization, Haiyang Zhang,
Stefan Hajnoczi, David S. Miller
In-Reply-To: <20191027080146.GA4472@stefanha-x1.localdomain>
On Sun, Oct 27, 2019 at 09:01:46AM +0100, Stefan Hajnoczi wrote:
> On Wed, Oct 23, 2019 at 11:55:40AM +0200, Stefano Garzarella wrote:
> > This series adds the multi-transports support to vsock, following
> > this proposal: https://www.spinics.net/lists/netdev/msg575792.html
> >
> > With the multi-transports support, we can use VSOCK with nested VMs
> > (using also different hypervisors) loading both guest->host and
> > host->guest transports at the same time.
> > Before this series, vmci-transport supported this behavior but only
> > using VMware hypervisor on L0, L1, etc.
> >
> > RFC: https://patchwork.ozlabs.org/cover/1168442/
> > RFC -> v1:
> > - Added R-b/A-b from Dexuan and Stefan
> > - Fixed comments and typos in several patches (Stefan)
> > - Patch 7: changed .notify_buffer_size return to void (Stefan)
> > - Added patch 8 to simplify the API exposed to the transports (Stefan)
> > - Patch 11:
> > + documented VSOCK_TRANSPORT_F_* flags (Stefan)
> > + fixed vsock_assign_transport() when the socket is already assigned
> > + moved features outside of struct vsock_transport, and used as
> > parameter of vsock_core_register() as a preparation of Patch 12
> > - Removed "vsock: add 'transport_hg' to handle g2h\h2g transports" patch
> > - Added patch 12 to register vmci_transport only when VMCI guest/host
> > are active
>
> Has there been feedback from Jorgen or someone else from VMware? A
> Reviewed-by or Acked-by would be nice since this patch series affects
> VMCI AF_VSOCK.
>
Unfortunately not for now, I'm adding to this thread some VMware guys that
reviewed latest vmci patches.
Would be nice to have your feedback for these changes.
Thanks in advance,
Stefano
^ permalink raw reply
* Re: BUG: MAX_LOCKDEP_KEYS too low!
From: syzbot @ 2019-10-29 14:09 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, aroulin, ast,
b.a.t.m.a.n, bridge, cleech, daniel, davem, dcaratti, dsa,
edumazet, f.fainelli, fw, gregkh, gustavo, gvaradar, haiyangz,
idosch, info, ivan.khoronzhuk, j.vosburgh, j, jakub.kicinski, jhs,
jiri, jiri, johan.hedberg, johannes.berg, john.hurley, jwi,
kstewart, kvalo, kys, lariel, linmiaohe, linux-bluetooth,
linux-hams, linux-hyperv, linux-kernel, linux-ppp, linux-wireless,
linux-wpan, liuhangbin, marcel, mareklindner
In-Reply-To: <0000000000009ea5720595dc03a3@google.com>
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12e05224e00000
start commit: 60c1769a Add linux-next specific files for 20191028
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=11e05224e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=16e05224e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=cb86688f30db053d
dashboard link: https://syzkaller.appspot.com/bug?extid=692f39f040c1f415567b
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10be9ed0e00000
Reported-by: syzbot+692f39f040c1f415567b@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: INFO: trying to register non-static key in bond_3ad_update_ad_actor_settings
From: syzbot @ 2019-10-29 9:52 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, aroulin, ast,
b.a.t.m.a.n, bridge, cleech, daniel, davem, dcaratti, dsa,
edumazet, f.fainelli, fw, gbastien, gregkh, gustavo, haiyangz,
idosch, info, ivan.khoronzhuk, j.vosburgh, j, jakub.kicinski, jhs,
jiri, jiri, johan.hedberg, johannes.berg, john.hurley, jwi,
kstewart, kvalo, kys, linmiaohe, linux-bluetooth, linux-hams,
linux-hyperv, linux-kernel, linux-ppp, linux-wireless, linux-wpan,
liuhangbin, marcel, mareklindner, mcroce
In-Reply-To: <000000000000044a7f0595fbaf2c@google.com>
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12b54d70e00000
start commit: 60c1769a Add linux-next specific files for 20191028
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=11b54d70e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=16b54d70e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=cb86688f30db053d
dashboard link: https://syzkaller.appspot.com/bug?extid=8da67f407bcba2c72e6e
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14d43a04e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16be3b9ce00000
Reported-by: syzbot+8da67f407bcba2c72e6e@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: INFO: trying to register non-static key in bond_3ad_update_lacp_rate
From: syzbot @ 2019-10-29 4:46 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, aroulin, ast,
b.a.t.m.a.n, bridge, cleech, daniel, davem, dcaratti, dsa,
edumazet, f.fainelli, fw, gregkh, gustavo, gvaradar, haiyangz,
idosch, info, ivan.khoronzhuk, j.vosburgh, j, jakub.kicinski, jhs,
jiri, jiri, johan.hedberg, johannes.berg, john.hurley, jwi,
kstewart, kvalo, kys, lariel, linmiaohe, linux-bluetooth,
linux-hams, linux-hyperv, linux-kernel, linux-ppp, linux-wireless,
linux-wpan, liuhangbin, marcel, mareklindner
In-Reply-To: <000000000000fc25a1059602460a@google.com>
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1467f674e00000
start commit: 60c1769a Add linux-next specific files for 20191028
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=1667f674e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=1267f674e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=cb86688f30db053d
dashboard link: https://syzkaller.appspot.com/bug?extid=0d083911ab18b710da71
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15381ee0e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11571570e00000
Reported-by: syzbot+0d083911ab18b710da71@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH v8 0/5] Add a unified parameter "nopvspin"
From: Zhenzhong Duan @ 2019-10-29 1:33 UTC (permalink / raw)
To: linux-kernel, tglx, pbonzini
Cc: mingo, bp, x86, rkrcmar, sean.j.christopherson, vkuznets,
wanpengli, jmattson, joro, boris.ostrovsky, jgross, peterz, will,
linux-hyperv, kvm, mikelley, kys, haiyangz, sthemmin, sashal
In-Reply-To: <1571829384-5309-1-git-send-email-zhenzhong.duan@oracle.com>
Hi Baolo, Thomas
This patchset is reviewed pass and keep silent for a while, will anyone
of you
consider to pick it up? Thanks
Zhenzhong
On 2019/10/23 19:16, Zhenzhong Duan wrote:
> There are cases folks want to disable spinlock optimization for
> debug/test purpose. Xen and hyperv already have parameters "xen_nopvspin"
> and "hv_nopvspin" to support that, but kvm doesn't.
>
> The first patch adds that feature to KVM guest with "nopvspin".
>
> For compatibility reason original parameters "xen_nopvspin" and
> "hv_nopvspin" are retained and marked obsolete.
>
> v8:
> PATCH2: use 'kvm-guest' instead of 'kvm_guest' [Sean Christopherson]
> PATCH3: add a comment to explain missed 'return' [Sean Christopherson]
>
> v7:
> PATCH3: update comment and use goto, add RB [Vitaly Kuznetsov]
>
> v6:
> PATCH1: add Reviewed-by [Vitaly Kuznetsov]
> PATCH2: change 'pv' to 'PV', add Reviewed-by [Vitaly Kuznetsov]
> PATCH3: refactor 'if' branch in kvm_spinlock_init() [Vitaly Kuznetsov]
>
> v5:
> PATCH1: new patch to revert a currently unnecessory commit,
> code is simpler a bit after that change. [Boris Ostrovsky]
> PATCH3: fold 'if' statement,add comments on virt_spin_lock_key,
> reorder with PATCH2 to better reflect dependency
> PATCH4: fold 'if' statement, add Reviewed-by [Boris Ostrovsky]
> PATCH5: add Reviewed-by [Michael Kelley]
>
> v4:
> PATCH1: use variable name nopvspin instead of pvspin and
> defined it as __initdata, changed print message,
> updated patch description [Sean Christopherson]
> PATCH2: remove Suggested-by, use "kvm-guest:" prefix [Sean Christopherson]
> PATCH3: make variable nopvsin and xen_pvspin coexist
> remove Reviewed-by due to code change [Sean Christopherson]
> PATCH4: make variable nopvsin and hv_pvspin coexist [Sean Christopherson]
>
> v3:
> PATCH2: Fix indentation
>
> v2:
> PATCH1: pick the print code change into separate PATCH2,
> updated patch description [Vitaly Kuznetsov]
> PATCH2: new patch with print code change [Vitaly Kuznetsov]
> PATCH3: add Reviewed-by [Juergen Gross]
>
> Zhenzhong Duan (5):
> Revert "KVM: X86: Fix setup the virt_spin_lock_key before static key
> get initialized"
> x86/kvm: Change print code to use pr_*() format
> x86/kvm: Add "nopvspin" parameter to disable PV spinlocks
> xen: Mark "xen_nopvspin" parameter obsolete
> x86/hyperv: Mark "hv_nopvspin" parameter obsolete
>
> Documentation/admin-guide/kernel-parameters.txt | 14 ++++-
> arch/x86/hyperv/hv_spinlock.c | 4 ++
> arch/x86/include/asm/qspinlock.h | 1 +
> arch/x86/kernel/kvm.c | 79 ++++++++++++++++---------
> arch/x86/xen/spinlock.c | 4 +-
> kernel/locking/qspinlock.c | 7 +++
> 6 files changed, 76 insertions(+), 33 deletions(-)
>
^ permalink raw reply
* Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Jakub Kicinski @ 2019-10-28 21:33 UTC (permalink / raw)
To: Haiyang Zhang
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <1572296801-4789-4-git-send-email-haiyangz@microsoft.com>
On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> This patch adds support of XDP in native mode for hv_netvsc driver, and
> transparently sets the XDP program on the associated VF NIC as well.
>
> XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
> before running XDP:
> ethtool -K eth0 lro off
>
> XDP actions not yet supported:
> XDP_TX, XDP_REDIRECT
I don't think we want to merge support without at least XDP_TX these
days..
And without the ability to prepend headers this may be the least
complete initial XDP implementation we've seen :(
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index d22a36f..688487b 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -122,8 +122,10 @@ static void free_netvsc_device(struct rcu_head *head)
> vfree(nvdev->send_buf);
> kfree(nvdev->send_section_map);
>
> - for (i = 0; i < VRSS_CHANNEL_MAX; i++)
> + for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
> + xdp_rxq_info_unreg(&nvdev->chan_table[i].xdp_rxq);
> vfree(nvdev->chan_table[i].mrc.slots);
> + }
>
> kfree(nvdev);
> }
> @@ -1370,6 +1372,10 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
> nvchan->net_device = net_device;
> u64_stats_init(&nvchan->tx_stats.syncp);
> u64_stats_init(&nvchan->rx_stats.syncp);
> +
> + xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
> + xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
> + MEM_TYPE_PAGE_SHARED, NULL);
These can fail.
> }
>
> /* Enable NAPI handler before init callbacks */
> diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
> new file mode 100644
> index 0000000..4d235ac
> --- /dev/null
> +++ b/drivers/net/hyperv/netvsc_bpf.c
> @@ -0,0 +1,211 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +/* Copyright (c) 2019, Microsoft Corporation.
> + *
> + * Author:
> + * Haiyang Zhang <haiyangz@microsoft.com>
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +#include <linux/netdevice.h>
> +#include <linux/etherdevice.h>
> +#include <linux/ethtool.h>
> +#include <linux/bpf.h>
> +#include <linux/bpf_trace.h>
> +#include <linux/kernel.h>
> +#include <net/xdp.h>
> +
> +#include <linux/mutex.h>
> +#include <linux/rtnetlink.h>
> +
> +#include "hyperv_net.h"
> +
> +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> + void **p_pbuf)
> +{
> + struct page *page = NULL;
> + void *data = nvchan->rsc.data[0];
> + u32 len = nvchan->rsc.len[0];
> + void *pbuf = data;
> + struct bpf_prog *prog;
> + struct xdp_buff xdp;
> + u32 act = XDP_PASS;
> +
> + *p_pbuf = NULL;
> +
> + rcu_read_lock();
> + prog = rcu_dereference(nvchan->bpf_prog);
> +
> + if (!prog || nvchan->rsc.cnt > 1)
Can rsc.cnt == 1 not be ensured at setup time? This looks quite
limiting if random frames could be forced to bypass the filter.
> + goto out;
> +
> + /* copy to a new page buffer if data are not within a page */
> + if (virt_to_page(data) != virt_to_page(data + len - 1)) {
> + page = alloc_page(GFP_ATOMIC);
> + if (!page)
> + goto out;
Returning XDP_PASS on allocation failure seems highly questionable.
> + pbuf = page_address(page);
> + memcpy(pbuf, nvchan->rsc.data[0], len);
> +
> + *p_pbuf = pbuf;
> + }
> +
> + xdp.data_hard_start = pbuf;
> + xdp.data = xdp.data_hard_start;
This patch also doesn't add any headroom for XDP to prepend data :(
> + xdp_set_data_meta_invalid(&xdp);
> + xdp.data_end = xdp.data + len;
> + xdp.rxq = &nvchan->xdp_rxq;
> + xdp.handle = 0;
> +
> + act = bpf_prog_run_xdp(prog, &xdp);
> +
> + switch (act) {
> + case XDP_PASS:
> + /* Pass to upper layers */
> + break;
> +
> + case XDP_ABORTED:
> + trace_xdp_exception(ndev, prog, act);
> + break;
> +
> + case XDP_DROP:
> + break;
> +
> + default:
> + bpf_warn_invalid_xdp_action(act);
> + }
> +
> +out:
> + rcu_read_unlock();
> +
> + if (page && act != XDP_PASS) {
> + *p_pbuf = NULL;
> + __free_page(page);
> + }
> +
> + return act;
> +}
> +
> +unsigned int netvsc_xdp_fraglen(unsigned int len)
> +{
> + return SKB_DATA_ALIGN(len) +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> +}
> +
> +struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev)
> +{
> + return rtnl_dereference(nvdev->chan_table[0].bpf_prog);
> +}
> +
> +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> + struct netvsc_device *nvdev)
> +{
> + struct bpf_prog *old_prog;
> + int frag_max, i;
> +
> + old_prog = netvsc_xdp_get(nvdev);
> +
> + if (!old_prog && !prog)
> + return 0;
I think this case is now handled by the core.
> + frag_max = netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
> + if (prog && frag_max > PAGE_SIZE) {
> + netdev_err(dev, "XDP: mtu:%u too large, frag:%u\n",
> + dev->mtu, frag_max);
> + return -EOPNOTSUPP;
> + }
> +
> + if (prog && (dev->features & NETIF_F_LRO)) {
> + netdev_err(dev, "XDP: not support LRO\n");
Please report this via extack, that way users will see it in the console
in which they're installing the program.
> + return -EOPNOTSUPP;
> + }
> +
> + if (prog) {
> + prog = bpf_prog_add(prog, nvdev->num_chn);
> + if (IS_ERR(prog))
> + return PTR_ERR(prog);
> + }
> +
> + for (i = 0; i < nvdev->num_chn; i++)
> + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> +
> + if (old_prog)
> + for (i = 0; i < nvdev->num_chn; i++)
> + bpf_prog_put(old_prog);
> +
> + return 0;
> +}
> +
> +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog)
> +{
> + struct netdev_bpf xdp;
> + bpf_op_t ndo_bpf;
> +
> + ASSERT_RTNL();
> +
> + if (!vf_netdev)
> + return 0;
> +
> + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> + if (!ndo_bpf)
> + return 0;
> +
> + memset(&xdp, 0, sizeof(xdp));
> +
> + xdp.command = XDP_SETUP_PROG;
> + xdp.prog = prog;
> +
> + return ndo_bpf(vf_netdev, &xdp);
IMHO the automatic propagation is not a good idea. Especially if the
propagation doesn't make the entire installation fail if VF doesn't
have ndo_bpf.
> +}
^ permalink raw reply
* [PATCH net-next, 0/4] hv_netvsc: Add XDP support and some error handling fixes
From: Haiyang Zhang @ 2019-10-28 21:06 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
This patch set fixes some error handling issues in netvsc driver,
and add XDP support.
Haiyang Zhang (4):
hv_netvsc: Fix error handling in netvsc_set_features()
hv_netvsc: Fix error handling in netvsc_attach()
hv_netvsc: Add XDP support
hv_netvsc: Update document for XDP support
.../networking/device_drivers/microsoft/netvsc.txt | 14 ++
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 15 ++
drivers/net/hyperv/netvsc.c | 8 +-
drivers/net/hyperv/netvsc_bpf.c | 211 +++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 150 ++++++++++++---
6 files changed, 374 insertions(+), 26 deletions(-)
create mode 100644 drivers/net/hyperv/netvsc_bpf.c
--
1.8.3.1
^ permalink raw reply
* [PATCH net-next, 1/4] hv_netvsc: Fix error handling in netvsc_set_features()
From: Haiyang Zhang @ 2019-10-28 21:07 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
In-Reply-To: <1572296801-4789-1-git-send-email-haiyangz@microsoft.com>
When an error is returned by rndis_filter_set_offload_params(), we should
still assign the unaffected features to ndev->features. Otherwise, these
features will be missing.
Fixes: d6792a5a0747 ("hv_netvsc: Add handler for LRO setting change")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 39dddcd..734e411 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1807,8 +1807,10 @@ static int netvsc_set_features(struct net_device *ndev,
ret = rndis_filter_set_offload_params(ndev, nvdev, &offloads);
- if (ret)
+ if (ret) {
features ^= NETIF_F_LRO;
+ ndev->features = features;
+ }
syncvf:
if (!vf_netdev)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-28 21:07 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
In-Reply-To: <1572296801-4789-1-git-send-email-haiyangz@microsoft.com>
This patch adds support of XDP in native mode for hv_netvsc driver, and
transparently sets the XDP program on the associated VF NIC as well.
XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
before running XDP:
ethtool -K eth0 lro off
XDP actions not yet supported:
XDP_TX, XDP_REDIRECT
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 15 +++
drivers/net/hyperv/netvsc.c | 8 +-
drivers/net/hyperv/netvsc_bpf.c | 211 ++++++++++++++++++++++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 141 ++++++++++++++++++++++-----
5 files changed, 351 insertions(+), 26 deletions(-)
create mode 100644 drivers/net/hyperv/netvsc_bpf.c
diff --git a/drivers/net/hyperv/Makefile b/drivers/net/hyperv/Makefile
index 3a2aa07..0db7cca 100644
--- a/drivers/net/hyperv/Makefile
+++ b/drivers/net/hyperv/Makefile
@@ -1,4 +1,4 @@
# SPDX-License-Identifier: GPL-2.0-only
obj-$(CONFIG_HYPERV_NET) += hv_netvsc.o
-hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o netvsc_trace.o
+hv_netvsc-y := netvsc_drv.o netvsc.o rndis_filter.o netvsc_trace.o netvsc_bpf.o
diff --git a/drivers/net/hyperv/hyperv_net.h b/drivers/net/hyperv/hyperv_net.h
index 670ef68..e5aa256 100644
--- a/drivers/net/hyperv/hyperv_net.h
+++ b/drivers/net/hyperv/hyperv_net.h
@@ -142,6 +142,8 @@ struct netvsc_device_info {
u32 send_section_size;
u32 recv_section_size;
+ struct bpf_prog *bprog;
+
u8 rss_key[NETVSC_HASH_KEYLEN];
};
@@ -199,6 +201,15 @@ int netvsc_recv_callback(struct net_device *net,
void netvsc_channel_cb(void *context);
int netvsc_poll(struct napi_struct *napi, int budget);
+u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
+ void **p_pbuf);
+unsigned int netvsc_xdp_fraglen(unsigned int len);
+struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev);
+int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+ struct netvsc_device *nvdev);
+int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog);
+int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf);
+
int rndis_set_subchannel(struct net_device *ndev,
struct netvsc_device *nvdev,
struct netvsc_device_info *dev_info);
@@ -865,6 +876,7 @@ struct netvsc_stats {
u64 bytes;
u64 broadcast;
u64 multicast;
+ u64 xdp_drop;
struct u64_stats_sync syncp;
};
@@ -965,6 +977,9 @@ struct netvsc_channel {
atomic_t queue_sends;
struct nvsc_rsc rsc;
+ struct bpf_prog __rcu *bpf_prog;
+ struct xdp_rxq_info xdp_rxq;
+
struct netvsc_stats tx_stats;
struct netvsc_stats rx_stats;
};
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
index d22a36f..688487b 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -122,8 +122,10 @@ static void free_netvsc_device(struct rcu_head *head)
vfree(nvdev->send_buf);
kfree(nvdev->send_section_map);
- for (i = 0; i < VRSS_CHANNEL_MAX; i++)
+ for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
+ xdp_rxq_info_unreg(&nvdev->chan_table[i].xdp_rxq);
vfree(nvdev->chan_table[i].mrc.slots);
+ }
kfree(nvdev);
}
@@ -1370,6 +1372,10 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
nvchan->net_device = net_device;
u64_stats_init(&nvchan->tx_stats.syncp);
u64_stats_init(&nvchan->rx_stats.syncp);
+
+ xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
+ xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
+ MEM_TYPE_PAGE_SHARED, NULL);
}
/* Enable NAPI handler before init callbacks */
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
new file mode 100644
index 0000000..4d235ac
--- /dev/null
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -0,0 +1,211 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/* Copyright (c) 2019, Microsoft Corporation.
+ *
+ * Author:
+ * Haiyang Zhang <haiyangz@microsoft.com>
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/netdevice.h>
+#include <linux/etherdevice.h>
+#include <linux/ethtool.h>
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
+#include <linux/kernel.h>
+#include <net/xdp.h>
+
+#include <linux/mutex.h>
+#include <linux/rtnetlink.h>
+
+#include "hyperv_net.h"
+
+u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
+ void **p_pbuf)
+{
+ struct page *page = NULL;
+ void *data = nvchan->rsc.data[0];
+ u32 len = nvchan->rsc.len[0];
+ void *pbuf = data;
+ struct bpf_prog *prog;
+ struct xdp_buff xdp;
+ u32 act = XDP_PASS;
+
+ *p_pbuf = NULL;
+
+ rcu_read_lock();
+ prog = rcu_dereference(nvchan->bpf_prog);
+
+ if (!prog || nvchan->rsc.cnt > 1)
+ goto out;
+
+ /* copy to a new page buffer if data are not within a page */
+ if (virt_to_page(data) != virt_to_page(data + len - 1)) {
+ page = alloc_page(GFP_ATOMIC);
+ if (!page)
+ goto out;
+
+ pbuf = page_address(page);
+ memcpy(pbuf, nvchan->rsc.data[0], len);
+
+ *p_pbuf = pbuf;
+ }
+
+ xdp.data_hard_start = pbuf;
+ xdp.data = xdp.data_hard_start;
+ xdp_set_data_meta_invalid(&xdp);
+ xdp.data_end = xdp.data + len;
+ xdp.rxq = &nvchan->xdp_rxq;
+ xdp.handle = 0;
+
+ act = bpf_prog_run_xdp(prog, &xdp);
+
+ switch (act) {
+ case XDP_PASS:
+ /* Pass to upper layers */
+ break;
+
+ case XDP_ABORTED:
+ trace_xdp_exception(ndev, prog, act);
+ break;
+
+ case XDP_DROP:
+ break;
+
+ default:
+ bpf_warn_invalid_xdp_action(act);
+ }
+
+out:
+ rcu_read_unlock();
+
+ if (page && act != XDP_PASS) {
+ *p_pbuf = NULL;
+ __free_page(page);
+ }
+
+ return act;
+}
+
+unsigned int netvsc_xdp_fraglen(unsigned int len)
+{
+ return SKB_DATA_ALIGN(len) +
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+}
+
+struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev)
+{
+ return rtnl_dereference(nvdev->chan_table[0].bpf_prog);
+}
+
+int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
+ struct netvsc_device *nvdev)
+{
+ struct bpf_prog *old_prog;
+ int frag_max, i;
+
+ old_prog = netvsc_xdp_get(nvdev);
+
+ if (!old_prog && !prog)
+ return 0;
+
+ frag_max = netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
+ if (prog && frag_max > PAGE_SIZE) {
+ netdev_err(dev, "XDP: mtu:%u too large, frag:%u\n",
+ dev->mtu, frag_max);
+ return -EOPNOTSUPP;
+ }
+
+ if (prog && (dev->features & NETIF_F_LRO)) {
+ netdev_err(dev, "XDP: not support LRO\n");
+ return -EOPNOTSUPP;
+ }
+
+ if (prog) {
+ prog = bpf_prog_add(prog, nvdev->num_chn);
+ if (IS_ERR(prog))
+ return PTR_ERR(prog);
+ }
+
+ for (i = 0; i < nvdev->num_chn; i++)
+ rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
+
+ if (old_prog)
+ for (i = 0; i < nvdev->num_chn; i++)
+ bpf_prog_put(old_prog);
+
+ return 0;
+}
+
+int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog)
+{
+ struct netdev_bpf xdp;
+ bpf_op_t ndo_bpf;
+
+ ASSERT_RTNL();
+
+ if (!vf_netdev)
+ return 0;
+
+ ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
+ if (!ndo_bpf)
+ return 0;
+
+ memset(&xdp, 0, sizeof(xdp));
+
+ xdp.command = XDP_SETUP_PROG;
+ xdp.prog = prog;
+
+ return ndo_bpf(vf_netdev, &xdp);
+}
+
+static u32 netvsc_xdp_query(struct netvsc_device *nvdev)
+{
+ struct bpf_prog *prog = netvsc_xdp_get(nvdev);
+
+ if (prog)
+ return prog->aux->id;
+
+ return 0;
+}
+
+int netvsc_bpf(struct net_device *dev, struct netdev_bpf *bpf)
+{
+ struct net_device_context *ndevctx = netdev_priv(dev);
+ struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
+ struct net_device *vf_netdev = rtnl_dereference(ndevctx->vf_netdev);
+ int ret;
+
+ if (!nvdev || nvdev->destroy) {
+ if (bpf->command == XDP_QUERY_PROG) {
+ bpf->prog_id = 0;
+ return 0; /* Query must always succeed */
+ } else {
+ return -ENODEV;
+ }
+ }
+
+ switch (bpf->command) {
+ case XDP_SETUP_PROG:
+ ret = netvsc_xdp_set(dev, bpf->prog, nvdev);
+
+ if (ret)
+ return ret;
+
+ ret = netvsc_vf_setxdp(vf_netdev, bpf->prog);
+
+ if (ret) {
+ netdev_err(dev, "vf_setxdp failed:%d\n", ret);
+ netvsc_xdp_set(dev, NULL, nvdev);
+ }
+
+ return ret;
+
+ case XDP_QUERY_PROG:
+ bpf->prog_id = netvsc_xdp_query(nvdev);
+ return 0;
+
+ default:
+ return -EINVAL;
+ }
+}
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index a14fc8e..415f8db 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -25,6 +25,7 @@
#include <linux/slab.h>
#include <linux/rtnetlink.h>
#include <linux/netpoll.h>
+#include <linux/bpf.h>
#include <net/arp.h>
#include <net/route.h>
@@ -760,7 +761,8 @@ static void netvsc_comp_ipcsum(struct sk_buff *skb)
}
static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
- struct netvsc_channel *nvchan)
+ struct netvsc_channel *nvchan,
+ void *pbuf)
{
struct napi_struct *napi = &nvchan->napi;
const struct ndis_pkt_8021q_info *vlan = nvchan->rsc.vlan;
@@ -769,16 +771,32 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
struct sk_buff *skb;
int i;
- skb = napi_alloc_skb(napi, nvchan->rsc.pktlen);
- if (!skb)
- return skb;
+ if (pbuf) {
+ unsigned int len = nvchan->rsc.pktlen;
+ unsigned int frag_size = netvsc_xdp_fraglen(len);
- /*
- * Copy to skb. This copy is needed here since the memory pointed by
- * hv_netvsc_packet cannot be deallocated
- */
- for (i = 0; i < nvchan->rsc.cnt; i++)
- skb_put_data(skb, nvchan->rsc.data[i], nvchan->rsc.len[i]);
+ skb = build_skb(pbuf, frag_size);
+
+ if (!skb) {
+ __free_page(virt_to_page(pbuf));
+ return NULL;
+ }
+
+ skb_put(skb, len);
+ skb->dev = napi->dev;
+ } else {
+ skb = napi_alloc_skb(napi, nvchan->rsc.pktlen);
+
+ if (!skb)
+ return NULL;
+
+ /* Copy to skb. This copy is needed here since the memory
+ * pointed by hv_netvsc_packet cannot be deallocated.
+ */
+ for (i = 0; i < nvchan->rsc.cnt; i++)
+ skb_put_data(skb, nvchan->rsc.data[i],
+ nvchan->rsc.len[i]);
+ }
skb->protocol = eth_type_trans(skb, net);
@@ -826,13 +844,25 @@ int netvsc_recv_callback(struct net_device *net,
struct vmbus_channel *channel = nvchan->channel;
u16 q_idx = channel->offermsg.offer.sub_channel_index;
struct sk_buff *skb;
- struct netvsc_stats *rx_stats;
+ struct netvsc_stats *rx_stats = &nvchan->rx_stats;
+ void *pbuf = NULL; /* page buffer */
+ u32 act;
if (net->reg_state != NETREG_REGISTERED)
return NVSP_STAT_FAIL;
+ act = netvsc_run_xdp(net, nvchan, &pbuf);
+
+ if (act != XDP_PASS) {
+ u64_stats_update_begin(&rx_stats->syncp);
+ rx_stats->xdp_drop++;
+ u64_stats_update_end(&rx_stats->syncp);
+
+ return NVSP_STAT_SUCCESS; /* consumed by XDP */
+ }
+
/* Allocate a skb - TODO direct I/O to pages? */
- skb = netvsc_alloc_recv_skb(net, nvchan);
+ skb = netvsc_alloc_recv_skb(net, nvchan, pbuf);
if (unlikely(!skb)) {
++net_device_ctx->eth_stats.rx_no_memory;
@@ -846,7 +876,6 @@ int netvsc_recv_callback(struct net_device *net,
* on the synthetic device because modifying the VF device
* statistics will not work correctly.
*/
- rx_stats = &nvchan->rx_stats;
u64_stats_update_begin(&rx_stats->syncp);
rx_stats->packets++;
rx_stats->bytes += nvchan->rsc.pktlen;
@@ -887,6 +916,7 @@ static void netvsc_get_channels(struct net_device *net,
(struct netvsc_device *nvdev)
{
struct netvsc_device_info *dev_info;
+ struct bpf_prog *prog;
dev_info = kzalloc(sizeof(*dev_info), GFP_ATOMIC);
@@ -894,6 +924,8 @@ static void netvsc_get_channels(struct net_device *net,
return NULL;
if (nvdev) {
+ ASSERT_RTNL();
+
dev_info->num_chn = nvdev->num_chn;
dev_info->send_sections = nvdev->send_section_cnt;
dev_info->send_section_size = nvdev->send_section_size;
@@ -902,6 +934,13 @@ static void netvsc_get_channels(struct net_device *net,
memcpy(dev_info->rss_key, nvdev->extension->rss_key,
NETVSC_HASH_KEYLEN);
+
+ prog = netvsc_xdp_get(nvdev);
+ if (prog) {
+ prog = bpf_prog_add(prog, 1);
+ if (!IS_ERR(prog))
+ dev_info->bprog = prog;
+ }
} else {
dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
dev_info->send_sections = NETVSC_DEFAULT_TX;
@@ -913,6 +952,17 @@ static void netvsc_get_channels(struct net_device *net,
return dev_info;
}
+/* Free struct netvsc_device_info */
+static void netvsc_devinfo_put(struct netvsc_device_info *dev_info)
+{
+ if (dev_info->bprog) {
+ ASSERT_RTNL();
+ bpf_prog_put(dev_info->bprog);
+ }
+
+ kfree(dev_info);
+}
+
static int netvsc_detach(struct net_device *ndev,
struct netvsc_device *nvdev)
{
@@ -924,6 +974,8 @@ static int netvsc_detach(struct net_device *ndev,
if (cancel_work_sync(&nvdev->subchan_work))
nvdev->num_chn = 1;
+ netvsc_xdp_set(ndev, NULL, nvdev);
+
/* If device was up (receiving) then shutdown */
if (netif_running(ndev)) {
netvsc_tx_disable(nvdev, ndev);
@@ -957,7 +1009,8 @@ static int netvsc_attach(struct net_device *ndev,
struct hv_device *hdev = ndev_ctx->device_ctx;
struct netvsc_device *nvdev;
struct rndis_device *rdev;
- int ret;
+ struct bpf_prog *prog;
+ int ret = 0;
nvdev = rndis_filter_device_add(hdev, dev_info);
if (IS_ERR(nvdev))
@@ -973,6 +1026,13 @@ static int netvsc_attach(struct net_device *ndev,
}
}
+ prog = dev_info->bprog;
+ if (prog) {
+ ret = netvsc_xdp_set(ndev, prog, nvdev);
+ if (ret)
+ goto err1;
+ }
+
/* In any case device is now ready */
netif_device_attach(ndev);
@@ -982,7 +1042,7 @@ static int netvsc_attach(struct net_device *ndev,
if (netif_running(ndev)) {
ret = rndis_filter_open(nvdev);
if (ret)
- goto err;
+ goto err2;
rdev = nvdev->extension;
if (!rdev->link_state)
@@ -991,9 +1051,10 @@ static int netvsc_attach(struct net_device *ndev,
return 0;
-err:
+err2:
netif_device_detach(ndev);
+err1:
rndis_filter_device_remove(hdev, nvdev);
return ret;
@@ -1043,7 +1104,7 @@ static int netvsc_set_channels(struct net_device *net,
}
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
@@ -1150,7 +1211,7 @@ static int netvsc_change_mtu(struct net_device *ndev, int mtu)
dev_set_mtu(vf_netdev, orig_mtu);
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
@@ -1375,8 +1436,8 @@ static int netvsc_set_mac_addr(struct net_device *ndev, void *p)
/* statistics per queue (rx/tx packets/bytes) */
#define NETVSC_PCPU_STATS_LEN (num_present_cpus() * ARRAY_SIZE(pcpu_stats))
-/* 4 statistics per queue (rx/tx packets/bytes) */
-#define NETVSC_QUEUE_STATS_LEN(dev) ((dev)->num_chn * 4)
+/* 5 statistics per queue (rx/tx packets/bytes, rx xdp_drop) */
+#define NETVSC_QUEUE_STATS_LEN(dev) ((dev)->num_chn * 5)
static int netvsc_get_sset_count(struct net_device *dev, int string_set)
{
@@ -1408,6 +1469,7 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
struct netvsc_ethtool_pcpu_stats *pcpu_sum;
unsigned int start;
u64 packets, bytes;
+ u64 xdp_drop;
int i, j, cpu;
if (!nvdev)
@@ -1436,9 +1498,11 @@ static void netvsc_get_ethtool_stats(struct net_device *dev,
start = u64_stats_fetch_begin_irq(&qstats->syncp);
packets = qstats->packets;
bytes = qstats->bytes;
+ xdp_drop = qstats->xdp_drop;
} while (u64_stats_fetch_retry_irq(&qstats->syncp, start));
data[i++] = packets;
data[i++] = bytes;
+ data[i++] = xdp_drop;
}
pcpu_sum = kvmalloc_array(num_possible_cpus(),
@@ -1486,6 +1550,8 @@ static void netvsc_get_strings(struct net_device *dev, u32 stringset, u8 *data)
p += ETH_GSTRING_LEN;
sprintf(p, "rx_queue_%u_bytes", i);
p += ETH_GSTRING_LEN;
+ sprintf(p, "rx_queue_%u_xdp_drop", i);
+ p += ETH_GSTRING_LEN;
}
for_each_present_cpu(cpu) {
@@ -1782,10 +1848,27 @@ static int netvsc_set_ringparam(struct net_device *ndev,
}
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
+static netdev_features_t netvsc_fix_features(struct net_device *ndev,
+ netdev_features_t features)
+{
+ struct net_device_context *ndevctx = netdev_priv(ndev);
+ struct netvsc_device *nvdev = rtnl_dereference(ndevctx->nvdev);
+
+ if (!nvdev || nvdev->destroy)
+ return features;
+
+ if ((features & NETIF_F_LRO) && netvsc_xdp_get(nvdev)) {
+ features ^= NETIF_F_LRO;
+ netdev_info(ndev, "Skip LRO - unsupported with XDP\n");
+ }
+
+ return features;
+}
+
static int netvsc_set_features(struct net_device *ndev,
netdev_features_t features)
{
@@ -1872,12 +1955,14 @@ static void netvsc_set_msglevel(struct net_device *ndev, u32 val)
.ndo_start_xmit = netvsc_start_xmit,
.ndo_change_rx_flags = netvsc_change_rx_flags,
.ndo_set_rx_mode = netvsc_set_rx_mode,
+ .ndo_fix_features = netvsc_fix_features,
.ndo_set_features = netvsc_set_features,
.ndo_change_mtu = netvsc_change_mtu,
.ndo_validate_addr = eth_validate_addr,
.ndo_set_mac_address = netvsc_set_mac_addr,
.ndo_select_queue = netvsc_select_queue,
.ndo_get_stats64 = netvsc_get_stats64,
+ .ndo_bpf = netvsc_bpf,
};
/*
@@ -2164,6 +2249,7 @@ static int netvsc_register_vf(struct net_device *vf_netdev)
{
struct net_device_context *net_device_ctx;
struct netvsc_device *netvsc_dev;
+ struct bpf_prog *prog;
struct net_device *ndev;
int ret;
@@ -2208,6 +2294,9 @@ static int netvsc_register_vf(struct net_device *vf_netdev)
vf_netdev->wanted_features = ndev->features;
netdev_update_features(vf_netdev);
+ prog = netvsc_xdp_get(netvsc_dev);
+ netvsc_vf_setxdp(vf_netdev, prog);
+
return NOTIFY_OK;
}
@@ -2249,6 +2338,8 @@ static int netvsc_unregister_vf(struct net_device *vf_netdev)
netdev_info(ndev, "VF unregistering: %s\n", vf_netdev->name);
+ netvsc_vf_setxdp(vf_netdev, NULL);
+
netdev_rx_handler_unregister(vf_netdev);
netdev_upper_dev_unlink(vf_netdev, ndev);
RCU_INIT_POINTER(net_device_ctx->vf_netdev, NULL);
@@ -2362,14 +2453,14 @@ static int netvsc_probe(struct hv_device *dev,
list_add(&net_device_ctx->list, &netvsc_dev_list);
rtnl_unlock();
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return 0;
register_failed:
rtnl_unlock();
rndis_filter_device_remove(dev, nvdev);
rndis_failed:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
devinfo_failed:
free_percpu(net_device_ctx->vf_stats);
no_stats:
@@ -2397,8 +2488,10 @@ static int netvsc_remove(struct hv_device *dev)
rtnl_lock();
nvdev = rtnl_dereference(ndev_ctx->nvdev);
- if (nvdev)
+ if (nvdev) {
cancel_work_sync(&nvdev->subchan_work);
+ netvsc_xdp_set(net, NULL, nvdev);
+ }
/*
* Call to the vsc driver to let it know that the device is being
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next, 4/4] hv_netvsc: Update document for XDP support
From: Haiyang Zhang @ 2019-10-28 21:07 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
In-Reply-To: <1572296801-4789-1-git-send-email-haiyangz@microsoft.com>
Added the new section in the document regarding XDP support
by hv_netvsc driver.
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
.../networking/device_drivers/microsoft/netvsc.txt | 14 ++++++++++++++
1 file changed, 14 insertions(+)
diff --git a/Documentation/networking/device_drivers/microsoft/netvsc.txt b/Documentation/networking/device_drivers/microsoft/netvsc.txt
index 3bfa635..69ccfca 100644
--- a/Documentation/networking/device_drivers/microsoft/netvsc.txt
+++ b/Documentation/networking/device_drivers/microsoft/netvsc.txt
@@ -82,3 +82,17 @@ Features
contain one or more packets. The send buffer is an optimization, the driver
will use slower method to handle very large packets or if the send buffer
area is exhausted.
+
+ XDP support
+ -----------
+ XDP (eXpress Data Path) is a feature that runs eBPF bytecode at the early
+ stage when packets arrive at a NIC card. The goal is to increase performance
+ for packet processing, reducing the overhead of SKB allocation and other
+ upper network layers.
+
+ hv_netvsc supports XDP in native mode, and transparently sets the XDP
+ program on the associated VF NIC as well.
+
+ XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
+ before running XDP:
+ ethtool -K eth0 lro off
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next, 2/4] hv_netvsc: Fix error handling in netvsc_attach()
From: Haiyang Zhang @ 2019-10-28 21:07 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
In-Reply-To: <1572296801-4789-1-git-send-email-haiyangz@microsoft.com>
If rndis_filter_open() fails, we need to remove the rndis device created
in earlier steps, before returning an error code. Otherwise, the retry of
netvsc_attach() from its callers will fail and hang.
Fixes: 7b2ee50c0cd5 ("hv_netvsc: common detach logic")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 734e411..a14fc8e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev,
if (netif_running(ndev)) {
ret = rndis_filter_open(nvdev);
if (ret)
- return ret;
+ goto err;
rdev = nvdev->extension;
if (!rdev->link_state)
@@ -990,6 +990,13 @@ static int netvsc_attach(struct net_device *ndev,
}
return 0;
+
+err:
+ netif_device_detach(ndev);
+
+ rndis_filter_device_remove(hdev, nvdev);
+
+ return ret;
}
static int netvsc_set_channels(struct net_device *net,
--
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