* [PATCH V3,net-next, 2/2] hv_netvsc: Update document for XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579713814-36061-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 | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/networking/device_drivers/microsoft/netvsc.txt b/Documentation/networking/device_drivers/microsoft/netvsc.txt
index 3bfa635..cd63556 100644
--- a/Documentation/networking/device_drivers/microsoft/netvsc.txt
+++ b/Documentation/networking/device_drivers/microsoft/netvsc.txt
@@ -82,3 +82,24 @@ 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.
+
+ Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
+ VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
+ is not recommended, also not propagated to synthetic NIC, and may be
+ overwritten by setting of synthetic NIC.
+
+ XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
+ before running XDP:
+ ethtool -K eth0 lro off
+
+ XDP_REDIRECT action is not yet supported.
--
1.8.3.1
^ permalink raw reply related
* [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 17:23 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579713814-36061-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.
Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
is not recommended, also not propagated to synthetic NIC, and may be
overwritten by setting of synthetic NIC.
The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
for XDP. We thought about re-use the RNDIS header space, but it's too
small. So we decided to copy the packets to a page buffer for XDP. And,
most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
most of the packets run on VF NIC. The synthetic NIC is considered as a
fallback data-path. So the data copy on netvsc won't impact performance
significantly.
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_REDIRECT
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
Changes:
v3: Minor code and comment updates.
v2: Added XDP_TX support. Addressed review comments.
---
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 21 +++-
drivers/net/hyperv/netvsc.c | 31 +++++-
drivers/net/hyperv/netvsc_bpf.c | 209 ++++++++++++++++++++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 175 +++++++++++++++++++++++++------
drivers/net/hyperv/rndis_filter.c | 2 +-
6 files changed, 401 insertions(+), 39 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 dc44819..abda736 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];
};
@@ -189,7 +191,8 @@ int netvsc_send(struct net_device *net,
struct hv_netvsc_packet *packet,
struct rndis_message *rndis_msg,
struct hv_page_buffer *page_buffer,
- struct sk_buff *skb);
+ struct sk_buff *skb,
+ bool xdp_tx);
void netvsc_linkstatus_callback(struct net_device *net,
struct rndis_message *resp);
int netvsc_recv_callback(struct net_device *net,
@@ -198,6 +201,16 @@ 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,
+ struct xdp_buff *xdp);
+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 netlink_ext_ack *extack,
+ 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);
@@ -832,6 +845,8 @@ struct nvsp_message {
#define RNDIS_MAX_PKT_DEFAULT 8
#define RNDIS_PKT_ALIGN_DEFAULT 8
+#define NETVSC_XDP_HDRM 256
+
struct multi_send_data {
struct sk_buff *skb; /* skb containing the pkt */
struct hv_netvsc_packet *pkt; /* netvsc pkt pending */
@@ -867,6 +882,7 @@ struct netvsc_stats {
u64 bytes;
u64 broadcast;
u64 multicast;
+ u64 xdp_drop;
struct u64_stats_sync syncp;
};
@@ -972,6 +988,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 eab83e7..ae3f308 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);
}
@@ -900,7 +902,8 @@ int netvsc_send(struct net_device *ndev,
struct hv_netvsc_packet *packet,
struct rndis_message *rndis_msg,
struct hv_page_buffer *pb,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ bool xdp_tx)
{
struct net_device_context *ndev_ctx = netdev_priv(ndev);
struct netvsc_device *net_device
@@ -923,10 +926,11 @@ int netvsc_send(struct net_device *ndev,
packet->send_buf_index = NETVSC_INVALID_INDEX;
packet->cp_partial = false;
- /* Send control message directly without accessing msd (Multi-Send
- * Data) field which may be changed during data packet processing.
+ /* Send a control message or XDP packet directly without accessing
+ * msd (Multi-Send Data) field which may be changed during data packet
+ * processing.
*/
- if (!skb)
+ if (!skb || xdp_tx)
return netvsc_send_pkt(device, packet, net_device, pb, skb);
/* batch packets in send buffer if possible */
@@ -1392,6 +1396,21 @@ 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);
+
+ ret = xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
+
+ if (ret) {
+ netdev_err(ndev, "xdp_rxq_info_reg fail: %d\n", ret);
+ goto cleanup2;
+ }
+
+ ret = xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
+ MEM_TYPE_PAGE_SHARED, NULL);
+
+ if (ret) {
+ netdev_err(ndev, "xdp reg_mem_model fail: %d\n", ret);
+ goto cleanup2;
+ }
}
/* Enable NAPI handler before init callbacks */
@@ -1437,6 +1456,8 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
cleanup:
netif_napi_del(&net_device->chan_table[0].napi);
+
+cleanup2:
free_netvsc_device(&net_device->rcu);
return ERR_PTR(ret);
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
new file mode 100644
index 0000000..20adfe5
--- /dev/null
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -0,0 +1,209 @@
+// 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,
+ struct xdp_buff *xdp)
+{
+ void *data = nvchan->rsc.data[0];
+ u32 len = nvchan->rsc.len[0];
+ struct page *page = NULL;
+ struct bpf_prog *prog;
+ u32 act = XDP_PASS;
+
+ xdp->data_hard_start = NULL;
+
+ rcu_read_lock();
+ prog = rcu_dereference(nvchan->bpf_prog);
+
+ if (!prog)
+ goto out;
+
+ /* allocate page buffer for data */
+ page = alloc_page(GFP_ATOMIC);
+ if (!page) {
+ act = XDP_DROP;
+ goto out;
+ }
+
+ xdp->data_hard_start = page_address(page);
+ xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
+ xdp_set_data_meta_invalid(xdp);
+ xdp->data_end = xdp->data + len;
+ xdp->rxq = &nvchan->xdp_rxq;
+ xdp->handle = 0;
+
+ memcpy(xdp->data, data, len);
+
+ act = bpf_prog_run_xdp(prog, xdp);
+
+ switch (act) {
+ case XDP_PASS:
+ case XDP_TX:
+ case XDP_DROP:
+ break;
+
+ case XDP_ABORTED:
+ trace_xdp_exception(ndev, prog, act);
+ break;
+
+ default:
+ bpf_warn_invalid_xdp_action(act);
+ }
+
+out:
+ rcu_read_unlock();
+
+ if (page && act != XDP_PASS && act != XDP_TX) {
+ __free_page(page);
+ xdp->data_hard_start = NULL;
+ }
+
+ 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 netlink_ext_ack *extack,
+ struct netvsc_device *nvdev)
+{
+ struct bpf_prog *old_prog;
+ int buf_max, i;
+
+ old_prog = netvsc_xdp_get(nvdev);
+
+ if (!old_prog && !prog)
+ return 0;
+
+ buf_max = NETVSC_XDP_HDRM + netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
+ if (prog && buf_max > PAGE_SIZE) {
+ netdev_err(dev, "XDP: mtu:%u too large, buf_max:%u\n",
+ dev->mtu, buf_max);
+ NL_SET_ERR_MSG_MOD(extack, "XDP: mtu too large");
+
+ return -EOPNOTSUPP;
+ }
+
+ if (prog && (dev->features & NETIF_F_LRO)) {
+ netdev_err(dev, "XDP: not support LRO\n");
+ NL_SET_ERR_MSG_MOD(extack, "XDP: not support LRO");
+
+ return -EOPNOTSUPP;
+ }
+
+ if (prog)
+ bpf_prog_add(prog, nvdev->num_chn);
+
+ 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);
+ struct netlink_ext_ack *extack = bpf->extack;
+ 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, extack, nvdev);
+
+ if (ret)
+ return ret;
+
+ ret = netvsc_vf_setxdp(vf_netdev, bpf->prog);
+
+ if (ret) {
+ netdev_err(dev, "vf_setxdp failed:%d\n", ret);
+ NL_SET_ERR_MSG_MOD(extack, "vf_setxdp failed");
+
+ netvsc_xdp_set(dev, NULL, extack, 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 f3f9eb8..bb1ace4 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>
@@ -519,7 +520,7 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
return rc;
}
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
+static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
struct hv_netvsc_packet *packet = NULL;
@@ -686,7 +687,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
/* timestamp packet in software */
skb_tx_timestamp(skb);
- ret = netvsc_send(net, packet, rndis_msg, pb, skb);
+ ret = netvsc_send(net, packet, rndis_msg, pb, skb, xdp_tx);
if (likely(ret == 0))
return NETDEV_TX_OK;
@@ -709,6 +710,11 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
goto drop;
}
+static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ return netvsc_xmit(skb, ndev, false);
+}
+
/*
* netvsc_linkstatus_callback - Link up/down notification
*/
@@ -751,6 +757,14 @@ void netvsc_linkstatus_callback(struct net_device *net,
schedule_delayed_work(&ndev_ctx->dwork, 0);
}
+static int netvsc_xdp_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ skb->queue_mapping = skb_get_rx_queue(skb);
+ __skb_push(skb, ETH_HLEN);
+
+ return netvsc_xmit(skb, ndev, true);
+}
+
static void netvsc_comp_ipcsum(struct sk_buff *skb)
{
struct iphdr *iph = (struct iphdr *)skb->data;
@@ -760,7 +774,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,
+ struct xdp_buff *xdp)
{
struct napi_struct *napi = &nvchan->napi;
const struct ndis_pkt_8021q_info *vlan = nvchan->rsc.vlan;
@@ -768,18 +783,37 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
nvchan->rsc.csum_info;
const u32 *hash_info = nvchan->rsc.hash_info;
struct sk_buff *skb;
+ void *xbuf = xdp->data_hard_start;
int i;
- skb = napi_alloc_skb(napi, nvchan->rsc.pktlen);
- if (!skb)
- return skb;
+ if (xbuf) {
+ unsigned int hdroom = xdp->data - xdp->data_hard_start;
+ unsigned int xlen = xdp->data_end - xdp->data;
+ unsigned int frag_size = netvsc_xdp_fraglen(hdroom + xlen);
- /*
- * 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(xbuf, frag_size);
+
+ if (!skb) {
+ __free_page(virt_to_page(xbuf));
+ return NULL;
+ }
+
+ skb_reserve(skb, hdroom);
+ skb_put(skb, xlen);
+ 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);
@@ -829,13 +863,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;
+ struct xdp_buff xdp;
+ u32 act;
if (net->reg_state != NETREG_REGISTERED)
return NVSP_STAT_FAIL;
+ act = netvsc_run_xdp(net, nvchan, &xdp);
+
+ if (act != XDP_PASS && act != XDP_TX) {
+ 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, &xdp);
if (unlikely(!skb)) {
++net_device_ctx->eth_stats.rx_no_memory;
@@ -849,7 +895,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;
@@ -860,6 +905,11 @@ int netvsc_recv_callback(struct net_device *net,
++rx_stats->multicast;
u64_stats_update_end(&rx_stats->syncp);
+ if (act == XDP_TX) {
+ netvsc_xdp_xmit(skb, net);
+ return NVSP_STAT_SUCCESS;
+ }
+
napi_gro_receive(&nvchan->napi, skb);
return NVSP_STAT_SUCCESS;
}
@@ -886,10 +936,11 @@ static void netvsc_get_channels(struct net_device *net,
/* Alloc struct netvsc_device_info, and initialize it from either existing
* struct netvsc_device, or from default values.
*/
-static struct netvsc_device_info *netvsc_devinfo_get
- (struct netvsc_device *nvdev)
+static
+struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
{
struct netvsc_device_info *dev_info;
+ struct bpf_prog *prog;
dev_info = kzalloc(sizeof(*dev_info), GFP_ATOMIC);
@@ -897,6 +948,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;
@@ -905,6 +958,12 @@ 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) {
+ bpf_prog_inc(prog);
+ dev_info->bprog = prog;
+ }
} else {
dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
dev_info->send_sections = NETVSC_DEFAULT_TX;
@@ -916,6 +975,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)
{
@@ -927,6 +997,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, NULL, nvdev);
+
/* If device was up (receiving) then shutdown */
if (netif_running(ndev)) {
netvsc_tx_disable(nvdev, ndev);
@@ -960,7 +1032,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))
@@ -976,6 +1049,13 @@ static int netvsc_attach(struct net_device *ndev,
}
}
+ prog = dev_info->bprog;
+ if (prog) {
+ ret = netvsc_xdp_set(ndev, prog, NULL, nvdev);
+ if (ret)
+ goto err1;
+ }
+
/* In any case device is now ready */
netif_device_attach(ndev);
@@ -985,7 +1065,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)
@@ -994,9 +1074,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;
@@ -1046,7 +1127,7 @@ static int netvsc_set_channels(struct net_device *net,
}
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
@@ -1153,7 +1234,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;
}
@@ -1378,8 +1459,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)
{
@@ -1411,6 +1492,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)
@@ -1439,9 +1521,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(),
@@ -1489,6 +1573,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) {
@@ -1785,10 +1871,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)
{
@@ -1875,12 +1978,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,
};
/*
@@ -2167,6 +2272,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;
@@ -2211,6 +2317,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;
}
@@ -2252,6 +2361,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);
@@ -2363,14 +2474,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:
@@ -2398,8 +2509,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, NULL, nvdev);
+ }
/*
* Call to the vsc driver to let it know that the device is being
@@ -2472,11 +2585,11 @@ static int netvsc_resume(struct hv_device *dev)
ret = netvsc_attach(net, device_info);
- rtnl_unlock();
-
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
net_device_ctx->saved_netvsc_dev_info = NULL;
+ rtnl_unlock();
+
return ret;
}
static const struct hv_vmbus_device_id id_table[] = {
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index e66d77d..b81ceba 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -235,7 +235,7 @@ static int rndis_filter_send_request(struct rndis_device *dev,
trace_rndis_send(dev->ndev, 0, &req->request_msg);
rcu_read_lock_bh();
- ret = netvsc_send(dev->ndev, packet, NULL, pb, NULL);
+ ret = netvsc_send(dev->ndev, packet, NULL, pb, NULL, false);
rcu_read_unlock_bh();
return ret;
--
1.8.3.1
^ permalink raw reply related
* RE: [PATCH v2 3/4] hv_utils: Support host-initiated hibernation request
From: Michael Kelley @ 2020-01-22 17:39 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB0148FDF9A3AF2352544E6DADBF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:32 PM
>
> Update the Shutdown IC version to 3.2, which is required for the host to
> send the hibernation request.
>
> The user is expected to create the below udev rule file, which is applied
> upon the host-initiated hibernation request:
>
> root@localhost:~# cat /usr/lib/udev/rules.d/40-vm-hibernation.rules
> SUBSYSTEM=="vmbus", ACTION=="change", DRIVER=="hv_utils",
> ENV{EVENT}=="hibernate", RUN+="/usr/bin/systemctl hibernate"
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_util.c | 52 +++++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 51 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index fe3a316380c2..d5216af62788 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -25,7 +25,9 @@
> #define SD_MAJOR 3
> #define SD_MINOR 0
> #define SD_MINOR_1 1
> +#define SD_MINOR_2 2
> #define SD_VERSION_3_1 (SD_MAJOR << 16 | SD_MINOR_1)
> +#define SD_VERSION_3_2 (SD_MAJOR << 16 | SD_MINOR_2)
> #define SD_VERSION (SD_MAJOR << 16 | SD_MINOR)
>
> #define SD_MAJOR_1 1
> @@ -52,9 +54,10 @@ static int sd_srv_version;
> static int ts_srv_version;
> static int hb_srv_version;
>
> -#define SD_VER_COUNT 3
> +#define SD_VER_COUNT 4
> static const int sd_versions[] = {
> SD_VERSION_3_1,
> + SD_VERSION_3_2,
I think these versions need to listed in descending order, so the new
SD_VERSION_3_2 should be listed first. Otherwise a Hyper-V host that
supports both 3.1 and 3.2 might match on 3.1 first without ever checking
for a match with 3.2.
> SD_VERSION,
> SD_VERSION_1
> };
> @@ -78,9 +81,45 @@ static const int fw_versions[] = {
> UTIL_WS2K8_FW_VERSION
> };
>
> +/*
> + * Send the "hibernate" udev event in a thread context.
> + */
> +struct hibernate_work_context {
> + struct work_struct work;
> + struct hv_device *dev;
> +};
> +
> +static struct hibernate_work_context hibernate_context;
> +static bool execute_hibernate;
> +
> +static void send_hibernate_uevent(struct work_struct *work)
> +{
> + char *uevent_env[2] = { "EVENT=hibernate", NULL };
> + struct hibernate_work_context *ctx;
> +
> + ctx = container_of(work, struct hibernate_work_context, work);
> +
> + kobject_uevent_env(&ctx->dev->device.kobj, KOBJ_CHANGE, uevent_env);
> +
> + pr_info("Sent hibernation uevent\n");
> +}
> +
> +static int hv_shutdown_init(struct hv_util_service *srv)
> +{
> + struct vmbus_channel *channel = srv->channel;
> +
> + INIT_WORK(&hibernate_context.work, send_hibernate_uevent);
> + hibernate_context.dev = channel->device_obj;
> +
> + execute_hibernate = hv_is_hibernation_supported();
> +
> + return 0;
> +}
> +
> static void shutdown_onchannelcallback(void *context);
> static struct hv_util_service util_shutdown = {
> .util_cb = shutdown_onchannelcallback,
> + .util_init = hv_shutdown_init,
> };
>
> static int hv_timesync_init(struct hv_util_service *srv);
> @@ -187,6 +226,17 @@ static void shutdown_onchannelcallback(void *context)
>
> schedule_work(&restart_work);
> break;
> + case 4:
> + case 5:
As before, I'm wondering about the interpretation of these numbers.
> + pr_info("Hibernation request received\n");
> +
> + if (execute_hibernate) {
> + icmsghdrp->status = HV_S_OK;
> + schedule_work(&hibernate_context.work);
Same comment here about the ordering of the schedule_work() call and the
sending of the response message. Seems like the code should be consistent
for all three cases -- shutdown, restart, and hibernate.
> + } else {
> + icmsghdrp->status = HV_E_FAIL;
> + }
> + break;
> default:
> icmsghdrp->status = HV_E_FAIL;
> execute_shutdown = false;
> --
> 2.19.1
^ permalink raw reply
* RE: [PATCH v2 4/4] hv_utils: Add the support of hibernation
From: Michael Kelley @ 2020-01-22 18:27 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB0148B7D12E62DD559A5D2B9DBF350@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Sunday, January 12, 2020 10:32 PM
>
> Add util_pre_suspend() and util_pre_resume() for some hv_utils devices
> (e.g. kvp/vss/fcopy), because they need special handling before
> util_suspend() calls vmbus_close().
>
> For kvp, all the possible pending work items should be cancelled.
>
> For vss and fcopy, extra clean-up needs to be done, i.e. fake a THAW
> message for hv_vss_daemon and fake a CANCEL_FCOPY message for
> hv_fcopy_daemonemon, otherwise when the VM resums back, the daemons
> can end up in an inconsistent state (i.e. the file systems are
> frozen but will never be thawed; the file transmitted via fcopy
> may not be complete). Note: there is an extra patch for the daemons:
> "Tools: hv: Reopen the devices if read() or write() returns errors",
> because the hv_utils driver can not guarantee the whole transaction
> finishes completely once util_suspend() starts to run (at this time,
> all the userspace processes are frozen).
>
> util_probe() disables channel->callback_event to avoid the race with
> the the channel callback.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> ---
> drivers/hv/hv_fcopy.c | 58 ++++++++++++++++++++++++++++++++++++-
> drivers/hv/hv_kvp.c | 44 ++++++++++++++++++++++++++--
> drivers/hv/hv_snapshot.c | 60 +++++++++++++++++++++++++++++++++++++--
> drivers/hv/hv_util.c | 60 ++++++++++++++++++++++++++++++++++++++-
> drivers/hv/hyperv_vmbus.h | 6 ++++
> include/linux/hyperv.h | 2 ++
> 6 files changed, 224 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/hv/hv_fcopy.c b/drivers/hv/hv_fcopy.c
> index 08fa4a5de644..d63853f16356 100644
> --- a/drivers/hv/hv_fcopy.c
> +++ b/drivers/hv/hv_fcopy.c
> @@ -346,9 +346,65 @@ int hv_fcopy_init(struct hv_util_service *srv)
> return 0;
> }
>
> +static void hv_fcopy_cancel_work(void)
> +{
> + cancel_delayed_work_sync(&fcopy_timeout_work);
> + cancel_work_sync(&fcopy_send_work);
> +}
> +
> +int hv_fcopy_pre_suspend(void)
> +{
> + struct vmbus_channel *channel = fcopy_transaction.recv_channel;
> + struct hv_fcopy_hdr *fcopy_msg;
> +
> + tasklet_disable(&channel->callback_event);
> +
> + /*
> + * Fake a CANCEL_FCOPY message to the user space daemon in case the
> + * daemon is in the middle of copying some file. It doesn't matter if
> + * there is already a message pending to be delivered to the user
> + * space: we force fcopy_transaction.state to be HVUTIL_READY, so the
> + * user space daemon's write() will fail with -EINVAL (see
> + * fcopy_on_msg()), and the daemon will reset the device by closing and
> + * re-opening it.
> + */
> + fcopy_msg = kzalloc(sizeof(*fcopy_msg), GFP_KERNEL);
> + if (!fcopy_msg)
> + goto err;
> +
> + fcopy_msg->operation = CANCEL_FCOPY;
> +
> + hv_fcopy_cancel_work();
> +
> + /* We don't care about the return value. */
> + hvutil_transport_send(hvt, fcopy_msg, sizeof(*fcopy_msg), NULL);
> +
> + kfree(fcopy_msg);
> +
> + fcopy_transaction.state = HVUTIL_READY;
Is the ordering correct here? It seems like the fcopy daemon could receive
the cancel message and do the write before the state is forced to
HVUTIL_READY.
> +
> + /* tasklet_enable() will be called in hv_fcopy_pre_resume(). */
> +
> + return 0;
> +err:
> + tasklet_enable(&channel->callback_event);
A nit, but if you did the memory allocation first, you could return -ENOMEM
immediately on error and avoid the err: label and re-enabling the tasklet.
> + return -ENOMEM;
> +}
> +
> +int hv_fcopy_pre_resume(void)
> +{
> + struct vmbus_channel *channel = fcopy_transaction.recv_channel;
> +
> + tasklet_enable(&channel->callback_event);
> +
> + return 0;
> +}
> +
> void hv_fcopy_deinit(void)
> {
> fcopy_transaction.state = HVUTIL_DEVICE_DYING;
> - cancel_delayed_work_sync(&fcopy_timeout_work);
> +
> + hv_fcopy_cancel_work();
> +
> hvutil_transport_destroy(hvt);
> }
> diff --git a/drivers/hv/hv_kvp.c b/drivers/hv/hv_kvp.c
> index ae7c028dc5a8..ca03f68df5d0 100644
> --- a/drivers/hv/hv_kvp.c
> +++ b/drivers/hv/hv_kvp.c
> @@ -758,11 +758,51 @@ hv_kvp_init(struct hv_util_service *srv)
> return 0;
> }
>
> -void hv_kvp_deinit(void)
> +static void hv_kvp_cancel_work(void)
> {
> - kvp_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(&kvp_host_handshake_work);
> cancel_delayed_work_sync(&kvp_timeout_work);
> cancel_work_sync(&kvp_sendkey_work);
> +}
> +
> +int hv_kvp_pre_suspend(void)
> +{
> + struct vmbus_channel *channel = kvp_transaction.recv_channel;
> +
> + tasklet_disable(&channel->callback_event);
> +
> + /*
> + * If there is a pending transtion, it's unnecessary to tell the host
> + * that the tranction will fail, becasue that is implied when
> + * util_suspend() calls vmbus_close() later.
> + */
> + hv_kvp_cancel_work();
> +
> + /*
> + * Forece the state to READY to handle the ICMSGTYPE_NEGOTIATE message
> + * later. The user space daemon may go out of order and its write()
> + * may get an EINVAL error: this doesn't matter since the daemon will
> + * reset the device by closing and re-opening the device.
> + */
> + kvp_transaction.state = HVUTIL_READY;
> +
> + return 0;
> +}
> +
> +int hv_kvp_pre_resume(void)
> +{
> + struct vmbus_channel *channel = kvp_transaction.recv_channel;
> +
> + tasklet_enable(&channel->callback_event);
> +
> + return 0;
> +}
> +
> +void hv_kvp_deinit(void)
> +{
> + kvp_transaction.state = HVUTIL_DEVICE_DYING;
> +
> + hv_kvp_cancel_work();
> +
> hvutil_transport_destroy(hvt);
> }
> diff --git a/drivers/hv/hv_snapshot.c b/drivers/hv/hv_snapshot.c
> index 03b6454268b3..eb766ff8841b 100644
> --- a/drivers/hv/hv_snapshot.c
> +++ b/drivers/hv/hv_snapshot.c
> @@ -229,6 +229,7 @@ static void vss_handle_request(struct work_struct *dummy)
> vss_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
> vss_send_op();
> return;
> +
Gratuitous blank line added?
> case VSS_OP_GET_DM_INFO:
> vss_transaction.msg->dm_info.flags = 0;
> break;
> @@ -379,10 +380,65 @@ hv_vss_init(struct hv_util_service *srv)
> return 0;
> }
>
> -void hv_vss_deinit(void)
> +static void hv_vss_cancel_work(void)
> {
> - vss_transaction.state = HVUTIL_DEVICE_DYING;
> cancel_delayed_work_sync(&vss_timeout_work);
> cancel_work_sync(&vss_handle_request_work);
> +}
> +
> +int hv_vss_pre_suspend(void)
> +{
> + struct vmbus_channel *channel = vss_transaction.recv_channel;
> + struct hv_vss_msg *vss_msg;
> +
> + tasklet_disable(&channel->callback_event);
> +
> + /*
> + * Fake a THAW message for the user space daemon in case the daemon
> + * has frozen the file systems. It doesn't matter if there is already
> + * a message pending to be delivered to the user space: we force
> + * vss_transaction.state to be HVUTIL_READY, so the user space daemon's
> + * write() will fail with -EINVAL (see vss_on_msg()), and the daemon
> + * will reset the device by closing and re-opening it.
> + */
> + vss_msg = kzalloc(sizeof(*vss_msg), GFP_KERNEL);
> + if (!vss_msg)
> + goto err;
> +
> + vss_msg->vss_hdr.operation = VSS_OP_THAW;
> +
> + /* Cancel any possible pending work. */
> + hv_vss_cancel_work();
> +
> + /* We don't care about the return value. */
> + hvutil_transport_send(hvt, vss_msg, sizeof(*vss_msg), NULL);
> +
> + kfree(vss_msg);
> +
> + vss_transaction.state = HVUTIL_READY;
Same comment about the ordering.
> +
> + /* tasklet_enable() will be called in hv_vss_pre_resume(). */
> +
> + return 0;
> +err:
> + tasklet_enable(&channel->callback_event);
> + return -ENOMEM;
Same comment about simplifying the error handling applies here.
> +}
> +
> +int hv_vss_pre_resume(void)
> +{
> + struct vmbus_channel *channel = vss_transaction.recv_channel;
> +
> + tasklet_enable(&channel->callback_event);
> +
> + return 0;
> +}
> +
> +void hv_vss_deinit(void)
> +{
> + vss_transaction.state = HVUTIL_DEVICE_DYING;
> +
> + hv_vss_cancel_work();
> +
> hvutil_transport_destroy(hvt);
> }
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index d5216af62788..255faa3d657c 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -123,12 +123,14 @@ static struct hv_util_service util_shutdown = {
> };
>
> static int hv_timesync_init(struct hv_util_service *srv);
> +static int hv_timesync_pre_suspend(void);
> static void hv_timesync_deinit(void);
>
> static void timesync_onchannelcallback(void *context);
> static struct hv_util_service util_timesynch = {
> .util_cb = timesync_onchannelcallback,
> .util_init = hv_timesync_init,
> + .util_pre_suspend = hv_timesync_pre_suspend,
> .util_deinit = hv_timesync_deinit,
> };
>
> @@ -140,18 +142,24 @@ static struct hv_util_service util_heartbeat = {
> static struct hv_util_service util_kvp = {
> .util_cb = hv_kvp_onchannelcallback,
> .util_init = hv_kvp_init,
> + .util_pre_suspend = hv_kvp_pre_suspend,
> + .util_pre_resume = hv_kvp_pre_resume,
> .util_deinit = hv_kvp_deinit,
> };
>
> static struct hv_util_service util_vss = {
> .util_cb = hv_vss_onchannelcallback,
> .util_init = hv_vss_init,
> + .util_pre_suspend = hv_vss_pre_suspend,
> + .util_pre_resume = hv_vss_pre_resume,
> .util_deinit = hv_vss_deinit,
> };
>
> static struct hv_util_service util_fcopy = {
> .util_cb = hv_fcopy_onchannelcallback,
> .util_init = hv_fcopy_init,
> + .util_pre_suspend = hv_fcopy_pre_suspend,
> + .util_pre_resume = hv_fcopy_pre_resume,
> .util_deinit = hv_fcopy_deinit,
> };
>
> @@ -512,6 +520,41 @@ static int util_remove(struct hv_device *dev)
> return 0;
> }
>
> +static int util_suspend(struct hv_device *dev)
> +{
> + struct hv_util_service *srv = hv_get_drvdata(dev);
> + int ret = 0;
> +
> + if (srv->util_pre_suspend) {
> + ret = srv->util_pre_suspend();
> +
Unneeded blank line?
> + if (ret)
> + return ret;
> + }
> +
> + vmbus_close(dev->channel);
> +
> + return 0;
> +}
> +
> +static int util_resume(struct hv_device *dev)
> +{
> + struct hv_util_service *srv = hv_get_drvdata(dev);
> + int ret = 0;
> +
> + if (srv->util_pre_resume) {
> + ret = srv->util_pre_resume();
> +
Unneeded blank line?
> + if (ret)
> + return ret;
> + }
> +
> + ret = vmbus_open(dev->channel, 4 * HV_HYP_PAGE_SIZE,
> + 4 * HV_HYP_PAGE_SIZE, NULL, 0, srv->util_cb,
> + dev->channel);
> + return ret;
> +}
> +
> static const struct hv_vmbus_device_id id_table[] = {
> /* Shutdown guid */
> { HV_SHUTDOWN_GUID,
> @@ -548,6 +591,8 @@ static struct hv_driver util_drv = {
> .id_table = id_table,
> .probe = util_probe,
> .remove = util_remove,
> + .suspend = util_suspend,
> + .resume = util_resume,
> .driver = {
> .probe_type = PROBE_PREFER_ASYNCHRONOUS,
> },
> @@ -617,11 +662,24 @@ static int hv_timesync_init(struct hv_util_service *srv)
> return 0;
> }
>
> +static void hv_timesync_cancel_work(void)
> +{
> + cancel_work_sync(&adj_time_work);
> +}
> +
> +static int hv_timesync_pre_suspend(void)
> +{
> + hv_timesync_cancel_work();
> +
> + return 0;
> +}
> +
> static void hv_timesync_deinit(void)
> {
> if (hv_ptp_clock)
> ptp_clock_unregister(hv_ptp_clock);
> - cancel_work_sync(&adj_time_work);
> +
> + hv_timesync_cancel_work();
> }
>
> static int __init init_hyperv_utils(void)
> diff --git a/drivers/hv/hyperv_vmbus.h b/drivers/hv/hyperv_vmbus.h
> index 20edcfd3b96c..f5fa3b3c9baf 100644
> --- a/drivers/hv/hyperv_vmbus.h
> +++ b/drivers/hv/hyperv_vmbus.h
> @@ -352,14 +352,20 @@ void vmbus_on_msg_dpc(unsigned long data);
>
> int hv_kvp_init(struct hv_util_service *srv);
> void hv_kvp_deinit(void);
> +int hv_kvp_pre_suspend(void);
> +int hv_kvp_pre_resume(void);
> void hv_kvp_onchannelcallback(void *context);
>
> int hv_vss_init(struct hv_util_service *srv);
> void hv_vss_deinit(void);
> +int hv_vss_pre_suspend(void);
> +int hv_vss_pre_resume(void);
> void hv_vss_onchannelcallback(void *context);
>
> int hv_fcopy_init(struct hv_util_service *srv);
> void hv_fcopy_deinit(void);
> +int hv_fcopy_pre_suspend(void);
> +int hv_fcopy_pre_resume(void);
> void hv_fcopy_onchannelcallback(void *context);
> void vmbus_initiate_unload(bool crash);
>
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 41c58011431e..692c89ccf5df 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -1435,6 +1435,8 @@ struct hv_util_service {
> void (*util_cb)(void *);
> int (*util_init)(struct hv_util_service *);
> void (*util_deinit)(void);
> + int (*util_pre_suspend)(void);
> + int (*util_pre_resume)(void);
> };
>
> struct vmbuspipe_hdr {
> --
> 2.19.1
^ permalink raw reply
* Re: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Jesper Dangaard Brouer @ 2020-01-22 19:51 UTC (permalink / raw)
To: Haiyang Zhang
Cc: brouer, sashal, linux-hyperv, netdev, kys, sthemmin, olaf,
vkuznets, davem, linux-kernel, Ilias Apalodimas
In-Reply-To: <1579713814-36061-2-git-send-email-haiyangz@microsoft.com>
On Wed, 22 Jan 2020 09:23:33 -0800
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> + struct xdp_buff *xdp)
> +{
> + void *data = nvchan->rsc.data[0];
> + u32 len = nvchan->rsc.len[0];
> + struct page *page = NULL;
> + struct bpf_prog *prog;
> + u32 act = XDP_PASS;
> +
> + xdp->data_hard_start = NULL;
> +
> + rcu_read_lock();
> + prog = rcu_dereference(nvchan->bpf_prog);
> +
> + if (!prog)
> + goto out;
> +
> + /* allocate page buffer for data */
> + page = alloc_page(GFP_ATOMIC);
The alloc_page() + __free_page() alone[1] cost 231 cycles(tsc) 64.395 ns.
Thus, the XDP_DROP case will already be limited to just around 10Gbit/s
14.88 Mpps (67.2ns).
XDP is suppose to be done for performance reasons. This looks like a
slowdown.
Measurement tool:
[1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/mm/bench/page_bench01.c
> + if (!page) {
> + act = XDP_DROP;
> + goto out;
> + }
> +
> + xdp->data_hard_start = page_address(page);
> + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> + xdp_set_data_meta_invalid(xdp);
> + xdp->data_end = xdp->data + len;
> + xdp->rxq = &nvchan->xdp_rxq;
> + xdp->handle = 0;
> +
> + memcpy(xdp->data, data, len);
And a memcpy.
> +
> + act = bpf_prog_run_xdp(prog, xdp);
> +
> + switch (act) {
> + case XDP_PASS:
> + case XDP_TX:
> + case XDP_DROP:
> + break;
> +
> + case XDP_ABORTED:
> + trace_xdp_exception(ndev, prog, act);
> + break;
> +
> + default:
> + bpf_warn_invalid_xdp_action(act);
> + }
> +
> +out:
> + rcu_read_unlock();
> +
> + if (page && act != XDP_PASS && act != XDP_TX) {
> + __free_page(page);
Given this runs under NAPI you could optimize this easily for XDP_DROP
(and XDP_ABORTED) by recycling the page in a driver local cache. (The
page_pool also have a driver local cache build in, but it might be
overkill to use page_pool in this simple case).
You could do this in a followup patch.
> + xdp->data_hard_start = NULL;
> + }
> +
> + return act;
> +}
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
^ permalink raw reply
* RE: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-22 20:29 UTC (permalink / raw)
To: Jesper Dangaard Brouer
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, Ilias Apalodimas
In-Reply-To: <20200122205133.00688f7c@carbon>
> -----Original Message-----
> From: Jesper Dangaard Brouer <brouer@redhat.com>
> Sent: Wednesday, January 22, 2020 2:52 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: brouer@redhat.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;
> Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Subject: Re: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
>
> On Wed, 22 Jan 2020 09:23:33 -0800
> Haiyang Zhang <haiyangz@microsoft.com> wrote:
>
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel *nvchan,
> > + struct xdp_buff *xdp)
> > +{
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + struct page *page = NULL;
> > + struct bpf_prog *prog;
> > + u32 act = XDP_PASS;
> > +
> > + xdp->data_hard_start = NULL;
> > +
> > + rcu_read_lock();
> > + prog = rcu_dereference(nvchan->bpf_prog);
> > +
> > + if (!prog)
> > + goto out;
> > +
> > + /* allocate page buffer for data */
> > + page = alloc_page(GFP_ATOMIC);
>
> The alloc_page() + __free_page() alone[1] cost 231 cycles(tsc) 64.395 ns.
> Thus, the XDP_DROP case will already be limited to just around 10Gbit/s
> 14.88 Mpps (67.2ns).
>
> XDP is suppose to be done for performance reasons. This looks like a slowdown.
>
> Measurement tool:
> [1]
> https://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.co
> m%2Fnetoptimizer%2Fprototype-
> kernel%2Fblob%2Fmaster%2Fkernel%2Fmm%2Fbench%2Fpage_bench01.c&am
> p;data=02%7C01%7Chaiyangz%40microsoft.com%7C681b5b13e50448d098d408
> d79f748522%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63715319
> 5109318994&sdata=pncqYIWm1yS5rDf%2BAIbWgskycmuofzl09yA1QmsRb
> M0%3D&reserved=0
On synthetic data path (netvsc), the per channel throughput is much slower than
10Gbps, because of the host side software based vSwitch. Also in most VMs on
Azure, Accelerated Network (SRIOV) is enabled. So the alloc_page() overhead on
synthetic data path won't impact performance significantly.
>
> > + if (!page) {
> > + act = XDP_DROP;
> > + goto out;
> > + }
> > +
> > + xdp->data_hard_start = page_address(page);
> > + xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
> > + xdp_set_data_meta_invalid(xdp);
> > + xdp->data_end = xdp->data + len;
> > + xdp->rxq = &nvchan->xdp_rxq;
> > + xdp->handle = 0;
> > +
> > + memcpy(xdp->data, data, len);
>
> And a memcpy.
As in the commit log:
The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
for XDP. We thought about re-use the RNDIS header space, but it's too
small. So we decided to copy the packets to a page buffer for XDP. And,
most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
most of the packets run on VF NIC. The synthetic NIC is considered as a
fallback data-path. So the data copy on netvsc won't impact performance
significantly.
>
> > +
> > + act = bpf_prog_run_xdp(prog, xdp);
> > +
> > + switch (act) {
> > + case XDP_PASS:
> > + case XDP_TX:
> > + case XDP_DROP:
> > + break;
> > +
> > + case XDP_ABORTED:
> > + trace_xdp_exception(ndev, prog, act);
> > + break;
> > +
> > + default:
> > + bpf_warn_invalid_xdp_action(act);
> > + }
> > +
> > +out:
> > + rcu_read_unlock();
> > +
> > + if (page && act != XDP_PASS && act != XDP_TX) {
> > + __free_page(page);
>
> Given this runs under NAPI you could optimize this easily for XDP_DROP (and
> XDP_ABORTED) by recycling the page in a driver local cache. (The page_pool
> also have a driver local cache build in, but it might be overkill to use page_pool
> in this simple case).
>
> You could do this in a followup patch.
I will do the optimization in a follow-up patch.
Thanks,
- Haiyang
^ permalink raw reply
* RE: [Patch v4 1/2] PCI: hv: Decouple the func definition in hv_dr_state from VSP message
From: Long Li @ 2020-01-22 21:42 UTC (permalink / raw)
To: longli@linuxonhyperv.com, KY Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Lorenzo Pieralisi, Andrew Murray,
Bjorn Helgaas, linux-hyperv@vger.kernel.org,
linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1578946101-74036-1-git-send-email-longli@linuxonhyperv.com>
Hi Lorenzo,
Can you take a look at this patch?
Thanks
Long
>Subject: [Patch v4 1/2] PCI: hv: Decouple the func definition in hv_dr_state from
>VSP message
>
>From: Long Li <longli@microsoft.com>
>
>hv_dr_state is used to find present PCI devices on the bus. The structure reuses
>struct pci_function_description from VSP message to describe a device.
>
>To prepare support for pci_function_description v2, decouple this dependence in
>hv_dr_state so it can work with both v1 and v2 VSP messages.
>
>There is no functionality change.
>
>Signed-off-by: Long Li <longli@microsoft.com>
>Reviewed-by: Michael Kelley <mikelley@microsoft.com>
>---
>Changes
>v2: Changed some spaces to tabs, changed failure code to -ENOMEM
>v3: Revised comment for function hv_pci_devices_present(), reformatted patch
>title
>v4: Fixed spelling
>
> drivers/pci/controller/pci-hyperv.c | 101 +++++++++++++++++++---------
> 1 file changed, 70 insertions(+), 31 deletions(-)
>
>diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-
>hyperv.c
>index f1f300218fab..3b3e1967cf08 100644
>--- a/drivers/pci/controller/pci-hyperv.c
>+++ b/drivers/pci/controller/pci-hyperv.c
>@@ -507,10 +507,24 @@ struct hv_dr_work {
> struct hv_pcibus_device *bus;
> };
>
>+struct hv_pcidev_description {
>+ u16 v_id; /* vendor ID */
>+ u16 d_id; /* device ID */
>+ u8 rev;
>+ u8 prog_intf;
>+ u8 subclass;
>+ u8 base_class;
>+ u32 subsystem_id;
>+ union win_slot_encoding win_slot;
>+ u32 ser; /* serial number */
>+ u32 flags;
>+ u16 virtual_numa_node;
>+};
>+
> struct hv_dr_state {
> struct list_head list_entry;
> u32 device_count;
>- struct pci_function_description func[0];
>+ struct hv_pcidev_description func[0];
> };
>
> enum hv_pcichild_state {
>@@ -527,7 +541,7 @@ struct hv_pci_dev {
> refcount_t refs;
> enum hv_pcichild_state state;
> struct pci_slot *pci_slot;
>- struct pci_function_description desc;
>+ struct hv_pcidev_description desc;
> bool reported_missing;
> struct hv_pcibus_device *hbus;
> struct work_struct wrk;
>@@ -1862,7 +1876,7 @@ static void q_resource_requirements(void *context,
>struct pci_response *resp,
> * Return: Pointer to the new tracking struct
> */
> static struct hv_pci_dev *new_pcichild_device(struct hv_pcibus_device *hbus,
>- struct pci_function_description *desc)
>+ struct hv_pcidev_description *desc)
> {
> struct hv_pci_dev *hpdev;
> struct pci_child_message *res_req;
>@@ -1973,7 +1987,7 @@ static void pci_devices_present_work(struct
>work_struct *work) {
> u32 child_no;
> bool found;
>- struct pci_function_description *new_desc;
>+ struct hv_pcidev_description *new_desc;
> struct hv_pci_dev *hpdev;
> struct hv_pcibus_device *hbus;
> struct list_head removed;
>@@ -2090,43 +2104,26 @@ static void pci_devices_present_work(struct
>work_struct *work)
> put_hvpcibus(hbus);
> kfree(dr);
> }
>-
> /**
>- * hv_pci_devices_present() - Handles list of new children
>+ * hv_pci_start_relations_work() - Queue work to start device discovery
> * @hbus: Root PCI bus, as understood by this driver
>- * @relations: Packet from host listing children
>+ * @dr: The list of children returned from host
> *
>- * This function is invoked whenever a new list of devices for
>- * this bus appears.
>+ * Return: 0 on success, -errno on failure
> */
>-static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
>- struct pci_bus_relations *relations)
>+static int hv_pci_start_relations_work(struct hv_pcibus_device *hbus,
>+ struct hv_dr_state *dr)
> {
>- struct hv_dr_state *dr;
> struct hv_dr_work *dr_wrk;
>- unsigned long flags;
> bool pending_dr;
>+ unsigned long flags;
>
> dr_wrk = kzalloc(sizeof(*dr_wrk), GFP_NOWAIT);
> if (!dr_wrk)
>- return;
>-
>- dr = kzalloc(offsetof(struct hv_dr_state, func) +
>- (sizeof(struct pci_function_description) *
>- (relations->device_count)), GFP_NOWAIT);
>- if (!dr) {
>- kfree(dr_wrk);
>- return;
>- }
>+ return -ENOMEM;
>
> INIT_WORK(&dr_wrk->wrk, pci_devices_present_work);
> dr_wrk->bus = hbus;
>- dr->device_count = relations->device_count;
>- if (dr->device_count != 0) {
>- memcpy(dr->func, relations->func,
>- sizeof(struct pci_function_description) *
>- dr->device_count);
>- }
>
> spin_lock_irqsave(&hbus->device_list_lock, flags);
> /*
>@@ -2144,6 +2141,47 @@ static void hv_pci_devices_present(struct
>hv_pcibus_device *hbus,
> get_hvpcibus(hbus);
> queue_work(hbus->wq, &dr_wrk->wrk);
> }
>+
>+ return 0;
>+}
>+
>+/**
>+ * hv_pci_devices_present() - Handle list of new children
>+ * @hbus: Root PCI bus, as understood by this driver
>+ * @relations: Packet from host listing children
>+ *
>+ * Process a new list of devices on the bus. The list of devices is
>+ * discovered by VSP and sent to us via VSP message PCI_BUS_RELATIONS,
>+ * whenever a new list of devices for this bus appears.
>+ */
>+static void hv_pci_devices_present(struct hv_pcibus_device *hbus,
>+ struct pci_bus_relations *relations) {
>+ struct hv_dr_state *dr;
>+ int i;
>+
>+ dr = kzalloc(offsetof(struct hv_dr_state, func) +
>+ (sizeof(struct hv_pcidev_description) *
>+ (relations->device_count)), GFP_NOWAIT);
>+
>+ if (!dr)
>+ return;
>+
>+ dr->device_count = relations->device_count;
>+ for (i = 0; i < dr->device_count; i++) {
>+ dr->func[i].v_id = relations->func[i].v_id;
>+ dr->func[i].d_id = relations->func[i].d_id;
>+ dr->func[i].rev = relations->func[i].rev;
>+ dr->func[i].prog_intf = relations->func[i].prog_intf;
>+ dr->func[i].subclass = relations->func[i].subclass;
>+ dr->func[i].base_class = relations->func[i].base_class;
>+ dr->func[i].subsystem_id = relations->func[i].subsystem_id;
>+ dr->func[i].win_slot = relations->func[i].win_slot;
>+ dr->func[i].ser = relations->func[i].ser;
>+ }
>+
>+ if (hv_pci_start_relations_work(hbus, dr))
>+ kfree(dr);
> }
>
> /**
>@@ -3018,7 +3056,7 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
> struct pci_packet teardown_packet;
> u8 buffer[sizeof(struct pci_message)];
> } pkt;
>- struct pci_bus_relations relations;
>+ struct hv_dr_state *dr;
> struct hv_pci_compl comp_pkt;
> int ret;
>
>@@ -3030,8 +3068,9 @@ static void hv_pci_bus_exit(struct hv_device *hdev)
> return;
>
> /* Delete any children which might still exist. */
>- memset(&relations, 0, sizeof(relations));
>- hv_pci_devices_present(hbus, &relations);
>+ dr = kzalloc(sizeof(*dr), GFP_KERNEL);
>+ if (dr && hv_pci_start_relations_work(hbus, dr))
>+ kfree(dr);
>
> ret = hv_send_resources_released(hdev);
> if (ret)
>--
>2.17.1
^ permalink raw reply
* RE: [PATCH v2 1/4] Tools: hv: Reopen the devices if read() or write() returns errors
From: Dexuan Cui @ 2020-01-23 8:10 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB1047BBD2C8C7B382D77EF981D70C0@DM5PR2101MB1047.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Wednesday, January 22, 2020 8:54 AM
> > ...
> > @@ -111,12 +113,16 @@ static int hv_copy_data(struct hv_do_fcopy
> *cpmsg)
> > static int hv_copy_finished(void)
> > {
> > close(target_fd);
> > + target_fd = -1;
> > + memset(target_fname, 0, sizeof(target_fname));
>
> I'm not completely clear on why target_fd and target_fname need to
> be reset. Could you add a comment with an explanation? Also,
After checking the code again, now I think we indeed don't need
"target_fd = -1;", but we need to reset target_fname because the kernel
hv_utils hibernation patch fakes a CANCEL_FCOPY message upon suspend,
and later when the VM resumes back the hv_fcopy_daemon may need
to handle the message by calling hv_copy_cancel(), which tries to remove
a file specified by target_fname.
If some file was copied successfully before suspend, currently
hv_copy_finished() doesn't reset target_fname; so after resume, when
the daemon handles the faked CANCEL_FCOPY message, hv_copy_cancel()
removes the file unexpectedly.
This patch resets target_fname in hv_copy_finished() to avoid the described
issue.
In case a file is not fully copied before suspend (which means hv_copy_finished()
is not called), we'd better also reset taret_fname hv_copy_cancel(), since we'd
better make sure we only try to remove the file once, when we suspend/resume
multiple times.
I'll add a comment in the next version of the patch.
> since target_fname is a null terminated string, it seems like
> target_fname[0] = 0 would be sufficient vs. zero'ing all 4096 bytes
> (PATH_MAX).
I agree. Will use this better method.
> > +reopen_fcopy_fd:
> > + hv_copy_cancel();
> > + in_handshake = 1;
> > fcopy_fd = open("/dev/vmbus/hv_fcopy", O_RDWR);
> >
> > if (fcopy_fd < 0) {
> > @@ -196,7 +205,8 @@ int main(int argc, char *argv[])
> > len = pread(fcopy_fd, &buffer, sizeof(buffer), 0);
> > if (len < 0) {
> > syslog(LOG_ERR, "pread failed: %s", strerror(errno));
> > - exit(EXIT_FAILURE);
> > + close(fcopy_fd);
> > + goto reopen_fcopy_fd;
>
> In this case and all similar cases in this patch, there may be some risk
> of getting stuck in a tight loop doing reopens if things are broken
> in some strange and bizarre way. Having an absolute limit on the
> number of reopens is potentially too restrictive as it could limit the
> number of times a VM could be hibernated and resumed. Ideally
> there could a simple rate limit on the reopens -- if it happens too frequently,
> go ahead and exit like the current code does. Thoughts?
If there is a bug, IMO it's better to get stuck in a tight loop, so the user
can notice it more quickly by the "top" command. :-)
With the patch, the daemon can get stuck in the loop only when the daemon
successfully reopens the dev file and registers itself to the kernel, but fails
to handle one of the messages. IMO the chance is pretty small, and if that
happens, there must be a bug in the hv_utils driver we need to fix, so, again,
IMO it's better to make the bug more noticeable by the tight loop. :-)
If the daemon fails to reopen the dev file or fails to register it to the kernel,
the daemon still calls exit().
> > static int vss_do_freeze(char *dir, unsigned int cmd)
> > {
> > @@ -155,8 +157,11 @@ static int vss_operate(int operation)
> > continue;
> > }
> > error |= vss_do_freeze(ent->mnt_dir, cmd);
> > - if (error && operation == VSS_OP_FREEZE)
> > - goto err;
> > + if (operation == VSS_OP_FREEZE) {
> > + if (error)
> > + goto err;
> > + fs_frozen = true;
> > + }
> > }
> >
> > endmntent(mounts);
>
> Shortly after the above code, there's code specifically to
> do the root filesystem last. It has the same error test as above,
> and it seems like it should also be setting fs_frozen = true if
> it is successful.
Yes, I missed that. Will add code for that.
> > @@ -167,6 +172,9 @@ static int vss_operate(int operation)
> > goto err;
> > }
> >
> > + if (operation == VSS_OP_THAW && !error)
> > + fs_frozen = false;
> > +
> > goto out;
> > err:
> > save_errno = errno;
> > @@ -175,6 +183,7 @@ static int vss_operate(int operation)
> > endmntent(mounts);
> > }
> > vss_operate(VSS_OP_THAW);
> > + fs_frozen = false;
> > /* Call syslog after we thaw all filesystems */
> > if (ent)
> > syslog(LOG_ERR, "FREEZE of %s failed; error:%d %s",
> > @@ -202,7 +211,7 @@ int main(int argc, char *argv[])
> > int op;
> > struct hv_vss_msg vss_msg[1];
> > int daemonize = 1, long_index = 0, opt;
> > - int in_handshake = 1;
> > + int in_handshake;
> > __u32 kernel_modver;
> >
> > static struct option long_options[] = {
> > @@ -232,6 +241,10 @@ int main(int argc, char *argv[])
> > openlog("Hyper-V VSS", 0, LOG_USER);
> > syslog(LOG_INFO, "VSS starting; pid is:%d", getpid());
> >
> > +reopen_vss_fd:
> > + if (fs_frozen)
> > + vss_operate(VSS_OP_THAW);
>
> Need to set fs_frozen = false after the above statement?
fs_frozen is set to false in vss_operate(), but there is a chance
vss_operate() may fail due to some reason, and hence fs_frozen may
remain to be true. I'll add the below code:
@@ -242,8 +242,14 @@ int main(int argc, char *argv[])
syslog(LOG_INFO, "VSS starting; pid is:%d", getpid());
reopen_vss_fd:
- if (fs_frozen)
- vss_operate(VSS_OP_THAW);
+ if (fs_frozen) {
+ if (vss_operate(VSS_OP_THAW) || fs_frozen) {
+ syslog(LOG_ERR, "failed to thaw file system: err=%d",
+ errno);
+ exit(EXIT_FAILURE);
+ }
+ }
+
in_handshake = 1;
vss_fd = open("/dev/vmbus/hv_vss", O_RDWR);
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v2 2/4] hv_utils: Support host-initiated restart request
From: Dexuan Cui @ 2020-01-23 8:11 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB104763C3447F4BB9C163F8F0D70C0@DM5PR2101MB1047.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Wednesday, January 22, 2020 9:16 AM
> >
> > To test the code, run this command on the host:
> >
> > Restart-VM $vm -Type Reboot
>
> Need a better commit message here. How about:
>
> The hv_util driver currently supports a "shutdown" operation initiated from the
> Hyper-V host. Newer versions of Hyper-V also support a "restart" operation.
> So add support for the updated protocol version that has "restart" support, and
> perform a clean reboot when such a message is received from Hyper-V.
>
> To test the restart functionality, run this PowerShell command on the Hyper-V
> host:
>
> Restart-VM <vmname> -Type Reboot
Thanks a lot! I'll use this version.
> > @@ -166,6 +179,14 @@ static void shutdown_onchannelcallback(void
> *context)
> > pr_info("Shutdown request received -"
> > " graceful shutdown initiated\n");
> > break;
> > + case 2:
> > + case 3:
>
> How are the flags values 0, 1, 2, and 3 interpreted? Perhaps a short comment
> would be helpful.
If bit 0 is 1, it means a flag of "doing the operation by force". IMO we'd like to
always perform the operation by force, even if the host doesn't set the flag -- this
is what the existing shutdown handling code does.
I'll add a comment.
>
> > + pr_info("Restart request received -"
> > + " graceful restart initiated\n");
> > + icmsghdrp->status = HV_S_OK;
> > +
> > + schedule_work(&restart_work);
> > + break;
>
> For case 0 and 1 (shutdown), the schedule_work() call is performed only
> after the response packet has been sent to the host. Is there a reason the
> new code for case 2 and 3 (restart) is doing it in the opposite order?
The channel callback runs in a tasklet context, and an active tasklet handler can
not be cancelled, so even if the "work" starts to run on another CPU immediately,
I'm sure the channel callback will send the response packet and finish normally.
This way we can save a local bool variable "execute_reboot". :-)
But, let me change the patch to follow the shutdown handling code for
consistency.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v2 3/4] hv_utils: Support host-initiated hibernation request
From: Dexuan Cui @ 2020-01-23 8:11 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB10477532BDC475656FA3817AD70C0@DM5PR2101MB1047.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> > --- a/drivers/hv/hv_util.c
> > +++ b/drivers/hv/hv_util.c
> > ...
> > static const int sd_versions[] = {
> > SD_VERSION_3_1,
> > + SD_VERSION_3_2,
>
> I think these versions need to listed in descending order, so the new
> SD_VERSION_3_2 should be listed first. Otherwise a Hyper-V host that
> supports both 3.1 and 3.2 might match on 3.1 first without ever checking
> for a match with 3.2.
Thanks! This is a rebasing mistake. Will fix it.
> > @@ -187,6 +226,17 @@ static void shutdown_onchannelcallback(void
> *context)
> >
> > schedule_work(&restart_work);
> > break;
> > + case 4:
> > + case 5:
>
> As before, I'm wondering about the interpretation of these numbers.
Will add a comment.
> > + pr_info("Hibernation request received\n");
> > +
> > + if (execute_hibernate) {
> > + icmsghdrp->status = HV_S_OK;
> > + schedule_work(&hibernate_context.work);
>
> Same comment here about the ordering of the schedule_work() call and the
> sending of the response message. Seems like the code should be consistent
> for all three cases -- shutdown, restart, and hibernate.
I agree. Will fix this.
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH v2 4/4] hv_utils: Add the support of hibernation
From: Dexuan Cui @ 2020-01-23 8:11 UTC (permalink / raw)
To: Michael Kelley, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <DM5PR2101MB104749677CD07DE42B5BD26DD70C0@DM5PR2101MB1047.namprd21.prod.outlook.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Wednesday, January 22, 2020 10:28 AM
> ...
> > +int hv_fcopy_pre_suspend(void)
> > +{
> > + struct vmbus_channel *channel = fcopy_transaction.recv_channel;
> > + struct hv_fcopy_hdr *fcopy_msg;
> > +
> > + tasklet_disable(&channel->callback_event);
> > + ...
> > + fcopy_msg = kzalloc(sizeof(*fcopy_msg), GFP_KERNEL);
> > + if (!fcopy_msg)
> > + goto err;
> > +
> > + fcopy_msg->operation = CANCEL_FCOPY;
> > +
> > + hv_fcopy_cancel_work();
> > +
> > + /* We don't care about the return value. */
> > + hvutil_transport_send(hvt, fcopy_msg, sizeof(*fcopy_msg), NULL);
> > +
> > + kfree(fcopy_msg);
> > +
> > + fcopy_transaction.state = HVUTIL_READY;
>
> Is the ordering correct here?
This is intentional. I'll add a comment (please see the below).
> It seems like the fcopy daemon could receive
> the cancel message and do the write before the state is forced to
> HVUTIL_READY.
This can not happen, because when we're here from util_suspend(), all the
userspace processes have been frozen (please refer to another mail from me
https://lkml.org/lkml/2020/1/13/1021). The userspace is thawed only after
util_resume() and the other part of the resume procedure finish.
When we're here in util_suspend(), we can be in any of the below states:
#1: hv_utils has not queued any message to the userspace daemon.
Now hv_fcopy_pre_suspend() queues a message to the daemon, and forces
the state to HVUTIL_READY; the daemon should read the message without
any error; later when the daemon calls write(), the write() returns -1 because
fcopy_transaction.state != HVUTIL_USERSPACE_REQ and fcopy_on_msg()
returns -EINVAL; the daemon responds to the write() error by closing and
re-opening the dev file, which triggers a reset in the hv_utils driver: see
hvt_op_release() -> hvt_reset() -> fcopy_on_reset(), and later the daemon
registers itself to the hv_utils driver, and everything comes back to normal.
#2: hv_utils has queued a message to the userspace daemon.
Now hv_fcopy_pre_suspend() fails to queue an extra message to the
daemon, but still forces the state to HVUTIL_READY.
#2.1 the userspace has not read the message.
The daemon reads the queued message and later the write() fails, so the
daemon closes and re-opens the dev file.
#2.2 the userspace has read the message, but has not called write() yet.
The write() fails, so the daemon closes and re-opens the dev file.
#2.3 the userspace has read the message, and has called write().
This is actualy the same as #1.
So, IMO the patch should be handling the scenarios correctly.
> > +
> > + /* tasklet_enable() will be called in hv_fcopy_pre_resume(). */
> > +
> > + return 0;
> > +err:
> > + tasklet_enable(&channel->callback_event);
>
> A nit, but if you did the memory allocation first, you could return -ENOMEM
> immediately on error and avoid the err: label and re-enabling the tasklet.
Good idea! I'll fix this.
> > + return -ENOMEM;
> > +}
> > ...
> > --- a/drivers/hv/hv_snapshot.c
> > +++ b/drivers/hv/hv_snapshot.c
> > @@ -229,6 +229,7 @@ static void vss_handle_request(struct work_struct
> *dummy)
> > vss_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
> > vss_send_op();
> > return;
> > +
>
> Gratuitous blank line added?
Will remove it. I probably tried to make the "return;" more noticeable.
> > case VSS_OP_GET_DM_INFO:
> > vss_transaction.msg->dm_info.flags = 0;
> > break;
> > ...
> > +int hv_vss_pre_suspend(void)
> > +{
> > ...
> > + /* We don't care about the return value. */
> > + hvutil_transport_send(hvt, vss_msg, sizeof(*vss_msg), NULL);
> > +
> > + kfree(vss_msg);
> > +
> > + vss_transaction.state = HVUTIL_READY;
>
> Same comment about the ordering.
I'll add a comment for this. I may add a long comment in util_suspend()
and add a short comment here.
> > +
> > + /* tasklet_enable() will be called in hv_vss_pre_resume(). */
> > +
> > + return 0;
> > +err:
> > + tasklet_enable(&channel->callback_event);
> > + return -ENOMEM;
>
> Same comment about simplifying the error handling applies here.
Will fix this.
> > +static int util_suspend(struct hv_device *dev)
> > +{
> > + struct hv_util_service *srv = hv_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (srv->util_pre_suspend) {
> > + ret = srv->util_pre_suspend();
> > +
>
> Unneeded blank line?
Will remove this.
> > + if (ret)
> > + return ret;
> > + }
> > +
> > + vmbus_close(dev->channel);
> > +
> > + return 0;
> > +}
> > +
> > +static int util_resume(struct hv_device *dev)
> > +{
> > + struct hv_util_service *srv = hv_get_drvdata(dev);
> > + int ret = 0;
> > +
> > + if (srv->util_pre_resume) {
> > + ret = srv->util_pre_resume();
> > +
>
> Unneeded blank line?
Will remove this.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH 2/2] pci: hyperv: Move retarget related struct definitions into tlfs
From: Boqun Feng @ 2020-01-23 10:40 UTC (permalink / raw)
To: Vitaly Kuznetsov
Cc: Michael Kelley, K. Y. Srinivasan, Haiyang Zhang,
Stephen Hemminger, Sasha Levin, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin,
maintainer:X86 ARCHITECTURE (32-BIT AND 64-BIT),
Lorenzo Pieralisi, Andrew Murray, Bjorn Helgaas,
open list:PCI NATIVE HOST BRIDGE AND ENDPOINT DRIVERS,
linux-hyperv, linux-kernel
In-Reply-To: <87blqxf9es.fsf@vitty.brq.redhat.com>
On Tue, Jan 21, 2020 at 10:25:47AM +0100, Vitaly Kuznetsov wrote:
[...]
> >
> > +#if IS_ENABLED(CONFIG_PCI_HYPERV)
> > +#define hv_set_msi_address_from_desc(msi_entry, msi_desc) \
> > +do { \
> > + (msi_entry)->address = (msi_desc)->msg.address_lo; \
> > +} while (0)
> > +
> > +#endif /* CONFIG_PCI_HYPERV */
>
> It seems to be pointless to put defines under #if IS_ENABLED(): in case
> it is not enabled and used you'll get a compilation error, in case it is
> enabled and not used no code is going to be generated anyways.
>
OK I will remove the #if IS_ENABLED() in the next version.
> > +
> > #else /* CONFIG_HYPERV */
> > static inline void hyperv_init(void) {}
> > static inline void hyperv_setup_mmu_ops(void) {}
> > diff --git a/drivers/pci/controller/pci-hyperv.c b/drivers/pci/controller/pci-hyperv.c
> > index aacfcc90d929..2240f2b3643e 100644
> > --- a/drivers/pci/controller/pci-hyperv.c
> > +++ b/drivers/pci/controller/pci-hyperv.c
> > @@ -406,36 +406,6 @@ struct pci_eject_response {
> >
> > static int pci_ring_size = (4 * PAGE_SIZE);
> >
> > -struct hv_interrupt_entry {
> > - u32 source; /* 1 for MSI(-X) */
> > - u32 reserved1;
> > - u32 address;
> > - u32 data;
> > -};
> > -
> > -/*
> > - * flags for hv_device_interrupt_target.flags
> > - */
> > -#define HV_DEVICE_INTERRUPT_TARGET_MULTICAST 1
> > -#define HV_DEVICE_INTERRUPT_TARGET_PROCESSOR_SET 2
> > -
> > -struct hv_device_interrupt_target {
> > - u32 vector;
> > - u32 flags;
> > - union {
> > - u64 vp_mask;
> > - struct hv_vpset vp_set;
> > - };
> > -};
> > -
> > -struct retarget_msi_interrupt {
> > - u64 partition_id; /* use "self" */
> > - u64 device_id;
> > - struct hv_interrupt_entry int_entry;
> > - u64 reserved2;
> > - struct hv_device_interrupt_target int_target;
> > -} __packed __aligned(8);
> > -
> > /*
> > * Driver specific state.
> > */
> > @@ -482,7 +452,7 @@ struct hv_pcibus_device {
> > struct workqueue_struct *wq;
> >
> > /* hypercall arg, must not cross page boundary */
> > - struct retarget_msi_interrupt retarget_msi_interrupt_params;
> > + struct hv_retarget_device_interrupt retarget_msi_interrupt_params;
> >
> > /*
> > * Don't put anything here: retarget_msi_interrupt_params must be last
> > @@ -1178,7 +1148,7 @@ static void hv_irq_unmask(struct irq_data *data)
> > {
> > struct msi_desc *msi_desc = irq_data_get_msi_desc(data);
> > struct irq_cfg *cfg = irqd_cfg(data);
> > - struct retarget_msi_interrupt *params;
> > + struct hv_retarget_device_interrupt *params;
> > struct hv_pcibus_device *hbus;
> > struct cpumask *dest;
> > cpumask_var_t tmp;
> > @@ -1200,8 +1170,8 @@ static void hv_irq_unmask(struct irq_data *data)
> > memset(params, 0, sizeof(*params));
> > params->partition_id = HV_PARTITION_ID_SELF;
> > params->int_entry.source = 1; /* MSI(-X) */
> > - params->int_entry.address = msi_desc->msg.address_lo;
> > - params->int_entry.data = msi_desc->msg.data;
> > + hv_set_msi_address_from_desc(¶ms->int_entry.msi_entry, msi_desc);
>
> I don't quite see why this hv_set_msi_address_from_desc() is needed at
> all.
>
I will add some description in the next version, the reason that
hv_set_msi_address_from_desc() is arm64 and x86 have different
hv_interrupt_entry formats. So a generic interface is needed so that
archs can have different ways to set ->address field from msi_desc.
Regards,
Boqun
> > + params->int_entry.msi_entry.data = msi_desc->msg.data;
> > params->device_id = (hbus->hdev->dev_instance.b[5] << 24) |
> > (hbus->hdev->dev_instance.b[4] << 16) |
> > (hbus->hdev->dev_instance.b[7] << 8) |
>
> --
> Vitaly
>
^ permalink raw reply
* Re: [RFC 0/6] vDSO support for Hyper-V guest on ARM64
From: Vincenzo Frascino @ 2020-01-23 10:48 UTC (permalink / raw)
To: Boqun Feng, linux-hyperv, linux-arm-kernel, linux-kernel
Cc: Sasha Levin, Stephen Hemminger, Catalin Marinas, Haiyang Zhang,
Michael Kelley, Stefano Stabellini, xen-devel, Thomas Gleixner,
K. Y. Srinivasan, Will Deacon
In-Reply-To: <20191216001922.23008-1-boqun.feng@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2527 bytes --]
Hi Boqun Feng,
sorry for the late reply.
On 16/12/2019 00:19, Boqun Feng wrote:
> Hi,
>
> This is the RFC patchset for vDSO support in ARM64 Hyper-V guest. To
> test it, Michael's ARM64 support patchset:
>
> https://lore.kernel.org/linux-arm-kernel/1570129355-16005-1-git-send-email-mikelley@microsoft.com/
>
> is needed.
>
> Similar as x86, Hyper-V on ARM64 use a TSC page for guests to read
> the virtualized hardware timer, this TSC page is read-only for the
> guests, so could be used for vDSO data page. And the vDSO (userspace)
> code could use the same code for timer reading as kernel, since
> they read the same TSC page.
>
I had a look to your patches and overall, I could not understand why we can't
use the arch_timer to do the same things you are doing with the one you
introduced in this series. What confuses me is that KVM works just fine with the
arch_timer which was designed with virtualization in mind. Why do we need
another one? Could you please explain?
> This patchset therefore extends ARM64's __vsdo_init() to allow multiple
> data pages and introduces the vclock_mode concept similar to x86 to
> allow different platforms (bare-metal, Hyper-V, etc.) to switch to
> different __arch_get_hw_counter() implementations. The rest of this
> patchset does the necessary setup for Hyper-V guests: mapping tsc page,
> enabling userspace to read cntvct, etc. to enable vDSO.
>
> This patchset consists of 6 patches:
>
> patch #1 allows hv_get_raw_timer() definition to be overridden for
> userspace and kernel to share the same hv_read_tsc_page() definition.
>
> patch #2 extends ARM64 to support multiple vDSO data pages.
>
> patch #3 introduces vclock_mode similiar to x86 to allow different
> __arch_get_hw_counter() implementations for different clocksources.
>
> patch #4 maps Hyper-V TSC page into vDSO data page.
>
> patch #5 allows userspace to read cntvct, so that userspace can
> efficiently read the clocksource.
>
> patch #6 enables the vDSO for ARM64 Hyper-V guest.
>
> The whole patchset is based on v5.5-rc1 plus Michael's ARM64 support
> patchset, and I've done a few tests with:
>
> https://github.com/nlynch-mentor/vdsotest
>
> Comments and suggestions are welcome!
>
> Regards,
> Boqun
>
> _______________________________________________
> linux-arm-kernel mailing list
> linux-arm-kernel@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
>
--
Regards,
Vincenzo
[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 14291 bytes --]
^ permalink raw reply
* RE: [PATCH v2 4/4] hv_utils: Add the support of hibernation
From: Michael Kelley @ 2020-01-23 15:31 UTC (permalink / raw)
To: Dexuan Cui, KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
sashal@kernel.org, Sasha Levin, linux-hyperv@vger.kernel.org,
vkuznets, linux-kernel@vger.kernel.org
In-Reply-To: <HK0P153MB0148D8719B52A34BDB6C5836BF0F0@HK0P153MB0148.APCP153.PROD.OUTLOOK.COM>
From: Dexuan Cui <decui@microsoft.com> Sent: Thursday, January 23, 2020 12:12 AM
>
> > From: Michael Kelley <mikelley@microsoft.com>
> > Sent: Wednesday, January 22, 2020 10:28 AM
> > ...
> > > +int hv_fcopy_pre_suspend(void)
> > > +{
> > > + struct vmbus_channel *channel = fcopy_transaction.recv_channel;
> > > + struct hv_fcopy_hdr *fcopy_msg;
> > > +
> > > + tasklet_disable(&channel->callback_event);
> > > + ...
> > > + fcopy_msg = kzalloc(sizeof(*fcopy_msg), GFP_KERNEL);
> > > + if (!fcopy_msg)
> > > + goto err;
> > > +
> > > + fcopy_msg->operation = CANCEL_FCOPY;
> > > +
> > > + hv_fcopy_cancel_work();
> > > +
> > > + /* We don't care about the return value. */
> > > + hvutil_transport_send(hvt, fcopy_msg, sizeof(*fcopy_msg), NULL);
> > > +
> > > + kfree(fcopy_msg);
> > > +
> > > + fcopy_transaction.state = HVUTIL_READY;
> >
> > Is the ordering correct here?
>
> This is intentional. I'll add a comment (please see the below).
>
> > It seems like the fcopy daemon could receive
> > the cancel message and do the write before the state is forced to
> > HVUTIL_READY.
>
> This can not happen, because when we're here from util_suspend(), all the
> userspace processes have been frozen (please refer to another mail from me
> https://lkml.org/lkml/2020/1/13/1021). The userspace is thawed only after
> util_resume() and the other part of the resume procedure finish.
Oh, right. That makes sense now.
>
> When we're here in util_suspend(), we can be in any of the below states:
>
> #1: hv_utils has not queued any message to the userspace daemon.
> Now hv_fcopy_pre_suspend() queues a message to the daemon, and forces
> the state to HVUTIL_READY; the daemon should read the message without
> any error; later when the daemon calls write(), the write() returns -1 because
> fcopy_transaction.state != HVUTIL_USERSPACE_REQ and fcopy_on_msg()
> returns -EINVAL; the daemon responds to the write() error by closing and
> re-opening the dev file, which triggers a reset in the hv_utils driver: see
> hvt_op_release() -> hvt_reset() -> fcopy_on_reset(), and later the daemon
> registers itself to the hv_utils driver, and everything comes back to normal.
>
> #2: hv_utils has queued a message to the userspace daemon.
> Now hv_fcopy_pre_suspend() fails to queue an extra message to the
> daemon, but still forces the state to HVUTIL_READY.
>
> #2.1 the userspace has not read the message.
> The daemon reads the queued message and later the write() fails, so the
> daemon closes and re-opens the dev file.
>
> #2.2 the userspace has read the message, but has not called write() yet.
> The write() fails, so the daemon closes and re-opens the dev file.
>
> #2.3 the userspace has read the message, and has called write().
> This is actualy the same as #1.
>
> So, IMO the patch should be handling the scenarios correctly.
>
> > > +
> > > + /* tasklet_enable() will be called in hv_fcopy_pre_resume(). */
> > > +
> > > + return 0;
> > > +err:
> > > + tasklet_enable(&channel->callback_event);
> >
> > A nit, but if you did the memory allocation first, you could return -ENOMEM
> > immediately on error and avoid the err: label and re-enabling the tasklet.
>
> Good idea! I'll fix this.
>
> > > + return -ENOMEM;
> > > +}
> > > ...
> > > --- a/drivers/hv/hv_snapshot.c
> > > +++ b/drivers/hv/hv_snapshot.c
> > > @@ -229,6 +229,7 @@ static void vss_handle_request(struct work_struct
> > *dummy)
> > > vss_transaction.state = HVUTIL_HOSTMSG_RECEIVED;
> > > vss_send_op();
> > > return;
> > > +
> >
> > Gratuitous blank line added?
>
> Will remove it. I probably tried to make the "return;" more noticeable.
>
> > > case VSS_OP_GET_DM_INFO:
> > > vss_transaction.msg->dm_info.flags = 0;
> > > break;
> > > ...
> > > +int hv_vss_pre_suspend(void)
> > > +{
> > > ...
> > > + /* We don't care about the return value. */
> > > + hvutil_transport_send(hvt, vss_msg, sizeof(*vss_msg), NULL);
> > > +
> > > + kfree(vss_msg);
> > > +
> > > + vss_transaction.state = HVUTIL_READY;
> >
> > Same comment about the ordering.
>
> I'll add a comment for this. I may add a long comment in util_suspend()
> and add a short comment here.
>
> > > +
> > > + /* tasklet_enable() will be called in hv_vss_pre_resume(). */
> > > +
> > > + return 0;
> > > +err:
> > > + tasklet_enable(&channel->callback_event);
> > > + return -ENOMEM;
> >
> > Same comment about simplifying the error handling applies here.
>
> Will fix this.
>
> > > +static int util_suspend(struct hv_device *dev)
> > > +{
> > > + struct hv_util_service *srv = hv_get_drvdata(dev);
> > > + int ret = 0;
> > > +
> > > + if (srv->util_pre_suspend) {
> > > + ret = srv->util_pre_suspend();
> > > +
> >
> > Unneeded blank line?
>
> Will remove this.
>
> > > + if (ret)
> > > + return ret;
> > > + }
> > > +
> > > + vmbus_close(dev->channel);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +static int util_resume(struct hv_device *dev)
> > > +{
> > > + struct hv_util_service *srv = hv_get_drvdata(dev);
> > > + int ret = 0;
> > > +
> > > + if (srv->util_pre_resume) {
> > > + ret = srv->util_pre_resume();
> > > +
> >
> > Unneeded blank line?
>
> Will remove this.
>
> Thanks,
> -- Dexuan
^ permalink raw reply
* Re: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Jakub Kicinski @ 2020-01-23 16:59 UTC (permalink / raw)
To: Haiyang Zhang
Cc: sashal, linux-hyperv, netdev, kys, sthemmin, olaf, vkuznets,
davem, linux-kernel
In-Reply-To: <1579713814-36061-2-git-send-email-haiyangz@microsoft.com>
On Wed, 22 Jan 2020 09:23:33 -0800, 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.
>
> Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
> VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
> is not recommended, also not propagated to synthetic NIC, and may be
> overwritten by setting of synthetic NIC.
>
> The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
> for XDP. We thought about re-use the RNDIS header space, but it's too
> small. So we decided to copy the packets to a page buffer for XDP. And,
> most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
> most of the packets run on VF NIC. The synthetic NIC is considered as a
> fallback data-path. So the data copy on netvsc won't impact performance
> significantly.
>
> 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_REDIRECT
>
> Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> ---
> Changes:
> v3: Minor code and comment updates.
> v2: Added XDP_TX support. Addressed review comments.
How does the locking of the TX path work? You seem to be just calling
the normal xmit method, but you don't hold the xmit queue lock, so the
stack can start xmit concurrently, no?
^ permalink raw reply
* RE: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-23 17:14 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: <20200123085906.20608707@cakuba>
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, January 23, 2020 11:59 AM
> 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 V3,net-next, 1/2] hv_netvsc: Add XDP support
>
> On Wed, 22 Jan 2020 09:23:33 -0800, 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.
> >
> > Setting / unsetting XDP program on synthetic NIC (netvsc) propagates
> > to VF NIC automatically. Setting / unsetting XDP program on VF NIC
> > directly is not recommended, also not propagated to synthetic NIC, and
> > may be overwritten by setting of synthetic NIC.
> >
> > The Azure/Hyper-V synthetic NIC receive buffer doesn't provide
> > headroom for XDP. We thought about re-use the RNDIS header space, but
> > it's too small. So we decided to copy the packets to a page buffer for
> > XDP. And, most of our VMs on Azure have Accelerated Network (SRIOV)
> > enabled, so most of the packets run on VF NIC. The synthetic NIC is
> > considered as a fallback data-path. So the data copy on netvsc won't
> > impact performance significantly.
> >
> > 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_REDIRECT
> >
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
> >
> > ---
> > Changes:
> > v3: Minor code and comment updates.
> > v2: Added XDP_TX support. Addressed review comments.
>
> How does the locking of the TX path work? You seem to be just calling the
> normal xmit method, but you don't hold the xmit queue lock, so the stack can
> start xmit concurrently, no?
The netvsc and vmbus can handle concurrent transmits, except the msd
(Multi-Send Data) field which can only be used by one queue.
I already added a new flag to netvsc_send(), so packets from XDP_TX won't use
the msd.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Jakub Kicinski @ 2020-01-23 17:30 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: <MN2PR21MB13757F7D19C11EC175FD9F98CA0F0@MN2PR21MB1375.namprd21.prod.outlook.com>
On Thu, 23 Jan 2020 17:14:06 +0000, Haiyang Zhang wrote:
> > > Changes:
> > > v3: Minor code and comment updates.
> > > v2: Added XDP_TX support. Addressed review comments.
> >
> > How does the locking of the TX path work? You seem to be just calling the
> > normal xmit method, but you don't hold the xmit queue lock, so the stack can
> > start xmit concurrently, no?
>
> The netvsc and vmbus can handle concurrent transmits, except the msd
> (Multi-Send Data) field which can only be used by one queue.
>
> I already added a new flag to netvsc_send(), so packets from XDP_TX won't use
> the msd.
I see, there's a few non-atomic counters there, but maybe that's not a
big deal.
What frees the skb if the ring is full, and netvsc_send_pkt() returns
-EAGAIN? Nothing checks the return value from netvsc_xdp_xmit().
^ permalink raw reply
* RE: [PATCH V3,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-23 17:44 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: <20200123093013.53d78485@cakuba>
> -----Original Message-----
> From: Jakub Kicinski <kuba@kernel.org>
> Sent: Thursday, January 23, 2020 12:30 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 V3,net-next, 1/2] hv_netvsc: Add XDP support
>
> On Thu, 23 Jan 2020 17:14:06 +0000, Haiyang Zhang wrote:
> > > > Changes:
> > > > v3: Minor code and comment updates.
> > > > v2: Added XDP_TX support. Addressed review comments.
> > >
> > > How does the locking of the TX path work? You seem to be just
> > > calling the normal xmit method, but you don't hold the xmit queue
> > > lock, so the stack can start xmit concurrently, no?
> >
> > The netvsc and vmbus can handle concurrent transmits, except the msd
> > (Multi-Send Data) field which can only be used by one queue.
> >
> > I already added a new flag to netvsc_send(), so packets from XDP_TX
> > won't use the msd.
>
> I see, there's a few non-atomic counters there, but maybe that's not a big deal.
Yes, those error counters are used less frequently, and not necessary to be precise.
> What frees the skb if the ring is full, and netvsc_send_pkt() returns -EAGAIN?
> Nothing checks the return value from netvsc_xdp_xmit().
Good catch! I will add skb free when -EAGAIN is returned for XDP_TX.
I will update the patch.
Thanks,
- Haiyang
^ permalink raw reply
* [PATCH V4,net-next, 0/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-23 21:52 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
Add XDP support and update related document.
Haiyang Zhang (2):
hv_netvsc: Add XDP support
hv_netvsc: Update document for XDP support
.../networking/device_drivers/microsoft/netvsc.txt | 21 +++
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 21 ++-
drivers/net/hyperv/netvsc.c | 31 ++-
drivers/net/hyperv/netvsc_bpf.c | 209 +++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 183 +++++++++++++++---
drivers/net/hyperv/rndis_filter.c | 2 +-
7 files changed, 430 insertions(+), 39 deletions(-)
create mode 100644 drivers/net/hyperv/netvsc_bpf.c
--
1.8.3.1
^ permalink raw reply
* [PATCH V4,net-next, 1/2] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2020-01-23 21:52 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579816355-6933-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.
Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
is not recommended, also not propagated to synthetic NIC, and may be
overwritten by setting of synthetic NIC.
The Azure/Hyper-V synthetic NIC receive buffer doesn't provide headroom
for XDP. We thought about re-use the RNDIS header space, but it's too
small. So we decided to copy the packets to a page buffer for XDP. And,
most of our VMs on Azure have Accelerated Network (SRIOV) enabled, so
most of the packets run on VF NIC. The synthetic NIC is considered as a
fallback data-path. So the data copy on netvsc won't impact performance
significantly.
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_REDIRECT
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
Changes:
v4: Fix handling xmit incomplete.
v3: Minor code and comment updates.
v2: Added XDP_TX support. Addressed review comments.
---
drivers/net/hyperv/Makefile | 2 +-
drivers/net/hyperv/hyperv_net.h | 21 +++-
drivers/net/hyperv/netvsc.c | 31 +++++-
drivers/net/hyperv/netvsc_bpf.c | 209 ++++++++++++++++++++++++++++++++++++++
drivers/net/hyperv/netvsc_drv.c | 183 +++++++++++++++++++++++++++------
drivers/net/hyperv/rndis_filter.c | 2 +-
6 files changed, 409 insertions(+), 39 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 dc44819..abda736 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];
};
@@ -189,7 +191,8 @@ int netvsc_send(struct net_device *net,
struct hv_netvsc_packet *packet,
struct rndis_message *rndis_msg,
struct hv_page_buffer *page_buffer,
- struct sk_buff *skb);
+ struct sk_buff *skb,
+ bool xdp_tx);
void netvsc_linkstatus_callback(struct net_device *net,
struct rndis_message *resp);
int netvsc_recv_callback(struct net_device *net,
@@ -198,6 +201,16 @@ 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,
+ struct xdp_buff *xdp);
+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 netlink_ext_ack *extack,
+ 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);
@@ -832,6 +845,8 @@ struct nvsp_message {
#define RNDIS_MAX_PKT_DEFAULT 8
#define RNDIS_PKT_ALIGN_DEFAULT 8
+#define NETVSC_XDP_HDRM 256
+
struct multi_send_data {
struct sk_buff *skb; /* skb containing the pkt */
struct hv_netvsc_packet *pkt; /* netvsc pkt pending */
@@ -867,6 +882,7 @@ struct netvsc_stats {
u64 bytes;
u64 broadcast;
u64 multicast;
+ u64 xdp_drop;
struct u64_stats_sync syncp;
};
@@ -972,6 +988,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 eab83e7..ae3f308 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);
}
@@ -900,7 +902,8 @@ int netvsc_send(struct net_device *ndev,
struct hv_netvsc_packet *packet,
struct rndis_message *rndis_msg,
struct hv_page_buffer *pb,
- struct sk_buff *skb)
+ struct sk_buff *skb,
+ bool xdp_tx)
{
struct net_device_context *ndev_ctx = netdev_priv(ndev);
struct netvsc_device *net_device
@@ -923,10 +926,11 @@ int netvsc_send(struct net_device *ndev,
packet->send_buf_index = NETVSC_INVALID_INDEX;
packet->cp_partial = false;
- /* Send control message directly without accessing msd (Multi-Send
- * Data) field which may be changed during data packet processing.
+ /* Send a control message or XDP packet directly without accessing
+ * msd (Multi-Send Data) field which may be changed during data packet
+ * processing.
*/
- if (!skb)
+ if (!skb || xdp_tx)
return netvsc_send_pkt(device, packet, net_device, pb, skb);
/* batch packets in send buffer if possible */
@@ -1392,6 +1396,21 @@ 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);
+
+ ret = xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
+
+ if (ret) {
+ netdev_err(ndev, "xdp_rxq_info_reg fail: %d\n", ret);
+ goto cleanup2;
+ }
+
+ ret = xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
+ MEM_TYPE_PAGE_SHARED, NULL);
+
+ if (ret) {
+ netdev_err(ndev, "xdp reg_mem_model fail: %d\n", ret);
+ goto cleanup2;
+ }
}
/* Enable NAPI handler before init callbacks */
@@ -1437,6 +1456,8 @@ struct netvsc_device *netvsc_device_add(struct hv_device *device,
cleanup:
netif_napi_del(&net_device->chan_table[0].napi);
+
+cleanup2:
free_netvsc_device(&net_device->rcu);
return ERR_PTR(ret);
diff --git a/drivers/net/hyperv/netvsc_bpf.c b/drivers/net/hyperv/netvsc_bpf.c
new file mode 100644
index 0000000..20adfe5
--- /dev/null
+++ b/drivers/net/hyperv/netvsc_bpf.c
@@ -0,0 +1,209 @@
+// 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,
+ struct xdp_buff *xdp)
+{
+ void *data = nvchan->rsc.data[0];
+ u32 len = nvchan->rsc.len[0];
+ struct page *page = NULL;
+ struct bpf_prog *prog;
+ u32 act = XDP_PASS;
+
+ xdp->data_hard_start = NULL;
+
+ rcu_read_lock();
+ prog = rcu_dereference(nvchan->bpf_prog);
+
+ if (!prog)
+ goto out;
+
+ /* allocate page buffer for data */
+ page = alloc_page(GFP_ATOMIC);
+ if (!page) {
+ act = XDP_DROP;
+ goto out;
+ }
+
+ xdp->data_hard_start = page_address(page);
+ xdp->data = xdp->data_hard_start + NETVSC_XDP_HDRM;
+ xdp_set_data_meta_invalid(xdp);
+ xdp->data_end = xdp->data + len;
+ xdp->rxq = &nvchan->xdp_rxq;
+ xdp->handle = 0;
+
+ memcpy(xdp->data, data, len);
+
+ act = bpf_prog_run_xdp(prog, xdp);
+
+ switch (act) {
+ case XDP_PASS:
+ case XDP_TX:
+ case XDP_DROP:
+ break;
+
+ case XDP_ABORTED:
+ trace_xdp_exception(ndev, prog, act);
+ break;
+
+ default:
+ bpf_warn_invalid_xdp_action(act);
+ }
+
+out:
+ rcu_read_unlock();
+
+ if (page && act != XDP_PASS && act != XDP_TX) {
+ __free_page(page);
+ xdp->data_hard_start = NULL;
+ }
+
+ 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 netlink_ext_ack *extack,
+ struct netvsc_device *nvdev)
+{
+ struct bpf_prog *old_prog;
+ int buf_max, i;
+
+ old_prog = netvsc_xdp_get(nvdev);
+
+ if (!old_prog && !prog)
+ return 0;
+
+ buf_max = NETVSC_XDP_HDRM + netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
+ if (prog && buf_max > PAGE_SIZE) {
+ netdev_err(dev, "XDP: mtu:%u too large, buf_max:%u\n",
+ dev->mtu, buf_max);
+ NL_SET_ERR_MSG_MOD(extack, "XDP: mtu too large");
+
+ return -EOPNOTSUPP;
+ }
+
+ if (prog && (dev->features & NETIF_F_LRO)) {
+ netdev_err(dev, "XDP: not support LRO\n");
+ NL_SET_ERR_MSG_MOD(extack, "XDP: not support LRO");
+
+ return -EOPNOTSUPP;
+ }
+
+ if (prog)
+ bpf_prog_add(prog, nvdev->num_chn);
+
+ 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);
+ struct netlink_ext_ack *extack = bpf->extack;
+ 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, extack, nvdev);
+
+ if (ret)
+ return ret;
+
+ ret = netvsc_vf_setxdp(vf_netdev, bpf->prog);
+
+ if (ret) {
+ netdev_err(dev, "vf_setxdp failed:%d\n", ret);
+ NL_SET_ERR_MSG_MOD(extack, "vf_setxdp failed");
+
+ netvsc_xdp_set(dev, NULL, extack, 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 f3f9eb8..8fc71bd 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>
@@ -519,7 +520,7 @@ static int netvsc_vf_xmit(struct net_device *net, struct net_device *vf_netdev,
return rc;
}
-static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
+static int netvsc_xmit(struct sk_buff *skb, struct net_device *net, bool xdp_tx)
{
struct net_device_context *net_device_ctx = netdev_priv(net);
struct hv_netvsc_packet *packet = NULL;
@@ -686,7 +687,7 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
/* timestamp packet in software */
skb_tx_timestamp(skb);
- ret = netvsc_send(net, packet, rndis_msg, pb, skb);
+ ret = netvsc_send(net, packet, rndis_msg, pb, skb, xdp_tx);
if (likely(ret == 0))
return NETDEV_TX_OK;
@@ -709,6 +710,11 @@ static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *net)
goto drop;
}
+static int netvsc_start_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ return netvsc_xmit(skb, ndev, false);
+}
+
/*
* netvsc_linkstatus_callback - Link up/down notification
*/
@@ -751,6 +757,22 @@ void netvsc_linkstatus_callback(struct net_device *net,
schedule_delayed_work(&ndev_ctx->dwork, 0);
}
+static void netvsc_xdp_xmit(struct sk_buff *skb, struct net_device *ndev)
+{
+ int rc;
+
+ skb->queue_mapping = skb_get_rx_queue(skb);
+ __skb_push(skb, ETH_HLEN);
+
+ rc = netvsc_xmit(skb, ndev, true);
+
+ if (dev_xmit_complete(rc))
+ return;
+
+ dev_kfree_skb_any(skb);
+ ndev->stats.tx_dropped++;
+}
+
static void netvsc_comp_ipcsum(struct sk_buff *skb)
{
struct iphdr *iph = (struct iphdr *)skb->data;
@@ -760,7 +782,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,
+ struct xdp_buff *xdp)
{
struct napi_struct *napi = &nvchan->napi;
const struct ndis_pkt_8021q_info *vlan = nvchan->rsc.vlan;
@@ -768,18 +791,37 @@ static struct sk_buff *netvsc_alloc_recv_skb(struct net_device *net,
nvchan->rsc.csum_info;
const u32 *hash_info = nvchan->rsc.hash_info;
struct sk_buff *skb;
+ void *xbuf = xdp->data_hard_start;
int i;
- skb = napi_alloc_skb(napi, nvchan->rsc.pktlen);
- if (!skb)
- return skb;
+ if (xbuf) {
+ unsigned int hdroom = xdp->data - xdp->data_hard_start;
+ unsigned int xlen = xdp->data_end - xdp->data;
+ unsigned int frag_size = netvsc_xdp_fraglen(hdroom + xlen);
- /*
- * 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(xbuf, frag_size);
+
+ if (!skb) {
+ __free_page(virt_to_page(xbuf));
+ return NULL;
+ }
+
+ skb_reserve(skb, hdroom);
+ skb_put(skb, xlen);
+ 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);
@@ -829,13 +871,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;
+ struct xdp_buff xdp;
+ u32 act;
if (net->reg_state != NETREG_REGISTERED)
return NVSP_STAT_FAIL;
+ act = netvsc_run_xdp(net, nvchan, &xdp);
+
+ if (act != XDP_PASS && act != XDP_TX) {
+ 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, &xdp);
if (unlikely(!skb)) {
++net_device_ctx->eth_stats.rx_no_memory;
@@ -849,7 +903,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;
@@ -860,6 +913,11 @@ int netvsc_recv_callback(struct net_device *net,
++rx_stats->multicast;
u64_stats_update_end(&rx_stats->syncp);
+ if (act == XDP_TX) {
+ netvsc_xdp_xmit(skb, net);
+ return NVSP_STAT_SUCCESS;
+ }
+
napi_gro_receive(&nvchan->napi, skb);
return NVSP_STAT_SUCCESS;
}
@@ -886,10 +944,11 @@ static void netvsc_get_channels(struct net_device *net,
/* Alloc struct netvsc_device_info, and initialize it from either existing
* struct netvsc_device, or from default values.
*/
-static struct netvsc_device_info *netvsc_devinfo_get
- (struct netvsc_device *nvdev)
+static
+struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
{
struct netvsc_device_info *dev_info;
+ struct bpf_prog *prog;
dev_info = kzalloc(sizeof(*dev_info), GFP_ATOMIC);
@@ -897,6 +956,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;
@@ -905,6 +966,12 @@ 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) {
+ bpf_prog_inc(prog);
+ dev_info->bprog = prog;
+ }
} else {
dev_info->num_chn = VRSS_CHANNEL_DEFAULT;
dev_info->send_sections = NETVSC_DEFAULT_TX;
@@ -916,6 +983,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)
{
@@ -927,6 +1005,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, NULL, nvdev);
+
/* If device was up (receiving) then shutdown */
if (netif_running(ndev)) {
netvsc_tx_disable(nvdev, ndev);
@@ -960,7 +1040,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))
@@ -976,6 +1057,13 @@ static int netvsc_attach(struct net_device *ndev,
}
}
+ prog = dev_info->bprog;
+ if (prog) {
+ ret = netvsc_xdp_set(ndev, prog, NULL, nvdev);
+ if (ret)
+ goto err1;
+ }
+
/* In any case device is now ready */
netif_device_attach(ndev);
@@ -985,7 +1073,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)
@@ -994,9 +1082,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;
@@ -1046,7 +1135,7 @@ static int netvsc_set_channels(struct net_device *net,
}
out:
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
return ret;
}
@@ -1153,7 +1242,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;
}
@@ -1378,8 +1467,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)
{
@@ -1411,6 +1500,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)
@@ -1439,9 +1529,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(),
@@ -1489,6 +1581,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) {
@@ -1785,10 +1879,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)
{
@@ -1875,12 +1986,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,
};
/*
@@ -2167,6 +2280,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;
@@ -2211,6 +2325,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;
}
@@ -2252,6 +2369,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);
@@ -2363,14 +2482,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:
@@ -2398,8 +2517,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, NULL, nvdev);
+ }
/*
* Call to the vsc driver to let it know that the device is being
@@ -2472,11 +2593,11 @@ static int netvsc_resume(struct hv_device *dev)
ret = netvsc_attach(net, device_info);
- rtnl_unlock();
-
- kfree(device_info);
+ netvsc_devinfo_put(device_info);
net_device_ctx->saved_netvsc_dev_info = NULL;
+ rtnl_unlock();
+
return ret;
}
static const struct hv_vmbus_device_id id_table[] = {
diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
index e66d77d..b81ceba 100644
--- a/drivers/net/hyperv/rndis_filter.c
+++ b/drivers/net/hyperv/rndis_filter.c
@@ -235,7 +235,7 @@ static int rndis_filter_send_request(struct rndis_device *dev,
trace_rndis_send(dev->ndev, 0, &req->request_msg);
rcu_read_lock_bh();
- ret = netvsc_send(dev->ndev, packet, NULL, pb, NULL);
+ ret = netvsc_send(dev->ndev, packet, NULL, pb, NULL, false);
rcu_read_unlock_bh();
return ret;
--
1.8.3.1
^ permalink raw reply related
* [PATCH V4,net-next, 2/2] hv_netvsc: Update document for XDP support
From: Haiyang Zhang @ 2020-01-23 21:52 UTC (permalink / raw)
To: sashal, linux-hyperv, netdev
Cc: haiyangz, kys, sthemmin, olaf, vkuznets, davem, linux-kernel
In-Reply-To: <1579816355-6933-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 | 21 +++++++++++++++++++++
1 file changed, 21 insertions(+)
diff --git a/Documentation/networking/device_drivers/microsoft/netvsc.txt b/Documentation/networking/device_drivers/microsoft/netvsc.txt
index 3bfa635..cd63556 100644
--- a/Documentation/networking/device_drivers/microsoft/netvsc.txt
+++ b/Documentation/networking/device_drivers/microsoft/netvsc.txt
@@ -82,3 +82,24 @@ 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.
+
+ Setting / unsetting XDP program on synthetic NIC (netvsc) propagates to
+ VF NIC automatically. Setting / unsetting XDP program on VF NIC directly
+ is not recommended, also not propagated to synthetic NIC, and may be
+ overwritten by setting of synthetic NIC.
+
+ XDP program cannot run with LRO (RSC) enabled, so you need to disable LRO
+ before running XDP:
+ ethtool -K eth0 lro off
+
+ XDP_REDIRECT action is not yet supported.
--
1.8.3.1
^ permalink raw reply related
* Re: [RFC 0/6] vDSO support for Hyper-V guest on ARM64
From: Boqun Feng @ 2020-01-24 6:32 UTC (permalink / raw)
To: Vincenzo Frascino
Cc: linux-hyperv, linux-arm-kernel, linux-kernel, Sasha Levin,
Stephen Hemminger, Catalin Marinas, Haiyang Zhang, Michael Kelley,
Stefano Stabellini, xen-devel, Thomas Gleixner, K. Y. Srinivasan,
Will Deacon
In-Reply-To: <ef6cb7ba-b448-cfa5-abbb-1d99d1396ce5@arm.com>
Hi Vincenzo,
On Thu, Jan 23, 2020 at 10:48:07AM +0000, Vincenzo Frascino wrote:
> Hi Boqun Feng,
>
> sorry for the late reply.
>
That's OK, thanks for your review ;-)
> On 16/12/2019 00:19, Boqun Feng wrote:
> > Hi,
> >
> > This is the RFC patchset for vDSO support in ARM64 Hyper-V guest. To
> > test it, Michael's ARM64 support patchset:
> >
> > https://lore.kernel.org/linux-arm-kernel/1570129355-16005-1-git-send-email-mikelley@microsoft.com/
> >
> > is needed.
> >
> > Similar as x86, Hyper-V on ARM64 use a TSC page for guests to read
> > the virtualized hardware timer, this TSC page is read-only for the
> > guests, so could be used for vDSO data page. And the vDSO (userspace)
> > code could use the same code for timer reading as kernel, since
> > they read the same TSC page.
> >
>
> I had a look to your patches and overall, I could not understand why we can't
> use the arch_timer to do the same things you are doing with the one you
> introduced in this series. What confuses me is that KVM works just fine with the
> arch_timer which was designed with virtualization in mind. Why do we need
> another one? Could you please explain?
>
Please note that the guest VM on Hyper-V for ARM64 doesn't use
arch_timer as the clocksource. See:
https://lore.kernel.org/linux-arm-kernel/1570129355-16005-7-git-send-email-mikelley@microsoft.com/
, ACPI_SIG_GTDT is used for setting up Hyper-V synthetic clocksource
and other initialization work.
So just to be clear, your suggestion is
1) Hyper-V guest on ARM64 should use arch_timer as clocksource and vDSO
will just work.
or
2) Even though arch_timer is not used as the clocksource, we can still
use it for vDSO.
?
Regards,
Boqun
> > This patchset therefore extends ARM64's __vsdo_init() to allow multiple
> > data pages and introduces the vclock_mode concept similar to x86 to
> > allow different platforms (bare-metal, Hyper-V, etc.) to switch to
> > different __arch_get_hw_counter() implementations. The rest of this
> > patchset does the necessary setup for Hyper-V guests: mapping tsc page,
> > enabling userspace to read cntvct, etc. to enable vDSO.
> >
> > This patchset consists of 6 patches:
> >
> > patch #1 allows hv_get_raw_timer() definition to be overridden for
> > userspace and kernel to share the same hv_read_tsc_page() definition.
> >
> > patch #2 extends ARM64 to support multiple vDSO data pages.
> >
> > patch #3 introduces vclock_mode similiar to x86 to allow different
> > __arch_get_hw_counter() implementations for different clocksources.
> >
> > patch #4 maps Hyper-V TSC page into vDSO data page.
> >
> > patch #5 allows userspace to read cntvct, so that userspace can
> > efficiently read the clocksource.
> >
> > patch #6 enables the vDSO for ARM64 Hyper-V guest.
> >
> > The whole patchset is based on v5.5-rc1 plus Michael's ARM64 support
> > patchset, and I've done a few tests with:
> >
> > https://github.com/nlynch-mentor/vdsotest
> >
> > Comments and suggestions are welcome!
> >
> > Regards,
> > Boqun
> >
> > _______________________________________________
> > linux-arm-kernel mailing list
> > linux-arm-kernel@lists.infradead.org
> > http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
> >
>
> --
> Regards,
> Vincenzo
^ permalink raw reply
* Re: [RFC 0/6] vDSO support for Hyper-V guest on ARM64
From: Vincenzo Frascino @ 2020-01-24 10:24 UTC (permalink / raw)
To: Boqun Feng
Cc: linux-hyperv, linux-arm-kernel, linux-kernel, Sasha Levin,
Stephen Hemminger, Catalin Marinas, Haiyang Zhang, Michael Kelley,
Stefano Stabellini, xen-devel, Thomas Gleixner, K. Y. Srinivasan,
Will Deacon
In-Reply-To: <20200124063215.GA93938@debian-boqun.qqnc3lrjykvubdpftowmye0fmh.lx.internal.cloudapp.net>
[-- Attachment #1: Type: text/plain, Size: 1287 bytes --]
Hi Boqun Feng,
On 24/01/2020 06:32, Boqun Feng wrote:
> Hi Vincenzo,
>
[...]
>>
>> I had a look to your patches and overall, I could not understand why we can't
>> use the arch_timer to do the same things you are doing with the one you
>> introduced in this series. What confuses me is that KVM works just fine with the
>> arch_timer which was designed with virtualization in mind. Why do we need
>> another one? Could you please explain?
>>
>
> Please note that the guest VM on Hyper-V for ARM64 doesn't use
> arch_timer as the clocksource. See:
>
> https://lore.kernel.org/linux-arm-kernel/1570129355-16005-7-git-send-email-mikelley@microsoft.com/
>
> , ACPI_SIG_GTDT is used for setting up Hyper-V synthetic clocksource
> and other initialization work.
>
I had a look a look at it and my question stands, why do we need another timer
on arm64?
> So just to be clear, your suggestion is
>
> 1) Hyper-V guest on ARM64 should use arch_timer as clocksource and vDSO
> will just work.
>
> or
>
> 2) Even though arch_timer is not used as the clocksource, we can still
> use it for vDSO.
>
> ?
>
Option #1 would be the preferred solution, unless there is a good reason against.
> Regards,
> Boqun
>
--
Regards,
Vincenzo
[-- Attachment #2: pEpkey.asc --]
[-- Type: application/pgp-keys, Size: 14291 bytes --]
^ permalink raw reply
* RE: [PATCH v6][RESEND] x86/hyperv: Suspend/resume the hypercall page for hibernation
From: Dexuan Cui @ 2020-01-25 1:44 UTC (permalink / raw)
To: arnd@arndb.de, bp@alien8.de, daniel.lezcano@linaro.org,
Haiyang Zhang, hpa@zytor.com, KY Srinivasan,
linux-hyperv@vger.kernel.org, linux-kernel@vger.kernel.org,
mingo@redhat.com, sashal@kernel.org, Stephen Hemminger,
tglx@linutronix.de, x86@kernel.org, Michael Kelley, Sasha Levin,
vkuznets
Cc: linux-arch@vger.kernel.org
In-Reply-To: <1578350559-130275-1-git-send-email-decui@microsoft.com>
> From: Dexuan Cui <decui@microsoft.com>
> Sent: Monday, January 6, 2020 2:43 PM
>
> This is needed for hibernation, e.g. when we resume the old kernel, we need
> to disable the "current" kernel's hypercall page and then resume the old
> kernel's.
>
> Signed-off-by: Dexuan Cui <decui@microsoft.com>
> Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> ---
>
> This is a RESEND of https://lkml.org/lkml/2019/11/20/68
>
> Please review.
>
> If it looks good, can you please pick it up through the tip.git tree?
>
> arch/x86/hyperv/hv_init.c | 48
> +++++++++++++++++++++++++++++++++++++++
> 1 file changed, 48 insertions(+)
Hi, Vitaly and x86 maintainers,
Can you please take a look at this patch?
Thanks,
-- Dexuan
^ permalink raw reply
* RE: [PATCH][RESEND] Input: hyperv-keyboard: Add the support of hibernation
From: Dexuan Cui @ 2020-01-25 1:59 UTC (permalink / raw)
To: Sasha Levin, dmitry.torokhov@gmail.com
Cc: KY Srinivasan, Haiyang Zhang, Stephen Hemminger,
linux-hyperv@vger.kernel.org, linux-input@vger.kernel.org,
linux-kernel@vger.kernel.org, Michael Kelley, Sasha Levin
In-Reply-To: <KU1P153MB01505A56BACA9DA4AF0E7BABBF370@KU1P153MB0150.APCP153.PROD.OUTLOOK.COM>
> From: linux-hyperv-owner@vger.kernel.org
> <linux-hyperv-owner@vger.kernel.org> On Behalf Of Dexuan Cui
> Sent: Wednesday, January 15, 2020 11:53 AM
> > ...
> > On Mon, Jan 06, 2020 at 02:40:44PM -0800, Dexuan Cui wrote:
> > >Add suspend() and resume() functions so the Hyper-V virtual keyboard
> > >can participate in VM hibernation.
> > >
> > >Note that the keyboard is a "wakeup" device that could abort an in-progress
> > >hibernation if there is keyboard event. No attempt is made to suppress
> > this
> > >behavior. If desired, a sysadmin can disable the keyboard as a wakeup
> > device
> > >using standard mechanisms such as:
> > >
> > >echo disabled >
> > /sys/bus/vmbus/drivers/hyperv_keyboard/XXX/power/wakeup
> > >(where XXX is the device's GUID)
> > >
> > >Reviewed-by: Michael Kelley <mikelley@microsoft.com>
> > >Signed-off-by: Dexuan Cui <decui@microsoft.com>
> > >---
> > >
> > >This is a RESEND of https://lkml.org/lkml/2019/11/24/115 .
> >
> > >Please review.
> > >
> > >If it looks good, Sasha Levin, can you please pick it up via the
> > >hyperv/linux.git tree, as you did last time for this driver?
> >
> > This will need an ack from the input driver maintainers, unless they
> > want to give a blanket ack to this type of patches.
> >
> > Thanks,
> > Sasha
>
> Hi Dmitry,
> May I have your Acked-by for this patch?
>
> Thanks
> -- Dexuan
It looks Dmitry may be on leave and there is no ACK from any input driver
maintainers. :-(
Sasha, for this patch, I would suggest it may as well go directly through the
Hyper-V tree because
1. It's really small and it's a pure Hyper-V change.
2. This is for a new functionality and it can not break any existing code.
3. I'm pretty sure it can not cause any merge conflict or build failure.
It's still wonderful to get an Ack or some comment from the input driver
maintainer(s), so maybe let's wait more time (e.g. a week?) and then I
suggust we proceed.
Thanks,
-- Dexuan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox