* Re: [PATCH bpf-next] selftests/bpf: fix a typo in map in map test
From: Daniel Borkmann @ 2018-08-03 8:17 UTC (permalink / raw)
To: Roman Gushchin, netdev
Cc: linux-kernel, kernel-team, Martin KaFai Lau, Arthur Fabre,
Alexei Starovoitov
In-Reply-To: <20180802224710.29662-1-guro@fb.com>
On 08/03/2018 12:47 AM, Roman Gushchin wrote:
> Commit fbeb1603bf4e ("bpf: verifier: MOV64 don't mark dst reg unbounded")
> revealed a typo in commit fb30d4b71214 ("bpf: Add tests for map-in-map"):
> BPF_MOV64_REG(BPF_REG_0, 0) was used instead of
> BPF_MOV64_IMM(BPF_REG_0, 0).
>
> I've noticed the problem by running bpf kselftests.
>
> Fixes: fb30d4b71214 ("bpf: Add tests for map-in-map")
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Martin KaFai Lau <kafai@fb.com>
> Cc: Arthur Fabre <afabre@cloudflare.com>
> Cc: Daniel Borkmann <daniel@iogearbox.net>
> Cc: Alexei Starovoitov <ast@kernel.org>
Applied to bpf-next, thanks Roman!
^ permalink raw reply
* [Patch net] ipv6: fix double refcount of fib6_metrics
From: Cong Wang @ 2018-08-03 6:20 UTC (permalink / raw)
To: netdev; +Cc: Cong Wang, Sabrina Dubroca, David Ahern
All the callers of ip6_rt_copy_init()/rt6_set_from() hold refcnt
of the "from" fib6_info, so there is no need to hold fib6_metrics
refcnt again, because fib6_metrics refcnt is only released when
fib6_info is gone, that is, they have the same life time, so the
whole fib6_metrics refcnt can be removed actually.
This fixes a kmemleak warning reported by Sabrina.
Fixes: 93531c674315 ("net/ipv6: separate handling of FIB entries from dst based routes")
Reported-by: Sabrina Dubroca <sd@queasysnail.net>
Cc: Sabrina Dubroca <sd@queasysnail.net>
Cc: David Ahern <dsahern@gmail.com>
Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com>
---
net/ipv6/route.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/net/ipv6/route.c b/net/ipv6/route.c
index ec18b3ce8b6d..7208c16302f6 100644
--- a/net/ipv6/route.c
+++ b/net/ipv6/route.c
@@ -978,10 +978,6 @@ static void rt6_set_from(struct rt6_info *rt, struct fib6_info *from)
rt->rt6i_flags &= ~RTF_EXPIRES;
rcu_assign_pointer(rt->from, from);
dst_init_metrics(&rt->dst, from->fib6_metrics->metrics, true);
- if (from->fib6_metrics != &dst_default_metrics) {
- rt->dst._metrics |= DST_METRICS_REFCOUNTED;
- refcount_inc(&from->fib6_metrics->refcnt);
- }
}
/* Caller must already hold reference to @ort */
--
2.14.4
^ permalink raw reply related
* Re: [PATCH v7 bpf-next 00/14] bpf: cgroup local storage
From: Daniel Borkmann @ 2018-08-03 8:14 UTC (permalink / raw)
To: Roman Gushchin, netdev
Cc: linux-kernel, kernel-team, Alexei Starovoitov, Martin KaFai Lau
In-Reply-To: <20180802212730.18579-1-guro@fb.com>
On 08/02/2018 11:27 PM, Roman Gushchin wrote:
> This patchset implements cgroup local storage for bpf programs.
> The main idea is to provide a fast accessible memory for storing
> various per-cgroup data, e.g. number of transmitted packets.
>
> Cgroup local storage looks as a special type of map for userspace,
> and is accessible using generic bpf maps API for reading and
> updating of the data. The (cgroup inode id, attachment type) pair
> is used as a map key.
>
> A user can't create new entries or destroy existing entries;
> it happens automatically when a user attaches/detaches a bpf program
> to a cgroup.
>
> From a bpf program's point of view, cgroup storage is accessible
> without lookup using the special get_local_storage() helper function.
> It takes a map fd as an argument. It always returns a valid pointer
> to the corresponding memory area.
>
> To implement such a lookup-free access a pointer to the cgroup
> storage is saved for an attachment of a bpf program to a cgroup,
> if required by the program. Before running the program, it's saved
> in a special global per-cpu variable, which is accessible from the
> get_local_storage() helper.
>
> This patchset implement only cgroup local storage, however the API
> is intentionally made extensible to support other local storage types
> further: e.g. thread local storage, socket local storage, etc.
>
> v7->v6:
> - fixed a use-after-free bug, caused by not clearing
> prog->aux->cgroup_storage pointer after releasing the map
>
> v6->v5:
> - fixed an error with returning -EINVAL instead of a pointer
>
> v5->v4:
> - fixed an issue in verifier (test that flags == 0 properly)
> - added a corresponding test
> - added a note about synchronization, sync docs to tools/uapi/...
> - switched the cgroup test to use XADD
> - added a check for attr->max_entries to be 0, and atter->max_flags
> to be sane
> - use bpf_uncharge_memlock() in bpf_uncharge_memlock()
> - rebased to bpf-next
>
> v4->v3:
> - fixed a leak in cgroup attachment code (discovered by Daniel)
> - cgroup storage map will be released if the corresponding
> bpf program failed to load by any reason
> - introduced bpf_uncharge_memlock() helper
>
> v3->v2:
> - fixed more build and sparse issues
> - rebased to bpf-next
>
> v2->v1:
> - fixed build issues
> - removed explicit rlimit calls in patch 14
> - rebased to bpf-next
>
> Signed-off-by: Roman Gushchin <guro@fb.com>
> Cc: Alexei Starovoitov <ast@kernel.org>
> Cc: Martin KaFai Lau <kafai@fb.com>
>
> Roman Gushchin (14):
> bpf: add ability to charge bpf maps memory dynamically
> bpf: introduce cgroup storage maps
> bpf: pass a pointer to a cgroup storage using pcpu variable
> bpf: allocate cgroup storage entries on attaching bpf programs
> bpf: extend bpf_prog_array to store pointers to the cgroup storage
> bpf/verifier: introduce BPF_PTR_TO_MAP_VALUE
> bpf: don't allow create maps of cgroup local storages
> bpf: introduce the bpf_get_local_storage() helper function
> bpf: sync bpf.h to tools/
> bpftool: add support for CGROUP_STORAGE maps
> bpf/test_run: support cgroup local storage
> selftests/bpf: add verifier cgroup storage tests
> selftests/bpf: add a cgroup storage test
> samples/bpf: extend test_cgrp2_attach2 test to use cgroup storage
Tossed the old series from bpf-next and replaced with this one which fixes
the syzkaller issue, thanks Roman!
^ permalink raw reply
* [PATCH RFC] net: qca_spi: Fix race condition in spi transfers
From: Stefan Wahren @ 2018-08-03 8:05 UTC (permalink / raw)
To: David S. Miller; +Cc: netdev, linux-kernel, Stefan Wahren
The performance optimization with the prepared spi messages
also introduced a race condition between the kernel thread
and the register dump. This could make spi_sync hang forever.
So revert the optimization completely.
Signed-off-by: Stefan Wahren <stefan.wahren@i2se.com>
Fixes: 291ab06ecf67 ("net: qualcomm: new Ethernet over SPI driver for QCA7000")
---
drivers/net/ethernet/qualcomm/qca_7k.c | 84 ++++++++++++------------
drivers/net/ethernet/qualcomm/qca_spi.c | 110 +++++++++++++++++---------------
drivers/net/ethernet/qualcomm/qca_spi.h | 5 --
3 files changed, 97 insertions(+), 102 deletions(-)
diff --git a/drivers/net/ethernet/qualcomm/qca_7k.c b/drivers/net/ethernet/qualcomm/qca_7k.c
index ffe7a16..e9914b6 100644
--- a/drivers/net/ethernet/qualcomm/qca_7k.c
+++ b/drivers/net/ethernet/qualcomm/qca_7k.c
@@ -43,41 +43,40 @@ qcaspi_spi_error(struct qcaspi *qca)
int
qcaspi_read_register(struct qcaspi *qca, u16 reg, u16 *result)
{
- __be16 rx_data;
__be16 tx_data;
- struct spi_transfer *transfer;
- struct spi_message *msg;
+ struct spi_transfer transfer[2];
+ struct spi_message msg;
int ret;
+ memset(transfer, 0, sizeof(transfer));
+
+ spi_message_init(&msg);
+
tx_data = cpu_to_be16(QCA7K_SPI_READ | QCA7K_SPI_INTERNAL | reg);
+ *result = 0;
+
+ transfer[0].tx_buf = &tx_data;
+ transfer[0].len = QCASPI_CMD_LEN;
+ transfer[1].rx_buf = result;
+ transfer[1].len = QCASPI_CMD_LEN;
+
+ spi_message_add_tail(&transfer[0], &msg);
if (qca->legacy_mode) {
- msg = &qca->spi_msg1;
- transfer = &qca->spi_xfer1;
- transfer->tx_buf = &tx_data;
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- spi_sync(qca->spi_dev, msg);
- } else {
- msg = &qca->spi_msg2;
- transfer = &qca->spi_xfer2[0];
- transfer->tx_buf = &tx_data;
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- transfer = &qca->spi_xfer2[1];
+ spi_sync(qca->spi_dev, &msg);
+ spi_message_init(&msg);
}
- transfer->tx_buf = NULL;
- transfer->rx_buf = &rx_data;
- transfer->len = QCASPI_CMD_LEN;
- ret = spi_sync(qca->spi_dev, msg);
+ spi_message_add_tail(&transfer[1], &msg);
+ ret = spi_sync(qca->spi_dev, &msg);
if (!ret)
- ret = msg->status;
+ ret = msg.status;
- if (ret)
+ if (ret) {
qcaspi_spi_error(qca);
- else
- *result = be16_to_cpu(rx_data);
+ } else {
+ *result = be16_to_cpu(*result);
+ }
return ret;
}
@@ -86,35 +85,32 @@ int
qcaspi_write_register(struct qcaspi *qca, u16 reg, u16 value)
{
__be16 tx_data[2];
- struct spi_transfer *transfer;
- struct spi_message *msg;
+ struct spi_transfer transfer[2];
+ struct spi_message msg;
int ret;
+ memset(&transfer, 0, sizeof(transfer));
+
+ spi_message_init(&msg);
+
tx_data[0] = cpu_to_be16(QCA7K_SPI_WRITE | QCA7K_SPI_INTERNAL | reg);
tx_data[1] = cpu_to_be16(value);
+ transfer[0].tx_buf = &tx_data[0];
+ transfer[0].len = QCASPI_CMD_LEN;
+ transfer[1].tx_buf = &tx_data[1];
+ transfer[1].len = QCASPI_CMD_LEN;
+
+ spi_message_add_tail(&transfer[0], &msg);
if (qca->legacy_mode) {
- msg = &qca->spi_msg1;
- transfer = &qca->spi_xfer1;
- transfer->tx_buf = &tx_data[0];
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- spi_sync(qca->spi_dev, msg);
- } else {
- msg = &qca->spi_msg2;
- transfer = &qca->spi_xfer2[0];
- transfer->tx_buf = &tx_data[0];
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- transfer = &qca->spi_xfer2[1];
+ spi_sync(qca->spi_dev, &msg);
+ spi_message_init(&msg);
}
- transfer->tx_buf = &tx_data[1];
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- ret = spi_sync(qca->spi_dev, msg);
+ spi_message_add_tail(&transfer[1], &msg);
+ ret = spi_sync(qca->spi_dev, &msg);
if (!ret)
- ret = msg->status;
+ ret = msg.status;
if (ret)
qcaspi_spi_error(qca);
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.c b/drivers/net/ethernet/qualcomm/qca_spi.c
index 206f026..66b775d 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.c
+++ b/drivers/net/ethernet/qualcomm/qca_spi.c
@@ -99,22 +99,24 @@ static u32
qcaspi_write_burst(struct qcaspi *qca, u8 *src, u32 len)
{
__be16 cmd;
- struct spi_message *msg = &qca->spi_msg2;
- struct spi_transfer *transfer = &qca->spi_xfer2[0];
+ struct spi_message msg;
+ struct spi_transfer transfer[2];
int ret;
+ memset(&transfer, 0, sizeof(transfer));
+ spi_message_init(&msg);
+
cmd = cpu_to_be16(QCA7K_SPI_WRITE | QCA7K_SPI_EXTERNAL);
- transfer->tx_buf = &cmd;
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- transfer = &qca->spi_xfer2[1];
- transfer->tx_buf = src;
- transfer->rx_buf = NULL;
- transfer->len = len;
+ transfer[0].tx_buf = &cmd;
+ transfer[0].len = QCASPI_CMD_LEN;
+ transfer[1].tx_buf = src;
+ transfer[1].len = len;
- ret = spi_sync(qca->spi_dev, msg);
+ spi_message_add_tail(&transfer[0], &msg);
+ spi_message_add_tail(&transfer[1], &msg);
+ ret = spi_sync(qca->spi_dev, &msg);
- if (ret || (msg->actual_length != QCASPI_CMD_LEN + len)) {
+ if (ret || (msg.actual_length != QCASPI_CMD_LEN + len)) {
qcaspi_spi_error(qca);
return 0;
}
@@ -125,17 +127,20 @@ qcaspi_write_burst(struct qcaspi *qca, u8 *src, u32 len)
static u32
qcaspi_write_legacy(struct qcaspi *qca, u8 *src, u32 len)
{
- struct spi_message *msg = &qca->spi_msg1;
- struct spi_transfer *transfer = &qca->spi_xfer1;
+ struct spi_message msg;
+ struct spi_transfer transfer;
int ret;
- transfer->tx_buf = src;
- transfer->rx_buf = NULL;
- transfer->len = len;
+ memset(&transfer, 0, sizeof(transfer));
+ spi_message_init(&msg);
+
+ transfer.tx_buf = src;
+ transfer.len = len;
- ret = spi_sync(qca->spi_dev, msg);
+ spi_message_add_tail(&transfer, &msg);
+ ret = spi_sync(qca->spi_dev, &msg);
- if (ret || (msg->actual_length != len)) {
+ if (ret || (msg.actual_length != len)) {
qcaspi_spi_error(qca);
return 0;
}
@@ -146,23 +151,25 @@ qcaspi_write_legacy(struct qcaspi *qca, u8 *src, u32 len)
static u32
qcaspi_read_burst(struct qcaspi *qca, u8 *dst, u32 len)
{
- struct spi_message *msg = &qca->spi_msg2;
+ struct spi_message msg;
__be16 cmd;
- struct spi_transfer *transfer = &qca->spi_xfer2[0];
+ struct spi_transfer transfer[2];
int ret;
+ memset(&transfer, 0, sizeof(transfer));
+ spi_message_init(&msg);
+
cmd = cpu_to_be16(QCA7K_SPI_READ | QCA7K_SPI_EXTERNAL);
- transfer->tx_buf = &cmd;
- transfer->rx_buf = NULL;
- transfer->len = QCASPI_CMD_LEN;
- transfer = &qca->spi_xfer2[1];
- transfer->tx_buf = NULL;
- transfer->rx_buf = dst;
- transfer->len = len;
+ transfer[0].tx_buf = &cmd;
+ transfer[0].len = QCASPI_CMD_LEN;
+ transfer[1].rx_buf = dst;
+ transfer[1].len = len;
- ret = spi_sync(qca->spi_dev, msg);
+ spi_message_add_tail(&transfer[0], &msg);
+ spi_message_add_tail(&transfer[1], &msg);
+ ret = spi_sync(qca->spi_dev, &msg);
- if (ret || (msg->actual_length != QCASPI_CMD_LEN + len)) {
+ if (ret || (msg.actual_length != QCASPI_CMD_LEN + len)) {
qcaspi_spi_error(qca);
return 0;
}
@@ -173,17 +180,20 @@ qcaspi_read_burst(struct qcaspi *qca, u8 *dst, u32 len)
static u32
qcaspi_read_legacy(struct qcaspi *qca, u8 *dst, u32 len)
{
- struct spi_message *msg = &qca->spi_msg1;
- struct spi_transfer *transfer = &qca->spi_xfer1;
+ struct spi_message msg;
+ struct spi_transfer transfer;
int ret;
- transfer->tx_buf = NULL;
- transfer->rx_buf = dst;
- transfer->len = len;
+ memset(&transfer, 0, sizeof(transfer));
+ spi_message_init(&msg);
- ret = spi_sync(qca->spi_dev, msg);
+ transfer.rx_buf = dst;
+ transfer.len = len;
- if (ret || (msg->actual_length != len)) {
+ spi_message_add_tail(&transfer, &msg);
+ ret = spi_sync(qca->spi_dev, &msg);
+
+ if (ret || (msg.actual_length != len)) {
qcaspi_spi_error(qca);
return 0;
}
@@ -195,19 +205,23 @@ static int
qcaspi_tx_cmd(struct qcaspi *qca, u16 cmd)
{
__be16 tx_data;
- struct spi_message *msg = &qca->spi_msg1;
- struct spi_transfer *transfer = &qca->spi_xfer1;
+ struct spi_message msg;
+ struct spi_transfer transfer;
int ret;
+ memset(&transfer, 0, sizeof(transfer));
+
+ spi_message_init(&msg);
+
tx_data = cpu_to_be16(cmd);
- transfer->len = sizeof(tx_data);
- transfer->tx_buf = &tx_data;
- transfer->rx_buf = NULL;
+ transfer.len = sizeof(cmd);
+ transfer.tx_buf = &tx_data;
+ spi_message_add_tail(&transfer, &msg);
- ret = spi_sync(qca->spi_dev, msg);
+ ret = spi_sync(qca->spi_dev, &msg);
if (!ret)
- ret = msg->status;
+ ret = msg.status;
if (ret)
qcaspi_spi_error(qca);
@@ -835,16 +849,6 @@ qcaspi_netdev_setup(struct net_device *dev)
qca = netdev_priv(dev);
memset(qca, 0, sizeof(struct qcaspi));
- memset(&qca->spi_xfer1, 0, sizeof(struct spi_transfer));
- memset(&qca->spi_xfer2, 0, sizeof(struct spi_transfer) * 2);
-
- spi_message_init(&qca->spi_msg1);
- spi_message_add_tail(&qca->spi_xfer1, &qca->spi_msg1);
-
- spi_message_init(&qca->spi_msg2);
- spi_message_add_tail(&qca->spi_xfer2[0], &qca->spi_msg2);
- spi_message_add_tail(&qca->spi_xfer2[1], &qca->spi_msg2);
-
memset(&qca->txr, 0, sizeof(qca->txr));
qca->txr.count = TX_RING_MAX_LEN;
}
diff --git a/drivers/net/ethernet/qualcomm/qca_spi.h b/drivers/net/ethernet/qualcomm/qca_spi.h
index fc4beb1..fc0e987 100644
--- a/drivers/net/ethernet/qualcomm/qca_spi.h
+++ b/drivers/net/ethernet/qualcomm/qca_spi.h
@@ -83,11 +83,6 @@ struct qcaspi {
struct tx_ring txr;
struct qcaspi_stats stats;
- struct spi_message spi_msg1;
- struct spi_message spi_msg2;
- struct spi_transfer spi_xfer1;
- struct spi_transfer spi_xfer2[2];
-
u8 *rx_buffer;
u32 buffer_size;
u8 sync;
--
2.7.4
^ permalink raw reply related
* [PATCH] rsi: remove redundant variables bss, wh and temp_flash_content
From: Colin King @ 2018-08-03 7:46 UTC (permalink / raw)
To: Kalle Valo, David S . Miller, Amitkumar Karwar, linux-wireless,
netdev
Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
Variables bss, wh and temp_flash_content are being assigned but are
never used hence they are redundant and can be removed.
Cleans up clang warnings:
warning: variable 'bss' set but not used [-Wunused-but-set-variable]
warning: variable 'wh' set but not used [-Wunused-but-set-variable]
warning: variable 'temp_flash_content' set but not used
[-Wunused-but-set-variable]
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/wireless/rsi/rsi_91x_hal.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/wireless/rsi/rsi_91x_hal.c b/drivers/net/wireless/rsi/rsi_91x_hal.c
index 27e6baf12e90..01edf960ff3c 100644
--- a/drivers/net/wireless/rsi/rsi_91x_hal.c
+++ b/drivers/net/wireless/rsi/rsi_91x_hal.c
@@ -57,7 +57,6 @@ int rsi_prepare_mgmt_desc(struct rsi_common *common, struct sk_buff *skb)
struct ieee80211_vif *vif;
struct rsi_mgmt_desc *mgmt_desc;
struct skb_info *tx_params;
- struct ieee80211_bss_conf *bss = NULL;
struct rsi_xtended_desc *xtend_desc = NULL;
u8 header_size;
u32 dword_align_bytes = 0;
@@ -91,7 +90,6 @@ int rsi_prepare_mgmt_desc(struct rsi_common *common, struct sk_buff *skb)
tx_params->internal_hdr_size = header_size;
memset(&skb->data[0], 0, header_size);
- bss = &vif->bss_conf;
wh = (struct ieee80211_hdr *)&skb->data[header_size];
mgmt_desc = (struct rsi_mgmt_desc *)skb->data;
@@ -148,7 +146,6 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
struct ieee80211_hdr *wh = NULL;
struct ieee80211_tx_info *info;
struct skb_info *tx_params;
- struct ieee80211_bss_conf *bss;
struct rsi_data_desc *data_desc;
struct rsi_xtended_desc *xtend_desc;
u8 ieee80211_size = MIN_802_11_HDR_LEN;
@@ -159,7 +156,6 @@ int rsi_prepare_data_desc(struct rsi_common *common, struct sk_buff *skb)
info = IEEE80211_SKB_CB(skb);
vif = info->control.vif;
- bss = &vif->bss_conf;
tx_params = (struct skb_info *)info->driver_data;
header_size = FRAME_DESC_SZ + sizeof(struct rsi_xtended_desc);
@@ -288,7 +284,6 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
struct ieee80211_tx_info *info;
struct skb_info *tx_params;
struct ieee80211_bss_conf *bss;
- struct ieee80211_hdr *wh;
int status = -EINVAL;
u8 header_size;
@@ -304,7 +299,6 @@ int rsi_send_data_pkt(struct rsi_common *common, struct sk_buff *skb)
bss = &vif->bss_conf;
tx_params = (struct skb_info *)info->driver_data;
header_size = tx_params->internal_hdr_size;
- wh = (struct ieee80211_hdr *)&skb->data[header_size];
if (((vif->type == NL80211_IFTYPE_STATION) ||
(vif->type == NL80211_IFTYPE_P2P_CLIENT)) &&
@@ -747,13 +741,11 @@ static int ping_pong_write(struct rsi_hw *adapter, u8 cmd, u8 *addr, u32 size)
static int auto_fw_upgrade(struct rsi_hw *adapter, u8 *flash_content,
u32 content_size)
{
- u8 cmd, *temp_flash_content;
+ u8 cmd;
u32 temp_content_size, num_flash, index;
u32 flash_start_address;
int status;
- temp_flash_content = flash_content;
-
if (content_size > MAX_FLASH_FILE_SIZE) {
rsi_dbg(ERR_ZONE,
"%s: Flash Content size is more than 400K %u\n",
--
2.17.1
^ permalink raw reply related
* Re: [V9fs-developer] [PATCH] net/9p: avoid request size exceed to the virtqueue number in the zero copy
From: Dominique Martinet @ 2018-08-03 7:32 UTC (permalink / raw)
To: jiangyiwen
Cc: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
Linux Kernel Mailing List, v9fs-developer, netdev
In-Reply-To: <5B63FB4D.1050703@huawei.com>
jiangyiwen wrote on Fri, Aug 03, 2018:
> Unfortunately, when the address(input and response headers) are not
> at page boundary, it will need two extra entry in the zero copy, or
> else it will cause sg array out of bounds.
>
> To avoid the problem, we should subtract two pages for maxsize.
Good catch, that must have been painful to figure.
Given we know how big the headers are (something like 11 or 23 bytes
depending on the op/direction, it's capped by P9_IOHDRSZ at 24),
couldn't we just cheat and not use the start of the buffer if we detect
it's overlapping?
It's probably faster to memcpy a few bytes than to use two pages for the
sg list...
It's definitely ugly though, just taking more margin here is probably
just as good.
I'm going to be picky about English again, sorry, please bear with me.
> Subject: net/9p: avoid request size exceed to the virtqueue number in
> the zero copy
This is >72 characters so a little bit too long, if possible to shorten
it.
I'm also not sure 'exceed' is a noun so I probably wouldn't have
understood this sentence without the rest of the message...
The balance is difficult but it doesn't need to contain too much details
either something like "9p/virtio: reduce transport maxsize" is simple
but probably enough as it describes what is done: someone can look at
the rest of the message for the justification.
> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
> ---
> net/9p/trans_virtio.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 6265d1d..63591b2 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -754,11 +754,12 @@ static void p9_virtio_remove(struct virtio_device *vdev)
> .cancel = p9_virtio_cancel,
> /*
> * We leave one entry for input and one entry for response
> - * headers. We also skip one more entry to accomodate, address
> - * that are not at page boundary, that can result in an extra
> - * page in zero copy.
> + * headers. We also skip three more entrys to accomodate
"entry"'s plural is "entries", this word is in checkpatch's dictionary
as a common typo
> + * (input + response headers + data pages), address
> + * that are not at page boundary, that can result in
> + * an extra page in zero copy.
> */
> - .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3),
> + .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 5),
> .def = 1,
> .owner = THIS_MODULE,
> };
Thanks,
--
Dominique
^ permalink raw reply
* [PATCH net-next] vhost: switch to use new message format
From: Jason Wang @ 2018-08-03 7:04 UTC (permalink / raw)
To: mst, jasowang; +Cc: netdev, linux-kernel, kvm, virtualization
We use to have message like:
struct vhost_msg {
int type;
union {
struct vhost_iotlb_msg iotlb;
__u8 padding[64];
};
};
Unfortunately, there will be a hole of 32bit in 64bit machine because
of the alignment. This leads a different formats between 32bit API and
64bit API. What's more it will break 32bit program running on 64bit
machine.
So fixing this by introducing a new message type with an explicit
32bit reserved field after type like:
struct vhost_msg_v2 {
int type;
__u32 reserved;
union {
struct vhost_iotlb_msg iotlb;
__u8 padding[64];
};
};
We will have a consistent ABI after switching to use this. To enable
this capability, introduce a new ioctl (VHOST_SET_BAKCEND_FEATURE) for
userspace to enable this feature (VHOST_BACKEND_F_IOTLB_V2).
Fixes: 6b1e6cc7855b ("vhost: new device IOTLB API")
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/vhost/net.c | 30 ++++++++++++++++++++
drivers/vhost/vhost.c | 71 ++++++++++++++++++++++++++++++++++------------
drivers/vhost/vhost.h | 11 ++++++-
include/uapi/linux/vhost.h | 18 ++++++++++++
4 files changed, 111 insertions(+), 19 deletions(-)
diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
index 367d802..4e656f8 100644
--- a/drivers/vhost/net.c
+++ b/drivers/vhost/net.c
@@ -78,6 +78,10 @@ enum {
};
enum {
+ VHOST_NET_BACKEND_FEATURES = (1ULL << VHOST_BACKEND_F_IOTLB_MSG_V2)
+};
+
+enum {
VHOST_NET_VQ_RX = 0,
VHOST_NET_VQ_TX = 1,
VHOST_NET_VQ_MAX = 2,
@@ -1399,6 +1403,21 @@ static long vhost_net_reset_owner(struct vhost_net *n)
return err;
}
+static int vhost_net_set_backend_features(struct vhost_net *n, u64 features)
+{
+ int i;
+
+ mutex_lock(&n->dev.mutex);
+ for (i = 0; i < VHOST_NET_VQ_MAX; ++i) {
+ mutex_lock(&n->vqs[i].vq.mutex);
+ n->vqs[i].vq.acked_backend_features = features;
+ mutex_unlock(&n->vqs[i].vq.mutex);
+ }
+ mutex_unlock(&n->dev.mutex);
+
+ return 0;
+}
+
static int vhost_net_set_features(struct vhost_net *n, u64 features)
{
size_t vhost_hlen, sock_hlen, hdr_len;
@@ -1489,6 +1508,17 @@ static long vhost_net_ioctl(struct file *f, unsigned int ioctl,
if (features & ~VHOST_NET_FEATURES)
return -EOPNOTSUPP;
return vhost_net_set_features(n, features);
+ case VHOST_GET_BACKEND_FEATURES:
+ features = VHOST_NET_BACKEND_FEATURES;
+ if (copy_to_user(featurep, &features, sizeof(features)))
+ return -EFAULT;
+ return 0;
+ case VHOST_SET_BACKEND_FEATURES:
+ if (copy_from_user(&features, featurep, sizeof(features)))
+ return -EFAULT;
+ if (features & ~VHOST_NET_BACKEND_FEATURES)
+ return -EOPNOTSUPP;
+ return vhost_net_set_backend_features(n, features);
case VHOST_RESET_OWNER:
return vhost_net_reset_owner(n);
case VHOST_SET_OWNER:
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index a502f1a..6f6c42d 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -315,6 +315,7 @@ static void vhost_vq_reset(struct vhost_dev *dev,
vq->log_addr = -1ull;
vq->private_data = NULL;
vq->acked_features = 0;
+ vq->acked_backend_features = 0;
vq->log_base = NULL;
vq->error_ctx = NULL;
vq->kick = NULL;
@@ -1027,28 +1028,40 @@ static int vhost_process_iotlb_msg(struct vhost_dev *dev,
ssize_t vhost_chr_write_iter(struct vhost_dev *dev,
struct iov_iter *from)
{
- struct vhost_msg_node node;
- unsigned size = sizeof(struct vhost_msg);
- size_t ret;
- int err;
+ struct vhost_iotlb_msg msg;
+ size_t offset;
+ int type, ret;
- if (iov_iter_count(from) < size)
- return 0;
- ret = copy_from_iter(&node.msg, size, from);
- if (ret != size)
+ ret = copy_from_iter(&type, sizeof(type), from);
+ if (ret != sizeof(type))
goto done;
- switch (node.msg.type) {
+ switch (type) {
case VHOST_IOTLB_MSG:
- err = vhost_process_iotlb_msg(dev, &node.msg.iotlb);
- if (err)
- ret = err;
+ /* There maybe a hole after type for V1 message type,
+ * so skip it here.
+ */
+ offset = offsetof(struct vhost_msg, iotlb) - sizeof(int);
+ break;
+ case VHOST_IOTLB_MSG_V2:
+ offset = sizeof(__u32);
break;
default:
ret = -EINVAL;
- break;
+ goto done;
+ }
+
+ iov_iter_advance(from, offset);
+ ret = copy_from_iter(&msg, sizeof(msg), from);
+ if (ret != sizeof(msg))
+ goto done;
+ if (vhost_process_iotlb_msg(dev, &msg)) {
+ ret = -EFAULT;
+ goto done;
}
+ ret = (type == VHOST_IOTLB_MSG) ? sizeof(struct vhost_msg) :
+ sizeof(struct vhost_msg_v2);
done:
return ret;
}
@@ -1107,13 +1120,28 @@ ssize_t vhost_chr_read_iter(struct vhost_dev *dev, struct iov_iter *to,
finish_wait(&dev->wait, &wait);
if (node) {
- ret = copy_to_iter(&node->msg, size, to);
+ struct vhost_iotlb_msg *msg;
+ void *start = &node->msg;
+
+ switch (node->msg.type) {
+ case VHOST_IOTLB_MSG:
+ size = sizeof(node->msg);
+ msg = &node->msg.iotlb;
+ break;
+ case VHOST_IOTLB_MSG_V2:
+ size = sizeof(node->msg_v2);
+ msg = &node->msg_v2.iotlb;
+ break;
+ default:
+ BUG();
+ break;
+ }
- if (ret != size || node->msg.type != VHOST_IOTLB_MISS) {
+ ret = copy_to_iter(start, size, to);
+ if (ret != size || msg->type != VHOST_IOTLB_MISS) {
kfree(node);
return ret;
}
-
vhost_enqueue_msg(dev, &dev->pending_list, node);
}
@@ -1126,12 +1154,19 @@ static int vhost_iotlb_miss(struct vhost_virtqueue *vq, u64 iova, int access)
struct vhost_dev *dev = vq->dev;
struct vhost_msg_node *node;
struct vhost_iotlb_msg *msg;
+ bool v2 = vhost_backend_has_feature(vq, VHOST_BACKEND_F_IOTLB_MSG_V2);
- node = vhost_new_msg(vq, VHOST_IOTLB_MISS);
+ node = vhost_new_msg(vq, v2 ? VHOST_IOTLB_MSG_V2 : VHOST_IOTLB_MSG);
if (!node)
return -ENOMEM;
- msg = &node->msg.iotlb;
+ if (v2) {
+ node->msg_v2.type = VHOST_IOTLB_MSG_V2;
+ msg = &node->msg_v2.iotlb;
+ } else {
+ msg = &node->msg.iotlb;
+ }
+
msg->type = VHOST_IOTLB_MISS;
msg->iova = iova;
msg->perm = access;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index 6c844b9..466ef75 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -132,6 +132,7 @@ struct vhost_virtqueue {
struct vhost_umem *iotlb;
void *private_data;
u64 acked_features;
+ u64 acked_backend_features;
/* Log write descriptors */
void __user *log_base;
struct vhost_log *log;
@@ -147,7 +148,10 @@ struct vhost_virtqueue {
};
struct vhost_msg_node {
- struct vhost_msg msg;
+ union {
+ struct vhost_msg msg;
+ struct vhost_msg_v2 msg_v2;
+ };
struct vhost_virtqueue *vq;
struct list_head node;
};
@@ -238,6 +242,11 @@ static inline bool vhost_has_feature(struct vhost_virtqueue *vq, int bit)
return vq->acked_features & (1ULL << bit);
}
+static inline bool vhost_backend_has_feature(struct vhost_virtqueue *vq, int bit)
+{
+ return vq->acked_backend_features & (1ULL << bit);
+}
+
#ifdef CONFIG_VHOST_CROSS_ENDIAN_LEGACY
static inline bool vhost_is_little_endian(struct vhost_virtqueue *vq)
{
diff --git a/include/uapi/linux/vhost.h b/include/uapi/linux/vhost.h
index c51f8e5..c176e9d 100644
--- a/include/uapi/linux/vhost.h
+++ b/include/uapi/linux/vhost.h
@@ -65,6 +65,7 @@ struct vhost_iotlb_msg {
};
#define VHOST_IOTLB_MSG 0x1
+#define VHOST_IOTLB_MSG_V2 0x2
struct vhost_msg {
int type;
@@ -74,6 +75,15 @@ struct vhost_msg {
};
};
+struct vhost_msg_v2 {
+ int type;
+ __u32 reserved;
+ union {
+ struct vhost_iotlb_msg iotlb;
+ __u8 padding[64];
+ };
+};
+
struct vhost_memory_region {
__u64 guest_phys_addr;
__u64 memory_size; /* bytes */
@@ -160,6 +170,14 @@ struct vhost_memory {
#define VHOST_GET_VRING_BUSYLOOP_TIMEOUT _IOW(VHOST_VIRTIO, 0x24, \
struct vhost_vring_state)
+/* Set or get vhost backend capability */
+
+/* Use message type V2 */
+#define VHOST_BACKEND_F_IOTLB_MSG_V2 0x1
+
+#define VHOST_SET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x25, __u64)
+#define VHOST_GET_BACKEND_FEATURES _IOW(VHOST_VIRTIO, 0x26, __u64)
+
/* VHOST_NET specific defines */
/* Attach virtio net ring to a raw socket, or tap device.
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] net:usb: Use ARRAY_SIZE instead of calculating the array size
From: zhong jiang @ 2018-08-03 6:53 UTC (permalink / raw)
To: woojung.huh, davem, UNGLinuxDriver; +Cc: netdev, linux-kernel
In-Reply-To: <1533279195-50245-1-git-send-email-zhongjiang@huawei.com>
We use ARRAY_SIZE to replace open code sizeof(lan78xx_regs) / sizeof(u32).
It make the code concise.
Signed-off-by: zhong jiang <zhongjiang@huawei.com>
---
drivers/net/usb/lan78xx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/usb/lan78xx.c b/drivers/net/usb/lan78xx.c
index 4662fa0..a9991c5 100644
--- a/drivers/net/usb/lan78xx.c
+++ b/drivers/net/usb/lan78xx.c
@@ -1649,7 +1649,7 @@ static int lan78xx_get_regs_len(struct net_device *netdev)
struct lan78xx_net *dev = netdev_priv(netdev);
/* Read Device/MAC registers */
- for (i = 0; i < (sizeof(lan78xx_regs) / sizeof(u32)); i++)
+ for (i = 0; i < ARRAY_SIZE(lan78xx_regs); i++)
lan78xx_read_reg(dev, lan78xx_regs[i], &data[i]);
if (!netdev->phydev)
--
1.7.12.4
^ permalink raw reply related
* [PATCH 0/2] Use ARRAY_SIZE to replace computing the size
From: zhong jiang @ 2018-08-03 6:53 UTC (permalink / raw)
To: woojung.huh, davem, UNGLinuxDriver; +Cc: netdev, linux-kernel
zhong jiang (2):
net:usb: Use ARRAY_SIZE instead of calculating the array size
include/net/bond_3ad: Simplify the code by using the ARRAY_SIZE
drivers/net/usb/lan78xx.c | 2 +-
include/net/bond_3ad.h | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
--
1.7.12.4
^ permalink raw reply
* [V9fs-developer] [PATCH] net/9p: avoid request size exceed to the virtqueue number in the zero copy
From: jiangyiwen @ 2018-08-03 6:50 UTC (permalink / raw)
To: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
Dominique Martinet
Cc: Linux Kernel Mailing List, v9fs-developer, netdev
Unfortunately, when the address(input and response headers) are not
at page boundary, it will need two extra entry in the zero copy, or
else it will cause sg array out of bounds.
To avoid the problem, we should subtract two pages for maxsize.
Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
---
net/9p/trans_virtio.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
index 6265d1d..63591b2 100644
--- a/net/9p/trans_virtio.c
+++ b/net/9p/trans_virtio.c
@@ -754,11 +754,12 @@ static void p9_virtio_remove(struct virtio_device *vdev)
.cancel = p9_virtio_cancel,
/*
* We leave one entry for input and one entry for response
- * headers. We also skip one more entry to accomodate, address
- * that are not at page boundary, that can result in an extra
- * page in zero copy.
+ * headers. We also skip three more entrys to accomodate
+ * (input + response headers + data pages), address
+ * that are not at page boundary, that can result in
+ * an extra page in zero copy.
*/
- .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 3),
+ .maxsize = PAGE_SIZE * (VIRTQUEUE_NUM - 5),
.def = 1,
.owner = THIS_MODULE,
};
--
1.8.3.1
^ permalink raw reply related
* Re: [V9fs-developer] [PATCH] net/9p: Modify the problem of BUG_ON judgment
From: piaojun @ 2018-08-03 6:18 UTC (permalink / raw)
To: jiangyiwen, Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
Dominique Martinet
Cc: v9fs-developer, Linux Kernel Mailing List, netdev
In-Reply-To: <5B63D5F6.6080109@huawei.com>
LGTM
On 2018/8/3 12:11, jiangyiwen wrote:
> Because the value of limit is VIRTQUEUE_NUM, if index is equal to
> limit, it will cause sg array out of bounds, so correct the judgement
> of BUG_ON.
>
> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
Acked-by: Jun Piao <piaojun@huawei.com>
> ---
> net/9p/trans_virtio.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
> index 6265d1d..08264ba 100644
> --- a/net/9p/trans_virtio.c
> +++ b/net/9p/trans_virtio.c
> @@ -191,7 +191,7 @@ static int pack_sg_list(struct scatterlist *sg, int start,
> s = rest_of_page(data);
> if (s > count)
> s = count;
> - BUG_ON(index > limit);
> + BUG_ON(index >= limit);
> /* Make sure we don't terminate early. */
> sg_unmark_end(&sg[index]);
> sg_set_buf(&sg[index++], data, s);
> @@ -236,6 +236,7 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
> s = PAGE_SIZE - data_off;
> if (s > count)
> s = count;
> + BUG_ON(index >= limit);
> /* Make sure we don't terminate early. */
> sg_unmark_end(&sg[index]);
> sg_set_page(&sg[index++], pdata[i++], s, data_off);
>
^ permalink raw reply
* Re: [V9fs-developer] [PATCH] net/9p: Modify the problem of BUG_ON judgment
From: jiangyiwen @ 2018-08-03 6:17 UTC (permalink / raw)
To: Dominique Martinet
Cc: Eric Van Hensbergen, Ron Minnich, Latchesar Ionkov,
Linux Kernel Mailing List, v9fs-developer, netdev
In-Reply-To: <20180803042308.GA4618@nautica>
On 2018/8/3 12:23, Dominique Martinet wrote:
> jiangyiwen wrote on Fri, Aug 03, 2018:
>> Because the value of limit is VIRTQUEUE_NUM, if index is equal to
>> limit, it will cause sg array out of bounds, so correct the judgement
>> of BUG_ON.
>>
>> Signed-off-by: Yiwen Jiang <jiangyiwen@huawei.com>
>
> I'm not sure you've acted on his mail or if you found this
> independantly, but this was reported by Dan Carpenter on the list in
> June.
> Would you mind if I add a tag for him?
> Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
>
> That aside this looks good, I'll take it.
>
Sorry, I didn't see it before, I tested this problem a few days ago.
It is true that this problem was discovered first by him. Thank you
for adding him.
>> ---
>> net/9p/trans_virtio.c | 3 ++-
>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/net/9p/trans_virtio.c b/net/9p/trans_virtio.c
>> index 6265d1d..08264ba 100644
>> --- a/net/9p/trans_virtio.c
>> +++ b/net/9p/trans_virtio.c
>> @@ -191,7 +191,7 @@ static int pack_sg_list(struct scatterlist *sg, int start,
>> s = rest_of_page(data);
>> if (s > count)
>> s = count;
>> - BUG_ON(index > limit);
>> + BUG_ON(index >= limit);
>> /* Make sure we don't terminate early. */
>> sg_unmark_end(&sg[index]);
>> sg_set_buf(&sg[index++], data, s);
>> @@ -236,6 +236,7 @@ static int p9_virtio_cancel(struct p9_client *client, struct p9_req_t *req)
>> s = PAGE_SIZE - data_off;
>> if (s > count)
>> s = count;
>> + BUG_ON(index >= limit);
>> /* Make sure we don't terminate early. */
>> sg_unmark_end(&sg[index]);
>> sg_set_page(&sg[index++], pdata[i++], s, data_off);
>
^ permalink raw reply
* Re: UDP packets arriving on wrong sockets
From: Andrew Cann @ 2018-08-03 4:19 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev
In-Reply-To: <CAF=yD-KJn8ODsE1cKfRwJ5nFLwnDw-K4s1UhijnxkqX7GtO0OQ@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 641 bytes --]
On Thu, Aug 02, 2018 at 11:21:41AM -0400, Willem de Bruijn wrote:
> You have two sockets bound to the same address and port? Is this using
> SO_REUSEPORT?
Yes, this is using SO_REUSEPORT.
My colleague wrote a python reproducer for this here:
https://gist.github.com/povilasb/53f1c802dbc2aca36a0ffa5b4cb95536
If you run server.py, then client.py, you should see packets arriving at
opposite sockets about half the time. My kernel version is NixOS 4.14.51, he
tested on two different machines - a debian 4.9.0 and fedora 28 4.17.2. We can
reproduce this on all kernels we tested.
Thanks in advance for any help you can give :)
- Andrew
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Tonghao Zhang @ 2018-08-03 4:14 UTC (permalink / raw)
To: makita.toshiaki
Cc: jasowang, mst, virtualization, Linux Kernel Network Developers
In-Reply-To: <540b49d5-4496-8d12-5df2-95e7f73cf5c6@lab.ntt.co.jp>
On Fri, Aug 3, 2018 at 11:40 AM Toshiaki Makita
<makita.toshiaki@lab.ntt.co.jp> wrote:
>
> On 2018/08/03 12:24, Tonghao Zhang wrote:
> > On Fri, Aug 3, 2018 at 11:07 AM Jason Wang <jasowang@redhat.com> wrote:
> >> On 2018年08月03日 10:51, Tonghao Zhang wrote:
> >>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> >>>>> On 2018/08/02 17:18, Jason Wang wrote:
> >>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> >>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> >>>>>>>> + struct vhost_virtqueue *rvq,
> >>>>>>>> + struct vhost_virtqueue *tvq,
> >>>>>>>> + bool rx)
> >>>>>>>> +{
> >>>>>>>> + struct socket *sock = rvq->private_data;
> >>>>>>>> +
> >>>>>>>> + if (rx)
> >>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
> >>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
> >>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
> >>>>>>>> + else {
> >>>>>>>> + /* On tx here, sock has no rx data, so we
> >>>>>>>> + * will wait for sock wakeup for rx, and
> >>>>>>>> + * vhost_enable_notify() is not needed. */
> >>>>>>> A possible case is we do have rx data but guest does not refill the rx
> >>>>>>> queue. In this case we may lose notifications from guest.
> >>>>>> Yes, should consider this case. thanks.
> >>>>> I'm a bit confused. Isn't this covered by the previous
> >>>>> "else if (sock && sk_has_rx_data(...))" block?
> >>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
> >>>> vhost_enble_notify() is false.
> >>>>
> >>>>>>>>> +
> >>>>>>>>> + cpu_relax();
> >>>>>>>>> + }
> >>>>>>>>> +
> >>>>>>>>> + preempt_enable();
> >>>>>>>>> +
> >>>>>>>>> + if (!rx)
> >>>>>>>>> + vhost_net_enable_vq(net, vq);
> >>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
> >>>>>>>> called soon.
> >>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>>>>>> so the network is broken.
> >>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >>>>>> there's no need to enable it since handle_rx() will do this for us.
> >>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
> >>>>> need to enable vq since in that case we have no rx data and handle_rx()
> >>>>> is not scheduled.
> >>>>>
> >>>> Yes.
> >>> So we will use the vhost_has_work() to check whether or not the
> >>> handle_rx is scheduled ?
> >>> If we use the vhost_has_work(), the work in the dev work_list may be
> >>> rx work, or tx work, right ?
> >>
> >> Yes. We can add a boolean to record whether or not we've called
> >> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> >> it was true.
> > so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
> > may not consider the case: work is tx work in the dev work list.
>
> Not sure what you are concerned but what I can say is that we need to
> poll rx work if vhost_has_work() detects tx work in
> vhost_net_rx_peek_head_len() since rx busypoll exits prematurely in that
> case.
In the handle_rx, when we busy poll, the vhost_has_work() return true,
because the tx but not rx work is in the dev work list.
and it is the most case, because tx work may be added to dev work list
anytime(not during busy poll) when guest kick the vhost-net.
so it is not necessary to add it., right ?
> >> So here's the needed changes:
> >>
> >> 1) Split the wakeup disabling to another patch
> >> 2) Squash the vhost_net_busy_poll_try_queue() and
> >> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
> >> duplicated checks.
> >> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
> >> poll_rx, this makes code more readable.
> > OK
> >> Thanks
> >>
> >>>> Thanks
> >>
> >
> >
>
> --
> Toshiaki Makita
>
^ permalink raw reply
* your photos need edit
From: Sam @ 2018-08-02 13:40 UTC (permalink / raw)
To: netdev
As a boutique team, we work personally with our clients to ensure the good
results.
Having over a decade of hands-on experience in photography and retouching,
we have been inspired
to expand our services to the public.
We are team of artists who have excelled in fields such as art,
photography, retouching, graphics and design.
No matter what your field of interest is in, we can learn to work with your
style to give you the best product.
We provide below image editing services:
Clipping path
Image cut out
Image shadow creation
Image masking
Photo retouching
Beauty retouching (skin, face, body retouching)
Glamour retouching
Product retouching
Color correction
and others
We provide testing for our services.
Thanks,
Sam
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 3:44 UTC (permalink / raw)
To: Toshiaki Makita, Tonghao Zhang
Cc: mst, virtualization, Linux Kernel Network Developers
In-Reply-To: <74edb26a-e715-cb49-4e52-62e4b45638d1@lab.ntt.co.jp>
On 2018年08月03日 11:32, Toshiaki Makita wrote:
> On 2018/08/03 12:07, Jason Wang wrote:
>> On 2018年08月02日 17:23, Jason Wang wrote:
>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>>> called soon.
>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>>> so the network is broken.
>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>>> there's no need to enable it since handle_rx() will do this for us.
>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>>> need to enable vq since in that case we have no rx data and handle_rx()
>>>> is not scheduled.
>>>>
>> Rethink about this, looks not. We enable rx wakeups in this case, so if
>> there's pending data, handle_rx() will be schedule after
>> vhost_net_enable_vq().
> You are right, but what I wanted to say is vhost_net_enable_vq() should
> be needed (I was talking about what would happen if
> vhost_net_enable_vq() were removed). Also, I think we should move
> vhost_net_enable_vq() from vhost_net_busy_poll() to this last "else"
> block because this is the case where rx wakeups is required.
> Anyway this part will be refactored so let's see what this code will
> look like in next version.
>
I get your point.
Thanks
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 3:43 UTC (permalink / raw)
To: Tonghao Zhang
Cc: makita.toshiaki, mst, virtualization,
Linux Kernel Network Developers
In-Reply-To: <CAMDZJNWZ+WH3JhCy81h1VSka7-z2zF1Tw-EnLTr26J_5SkQZTg@mail.gmail.com>
On 2018年08月03日 11:24, Tonghao Zhang wrote:
> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>>>
>>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>>>> + struct vhost_virtqueue *rvq,
>>>>>>>> + struct vhost_virtqueue *tvq,
>>>>>>>> + bool rx)
>>>>>>>> +{
>>>>>>>> + struct socket *sock = rvq->private_data;
>>>>>>>> +
>>>>>>>> + if (rx)
>>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>>>> + else {
>>>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>>>> + * will wait for sock wakeup for rx, and
>>>>>>>> + * vhost_enable_notify() is not needed. */
>>>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>>>> queue. In this case we may lose notifications from guest.
>>>>>> Yes, should consider this case. thanks.
>>>>> I'm a bit confused. Isn't this covered by the previous
>>>>> "else if (sock && sk_has_rx_data(...))" block?
>>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>>>> vhost_enble_notify() is false.
>>>>
>>>>>>>>> +
>>>>>>>>> + cpu_relax();
>>>>>>>>> + }
>>>>>>>>> +
>>>>>>>>> + preempt_enable();
>>>>>>>>> +
>>>>>>>>> + if (!rx)
>>>>>>>>> + vhost_net_enable_vq(net, vq);
>>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>>>> called soon.
>>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>>>> so the network is broken.
>>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>>>> there's no need to enable it since handle_rx() will do this for us.
>>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>>>> need to enable vq since in that case we have no rx data and handle_rx()
>>>>> is not scheduled.
>>>>>
>>>> Yes.
>>> So we will use the vhost_has_work() to check whether or not the
>>> handle_rx is scheduled ?
>>> If we use the vhost_has_work(), the work in the dev work_list may be
>>> rx work, or tx work, right ?
>> Yes. We can add a boolean to record whether or not we've called
>> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
>> it was true.
> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
> may not consider the case: work is tx work in the dev work list.
So two kinds of work, tx kick or tx wakeup.
For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
by not enabling kick if we found something is pending on txq. For tx
wakeup, yes, the commit does not consider it. And that's why we want to
disable tx wakeups during busy polling.
Thanks
>
>> So here's the needed changes:
>>
>> 1) Split the wakeup disabling to another patch
>> 2) Squash the vhost_net_busy_poll_try_queue() and
>> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
>> duplicated checks.
>> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
>> poll_rx, this makes code more readable.
> OK
>> Thanks
>>
>>>> Thanks
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Toshiaki Makita @ 2018-08-03 5:25 UTC (permalink / raw)
To: Jason Wang, Tonghao Zhang
Cc: Linux Kernel Network Developers, virtualization, mst
In-Reply-To: <155c4d23-ca90-b8f7-5a1f-bc54a92e922e@redhat.com>
On 2018/08/03 14:07, Jason Wang wrote:
> On 2018年08月03日 12:04, Tonghao Zhang wrote:
>> On Fri, Aug 3, 2018 at 11:43 AM Jason Wang<jasowang@redhat.com> wrote:
>>>
>>> On 2018年08月03日 11:24, Tonghao Zhang wrote:
>>>> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang<jasowang@redhat.com> wrote:
>>>>> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>>>>>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang<jasowang@redhat.com>
>>>>>> wrote:
>>>>>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>>>>>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>>>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>>>>>>> + struct vhost_virtqueue *rvq,
>>>>>>>>>>> + struct vhost_virtqueue *tvq,
>>>>>>>>>>> + bool rx)
>>>>>>>>>>> +{
>>>>>>>>>>> + struct socket *sock = rvq->private_data;
>>>>>>>>>>> +
>>>>>>>>>>> + if (rx)
>>>>>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>>>>>>> + else {
>>>>>>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>>>>>>> + * will wait for sock wakeup for rx, and
>>>>>>>>>>> + * vhost_enable_notify() is not needed. */
>>>>>>>>>> A possible case is we do have rx data but guest does not
>>>>>>>>>> refill the rx
>>>>>>>>>> queue. In this case we may lose notifications from guest.
>>>>>>>>> Yes, should consider this case. thanks.
>>>>>>>> I'm a bit confused. Isn't this covered by the previous
>>>>>>>> "else if (sock && sk_has_rx_data(...))" block?
>>>>>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>>>>>>> vhost_enble_notify() is false.
>>>>>>>
>>>>>>>>>>>> +
>>>>>>>>>>>> + cpu_relax();
>>>>>>>>>>>> + }
>>>>>>>>>>>> +
>>>>>>>>>>>> + preempt_enable();
>>>>>>>>>>>> +
>>>>>>>>>>>> + if (!rx)
>>>>>>>>>>>> + vhost_net_enable_vq(net, vq);
>>>>>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx()
>>>>>>>>>>> will be
>>>>>>>>>>> called soon.
>>>>>>>>>> If we disable rx virtqueue in handle_tx and don't send packets
>>>>>>>>>> from
>>>>>>>>>> guest anymore(handle_tx is not called), so we can wake up for
>>>>>>>>>> sock rx.
>>>>>>>>>> so the network is broken.
>>>>>>>>> Not sure I understand here. I mean is we schedule work for
>>>>>>>>> handle_rx(),
>>>>>>>>> there's no need to enable it since handle_rx() will do this for
>>>>>>>>> us.
>>>>>>>> Looks like in the last "else" block in
>>>>>>>> vhost_net_busy_poll_check() we
>>>>>>>> need to enable vq since in that case we have no rx data and
>>>>>>>> handle_rx()
>>>>>>>> is not scheduled.
>>>>>>>>
>>>>>>> Yes.
>>>>>> So we will use the vhost_has_work() to check whether or not the
>>>>>> handle_rx is scheduled ?
>>>>>> If we use the vhost_has_work(), the work in the dev work_list may be
>>>>>> rx work, or tx work, right ?
>>>>> Yes. We can add a boolean to record whether or not we've called
>>>>> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
>>>>> it was true.
>>>> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during
>>>> busypoll"
>>>> may not consider the case: work is tx work in the dev work list.
>>> So two kinds of work, tx kick or tx wakeup.
>>>
>>> For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
>>> by not enabling kick if we found something is pending on txq. For tx
>>> wakeup, yes, the commit does not consider it. And that's why we want to
>>> disable tx wakeups during busy polling.
>> And in the handle_rx but not busy polling, the tx can wakeup anytime
>> and the tx work will be added to dev work list. In that case, if we
>> add
>> the rx poll to the queue, it is necessary ？ the commit be294a51a may
>> check whether the rx work is in the dev work list.
>
> I think the point this we don't poll rx during tx at that time. So if rx
> poll is interrupted, we should reschedule handle_rx(). After we poll rx
> on handle_tx(), we can try to optimize this on top.
That's true. We may be able to skip poll_queue in handle_rx/tx after
rx/tx busypolling is unified by this patch set.
--
Toshiaki Makita
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Tonghao Zhang @ 2018-08-03 3:24 UTC (permalink / raw)
To: jasowang
Cc: makita.toshiaki, mst, virtualization,
Linux Kernel Network Developers
In-Reply-To: <fc4dc137-98de-2cee-4f41-e23ab4eb2492@redhat.com>
On Fri, Aug 3, 2018 at 11:07 AM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年08月03日 10:51, Tonghao Zhang wrote:
> > On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >>
> >> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> >>> On 2018/08/02 17:18, Jason Wang wrote:
> >>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> >>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> >>>>>> + struct vhost_virtqueue *rvq,
> >>>>>> + struct vhost_virtqueue *tvq,
> >>>>>> + bool rx)
> >>>>>> +{
> >>>>>> + struct socket *sock = rvq->private_data;
> >>>>>> +
> >>>>>> + if (rx)
> >>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
> >>>>>> + else if (sock && sk_has_rx_data(sock->sk))
> >>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
> >>>>>> + else {
> >>>>>> + /* On tx here, sock has no rx data, so we
> >>>>>> + * will wait for sock wakeup for rx, and
> >>>>>> + * vhost_enable_notify() is not needed. */
> >>>>> A possible case is we do have rx data but guest does not refill the rx
> >>>>> queue. In this case we may lose notifications from guest.
> >>>> Yes, should consider this case. thanks.
> >>> I'm a bit confused. Isn't this covered by the previous
> >>> "else if (sock && sk_has_rx_data(...))" block?
> >> The problem is it does nothing if vhost_vq_avail_empty() is true and
> >> vhost_enble_notify() is false.
> >>
> >>>>>>> +
> >>>>>>> + cpu_relax();
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + preempt_enable();
> >>>>>>> +
> >>>>>>> + if (!rx)
> >>>>>>> + vhost_net_enable_vq(net, vq);
> >>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
> >>>>>> called soon.
> >>>>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>>>> so the network is broken.
> >>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >>>> there's no need to enable it since handle_rx() will do this for us.
> >>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
> >>> need to enable vq since in that case we have no rx data and handle_rx()
> >>> is not scheduled.
> >>>
> >> Yes.
> > So we will use the vhost_has_work() to check whether or not the
> > handle_rx is scheduled ?
> > If we use the vhost_has_work(), the work in the dev work_list may be
> > rx work, or tx work, right ?
>
> Yes. We can add a boolean to record whether or not we've called
> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
> it was true.
so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
may not consider the case: work is tx work in the dev work list.
> So here's the needed changes:
>
> 1) Split the wakeup disabling to another patch
> 2) Squash the vhost_net_busy_poll_try_queue() and
> vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
> duplicated checks.
> 3) If possible, rename the boolean rx in vhost_net_busy_poll() to
> poll_rx, this makes code more readable.
OK
> Thanks
>
> >> Thanks
>
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 5:07 UTC (permalink / raw)
To: Tonghao Zhang; +Cc: Linux Kernel Network Developers, mst, virtualization
In-Reply-To: <CAMDZJNVhmWVEeHbZDa3ooFsFXNTtKSH=ey6izfs8CeakbQLjZQ@mail.gmail.com>
On 2018年08月03日 12:04, Tonghao Zhang wrote:
> On Fri, Aug 3, 2018 at 11:43 AM Jason Wang<jasowang@redhat.com> wrote:
>>
>> On 2018年08月03日 11:24, Tonghao Zhang wrote:
>>> On Fri, Aug 3, 2018 at 11:07 AM Jason Wang<jasowang@redhat.com> wrote:
>>>> On 2018年08月03日 10:51, Tonghao Zhang wrote:
>>>>> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang<jasowang@redhat.com> wrote:
>>>>>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>>>>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>>>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>>>>>> + struct vhost_virtqueue *rvq,
>>>>>>>>>> + struct vhost_virtqueue *tvq,
>>>>>>>>>> + bool rx)
>>>>>>>>>> +{
>>>>>>>>>> + struct socket *sock = rvq->private_data;
>>>>>>>>>> +
>>>>>>>>>> + if (rx)
>>>>>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>>>>>> + else {
>>>>>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>>>>>> + * will wait for sock wakeup for rx, and
>>>>>>>>>> + * vhost_enable_notify() is not needed. */
>>>>>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>>>>>> queue. In this case we may lose notifications from guest.
>>>>>>>> Yes, should consider this case. thanks.
>>>>>>> I'm a bit confused. Isn't this covered by the previous
>>>>>>> "else if (sock && sk_has_rx_data(...))" block?
>>>>>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>>>>>> vhost_enble_notify() is false.
>>>>>>
>>>>>>>>>>> +
>>>>>>>>>>> + cpu_relax();
>>>>>>>>>>> + }
>>>>>>>>>>> +
>>>>>>>>>>> + preempt_enable();
>>>>>>>>>>> +
>>>>>>>>>>> + if (!rx)
>>>>>>>>>>> + vhost_net_enable_vq(net, vq);
>>>>>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>>>>>> called soon.
>>>>>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>>>>>> so the network is broken.
>>>>>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>>>>>> there's no need to enable it since handle_rx() will do this for us.
>>>>>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>>>>>> need to enable vq since in that case we have no rx data and handle_rx()
>>>>>>> is not scheduled.
>>>>>>>
>>>>>> Yes.
>>>>> So we will use the vhost_has_work() to check whether or not the
>>>>> handle_rx is scheduled ?
>>>>> If we use the vhost_has_work(), the work in the dev work_list may be
>>>>> rx work, or tx work, right ?
>>>> Yes. We can add a boolean to record whether or not we've called
>>>> vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
>>>> it was true.
>>> so, the commit be294a51a "vhost_net: Avoid rx queue wake-ups during busypoll"
>>> may not consider the case: work is tx work in the dev work list.
>> So two kinds of work, tx kick or tx wakeup.
>>
>> For tx kick, we check vhost_vq_avail_empty() and avoid unnecessary kicks
>> by not enabling kick if we found something is pending on txq. For tx
>> wakeup, yes, the commit does not consider it. And that's why we want to
>> disable tx wakeups during busy polling.
> And in the handle_rx but not busy polling, the tx can wakeup anytime
> and the tx work will be added to dev work list. In that case, if we
> add
> the rx poll to the queue, it is necessary ？ the commit be294a51a may
> check whether the rx work is in the dev work list.
I think the point this we don't poll rx during tx at that time. So if rx
poll is interrupted, we should reschedule handle_rx(). After we poll rx
on handle_tx(), we can try to optimize this on top.
Thanks
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 3:07 UTC (permalink / raw)
To: Tonghao Zhang
Cc: makita.toshiaki, mst, virtualization,
Linux Kernel Network Developers
In-Reply-To: <CAMDZJNUMBYOVh+pzaykRq23ePqNR7BOA8wP_=i85-PXvwaxuBA@mail.gmail.com>
On 2018年08月03日 10:51, Tonghao Zhang wrote:
> On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>>
>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>> + struct vhost_virtqueue *rvq,
>>>>>> + struct vhost_virtqueue *tvq,
>>>>>> + bool rx)
>>>>>> +{
>>>>>> + struct socket *sock = rvq->private_data;
>>>>>> +
>>>>>> + if (rx)
>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>> + else {
>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>> + * will wait for sock wakeup for rx, and
>>>>>> + * vhost_enable_notify() is not needed. */
>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>> queue. In this case we may lose notifications from guest.
>>>> Yes, should consider this case. thanks.
>>> I'm a bit confused. Isn't this covered by the previous
>>> "else if (sock && sk_has_rx_data(...))" block?
>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>> vhost_enble_notify() is false.
>>
>>>>>>> +
>>>>>>> + cpu_relax();
>>>>>>> + }
>>>>>>> +
>>>>>>> + preempt_enable();
>>>>>>> +
>>>>>>> + if (!rx)
>>>>>>> + vhost_net_enable_vq(net, vq);
>>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>>> called soon.
>>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>>> so the network is broken.
>>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>>> there's no need to enable it since handle_rx() will do this for us.
>>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>>> need to enable vq since in that case we have no rx data and handle_rx()
>>> is not scheduled.
>>>
>> Yes.
> So we will use the vhost_has_work() to check whether or not the
> handle_rx is scheduled ?
> If we use the vhost_has_work(), the work in the dev work_list may be
> rx work, or tx work, right ?
Yes. We can add a boolean to record whether or not we've called
vhost_poll_queue() for rvq. And avoid calling vhost_net_enable_vq() if
it was true.
So here's the needed changes:
1) Split the wakeup disabling to another patch
2) Squash the vhost_net_busy_poll_try_queue() and
vhost_net_busy_poll_check() into vhost_net_busy_poll() and reduce
duplicated checks.
3) If possible, rename the boolean rx in vhost_net_busy_poll() to
poll_rx, this makes code more readable.
Thanks
>> Thanks
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 3:07 UTC (permalink / raw)
To: Toshiaki Makita, Tonghao Zhang
Cc: mst, virtualization, Linux Kernel Network Developers
In-Reply-To: <3272c3b4-a44c-8554-329e-8a5e1a59aafd@redhat.com>
On 2018年08月02日 17:23, Jason Wang wrote:
>>>>>>
>>>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
>>>>> called soon.
>>>> If we disable rx virtqueue in handle_tx and don't send packets from
>>>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
>>>> so the network is broken.
>>> Not sure I understand here. I mean is we schedule work for handle_rx(),
>>> there's no need to enable it since handle_rx() will do this for us.
>> Looks like in the last "else" block in vhost_net_busy_poll_check() we
>> need to enable vq since in that case we have no rx data and handle_rx()
>> is not scheduled.
>>
>
> Yes.
>
> Thanks
Rethink about this, looks not. We enable rx wakeups in this case, so if
there's pending data, handle_rx() will be schedule after
vhost_net_enable_vq().
Thanks
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Tonghao Zhang @ 2018-08-03 2:51 UTC (permalink / raw)
To: jasowang
Cc: makita.toshiaki, mst, virtualization,
Linux Kernel Network Developers
In-Reply-To: <3272c3b4-a44c-8554-329e-8a5e1a59aafd@redhat.com>
On Thu, Aug 2, 2018 at 5:23 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
>
> On 2018年08月02日 16:41, Toshiaki Makita wrote:
> > On 2018/08/02 17:18, Jason Wang wrote:
> >> On 2018年08月01日 17:52, Tonghao Zhang wrote:
> >>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
> >>>> + struct vhost_virtqueue *rvq,
> >>>> + struct vhost_virtqueue *tvq,
> >>>> + bool rx)
> >>>> +{
> >>>> + struct socket *sock = rvq->private_data;
> >>>> +
> >>>> + if (rx)
> >>>> + vhost_net_busy_poll_try_queue(net, tvq);
> >>>> + else if (sock && sk_has_rx_data(sock->sk))
> >>>> + vhost_net_busy_poll_try_queue(net, rvq);
> >>>> + else {
> >>>> + /* On tx here, sock has no rx data, so we
> >>>> + * will wait for sock wakeup for rx, and
> >>>> + * vhost_enable_notify() is not needed. */
> >>> A possible case is we do have rx data but guest does not refill the rx
> >>> queue. In this case we may lose notifications from guest.
> >> Yes, should consider this case. thanks.
> > I'm a bit confused. Isn't this covered by the previous
> > "else if (sock && sk_has_rx_data(...))" block?
>
> The problem is it does nothing if vhost_vq_avail_empty() is true and
> vhost_enble_notify() is false.
>
> >
> >>>>> +
> >>>>> + cpu_relax();
> >>>>> + }
> >>>>> +
> >>>>> + preempt_enable();
> >>>>> +
> >>>>> + if (!rx)
> >>>>> + vhost_net_enable_vq(net, vq);
> >>>> No need to enable rx virtqueue, if we are sure handle_rx() will be
> >>>> called soon.
> >>> If we disable rx virtqueue in handle_tx and don't send packets from
> >>> guest anymore(handle_tx is not called), so we can wake up for sock rx.
> >>> so the network is broken.
> >> Not sure I understand here. I mean is we schedule work for handle_rx(),
> >> there's no need to enable it since handle_rx() will do this for us.
> > Looks like in the last "else" block in vhost_net_busy_poll_check() we
> > need to enable vq since in that case we have no rx data and handle_rx()
> > is not scheduled.
> >
>
> Yes.
So we will use the vhost_has_work() to check whether or not the
handle_rx is scheduled ?
If we use the vhost_has_work(), the work in the dev work_list may be
rx work, or tx work, right ?
> Thanks
^ permalink raw reply
* Re: [PATCH net-next] tools: bpf: fix BTF code added twice to different trees
From: David Miller @ 2018-08-03 2:45 UTC (permalink / raw)
To: jakub.kicinski
Cc: alexei.starovoitov, daniel, kafai, osk, oss-drivers, netdev
In-Reply-To: <20180803023027.32721-1-jakub.kicinski@netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Thu, 2 Aug 2018 19:30:27 -0700
> commit 38d5d3b3d5db ("bpf: Introduce BPF_ANNOTATE_KV_PAIR")
>
> added to the bpf and net trees what
>
> commit 92b57121ca79 ("bpf: btf: export btf types and name by offset from lib")
>
> has already added to bpf-next/net-next, but in slightly different
> location. Remove the duplicates (to fix build of libbpf).
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Aha, you beat me to it.
Applied, thanks!
^ permalink raw reply
* Re: [PATCH net-next v7 3/4] net: vhost: factor out busy polling logic to vhost_net_busy_poll()
From: Jason Wang @ 2018-08-03 2:38 UTC (permalink / raw)
To: Toshiaki Makita, Tonghao Zhang
Cc: mst, virtualization, Linux Kernel Network Developers
In-Reply-To: <e2917cb0-f8dd-90c7-5a44-5333ad1d1ee8@lab.ntt.co.jp>
On 2018年08月02日 17:57, Toshiaki Makita wrote:
> On 2018/08/02 18:23, Jason Wang wrote:
>> On 2018年08月02日 16:41, Toshiaki Makita wrote:
>>> On 2018/08/02 17:18, Jason Wang wrote:
>>>> On 2018年08月01日 17:52, Tonghao Zhang wrote:
>>>>>> +static void vhost_net_busy_poll_check(struct vhost_net *net,
>>>>>> + struct vhost_virtqueue *rvq,
>>>>>> + struct vhost_virtqueue *tvq,
>>>>>> + bool rx)
>>>>>> +{
>>>>>> + struct socket *sock = rvq->private_data;
>>>>>> +
>>>>>> + if (rx)
>>>>>> + vhost_net_busy_poll_try_queue(net, tvq);
>>>>>> + else if (sock && sk_has_rx_data(sock->sk))
>>>>>> + vhost_net_busy_poll_try_queue(net, rvq);
>>>>>> + else {
>>>>>> + /* On tx here, sock has no rx data, so we
>>>>>> + * will wait for sock wakeup for rx, and
>>>>>> + * vhost_enable_notify() is not needed. */
>>>>> A possible case is we do have rx data but guest does not refill the rx
>>>>> queue. In this case we may lose notifications from guest.
>>>> Yes, should consider this case. thanks.
>>> I'm a bit confused. Isn't this covered by the previous
>>> "else if (sock && sk_has_rx_data(...))" block?
>> The problem is it does nothing if vhost_vq_avail_empty() is true and
>> vhost_enble_notify() is false.
> If vhost_enable_notify() is false, guest will eventually kicks vq, no?
>
Yes, you are right.
Thanks
^ 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